diff --git a/changelog.d/10491-copying-minor-single-decode.md b/changelog.d/10491-copying-minor-single-decode.md new file mode 100644 index 0000000000..ee5939e024 --- /dev/null +++ b/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. diff --git a/crates/perry-runtime/src/gc/copying.rs b/crates/perry-runtime/src/gc/copying.rs index 52442f4f6f..7a510d1d64 100644 --- a/crates/perry-runtime/src/gc/copying.rs +++ b/crates/perry-runtime/src/gc/copying.rs @@ -313,19 +313,6 @@ impl CopyingNurseryCollector { } } - pub(super) fn visit_value_bits(&mut self, bits: u64) -> Option { - 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 { 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 diff --git a/crates/perry-runtime/src/gc/copying_parent_facts.rs b/crates/perry-runtime/src/gc/copying_parent_facts.rs index 2cecd82327..19b6d06833 100644 --- a/crates/perry-runtime/src/gc/copying_parent_facts.rs +++ b/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 = 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 { + 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, 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 { + 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); } } } diff --git a/crates/perry-runtime/src/gc/tests/copy_slot_decode.rs b/crates/perry-runtime/src/gc/tests/copy_slot_decode.rs new file mode 100644 index 0000000000..68f6d1008c --- /dev/null +++ b/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 { + unsafe { + let s = addr as *const crate::StringHeader; + let data = (s as *const u8).add(std::mem::size_of::()); + 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::()) + 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 { + 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::() + .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:?}" + ); +} diff --git a/crates/perry-runtime/src/gc/tests/mod.rs b/crates/perry-runtime/src/gc/tests/mod.rs index 1ffdcb9893..3307fdea2b 100644 --- a/crates/perry-runtime/src/gc/tests/mod.rs +++ b/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_decode; mod copy_slot_hoists; mod copying; mod copying_side_tables;