Uh oh!
There was an error while loading. Please reload this page.
perf(router-core): faster enumerable-key collection in structural sharing - #8010
Merged
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.
Contributor
View your CI Pipeline Execution ↗ for commit bf052ab
☁️ Nx Cloud last updated this comment at |
Contributor
🚀 Changeset Version Preview1 package(s) bumped directly, 22 bumped as dependents. 🟩 Patch bumps
|
Contributor
📝 WalkthroughWalkthroughThe PR updates enumerable-key detection in ChangesEnumerable key performance update
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
Bundle Size Benchmarks
The following scenarios have bundle-size changes compared with the baseline:
Current gzip tracks all emitted client JS chunks. Initial gzip tracks only the entry/import graph. Trend sparkline is historical current gzip ending with this PR measurement; lower is better. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/router-core/src/utils.ts (1)
307-326: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the correct element type for the key array.
keysis inferred asstring[]fromObject.keys(o), but the slow path appends symbol keys. DeclarekeysasArray<string | symbol>and remove the redundantpush(...)cast.Proposed fix
- const keys = Object.keys(o)+ const keys: Array<string | symbol> = Object.keys(o) ... - ;(keys as Array<string | symbol>).push(symbol)+ keys.push(symbol)🤖 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` around lines 307 - 326, Update the keys declaration in the shown key-collection logic to use Array<string | symbol> so it supports both string and symbol keys, then remove the redundant cast from the symbol push in the slow path.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@packages/router-core/src/utils.ts`:
- Around line 307-326: Update the keys declaration in the shown key-collection
logic to use Array<string | symbol> so it supports both string and symbol keys,
then remove the redundant cast from the symbol push in the slow path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 94447b85-30c5-4efd-8812-495289a09209
📒 Files selected for processing (3)
.changeset/perf-structural-sharing-enumerable-keys.mdpackages/router-core/src/utils.tspackages/router-core/tests/utils.test.ts
Merging this PR will degrade performance by 28.98%
|
| Mode | Benchmark | BASE | HEAD | Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Memory | mem client preload-churn (vue) | 776.2 KB | 735.3 KB | +5.57% |
| 👁 | Simulation | ssr dehydrate plain control (solid) | 119.6 ms | 129.4 ms | -7.61% |
| 👁 | Simulation | ssr dehydrate rich types (solid) | 87.3 ms | 99 ms | -11.84% |
| 👁 | Memory | mem server error-paths redirect (solid) | 588.2 KB | 648.6 KB | -9.31% |
| 👁 | Memory | mem server error-paths redirect (vue) | 294 KB | 692 KB | -57.52% |
| 👁 | Simulation | client-links navigation loop (vue) | 163.9 ms | 173.9 ms | -5.77% |
| 👁 | Memory | mem server error-paths unmatched (react) | 263.6 KB | 923.6 KB | -71.46% |
| 👁 | Memory | mem client unique-location-churn (solid) | 341.6 KB | 470.1 KB | -27.32% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing perf/structural-sharing-enumerable-keys-7664 (bf052ab) with main (c568caf)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supersedes #7664. The original PR is closed, comes from a private fork, and does not allow maintainer edits, so it could not be brought up to date in place.
The original commit was cherry-picked onto current
main, preserving Yagiz Nizipli as author. A separate follow-up addresses the outstanding review request to use block bodies for the changedifstatements.What
propertyIsEnumerableloop withObject.keysplus onegetOwnPropertyNameslength comparison.Tests
CI=1 NX_DAEMON=false pnpm nx run @tanstack/router-core:test:unit --outputStyle=stream --skipRemoteCache -- tests/utils.test.ts(117 passed, 3 expected failures)CI=1 NX_DAEMON=false pnpm nx run @tanstack/router-core:test:types --outputStyle=stream --skipRemoteCacheCI=1 NX_DAEMON=false pnpm nx run @tanstack/router-core:test:eslint --outputStyle=stream --skipRemoteCache(no errors; existing unrelated warnings)pnpm prettier --check .changeset/perf-structural-sharing-enumerable-keys.md packages/router-core/src/utils.ts packages/router-core/tests/utils.test.tsSummary by CodeRabbit
Performance
Bug Fixes