perf(router-core): faster enumerable-key collection in structural sharing#7664
perf(router-core): faster enumerable-key collection in structural sharing#7664anonrig wants to merge 1 commit into
Conversation
…ring getEnumerableOwnKeys (used by replaceEqualDeep) called Object.getOwnPropertyNames and then invoked propertyIsEnumerable once per key to verify every own string prop is enumerable. Replace that O(n) loop of JS method calls with Object.keys (native, enumerable-only) plus a single length comparison against getOwnPropertyNames to detect non-enumerable string props. Symbol handling is unchanged. Behavior is identical (verified against the existing suite + fuzzing): the function still returns the enumerable own keys in the same order and still bails (returns false) for objects with any non-enumerable own string or symbol property. replaceEqualDeep runs on every selector result on every state update when defaultStructuralSharing is enabled, so this is a hot client path for selector-heavy apps. Measured ~1.3-1.5x faster on typical router state.
📝 WalkthroughWalkthrough
ChangesStructural Sharing Enumerable Key Optimization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/router-core/src/utils.ts`:
- Line 308: The if statement checking keys.length against
Object.getOwnPropertyNames(o).length on line 308 uses a one-line body without
curly braces, which violates the repository's style guidelines requiring braces
for all control statements. Add curly braces around the return false statement
to wrap the body. Apply the same fix to the similar if statements also mentioned
on lines 314 and 319.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c38a969f-acca-43e4-97f3-10cdc0a50a17
📒 Files selected for processing (3)
.changeset/perf-structural-sharing-enumerable-keys.mdpackages/router-core/src/utils.tspackages/router-core/tests/utils.test.ts
| // "clone-friendly" -> bail. This replaces an O(n) loop of | ||
| // `propertyIsEnumerable` calls with two native calls. | ||
| const keys = Object.keys(o) | ||
| if (keys.length !== Object.getOwnPropertyNames(o).length) return false |
There was a problem hiding this comment.
Use block bodies for all changed if statements.
These one-line if bodies violate the repository rule requiring braces on control statements.
Suggested patch
- if (keys.length !== Object.getOwnPropertyNames(o).length) return false
+ if (keys.length !== Object.getOwnPropertyNames(o).length) {
+ return false
+ }
@@
- if (symbols.length === 0) return keys
+ if (symbols.length === 0) {
+ return keys
+ }
@@
- if (!isEnumerable.call(o, symbol)) return false
+ if (!isEnumerable.call(o, symbol)) {
+ return false
+ }As per coding guidelines, "**/*.{ts,tsx,js,jsx}: Always use curly braces for if, else, loops, and similar control statements."
Also applies to: 314-314, 319-319
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/router-core/src/utils.ts` at line 308, The if statement checking
keys.length against Object.getOwnPropertyNames(o).length on line 308 uses a
one-line body without curly braces, which violates the repository's style
guidelines requiring braces for all control statements. Add curly braces around
the return false statement to wrap the body. Apply the same fix to the similar
if statements also mentioned on lines 314 and 319.
Source: Coding guidelines
|
View your CI Pipeline Execution ↗ for commit d18d300
☁️ Nx Cloud last updated this comment at |
Merging this PR will degrade performance by 11.21%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | mem aborted-requests (solid) |
1.8 MB | 2.7 MB | -35.01% |
| ❌ | Memory | mem serialization-payload (solid) |
6.9 MB | 8.9 MB | -22.2% |
| ❌ | Memory | mem peak-large-page (solid) |
3.4 MB | 3.8 MB | -11.32% |
| ❌ | Memory | mem navigation-churn (solid) |
1.4 MB | 1.5 MB | -5.76% |
| ❌ | Memory | mem aborted-requests (vue) |
959.1 KB | 989.5 KB | -3.07% |
| ⚡ | Memory | mem streaming-peak chunked (vue) |
12.2 MB | 11.9 MB | +3.1% |
| ⚡ | Memory | mem request-churn (solid) |
1.2 MB | 1.1 MB | +3.04% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing anonrig:perf/structural-sharing-enumerable-keys (d18d300) with main (f23ed0f)1
Footnotes
1 participant
What
Speed up
getEnumerableOwnKeys— the helper that backsreplaceEqualDeep(structural sharing) inpackages/router-core/src/utils.ts.It previously called
Object.getOwnPropertyNames(o)and then invokedpropertyIsEnumerableonce per key to confirm every own string property is enumerable. This swaps that O(n) loop of JS method calls forObject.keys(o)(native, enumerable-only) plus a single length comparison againstgetOwnPropertyNames(o)to detect non-enumerable own string props. Symbol handling is unchanged.Why
replaceEqualDeepruns on every selector result on every state update whendefaultStructuralSharingis enabled, sogetEnumerableOwnKeysis called for every plain object it walks. For selector-heavy apps this is a hot client path (it short-circuits on the server, so this is purely a client-side win).Benchmark on the real function (Vitest
bench, node env whereisServeris false), deeply-equal new state object shaped like typical router state (search / params / nested loaderData / context):≈ 1.36× faster in-situ (an isolated A/B of just the key-collection step measured ~1.55×). The win grows with object key count, since the old path made one
propertyIsEnumerablecall per key.Correctness
Behavior is identical:
Object.keysandObject.getOwnPropertyNamesfollow the same[[OwnPropertyKeys]]ordering (integer indices ascending, then string keys in insertion order), and in the non-bail case every string key is enumerable, so the two orderings coincide.false) for objects with any non-enumerable own string property (length mismatch) or any non-enumerable own symbol (existing per-symbol check).Verified by:
getEnumerableOwnKeys behavior (via replaceEqualDeep)suite (non-enumerable strings/symbols, frozen/sealed,Object.create(null), numeric keys, shadowing, mixed string+symbol).next.Notes
replaceEqualDeepitself is untouched.@tanstack/router-corepatch).Summary by CodeRabbit
Performance
Tests