perf(gc): nursery cap + scavenge on by default — peak RSS -69% - #7377
Merged
Conversation
added 2 commits
August 4, 2026 14:04
Measured -69% RSS at +3% wall over the 8 gc_ratchet probes, 11/11 probes byte-identical to Node, gap suite tracking the 447/19/13 baseline. See #7372 for the full 2x2 and why neither half is worth shipping alone. Blocked on 26 gc::tests failures (baseline flake is 1-4, #7365), concentrated in the budgeted/incremental path that scavenge's deferral bypasses. Includes the one cause already understood: force_legacy_gc_pacing un-capped the trigger by pinning the polls flag, which stops working once the cap is unconditional -- the guard now suppresses the cap directly. Mechanically pinning legacy pacing across incremental_sweep_reclaim fixed only 3 of 10, so the rest need individual judgement. Pushed as a reference for #7372, not for merge.
799,604,736 -> 245,055,488 bytes across the 8 gc_ratchet probes at +2%
wall, all 8 byte-identical to Node.
Measured as a 2x2, because the one-armed version of this measurement is
misleading:
no scavenge scavenge
no cap baseline +0% RSS
cap 16 MB -33%/+23% -69%/+3%
Scavenge alone moves nothing; the cap alone trades a third of the
footprint for a quarter of the wall time. Together the cap makes
collections frequent and scavenge makes them evacuating (O(live) copying)
instead of O(heap) sweeps, so the frequency is cheap. #7056 measured the
cap and recommended decoupling it -- but it was gated behind two knobs
that both defaulted OFF, so it had never been active in a shipped build,
and following that recommendation literally ships the bad arm.
Enabling scavenge also defers alloc-point collections to a precise
safepoint instead of collecting behind a forced conservative scan, which
became reasonable only when #7370 made native roots the default.
TEST WORK, which was the real cost: this first produced 23 gc:: failures.
10 of them were one bug -- force_legacy_gc_pacing() pinned only the
moving-loop-polls flag, which used to be enough because the cap and the
deferral branch both hung off it. With the cap unconditional and scavenge
default-on the guard silently stopped pinning anything, so tests that
correctly declared their pacing mode were running in the wrong one. It
now pins all three. The other 13 drive the budgeted/incremental stepper
without any guard; they pin it explicitly now, since the shipped default
bypasses that path by design.
Remaining suite variance (3-4 failures) is the pre-existing flake in
#7365 -- clean main gives 1/2/2/3/4 on the same runs.
📝 WalkthroughWalkthroughThe PR enables the 16 MB nursery cap and evacuating scavenge by default. It adds environment and test overrides, updates pacing guards, pins legacy pacing in affected GC tests, and documents the behavior and measurements. ChangesGC default pacing
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
crates/perry-runtime/src/gc/mod.rs (1)
308-340: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an OFF-state CI arm for
PERRY_GC_SCAVENGE.
PERRY_GC_SCAVENGEnow defaults on at runtime, but the GC workflows do not carryPERRY_GC_SCAVENGE=0. Keep the production rollback path tested through a GC suite arm, or reclassify this as an untested knob per the GC environment-knob guideline.🤖 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 `@crates/perry-runtime/src/gc/mod.rs` around lines 308 - 340, The gc_scavenge_enabled function now defaults scavenge to ON, but the GC test workflows do not currently test the OFF state with PERRY_GC_SCAVENGE=0. Add a CI workflow arm that runs the GC test suite with the PERRY_GC_SCAVENGE=0 environment variable set to ensure the fallback code path and production rollback scenario remain tested and functional.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.
Inline comments:
In `@changelog.d/7377-nursery-cap-and-scavenge-default.md`:
- Around line 3-4: Update the rollback description in the changelog entry to
state that PERRY_GC_SCAVENGE=0 disables scavenging only; do not claim it reverts
the nursery cap, since effective_next_arena_trigger() continues applying that
cap in production.
- Around line 9-18: Make the cap-plus-scavenge wall-time result consistent in
the benchmark table and the accompanying summary text, using the single recorded
probe value; update either the “+3% wall” table entry or the “+2% wall” value in
the paragraph while preserving the reported RSS and byte results.
In `@crates/perry-runtime/src/gc/tests/teardown.rs`:
- Around line 5-9: Move the force_legacy_gc_pacing guard and its explanatory
comment into the worker-thread closure created near this test’s thread setup, so
the allocation probe uses the legacy pacing override. If that worker does not
need legacy pacing, remove both the guard and comment instead.
---
Nitpick comments:
In `@crates/perry-runtime/src/gc/mod.rs`:
- Around line 308-340: The gc_scavenge_enabled function now defaults scavenge to
ON, but the GC test workflows do not currently test the OFF state with
PERRY_GC_SCAVENGE=0. Add a CI workflow arm that runs the GC test suite with the
PERRY_GC_SCAVENGE=0 environment variable set to ensure the fallback code path
and production rollback scenario remain tested and functional.
🪄 Autofix
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 Plus
Run ID: 44729e78-037b-4915-a5ce-a60476ca1f71
📒 Files selected for processing (8)
changelog.d/7377-nursery-cap-and-scavenge-default.mdcrates/perry-runtime/src/gc/mod.rscrates/perry-runtime/src/gc/policy.rscrates/perry-runtime/src/gc/tests/debt_pacer.rscrates/perry-runtime/src/gc/tests/incremental_sweep_reclaim.rscrates/perry-runtime/src/gc/tests/root_words.rscrates/perry-runtime/src/gc/tests/teardown.rscrates/perry-runtime/src/gc/tests/temp_roots.rs
Comment on lines
+3
to
+4
| - **Peak RSS drops ~69%.** The 16 MB nursery cap and the evacuating scavenge are | ||
| now on by default; `PERRY_GC_SCAVENGE=0` reverts both for bisection. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the rollback description.
PERRY_GC_SCAVENGE=0 disables scavenging only. effective_next_arena_trigger() still applies the nursery cap in production. Do not state that this setting reverts both controls.
🤖 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 `@changelog.d/7377-nursery-cap-and-scavenge-default.md` around lines 3 - 4,
Update the rollback description in the changelog entry to state that
PERRY_GC_SCAVENGE=0 disables scavenging only; do not claim it reverts the
nursery cap, since effective_next_arena_trigger() continues applying that cap in
production.
Comment on lines
+9
to
+18
| | | no scavenge | scavenge | | ||
| |---|---:|---:| | ||
| | **no cap** | baseline | +0% RSS, +2% wall | | ||
| | **cap 16 MB** | −33% RSS, **+23% wall** | **−69% RSS, +3% wall** | | ||
|
|
||
| Scavenge alone moves nothing. The cap alone trades a third of the footprint | ||
| for a quarter of the wall time. Together the cap makes collections frequent | ||
| and scavenge makes them *evacuating* (O(live) copying) rather than O(heap) | ||
| sweeps, so the frequency is cheap: **799,604,736 → 245,055,488 bytes at +2% | ||
| wall**, all 8 probes byte-identical to Node. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use one wall-time result.
The table reports +3% wall for cap plus scavenge. Line 18 reports +2% wall for the same configuration. Update one value from the recorded probe result.
🤖 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 `@changelog.d/7377-nursery-cap-and-scavenge-default.md` around lines 9 - 18,
Make the cap-plus-scavenge wall-time result consistent in the benchmark table
and the accompanying summary text, using the single recorded probe value; update
either the “+3% wall” table entry or the “+2% wall” value in the paragraph while
preserving the reported RSS and byte results.
Comment on lines
+5
to
+9
| // #7056: drives the BUDGETED stepper via `complete_budgeted_gc_cycle`, | ||
| // which the shipped default bypasses (scavenge defers alloc-point | ||
| // collections to a precise safepoint). Pin legacy pacing so the cycle | ||
| // actually starts and this keeps testing what it was written for. | ||
| let _legacy_pacing = crate::gc::policy::force_legacy_gc_pacing(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Create the pacing guard in the worker thread.
force_legacy_gc_pacing() writes thread-local overrides. The allocation probe runs in the thread created at Line 13, so this parent-thread guard does not change its pacing mode. Create the guard inside the closure if the worker must use legacy pacing. Otherwise, remove this guard and its comment.
🤖 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 `@crates/perry-runtime/src/gc/tests/teardown.rs` around lines 5 - 9, Move the
force_legacy_gc_pacing guard and its explanatory comment into the worker-thread
closure created near this test’s thread setup, so the allocation probe uses the
legacy pacing override. If that worker does not need legacy pacing, remove both
the guard and comment instead.
proggeramlug
added a commit
that referenced
this pull request
Aug 4, 2026
* docs(plan): fold in the 2026-08-04 findings Two things this plan treated as measured were not. Statepoints could not compile on aarch64-ELF at all -- a hard failure on a default-on path, from two stacked bugs (#7390: the compact stack-map parser did not model GNU-as `sym = expr`, emitted only at -O3 and only on ELF; and the assembler was not told the -mcpu the code generator was told, so Graviton-emitted SVE was rejected) behind two toolchain ones (#7384, #7388). And three of the four RS4GC matrix arms had NEVER executed, in any run, for want of a concurrency group (#7393). Every "the ELF arm is the only one red" conclusion rested on arms that never reached a runner. That is a fifth way a gate cannot fail, and it is now written down. Also folded in: nine Layer 3 rooting fixes and the rule they share (ordering, not missing roots; a fault that MOVES is a real fix, one that does not move by a byte was already dead before you rooted it); #7380's type confusion and the `gc_type == GC_TYPE_OBJECT` generalisation; RSS -69% (#7377); and the first honest performance measurement -- two benchmarks that measure nothing (#7395) and the array-store guard's siting cost (#7396). The Layer 1 framing is corrected: lower_exprs_rooted already implements the RFC's proposal for codegen operands, gated on any_later_ref_may_trigger_gc, and all four arms of func_ref.rs use it. So the gap is Layer 3, where #7389 supplies the first structural answer. * docs: name the fragment for its real PR (#7397) --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
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.
Closes #7372. Peak RSS drops ~69% — 799,604,736 → 245,055,488 bytes across the 8
gc_ratchetprobes, at +2% wall, all 8 byte-identical to Node.Neither half is worth shipping alone
Measured as a 2×2, because the one-armed version of this measurement is actively misleading:
Scavenge alone moves nothing. The cap alone trades a third of the footprint for a quarter of the wall time. Together, the cap makes collections frequent and scavenge makes them evacuating (O(live) copying) rather than O(heap) sweeps — so the frequency is cheap instead of expensive.
This is why #7056's recommendation could not be followed literally. It measured the cap correctly and said "decouple it and keep it" — but the cap was gated behind two knobs that both default off, so it had never been active in a shipped build, and enabling just the cap ships the −33%/+23% arm. I built that arm first and the 2×2 caught it.
Enabling scavenge also defers alloc-point collections to a precise safepoint instead of collecting behind a forced conservative scan. That became reasonable only when #7370 made native roots the default, so this change is sequenced after it rather than independent of it.
The test work was the real cost
This first produced 23
gc::test failures. They resolved into two groups, and the first is a latent bug worth naming on its own:force_legacy_gc_pacing()had silently stopped pinning anything (10 failures). It set only the moving-loop-polls flag, which used to be sufficient because the nursery cap and the deferral branch both hung off that flag. Making the cap unconditional and scavenge default-on broke that coupling — so tests that correctly declared their pacing mode were quietly running in the wrong one. The guard now pins all three knobs, andgc_scavenge_enabled()gained a thread-local test override mirroring the polls one.The other 13 drive the budgeted/incremental stepper with no guard at all. The shipped default bypasses that path by design (deferral), so they now pin legacy pacing explicitly. Their subject is the legacy collector; the new default's behaviour is covered by the probe matrix.
Verification
gc::tests: 23 failures → 1, and that one (teardown::map_set_side_allocations_release_on_thread_exit) is in the known flaky family catalogued in cargo-test: perry-runtime's suite fails a different number of tests on every run #7365.perry-runtimesuite over 4 runs: 4/4/3/4 failures, against clean main's 1/2/2/3/4 on the same runs — same band, all cargo-test: perry-runtime's suite fails a different number of tests on every run #7365.Summary by CodeRabbit
New Features
PERRY_GC_SCAVENGE=0,off, orfalsesetting to disable scavenging when needed.Bug Fixes