fix(gc): two passes retained unreachable objects — block persistence on precise-root cycles, remembered set as roots on full traces (#9628, #9629)

changelog.d/9628-9629-gc-retention.md

@@ -0,0 +1,55 @@
+**Two collector passes kept objects alive that nothing could reach (#9628, #9629).**
+
+**Block persistence ran on cycles whose root set is complete (#9628).** The
+pass force-marks every object in a nursery block that still holds one
+reachable object, and pushes each onto the trace worklist so their children
+are retained too. It exists for one hazard: an object the mutator holds only
+in a register across a collection, whose block survives but whose
+individually-swept malloc children do not, so the mutator later reads freed
+memory (#43/#44). That hazard needs a root set the scan cannot see into.
+
+Its early-out skipped the pass only when the conservative stack+register scan
+had run. #7558 then removed that scan from `manual_gc_collect_now` on the
+argument that a call is a statepoint and everything live across it is rooted
+by codegen — so every explicit `gc()` started running the pass instead of
+skipping it, and the comment above the early-out went on naming `gc()` as a
+case that skips. Measured on a fixture that drops 11,000 objects and calls
+`gc()`: 13,364 objects force-marked, none reachable from a root.
+
+The condition now asks the question the pass is actually about, "can a
+root-invisible register hold a live object here?", and answers no in three
+cases: the conservative scan ran (as before), no generated frame is live on
+this thread (verbatim the completeness argument `pressure.rs` already makes),
+or the collection is an explicit `gc()` (#7558's argument). Safepoint-driven
+collections still run the pass; they are equally provably redundant, but the
+cycle has no signal for "at a safepoint" yet and this change does not invent
+one. `PERRY_GC_BLOCK_PERSIST_ALWAYS=1` restores the old behaviour.
+
+**A full mark-sweep marked the remembered set as roots (#9629).** For a minor
+that is necessary: it deliberately does not trace the old generation, so
+old-to-young edges genuinely are roots. A full trace visits the old
+generation from the real root set, so every young object a LIVE old object
+points at is reached anyway, and the dirty-page scan validates only that the
+owner is a plausible pointer, never that it is live. The one thing marking
+added was the young objects reachable exclusively from DEAD old objects.
+
+Full traces now take the snapshot but mark nothing from it. Taking the
+snapshot is not optional and this is the trap in the change: that call is
+what lazily arms the write barrier and reconstructs the log from the heap, so
+skipping it as well would drop old-to-young stores, which fails in the far
+worse direction.
+
+Both tests fail when their own fix is reverted, which is how they were
+checked: the first reports 20,000 resurrected objects, the second a non-zero
+`newly_marked` on a full cycle.
+
+**Measured effect, stated honestly.** On the compiled claude-code TUI at idle
+(Linux, three full collections), block persistence force-marked **0** objects:
+its recent-block window happened to hold no dead neighbours, so #9628 buys
+nothing there and the 13,364 figure above is a fixture result, not a TUI one.
+The remembered-set marking on those same three full cycles marked **845, 800
+and 800** young objects as roots. After the fix a full trace marks none of
+them, and every one that is genuinely reachable is still reached from the real
+roots; the retention actually removed is the subset reachable only from dead
+old objects, which this telemetry does not separate. Both changes are
+correctness and hygiene rather than a large idle-memory win.

crates/perry-runtime/src/gc/barrier/mod.rs

@@ -310,6 +310,27 @@ pub(super) struct RememberedSetRootMarkState {
 
 impl RememberedSetRootMarkState {
     pub(super) fn new() -> Self {
+        Self::new_with_marking(true)
+    }
+
+    /// #9629: a FULL trace visits the old generation from the real root set,
+    /// so every young object a LIVE old object points at is reached anyway.
+    /// Marking from the remembered set on top of that adds exactly one thing:
+    /// the young objects reachable only from old objects that are themselves
+    /// DEAD. `DirtyHeaderSlotScan::new` validates that the dirty page's header
+    /// is a plausible pointer (`valid_ptrs`), never that it is live, so a dead
+    /// old owner's slots are marked as roots like any other.
+    ///
+    /// `mark = false` therefore skips the marking for full traces while still
+    /// taking the snapshot, which is NOT optional: `remembered_dirty_snapshot`
+    /// is what lazily arms the write barrier and reconstructs the log from the
+    /// heap (`barrier_arming::arm_and_reconstruct_remembered_set_if_unarmed`).
+    /// Skipping the snapshot as well would leave a thread's old-to-young
+    /// stores unlogged, which fails in the opposite and far worse direction.
+    ///
+    /// A minor keeps marking: it deliberately does not trace the old
+    /// generation, so there old-to-young edges genuinely are roots.
+    pub(super) fn new_with_marking(mark: bool) -> Self {
         let snapshot = remembered_dirty_snapshot();
         let stats = RememberedSetTraceStats {
             entries_scanned: snapshot.dirty_old_pages.len()
@@ -322,7 +343,7 @@ impl RememberedSetRootMarkState {
         let old_page_cursor = (!snapshot.dirty_old_pages.is_empty())
             .then(|| crate::arena::OldArenaPageObjectCursor::new(&snapshot.dirty_old_pages));
 
-        Self {
+        let mut state = Self {
             snapshot,
             stats,
             old_page_cursor,
@@ -331,7 +352,17 @@ impl RememberedSetRootMarkState {
             seen_headers: crate::fast_hash::new_ptr_hash_set(),
             current_header: None,
             finalized: false,
+        };
+        if !mark {
+            // Snapshot taken (barrier armed, log reconstructed); mark nothing.
+            // The reported set size stays truthful — only `newly_marked` is 0.
+            state.old_page_cursor = None;
+            state.external_cursor = state.snapshot.external_dirty_entries.len();
+            state.fallback_cursor = state.snapshot.fallback_headers.len();
+            state.stats.dirty_pages_after = remembered_dirty_page_count();
+            state.finalized = true;
         }
+        state
     }
 
     pub(super) fn step(&mut self, valid_ptrs: &ValidPointerSet, budget: usize) -> bool {

crates/perry-runtime/src/gc/cycle.rs

@@ -1,5 +1,7 @@
 use super::barrier::{ConservativePinClearState, RememberedSetClearState};
-use super::cycle_malloc_trim::{run_allocator_purge, run_malloc_trim};
+use super::cycle_malloc_trim::{
+    block_persist_always_enabled, run_allocator_purge, run_malloc_trim,
+};
 use super::*;
 
 #[derive(Clone, Copy, Debug, Eq, PartialEq)]
@@ -530,16 +532,21 @@ struct RootScanCycleState {
     mutable_registered: Option<MutableRegisteredRootScanState>,
     legacy_registered: Option<LegacyRegisteredRootScanState>,
     remembered_set: Option<RememberedSetRootMarkState>,
+    /// #9629: whether this cycle's trace stops at the old generation. Only a
+    /// minor needs the remembered set marked as roots; see the
+    /// `RememberedSet` subphase.
+    minor_only: bool,
 }
 
 impl RootScanCycleState {
-    fn new() -> Self {
+    fn new(minor_only: bool) -> Self {
         Self {
             subphase: RootScanSubphase::ConservativeStack,
             mutable_slot_cursor: MutableRootSlotScanCursor::default(),
             mutable_registered: None,
             legacy_registered: None,
             remembered_set: None,
+            minor_only,
         }
     }
 
@@ -667,9 +674,15 @@ impl RootScanCycleState {
                 false
             }
             RootScanSubphase::RememberedSet => {
-                let state = self
-                    .remembered_set
-                    .get_or_insert_with(RememberedSetRootMarkState::new);
+                // #9629: only a MINOR needs old->young edges as roots; a full
+                // trace reaches them through the live old objects themselves,
+                // and marking from the set additionally retains whatever only
+                // DEAD old objects point at. The snapshot is still taken (it
+                // arms the write barrier) — see `new_with_marking`.
+                let mark_from_set = self.minor_only;
+                let state = self.remembered_set.get_or_insert_with(|| {
+                    RememberedSetRootMarkState::new_with_marking(mark_from_set)
+                });
                 if state.step(valid_ptrs, budget) {
                     if let Some(trace) = trace.as_mut() {
                         trace.remembered_set = state.stats();
@@ -1108,7 +1121,9 @@ impl GcCycleState {
             .as_ref()
             .is_some_and(|minor| minor.evacuation_policy.considered);
 
-        self.root_scan.get_or_insert_with(RootScanCycleState::new);
+        let minor_only = matches!(self.collection_kind, GcCollectionKind::Minor);
+        self.root_scan
+            .get_or_insert_with(|| RootScanCycleState::new(minor_only));
         // Phase 4 (incremental old-gen, gated): allow the initial root-scan step
         // of a budgeted cycle to run unbudgeted scanners synchronously (a
         // bounded initial-mark pause), so the stepper can start on programs that
@@ -1166,6 +1181,57 @@ impl GcCycleState {
         trace_phase_record(&mut self.trace, "trace_worklist", phase_start);
     }
 
+    /// Is the block-persistence pass redundant for THIS cycle? (#9628)
+    ///
+    /// The pass exists for one hazard, stated at
+    /// `trace::mark_block_persisting_arena_objects`: an object that the
+    /// mutator still holds **in a register** and will store somewhere after
+    /// the collection returns, which the root scan cannot see. Its arena block
+    /// survives (block reset is all-or-nothing and a neighbour is reachable),
+    /// so the object's own memory is intact — but its individually-swept
+    /// malloc children are freed under it, and the mutator then reads freed
+    /// memory. Issues #43/#44.
+    ///
+    /// So the question the pass should ask is "can a root-invisible register
+    /// hold a live object here?", and the answer is no in three cases:
+    ///
+    /// * the conservative stack+register scan RAN, which pins exactly those
+    ///   objects — the original condition, and the only one this used to test;
+    /// * no generated frame is live on this thread, so there is no register
+    ///   holding a JS value at all. This is verbatim the completeness argument
+    ///   `pressure.rs` already makes for skipping `force_full_scan`;
+    /// * the collection is an explicit `gc()`. A call is a statepoint: every
+    ///   value live across it is rooted by codegen, because it has to survive
+    ///   the call whether or not the call collects. That is #7558's argument
+    ///   for dropping the conservative scan from `manual_gc_collect_now`, and
+    ///   it is the same argument.
+    ///
+    /// Before this, only the first case skipped, so the modern precise-root
+    /// paths — every explicit `gc()` and every safepoint-driven collection —
+    /// ran the pass and force-marked their recent window. That is not a small
+    /// effect: the pass pushes each resurrected object onto the trace
+    /// worklist, so it also retains everything reachable FROM the dead
+    /// objects. Measured on a 30-line fixture that drops 11,000 objects and
+    /// calls `gc()`: 13,364 objects force-marked, none reachable from a root.
+    ///
+    /// `PERRY_GC_BLOCK_PERSIST_ALWAYS=1` restores the pre-#9628 behaviour for
+    /// bisection.
+    fn block_persistence_is_redundant(&self) -> bool {
+        if block_persist_always_enabled() {
+            return false;
+        }
+        if matches!(
+            super::roots::conservative_stack_scan_decision(),
+            super::roots::ConservativeStackScanDecision::Scan
+        ) {
+            return true;
+        }
+        if !super::roots::shadow_stack_has_active_frame() {
+            return true;
+        }
+        matches!(self.trigger_kind, GcTriggerKind::Manual)
+    }
+
     fn step_block_persistence(&mut self, budget: GcWorkBudget) {
         // #6010: block persistence exists to protect REGISTER-HELD recent
         // objects that precise (shadow-stack) roots can't see (#43/#44). A
@@ -1178,10 +1244,7 @@ impl GcCycleState {
         // window, which made garbage there immortal — dead Maps/Sets kept
         // multi-MB external buffers for the life of the process (#6010:
         // 1.4 GB RSS on a Map-churn benchmark whose live heap was ~1 MB).
-        if matches!(
-            super::roots::conservative_stack_scan_decision(),
-            super::roots::ConservativeStackScanDecision::Scan
-        ) {
+        if self.block_persistence_is_redundant() {
             self.phase = GcCyclePhase::AtomicFinalize;
             return;
         }
@@ -1281,7 +1344,10 @@ impl GcCycleState {
                     let _remark_timer = instruments::FinalRemarkTimer::start();
                     let valid_ptrs = self.valid_ptrs.as_ref().expect("valid pointer set built");
                     let minor_only = self.minor.is_some();
-                    let remark_scan = self.root_scan.get_or_insert_with(RootScanCycleState::new);
+                    let remark_minor_only = matches!(self.collection_kind, GcCollectionKind::Minor);
+                    let remark_scan = self
+                        .root_scan
+                        .get_or_insert_with(|| RootScanCycleState::new(remark_minor_only));
                     while !remark_scan.step_current_subphase(
                         valid_ptrs,
                         &mut self.trace,

crates/perry-runtime/src/gc/cycle_malloc_trim.rs

@@ -122,6 +122,15 @@ pub(super) fn run_allocator_purge(major: bool) -> MallocTrimOutcome {
     }
 }
 
+/// `PERRY_GC_BLOCK_PERSIST_ALWAYS` — OFF by default. Set it to run the
+/// block-persistence pass on every cycle again, i.e. the behaviour before
+/// #9628 taught it to skip cycles whose root set is complete. Bisection knob;
+/// see `cycle::GcCycleState::block_persistence_is_redundant`.
+pub(super) fn block_persist_always_enabled() -> bool {
+    static ENABLED: std::sync::OnceLock<bool> = std::sync::OnceLock::new();
+    *ENABLED.get_or_init(|| crate::gc::env_flag_enabled("PERRY_GC_BLOCK_PERSIST_ALWAYS"))
+}
+
 /// `PERRY_GC_MALLOC_PURGE` — ON by default; `=0`/`off`/`false` disables the
 /// mimalloc purge above and restores the pre-#9612 behaviour.
 fn allocator_purge_enabled() -> bool {

crates/perry-runtime/src/gc/tests/cycle_state.rs

@@ -720,7 +720,15 @@ fn root_scan_slices_remembered_set_dirty_slots_with_tiny_budget() {
     }
     assert!(remembered_set_size() > 0);
 
-    let mut state = GcCycleState::new_full(trace_snapshot(GcTriggerKind::Manual));
+    // #9629: a MINOR, deliberately. The subject here is that the
+    // remembered-set subphase respects a tiny budget instead of running to
+    // completion in one step, and a minor is where that marking now lives: a
+    // full trace reaches every LIVE old object's children on its own, so it
+    // takes the snapshot and marks nothing. This fixture's `old_obj` is
+    // unrooted, so under a full cycle the mark assertion below would be
+    // asserting that a DEAD old object keeps its children alive, which is the
+    // retention #9629 removes.
+    let mut state = start_minor_fallback_state(trace_snapshot(GcTriggerKind::Manual));
     run_cycle_until_phase(&mut state, GcCyclePhase::RootScan);
 
     let mut root_steps = 0usize;
@@ -769,7 +777,15 @@ fn root_scan_slices_remembered_set_dirty_old_pages_with_tiny_budget() {
     }
     assert!(remembered_set_size() > 0);
 
-    let mut state = GcCycleState::new_full(trace_snapshot(GcTriggerKind::Manual));
+    // #9629: a MINOR, deliberately. The subject here is that the
+    // remembered-set subphase respects a tiny budget instead of running to
+    // completion in one step, and a minor is where that marking now lives: a
+    // full trace reaches every LIVE old object's children on its own, so it
+    // takes the snapshot and marks nothing. This fixture's `old_obj` is
+    // unrooted, so under a full cycle the mark assertion below would be
+    // asserting that a DEAD old object keeps its children alive, which is the
+    // retention #9629 removes.
+    let mut state = start_minor_fallback_state(trace_snapshot(GcTriggerKind::Manual));
     run_cycle_until_phase(&mut state, GcCyclePhase::RootScan);
 
     let mut root_steps = 0usize;

crates/perry-runtime/src/gc/tests/mod.rs

@@ -8,6 +8,7 @@ mod budgeted_step_api;
 mod buffer_bound_method_name;
 mod buffer_side_tables;
 mod census;
+mod retention_9628_9629;
 mod concat_site;
 mod contract;
 mod copying;