perf(gc): stop the runtime write barrier re-deriving its parent address (#7187 increment 1)

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.

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;

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<usize, PageGenerationSlot, BuildHasherDefault<IdentityHasher>>;
+/// #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<usize, PageGenerationSlot>;
 type OldGenPageObjectMap = crate::fast_hash::PtrHashMap<usize, Vec<usize>>;
 type OldGenPageMetaMap = crate::fast_hash::PtrHashMap<usize, OldPageMeta>;
 
@@ -248,7 +240,7 @@ pub(crate) struct OldArenaSourceBlockSelection {
 
 thread_local! {
     static PAGE_GENERATIONS: RefCell<PageGenerationMap> =
-        RefCell::new(HashMap::with_hasher(BuildHasherDefault::<IdentityHasher>::default()));
+        RefCell::new(crate::fast_hash::new_ptr_hash_map());
 
     static PAGE_GENERATION_CACHE: Cell<PageGenerationCache> =
         const { Cell::new(PageGenerationCache::empty()) };
@@ -919,3 +911,104 @@ pub(crate) fn old_page_meta_for_tests(page: usize) -> Option<OldPageMeta> {
             .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<u64> = (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<u64> = (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);
+    }
+}