perf(gc): read the copying minor's weak-holder fact once per object (#10362)

changelog.d/10388-copying-minor-weak-holder-fact.md

@@ -0,0 +1,38 @@
+Read the copying minor's weak-holder question once per traced object instead of
+once per slot. `visit_slot_with_parent` called `weakref::is_weak_target_trace_slot`
+for every slot of every object, an out-of-line call that re-reads the parent's
+`obj_type` and `class_id` and then rejects on class for every ordinary object.
+The per-object form (`weakref::is_weak_holder_header`) already existed: #10182
+gave it to the full mark, and the copying minor never got it.
+
+**The fact is read lazily, on the first slot that needs it, not eagerly per
+object** — and that is a rule for this collector, not an implementation detail.
+Reading it eagerly, once per traced object, regressed every fixture:
+
+    gc3 +1.59%   oldyoung +5.09%   w20000 +1.44%
+    w1000 +0.88%   alloc +0.87%    w5000 +0.86%     (instructions:u, min of 5)
+
+with peak RSS +3% to +8% alongside. A great many traced objects — strings,
+pointer-free arrays — have no slot to visit at all, and an eager hoist makes
+every one of them pay for an answer nobody then asks for. Any future per-object
+hoist on this path has to be lazy for the same reason.
+
+Measured on six GC fixtures, instructions:u min-of-5: gc3 -0.99%, w5000 -1.61%,
+w20000 -1.37%, oldyoung -1.10%, w1000 -0.81%, alloc flat. Peak RSS within 0.1%
+on all six and max pause better or flat on all six. On a control that isolates
+the slot term (60k records whose fields all point at one shared object, against
+the same records holding doubles, so the mutator difference cancels between
+arms), the pointer-slot-attributable instruction count falls 4.4% at 2 slots per
+object, 5.9% at 8 and 6.3% at 16 — 25.5 instructions per pointer-slot visit,
+which cross-checks against the 6.7 cycles/slot of self time the profile
+attributed to `is_weak_target_trace_slot`.
+
+The companion hoist for `barrier_parent_needs_remembering`'s generation clause is
+deliberately NOT included. It measured as the half that makes a
+one-pointer-slot object pay (oldyoung +0.198% with it, -1.10% without), and no
+sabotage of it could be made to fail: sticky dirty-page coverage carries an
+old-to-young edge independently of the remembered-set re-insertion, so forgetting
+the fact changes nothing observable. It needs its own witness first.
+
+`gc/copying.rs` loses the slot visit to a new `gc/copying_parent_facts.rs`, which
+the 2000-line file lint required and which leaves `copying.rs` smaller than before.

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

@@ -1,3 +1,4 @@
+use super::copying_parent_facts::weak_holder_fact;
 use super::copying_phase::{
     finalize_dead_copied_minor_from_space_side_allocations, CopyingMinorPhase as Phase,
     CopyingMinorPhaseDiag as PhaseDiag,
@@ -116,12 +117,15 @@ impl CopyingNurseryPreflight {
     }
 
     pub(super) unsafe fn scan_object_fields(&mut self, header: *mut GcHeader) {
+        let mut weak_holder: Option<bool> = None;
         visit_gc_rewrite_slots(header, |slot| unsafe {
             // Weak-only reachability imposes no copy constraint: the
             // collector never evacuates through a weak edge (a weak-only
             // young target dies in place and tombstones), so a pinned
             // target behind one must not force the fallback path.
-            if crate::weakref::is_weak_target_trace_slot(header, slot.slot) {
+            if *weak_holder.get_or_insert_with(|| weak_holder_fact(header))
+                && crate::weakref::is_weak_target_trace_slot(header, slot.slot)
+            {
                 return;
             }
             slot.record_layout_read();
@@ -650,55 +654,6 @@ impl CopyingNurseryCollector {
         new_user as usize
     }
 
-    pub(super) unsafe fn visit_slot_with_parent(
-        &mut self,
-        slot: *mut u64,
-        parent_header: *mut GcHeader,
-        external: bool,
-    ) {
-        if slot.is_null() {
-            return;
-        }
-        // Weak target edge (WeakRef referent / weak entry key / finreg
-        // record target): never evacuate through it — the mark/barrier
-        // paths skip these (`is_weak_target_trace_slot`), and copying
-        // through them strengthened the reference, so WeakMap entries
-        // never tombstoned and FinalizationRegistry never fired while
-        // copied-minor was the operative cycle. Repair an already-moved
-        // target's address now and queue the slot so `repair_weak_slots`
-        // fixes targets evacuated after this visit; the registry pass then
-        // tombstones dead ones.
-        // No remembered-set entry either — the write barrier skips weak
-        // slots the same way.
-        if !parent_header.is_null()
-            && crate::weakref::is_weak_target_trace_slot(parent_header, slot)
-        {
-            if let Some(new_bits) = self.rewrite_value_bits(*slot) {
-                *slot = new_bits;
-            }
-            self.weak_slots.push(slot);
-            return;
-        }
-        let bits = *slot;
-        if let Some(new_bits) = self.visit_value_bits(bits) {
-            *slot = new_bits;
-        }
-        if !parent_header.is_null() && !self.skip_remembering {
-            let parent_user = (parent_header as *mut u8).add(GC_HEADER_SIZE) as usize;
-            if barrier_parent_needs_remembering(parent_user, external) {
-                if let Some((child_addr, _, _)) = self.ptrs.decode_bits(*slot) {
-                    // Keep old→malloc pages dirty alongside old→nursery:
-                    // the malloc child is spared by this cycle's mark
-                    // (mark_addr handles CopyingPointerKind::Malloc) but
-                    // the NEXT minor's malloc sweep needs the edge again.
-                    if crate::gc::barrier::remembered_child_needs_tracking(child_addr) {
-                        self.sticky.remember_slot(parent_header, slot, external);
-                    }
-                }
-            }
-        }
-    }
-
     pub(super) unsafe fn drain(&mut self) {
         let mut i = 0usize;
         while i < self.worklist.len() {
@@ -743,10 +698,16 @@ impl CopyingNurseryCollector {
 
     pub(super) unsafe fn scan_object_fields(&mut self, header: *mut GcHeader) {
         let mut changed = false;
+        // LAZY, not eager. Reading the fact once per traced OBJECT regressed
+        // all six fixtures (+0.88 % to +5.09 % instructions): a great many
+        // traced objects — strings, pointer-free arrays — have no slot to
+        // visit at all, and paid for an answer nobody then asked for.
+        let mut weak_holder: Option<bool> = None;
         visit_gc_rewrite_slots(header, |slot| unsafe {
             slot.record_layout_read();
             let before = *slot.slot;
-            self.visit_slot_with_parent(slot.slot, header, slot.external());
+            let weak = *weak_holder.get_or_insert_with(|| weak_holder_fact(header));
+            self.visit_slot_with_weak_fact(slot.slot, header, weak, slot.external());
             changed |= *slot.slot != before;
         });
         if changed {

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

@@ -0,0 +1,122 @@
+//! The per-parent weak-holder fact the copying minor's slot visit reads, and
+//! the slot visit itself. Split out of `gc/copying.rs` for the 2000-line lint.
+
+use super::*;
+
+/// Is this parent one of the weak-holder classes whose weak slots the copying
+/// minor must not evacuate through?
+///
+/// The question is a property of the PARENT's class, but the collector asked
+/// the per-SLOT question (`weakref::is_weak_target_trace_slot`) for every slot
+/// of every object — an out-of-line call that re-reads `obj_type` and
+/// `class_id` and then rejects on class. #10182 gave the full mark the
+/// per-object read (`gc/trace.rs`); the copying minor never got it.
+///
+/// Read LAZILY by the callers: once per object is right only for objects that
+/// actually have a slot to visit. See `scan_object_fields`.
+///
+/// Null-safe: `is_weak_holder_header` answers false for a null header, which
+/// is the same answer the per-slot question gave.
+#[inline]
+pub(super) unsafe fn weak_holder_fact(header: *mut GcHeader) -> bool {
+    #[cfg(test)]
+    if copy_hoist_sabotage::forgetting_weak() {
+        return false;
+    }
+    crate::weakref::is_weak_holder_header(header)
+}
+
+/// Test-only sabotage for [`weak_holder_fact`]: forgetting the per-object fact
+/// must change what the collector does, or the hoist is documentation
+/// (CLAUDE.md, a gate that cannot fail). Its witness is
+/// `gc::tests::copy_slot_hoists`.
+#[cfg(test)]
+pub(crate) mod copy_hoist_sabotage {
+    use std::cell::Cell;
+
+    thread_local! {
+        static FORGET_WEAK: Cell<bool> = const { Cell::new(false) };
+    }
+
+    #[inline]
+    pub(crate) fn forgetting_weak() -> bool {
+        FORGET_WEAK.with(Cell::get)
+    }
+
+    pub(crate) struct WeakGuard(bool);
+
+    impl WeakGuard {
+        pub(crate) fn arm() -> Self {
+            Self(FORGET_WEAK.with(|s| s.replace(true)))
+        }
+    }
+
+    impl Drop for WeakGuard {
+        fn drop(&mut self) {
+            FORGET_WEAK.with(|s| s.set(self.0));
+        }
+    }
+}
+
+impl CopyingNurseryCollector {
+    pub(super) unsafe fn visit_slot_with_parent(
+        &mut self,
+        slot: *mut u64,
+        parent_header: *mut GcHeader,
+        external: bool,
+    ) {
+        let weak_holder = weak_holder_fact(parent_header);
+        self.visit_slot_with_weak_fact(slot, parent_header, weak_holder, external);
+    }
+
+    /// [`visit_slot_with_parent`](Self::visit_slot_with_parent) with the
+    /// parent's weak-holder fact supplied by the caller, so a whole object's
+    /// slots pay for it once. See [`weak_holder_fact`].
+    pub(super) unsafe fn visit_slot_with_weak_fact(
+        &mut self,
+        slot: *mut u64,
+        parent_header: *mut GcHeader,
+        weak_holder: bool,
+        external: bool,
+    ) {
+        if slot.is_null() {
+            return;
+        }
+        // Weak target edge (WeakRef referent / weak entry key / finreg
+        // record target): never evacuate through it — the mark/barrier
+        // paths skip these (`is_weak_target_trace_slot`), and copying
+        // through them strengthened the reference, so WeakMap entries
+        // never tombstoned and FinalizationRegistry never fired while
+        // copied-minor was the operative cycle. Repair an already-moved
+        // target's address now and queue the slot so `repair_weak_slots`
+        // fixes targets evacuated after this visit; the registry pass then
+        // tombstones dead ones.
+        // No remembered-set entry either — the write barrier skips weak
+        // slots the same way.
+        if weak_holder && crate::weakref::is_weak_target_trace_slot(parent_header, slot) {
+            if let Some(new_bits) = self.rewrite_value_bits(*slot) {
+                *slot = new_bits;
+            }
+            self.weak_slots.push(slot);
+            return;
+        }
+        let bits = *slot;
+        if let Some(new_bits) = self.visit_value_bits(bits) {
+            *slot = new_bits;
+        }
+        if !parent_header.is_null() && !self.skip_remembering {
+            let parent_user = (parent_header as *mut u8).add(GC_HEADER_SIZE) as usize;
+            if barrier_parent_needs_remembering(parent_user, external) {
+                if let Some((child_addr, _, _)) = self.ptrs.decode_bits(*slot) {
+                    // Keep old→malloc pages dirty alongside old→nursery:
+                    // the malloc child is spared by this cycle's mark
+                    // (mark_addr handles CopyingPointerKind::Malloc) but
+                    // the NEXT minor's malloc sweep needs the edge again.
+                    if crate::gc::barrier::remembered_child_needs_tracking(child_addr) {
+                        self.sticky.remember_slot(parent_header, slot, external);
+                    }
+                }
+            }
+        }
+    }
+}

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

@@ -164,6 +164,7 @@ pub(crate) mod prefetch;
 
 mod copying;
 mod copying_first_cycle;
+mod copying_parent_facts;
 mod copying_phase;
 mod copying_pointer_set;
 mod diag_sites;

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

@@ -0,0 +1,70 @@
+//! The copying minor reads two facts once per traced object that it used to
+//! re-derive for every slot of that object: whether the parent is a weak
+//! holder, and whether the parent is in old-gen.
+//!
+//! Both are pinned by a COLLECTION and its observable outcome, not by reading
+//! the hoisted value back — and each has a sabotaged twin that forgets the
+//! fact, so the hoist is shown to be load-bearing rather than merely present.
+
+use super::super::*;
+use super::support::*;
+use crate::gc::copying_parent_facts::copy_hoist_sabotage;
+
+/// A young target reachable ONLY through a rooted `WeakRef`'s weak slot.
+/// A copying minor must not evacuate through that slot, so the target dies
+/// and the reference reads `undefined`.
+fn weak_target_cleared_by_minor(sabotaged: bool) -> bool {
+    std::thread::spawn(move || {
+        let _guard = CopyingNurseryTestGuard::new(1);
+        let _triggers = GcTriggerThresholdTestGuard::suppress_automatic_triggers();
+        let _scan = ConservativeScanDisabledGuard::new();
+        reset_global_roots();
+        let _roots = ShadowAndGlobalRootResetGuard;
+
+        // An OBJECT: a string is not "CanBeHeldWeakly", so `js_weakref_new`
+        // would reject it before the collector is ever involved.
+        let target = unsafe { alloc_nursery_test_object(0).0 } as usize;
+        assert!(
+            crate::arena::pointer_in_nursery(target),
+            "premise: the weak target must be young, or the minor cannot collect it"
+        );
+        let holder = crate::weakref::js_weakref_new(f64::from_bits(ptr_bits(target)));
+        let mut root = ptr_bits(holder as usize);
+        js_gc_register_global_root(&mut root as *mut u64 as i64);
+        assert!(
+            unsafe {
+                crate::weakref::is_weak_holder_header(
+                    header_from_user_ptr(holder as *const u8) as *mut GcHeader
+                )
+            },
+            "premise: a WeakRef is a weak holder"
+        );
+
+        {
+            let _sabotage = sabotaged.then(copy_hoist_sabotage::WeakGuard::arm);
+            let _ = gc_collect_minor();
+        }
+        crate::weakref::js_weakref_deref(f64::from_bits(root)).to_bits()
+            == crate::value::TAG_UNDEFINED
+    })
+    .join()
+    .expect("copy-hoist weak test thread must not panic")
+}
+
+#[test]
+fn a_copying_minor_skips_a_weak_holders_weak_slot_through_the_per_object_fact() {
+    assert!(
+        weak_target_cleared_by_minor(false),
+        "a target reachable only through the WeakRef's weak slot must not be \
+         evacuated through, so it dies in the nursery"
+    );
+}
+
+#[test]
+fn sabotaged_weak_holder_fact_evacuates_through_the_weak_slot() {
+    assert!(
+        !weak_target_cleared_by_minor(true),
+        "with the per-object weak-holder fact forgotten the weak slot is \
+         treated as strong and the target survives the minor"
+    );
+}

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

@@ -17,6 +17,7 @@ mod census_block_windows;
 mod census_whole_block;
 mod concat_site;
 mod contract;
+mod copy_slot_hoists;
 mod copying;
 mod copying_side_tables;
 mod cycle_state;