Uh oh!
There was an error while loading. Please reload this page.
bench(memory): settle-until-quiescent GC barrier between iterations - #7739
Merged
Conversation
The fixed-turn settle before the pinned collection was hardware-fragile: teardown needing one more event-loop turn on a given runner leaked a whole payload of garbage past the collection point and flipped the measured peak bimodally (serialization-payload solid/react resurfacing the pre-#7730 6.2MB attractor on some CI runners). Replace it with an adaptive barrier that hops and collects until the post-GC heap size stops moving, with a minimum-turn floor matching the fixed count it replaces (React's stream teardown spans several turns and races a shorter floor). Also pin collections in request-churn and the aborted-requests drain: their peak floated on GC scheduling even with bit-identical work (18% spread on CI for solid churn). Reachable accumulation stays visible to the metric - a forced collection cannot reclaim leaked or cached objects, it only removes floating garbage, whose collection timing was the dominant cross-run noise source. Verified with a synthetic reachable leak: +4.7% peak over a 0.6% A/A noise floor, where the old 18% noise would have swallowed it. Local A/A spreads (3x codspeed run per suite): solid <=1%, react <=2.2% except request-churn (rare +130KB mode, framework teardown variance), vue <=2.3% except aborted-requests (~8%, intra-iteration work variance, down from 21% on CI). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors memory benchmark GC settling into a shared async helper, enables GC pinning in several sequential workloads, reduces iteration counts in multiple benchmark scenarios, and adds runtime flags to benchmark Vitest configs. ChangesGC pinning and benchmark tuning
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
🚀 Changeset Version PreviewNo changeset entries found. Merging this PR will not cause a version bump for any packages. |
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. |
Contributor
View your CI Pipeline Execution ↗ for commit 1517be5
☁️ Nx Cloud last updated this comment at |
Merging this PR will improve performance by 39.28%
|
| Mode | Benchmark | BASE | HEAD | Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Memory | mem error-paths unmatched (react) | 1,117.5 KB | 300.3 KB | ×3.7 |
| ⚡ | Memory | mem error-paths not-found (react) | 942.4 KB | 266.7 KB | ×3.5 |
| ⚡ | Memory | mem request-churn (react) | 1,622 KB | 497.5 KB | ×3.3 |
| ⚡ | Memory | mem request-churn (solid) | 1,134.2 KB | 417.3 KB | ×2.7 |
| ⚡ | Memory | mem aborted-requests (react) | 1,324.5 KB | 525.7 KB | ×2.5 |
| ⚡ | Memory | mem error-paths redirect (react) | 470 KB | 211.4 KB | ×2.2 |
| ⚡ | Memory | mem error-paths not-found (vue) | 758.7 KB | 381.3 KB | +99% |
| ⚡ | Memory | mem aborted-requests (vue) | 911.7 KB | 458.9 KB | +98.66% |
| ⚡ | Memory | mem aborted-requests (solid) | 1,846.7 KB | 936.8 KB | +97.13% |
| ⚡ | Memory | mem error-paths not-found (solid) | 737.1 KB | 410.3 KB | +79.62% |
| ⚡ | Memory | mem request-churn (vue) | 1,280 KB | 782.7 KB | +63.53% |
| ⚡ | Memory | mem server-fn-churn (solid) | 394.6 KB | 268.5 KB | +46.96% |
| ⚡ | Memory | mem server-fn-churn (vue) | 394.6 KB | 274.7 KB | +43.68% |
| ⚡ | Memory | mem server-fn-churn (react) | 394 KB | 276.9 KB | +42.31% |
| ⚡ | Memory | mem error-paths unmatched (vue) | 563 KB | 467.5 KB | +20.41% |
| ⚡ | Memory | mem unique-location-churn (solid) | 998.4 KB | 842.6 KB | +18.49% |
| ⚡ | Memory | mem error-paths redirect (solid) | 469.9 KB | 441.2 KB | +6.52% |
| 👁 | Memory | mem error-paths error (solid) | 724.4 KB | 912.9 KB | -20.65% |
| 👁 | Memory | mem error-paths unmatched (solid) | 256.3 KB | 472.6 KB | -45.76% |
| 👁 | Memory | mem peak-large-page (solid) | 760.4 KB | 802.9 KB | -5.29% |
| ... | ... | ... | ... | ... | ... |
ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.
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 memory-bench-settle-barrier (1517be5) with main (208100b)
The adaptive exit (collect until post-GC heap size stops moving) made the collection points land at data-dependent turns on CI: heap readings never fully stabilize there, so the exit point - and every subsequent GC point - shifted between identical runs. The first CI A/A pair showed it flipping serialization-payload (vue) to a 6MB mode (67%), inflating peak-large-page (vue) 3.4x, and adding 7-8% swings to the react/vue abort scenarios, while fixing the solid suite. Every step is now a fixed count: 16 settle turns (enough for React/Vue multi-turn teardown that the original 2-turn barrier raced), one pinned collection, one turn, one more collection for finalizer fallout. Fixed counts make the GC points identical across runs by construction, which is the property the barrier exists to provide. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
On CI (eBPF instrument + --no-opt) one pinned iteration costs ~0.1-0.6s, so the previous counts put request-churn at ~36s measured and the error -paths/server-fn benches at 0.1-0.5s - outside the useful 2-10s range in both directions. Retune: request-churn/aborted-requests/error-paths/ server-fn-churn to 40 iterations, serialization-payload to 12; peak-large-page and streaming-peak already sit in range at 20. The floor is raised by pinning error-paths and server-fn-churn rather than inflating their iteration counts: thousands of unpinned iterations would cross V8's own GC scheduling and reintroduce the timing noise the barrier removes, while the pin's fixed per-iteration cost lifts them into range at 40 iterations and makes their collection points deterministic. Note: fewer churn iterations proportionally shrink the accumulation window, so peak-based leak sensitivity drops accordingly; leaks remain visible in total-allocated (forced compactions re-copy the growing live set every iteration). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The retuned (shorter) runs exposed a recurring V8 internal event - a +7-13MB allocation burst with the signature of bytecode flush + lazy recompile - that lands inside or outside the measured window at random, flipping peaks by whole MBs between identical CI runs (request-churn vue 785KB vs 3.2MB). The pinned collections age code fast enough for V8 to flush unused bytecode mid-run; --no-flush-bytecode keeps compiled code alive for the process lifetime so the measured window sees no recompile bursts. Vite's mergeConfig concatenates the scenario's test.execArgv with the flags the CodSpeed plugin injects, so both apply to the fork workers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
benchmarks/memory/server/scenarios/peak-large-page/react/vite.config.ts (1)
25-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComment references pinned collections that don't apply to this scenario.
The rationale comment says "the pinned collections age code fast enough for V8 to flush unused bytecode," but per the scenario-wiring changes, GC pinning is only added to
aborted-requests,request-churn,error-paths, andserver-fn-churn— notpeak-large-page. This looks like a copy-pasted comment that misattributes the cause here.✏️ Suggested comment fix
- // Keep lazily-compiled code alive for the whole run: the pinned- // collections age code fast enough for V8 to flush unused bytecode,- // and the mid-measurement recompile injects a multi-MB allocation- // burst at a run-dependent time.+ // Keep lazily-compiled code alive for the whole run: V8 may flush+ // unused bytecode mid-run, and the recompile injects a multi-MB+ // allocation burst at a run-dependent time. execArgv: ['--no-flush-bytecode'],🤖 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 `@benchmarks/memory/server/scenarios/peak-large-page/react/vite.config.ts` around lines 25 - 29, The rationale comment on the execArgv setting is misattributing GC pinning to this scenario, since peak-large-page does not use the pinned collections added in the other scenarios. Update the comment near the execArgv ['--no-flush-bytecode'] configuration in vite.config.ts to describe only the actual behavior for peak-large-page, keeping the note about preserving lazily compiled code alive without referring to pinned collections or the other scenario-specific GC wiring.
🤖 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 `@benchmarks/memory/server/scenarios/peak-large-page/react/vite.config.ts`:
- Around line 25-29: The rationale comment on the execArgv setting is
misattributing GC pinning to this scenario, since peak-large-page does not use
the pinned collections added in the other scenarios. Update the comment near the
execArgv ['--no-flush-bytecode'] configuration in vite.config.ts to describe
only the actual behavior for peak-large-page, keeping the note about preserving
lazily compiled code alive without referring to pinned collections or the other
scenario-specific GC wiring.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 566d0502-4751-4171-add6-f7b6c745f781
📒 Files selected for processing (21)
benchmarks/memory/server/scenarios/aborted-requests/react/vite.config.tsbenchmarks/memory/server/scenarios/aborted-requests/solid/vite.config.tsbenchmarks/memory/server/scenarios/aborted-requests/vue/vite.config.tsbenchmarks/memory/server/scenarios/error-paths/react/vite.config.tsbenchmarks/memory/server/scenarios/error-paths/solid/vite.config.tsbenchmarks/memory/server/scenarios/error-paths/vue/vite.config.tsbenchmarks/memory/server/scenarios/peak-large-page/react/vite.config.tsbenchmarks/memory/server/scenarios/peak-large-page/solid/vite.config.tsbenchmarks/memory/server/scenarios/peak-large-page/vue/vite.config.tsbenchmarks/memory/server/scenarios/request-churn/react/vite.config.tsbenchmarks/memory/server/scenarios/request-churn/solid/vite.config.tsbenchmarks/memory/server/scenarios/request-churn/vue/vite.config.tsbenchmarks/memory/server/scenarios/serialization-payload/react/vite.config.tsbenchmarks/memory/server/scenarios/serialization-payload/solid/vite.config.tsbenchmarks/memory/server/scenarios/serialization-payload/vue/vite.config.tsbenchmarks/memory/server/scenarios/server-fn-churn/react/vite.config.tsbenchmarks/memory/server/scenarios/server-fn-churn/solid/vite.config.tsbenchmarks/memory/server/scenarios/server-fn-churn/vue/vite.config.tsbenchmarks/memory/server/scenarios/streaming-peak/react/vite.config.tsbenchmarks/memory/server/scenarios/streaming-peak/solid/vite.config.tsbenchmarks/memory/server/scenarios/streaming-peak/vue/vite.config.ts
--no-flush-bytecode alone did not stop the run-to-run flips: the recurring event is heap-space growth, which allocates several MB at a run-dependent moment inside the shortened measured windows (error-paths unmatched vue 674KB vs 2MB, serialization-payload solid resurfacing its 6.4MB attractor on one run in three). Pre-sizing old space and pinning the semi-space size removes mid-measurement growth while leaving the per-iteration page map/unmap signal intact - local runs confirm peaks still track payload sizes exactly (serialization 3.5MB, streaming 30.3MB, levels unchanged). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
benchmarks/memory/server/scenarios/aborted-requests/solid/vite.config.ts (1)
29-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared
execArgvblock to avoid drift across ~20 configs.This identical array (flags + comments) is duplicated verbatim across at least 20 scenario vite.config.ts files in this PR. Any future tuning of the heap-sizing values will require touching every file consistently, risking drift.
Consider extracting to a shared constant in
benchmarks/memory/server/bench-utils.ts(or a dedicated config module) and importing it into each scenario config.♻️ Example extraction
// benchmarks/memory/server/bench-execargv.tsexportconststableHeapExecArgv=['--no-flush-bytecode','--initial-old-space-size=64','--min-semi-space-size=16','--max-semi-space-size=16',];- execArgv: [- '--no-flush-bytecode',- '--initial-old-space-size=64',- '--min-semi-space-size=16',- '--max-semi-space-size=16',- ],+ execArgv: stableHeapExecArgv,🤖 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 `@benchmarks/memory/server/scenarios/aborted-requests/solid/vite.config.ts` around lines 29 - 37, The execArgv array is duplicated across many scenario vite.config.ts files, which will cause config drift when the heap flags change. Extract the shared flags into a single exported constant in a common module such as bench-utils.ts or a dedicated bench-execargv.ts, then import and reuse it from this Vite config and the other scenario configs that currently inline the same block.benchmarks/memory/server/scenarios/peak-large-page/react/vite.config.ts (1)
29-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
execArgvblock across ~20 configs — extract to a shared constant.The same 4-flag array (plus comments) is repeated verbatim in every scenario's
vite.config.ts(react/solid/vue × 7 scenarios). Centralizing this in a shared module (e.g.,benchmarks/memory/server/bench-execargv.ts) would avoid drift if flag values are re-tuned later, and matches the PR's own instinct to keep GC-settling logic in a shared helper.♻️ Suggested extraction
// benchmarks/memory/server/bench-execargv.tsexportconstmemoryBenchExecArgv=['--no-flush-bytecode','--initial-old-space-size=64','--min-semi-space-size=16','--max-semi-space-size=16',]+import { memoryBenchExecArgv } from '../../../bench-execargv' ... - execArgv: [- '--no-flush-bytecode',- // Pre-size the V8 heap so no space has to grow mid-measurement:- // heap-growth events allocate several MB at a run-dependent moment,- // which flips the measured peak bimodally between identical runs.- '--initial-old-space-size=64',- '--min-semi-space-size=16',- '--max-semi-space-size=16',- ],+ execArgv: memoryBenchExecArgv,🤖 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 `@benchmarks/memory/server/scenarios/peak-large-page/react/vite.config.ts` around lines 29 - 37, The repeated execArgv flag array in the scenario Vite configs should be centralized into a shared constant to prevent drift across the many duplicated files. Extract the common 4-flag setup from the scenario-specific vite.config.ts files into a shared module such as bench-execargv.ts, then import and reuse it from each config; keep the unique scenario wiring in place while referencing the shared memoryBenchExecArgv symbol.
🤖 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 `@benchmarks/memory/server/scenarios/aborted-requests/solid/vite.config.ts`:
- Around line 29-37: The execArgv array is duplicated across many scenario
vite.config.ts files, which will cause config drift when the heap flags change.
Extract the shared flags into a single exported constant in a common module such
as bench-utils.ts or a dedicated bench-execargv.ts, then import and reuse it
from this Vite config and the other scenario configs that currently inline the
same block.
In `@benchmarks/memory/server/scenarios/peak-large-page/react/vite.config.ts`:
- Around line 29-37: The repeated execArgv flag array in the scenario Vite
configs should be centralized into a shared constant to prevent drift across the
many duplicated files. Extract the common 4-flag setup from the
scenario-specific vite.config.ts files into a shared module such as
bench-execargv.ts, then import and reuse it from each config; keep the unique
scenario wiring in place while referencing the shared memoryBenchExecArgv
symbol.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2c668745-5828-4db9-a678-c30c2dffb80e
📒 Files selected for processing (21)
benchmarks/memory/server/scenarios/aborted-requests/react/vite.config.tsbenchmarks/memory/server/scenarios/aborted-requests/solid/vite.config.tsbenchmarks/memory/server/scenarios/aborted-requests/vue/vite.config.tsbenchmarks/memory/server/scenarios/error-paths/react/vite.config.tsbenchmarks/memory/server/scenarios/error-paths/solid/vite.config.tsbenchmarks/memory/server/scenarios/error-paths/vue/vite.config.tsbenchmarks/memory/server/scenarios/peak-large-page/react/vite.config.tsbenchmarks/memory/server/scenarios/peak-large-page/solid/vite.config.tsbenchmarks/memory/server/scenarios/peak-large-page/vue/vite.config.tsbenchmarks/memory/server/scenarios/request-churn/react/vite.config.tsbenchmarks/memory/server/scenarios/request-churn/solid/vite.config.tsbenchmarks/memory/server/scenarios/request-churn/vue/vite.config.tsbenchmarks/memory/server/scenarios/serialization-payload/react/vite.config.tsbenchmarks/memory/server/scenarios/serialization-payload/solid/vite.config.tsbenchmarks/memory/server/scenarios/serialization-payload/vue/vite.config.tsbenchmarks/memory/server/scenarios/server-fn-churn/react/vite.config.tsbenchmarks/memory/server/scenarios/server-fn-churn/solid/vite.config.tsbenchmarks/memory/server/scenarios/server-fn-churn/vue/vite.config.tsbenchmarks/memory/server/scenarios/streaming-peak/react/vite.config.tsbenchmarks/memory/server/scenarios/streaming-peak/solid/vite.config.tsbenchmarks/memory/server/scenarios/streaming-peak/vue/vite.config.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- benchmarks/memory/server/scenarios/aborted-requests/react/vite.config.ts
- benchmarks/memory/server/scenarios/error-paths/react/vite.config.ts
- benchmarks/memory/server/scenarios/serialization-payload/react/vite.config.ts
- benchmarks/memory/server/scenarios/streaming-peak/vue/vite.config.ts
- benchmarks/memory/server/scenarios/request-churn/react/vite.config.ts
error-paths and server-fn-churn sat at 5-8s measured on CI; fewer iterations bring them to ~2.3-3.4s, keeping the 2s floor while shrinking the window a stray allocator event can land in. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The residual CI flips all share one signature: exactly one iteration's payload added to the measured peak (serialization-payload vue 3.5 vs 6.9MB, peak-large-page vue 780 vs 1349KB, error-paths redirect vue 300 vs 885KB). On some runners the response teardown holds the payload past the fixed settle window - released only by a later internal timer - so it survives the pinned collection and bleeds into the next iteration. After the fixed 16-turn settle and collection, compare the post-GC heap against the smallest post-collection size seen this run (the inter- iteration floor): while it has not returned to the floor, keep hopping and collecting, bounded at 64 turns. The barrier stays fixed-count in the common case and extends only in exactly the failure case. Workloads that genuinely accumulate reachable memory raise the floor as they go, so accumulation still measures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The floor-verified barrier fixed its target - the serialization family went to <=0.45% across three CI runs, attractor gone - but destabilized churn/abort scenarios (request-churn solid 420KB vs 3.4MB): their inter- iteration floor legitimately drifts, so the verification capped out chronically and the ~48 extra collections per iteration eventually hit a compaction that double-copies the live set. Split by scenario shape: serialization-payload, peak-large-page and streaming-peak keep the verified barrier (flat floor, proven fix); churn and abort scenarios go back to the plain fixed-count barrier that held them at <=1.7% in the two previous CI rounds. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Uh oh!
There was an error while loading. Please reload this page.
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
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.
Context
After #7730, memory benchmarks still flagged on PRs that never touched
benchmarks/memory/**(observed throughout #7732). Analysis of 7 code-identical CI runs — including a same-commit main A/A pair — showed the survivors:serialization-payload (solid)serialization-payload (react)aborted-requests (vue)request-churn (solid)Root cause: the fixed 2-hop settle before the pinned collection is hardware-fragile. When renderer/stream teardown needs one more event-loop turn on a given runner (the GitHub pool mixes EPYC 9V74 / EPYC 7763 / Xeon 8370C), a whole payload of garbage slips past the collection point, V8 grows the heap, and the measured peak flips bimodally.
Changes
bench-utils.ts): hop one event-loop turn +gc()until the post-GC heap size stops moving (two identical consecutive readings), with a minimum-turn floor of 8 — React's stream teardown spans several turns and races a stability-only exit — and a cap of 16 so a drifting heap cannot stall the run. Smoke runs without--expose-gckeep the fixed timer hops. The barrier settles strictly more than the fixed-count one it replaces, never less.request-churn, theaborted-requestsdrain): their peak floated on GC timing even with bit-identical work. Accumulation signals stay visible — a forced collection cannot reclaim leaked or cached objects, it only removes floating garbage, whose collection timing was the dominant noise source. Verified with a synthetic reachable leak (~1.6 MB over the run): +4.7% peak over the new 0.6% A/A noise floor, a clean flag where the old 18% noise would have swallowed it.Validation (local, 3×
codspeed run --mode memoryper suite, Node 24)request-churn18%→0.8%,serialization-payload78%→0.6%)aborted-requests~8% (down from 21%; residual is intra-iteration work variance in the shell-read path — follow-up)request-churn(framework teardown variance; was bit-stable on CI before, watching the CI A/A here)Expected on this PR
One-time baseline shifts on the pinned benches (e.g.
request-churn (solid)1.2 MB→470 KB,aborted-requestslevels move,totalAllocatedrises from forced compactions). Twoworkflow_dispatchA/A runs are already queued on this branch to measure real CI spreads.🤖 Generated with Claude Code
Summary by CodeRabbit