diff --git a/changelog.d/6933-minor-sweep-old-gen-finalize.md b/changelog.d/6933-minor-sweep-old-gen-finalize.md new file mode 100644 index 0000000000..70c9600dfd --- /dev/null +++ b/changelog.d/6933-minor-sweep-old-gen-finalize.md @@ -0,0 +1,34 @@ +fix(gc): a minor sweep no longer finalizes unmarked old-generation objects (#6892). + +A minor trace never marks the old generation — old-gen parents are black +leaves, visited only through dirty remembered-set pages — so "unmarked" in a +minor means *unvisited*, not *dead*. `ArenaSweepObjectsState::process_object` +sent every unmarked old-gen object down `reclaim_dead_object` anyway. A minor +never frees old-gen memory, so this stayed latent; the damage was +`finalize_dead_arena_payload`, whose `layout_clear_for_ptr` dropped the live +object's `LAYOUT_SLOT_MASKS` entry (plus its payload side tables and external +payload buffers). + +That left a live old-gen object in `GC_LAYOUT_SIDE_MASK` state with no mask. +The next `layout_note_slot` rebuilt the mask from that single slot, so the +following trace visited one element and skipped the object's other pointer +slots — sweeping children that were still referenced. Reads then hit a +recycled address and returned `undefined`, which is why the reported symptom +was a varying `Cannot read properties of undefined (reading '')`. + +`unmarked_is_provably_dead()` now gates reclaim on "the trace covered this +object's generation": old-gen blocks are exempt during a minor sweep, except +blocks selected for old-page defrag in the same cycle, whose live contents +were evacuated by that cycle. Full traces are unchanged. The sweep's +`retain_all_forwarded_stubs` flag is renamed `minor_sweep`, which is what +every call site already passed and what both retention rules now key off. + +Found via the Milo compiler built by Perry: `emit-ir` over a file importing +`std/fetch` threw, and its LLVM IR is now byte-identical to the same compiler +under bun. 51/53 of milo's examples now match byte-for-byte. + +Side effect worth knowing: minors used to report every unmarked old object as +dead bytes to `old_page_account_swept_object`, i.e. phantom fragmentation +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. diff --git a/crates/perry-runtime/src/gc/oldgen.rs b/crates/perry-runtime/src/gc/oldgen.rs index 1ffe7c376a..28f9b4dbfd 100644 --- a/crates/perry-runtime/src/gc/oldgen.rs +++ b/crates/perry-runtime/src/gc/oldgen.rs @@ -829,7 +829,7 @@ fn sweep_with_age_bump_and_old_reclaim_targets( ) -> SweepTraceStats { // These synchronous wrappers age-bump exactly when sweeping a MINOR // trace, so `do_age_bump` doubles as the minor-ness signal for the - // forwarded-stub retention rule (see `retain_all_forwarded_stubs`). + // old-gen retention rules (see `minor_sweep`). let mut state = IncrementalSweepState::new( do_age_bump, reclaim_dead_old_blocks, @@ -1198,7 +1198,7 @@ impl IncrementalSweepState { reclaim_dead_old_blocks: bool, targeted_old_blocks: Option>, sweep_malloc: bool, - retain_all_forwarded_stubs: bool, + minor_sweep: bool, ) -> Self { Self { subphase: SweepCycleSubphase::Malloc, @@ -1210,7 +1210,8 @@ impl IncrementalSweepState { arena: ArenaSweepObjectsState::new( do_age_bump, reclaim_dead_old_blocks, - retain_all_forwarded_stubs, + minor_sweep, + targeted_old_blocks.clone(), ), cleanup: None, reclaim_dead_old_blocks, @@ -1341,17 +1342,34 @@ struct ArenaSweepObjectsState { overflow_active: bool, do_age_bump: bool, reclaim_dead_old_blocks: bool, - /// Minor sweeps must retain EVERY forwarding stub: array growth installs - /// PERMANENT stubs (#6228 — stale pre-growth pointers keep resolving for - /// reads, references are never rewritten), and a minor treats old-gen - /// parents as black leaves whose slots are only visited via dirty pages. - /// An old parent (e.g. a long-lived Map's entries buffer) whose page is - /// no longer dirty never marks the stub its slot points at, so - /// "unmarked stub" does NOT imply "unreferenced" in a minor — reclaiming - /// it is a use-after-free (reads through the stale pointer return - /// reused-memory garbage). Full traces DO visit every live parent, so - /// mark-based stub reclaim stays sound (and bounds the accumulation). - retain_all_forwarded_stubs: bool, + /// This sweep follows a MINOR trace, whose mark bits say nothing about the + /// old generation: old-gen parents are black leaves whose slots are only + /// visited through dirty remembered-set pages, so an object reachable only + /// from a non-dirty old parent is never marked. "Unmarked" therefore does + /// NOT imply "dead" for anything in the old generation. + /// + /// Two consequences, both handled below: + /// + /// * Forwarding stubs must ALL be retained: array growth installs + /// PERMANENT stubs (#6228 — stale pre-growth pointers keep resolving for + /// reads, references are never rewritten). An old parent (e.g. a + /// long-lived Map's entries buffer) whose page is no longer dirty never + /// marks the stub its slot points at, so reclaiming it is a + /// use-after-free. + /// * Ordinary old-gen objects must not be reclaimed either (#6892). The + /// minor never frees their memory, but `reclaim_dead_object` still runs + /// `finalize_dead_arena_payload` on them, which wipes a LIVE object's GC + /// slot-layout mask and payload side tables and frees its external + /// payload buffers. + /// + /// Full traces DO visit every live parent, so mark-based reclaim stays + /// sound there (and bounds the accumulation). + minor_sweep: bool, + /// Old-gen blocks selected for page defrag this cycle. Their live contents + /// were evacuated out during this same cycle, so what is left really is + /// reclaimable even in a minor — and the block-level reclaim needs + /// `block_has_live` to stay false for them. + targeted_old_blocks: Option>, freed_bytes: u64, retained_forwarded_stub_objects: usize, retained_forwarded_stub_bytes: usize, @@ -1361,7 +1379,8 @@ impl ArenaSweepObjectsState { fn new( do_age_bump: bool, reclaim_dead_old_blocks: bool, - retain_all_forwarded_stubs: bool, + minor_sweep: bool, + targeted_old_blocks: Option>, ) -> Self { let n_blocks = crate::arena::arena_block_count(); let block_snapshots = crate::arena::arena_block_snapshots(); @@ -1378,7 +1397,8 @@ impl ArenaSweepObjectsState { || crate::closure::closure_dynamic_side_tables_nonempty(), do_age_bump, reclaim_dead_old_blocks, - retain_all_forwarded_stubs, + minor_sweep, + targeted_old_blocks, freed_bytes: 0, retained_forwarded_stub_objects: 0, retained_forwarded_stub_bytes: 0, @@ -1443,13 +1463,36 @@ impl ArenaSweepObjectsState { self.process_forwarded_object(header, block_idx, flags); return; } - if flags & GC_FLAG_MARKED == 0 { + if flags & GC_FLAG_MARKED == 0 && self.unmarked_is_provably_dead(block_idx) { self.reclaim_dead_object(header, block_idx); } else { self.keep_live_object(header, block_idx, flags, age_bump_this, false); } } } + + /// Does `flags & MARKED == 0` actually prove this object is garbage? + /// + /// Only when the trace that produced the marks covered the object's + /// generation. A minor trace never marks the old generation (see + /// `minor_sweep`), so an unmarked old-gen object is merely *unvisited* — + /// it stays live and must not be finalized. #6892: reclaiming one wiped + /// the GC slot-layout mask of a live old-gen array, after which the next + /// `layout_note_slot` rebuilt the mask from a single slot and the + /// following minor stopped tracing the array's other pointer elements, + /// sweeping objects that were still referenced. + /// + /// The old-page defrag targets are exempt: this cycle evacuated their live + /// contents, so the remainder is genuinely reclaimable. + #[inline] + fn unmarked_is_provably_dead(&self, block_idx: usize) -> bool { + if !self.minor_sweep || block_idx < self.old_block_start { + return true; + } + self.targeted_old_blocks + .as_ref() + .is_some_and(|selected| selected.contains(&block_idx)) + } } impl ArenaSweepObjectsState { @@ -1490,10 +1533,10 @@ impl ArenaSweepObjectsState { block_idx: usize, flags: u8, ) { - // See `retain_all_forwarded_stubs`: a minor cannot prove a stub - // unreferenced (old-gen parents are black leaves), so it must keep - // them all; a full trace reclaims the genuinely unreferenced ones. - let retain_stub = self.retain_all_forwarded_stubs + // See `minor_sweep`: a minor cannot prove a stub unreferenced (old-gen + // parents are black leaves), so it must keep them all; a full trace + // reclaims the genuinely unreferenced ones. + let retain_stub = self.minor_sweep || flags & GC_FLAG_MARKED != 0 || (block_idx < self.resettable_general_n && crate::arena::general_block_in_recent_window(block_idx)); diff --git a/crates/perry-runtime/src/gc/tests/oldgen.rs b/crates/perry-runtime/src/gc/tests/oldgen.rs index 188a60c44c..a46883567b 100644 --- a/crates/perry-runtime/src/gc/tests/oldgen.rs +++ b/crates/perry-runtime/src/gc/tests/oldgen.rs @@ -1230,3 +1230,90 @@ fn test_minor_preserves_old_to_young_edge_across_minors() { clear_marks(); remembered_set_clear(); } + +/// #6892 — a MINOR sweep must not finalize an unmarked OLD-generation object. +/// +/// Minor traces never mark the old generation (old-gen parents are black +/// leaves, reached only through dirty remembered-set pages), so "unmarked" in +/// a minor means "unvisited", not "dead". Treating such an object as garbage +/// ran `finalize_dead_arena_payload` on a live object, whose +/// `layout_clear_for_ptr` wiped its GC slot-layout mask. The next +/// `layout_note_slot` then rebuilt the mask from a single slot, so the +/// following trace stopped visiting the object's other pointer slots and swept +/// children that were still referenced. +#[test] +fn test_minor_sweep_keeps_unmarked_old_object_layout_mask() { + let _isolation = copying_nursery_isolation_lock(); + reset_remembered_set(); + clear_marks(); + clear_mark_seeds(); + crate::arena::old_pages_begin_gc_cycle(); + + // A live old-gen object with one pointer slot, plus the slot-layout mask + // the collector reads to find that pointer. + let old_obj = crate::arena::arena_alloc_gc_old(3 * 8, 8, GC_TYPE_OBJECT) as usize; + unsafe { + std::ptr::write_bytes(old_obj as *mut u8, 0, 3 * 8); + // A freshly allocated payload starts pointer-free; the first pointer + // store is what promotes it to a side mask. + crate::gc::layout_init_pointer_free(old_obj as *mut u8); + } + let child = crate::arena::arena_alloc_gc_old(16, 8, GC_TYPE_STRING) as usize; + layout_note_slot(old_obj, 0, string_bits(child)); + assert_eq!( + test_layout_pointer_slot_count(old_obj, 3), + Some(1), + "precondition: the old object starts with a one-pointer slot mask" + ); + + // A minor sweep. `old_obj` is deliberately left UNMARKED — that is exactly + // the state a minor trace leaves every old-gen object in. + let mut sweep = IncrementalSweepState::new(true, false, None, false, true); + let _ = sweep.finish_unbounded(); + + assert_eq!( + test_layout_pointer_slot_count(old_obj, 3), + Some(1), + "#6892: minor sweep wiped the slot-layout mask of a live old-gen object" + ); + + clear_marks(); + remembered_set_clear(); +} + +/// Converse of `test_minor_sweep_keeps_unmarked_old_object_layout_mask`: a FULL +/// trace does visit every live parent, so unmarked really does mean dead there +/// and old-gen reclamation must still happen. Guards the #6892 fix against +/// being widened into "never reclaim the old generation". +#[test] +fn test_full_sweep_still_finalizes_unmarked_old_object() { + let _isolation = copying_nursery_isolation_lock(); + reset_remembered_set(); + clear_marks(); + clear_mark_seeds(); + crate::arena::old_pages_begin_gc_cycle(); + + let old_obj = crate::arena::arena_alloc_gc_old(3 * 8, 8, GC_TYPE_OBJECT) as usize; + unsafe { + std::ptr::write_bytes(old_obj as *mut u8, 0, 3 * 8); + // A freshly allocated payload starts pointer-free; the first pointer + // store is what promotes it to a side mask. + crate::gc::layout_init_pointer_free(old_obj as *mut u8); + } + let child = crate::arena::arena_alloc_gc_old(16, 8, GC_TYPE_STRING) as usize; + layout_note_slot(old_obj, 0, string_bits(child)); + assert_eq!(test_layout_pointer_slot_count(old_obj, 3), Some(1)); + + // Full trace (`minor_sweep = false`): unmarked is provably dead. + let mut sweep = IncrementalSweepState::new(false, true, None, false, false); + let _ = sweep.finish_unbounded(); + + assert_eq!( + test_layout_pointer_slot_count(old_obj, 3), + None, + "a full sweep must still finalize genuinely dead old-gen objects" + ); + + clear_marks(); + remembered_set_clear(); +}