immer
immer copied to clipboard
feat: implement useStrictShallowCopy
Description
Fixes #867
This PR improves the performance when updating the object that contains a large number of entries.
Problem
The current immer takes a long time for creating new object when the draft object has large amount of keys.
From some of the investigation, I found the cause that is getOwnPropertyDescriptors
(here) .
In my computer, the ownKeys
and its iteration don't take a long time but getOwnPropertyDescriptors
takes time near the 10ms.
I can understand that the getOwnPropertyDescriptors
is needed. Because immer should copy object's descriptors such as getter/setter/non-enumerable keys.
But some of the users doesn't need that strictness, I think.
Solution
Add the ability to opt-out of strict shallow copy implementation for performance reasons.
import { useStrictShallowCopy } from 'immer';
useStrictShallowCopy(false);
Result
The following results show the benefit of this patch.
// Iteration count:
// 50
//
// Base object:
// Array(10000)
// .fill(0)
// .map((_, i) => [i, i])
//
// Test case:
// for (let i = 0; i < MAX; i++) {
// produce(baseState, draft => {
// draft[5000]++
// })
// }
# large-obj - mutate large object
immer (proxy) - with setUseStrictShallowCopy: 354ms
immer (proxy) - without setUseStrictShallowCopy: 79ms
immer (es5) - with setUseStrictShallowCopy: 1228ms
immer (es5) - without setUseStrictShallowCopy: 655ms
Deploy request for quizzical-lovelace-dcbd6a pending review.
Visit the deploys page to approve it
Name | Link |
---|---|
Latest commit | f9cdf8e6b468e5b35d37bfcb7c20ecce920a6012 |
I didn't read the code yet, but could you include a rationale in the PR description, what are you updating and why is it faster, what does the API look like, in which cases will it break things, etc?
Pull Request Test Coverage Report for Build 4024919453
Warning: This coverage report may be inaccurate.
We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report. To ensure accuracy in future PRs, please see these guidelines. A quick fix for this PR: rebase it; your next report should be accurate.
- 18 of 19 (94.74%) changed or added relevant lines in 5 files are covered.
- No unchanged relevant lines lost coverage.
- Overall coverage decreased (-0.1%) to 98.168%
Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
---|---|---|---|
src/core/immerClass.ts | 3 | 4 | 75.0% |
<!-- | Total: | 18 | 19 |
Totals | |
---|---|
Change from base Build 3932225358: | -0.1% |
Covered Lines: | 801 |
Relevant Lines: | 813 |
💛 - Coveralls
Thank you for your advice. I will update the PR description and try a more rational explanation. It may take some time.
@hrsh7th @mweststrate this looks like a great PR to give users (like me) meaningfully better performance with large objects (>1000 keys).
Was it closed just for lack of activity?
It was missing rationale IIRC when it was closed, but now I see it is present, so reopening :) It might take some time to process though due to my own personal circumstances. Apologies upfront!
I think it makes sense to make this behavior the new default, ship it as separate major version and make the old behavior opt-in using a flag.
Thank you for your review.
Nice to hear that this PR is an acceptable change.
I noticed that the test is failing. I will fix it.
Could you approve the GitHub Actions?
In my environment, this branch is passed the all tests... I want to re-try GitHub Actions.
done!
Thank you! It seems to pass the tests. I'll do the necessary work, such as fixes that change the default behavior.
I'm not confident about the documentation part, but I think the code part is ready for review.
:tada: This PR is included in version 10.0.0 :tada:
The release is available on:
Your semantic-release bot :package::rocket: