fix(gc): minor sweep must not finalize unmarked old-gen objects (#6892) - #6933
Conversation
A minor trace never marks the old generation: old-gen parents are black
leaves, reached only through dirty remembered-set pages. "Unmarked" in a
minor therefore means "unvisited", not "dead" — the sweep's own
`retain_all_forwarded_stubs` rule already spells this out for forwarding
stubs, and cycle.rs says it again ("Minor traces never mark the old
generation, so deadness there is only trusted for untenured nursery
headers"). `ArenaSweepObjectsState::process_object` did not honour it for
ordinary objects: any unmarked old-gen object took the `reclaim_dead_object`
path.
A minor never frees old-gen memory, so this looked harmless — but
`reclaim_dead_object` still runs `finalize_dead_arena_payload` on the
object, and that calls `layout_clear_for_ptr`, which drops the object's
entry from `LAYOUT_SLOT_MASKS` (plus its payload side tables and external
payload buffers). The live old-gen object is left in state
`GC_LAYOUT_SIDE_MASK` with no mask. The next `layout_note_slot` on it
rebuilds a mask from that single slot, so the following trace visits only
that one element and skips every other pointer slot — and the children
behind them are swept while still referenced. The mutator then reads a
recycled address and gets `undefined`, which is why the reported symptom
was a varying `Cannot read properties of undefined (reading '<field>')`.
Fix: `unmarked_is_provably_dead()` — an unmarked object is only provably
dead when the trace covered its generation. Old-gen blocks are exempt in a
minor sweep, except blocks selected for old-page defrag this cycle, whose
live contents were evacuated during the same cycle (and whose block-level
reclaim needs `block_has_live` to stay false). Full traces are unchanged:
they visit every live parent, so mark-based old-gen reclaim stays sound.
Renames `retain_all_forwarded_stubs` to `minor_sweep`, which is what every
call site already passes and what both rules now key off.
Repro (issue #6892): the Milo compiler built by Perry, running
`emit-ir` over a file importing `std/fetch`. Before: throws. After: exits 0
and its LLVM IR is byte-identical to the same compiler under bun. The
env matrix that motivated the report (`PERRY_GEN_GC=0` and
`PERRY_WRITE_BARRIERS=0` pass, `PERRY_GEN_GC_EVACUATE=0` and
`PERRY_GC_VERIFY_EVACUATION=1` fail) is now uniformly green, as are
`PERRY_GC_FORCE_EVACUATE=1` and the two stress flags combined.
Note on old-page defrag input: minors previously fed
`old_page_account_swept_object(live=false)` for every unmarked old object,
i.e. phantom dead bytes derived from mark bits that say nothing about the
old generation. They now report those objects live, so the defrag
selector's dead-byte signal comes from full traces, where it is real.
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 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 |
1 participant
Fixes #6892.
Root cause
A minor trace never marks the old generation — old-gen parents are black
leaves, reached only through dirty remembered-set pages. So in a minor,
"unmarked" means unvisited, not dead.
The sweep already encodes this rule for forwarding stubs
(
retain_all_forwarded_stubs), andcycle.rsstates it outright when itbuilds the sweep state:
But
ArenaSweepObjectsState::process_objectdid not honour it for ordinaryobjects — every unmarked old-gen object took the
reclaim_dead_objectpath.A minor never frees old-gen memory, which is why this stayed latent. The
damage is the side effects:
reclaim_dead_objectrunsfinalize_dead_arena_payload, which callslayout_clear_for_ptrand dropsthe object's entry from
LAYOUT_SLOT_MASKS(along with its payload sidetables and external payload buffers).
That leaves a live old-gen object in state
GC_LAYOUT_SIDE_MASKwith nomask. The next
layout_note_sloton it rebuilds a mask from that one slot,so the following trace visits only that element and skips every other
pointer slot in the object. The children behind those slots are then swept
while still referenced; the mutator reads a recycled address and gets
undefined. That is why the reported symptom was a varying field name —'tag','name','span','isExtern'— rather than one specific site.How it was localized
Instrumented builds, each step narrowing the blast radius on the issue's
reproducer:
(
verify_minor_unmarked_young_children_report,verify_marked_heap_report_nonfatal), and so was a widened verifier thatalso covered the parent classes they miss (Longlived-arena and
malloc-tracked parents, which survive a minor without being marked):
387k parents, zero dropped edges. So marking was not the problem, and
this is not a missing write barrier.
reclaim_dead_object's side effects isolatedlayout_clear_for_ptr, then theLAYOUT_SLOT_MASKSremoval specifically.OLD:array(ty=1,flags=0x22)=8— eightARENA|TENURED, unmarked, liveold-generation arrays wiped during a minor sweep.
Fix
unmarked_is_provably_dead(): an unmarked object is only provably dead whenthe trace covered its generation. Old-gen blocks are exempt during a minor
sweep — except blocks selected for old-page defrag this cycle, whose live
contents were evacuated during that same cycle (and whose block-level reclaim
needs
block_has_liveto stay false for them).Full traces are unchanged: they visit every live parent, so mark-based old-gen
reclaim stays sound there.
retain_all_forwarded_stubsis renamed tominor_sweep— it is what every call site already passes, and both rules nowkey off it.
This also makes the arena object sweep consistent with every other
post-trace finalizer, which already applied exactly this rule and were the
reason the bug stayed narrow:
registered_map_is_dead_post_trace(and theSet / buffer / typed-array twins) and
dead_owner::owner_is_deadall refuseto trust an unmarked header on a non-full trace unless it is an untenured
nursery object.
ArenaSweepObjectsState::process_objectwas the one placethat skipped the check.
Validation
Reproducer: the Milo compiler built by Perry, running
emit-irover a fileimporting
std/fetch.PERRY_GEN_GC=0PERRY_WRITE_BARRIERS=0PERRY_GEN_GC_EVACUATE=0PERRY_GC_VERIFY_EVACUATION=1PERRY_GC_FORCE_EVACUATE=1PERRY_GC_FORCE_EVACUATE=1+PERRY_GC_VERIFY_EVACUATION=1together: pass,no verifier panic.
Milo's example corpus, Perry-built compiler vs the same compiler under bun,
byte-comparing
emit-iroutput: 51/53 identical.Regression tests (
gc::tests::oldgen):test_minor_sweep_keeps_unmarked_old_object_layout_mask— fails on theparent commit, passes with the fix.
test_full_sweep_still_finalizes_unmarked_old_object— the converse, sothe fix can't be widened into "never reclaim the old generation".
cargo test -p perry-runtime -- --test-threads=1: 1478 passed, 0 failed.(The default parallel run has 4–5 pre-existing failures in
gc::tests::teardown,global_this_webassemblyandnative_module_stream;they reproduce identically with this fix patched out.)
cargo fmt --all -- --check: clean.Not fixed by this PR
Two milo examples still differ, and neither is this bug. Both fail under
every GC arm including
PERRY_GEN_GC=0(full mark-sweep, no generationalcollector at all), so neither is a generational-GC liveness defect:
examples/cli-tools/fmt.milo—Cannot read properties of undefined (reading 'name'), deterministic across repeated runs and every arm.examples/net/serve.milo— same shape. The reported field still shiftsbetween arms (
'tag'vs'attributes'), so something upstream of it isallocation-timing sensitive, but the failure itself survives turning the
generational collector off entirely.
Filed separately rather than folded in here — they are a different defect and
this PR's reproducer is byte-clean.
None of the recent elision PRs (#6919, #6911, #6915, #6916) contributed: the
bug predates all of them (reported at
fd7475182), the fault is in theruntime sweep rather than in barrier emission, and no barrier was elided for
a value that is a pointer.
Perf note
Barrier emission density is unchanged. One behavioural note: minors
previously fed
old_page_account_swept_object(live=false)for every unmarkedold object — phantom dead bytes derived from mark bits that say nothing about
the old generation. They now report those objects live, so the old-page
defrag selector's dead-byte signal comes from full traces, where it is real.