refactor(query-core): improve replaceEqualDeep performance #9604
Conversation
WalkthroughReplaces previous replaceEqualDeep with a unified deep-merge handling arrays and plain objects, adds type guards ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Utils as replaceEqualDeep
Caller->>Utils: replaceEqualDeep(a, b)
alt not both arrays nor both plain objects
Utils-->>Caller: return b
else Arrays
Note over Utils: build copy = new Array(b.length)
loop for each index i
Utils->>Utils: aItem = a[i]\nrecurse = replaceEqualDeep(aItem, b[i])
alt aItem === b[i] or both undefined
Note over Utils: equalItems++
else
Note over Utils: copy[i] = recurse
end
end
alt lengths match and equalItems == a.length
Utils-->>Caller: return a
else
Utils-->>Caller: return copy
end
else Plain Objects
Note over Utils: build copy = {}
loop for each own key in b
Utils->>Utils: aItem = a[key]\nrecurse = replaceEqualDeep(aItem, b[key])
alt aItem === b[key] or both undefined
Note over Utils: equalItems++
else
Note over Utils: copy[key] = recurse
end
end
alt own-key counts match and equalItems == aKeyCount
Utils-->>Caller: return a
else
Utils-->>Caller: return copy
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
View your CI Pipeline Execution ↗ for commit c433b8f
☁️ Nx Cloud last updated this comment at |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9604 +/- ##
===========================================
+ Coverage 45.17% 59.29% +14.12%
===========================================
Files 208 137 -71
Lines 8327 5570 -2757
Branches 1879 1501 -378
===========================================
- Hits 3762 3303 -459
+ Misses 4118 1963 -2155
+ Partials 447 304 -143 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/query-core/src/utils.ts (1)
273-286: Micro-opt: hoist hasOwn and avoid double reads of b[key]Hoist
hasOwnand reusebItemto shave a couple of lookups in tight loops.Apply:
- let equalItems = 0 + const hasOwn = Object.prototype.hasOwnProperty + let equalItems = 0 @@ - if ( - (array || Object.prototype.hasOwnProperty.call(a, key)) && + if ( + (array || hasOwn.call(a, key)) &&
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/query-core/src/utils.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/query-core/src/utils.ts (1)
packages/query-core/src/index.ts (1)
replaceEqualDeep(19-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Preview
- GitHub Check: Test
🔇 Additional comments (3)
packages/query-core/src/utils.ts (3)
262-263: Confirm intent: bail out for non-plain arrays/objectsEarly-returning
bwhen neither a plain array nor a plain object skips structural sharing for sparse arrays, typed arrays, class instances, Dates, Maps/Sets, etc. Verify this matches prior behavior and expectations for consumers. If parity with previous semantics is desired, add tests around sparse arrays and typed arrays.
264-269: Prealloc and key-caching look goodPre-allocating
new Array(bSize)and caching key lists should reduce allocations and property lookups on hot paths.
292-292: Return condition reads clearlyReusing
aonly when sizes match and all items are equal preserves structural sharing without false positives. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/query-core/src/utils.ts (1)
271-283: Cache bItem to avoid repeated property lookups inside the hot loop.Reads
b[key]twice per iteration. Caching improves locality and aligns with the PR’s stated perf goals.for (let i = 0; i < bSize; i++) { const key = array ? i : bItems[i] const aItem = a[key] + const bItem = b[key] if ( - (array || Object.prototype.hasOwnProperty.call(a, key)) && - aItem === undefined && - b[key] === undefined + (array || Object.prototype.hasOwnProperty.call(a, key)) && + aItem === undefined && + bItem === undefined ) { copy[key] = undefined equalItems++ } else { - const value = replaceEqualDeep(aItem, b[key]) + const value = replaceEqualDeep(aItem, bItem) copy[key] = value if (value === aItem && aItem !== undefined) { equalItems++ } } }
🧹 Nitpick comments (1)
packages/query-core/src/utils.ts (1)
261-268: Early-return and pre-allocation look good; trim an unnecessary allocation.
aItemsis only used to computeaSize. Avoid creating it to reduce work/memory.- const aItems = array ? a : Object.keys(a) - const aSize = aItems.length + const aSize = array ? a.length : Object.keys(a).length
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/query-core/src/utils.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/query-core/src/utils.ts (1)
packages/query-core/src/index.ts (1)
replaceEqualDeep(19-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (1)
packages/query-core/src/utils.ts (1)
275-276: Prototype-safe hasOwnProperty: LGTM.Switching to
Object.prototype.hasOwnProperty.call(a, key)avoids issues whenhasOwnPropertyis shadowed.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/query-core/src/utils.ts (1)
373-402: Use safe own-check and correct prototype comment
- Change the own-property check on
protto use the helper for safety:if (!hasOwn.call(prot, 'isPrototypeOf')) { return false }- Revise the comment before the
getPrototypeOfguard to accurately state that we reject any object whose prototype isn’t exactlyObject.prototype(null-proto objects are already accepted by the earlyctor === undefinedreturn).
♻️ Duplicate comments (1)
packages/query-core/src/utils.ts (1)
248-248: Good fix: prototype-safe own-property check and cached reads.Resolved the earlier “shadowed hasOwnProperty” concern and reduced repeated property access by caching
aItem/bItem. Looks solid.Also applies to: 319-327
🧹 Nitpick comments (3)
packages/query-core/src/utils.ts (3)
255-256: Preferunknownoveranyin the implementation signature.Keeping the overload
replaceEqualDeep<T>(a: unknown, b: T): Tis great. For the implementation, returningunknownavoids leakinganywhile staying compatible with the overloads.-export function replaceEqualDeep(a: unknown, b: unknown): any { +export function replaceEqualDeep(a: unknown, b: unknown): unknown {
311-345: Object path: tight loop and O(1) own-checks LGTM.
for..inoverbavoids materializing key arrays.- Equality counting gated by
hasOwn.call(a, k)is correct.- Recursion preserves references where possible.
One note: symbol keys aren’t considered (consistent with JSON-only constraint). If that’s intentional, consider documenting it near
replaceDatato preempt surprises.
368-370: Sparse/augmented arrays won’t be treated as “plain arrays”. Please confirm.
isPlainArrayreturns false for sparse arrays or arrays with extra enumerable props (length !== Object.keys(value).length). That changes behavior vs a naiveArray.isArraycheck and will fall back to “return b” instead of structural sharing. If intentional, add tests for:
- sparse arrays (holes)
- arrays with extra enumerable props
I can draft these tests if desired.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/query-core/src/utils.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (2)
packages/query-core/src/utils.ts (2)
261-299: Array path perf refactor looks correct.Pre-allocation + strict-equality fast path + recursive sharing are implemented correctly, and the equality accounting (
i < aSize) handles length mismatches safely.
301-309: Type-mismatch early return preserves previous semantics.Returning
bwhen only one side is an array or a non-plain object matches the intended “JSON-only” structural sharing behavior. Good.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/query-core/src/utils.ts (2)
345-346: Don’t call a potentially shadowed hasOwnProperty on prototypes
prot.hasOwnProperty('isPrototypeOf')can throw ifhasOwnPropertyis overridden on that prototype. Use the prototype-safe helper.Apply:
- if (!prot.hasOwnProperty('isPrototypeOf')) { + if (!hasOwn.call(prot, 'isPrototypeOf')) {
327-356: Allow null-prototype objects in isPlainObject
Relax the final prototype check so thatObject.getPrototypeOf(o) === nullis treated as plain (e.g. changeif (Object.getPrototypeOf(o) !== Object.prototype) { return false }to something like
const proto = Object.getPrototypeOf(o) if (proto !== Object.prototype && proto !== null) { return false }) to restore the existing test behavior in utils.test.tsx.
♻️ Duplicate comments (1)
packages/query-core/src/utils.ts (1)
248-248: Prototype-safe own-property helper: LGTMUsing a cached reference and calling via
.callavoids shadowing risks. This addresses the earlier review concern.
🧹 Nitpick comments (1)
packages/query-core/src/utils.ts (1)
322-324: Clarify dense-array intent in isPlainArrayMinor: add a short doc comment noting that sparse arrays or arrays with extra enumerable props return false.
+/** True only for dense Arrays with no extra enumerable properties (sparse arrays excluded). */ export function isPlainArray(value: unknown): value is Array<unknown> { return Array.isArray(value) && value.length === Object.keys(value).length }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/query-core/src/utils.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/query-core/src/utils.ts (1)
packages/query-core/src/index.ts (1)
replaceEqualDeep(19-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (1)
packages/query-core/src/utils.ts (1)
263-300: replaceEqualDeep hot path and allocation strategy: LGTMEarly bailout + preallocated arrays + cached aItem/bItem lookups are clean and align with the perf goals. The equality accounting also correctly preserves
aonly when sizes match and all items are equal.
This PR replicates the changes that were done in TanStack/router#5046.
TL;DR: Improve
replaceEqualDeepperformance by ~2xHow?
new Array(bSize)), to avoid the memory management that happens under the hood every time we add an item to ita[key]) and instead store it asconst aItemhasOwnPropertyinstead of creating aSetof the keys, because in theoryhasOwnPropertyis already O(1) for lookups, so we don't need to create an extra data structure for thisBenchmark
Results
Summary by CodeRabbit
Bug Fixes
Refactor