perf(router-core): share settle chain between superseded navigation waiters - #8222
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe router now chains load transactions to their current successors. Awaiting any transaction waits for the successor chain to settle. A regression test covers 25 same-tick navigations and the final idle state. ChangesTransaction settlement
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Superseded navigation promises now share successor completion and resolve once the final navigation settles, with regression coverage for same-tick bursts. No current merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant Navigation
participant loadClientRoute
participant LoadTransaction
participant awaitCurrent
Navigation->>loadClientRoute: start navigation load
loadClientRoute->>LoadTransaction: await tx[5]
LoadTransaction->>awaitCurrent: follow successor chain
awaitCurrent->>LoadTransaction: await successor completion
LoadTransaction-->>loadClientRoute: resolve after chain settles
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is detailed and directly related to the changes. It explains the quadratic behavior, the implementation, performance evidence, test coverage, and the pending CodSpeed check. It uses headings that differ from the repository template and does not explicitly include the template checklist or release-impact section, but the required information is mostly present. Full details: Docstring CoverageExplanation Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 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 |
|
View your CI Pipeline Execution ↗ for commit 8c76f33
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version Preview5 package(s) bumped directly, 18 bumped as dependents. 🟩 Patch bumps
|
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. |
Merging this PR will improve performance by 3.82%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Memory | mem server error-paths redirect (vue) |
891.9 KB | 415.9 KB | ×2.1 |
| ⚡ | Memory | mem server error-paths not-found (solid) |
698.7 KB | 563.1 KB | +24.08% |
| ⚡ | Memory | mem client preload-churn (vue) |
919.3 KB | 760.7 KB | +20.85% |
| ⚡ | Memory | mem client navigation-churn (vue) |
1.7 MB | 1.5 MB | +12% |
| ⚡ | Simulation | client-nested-params navigation loop (react) |
151.2 ms | 137 ms | +10.33% |
| ⚡ | Memory | mem client navigation-churn (solid) |
649.4 KB | 605.7 KB | +7.21% |
| ⚡ | Memory | mem server error-paths unmatched (react) |
441.5 KB | 413.9 KB | +6.66% |
| ⚡ | Simulation | ssr global-mw document (solid) |
366.2 ms | 353 ms | +3.74% |
| ⚡ | Simulation | ssr server-fn multipart (solid) |
144.3 ms | 139.5 ms | +3.48% |
| ⚡ | Memory | mem server request-churn (solid) |
733.2 KB | 709.4 KB | +3.35% |
| ⚡ | Memory | mem server serialization-payload (solid) |
4.7 MB | 4.5 MB | +3.19% |
| 👁 | Memory | mem server aborted-requests (vue) |
996.3 KB | 1,057.1 KB | -5.75% |
| 👁 | Simulation | client-async-pipeline navigation loop (react) |
61.9 ms | 64.5 ms | -4.02% |
| 👁 | Memory | mem server aborted-requests (solid) |
1.2 MB | 1.2 MB | -4.51% |
| 👁 | Memory | mem server server-fn-churn (solid) |
336.7 KB | 360.2 KB | -6.54% |
| 👁 | Memory | mem server aborted-requests (react) |
844.5 KB | 943.3 KB | -10.48% |
| 👁 | Memory | mem server error-paths error (react) |
985 KB | 1,019.3 KB | -3.36% |
| 👁 | Memory | mem server error-paths not-found (react) |
436.4 KB | 456.7 KB | -4.45% |
| 👁 | Memory | mem server error-paths redirect (react) |
315.9 KB | 325.9 KB | -3.07% |
| 👁 | Memory | mem server request-churn (react) |
639.3 KB | 688.9 KB | -7.21% |
| ... | ... | ... | ... | ... | ... |
ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.
Tip
Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.
Comparing fix/await-current-shared-settle (8c76f33) with main (37877da)
…e chain `awaitCurrent` looped `await current.done` and re-read `router._tx` after each completion. Under back-to-back navigations (anything that starts the next navigation in the same tick as `onRendered`, including the client-nav benchmarks) every superseded waiter woke on every successor, so a burst of N navigations cost N²/2 microtask iterations and kept every superseded transaction alive until the router went idle. Each transaction's `done` promise now follows its current successor, so every waiter on a transaction shares the same chain instead of polling successors independently. A waiter still resolves once the router has settled on a transaction other than its owner. Measured in benchmarks/client-nav (react, navigations per 10s, jsdom): baseline 15.4K -> 43K, route-tree-scale 17.9K -> 98K, search-params 16.5K -> 55K. The instrumented baseline run showed 125.8M loop iterations for 15,867 navigations before the change. Bundle: react-router.minimal -1 B gzip vs main. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C1tX2n8xegVBsZqoJPu7iv
11997c8 to
8c76f33
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Summary
awaitCurrentinload-client.tsloopedawait current.doneand re-readrouter._txafter each completion. Under back-to-back navigations (anything that starts the next navigation in the same tick asonRendered, which includes thebenchmarks/client-navharness) every superseded waiter woke on every successor. A burst of N navigations cost N²/2 microtask iterations and kept every superseded transaction (matches, closures, controllers) alive until the router went idle.Each transaction's
donepromise now follows its current successor, so every waiter on a transaction shares one chain instead of polling successors independently. Semantics are unchanged: a waiter resolves once the router has settled on a transaction other than its owner. A new test pins that contract for a same-tick burst.Evidence
Instrumenting the built baseline benchmark bundle before the change: 15,867 navigations, 125,857,045
awaitCurrentloop iterations (= N²/2). In the CPU profiles this shows up asawaitCurrentat 20 to 42 percent self time plus(program)at 30 to 44 percent in the baseline, search-params, route-tree-scale and nested-params scenarios. Loaders and mount are unaffected because their timer hops let the chain settle.Navigations per 10 seconds in
benchmarks/client-nav(react, jsdom, same harness), measured by patching the built bundle:Real-world impact is bounded to navigation bursts within a tick, but the retained transactions were a leak until idle, and the CodSpeed client-nav numbers were dominated by this, so any perf work benchmarked after this lands should be re-baselined.
Test plan
packages/router-core: vitest (107 files, 1613 passed), tsc, eslintpackages/react-router: vitest (77 files, 1037 passed)navigation-burst-settle.test.ts🤖 Generated with Claude Code
https://claude.ai/code/session_01C1tX2n8xegVBsZqoJPu7iv
Summary by CodeRabbit