fix(gc): restore evacuation at precise safepoints — the pacing half of #7682 - #7690
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe change updates moving-loop poll and nursery-cap documentation, gates nursery-cap enforcement on evacuating minors, revises GC tests, and records routing, benchmark, witness, and release information. ChangesGC policy and validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 4
🤖 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/7690-polls-and-nursery-cap.md`:
- Line 77: The changelog fragment must not leave the gc-ratchet baseline update
ambiguous: either regenerate and record the pre-change evacuation-accounting
baseline, or explicitly reword the entry to identify it as a follow-up outside
this change.
In `@crates/perry-runtime/src/gc/mod.rs`:
- Around line 350-361: Update the allocation-point comment near gc_check_trigger
to reflect that gc_moving_loop_polls_enabled() is enabled by default and
allocating loops use the poll-enabled route. Qualify the statement that the
alloc-point minor runs immediately so it applies only when polling is explicitly
disabled; leave the surrounding kill-policy discussion unchanged.
- Around line 350-361: Correct the explanatory comment near gc_check_trigger to
accurately describe the interaction between PERRY_GC_SCAVENGE,
PERRY_GC_INCREMENTAL, and the mutable/FFI-mutable scanner predicates. Do not
claim registered_root_scanners_block_budgeted_gc() holds for every compiled
binary or that scavenge is nearly inert; document that disabling scavenge with
moving-loop polls off can route nursery triggers to the budgeted stepper and
make alloc-point scavenge a meaningful pacing choice.
In
`@crates/perry-runtime/src/gc/tests/runtime_roots/generator_attach_prototype.rs`:
- Around line 173-179: Update generator_attach_prototype.rs at lines 173-179 to
retain this allocation-point witness as an explicit
GC_MOVING_LOOP_POLLS_TEST_OVERRIDE=false kill-switch test, renaming it if needed
to describe that mode. At lines 202-208, add or rework a separate witness using
shipped-default pacing with moving-loop polls enabled, triggering relocation at
a precise loop safepoint; preserve the existing allocation-point behavior in the
kill-switch test.
🪄 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: 832fa3ba-e622-4819-b4af-44c73145cac4
📒 Files selected for processing (7)
changelog.d/7690-polls-and-nursery-cap.mdcrates/perry-codegen/src/stmt/loops.rscrates/perry-runtime/src/gc/mod.rscrates/perry-runtime/src/gc/policy.rscrates/perry-runtime/src/gc/tests/runtime_roots/generator_attach_prototype.rscrates/perry-runtime/src/gc/tests/triggers.rsscripts/gc_repsel_matrix.sh
The
|
Parts 1 and 3 are right. Part 2 silently reverts #7669's 34×.First, confirming the premise, because it is mine to own: #7687 alone is a severe regression and I shipped it. On this host And the safepoint number is the strongest thing in the PR:
"Polls off" really does mean "never collect precisely at all", and after #7687 that is the more dangerous state. The blockerThree codegen tests fail on this branch and pass on Measured consequence on #7480's kernel (200k × 50, identical checksums):
6.7×. The clone blocks are still emitted — 56 on both arms — but the fast arm now carries 4 Why the obvious repair is unsound, and what I think is rightThe tempting fix is to teach the admission that a back-edge poll is not a "GC-unsafe call" for its purposes. That is unsound, and
A poll can collect, a collection can move the array, and the fast clone holds a base derived in the preheader. That is the #7660 growth-forwarding SIGBUS shape with a different trigger — and #7660 was three days ago. The sound repair is to emit the poll per-arm rather than per-loop. If that turns out to be more than a follow-up, splitting is fine: parts 1 and 3 stand alone and fix the regression I shipped. Worth measuring whether the cap fix alone recovers most of the 9.1× — that is one build, and if it does, part 2 can land on its own schedule with the per-arm poll. Not yours
Everything else I checked
Re-pinning those two on |
Measured: part 2 is a net negative on both benchmarks — the cap fix does all the workI built the "parts 1+3 only" configuration (this branch, polls forced off at both compile and run time, so the cap fix and the doc/test changes are live and the default flip is not):
Two things fall out, and the first one I did not expect:
So the split is not a scheduling compromise — parts 1 and 3 are strictly better than the whole PR on both measurements, and they fix the regression I shipped with #7687. Checksums identical in every cell ( This does not weaken the argument for pollsThe precision case stands and I still find it convincing: 0 declared safepoints with polls off, 38 with them on, and after #7687 "polls off" genuinely means "never collect precisely at all". That is a real hole and it should be closed. But it should be closed with the per-arm poll emission, not before it. Suggested shape: land parts 1 + 3 now as the #7687 regression fix — they need no new soundness argument and they are the better configuration today. Take part 2 as its own PR with the per-arm emission, where its cost can be measured against a fast clone that still fires. Happy to re-audit either half as soon as it is up. |
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 `@docs/src/internals/gc-rooting-invariant.md`:
- Line 27: Update the back-edge poll default-state description in the
documentation to say that default runs cover this bug class only when execution
reaches an allocating loop with an emitted poll, and explicitly state that
PERRY_GC_MOVING_LOOP_POLLS=0 disables that coverage. Keep the Line 27 default-on
description consistent.
🪄 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: 9883ae43-abc1-4d68-9754-cc6f4a13e7ab
📒 Files selected for processing (3)
CLAUDE.mdcrates/perry-runtime/src/gc/mod.rsdocs/src/internals/gc-rooting-invariant.md
CodeRabbit's Major finding, and it is right: force_shipped_default_gc_pacing() pins polls OFF, which stopped being the shipped default in the same PR that introduced the guard. Every test naming it was claiming to assert the default while asserting the kill switch. - renamed to force_alloc_point_minor_pacing() and documented as the PERRY_GC_MOVING_LOOP_POLLS=0 configuration it selects; - the three tests that use it renamed to say so, and the #7682 regression test keeps its assertion: '=0' is supported, and moving the collection elsewhere by default is no reason to let the alloc-point minor relocate when a user turns that route off; - added the_shipped_default_defers_the_trigger_out_of_the_callees_window, the default-paced witness the review asked for, in the only non-vacuous form available: under the default there is no collection inside the callee to relocate anything, so it asserts the routing that removed it (no collection + GC_SAFEPOINT_PENDING set, which is also its live-subject check). Also: scoped the gc-ratchet baseline note as explicitly out of scope rather than leaving it ambiguous, and fixed the second stale 'the poll is off by default' claim in the rooting-invariant doc (line 27 was corrected in 2f0fe92, lines 53-54 were not).
Review adjustments pushed (
|
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 `@docs/src/internals/gc-rooting-invariant.md`:
- Around line 53-58: Update the default coverage sentence in the back-edge poll
discussion to state that coverage occurs only when execution reaches a
potentially allocating loop whose lowering path emits a poll. Explicitly exclude
specialized or versioned for, for-of, and for-in lowering paths from the claim,
while preserving the existing alloc-free-loop and PERRY_GC_MOVING_LOOP_POLLS=0
statements.
🪄 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: fcdbdc2e-85db-4c67-ae74-8cd18e51011c
📒 Files selected for processing (5)
changelog.d/7690-polls-and-nursery-cap.mdcrates/perry-runtime/src/gc/policy.rscrates/perry-runtime/src/gc/tests/runtime_roots/generator_attach_prototype.rscrates/perry-runtime/src/gc/tests/scan_fallback.rsdocs/src/internals/gc-rooting-invariant.md
🚧 Files skipped from review as they are similar to previous changes (2)
- changelog.d/7690-polls-and-nursery-cap.md
- crates/perry-runtime/src/gc/policy.rs
…cuate The cap's basis is copying_from_space_in_use_bytes(), which a NON-MOVING minor does not reduce — it sweeps in place and from-space stays occupied. So once #7682 forced the alloc-point minor non-moving, a capped trigger was due again on the very next block: one whole-arena collection per 1 MB allocated. Measured on the quiet host, test_gap_gc_index_get_receiver_rooting went 0.66s -> 6.6s, and 0.13s with the cap lifted — a livelock, not the '+23% wall for -33% RSS' the cap-only cell of #7056's 2x2 measured (every collection there still evacuated). Restores the pre-#7056 gating on gc_moving_loop_polls_enabled, so the cap returns automatically, and in the configuration it was measured in, whenever that flag goes default-ON again. (cherry picked from commit bc06b69)
(cherry picked from commit f39ec36)
(cherry picked from commit d5d8409)
…guard (cherry picked from commit 514b6e9)
…retired) Follow-up to #7687, which landed only the first of three changes. Both conditions #7161 named for putting this back are met: its correctness reason closed with #7154 on 2026-08-01, and its codegen-quality reason is discharged by its own stated condition — emit_gc_loop_safepoint already consults loop_purity::loop_may_allocate, so vectorizable loops stay call-free. After #7687 leaving it off is the more dangerous state. Nursery pressure has exactly two precise collection points, this poll and the microtask-pump boundary, and a compute-only program reaches neither with polls off — so every nursery collection lands at the register-imprecise alloc point, which #7687 correctly refuses to let move. 'Polls off' does not mean 'collect later, precisely'; it means 'never collect precisely at all'. Also repairs the two #7577 generator witnesses: they inject their collection at an alloc point, which now neither moves (#7687's guard) nor happens there (the deferral), so both failed on their own live-subject assertion. They pin shipped-default pacing plus a scan override and assert the same invariant.
…iant doc Lost in the cherry-pick onto main (the commit carrying them conflicted on two unrelated files and was re-applied code-only). Both statements would be false the moment this branch lands, which is the exact defect class the branch is about. Also corrects this PR's own earlier draft of the PERRY_GC_SCAVENGE kill-policy note, which claimed the knob was near-inert on the strength of a disjunct that does not hold under the default incremental stepper.
CodeRabbit's Major finding, and it is right: force_shipped_default_gc_pacing() pins polls OFF, which stopped being the shipped default in the same PR that introduced the guard. Every test naming it was claiming to assert the default while asserting the kill switch. - renamed to force_alloc_point_minor_pacing() and documented as the PERRY_GC_MOVING_LOOP_POLLS=0 configuration it selects; - the three tests that use it renamed to say so, and the #7682 regression test keeps its assertion: '=0' is supported, and moving the collection elsewhere by default is no reason to let the alloc-point minor relocate when a user turns that route off; - added the_shipped_default_defers_the_trigger_out_of_the_callees_window, the default-paced witness the review asked for, in the only non-vacuous form available: under the default there is no collection inside the callee to relocate anything, so it asserts the routing that removed it (no collection + GC_SAFEPOINT_PENDING set, which is also its live-subject check). Also: scoped the gc-ratchet baseline note as explicitly out of scope rather than leaving it ambiguous, and fixed the second stale 'the poll is off by default' claim in the rooting-invariant doc (line 27 was corrected in 2f0fe92, lines 53-54 were not).
The flip costs 6.6x on #7480's kernel -- a poll is a call, so the element-shape fast clone's call-free admission declines and control falls to the slow arm -- and it makes its own regression test 2.4x slower (0.32s -> 0.76s). The nursery-cap fix alone takes index_get from 6.94s to 0.11s with #7480 unchanged. Part 2 wants per-arm poll emission, which is a separate change. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
6a67940 to
3789b24
Compare
Merging, reduced to the nursery-cap fix. I made the change rather than asking for it.The newest commits ( Final measurement, all on one build:
Item 6 is genuinely untouched, not just "close": five runs each, fix
What I changed and whyReverted the default on both sides ( Kept everything else: the The precision argument is not withdrawn0 declared safepoints with polls off against 38 with them on is real, and after #7687 "polls off" does mean "never collect precisely at all". That should land — with per-arm emission. The shortcut of teaching the admission to ignore polls is unsound, and I have recorded all of this in the changelog fragment so the follow-up starts from the measurement rather than from the argument. Gates: 24/24 lint, |
1 participant
Follow-up to #7687, which landed only the first of three changes.
#7687's guard is correct — an allocation-point collection may not MOVE anything,
because that program point is described by neither root lowering. On its own it
is also not shippable, and
mainis in that state right now:test_gap_gc_index_get_receiver_rootingwent 0.66 s → 6.6 s on main. Gaptests near the harness's 10 s
PERRY_RUN_TIMEOUTcan now be recorded ascrash— that is how #7687's own probe first failed, before it was resized.1. The nursery cap applies only when the minor can evacuate
The cap's basis is
copying_from_space_in_use_bytes(), which a non-movingminor never reduces — it sweeps in place and from-space stays occupied. A capped
trigger firing a non-moving minor is therefore due again on the very next block:
one whole-arena collection per 1 MB allocated. Confirmed with no rebuild at all,
via the tuning dial —
PERRY_GC_SCAVENGE_NURSERY_MB=4096takes that test to0.13 s. Same shape as #7592.
#7056's own 2x2 already said the cap and the evacuating minor "ship together,
because either alone is a bad trade". That was advisory;
nursery_cap_activemakes it load-bearing.
2. Back-edge polls default-ON (#7161's stopgap retired)
Both conditions #7161 named are met:
and that class now has a static gate whose allowlist is empty.
back-edge … until the poll is emitted only in loops that actually
ALLOCATE". It already is:
emit_gc_loop_safepointconsultsloop_purity::loop_may_allocate. Only the doc comment above the flag nevergot updated.
And after #7687, leaving it off is the more dangerous state. Nursery pressure
has exactly two precise collection points — this poll and the microtask-pump
boundary — and a compute-only program reaches neither with polls off. "Polls
off" never meant "collect later, precisely"; it meant "never collect precisely
at all".
The flip is less of a leap than it looks:
gc-moving-witnessesrunsgc_repsel_matrix.sh --arms loop_polls --filter test_gap_gc_on every PR, andloop_pollsis the configuration this makes default. That 56-file corpushas been gated in exactly this mode all along, with UNVER rejected as hard as
FAIL.
It also makes an assertion already on
maintrue again:test-parity/gc_matrix_inert_arms.txtno longer listsdefaultas known-inert,i.e.
maincurrently claims the shipped default relocates while polls are off.3. The two #7577 witnesses
generator_attach_prototype's pair inject their collection at an allocationpoint, which after #7687 neither moves nor — with polls on — happens there at
all. Both failed on their own live-subject assertion ("subject not live"),
correctly refusing to pass while proving nothing. They now pin
force_shipped_default_gc_pacing()plus a scan override, and assert exactly whatthey asserted before: a runtime helper must not bind a receiver's address
across its own allocation.
It has to be that guard and not
force_legacy_gc_pacing(), which also turnsscavenge off — and scavenge is the disjunct that routes nursery pressure to the
direct arm at all (
registered_root_scanners_block_budgeted_gc()reduces to"any COPY-ONLY scanner" under
gc_incremental_enabled(), and that registry holdsonly a mutable one). With scavenge off the trigger goes to the budgeted stepper,
which is non-moving by construction — a third route to the identical message.
All three are recorded in the file.
Verification
cargo test -p perry-runtime --libiso_FIB/ gap probe, default env[gc-copy-minor] declared_safepointtrue(was 0/37)DEPTH=800gc_root_dominance_check.pyunder polls-ON codegengc-ratchetpins the pre-change evacuation accounting and needs regenerating onthe pinned host.
Summary by CodeRabbit
Bug Fixes
Documentation