-
-
Notifications
You must be signed in to change notification settings - Fork 867
Optimize Immer performance where possible #1164
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
94017ad
7c3ae12
1a8c36b
1b56279
2f00eec
eadd0f1
8bf3158
8c68f4f
a2162e8
a08e62c
a450685
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -56,11 +56,16 @@ function finalize(rootScope: ImmerScope, value: any, path?: PatchPath) { | |
| // Don't recurse in tho recursive data structures | ||
| if (isFrozen(value)) return value | ||
|
|
||
| const useStrictIteration = rootScope.immer_.shouldUseStrictIteration() | ||
|
|
||
| const state: ImmerState = value[DRAFT_STATE] | ||
| // A plain object, might need freezing, might contain drafts | ||
| if (!state) { | ||
| each(value, (key, childValue) => | ||
| finalizeProperty(rootScope, state, value, key, childValue, path) | ||
| each( | ||
| value, | ||
| (key, childValue) => | ||
| finalizeProperty(rootScope, state, value, key, childValue, path), | ||
| useStrictIteration | ||
| ) | ||
| return value | ||
| } | ||
|
|
@@ -87,8 +92,19 @@ function finalize(rootScope: ImmerScope, value: any, path?: PatchPath) { | |
| result.clear() | ||
| isSet = true | ||
| } | ||
| each(resultEach, (key, childValue) => | ||
| finalizeProperty(rootScope, state, result, key, childValue, path, isSet) | ||
| each( | ||
| resultEach, | ||
| (key, childValue) => | ||
| finalizeProperty( | ||
| rootScope, | ||
| state, | ||
| result, | ||
| key, | ||
| childValue, | ||
| path, | ||
| isSet | ||
| ), | ||
| useStrictIteration | ||
| ) | ||
| // everything inside is frozen, we can freeze here | ||
| maybeFreeze(rootScope, result, false) | ||
|
|
@@ -114,6 +130,18 @@ function finalizeProperty( | |
| rootPath?: PatchPath, | ||
| targetIsSet?: boolean | ||
| ) { | ||
| if (childValue == null) { | ||
| return | ||
| } | ||
|
|
||
| if (typeof childValue !== "object" && !targetIsSet) { | ||
| return | ||
| } | ||
| const childIsFrozen = isFrozen(childValue) | ||
| if (childIsFrozen && !targetIsSet) { | ||
| return | ||
| } | ||
|
|
||
| if (process.env.NODE_ENV !== "production" && childValue === targetObject) | ||
| die(5) | ||
| if (isDraft(childValue)) { | ||
|
|
@@ -136,7 +164,7 @@ function finalizeProperty( | |
| targetObject.add(childValue) | ||
| } | ||
| // Search new objects for unfinalized drafts. Frozen objects should never contain drafts. | ||
| if (isDraftable(childValue) && !isFrozen(childValue)) { | ||
| if (isDraftable(childValue) && !childIsFrozen) { | ||
| if (!rootScope.immer_.autoFreeze_ && rootScope.unfinalizedDrafts_ < 1) { | ||
| // optimization: if an object is not a draft, and we don't have to | ||
| // deepfreeze everything, and we are sure that no drafts are left in the remaining object | ||
|
|
@@ -145,6 +173,15 @@ function finalizeProperty( | |
| // See add-data.js perf test | ||
| return | ||
| } | ||
| if ( | ||
| parentState && | ||
| parentState.base_ && | ||
| parentState.base_[prop] === childValue && | ||
| childIsFrozen | ||
| ) { | ||
| // Object is unchanged from base - no need to process further | ||
| return | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious, how does this branch do in coverage? |
||
| } | ||
| finalize(rootScope, childValue) | ||
| // Immer deep freezes plain objects, so if there is no parent state, we freeze as well | ||
| // Per #590, we never freeze symbolic properties. Just to make sure don't accidentally interfere | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,22 +35,26 @@ export function isDraftable(value: any): boolean { | |
| } | ||
|
|
||
| const objectCtorString = Object.prototype.constructor.toString() | ||
| const cachedCtorStrings = new WeakMap() | ||
| /*#__PURE__*/ | ||
| export function isPlainObject(value: any): boolean { | ||
| if (!value || typeof value !== "object") return false | ||
| const proto = getPrototypeOf(value) | ||
| if (proto === null) { | ||
| return true | ||
| } | ||
| const proto = Object.getPrototypeOf(value) | ||
| if (proto === null || proto === Object.prototype) return true | ||
|
|
||
| const Ctor = | ||
| Object.hasOwnProperty.call(proto, "constructor") && proto.constructor | ||
|
|
||
| if (Ctor === Object) return true | ||
|
|
||
| return ( | ||
| typeof Ctor == "function" && | ||
| Function.toString.call(Ctor) === objectCtorString | ||
| ) | ||
| if (typeof Ctor !== "function") return false | ||
|
|
||
| let ctorString = cachedCtorStrings.get(Ctor) | ||
| if (ctorString === undefined) { | ||
| ctorString = Function.toString.call(Ctor) | ||
| cachedCtorStrings.set(Ctor, ctorString) | ||
| } | ||
|
|
||
| return ctorString === objectCtorString | ||
| } | ||
|
|
||
| /** Get the underlying object that is represented by the given draft */ | ||
|
|
@@ -64,15 +68,23 @@ export function original(value: Drafted<any>): any { | |
| /** | ||
| * Each iterates a map, set or array. | ||
| * Or, if any other kind of object, all of its own properties. | ||
| * Regardless whether they are enumerable or symbols | ||
| * | ||
| * @param obj The object to iterate over | ||
| * @param iter The iterator function | ||
| * @param strict When true (default), includes symbols and non-enumerable properties. | ||
| * When false, uses looseiteration over only enumerable string properties. | ||
| */ | ||
| export function each<T extends Objectish>( | ||
| obj: T, | ||
| iter: (key: string | number, value: any, source: T) => void | ||
| iter: (key: string | number, value: any, source: T) => void, | ||
| strict?: boolean | ||
| ): void | ||
| export function each(obj: any, iter: any) { | ||
| export function each(obj: any, iter: any, strict: boolean = true) { | ||
| if (getArchtype(obj) === ArchType.Object) { | ||
| 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) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per comment below, looks like |
||
| keys.forEach(key => { | ||
| iter(key, obj[key], obj) | ||
| }) | ||
| } else { | ||
|
|
@@ -198,12 +210,12 @@ export function freeze<T>(obj: T, deep?: boolean): T | |
| export function freeze<T>(obj: any, deep: boolean = false): T { | ||
| if (isFrozen(obj) || isDraft(obj) || !isDraftable(obj)) return obj | ||
| if (getArchtype(obj) > 1 /* Map or Set */) { | ||
| Object.defineProperties(obj, { | ||
| set: {value: dontMutateFrozenCollections as any}, | ||
| add: {value: dontMutateFrozenCollections as any}, | ||
| clear: {value: dontMutateFrozenCollections as any}, | ||
| delete: {value: dontMutateFrozenCollections as any} | ||
| }) | ||
| Object.defineProperties(obj, { | ||
| set: dontMutateMethodOverride, | ||
| add: dontMutateMethodOverride, | ||
| clear: dontMutateMethodOverride, | ||
| delete: dontMutateMethodOverride | ||
| }) | ||
| } | ||
| Object.freeze(obj) | ||
| if (deep) | ||
|
|
@@ -217,6 +229,12 @@ function dontMutateFrozenCollections() { | |
| die(2) | ||
| } | ||
|
|
||
| const dontMutateMethodOverride = { | ||
| value: dontMutateFrozenCollections | ||
| } | ||
|
|
||
| export function isFrozen(obj: any): boolean { | ||
| // Fast path: primitives and null/undefined are always "frozen" | ||
| if (obj === null || typeof obj !== "object") return true | ||
| return Object.isFrozen(obj) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not following this one entirely, might deserve a comment 😅