diff --git a/changelog.d/7193-barrier-parent-classify.md b/changelog.d/7193-barrier-parent-classify.md new file mode 100644 index 0000000000..21c01b7ec1 --- /dev/null +++ b/changelog.d/7193-barrier-parent-classify.md @@ -0,0 +1,54 @@ +### perf(gc): stop the runtime write barrier re-deriving a parent address it was handed + +`classify_heap_generation` is **19.03% of `benchmarks/app-patterns/kernels/batch.ts`** +(657M instructions, #7170's ranked profile) — the single largest symbol in that +program, called from the write barrier, **with zero collections running**. Four +page-map classifications happen per barriered element store on that path, and +one of them answers a question the caller had already answered. + +**What was redundant.** `runtime_write_barrier_slot` receives `parent_addr: usize` +— a decoded GC user pointer — and handed it to `js_write_barrier_slot` as a bare +`u64`, so `decode_heap_addr` fell into its "possible raw pointer" arm and paid a +full `classify_heap_generation` to recover it, immediately before +`barrier_parent_needs_remembering` classified the same address again. The three +runtime entry points now share `write_barrier_slot_decoded`, which keeps the +parent as a `usize` throughout. Outcome-preserving: a malloc-GC parent used to +exit at `NonPointerParentSkips` (classification returned `Unknown`, so the decode +returned 0) and now exits at `ParentNotOldSkips` — different counter, same +remembered-set effect. `runtime_write_barrier_slot_matches_nanboxed_entry_point` +pins that across every parent generation x child kind. + +**A latent deref the round-trip was hiding.** `malloc_gc_parent_addr` +dereferences `parent_addr - GC_HEADER_SIZE` behind a bare +`< GC_HEADER_SIZE + 0x1000` floor, which admits every handle-band id and every +out-of-range garbage word. It was safe only because its callers happened to +filter first — including *by accident*: `runtime_write_barrier_external_slot` +NaN-boxed its parent, and an address with high bits set ORs into something that +is no longer `POINTER_TAG`, so the decode rejected it. +`closure/dynamic_props.rs` parks props under exactly such non-address owner keys +and depended on that accident. Both filters are now one explicit predicate, +`barrier_parent_addr_is_dereferenceable` (the canonical +`addr_class::is_plausible_heap_addr`, plus the 8-alignment `addr_class` does not +cover), applied in `write_barrier_slot_decoded` *and* inside +`malloc_gc_parent_addr` itself — so the function that dereferences carries its +own guard rather than trusting callers. + +**And the page-generation map's hasher.** `PAGE_GENERATIONS` carried a bespoke +identity hasher whose `write_usize` stored the key verbatim. `HashMap` is +hashbrown: the bucket index comes from the hash's low bits, but the SIMD control +byte is `hash >> 57`. Keys are `addr >> GENERATION_CLASS_SHIFT` — around 2^26 — +so **the control byte was zero for every entry in the table**, every group probe +matched every occupied slot, and each match cost a real key comparison before +the right one was found, on a lookup the write barrier performs several times +per heap store. Measured by the new regression test with the old hasher +reinstated: **1 distinct control byte across 64 consecutive buckets.** The map +now uses `fast_hash::PtrHasher`, the project's existing answer to this exact +failure (see its module doc and the 455 ms -> 830 ms regression its `mix` step +records). The map is only ever point-queried, so iteration order is not +observable. + +Design note for the remaining three classifications — lazy barrier arming, and +why "nothing has collected yet" is *not* sufficient on its own (born-old +allocation creates old->young edges before any collector runs) — is on #7187. + +Refs #7187, #7170, #5094. diff --git a/crates/perry-runtime/src/arena/mod.rs b/crates/perry-runtime/src/arena/mod.rs index b8adae9307..6c361783da 100644 --- a/crates/perry-runtime/src/arena/mod.rs +++ b/crates/perry-runtime/src/arena/mod.rs @@ -6,8 +6,7 @@ pub(crate) use std::alloc::{alloc, Layout}; pub(crate) use std::cell::{Cell, RefCell, UnsafeCell}; -pub(crate) use std::collections::{hash_map::Entry, HashMap}; -pub(crate) use std::hash::{BuildHasherDefault, Hasher}; +pub(crate) use std::collections::hash_map::Entry; mod allocators; mod block; diff --git a/crates/perry-runtime/src/arena/page_meta.rs b/crates/perry-runtime/src/arena/page_meta.rs index a71dcbe598..7767ed2598 100644 --- a/crates/perry-runtime/src/arena/page_meta.rs +++ b/crates/perry-runtime/src/arena/page_meta.rs @@ -108,31 +108,23 @@ impl PageGenerationCache { } } -#[derive(Default)] -struct IdentityHasher(u64); - -impl Hasher for IdentityHasher { - #[inline] - fn write(&mut self, bytes: &[u8]) { - let mut hash = 0u64; - for (idx, byte) in bytes.iter().take(8).enumerate() { - hash |= (*byte as u64) << (idx * 8); - } - self.0 = hash; - } - - #[inline] - fn write_usize(&mut self, value: usize) { - self.0 = value as u64; - } - - #[inline] - fn finish(&self) -> u64 { - self.0 - } -} - -type PageGenerationMap = HashMap>; +/// #7187: this map used to carry a bespoke identity hasher (`write_usize` +/// stored the key verbatim). `HashMap` is hashbrown, which takes the bucket +/// index from the hash's LOW bits and the SIMD control byte from +/// `hash >> 57`. Keys here are `addr >> GENERATION_CLASS_SHIFT` — around 2^26 +/// for a typical heap address — so the top seven bits were zero for **every** +/// entry in the table, and every group probe matched every occupied slot in +/// the group. Each of those matches costs a real key comparison (a scattered +/// load into a bucket) before the right one is found, on a lookup that the +/// write barrier performs several times per heap store. +/// +/// `fast_hash::PtrHasher` is the project's existing answer to exactly this — +/// see its module doc, and the `mix(h) = h ^ (h >> 32)` avalanche step whose +/// comment records a 455 ms → 830 ms regression from omitting it. The map is +/// only ever point-queried (`get` / `get_mut` / `insert` / `remove`; the +/// `first_key..=last_key` loops walk key *ranges*, not the map), so iteration +/// order is not observable and this carries no determinism exposure. +type PageGenerationMap = crate::fast_hash::PtrHashMap; type OldGenPageObjectMap = crate::fast_hash::PtrHashMap>; type OldGenPageMetaMap = crate::fast_hash::PtrHashMap; @@ -248,7 +240,7 @@ pub(crate) struct OldArenaSourceBlockSelection { thread_local! { static PAGE_GENERATIONS: RefCell = - RefCell::new(HashMap::with_hasher(BuildHasherDefault::::default())); + RefCell::new(crate::fast_hash::new_ptr_hash_map()); static PAGE_GENERATION_CACHE: Cell = const { Cell::new(PageGenerationCache::empty()) }; @@ -919,3 +911,104 @@ pub(crate) fn old_page_meta_for_tests(page: usize) -> Option { .map(|page_meta| normalize_dirty_slots_for_epoch(page_meta, current_epoch)) }) } + +#[cfg(test)] +mod page_generation_hasher_tests { + use super::*; + use std::collections::HashSet; + use std::hash::BuildHasher; + + /// #7187 regression guard for `PageGenerationMap`'s hasher. + /// + /// `HashMap` is hashbrown: the bucket index comes from the hash's low bits, + /// but the SIMD control byte — the filter that decides whether a group + /// probe needs a real key comparison — is `hash >> 57`. Generation class + /// keys are `addr >> GENERATION_CLASS_SHIFT`, so an identity hasher (which + /// this map carried until #7187) produces a value around 2^26 whose top + /// seven bits are zero for **every** key in the table. Every occupied slot + /// in a probed group then matches, and each match costs a scattered load + /// plus a key comparison — on a lookup the write barrier performs several + /// times per heap store. + /// + /// This asserts the property directly rather than asserting "we call + /// `PtrHasher`": reinstating any non-mixing hasher collapses the control + /// byte to a single value and fails here. + #[test] + fn control_byte_is_spread_across_generation_class_keys() { + let map = PageGenerationMap::default(); + let build = map.hasher(); + + // Realistic 48-bit heap addresses, one per 1 MiB generation bucket — + // the exact key population `classify_heap_generation` looks up. + let base: usize = 0x0000_7f31_0000_0000; + let control_bytes: HashSet = (0..64) + .map(|i| { + let addr = base + i * (1usize << GENERATION_CLASS_SHIFT); + (build.hash_one(generation_class_key_for_addr(addr)) >> 57) & 0x7f + }) + .collect(); + + assert!( + control_bytes.len() >= 32, + "hashbrown control byte must vary across generation class keys, got {} \ + distinct values from 64 consecutive buckets (an identity hasher yields 1)", + control_bytes.len() + ); + } + + /// The bucket index (low bits) must stay well spread too — mixing that put + /// all the entropy in the high bits and left the low bits constant would + /// trade a control-byte collision for a far worse bucket collision. This is + /// the failure `fast_hash`'s `mix` step exists for. + #[test] + fn bucket_index_is_spread_across_generation_class_keys() { + let map = PageGenerationMap::default(); + let build = map.hasher(); + + let base: usize = 0x0000_7f31_0000_0000; + let low_bits: HashSet = (0..64) + .map(|i| { + let addr = base + i * (1usize << GENERATION_CLASS_SHIFT); + build.hash_one(generation_class_key_for_addr(addr)) & 0x3f + }) + .collect(); + + assert!( + low_bits.len() >= 32, + "bucket index must vary across generation class keys, got {} distinct \ + values from 64 consecutive buckets", + low_bits.len() + ); + } + + /// The map must still answer correctly after the hasher change — a + /// point-query round trip over many buckets, which is the only way this map + /// is ever used. + #[test] + fn point_queries_round_trip_across_many_buckets() { + let mut map = PageGenerationMap::default(); + let base: usize = 0x0000_7f31_0000_0000; + for i in 0..256usize { + let addr = base + i * (1usize << GENERATION_CLASS_SHIFT); + map.insert( + generation_class_key_for_addr(addr), + PageGenerationSlot::Single(PageGenerationRange { + base: addr, + end: addr + (1 << GENERATION_CLASS_SHIFT), + generation: HeapGeneration::Old, + space: HeapSpace::Old, + }), + ); + } + for i in 0..256usize { + let addr = base + i * (1usize << GENERATION_CLASS_SHIFT); + let found = map + .get(&generation_class_key_for_addr(addr)) + .and_then(|slot| slot.find(addr + 0x40)) + .expect("every inserted bucket must be found by point query"); + assert_eq!(found.generation, HeapGeneration::Old); + assert_eq!(found.base, addr); + } + assert_eq!(map.len(), 256); + } +} diff --git a/crates/perry-runtime/src/gc/barrier.rs b/crates/perry-runtime/src/gc/barrier.rs index 22f785c399..55bd79dde8 100644 --- a/crates/perry-runtime/src/gc/barrier.rs +++ b/crates/perry-runtime/src/gc/barrier.rs @@ -964,20 +964,126 @@ pub(super) fn write_barrier_slot_inner( // unconditional thread-local access, which dominated tight numeric store // loops (#6011: `ema[i] = ` spent more time in this preamble than // in the store itself). + let Some(child_addr) = barrier_child_prologue(child) else { + return; + }; + // Decode the parent — must be a NaN-boxed heap pointer. + let parent_addr = decode_heap_addr(parent); + if parent_addr == 0 { + bump_write_barrier_trace_counter(BarrierTraceCounter::NonPointerParentSkips); + return; + } + write_barrier_decoded_parent(parent_addr, slot_addr, child_addr, external_slot); +} + +/// The never-skippable half of the barrier: decode the stored child and shade +/// it for any in-progress incremental cycle. Returns the child's heap address, +/// or `None` when the store published no heap pointer at all (every numeric +/// array/field store — the #6011 fast path, which must stay the cheapest exit). +#[inline] +fn barrier_child_prologue(child: u64) -> Option { let child_addr = decode_heap_addr(child); + bump_write_barrier_trace_counter(BarrierTraceCounter::Calls); if child_addr == 0 { - bump_write_barrier_trace_counter(BarrierTraceCounter::Calls); bump_write_barrier_trace_counter(BarrierTraceCounter::NonPointerChildSkips); - return; + return None; } - bump_write_barrier_trace_counter(BarrierTraceCounter::Calls); incremental_mark_barrier_value(child); - // Decode the parent — must be a NaN-boxed heap pointer. - let parent_addr = decode_heap_addr(parent); - if parent_addr == 0 { + Some(child_addr) +} + +/// [`write_barrier_slot_inner`] for a caller that already holds the parent as +/// a plain GC user pointer — see [`write_barrier_decoded_parent`] for why the +/// `u64` round-trip is worth avoiding (#7187). +pub(super) fn write_barrier_slot_decoded( + parent_addr: usize, + slot_addr: usize, + child: u64, + external_slot: bool, +) { + let Some(child_addr) = barrier_child_prologue(child) else { + return; + }; + // The NaN-box round-trip this replaces was also FILTERING, not just + // decoding, and dropping the filter is a segfault rather than a wrong + // answer: `barrier_parent_needs_remembering` reaches + // `malloc_gc_parent_addr`, which dereferences. Two filters were in play + // and both are reproduced here, explicitly: + // + // * bare-`u64` callers (`runtime_write_barrier_slot`) took + // `decode_heap_addr`'s raw-pointer arm: 48-bit, above `0x10000`, + // 8-aligned, then an arena classification. + // * NaN-boxing callers (`runtime_write_barrier_external_slot`) were + // filtered by ACCIDENT — a parent address with high bits set ORs into + // something that is no longer `POINTER_TAG`, so `decode_heap_addr` + // returned 0. `closure/dynamic_props.rs` parks props under + // non-address owner keys and relies on this (its unit test uses + // `0xC10C_AB1E_0000_1803`). + // + // [`barrier_parent_addr_is_dereferenceable`] subsumes both. A real GC user + // pointer — arena block or malloc — satisfies every clause, so no genuine + // old→young edge is filtered here. + if !barrier_parent_addr_is_dereferenceable(parent_addr) { bump_write_barrier_trace_counter(BarrierTraceCounter::NonPointerParentSkips); return; } + write_barrier_decoded_parent(parent_addr, slot_addr, child_addr, external_slot); +} + +/// May the barrier treat `parent_addr` as a GC user pointer — classify it and, +/// on the external-slot path, read its `GcHeader`? +/// +/// The canonical magnitude predicate plus the 8-alignment that +/// `decode_heap_addr`'s raw-pointer arm checked and `addr_class` does not: a +/// misaligned `GcHeader` read is UB before it is a wrong answer. +/// +/// This is a *plausibility* test, not a validity test. Its contract is the one +/// [`crate::value::addr_class::try_read_gc_header`] documents — an aligned, +/// in-range, but stale or unmapped address is still dereferenced, and the +/// `obj_type` / registry checks layered above are what catch reuse. +#[inline] +pub(super) fn barrier_parent_addr_is_dereferenceable(parent_addr: usize) -> bool { + crate::value::addr_class::is_plausible_heap_addr(parent_addr) && parent_addr.is_multiple_of(8) +} + +/// The remembered-set half of the barrier, entered with the parent address +/// **already decoded**. +/// +/// #7187: every Rust-side barrier caller holds the parent as a plain `usize` +/// GC user pointer. Routing those through [`write_barrier_slot_inner`] meant +/// re-encoding the address as a bare `u64` so [`decode_heap_addr`] could +/// re-derive it — and its bare-pointer arm pays a full +/// `classify_heap_generation` to do so, immediately before +/// [`barrier_parent_needs_remembering`] classifies the same address again. +/// That was one of FOUR page-map classifications per barriered store on the +/// `batch.ts` sort path (#7170 measured `classify_heap_generation` at 19.03% +/// of that program, ~657M instructions, with zero collections running), and +/// the only one that answered a question the caller had already answered. +/// +/// Dropping the round-trip is outcome-preserving, including for the one +/// operand class that reached the bare-pointer arm and failed it: a +/// malloc-GC parent classifies `Unknown`, so `decode_heap_addr` used to +/// return 0 and the barrier exited at `NonPointerParentSkips`. It now +/// reaches `barrier_parent_needs_remembering(parent, external_slot)`, which +/// classifies `Unknown`, is not `Old`, and — for the non-external callers +/// that took this path — exits at `ParentNotOldSkips`. Different counter, +/// same remembered-set effect (none). The external/malloc parents that +/// genuinely need remembering arrive through +/// [`runtime_write_barrier_external_slot`] / [`runtime_write_barrier_gc_slot`], +/// which already tag their parent and are unaffected. +/// +/// Callers must pass a real GC user pointer. `decode_heap_addr`'s shape +/// pre-filter (48-bit, above the handle band, 8-aligned) is not applied +/// here, because the Rust callers derive `parent_addr` from a live +/// `*mut ArrayHeader` / `*mut ObjectHeader` / … rather than from JS value +/// bits. +#[inline] +pub(super) fn write_barrier_decoded_parent( + parent_addr: usize, + slot_addr: usize, + child_addr: usize, + external_slot: bool, +) { // Old → young check. Runtime-owned malloc GC objects are outside // the nursery and must be treated as old when the caller uses the // external-slot path for fields or side buffers. @@ -1045,9 +1151,18 @@ pub(super) fn barrier_parent_needs_remembering(parent_addr: usize, external_slot external_slot && malloc_gc_parent_addr(parent_addr) } +/// #7187: this DEREFERENCES `parent_addr - GC_HEADER_SIZE`, and its only +/// pre-deref guard used to be a bare `< GC_HEADER_SIZE + 0x1000` floor — +/// which admits every handle-band id and every out-of-range garbage word. +/// It was safe purely because its callers happened to filter first +/// (`decode_heap_addr`'s shape pre-filter, or a NaN-box tag that a +/// non-canonical address corrupted into rejection). That is exactly the +/// "raw address deref behind an accidental guard" class `addr_class` exists +/// to end, and `forwarded_heap_owner` three modules over already reaches for +/// the safe reader. Classify the magnitude FIRST, then dereference. #[inline] pub(super) fn malloc_gc_parent_addr(parent_addr: usize) -> bool { - if parent_addr < GC_HEADER_SIZE + 0x1000 { + if !barrier_parent_addr_is_dereferenceable(parent_addr) { return false; } unsafe { @@ -1215,7 +1330,7 @@ pub(crate) fn runtime_write_barrier_slot(parent_addr: usize, slot_addr: usize, c incremental_mark_barrier_value(child_bits); return; } - js_write_barrier_slot(parent_addr as u64, slot_addr as u64, child_bits); + write_barrier_slot_decoded(parent_addr, slot_addr, child_bits, false); } /// Canonicalize an **INT32-boxed** numeric store into a raw-f64-masked slot of @@ -1299,12 +1414,7 @@ pub(crate) fn runtime_write_barrier_external_slot( incremental_mark_barrier_value(child_bits); return; } - write_barrier_slot_inner( - POINTER_TAG | (parent_addr as u64), - slot_addr, - child_bits, - true, - ); + write_barrier_slot_decoded(parent_addr, slot_addr, child_bits, true); } pub(crate) fn runtime_write_barrier_gc_slot(parent_addr: usize, slot_addr: usize, child_bits: u64) { @@ -1316,12 +1426,7 @@ pub(crate) fn runtime_write_barrier_gc_slot(parent_addr: usize, slot_addr: usize crate::arena::classify_heap_generation(parent_addr), crate::arena::HeapGeneration::Unknown ) && malloc_gc_parent_addr(parent_addr); - write_barrier_slot_inner( - POINTER_TAG | (parent_addr as u64 & POINTER_MASK), - slot_addr, - child_bits, - parent_is_malloc_gc, - ); + write_barrier_slot_decoded(parent_addr, slot_addr, child_bits, parent_is_malloc_gc); } #[inline] diff --git a/crates/perry-runtime/src/gc/tests/barrier_decoded_parent.rs b/crates/perry-runtime/src/gc/tests/barrier_decoded_parent.rs new file mode 100644 index 0000000000..d67635fed4 --- /dev/null +++ b/crates/perry-runtime/src/gc/tests/barrier_decoded_parent.rs @@ -0,0 +1,253 @@ +//! #7187: the Rust-side write-barrier entry points hand +//! `write_barrier_slot_inner` a parent address they already hold. They used to +//! re-encode it as a bare `u64` so `decode_heap_addr` could re-derive it — and +//! that arm pays a full `classify_heap_generation` immediately before +//! `barrier_parent_needs_remembering` classifies the same address again. +//! +//! These tests pin the two things removing the round-trip depends on: that the +//! decoded route leaves the collector exactly the remembered set the NaN-boxed +//! route did, and that the filtering the round-trip was silently providing is +//! now explicit — `malloc_gc_parent_addr` DEREFERENCES its argument. + +use super::super::barrier::{malloc_gc_parent_addr, remembered_dirty_page_count}; +use super::super::*; +use super::support::*; + +fn remembered_maintenance_entry_count() -> usize { + let dirty_old = DIRTY_OLD_PAGES.with(|s| s.borrow().len()); + let external_dirty = + EXTERNAL_DIRTY_SLOT_PAGES.with(|s| s.borrow().values().map(Vec::len).sum::()); + let fallback = REMEMBERED_SET.with(|s| s.borrow().len()); + dirty_old + external_dirty + fallback +} + +/// The remembered-set state a barrier call produced, as the collector will +/// later read it. Compared between the decoded and NaN-boxed entry points — +/// counter names may differ (that IS the observable delta, pinned separately +/// below), but what reaches the collector must not. +fn remembered_state_fingerprint() -> (usize, usize, usize) { + ( + remembered_set_size(), + remembered_dirty_page_count(), + remembered_maintenance_entry_count(), + ) +} + +#[test] +fn runtime_write_barrier_slot_remembers_old_to_young_edge() { + let _guard = GcTestIsolationGuard::new(); + reset_remembered_set(); + + let young = crate::arena::arena_alloc_gc(40, 8, GC_TYPE_OBJECT) as usize; + let (old_obj, fields) = unsafe { alloc_old_test_object(1) }; + let child_bits = ptr_bits(young); + unsafe { + *fields = child_bits; + } + let dirty_page = crate::arena::generation_page_for_addr(fields as usize); + assert!(!old_page_dirty_for(dirty_page)); + + // The `note_array_slot` / `runtime_store_jsvalue_slot` entry point — the + // one every born-old array element store goes through. + runtime_write_barrier_slot(old_obj as usize, fields as usize, child_bits); + + assert_eq!( + remembered_dirty_page_count(), + 1, + "old→young store through the decoded entry point must dirty the slot page" + ); + assert!( + old_page_dirty_for(dirty_page), + "old-page metadata should mirror the remembered dirty page" + ); + + reset_remembered_set(); +} + +#[test] +fn runtime_write_barrier_slot_matches_nanboxed_entry_point() { + let _guard = GcTestIsolationGuard::new(); + activate_malloc_registry_for_tests(); + + let young = crate::arena::arena_alloc_gc(40, 8, GC_TYPE_OBJECT) as usize; + let old_child = crate::arena::arena_alloc_gc_old(40, 8, GC_TYPE_OBJECT) as usize; + let malloc_child = gc_malloc( + std::mem::size_of::(), + GC_TYPE_CLOSURE, + ); + unsafe { + init_test_closure(malloc_child); + } + + // Every parent generation the barrier distinguishes, paired with every + // child kind. `malloc_parent` is the case the removed `decode_heap_addr` + // filter used to reject outright (classify == Unknown → address 0 → + // `NonPointerParentSkips`); it now reaches `barrier_parent_needs_remembering` + // and exits at `ParentNotOldSkips` instead. Same remembered-set effect. + let malloc_parent = gc_malloc( + std::mem::size_of::() + 8, + GC_TYPE_OBJECT, + ); + let children: [(&str, u64); 4] = [ + ("young", ptr_bits(young)), + ("old", ptr_bits(old_child)), + ("malloc", ptr_bits(malloc_child as usize)), + ("primitive", 1.5f64.to_bits()), + ]; + + for (child_label, child_bits) in children { + for parent_label in ["old", "nursery", "malloc"] { + let (parent_addr, slot_addr) = unsafe { + match parent_label { + "old" => { + let (obj, fields) = alloc_old_test_object(1); + (obj as usize, fields as usize) + } + "nursery" => { + let (obj, fields) = alloc_nursery_test_object(1); + (obj as usize, fields as usize) + } + _ => { + let fields = (malloc_parent as *mut u8) + .add(std::mem::size_of::()); + (malloc_parent as usize, fields as usize) + } + } + }; + unsafe { + std::ptr::write(slot_addr as *mut u64, child_bits); + } + + reset_remembered_set(); + js_write_barrier_slot(ptr_bits(parent_addr), slot_addr as u64, child_bits); + let nanboxed = remembered_state_fingerprint(); + + reset_remembered_set(); + runtime_write_barrier_slot(parent_addr, slot_addr, child_bits); + let decoded = remembered_state_fingerprint(); + + assert_eq!( + decoded, nanboxed, + "parent={parent_label} child={child_label}: the decoded entry point must \ + leave the collector exactly the remembered set the NaN-boxed one does" + ); + } + } + + reset_remembered_set(); + clear_marks(); +} + +#[test] +fn runtime_write_barrier_slot_malloc_parent_skips_as_not_old() { + let _guard = GcTestIsolationGuard::new(); + reset_remembered_set(); + activate_malloc_registry_for_tests(); + let tracing = gc_trace_enabled(); + let _ = take_write_barrier_trace_counters(); + + let young = crate::arena::arena_alloc_gc(40, 8, GC_TYPE_OBJECT) as usize; + let malloc_parent = gc_malloc( + std::mem::size_of::() + 8, + GC_TYPE_OBJECT, + ); + let slot = unsafe { + (malloc_parent as *mut u8).add(std::mem::size_of::()) + as *mut u64 + }; + let child_bits = ptr_bits(young); + unsafe { + std::ptr::write(slot, child_bits); + } + + runtime_write_barrier_slot(malloc_parent as usize, slot as usize, child_bits); + + // The invariant: a malloc-GC parent reached through the NON-external entry + // point is not an old parent, so nothing is remembered. Unchanged by #7187 + // — only which skip counter reports it moved. + assert_eq!( + remembered_state_fingerprint(), + (0, 0, 0), + "non-external malloc-GC parent must record no old→young edge" + ); + + let counters = take_write_barrier_trace_counters(); + if tracing { + assert_eq!(counters.calls, 1); + assert_eq!( + counters.non_pointer_parent_skips, 0, + "the parent address is real — it must no longer be rejected as a non-pointer" + ); + assert_eq!( + counters.parent_not_old_skips, 1, + "it is rejected for the reason that is actually true: not an old parent" + ); + } + + reset_remembered_set(); + clear_marks(); +} + +#[test] +fn runtime_barrier_entry_points_reject_implausible_parents_without_dereferencing() { + let _guard = GcTestIsolationGuard::new(); + reset_remembered_set(); + let tracing = gc_trace_enabled(); + let _ = take_write_barrier_trace_counters(); + + let young = crate::arena::arena_alloc_gc(40, 8, GC_TYPE_OBJECT) as usize; + let child_bits = ptr_bits(young); + let mut slot = child_bits; + let slot_addr = &mut slot as *mut u64 as usize; + + // `barrier_parent_needs_remembering` → `malloc_gc_parent_addr` + // DEREFERENCES `parent - GC_HEADER_SIZE`. Every one of these owner shapes + // reaches the runtime barrier entry points in the wild and none may be + // dereferenced: + // - the `closure/dynamic_props` side-table owner key (real: its own unit + // test parks props under `0xC10C_AB1E_0000_1803`), + // - native handle-band ids (`#4740`, `#6271`), + // - a heap-shaped but unaligned word. + let implausible: [(&str, usize); 6] = [ + ("closure prop owner key", 0xC10C_AB1E_0000_1803), + ("common handle band", 0x1234), + ("fetch handle band", 0x4_0010), + ("proxy id band", 0xF_0008), + ("above platform heap range", 0x9000_0000_0000), + ("unaligned heap-shaped word", 0x0000_7f31_0000_1003), + ]; + + for (label, parent) in implausible { + runtime_write_barrier_slot(parent, slot_addr, child_bits); + runtime_write_barrier_external_slot(parent, slot_addr, child_bits); + runtime_write_barrier_gc_slot(parent, slot_addr, child_bits); + assert_eq!( + remembered_state_fingerprint(), + (0, 0, 0), + "{label}: an implausible parent must record no edge" + ); + } + + let counters = take_write_barrier_trace_counters(); + if tracing { + assert_eq!(counters.calls, 18); + assert_eq!( + counters.non_pointer_parent_skips, 18, + "every implausible parent must be rejected before any header read" + ); + assert_eq!(counters.old_to_young_slow_hits, 0); + } + + // The predicate itself, pinned: `malloc_gc_parent_addr` must answer false + // without dereferencing. If the guard is removed this line segfaults + // rather than failing, which is the point of asserting it here as well as + // through the entry points above. + for (label, parent) in implausible { + assert!( + !malloc_gc_parent_addr(parent), + "{label}: must not be classified as a malloc-GC parent" + ); + } + + reset_remembered_set(); +} diff --git a/crates/perry-runtime/src/gc/tests/mod.rs b/crates/perry-runtime/src/gc/tests/mod.rs index 52026c1819..e0b038a1bc 100644 --- a/crates/perry-runtime/src/gc/tests/mod.rs +++ b/crates/perry-runtime/src/gc/tests/mod.rs @@ -1,5 +1,6 @@ mod alloc; mod barrier; +mod barrier_decoded_parent; mod budgeted_step_api; mod buffer_side_tables; mod contract;