perf(router-core): reduce replaceEqualDeep allocations - #8411
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughChangesStructural sharing
Priority: ⬇️ Low Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to No concrete current-head defect remains; the PR is ready for normal merge checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 3 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
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. |
|
View your CI Pipeline Execution ↗ for commit 935bfb3
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version Preview3 package(s) bumped directly, 20 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 degrade performance by 50.05%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | mem client interrupted-navigations (solid) |
252.3 KB | 505.2 KB | -50.05% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing codex/replace-equal-deep-child-buffers (935bfb3) with schiller-manuel-perf-replace-equal-deep-split-phase (28220be)
ce00e84 to
935bfb3
Compare
🎯 Changes
Stacked on #8389 (
schiller-manuel-perf-replace-equal-deep-split-phase).Fix structural-sharing cases where the previous value could retain removed symbols, a sparse array could pass validation when a hole and an extra key cancel out, or a built-in with an own null/undefined
constructorcould be treated as a plain object. Signed zero consistently prefers the previous value wherever strict equality matches. Prototype-based classification preserves inherited nullish-constructor compatibility.replaceEqualDeepresolves object children into the private previous-key array. It returnsprevwhen equal, reusesnextwhen its children already contain every resolved value/reference, and otherwise creates a copy. Null-prototype mode controls newly constructed objects; either input can be reused with its original prototype. Arrays scan the equal prefix and reuse the validated next-key buffer for changed results. Neither input is mutated.Getters, cross-realm behavior, ordinary own
__proto__copying, an ownconstructor: Object, non-enumerable array indices, and cyclic result semantics beyond the existing depth limit are outside the requested contract. Auxiliary array metadata, including symbols, remains an existing limitation.Three-way CPU benchmark
Against #8389, #8411 takes 18.1% less time for a changed flat search, 32.7% less for successive ordinary search updates, 48.1% less for the two-call selector sequence, and 29.1–43.3% less for changed long primitive arrays. The costs are 13.1–32.3% more time for equal search scenarios, 50.2% more for equal empty objects, 39.3% more for equal arrays of objects, and 19.8% more for arrays of copied objects with one changed item. The fixed mixed fixture is +2.0%; it is one supplied scenario, not an estimate of production workload frequencies.
Against current main, equal/changed flat search takes 31.3%/38.2% less time, equal arrays of objects 32.6% less, and a long primitive array with the last item changed 34.5% less. The full table retains every regression, including output-consumption timings, and flags the constructor-key behavior mismatch.
6954a370a76251cf933ede95562c968adc4e847b6c269b342a577ea1629c7a9412f1f1345c7a7d81fda9b8e7dc260276b5015361068750d2fba96dd49841e93dc8b3e3961db5218160b2e03a00135107f9df0f93289cf2ba93f16ae9b1f5a8b3a8cd71860a6cea95ce00e84b794626a3e9c548ccd94d3d4738724345aaced5f47a2bcb3107244563407fcbfb83bda205c7c69226b18487f43000fa55All 60 scenarios from
packages/router-core/tests/replace-equal-deep.bench.tsat #8411 were run in one Vitest invocation: each original scenario is onedescribe, containingbench('main'),bench('#8389'), andbench('#8411'). Vitest runs the three entries sequentially in the same worker/process (main, #8389, #8411 order per scenario); they do not compete concurrently for CPU. The original 1,000-iteration callback bodies, selector sequences, mixed fixture and array consumers were retained. Each callback has its own version-specific module and source/helper imports. The full source snapshots are byte-for-byte copies from the pinned commits above.Node v25.8.1, V8 14.1.146.11-node.21, Vitest 4.1.4, Tinybench 2.9.0, Apple M3 Max, macOS 15.7.9 arm64; existing jsdom benchmark environment. One worker; no parallel test files. Each bench received 250 ms warmup, at least 1,000 ms measurement and at least 50 samples. No concurrent agent benchmark/build/test load was run. Timing retains normal GC and scheduling effects.
Numbers below are mean nanoseconds per original inner-loop action, lower is better. Deltas are time changes, so negative is faster. Most actions are one function call. The selector-followed-by-same-reference scenario is two calls per action; consumption scenarios are one complete array sum/reduce per action, with construction outside timing. Raw
hzis 1,000-action callback batches per second. No workload-frequency assumptions or aggregate score are used.Preflight retained the original value/sharing assertions and added value checks for every timed replacement fixture. Returning next and retaining its prototype are permitted. All three passed these checks with the explicitly recorded constructor-key difference below; preflight completed before timing. ‡ Current main classifies the null-prototype record with own
constructor: 'foo'as non-plain and returns incoming next rather than sharing previous. The PR versions classify it as plain and share previous. Thus the constructor-key rows and the mixed case containing that fixture perform different work; main's bailout is not an equivalent sharing optimization. Both changed-selector next reuse and ordinary changed-search next reuse are false on main/#8389, true on #8411, which is permitted behavior and part of the measured optimization.The tables contain function microbenchmarks and downstream array-consumption timings, not browser navigation, allocation, memory or bundle measurements. No new conclusions about those other metrics follow from this run.
Across 180 benches: relative margin of error (RME) 0.02–1.52%; sample counts 50–169,758. Each sample contains 1,000 original loop actions. 0 benches have RME >3%; 1 have p999 >5× median.
Long-tail rows:
RME and percentiles describe within-run samples; they do not establish machine-to-machine or between-run reproducibility. Small deltas should not be generalized beyond this run. With fewer than 1,000 samples, p999 is effectively an observed extreme, not a well-resolved estimate of a population tail.
All 60 scenarios, paired in the same run
Focused repeat: fractional array reduction
The first run's #8389 fractional-array reduce had p999 6,099.3 ns versus median 636.4 ns, so this one scenario was repeated with all three implementations together in one process, reversing registration order to #8411, #8389, main. Warmup increased to 500 ms, measurement to at least 3,000 ms, and minimum samples to 100. No source, fixtures or timed callback bodies changed. The full table above retains every original-run mean; these repeat results are separate.
In the repeat, #8411 time was -0.7% versus main and -2.0% versus #8389. The original small regression versus main (+2.9%) did not persist; this consumer is inconclusive across runs. The outlier cause was not profiled.
Allocation and application bundle size
These are the previously completed measurements of the exact current source against #8389, separate from the three-way CPU run above. Allocation and bundle size for current
mainwere not measured in this comparison.Measured complete
utils.tsSHA-256:aaced5f47a2bcb3107244563407fcbfb83bda205c7c69226b18487f43000fa55. Fixed #8389 baseline:068750d2fba96dd49841e93dc8b3e3961db52181.Changed ordinary search in null-prototype mode allocates 1,201 → 761 V8 heap bytes with a null-prototype previous value, or 793 → 353 bytes with an ordinary previous value. Additional retained output falls from about 280 → ≈0 bytes with both inputs already retained. This is not total application memory or RSS; allocation includes output, and retention is measured after full GC in three fresh processes. Equal records and copied-object arrays still allocate more temporary heap than #8389.
All 18 application scenarios were rebuilt against #8389. Total gzip increases by 48–85 bytes:
Initial gzip changes by +48–84 B, raw JS by +62–88 B, and Brotli by −84 to +146 B. JavaScript-file counts are unchanged. The extra bytes accompany the correctness fixes and changed-object/array allocation savings.
Validation
The CPU comparison above is new. Previously completed on the current production source:
✅ Checklist
🚀 Release Impact
Summary by CodeRabbit
Performance
Bug Fixes
Testing