perf(gc): decode a visited word once in the copying minor (#10362)

changelog.d/10491-copying-minor-single-decode.md

@@ -0,0 +1,35 @@
+Decode each word the copying minor visits once. A raw (untagged) word was
+classified twice: `CopyingPointerSet::decode_bits` classified it only to
+validate it, and `mark_addr` classified it again. Every traced shaped object
+visits its shape record's `keys` word, a raw address, so that was a second
+page-table probe and header read per traced object. The slot visit's
+remembering arm then re-decoded the slot it had just decoded. The validating
+classification is now the one the mark uses (the memo is still consulted after
+it, as before), and the remembering arm reuses the child the visit decoded;
+only a raw word that moved is validated again, which is all the re-decode could
+still reject.
+
+Two codegen facts are load-bearing and pinned by comment. The decode is
+`#[inline(always)]`: out of line, its call frame and the by-memory return of
+its result cost as much as the classification it saves (the first cut measured
+flat to +1.05%). And `barrier_parent_needs_remembering` is asked before the
+visit rather than after. It reads only the parent and the slot's address, so
+the order does not change the answer, but asked after, the optimizer
+duplicated the call into both decode arms and stopped inlining it, which gave
+back a third of the win on gc3 (-1.20% instead of -1.80%) and more than a third
+on w20000 (-0.84% instead of -1.44%).
+
+Measured on six GC fixtures, instructions:u min-of-5: gc3 -1.83%, w5000 -1.77%,
+w20000 -1.52%, oldyoung -1.46%, w1000 -0.84%, alloc flat (-951 instructions).
+Exact instruction counts under callgrind agree: gc3 -1.79%, with
+`classify_arena` calls down from 6.09M to 4.20M. On the pointer-slot control
+(60k records whose K fields all point at one shared object, against the same
+records holding doubles) the per-slot term falls from 379.2 to 349.6
+instructions at K=16, counted exactly under callgrind: a memo hit no longer pays a call to
+`mark_addr`, and the re-decode's classification is gone.
+
+The page-generation cache was read before any of this was attempted. It runs
+the direct-mapped table arm with a 93.4-97.3% hit rate, and at most 0.02% of
+lookups are capacity misses. Nearly every miss is an address in no registered
+block: the shape record's `keys` slot, which lives outside the heap. So the
+cache's size was not the problem, and nothing here changes it.

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

@@ -313,19 +313,6 @@ impl CopyingNurseryCollector {
         }
     }
 
-    pub(super) fn visit_value_bits(&mut self, bits: u64) -> Option<u64> {
-        let (addr, is_nanbox, tag) = self.ptrs.decode_bits(bits)?;
-        let new_addr = self.mark_addr(addr)?;
-        if new_addr == addr {
-            return None;
-        }
-        Some(if is_nanbox {
-            tag | (new_addr as u64 & POINTER_MASK)
-        } else {
-            new_addr as u64
-        })
-    }
-
     pub(super) fn visit_raw_addr(&mut self, addr: usize) -> Option<usize> {
         let new_addr = self.mark_addr(addr)?;
         (new_addr != addr).then_some(new_addr)
@@ -430,6 +417,20 @@ impl CopyingNurseryCollector {
             return Some(self.memo_result);
         }
         let ptr = self.ptrs.classify(addr)?;
+        Some(self.mark_classified(addr, ptr))
+    }
+
+    /// [`mark_addr`](Self::mark_addr) for an address the caller has already
+    /// classified: the memo, then the mark, without classifying again.
+    #[inline]
+    pub(super) fn mark_classified_addr(&mut self, addr: usize, ptr: CopyingPointer) -> usize {
+        if addr == self.memo_addr {
+            return self.memo_result;
+        }
+        self.mark_classified(addr, ptr)
+    }
+
+    fn mark_classified(&mut self, addr: usize, ptr: CopyingPointer) -> usize {
         let result = match ptr.kind {
             CopyingPointerKind::Eden | CopyingPointerKind::FromSurvivor => unsafe {
                 self.move_young(ptr)
@@ -457,7 +458,7 @@ impl CopyingNurseryCollector {
         };
         self.memo_addr = addr;
         self.memo_result = result;
-        Some(result)
+        result
     }
 
     /// #7742: the object's block is being promoted whole, in place. It does not

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

@@ -1,5 +1,6 @@
-//! 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.
+//! The per-parent weak-holder fact the copying minor's slot visit reads, the
+//! slot visit itself, and its single decode of the visited word. Split out of
+//! `gc/copying.rs` for the 2000-line lint.
 
 use super::*;
 
@@ -58,7 +59,107 @@ pub(crate) mod copy_hoist_sabotage {
     }
 }
 
+/// Test-only sabotage for the single decode per slot visit
+/// (`visit_value_bits_child`). Witness: `gc::tests::copy_slot_decode`.
+#[cfg(test)]
+pub(crate) mod copy_decode_sabotage {
+    use std::cell::Cell;
+
+    /// The validated raw word is dropped instead of marked.
+    pub(crate) const RAW_MARK: u8 = 1;
+    /// The remembering arm loses the child the visit decoded.
+    pub(crate) const CHILD: u8 = 2;
+
+    thread_local! {
+        static FORGET: Cell<u8> = const { Cell::new(0) };
+    }
+
+    pub(crate) fn forgetting(what: u8) -> bool {
+        FORGET.with(|f| f.get() & what != 0)
+    }
+
+    pub(crate) struct Guard(u8);
+
+    impl Guard {
+        pub(crate) fn arm(what: u8) -> Self {
+            Self(FORGET.with(|f| f.replace(f.get() | what)))
+        }
+    }
+
+    impl Drop for Guard {
+        fn drop(&mut self) {
+            FORGET.with(|f| f.set(self.0));
+        }
+    }
+}
+
 impl CopyingNurseryCollector {
+    /// The root visitors' form of [`Self::visit_value_bits_child`]. Always
+    /// inlined too: out of line it added a call frame per root word.
+    #[inline(always)]
+    pub(super) fn visit_value_bits(&mut self, bits: u64) -> Option<u64> {
+        self.visit_value_bits_child(bits)?.1
+    }
+
+    /// Decode, classify and mark one value word ONCE: the child's address as
+    /// the word reads after this visit, the word's new bits if the child
+    /// moved, and whether the word was raw. `None` is exactly
+    /// `CopyingPointerSet::decode_bits`'s `None`: not a heap reference.
+    ///
+    /// A raw word used to be classified twice — by `decode_bits`, only to
+    /// validate it, and again by `mark_addr`. Every traced shaped object visits
+    /// its shape record's `keys` word, a raw address, so that was a second
+    /// page-table probe and header read per traced object. The validating
+    /// classification is now the one the mark uses; the memo is still
+    /// consulted after it, as before.
+    ///
+    /// Always inlined: out of line, the extra frame and the by-memory return
+    /// of the triple cost as much as the classification it saves.
+    #[inline(always)]
+    pub(super) fn visit_value_bits_child(
+        &mut self,
+        bits: u64,
+    ) -> Option<(usize, Option<u64>, bool)> {
+        let tag = bits & TAG_MASK;
+        if tag == POINTER_TAG || tag == STRING_TAG || tag == BIGINT_TAG {
+            let addr = (bits & POINTER_MASK) as usize;
+            if addr == 0 {
+                return None;
+            }
+            // An unclassifiable NaN-boxed word is still a reference to the
+            // remembering arm, which never classified NaN-boxed words.
+            return Some(match self.mark_addr(addr) {
+                Some(new_addr) if new_addr != addr => {
+                    let new_bits = tag | (new_addr as u64 & POINTER_MASK);
+                    ((new_bits & POINTER_MASK) as usize, Some(new_bits), false)
+                }
+                _ => (addr, None, false),
+            });
+        }
+        if tag >= 0x7FF8_0000_0000_0000 || !CopyingPointerSet::raw_pointer_candidate(bits) {
+            return None;
+        }
+        let addr = bits as usize;
+        let ptr = self.ptrs.classify(addr)?;
+        #[cfg(test)]
+        if copy_decode_sabotage::forgetting(copy_decode_sabotage::RAW_MARK) {
+            return None;
+        }
+        let new_addr = self.mark_classified_addr(addr, ptr);
+        Some((
+            new_addr,
+            (new_addr != addr).then_some(new_addr as u64),
+            true,
+        ))
+    }
+
+    /// The remembering arm's one remaining re-decode: a raw word that moved.
+    /// Out of line so the rare case does not grow the slot visit it sits in.
+    #[inline(never)]
+    fn revalidate_moved_raw(&self, bits: u64) -> Option<usize> {
+        self.ptrs.decode_bits(bits).map(|(addr, _, _)| addr)
+    }
+
     pub(super) unsafe fn visit_slot_with_parent(
         &mut self,
         slot: *mut u64,
@@ -100,22 +201,39 @@ impl CopyingNurseryCollector {
             self.weak_slots.push(slot);
             return;
         }
-        let bits = *slot;
-        if let Some(new_bits) = self.visit_value_bits(bits) {
+        // Asked BEFORE the visit: it reads only the parent and the slot's own
+        // address, never the child. Asked after, the optimizer duplicated the
+        // call into both decode arms and then stopped inlining it.
+        let remembering = !parent_header.is_null()
+            && !self.skip_remembering
+            && barrier_parent_needs_remembering(
+                (parent_header as *mut u8).add(GC_HEADER_SIZE) as usize,
+                external,
+            );
+        let visited = self.visit_value_bits_child(*slot);
+        if let Some((_, Some(new_bits), _)) = visited {
             *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);
-                    }
-                }
+        if !remembering {
+            return;
+        }
+        // The visit above already decoded this word; re-decoding `*slot`
+        // repeated it. Only a raw word that MOVED is validated again, which is
+        // all the re-decode could still reject.
+        let child = match visited {
+            Some((_, Some(new_bits), true)) => self.revalidate_moved_raw(new_bits),
+            other => other.map(|(addr, _, _)| addr).filter(|&addr| addr != 0),
+        };
+        #[cfg(test)]
+        let child =
+            child.filter(|_| !copy_decode_sabotage::forgetting(copy_decode_sabotage::CHILD));
+        if let Some(child_addr) = child {
+            // 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/tests/copy_slot_decode.rs

@@ -0,0 +1,141 @@
+//! The copying minor decodes each visited word ONCE (`visit_value_bits_child`):
+//! a raw word's validating classification is the one the mark uses, and the
+//! remembering arm reuses the child the visit decoded instead of re-decoding
+//! the slot. Both halves are pinned by collections and what they leave in the
+//! heap, and each has a sabotaged twin that must fail.
+
+use super::super::*;
+use super::support::*;
+use crate::gc::copying_parent_facts::copy_decode_sabotage::{Guard, CHILD, RAW_MARK};
+
+fn string_bytes(addr: usize) -> Vec<u8> {
+    unsafe {
+        let s = addr as *const crate::StringHeader;
+        let data = (s as *const u8).add(std::mem::size_of::<crate::StringHeader>());
+        std::slice::from_raw_parts(data, (*s).byte_len as usize).to_vec()
+    }
+}
+
+/// A young string reachable ONLY through a RAW (untagged) word in a rooted
+/// young object. Returns whether the minor evacuated it through that word.
+fn raw_child_evacuated(sabotaged: bool) -> bool {
+    std::thread::spawn(move || {
+        let _guard = CopyingNurseryTestGuard::new(1);
+        let _triggers = GcTriggerThresholdTestGuard::suppress_automatic_triggers();
+        let _scan = ConservativeScanDisabledGuard::new();
+        let _roots = ShadowAndGlobalRootResetGuard;
+        let (parent, fields) = unsafe { alloc_nursery_test_object(1) };
+        let child = young_leaf();
+        let expected = string_bytes(child);
+        unsafe { *fields = child as u64 };
+        js_shadow_slot_set(0, ptr_bits(parent as usize));
+        assert!(
+            crate::arena::pointer_in_nursery(child),
+            "premise: the child must be young, or there is nothing to evacuate"
+        );
+        {
+            let _sabotage = sabotaged.then(|| Guard::arm(RAW_MARK));
+            let _ = gc_collect_minor();
+        }
+        let parent_after = (js_shadow_slot_get(0) & POINTER_MASK) as usize;
+        assert_ne!(
+            parent_after, parent as usize,
+            "premise: the rooted parent moved"
+        );
+        let word = unsafe {
+            *((parent_after as *const u8).add(std::mem::size_of::<crate::object::ObjectHeader>())
+                as *const u64)
+        };
+        // Checked before any read through `word`: a stale word names from-space.
+        word != child as u64 && string_bytes(word as usize) == expected
+    })
+    .join()
+    .expect("raw-word decode test thread must not panic")
+}
+
+#[test]
+fn a_raw_word_is_marked_through_its_validating_classification() {
+    assert!(
+        raw_child_evacuated(false),
+        "the young child behind a raw word must be evacuated and the word rewritten"
+    );
+}
+
+#[test]
+fn sabotaged_raw_mark_leaves_the_raw_word_stale() {
+    assert!(
+        !raw_child_evacuated(true),
+        "with the validated raw word dropped instead of marked, the child is not evacuated"
+    );
+}
+
+/// An OLD parent whose NaN-boxed slot holds a young child, handed to the minor
+/// through the write barrier, then two minors: the second finds the edge only
+/// if the first re-remembered it from the child its visit decoded. `Err` is
+/// the collection thread's panic message.
+fn old_edge_across_two_minors(sabotaged: bool) -> Result<bool, String> {
+    std::thread::spawn(move || {
+        let _guard = CopyingNurseryTestGuard::new(1);
+        let _tenuring = crate::gc::tenuring::set_survivals_for_test(
+            crate::gc::tenuring::GC_TENURING_SURVIVALS_MAX,
+        );
+        let _triggers = GcTriggerThresholdTestGuard::suppress_automatic_triggers();
+        let _scan = ConservativeScanDisabledGuard::new();
+        let _roots = ShadowAndGlobalRootResetGuard;
+        let (parent, fields) = unsafe { alloc_old_test_object(1) };
+        let child = young_leaf();
+        let expected = string_bytes(child);
+        unsafe { *fields = ptr_bits(child) };
+        js_write_barrier_slot(ptr_bits(parent as usize), fields as u64, ptr_bits(child));
+        assert!(
+            crate::arena::pointer_in_old_gen(parent as usize)
+                && crate::arena::pointer_in_nursery(child),
+            "premise: an old parent and a young child"
+        );
+        let read = || unsafe { (*fields & POINTER_MASK) as usize };
+        {
+            let _sabotage = sabotaged.then(|| Guard::arm(CHILD));
+            let _ = gc_collect_minor();
+        }
+        let first = read();
+        assert!(
+            first != child && crate::arena::pointer_in_nursery(first),
+            "premise: the first minor copied the child within the nursery"
+        );
+        let _ = gc_collect_minor();
+        let second = read();
+        second != first && string_bytes(second) == expected
+    })
+    .join()
+    .map_err(|payload| {
+        payload
+            .downcast_ref::<String>()
+            .cloned()
+            .or_else(|| payload.downcast_ref::<&str>().map(|s| s.to_string()))
+            .unwrap_or_default()
+    })
+}
+
+#[test]
+fn an_old_parents_edge_is_remembered_from_the_child_the_visit_decoded() {
+    assert_eq!(
+        old_edge_across_two_minors(false),
+        Ok(true),
+        "the second minor must find and move the child through the remembered edge"
+    );
+}
+
+/// In a release build `restore_surviving_dirty_coverage` would re-add the page
+/// the arm failed to remember, which is why a forgotten remembered-set entry
+/// is invisible to a survival check alone. In the debug build `cargo test`
+/// runs, the same walk cross-checks the dirty scan's per-slot re-remembering
+/// and refuses the disagreement — that refusal is this twin's observable.
+#[test]
+fn sabotaged_remembering_arm_is_refused_by_the_coverage_cross_check() {
+    let outcome = old_edge_across_two_minors(true);
+    assert!(
+        matches!(&outcome, Err(message) if message.contains("restore_surviving_dirty_coverage")),
+        "with the decoded child forgotten, the coverage walk must report the \
+         unremembered page; got {outcome:?}"
+    );
+}