fix(gc): stop the collector moving objects at register-imprecise allocation points (#7682) - #7687
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAllocation-point nursery collections now force conservative scanning and remain non-moving. Runtime tests cover native-stack retention under shipped pacing. A TypeScript interpreter regression and corpus entry validate correct output and allocation-point behavior. ChangesAllocation-point GC safety
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant GeneratedCode
participant gc_check_trigger
participant ManualGcScanGuard
participant NativeStack
GeneratedCode->>gc_check_trigger: request allocation-point collection
gc_check_trigger->>ManualGcScanGuard: force_full_scan()
ManualGcScanGuard->>NativeStack: conservatively scan imprecise roots
NativeStack-->>GeneratedCode: retain pointer without evacuation
🚥 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 |
`gc_check_trigger`'s nursery-churn arm collects from inside `arena_cell_alloc` — at whatever half-finished expression needed a fresh block. Neither root lowering describes that point: the shadow stack names only values already stored to a slot, and RS4GC relocates only what it can type as `ptr addrspace(1)`, which a NaN-boxed `double` operand is not. The `force_full_scan()` there exists for immobility, not retention: a conservative scan makes the copying minor ineligible so nothing relocates. `PERRY_GC_SCAVENGE` gated that guard off, on the strength of a doc comment claiming the flag was "OFF by default … for measurement only" (it has been ON since #7056) and a body comment claiming it "defers alloc-point collections to a precise safepoint" (that deferral is gated on `gc_moving_loop_polls_enabled`, OFF since #7161). So the shipped default ran an evacuating minor at a register-imprecise point, and a tree-walking interpreter silently returned 1708662 instead of 1708840 — every run, no crash, no diagnostic. The guard is now unconditional. Scavenge keeps its pacing job; copying minors keep running at the precise safepoints.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/perry-runtime/src/gc/policy.rs (1)
1880-1891: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlso correct the stale lead-in comment above this arm.
This block states the correct facts:
PERRY_GC_SCAVENGEis ON by default since#7056, and the scan-skip is gone. The lead-in comment for the sameifstill states the opposite:
- Line 1761 reads
PERRY_GC_SCAVENGE (Phase-1 de-risking, OFF by default).- Line 1767 points the reader to "the
force_full_scanskip below", which no longer exists.- Lines 1775-1777 describe
gc_scavenge_enabled()as the branch "which skips the conservative scan HERE".One function now carries two comments that disagree about the same flag. That is the same two-comments-disagree shape this PR identifies as the cause of
#7682. Update lines 1761-1778 so the arm has a single description.♻️ Suggested rewrite of the stale lead-in text
- // PERRY_GC_SCAVENGE (Phase-1 de-risking, OFF by default): when the budgeted - // stepper is NOT blocked (all scanners budgeted), the nursery-churn triggers + // PERRY_GC_SCAVENGE (PACING, ON by default since `#7056`): when the budgeted + // stepper is NOT blocked (all scanners budgeted), the nursery-churn triggers // fall through to the budgeted mutator-assist step below, which is // deliberately non-moving (`low_pause_non_moving = is_budgeted()`), so a // reallocation-heavy loop's minors free nothing. Route those triggers to the - // direct (non-budgeted, atomic) minor here instead so the copying/evacuating - // fast path can run (see the `force_full_scan` skip below). + // direct (non-budgeted, atomic) minor here instead of leaving them to the + // stepper. The flag decides WHICH collector runs the trigger; it does not + // decide whether that collector may move (`#7682`).- // (`js_gc_loop_safepoint` → `gc_safepoint_moving_minor`), NOT here at the - // register-imprecise alloc point. Unlike `gc_scavenge_enabled()` (which skips - // the conservative scan HERE — sound only if the alloc point is precise), the - // loop-polls path never reaches the skip: it always defers to a real - // safepoint. + // (`js_gc_loop_safepoint` → `gc_safepoint_moving_minor`), NOT here at the + // register-imprecise alloc point. The alloc-point minor below is + // unconditionally non-moving (`force_full_scan`, `#7682`), so the loop-polls + // route is what makes an EVACUATING nursery collection possible at all.🤖 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/policy.rs` around lines 1880 - 1891, Update the stale lead-in comment for the same conditional arm, removing the claim that PERRY_GC_SCAVENGE is off by default and the references to the nonexistent force_full_scan skip or scan-skipping behavior. Make the comment consistently state that the flag is on by default since `#7056` and that the scan-skip path is gone, matching the surrounding arm comment.
🤖 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 `@crates/perry-runtime/src/gc/policy.rs`:
- Around line 1880-1891: Update the stale lead-in comment for the same
conditional arm, removing the claim that PERRY_GC_SCAVENGE is off by default and
the references to the nonexistent force_full_scan skip or scan-skipping
behavior. Make the comment consistently state that the flag is on by default
since `#7056` and that the scan-skip path is gone, matching the surrounding arm
comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 55a72ec5-905c-4e13-b4cb-29c61b3ef514
📒 Files selected for processing (6)
changelog.d/7687-alloc-point-collections-must-not-move.mdcrates/perry-runtime/src/gc/mod.rscrates/perry-runtime/src/gc/policy.rscrates/perry-runtime/src/gc/tests/scan_fallback.rstest-files/test_gap_gc_alloc_point_no_move.tstest-parity/gc_repsel_corpus.txt
Audit — merging as v0.5.1389. P0 confirmed and fixed.Reproduced independently, both arms built
A silently wrong number on default settings, from ordinary TypeScript. No crash, no diagnostic. That is the worst failure shape this project has. The root cause is the one that matters most
So the guard's job at the alloc point is not retention but immobility — the conservative scan makes the copying minor ineligible, and that is the entire mechanism keeping a register-held value valid across a collection nobody can describe. Gating it on a pacing flag traded a cost problem for a soundness problem, which is exactly what #7148 forbids. And both halves of the reason it was conditional were false. The doc comment said "OFF by default … NOT sound as a production default"; eight lines below, the body said "ON BY DEFAULT (#7056)". That is the #6987 shape CLAUDE.md warns about — "a merge decision was made on the wrong one" — and this time the stale half was the one carrying the soundness argument. The body's fallback claim (that enabling it also defers to a precise safepoint) was gated on Sabotage verified independentlyRestoring the The control is the part I want to single out: "the malloc sweep never ran" and "the guard held" are the same green without it. That is the distinction four of this repo's gates have failed to make.
Why every existing gate was greenThe table in the description is correct and worth keeping: Gates: 24/24 lint, fmt clean, The cost, and the immediate follow-upAlloc-point nursery collections are non-moving again — what they were before #7056 — with copying minors still running at the precise safepoints where the root set is real. That is the right trade for a correctness bug of this shape. This PR does not carry the ratchet re-pin, so The two sound routes you name for getting the evacuation back — make the alloc point precise, or let the copying minor treat conservative roots as pinning sources rather than refusing outright ( |
a6a94b2 to
ac3b7ab
Compare
The #7687 merge landed without its bump: the branch was based on 0.5.1387 and my sed targeted 1388, so it matched nothing and the commit was empty. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
Measured cost, and why it is recoverablePinned quiet host (load 1.4), 5 repeats, the interpreter probe. Same binary in
B is a one-env-var reproducer on the fixed binary — Two things worth reading off it:
The collector was already printing this
37 of those, and zero It also restores a claim already checked in
Gates
Known sharp edge, not changed here
|
Update: the fix now costs nothingThe soundness fix alone was not shippable, and chasing that produced two more
Same speed, same footprint, right answer. 37 copying minors — the same count the (2) The nursery cap applies only when the minor can evacuateFixing (1) alone took #7056's own 2x2 already said the cap and the evacuating minor "ship together, (3) Moving-loop back-edge polls default-ON (#7161's stopgap retired)Both conditions #7161 named for putting this back are met:
And after (1), leaving it off was the more dangerous state. Nursery pressure The flip is less of a leap than it looks. The four Two tests changed, and neither was waved through
Local verification
Residual risk, statedThe rest of the gap corpus (479 tests) under polls-ON is not gated per-PR —
|
proggeramlug
changed the title
…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.
…#7682 (#7690) * fix(gc): the scavenge nursery cap applies only when the minor can evacuate 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) * test(gc): mirror the new nursery-cap gate in the trigger arithmetic test (cherry picked from commit f39ec36) * docs(gc-matrix): the shipped default reaches the safepoint route again (cherry picked from commit d5d8409) * docs(gc): correct the alloc-point prose scavenge no longer skips the guard (cherry picked from commit 514b6e9) * fix(gc): moving-loop back-edge polls default ON again (#7161 stopgap 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. * docs(changelog): fragment for the pacing follow-up * docs(changelog): key the fragment on PR #7690 * docs: the polls default is ON again — CLAUDE.md and the rooting-invariant 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. * review(#7690): the pacing guard is the kill switch, not the default 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). * fix(gc): defer the back-edge-poll default flip; keep the nursery-cap fix 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 * chore: bump version to 0.5.1393 Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
…he doc (#7690) #7690 wrote the entire default-ON argument into two doc comments — the runtime's `moving_loop_polls_enabled_from_env` and codegen's `moving_safepoint_polls_enabled` — and changed neither body. Both still matched `1|on|true`, i.e. default OFF, and no test pinned the default in either direction, even though the runtime predicate had been factored out expressly to make it "unit-testable without touching process env". That is not a slower configuration, it is a different collector. Nursery pressure has exactly two precise collection points, the loop back-edge poll and the outermost microtask-pump boundary. With no poll emitted, a compute-only program reaches neither, so every nursery collection happened at the register-imprecise allocation point — where #7687 had just made it correctly non-moving. The shipped result was a collector with no nursery evacuation at all. Measured on the quiet bench host, best-of-3, `PERRY_NO_AUTO_OPTIMIZE=1` with a pinned `PERRY_RUNTIME_DIR`, against `a853135aa` binaries rerun back-to-back on the same host: | bench | main | this | a853135 | |---|--:|--:|--:| | churn | 1.01 | 0.45 | 0.66 | | churn_alloc | 0.90 | 0.42 | 0.36 | | push_cls | 0.89 | 0.40 | 0.34 | | retain | 2.33 | 1.37 | 1.33 | | tree | 5.06 | 1.63 | 5.97 | | tree_wide | 7.26 | 2.11 | 12.38 | | cycles | 0.29 | 0.19 | 0.96 | `churn_alloc` ran 13 whole-arena full collections (0.477 s of pause) where the same program at `a853135aa` ran 105 copying minors (0.016 s). `tree`'s GC pause falls 4.107 s -> 0.626 s and its max pause 266 ms -> 23 ms; `trace_worklist` drops from 2,877 ms out of the top six phases entirely. The #7161 blocker that made polls-off a stopgap is separately discharged: a poll at every back-edge defeated the #7480 element-shape fast clone, and step 4 of that work now refuses to emit a poll inside a call-free-by-construction clone. Measured both ways, `churn_read` is 0.02 s. Costs, measured rather than argued: `deeplist` 0.03 -> 0.33 and `retain1` 0.03 -> 0.42. Both are workloads whose heap stays under the initial 64 MB threshold, so they previously ran ZERO collections and the moving nursery is pure added cost; both still beat `a853135aa` (1.09 / —). `push_num` 0.16 -> 0.17. Three tests pin what was unpinned: `polls_default_is_on` and its codegen mirror `moving_safepoint_poll_default::unset_emits_the_poll` each pin one half against the full spelling table, and `polls_default_matches_codegen_mirror` pins that the two crates agree — the disagreement is silent in both directions, so it needs its own assertion rather than being left to two doc comments claiming they match.
…he doc (#7690, #7682) (#7721) * fix(gc): make the moving-loop poll default ON in the code, not just the doc (#7690) #7690 wrote the entire default-ON argument into two doc comments — the runtime's `moving_loop_polls_enabled_from_env` and codegen's `moving_safepoint_polls_enabled` — and changed neither body. Both still matched `1|on|true`, i.e. default OFF, and no test pinned the default in either direction, even though the runtime predicate had been factored out expressly to make it "unit-testable without touching process env". That is not a slower configuration, it is a different collector. Nursery pressure has exactly two precise collection points, the loop back-edge poll and the outermost microtask-pump boundary. With no poll emitted, a compute-only program reaches neither, so every nursery collection happened at the register-imprecise allocation point — where #7687 had just made it correctly non-moving. The shipped result was a collector with no nursery evacuation at all. Measured on the quiet bench host, best-of-3, `PERRY_NO_AUTO_OPTIMIZE=1` with a pinned `PERRY_RUNTIME_DIR`, against `a853135aa` binaries rerun back-to-back on the same host: | bench | main | this | a853135 | |---|--:|--:|--:| | churn | 1.01 | 0.45 | 0.66 | | churn_alloc | 0.90 | 0.42 | 0.36 | | push_cls | 0.89 | 0.40 | 0.34 | | retain | 2.33 | 1.37 | 1.33 | | tree | 5.06 | 1.63 | 5.97 | | tree_wide | 7.26 | 2.11 | 12.38 | | cycles | 0.29 | 0.19 | 0.96 | `churn_alloc` ran 13 whole-arena full collections (0.477 s of pause) where the same program at `a853135aa` ran 105 copying minors (0.016 s). `tree`'s GC pause falls 4.107 s -> 0.626 s and its max pause 266 ms -> 23 ms; `trace_worklist` drops from 2,877 ms out of the top six phases entirely. The #7161 blocker that made polls-off a stopgap is separately discharged: a poll at every back-edge defeated the #7480 element-shape fast clone, and step 4 of that work now refuses to emit a poll inside a call-free-by-construction clone. Measured both ways, `churn_read` is 0.02 s. Costs, measured rather than argued: `deeplist` 0.03 -> 0.33 and `retain1` 0.03 -> 0.42. Both are workloads whose heap stays under the initial 64 MB threshold, so they previously ran ZERO collections and the moving nursery is pure added cost; both still beat `a853135aa` (1.09 / —). `push_num` 0.16 -> 0.17. Three tests pin what was unpinned: `polls_default_is_on` and its codegen mirror `moving_safepoint_poll_default::unset_emits_the_poll` each pin one half against the full spelling table, and `polls_default_matches_codegen_mirror` pins that the two crates agree — the disagreement is silent in both directions, so it needs its own assertion rather than being left to two doc comments claiming they match. * perf(gc): stop paying a shape-layout hash lookup per traced object for a disabled counter `heap_payload_slot_selection` runs once per traced object per GC walk (mark, rewrite, verify). For every GC_TYPE_OBJECT it computed `raw_numeric_object_slots` via `with_typed_descriptor_for_query` — a per-object map probe plus, for every class instance, a `SHAPE_LAYOUTS` hash lookup behind a TLS RefCell borrow. That number has exactly one consumer, `record_layout_raw_numeric_object_field_range_skipped`, which returns on its first line unless PERRY_GC_LAYOUT_SCAN_TRACE armed the counter. So the shipped collector paid a hash lookup per object to produce a number nothing read — the same shape as #7702, where a facility disabled at runtime was still having its arguments evaluated. Gate the computation on `layout_scan_trace_active()`. Second item, same walk: `shape_shared_pointer_mask` returned `shape_shared_descriptor(user_ptr).map(|d| d.pointer_mask)`, cloning the whole `TypedLayoutDescriptor` to keep one of its two masks. `LayoutSlotMask` is `Heap(Vec<u64>)` above 64 slots, so a traced wide object allocated and freed a second vector — the `raw_f64_mask` — on every walk. Borrow through `with_shape_shared_descriptor` and clone only the mask returned; `shape_shared_descriptor` had no other caller and is removed rather than left as dead code. * test(gc): declare the pacing the alloc-point rooting tests actually assert at Four `runtime_roots` tests took no pacing guard, so they inherited the process default — which this stack changes. They are not asserting about the default; they are asserting that a specific runtime helper's object survives a collection that happens at the allocation point, and they reach that collection through the direct alloc-point minor. Under moving-loop polls that pressure is deferred to a precise safepoint, and a Rust unit test has no loop back-edge poll to drain it, so no collection runs and `assert_automatic_minor_gc_progressed` reports neither a finished assist nor an ACTIVE budgeted cycle. `force_legacy_gc_pacing` is the wrong repair and the tests say so themselves. Three of them carry an evacuation witness — "the minor did not evacuate, so nothing here was exercised and a green result would be meaningless" — and legacy pacing hands the work to the budgeted stepper, which is deliberately non-moving. Pinning it turns a failed assist assertion into a failed liveness assertion, which is the witness doing its job. `force_alloc_point_minor_pacing` (polls OFF, scavenge ON) is the one combination in which both halves hold, and it is the configuration these tests were written against. `symbol_description` has no evacuation witness and takes `force_legacy_gc_pacing`. The moving default's rooting coverage for these helpers is the gap suite's `test_gap_gc_*_rooting.ts` cases and the zeal + from-space-protect runs, not this vehicle — recorded in each test so the next reader does not mistake a pinned pacing for the default being untested. * docs(changelog): fragment for the moving-loop poll default (#7714) * docs(changelog): key the fragment to its PR number (#7721) * chore: bump version to 0.5.1418 Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
1 participant
Fixes #7682.
The bug
A 189-statement tree-walking interpreter — ordinary TypeScript, no exotic
construct — returned a silently wrong number on default settings, every run:
1708662where Node and a fully static build both give1708840. No crash, noTypeError, no diagnostic.Root cause
gc_check_trigger's nursery-churn arm collects from insidearena_cell_alloc,i.e. at whatever half-finished expression happened to need a fresh arena block.
That program point is described by neither root lowering: the shadow stack
names only values codegen has already stored to a slot, and RS4GC relocates only
what it can type as
ptr addrspace(1), which a NaN-boxeddoubleoperand in anSSA register is not.
The arm therefore took
ManualGcScanGuard::force_full_scan(). Its job there isnot retention but immobility — a conservative native-stack scan makes the
copying minor ineligible (
CopiedMinorFallbackReason::ConservativeStack), so thenon-moving in-place minor runs and nothing relocates.
PERRY_GC_SCAVENGEgated that guard off. The guard is now unconditional.Why it was conditional, and why both halves of the reason were false
The flag's doc comment said:
Eight lines below it, the body said
ON BY DEFAULT (#7056). That is the #6987shape CLAUDE.md warns about, and this time the stale half was the one carrying
the soundness argument.
The body's own claim — "enabling this also defers alloc-point collections to a
precise safepoint" — was false too: that deferral is gated on
gc_moving_loop_polls_enabled(), OFF by default since #7161. In the shippedconfiguration the two flags disagree, the deferral is dead code, and the
alloc-point minor ran right there with neither a scan nor a safepoint.
The failure, end to end
evalNodelowers{ names: [n.name], … }by readingn.nameinto a register,then inline-bump-allocating the one-element array. The bump overflows its block,
js_inline_arena_slow_alloc→arena_cell_alloc→gc_check_triggerruns anevacuating minor, and the string moves. Control returns to the merge block —
which LLVM tail-merged across the fast and slow paths — and stores the pre-move
address into the new array.
lookupthen comparesnames[i] === name, a livestring against a moved one, falls through to its default, and naive
fib, whichis just a count of leaves returning
1, comes back short by exactly the numberof missed lookups.
Localised with
PERRY_GC_PROTECT_FROMSPACE=1 DEPTH=800 PERRY_GC_ZEAL=1, thendisassembled: the reported frame was the caller (the faulting callee is a
frameless leaf), and the faulting instruction is
js_string_addref_if_heap_stringreading
refcountatuser_ptr + 12on the value the tail-merged block juststored.
Why every existing gate was green
PERRY_GC_VERIFY_MARKPERRY_GC_FROMSPACE_SCANmarked=false, i.e. already dead — same reasongc_root_dominance_check.pytest_gap_gc_*probe corpusTests
the_alloc_point_nursery_minor_retains_native_stack_values_under_shipped_pacingdrives the arm with a value reachable only from a live native-stack word and
asserts it survives, that a collection actually ran, and that the census
counted the forced scan.
the_alloc_point_plant_dies_when_the_scan_is_pinned_offis its sabotagecontrol: identical plant, scan pinned off, plant must DIE. Without it, "the
malloc sweep never ran" and "the guard held" are the same green.
policy::force_shipped_default_gc_pacingpins polls OFF + scavenge ON.That combination had no test guard —
force_legacy_gc_pacingpins both off,force_moving_gc_pacingpins both on — so every test in the crate declared apacing mode in which the two flags agreed, and the interaction that broke is
exactly the one where they disagree.
test-files/test_gap_gc_alloc_point_no_move.tsis the interpreter itself,diffed byte-for-byte against Node, registered in
test-parity/gc_repsel_corpus.txtsogc-moving-witnessesruns it.Verified non-vacuous both ways. The gap probe prints
1708662against thepre-fix runtime under the harness's own
PERRY_NO_AUTO_OPTIMIZE=1; and with theone-line guard reverted, the new unit test fails.
Cost, stated plainly
Alloc-point nursery collections are non-moving again — what they were before
#7056. Copying minors keep running at the precise safepoints
(
gc_safepoint_moving_minor), where the root set is real.PERRY_GC_SCAVENGEkeeps its other job (routing nursery-churn triggers to the direct minor rather
than the budgeted non-moving stepper) and is now documented as the pacing knob
it is.
The
gc-ratchetbaseline pins the old, evacuating behaviour and will report abreach on the evacuation counters. That artifact is regenerated only when a
change is deliberately accepted, on the pinned quiet host; this is such a change.
Measured delta is in the comments below.
The way to get the evacuation back soundly is to make the alloc point precise,
or to let the copying minor run with conservative roots as pinning sources
rather than refusing outright (
CopiedMinorEligibility::evaluate_with_stack_decisionreturns
ConservativeStackunconditionally today). Both are their own changewith their own proof obligation, and neither belongs in a P0 correctness fix.
Summary by CodeRabbit
Bug Fixes
Tests
Documentation