perf: optimize new-process-route-tree.ts #7449
Open
schiller-manuel wants to merge 4 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughRefactors route-tree parsing, construction, and matching: replaces helper-based segment parsing with inline char-code parsing, defers dynamic-child sorting via a ChangesRoute-tree parsing, construction, and matching optimization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
|
View your CI Pipeline Execution ↗ for commit 3ea9836
☁️ Nx Cloud last updated this comment at |
Contributor
🚀 Changeset Version PreviewNo changeset entries found. Merging this PR will not cause a version bump for any packages. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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/tests/new-process-route-tree.bench.ts`:
- Around line 252-277: The benchmarks for processRouteMasks are using
masksProcessed (from makeRouteTree()) for every case, so staticHeavyMasks and
sortableFanoutMasks are not being measured against their matching processed
trees; fix by computing and passing the correct processedTree for each mask:
call processRouteTree(staticHeavyTree).processedTree for the static-heavy masks
benchmark and processRouteTree(sortableFanoutTree).processedTree for the
sortable dynamic fanout masks benchmark, and pass those results into
processRouteMasks instead of reusing masksProcessed (references:
processRouteMasks, masksProcessed, staticHeavyMasks, sortableFanoutMasks,
processRouteTree, staticHeavyTree, sortableFanoutTree).
🪄 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: 907cc122-3cd0-4fc2-abdd-e69027456d01
📒 Files selected for processing (6)
packages/router-core/package.jsonpackages/router-core/src/new-process-route-tree.tspackages/router-core/tests/match-params.test.tspackages/router-core/tests/new-process-route-tree.bench.tspackages/router-core/tests/new-process-route-tree.test.tspackages/router-core/tests/optional-path-params.test.ts
Comment on lines
+252
to
+277
| const masksProcessed = processRouteTree(makeRouteTree()).processedTree | ||
| const decodeProcessed = processRouteTree(makeDecodeRouteTree()).processedTree | ||
| const encodedDecodePaths = makeDecodePaths(true) | ||
| const mixedDecodePaths = makeMixedDecodePaths() | ||
| let encodedDecodeIndex = 0 | ||
| let mixedDecodeIndex = 0 | ||
|
|
||
| bench('processRouteTree mixed tree', () => { | ||
| processRouteTree(routeTree) | ||
| }) | ||
|
|
||
| bench('processRouteTree static-heavy singleton dynamics', () => { | ||
| processRouteTree(staticHeavyTree) | ||
| }) | ||
|
|
||
| bench('processRouteTree sortable dynamic fanout', () => { | ||
| processRouteTree(sortableFanoutTree) | ||
| }) | ||
|
|
||
| bench('processRouteMasks static-heavy singleton dynamics', () => { | ||
| processRouteMasks(staticHeavyMasks, masksProcessed) | ||
| }) | ||
|
|
||
| bench('processRouteMasks sortable dynamic fanout', () => { | ||
| processRouteMasks(sortableFanoutMasks, masksProcessed) | ||
| }) |
Contributor
There was a problem hiding this comment.
Use matching processed trees for each processRouteMasks benchmark case.
staticHeavyMasks and sortableFanoutMasks are benchmarked against masksProcessed built from makeRouteTree(), so these two cases don't measure the intended tree/mask combinations.
Proposed fix
- const masksProcessed = processRouteTree(makeRouteTree()).processedTree
+ const staticHeavyProcessed = processRouteTree(staticHeavyTree).processedTree
+ const sortableFanoutProcessed =
+ processRouteTree(sortableFanoutTree).processedTree
@@
bench('processRouteMasks static-heavy singleton dynamics', () => {
- processRouteMasks(staticHeavyMasks, masksProcessed)
+ processRouteMasks(staticHeavyMasks, staticHeavyProcessed)
})
@@
bench('processRouteMasks sortable dynamic fanout', () => {
- processRouteMasks(sortableFanoutMasks, masksProcessed)
+ processRouteMasks(sortableFanoutMasks, sortableFanoutProcessed)
})🤖 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/tests/new-process-route-tree.bench.ts` around lines 252
- 277, The benchmarks for processRouteMasks are using masksProcessed (from
makeRouteTree()) for every case, so staticHeavyMasks and sortableFanoutMasks are
not being measured against their matching processed trees; fix by computing and
passing the correct processedTree for each mask: call
processRouteTree(staticHeavyTree).processedTree for the static-heavy masks
benchmark and processRouteTree(sortableFanoutTree).processedTree for the
sortable dynamic fanout masks benchmark, and pass those results into
processRouteMasks instead of reusing masksProcessed (references:
processRouteMasks, masksProcessed, staticHeavyMasks, sortableFanoutMasks,
processRouteTree, staticHeavyTree, sortableFanoutTree).
Contributor
Bundle Size Benchmarks
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. |
Merging this PR will not alter performance
Comparing 
Footnotes
|
## Summary - Optimized route-tree post-processing by sorting only dynamic child arrays that can need ordering. - Applied the same sortable-array tracking to route masks. - Added a param decode fast path that skips `decodeURIComponent` when a matched value has no `%` escape. - Removed the recursive `sortTreeNodes` post-build tree walk. - Pass `sortables` through recursive route parsing so nested dynamic arrays are recorded during construction. - Skip sortable tracking for single-route lazy matching, where no sibling ordering can be needed. ## Bundle Size Scenario: `react-router.minimal` | Metric | Before | After | Delta | | --- | ---: | ---: | ---: | | gzip | 89,392 | 89,269 | -123 | | initial gzip | 89,251 | 89,130 | -121 | | raw | 280,592 | 279,600 | -992 | | brotli | 77,753 | 77,670 | -83 | ## Focused Benchmarks Benchmarks were compared against a baseline worktree with the same benchmark cases applied and only the implementation different. | Case | Baseline hz | Current hz | Delta | | --- | ---: | ---: | ---: | | `processRouteTree static-heavy singleton dynamics` | 2,953.62 | 3,315.89 | +12.26% | | `processRouteTree sortable dynamic fanout` | 23,016.48 | 24,293.93 | +5.55% | | `processRouteMasks static-heavy singleton dynamics` | 3,654.97 | 4,054.59 | +10.93% | | `processRouteMasks sortable dynamic fanout` | 26,499.82 | 27,733.59 | +4.66% | | `findRouteMatch decode mixed90 params uncached batch` | 29,618.76 | 40,791.57 | +37.72% | | `findRouteMatch decode encoded params uncached batch` | 34,053.80 | 32,279.86 | -5.21% | Notes: - Route construction improves most when the tree has many singleton dynamic arrays, because the full post-build traversal is removed. - Mostly-unencoded param extraction improves by avoiding unnecessary decoding. - Encoded-only params are the tradeoff case because the `%` check adds overhead before decoding. ## Validation - `tests/new-process-route-tree.test.ts`: passed, `173 passed`, no type errors. - Focused perf cases: passed. - `git diff --check`: clean.
## Setup BEFORE: clean `HEAD` worktree with only new tests/benchmarks applied. AFTER: final route-tree implementation with `options` local restored after perf isolation. Relevant logs: - BEFORE worktree: `/var/folders/6f/2t42ntqs4yv4h6qwzbh5pmcm0000gn/T/opencode/route-tree-before-compare` - AFTER worktree: `/var/folders/6f/2t42ntqs4yv4h6qwzbh5pmcm0000gn/T/opencode/route-tree-after-compare` - Final main perf: `/tmp/route-tree-final-perf2.log` - Final full bundle diff: `/tmp/route-tree-final-full-bundle-diff2.txt` ## Tests | Check | BEFORE | AFTER | | --- | --- | --- | | focused unit | 243 passed | 243 passed | | router-core types | passed | passed | | route-tree perf bench | passed | passed | | relevant e2e | not rerun in BEFORE worktree | 44 passed | | whitespace | n/a | clean | ## Bundle Size Direct comparison for this diff, `react-router.minimal`: | Metric | BEFORE | AFTER | Delta | | --- | ---: | ---: | ---: | | gzip | 89269 | 89214 | -55 | | initial gzip | 89130 | 89073 | -57 | | raw | 279600 | 279460 | -140 | | brotli | 77670 | 77582 | -88 | Full benchmark vs unoptimized attribution baseline: | Scenario | Gzip Delta | | --- | ---: | | react-router.full | -159 | | react-router.minimal | -178 | | react-start.deferred-hydration | -207 | | react-start.full | -168 | | react-start.minimal | -177 | | react-start.rsbuild.full | -190 | | react-start.rsbuild.minimal | -175 | | solid-router.full | -165 | | solid-router.minimal | -131 | | solid-start.deferred-hydration | -151 | | solid-start.full | -152 | | solid-start.minimal | -159 | | vue-router.full | -154 | | vue-router.minimal | -148 | ## Perf Matched full-run comparison, same tests/benches in temp worktrees: | Bench | BEFORE hz | AFTER hz | Delta | Relevance | | --- | ---: | ---: | ---: | --- | | processRouteTree mixed tree | 8482.16 | 8406.43 | -0.9% | not relevant | | processRouteTree static-heavy singleton dynamics | 3376.49 | 3414.27 | +1.1% | not relevant, high variance | | processRouteTree sortable dynamic fanout | 25148.24 | 25787.98 | +2.5% | likely positive | | processRouteTree parsed priority fanout | 46308.12 | 45775.45 | -1.2% | contradicted by focused rerun | | processRouteTree optional fanout | 24307.28 | 25594.74 | +5.3% | likely positive, focused overlap | | processRouteTree wildcard fanout | 38286.45 | 40198.57 | +5.0% | not reproduced by focused rerun | | processRouteMasks static-heavy singleton dynamics | 3804.66 | 3709.58 | -2.5% | not relevant, overlaps | | processRouteMasks sortable dynamic fanout | 27902.76 | 28855.50 | +3.4% | likely positive | | findRouteMatch decode encoded params batch | 27156.51 | 30521.50 | +12.4% | not relevant, high RME | | findRouteMatch decode plain params batch | 39202.70 | 42038.73 | +7.2% | not relevant, high RME | | findRouteMatch decode mixed90 params batch | 38627.50 | 38124.96 | -1.3% | not relevant, high RME | | findRouteMatch sortable dynamic fanout | 19710991.25 | 19006507.35 | -3.6% | not relevant, high variance | Focused reruns used for suspected cases: | Bench | BEFORE hz | AFTER hz | Delta | Relevance | | --- | ---: | ---: | ---: | --- | | processRouteTree parsed priority fanout | 47007.46 +/-0.32% | 48042.74 +/-0.29% | +2.2% | statistically relevant positive | | processRouteTree optional fanout | 25094.35 +/-2.24% | 25718.38 +/-1.30% | +2.5% | not conclusive, ranges overlap | | processRouteTree wildcard fanout | 39812.35 +/-1.79% | 39765.98 +/-3.07% | -0.1% | not relevant | | processRouteTree static-heavy singleton dynamics | 3417.00 +/-3.12% | 3392.02 +/-0.77% | -0.7% | not relevant | Static-heavy note: one narrow run showed a clear slowdown before restoring the `options` local. After restoring it, repeated matched runs no longer showed a stable regression; identical BEFORE runs varied more than the final AFTER delta. ## Conclusion Final candidate keeps bundle wins and no statistically reliable perf regression remains. The only statistically relevant focused perf signal is positive for parsed priority fanout.
schiller-manuel
force-pushed
the
perf-process-route-tree
branch
from
4fa429d to
3ea9836
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/router-core/tests/new-process-route-tree.bench.ts (1)
326-367:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBenchmark input mismatch for
processRouteMasksscenarios.Line 326 builds
masksProcessedfrommakeRouteTree(), then Lines 361-367 reuse it forstaticHeavyMasksandsortableFanoutMasks. Those two cases are not measured against their matching processed trees, so the benchmark results are skewed.Proposed fix
- const masksProcessed = processRouteTree(makeRouteTree()).processedTree + const staticHeavyProcessed = processRouteTree(staticHeavyTree).processedTree + const sortableFanoutProcessedForMasks = + processRouteTree(sortableFanoutTree).processedTree @@ bench('processRouteMasks static-heavy singleton dynamics', () => { - processRouteMasks(staticHeavyMasks, masksProcessed) + processRouteMasks(staticHeavyMasks, staticHeavyProcessed) }) @@ bench('processRouteMasks sortable dynamic fanout', () => { - processRouteMasks(sortableFanoutMasks, masksProcessed) + processRouteMasks(sortableFanoutMasks, sortableFanoutProcessedForMasks) })🤖 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/tests/new-process-route-tree.bench.ts` around lines 326 - 367, The benchmark uses masksProcessed = processRouteTree(makeRouteTree()).processedTree for all mask tests, causing mismatched inputs; update the two processRouteMasks calls to use processed trees that match their masks by creating corresponding processedTree variables (e.g., const staticHeavyProcessed = processRouteTree(staticHeavyTree).processedTree and const sortableFanoutProcessed = processRouteTree(sortableFanoutTree).processedTree) and pass staticHeavyProcessed to processRouteMasks(staticHeavyMasks, ...) and sortableFanoutProcessed to processRouteMasks(sortableFanoutMasks, ...), leaving other benchmarks unchanged.
🤖 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.
Duplicate comments:
In `@packages/router-core/tests/new-process-route-tree.bench.ts`:
- Around line 326-367: The benchmark uses masksProcessed =
processRouteTree(makeRouteTree()).processedTree for all mask tests, causing
mismatched inputs; update the two processRouteMasks calls to use processed trees
that match their masks by creating corresponding processedTree variables (e.g.,
const staticHeavyProcessed = processRouteTree(staticHeavyTree).processedTree and
const sortableFanoutProcessed =
processRouteTree(sortableFanoutTree).processedTree) and pass
staticHeavyProcessed to processRouteMasks(staticHeavyMasks, ...) and
sortableFanoutProcessed to processRouteMasks(sortableFanoutMasks, ...), leaving
other benchmarks unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7bc6c833-1305-4842-b733-d6d435fd96ba
📒 Files selected for processing (6)
packages/router-core/package.jsonpackages/router-core/src/new-process-route-tree.tspackages/router-core/tests/match-params.test.tspackages/router-core/tests/new-process-route-tree.bench.tspackages/router-core/tests/new-process-route-tree.test.tspackages/router-core/tests/optional-path-params.test.ts
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Important
At least one additional CI pipeline execution has run since the conclusion below was written and it may no longer be applicable.
Nx Cloud has identified a possible root cause for your failed CI:
We were unable to attribute this E2E failure to the changes in this PR. The failing test (client side navigating to a route with scripts) lives in tanstack-solid-start-e2e-basic, a project not touched by this PR, and the script-injection behavior it exercises has no direct relationship to the route-tree sorting or param-decode optimizations introduced here. We recommend re-running the pipeline to determine whether this is a transient environment issue.
No code changes were suggested for this issue.
Trigger a rerun:
🎓 Learn more about Self-Healing CI on nx.dev
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
decodeURIComponentwhen a matched value has no%escape.sortTreeNodespost-build tree walk.sortablesthrough recursive route parsing so nested dynamic arrays are recorded during construction.Bundle Size
Scenario:
react-router.minimalFocused Benchmarks
Benchmarks were compared against a baseline worktree with the same benchmark cases applied and only the implementation different.
processRouteTree static-heavy singleton dynamicsprocessRouteTree sortable dynamic fanoutprocessRouteMasks static-heavy singleton dynamicsprocessRouteMasks sortable dynamic fanoutfindRouteMatch decode mixed90 params uncached batchfindRouteMatch decode encoded params uncached batchNotes:
%check adds overhead before decoding.Validation
tests/new-process-route-tree.test.ts: passed,173 passed, no type errors.git diff --check: clean.Summary by CodeRabbit
Bug Fixes
Tests
Chores