Optimize Immer performance where possible#1164
Conversation
Pull Request Test Coverage Report for Build 18295597982Details
💛 - Coveralls |
|
Need to do some cleanup on leftover comments and the old vs new |
| Reflect.ownKeys(obj).forEach(key => { | ||
| // If strict, we do a full iteration including symbols and non-enumerable properties | ||
| // Otherwise, we only iterate enumerable string properties for performance | ||
| const keys = strict ? Reflect.ownKeys(obj) : Object.keys(obj) |
There was a problem hiding this comment.
Might be worth checking out what the fastest way to iterate is here. I can imagine that a for..in loop is actually faster than allocating the keys array first.
There was a problem hiding this comment.
Per comment below, looks like for..in is a bit better with small objects, but Object.keys() is consistently better for larger objects. (And Reflect.ownKeys() is somewhat slower than Object.keys(), so definitely enough to justify a change here.)
src/utils/common.ts
Outdated
| delete: {value: dontMutateFrozenCollections as any} | ||
| }) | ||
| Object.defineProperties(obj, { | ||
| set: {value: dontMutateFrozenCollections as any}, |
There was a problem hiding this comment.
Optimization opportunity: we could make this a single constant object
There was a problem hiding this comment.
Both the whole properties and descriptors
src/core/finalize.ts
Outdated
| value, | ||
| (key, childValue) => | ||
| finalizeProperty(rootScope, state, value, key, childValue, path), | ||
| rootScope.immer_.shouldUseStrictIteration(value) |
There was a problem hiding this comment.
Q: would this be marginally faster if we made this a param to finalize?
mweststrate
left a comment
mweststrate
left a comment
There was a problem hiding this comment.
This is cool stuff! Left some comments
src/utils/common.ts
Outdated
| return !!value && !!value[DRAFT_STATE] | ||
| } | ||
|
|
||
| const isDraftableCache = new WeakMap<object, boolean>() |
There was a problem hiding this comment.
Probably good to move this to a stand alone PR? I'd not be suprised that the map lookups are going to loose from the type reflection in large data sets.
There was a problem hiding this comment.
Yeah, now that I've got the benchmark system more updated, it looks like this is actually a regression. Avg +15% better without it, avg -34% worse with it. Removing!
| childIsFrozen | ||
| ) { | ||
| // Object is unchanged from base - no need to process further | ||
| return |
There was a problem hiding this comment.
Curious, how does this branch do in coverage?
| return | ||
| } | ||
|
|
||
| if (typeof childValue !== "object" && !targetIsSet) { |
There was a problem hiding this comment.
Not following this one entirely, might deserve a comment 😅
|
I generated a Mitata comparison of object key iteration methods. Looks like So, there's enough difference to be worth using |
markerikson
force-pushed
the
feature/optimize-immer-perf
branch
from
9e264fc to
a08e62c
Compare
|
I've gone through and benchmarked each of the changes from this branch individually vs v10.1.13:
Given that, I've rebased the branch to remove the I also checked the fast bailout in I've left strict iteration set to So, this PR as-is is a ~5% improvement and could be merged as-is. |
|
Thanks! I'll give it another pass and will then merge. I'll be travelling next week, so depending on jetlag that will help or delay, but will otherwise process the week afterwards. |
mweststrate
left a comment
mweststrate
left a comment
There was a problem hiding this comment.
Looking great! Merging and releasing as is. I suggest we swap the default in the finalization rework PR, and release that entirely as new master release.
|
🎉 This PR is included in version 10.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
3 participants
Overview
(Effectively stacked on #1162 for Vitest and #1163 for the perf benchmark harness, but keeping this separate for ease of review.)
Per #1152 , I've been investigating Immer's loss of performance over the last several major versions.
Using the perf benchmark harness and CPU profiling, I was able to use AI to help identify a series of hotspots and slow functions, then try out various approaches to optimize them and check the results. The benchmark numbers are noisier than I'd like, but as best as I can tell all of these are an improvement individually, and together.
The additional good news is that the changes in this PR are fairly small tweaks (per-function caching or early bailouts), and not major architectural changes. So, these should be feasible to review and merge, and also it means that a larger architectural change might have additional wins.
Optimizations
This PR contains several optimizations:
isPlainObject: This turned out to be a surprising hotspot right away. The culprit seems to be the repeated constructor stringification, which is almost always the same reference with plain objects. Using aWeakMapto cache that eliminatedisPlainObjectas a hotspot.finalizeProperty: Adding some early returns avoided running the rest of the logic in cases like primitives.isDraftable: anotherWeakMapcache, plus adding a fast bailout patheach: ported the logic from thefaster-iteration-experimentbranch that usesObject.keys()instead ofReflect.ownKeys(), and added a newsetUseStrictIterationoption. Currently it defaults totrueto maintain the existing behavior.Perf Results (updated 2025-10-14)
If I run the benchmark with these changes with
strictIteration: true, the relative results are:With
strictIteration: false(ie loose iteration), I get:So switching from strict to loose iteration is a significant perf win, but also technically a breaking change.
Further architectural changes
This PR will be followed by two more that are significantly architecturally different: