Use original instead of immer draft for perf

[GeorchW]

Immer needs to do a lot of bookkeeping to keep track of potential changes. But copyWithStructuralSharing is pure, so all the work is unneeded. In my quick tests, this made a x10 performance difference. Immer docs say that the difference should be more like x2-x3, so maybe the true difference is another in real applications. But I'm sure it's not just a 10% thing :-)

Here's the quick test I've run:

const { produce, original } = require("immer");

const createData = () => JSON.parse(OMITTED); // 100kb of real-world data
const oldData = createData();
const newData = createData();

const v1 = () => produce(draft => copyWithStructuralSharing(draft, newData))(oldData);
const v2 = () => produce(draft => copyWithStructuralSharing(original(draft), newData))(oldData);

// warmup
for (let i = 0; i < 100; i++) {
  v1();
  v2();
}

console.time("v1");
for (let i = 0; i < 100; i++) {
  v1();
}
console.timeEnd("v1");

console.time("v2");
for (let i = 0; i < 100; i++) {
  v2();
}
console.timeEnd("v2");

Results:

v1: 992.444ms
v2: 92.098ms

[codesandbox]

  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

[netlify]

✅ Deploy Preview for redux-starter-kit-docs ready!

Name	Link
🔨 Latest commit	933502c
🔍 Latest deploy log	https://app.netlify.com/sites/redux-starter-kit-docs/deploys/643c6265eb113b000805de2b
😎 Deploy Preview	https://deploy-preview-3270--redux-starter-kit-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 933502c:

Sandbox	Source
Vanilla	Configuration
Vanilla Typescript	Configuration
rsk-github-issues-example	Configuration
@examples-query-react/basic	Configuration
@examples-query-react/advanced	Configuration
@examples-action-listener/counter	Configuration

[markerikson]

Looks like the current change is busted:

  ● skip behaviour › skipping a previously fetched query retains the existing value as `data`, but clears `currentData`

    [Immer] 'original' expects a draft, got: undefined

      at die (../../node_modules/immer/src/utils/errors.ts:53:9)
      at original (../../node_modules/immer/src/utils/common.ts:59:23)
      at node_modules/@reduxjs/toolkit/src/query/core/buildSlice.ts:212:25
      at updateQuerySubstateIfExists (node_modules/@reduxjs/toolkit/src/query/core/buildSlice.ts:52:5)

Try adding an isDraft() check and only calling original if the current value is a draft?

[markerikson]

Also, can you try to add some tests for copyWithStructuralSharing that demonstrate the perf improvements, somehow? (not sure what an appropriate test would be off the top of my head)

[GeorchW]

Sorry for the broken build. It should be fixed now.

I'm not sure if a test would be justified, since the change is pretty small and due to the reasons below. (I'll try to write one if you insist. :-) )

1. The overhead is pretty obvious IMO

The amount of self time that immer takes in the profiler for this bit of code was the motivation for me to change it. Here's a screenshot:

All of these shortened function names are immer-internal functions. (The source maps seem to be broken, but I've inspected the underlying code.) They are even visible in a screenshot for the previous issue that dealt with the performance of this code (#1896).

It's also very obvious when writing code that demonstrates the difference in isolation, as in the original MR text.

But apart from that, it is known that immer has quite a bit of overhead, even the Immer docs mention it and suggest "breaking out" for hot code. For the most part, this difference is negligible, since the amount of data changed by a reducer is tiny (constant per request). But when fetching a big JSON document, it is noticeable (linear).

2. A test would be permanently increasing the test duration significantly

Performance tests need to run for a bit of time, otherwise they might become flaky (JIT needs to be warmed up, a GC may trigger, there's a noisy neighbour on the physical CI host, the OS decides that another process gets more time etc.). This means this test alone could need a couple seconds. We would permanently increase the runtime of our tests by this amount, reducing developer experience and prolonging pipeline runs. Tests for similar changes are usually multiple orders of magnitude less overhead.

3. A test would be difficult to write

(you already mentioned this)

---

Sorry, I got a bit carried away writing this comment.

[markerikson]

Had to add a check for isDraft(subState.data), but this seems to pass now!

[GeorchW]

Oh, thank you so much for revisiting this! I almost forgot it <3