perf(shapes): stop paying SipHash on every ShapeId probe, and pin the id a shape resolves to

changelog.d/8157-shape-descriptor-siphash.md

@@ -0,0 +1,59 @@
+### Performance
+
+**The ShapeId descriptor table no longer pays SipHash on every probe, and the
+id a shape resolves to no longer depends on hash iteration order.**
+
+`ShapeTableInner::descriptors` — the map `shape_descriptor_by_id` reads — was a
+`std::collections::HashMap<u32, _>` with the default `RandomState`, i.e.
+SipHash-1-3 on a bare `u32`. It is the hottest lookup in the object model:
+`object_is_regular` runs it once per array element-shape test (3,000,000 times
+on the `retain` bench, 20,000,002 on `churn`) and `object_live_slot_count` runs
+it on every indexed field get/set. The sibling field on the same struct,
+`indices`, already used `crate::fast_hash::PtrHashMap`; `descriptors` and
+`ids_by_keys` were simply never converted, and `fast_hash`'s own module doc
+already records the identical finding ("`hash_one` was 14% leaf samples") for
+the pointer-keyed registries.
+
+`PtrHasherImpl` gains a `write_u32` fast path — without it a `u32` key falls
+into the generic byte-stream fallback, because `Hasher`'s default `write_u32`
+forwards to `write(&n.to_ne_bytes())`: four rotate/xor rounds for a key that
+needs one multiply. `ids_by_facts` is deliberately NOT converted, since
+`PtrHasher`'s `write_*` methods overwrite the accumulator rather than folding
+it — right for a one-word key, wrong for that five-field one.
+
+`rebuild_descriptor_reverse_indices` now sorts each id vector. The rebuild
+walks `descriptors` in HASH order and `shape_descriptor_ensure_with_generation`
+reuses `ids.first()`, so which ShapeId a facts key resolved to after a GC
+rewrite depended on the hasher: two objects with identical facts, one born
+before a collection and one after, carried different ids, and every id-keyed
+consumer (the typed shape-layout install, the emitted PICs) split its
+population. Latent before, exposed by the hasher swap — which alone measured
+`interp` +3.4% with `gc::layout::init_typed_shape_layout` newly doubled in the
+profile, and `interp` −5.9% once the order was pinned.
+
+Measured on the 19-program corpus, `/usr/bin/time -l`, best-of-3, per-arm
+`PERRY_RUNTIME_DIR` and object cache, archives `cmp`-verified to differ, all 19
+stdouts byte-compared and exit-checked:
+
+```
+churn -25.2%   deeplist -17.2%   retain_wide -15.2%   retain1 -15.1%
+cycles -14.9%  retain -14.7%     retain_wide1 -14.3%  tree -13.4%
+shapes  -9.4%  tree_wide  -6.5%  interp        -5.9%  iso_miss -4.7%
+pipeline -3.6% asyncpipe  -1.1%  fib40 / push_num / churn_read ~0
+```
+
+Peak memory footprint is unchanged on every row.
+
+Two discriminating tests: `u32_keys_take_the_multiplicative_fast_path` goes red
+if `write_u32` is deleted (the default forwards to the byte fold and produces a
+different digest), and `sequential_shape_ids_spread_across_low_bit_buckets`
+goes red if `mix`'s avalanche is dropped, since ShapeIds are a dense run in the
+top half of `u32` while `HashMap` indexes on the low bits.
+
+The symbol profile that found this was previously recorded as structurally
+unobtainable. It needs no debug build — only `CARGO_PROFILE_RELEASE_STRIP=none`
+plus `PERRY_DEBUG_SYMBOLS=1`, which skips perry's own post-link `strip`. The
+"zero `_ZN` frames in `nm`" observation was that `strip`, not LTO
+internalization.
+
+Refs #8125, #8113, #8122.

crates/perry-runtime/src/fast_hash.rs

@@ -54,6 +54,19 @@ impl Hasher for PtrHasherImpl {
         }
         self.0 = mix(h.wrapping_mul(PTR_MIX));
     }
+    /// #8125: `u32` keys must not fall into the byte-stream fallback above.
+    /// `Hash for u32` calls `write_u32`, and `Hasher`'s DEFAULT `write_u32`
+    /// forwards to `write(&n.to_ne_bytes())` — four rotate/xor iterations plus
+    /// the multiply, for a key that needs exactly one multiply. The shape
+    /// descriptor table (`object::shapes`) is keyed by a bare `u32` ShapeId and
+    /// is probed once per allocated object and once per array element-shape
+    /// test, so this override is the difference between a fast path and a
+    /// loop. Deliberately identical to `write_u64` on the same numeric value;
+    /// `u32_keys_take_the_multiplicative_fast_path` pins that.
+    #[inline]
+    fn write_u32(&mut self, n: u32) {
+        self.0 = mix((n as u64).wrapping_mul(PTR_MIX));
+    }
     #[inline]
     fn write_u64(&mut self, n: u64) {
         self.0 = mix(n.wrapping_mul(PTR_MIX));
@@ -183,6 +196,49 @@ mod tests {
         assert_eq!(m.get(&0x9999), None);
     }
 
+    /// #8125: a `u32` key must reach the single-multiply path, not the
+    /// per-byte fold. `Hash for u32` calls `write_u32`; without an override
+    /// `Hasher`'s default `write_u32` forwards to `write(&n.to_ne_bytes())`.
+    /// Deleting `PtrHasherImpl::write_u32` turns this red.
+    #[test]
+    fn u32_keys_take_the_multiplicative_fast_path() {
+        use std::hash::Hash;
+        for n in [0u32, 1, 0x8000_0000, 0x8000_02ff, u32::MAX] {
+            let mut via_hash = PtrHasher.build_hasher();
+            n.hash(&mut via_hash);
+            let mut via_u64 = PtrHasher.build_hasher();
+            via_u64.write_u64(n as u64);
+            assert_eq!(
+                via_hash.finish(),
+                via_u64.finish(),
+                "u32 key {n:#x} fell into the byte-stream fallback"
+            );
+        }
+    }
+
+    /// ShapeIds are minted sequentially from `SHAPE_ID_BASE` (0x8000_0000), so
+    /// the descriptor table's keys are a dense run in the TOP half of the u32
+    /// range. Multiplicative mixing has to spread that run across buckets —
+    /// `HashMap` indexes on the LOW bits, and a run of consecutive integers has
+    /// no entropy there. Dropping `mix` (the `^= h >> 32` avalanche) collapses
+    /// this to a handful of buckets and turns the map into a linked list.
+    #[test]
+    fn sequential_shape_ids_spread_across_low_bit_buckets() {
+        use std::collections::HashSet;
+        let base = 0x8000_0000u32;
+        let mut buckets = HashSet::new();
+        for i in 0..1024u32 {
+            let mut h = PtrHasher.build_hasher();
+            h.write_u32(base + i);
+            buckets.insert(h.finish() & 0x3ff);
+        }
+        assert!(
+            buckets.len() > 512,
+            "sequential ShapeIds hashed into only {} of 1024 buckets",
+            buckets.len()
+        );
+    }
+
     /// Pointer-aligned keys collide trivially with multiply-only on the
     /// low bits — Fibonacci-hash mixing into the upper bits is what
     /// keeps the buckets evenly populated. Sanity-check that 1000 8-byte-

crates/perry-runtime/src/object/shapes.rs

@@ -72,14 +72,36 @@ struct ShapeFacts {
 
 struct ShapeTableInner {
     indices: crate::fast_hash::PtrHashMap<usize, ShapeIndex>,
-    descriptors: HashMap<u32, ShapeDescriptor>,
+    /// #8125: `PtrHashMap`, not the SipHash default.
+    ///
+    /// This is the map `shape_descriptor_by_id` probes, and that probe is the
+    /// single hottest runtime lookup in the object model: `object_is_regular`
+    /// runs it once per array element-shape test (3 M times on the `retain`
+    /// bench, 20 M on `churn`) and, since #8113 deleted
+    /// `ObjectHeader::field_count`, `object_live_slot_count` runs it on every
+    /// indexed field get/set. A symbol profile of the `shapes` bench
+    /// (`PERRY_DEBUG_SYMBOLS=1` + `sample`) put `RandomState::hash_one` at the
+    /// TOP of self time with `shape_descriptor_by_id` fourth — together ~22% of
+    /// the program, nearly all of it SipHash on a bare `u32`.
+    ///
+    /// The key is a ShapeId minted by this process from a monotonic counter.
+    /// No external input reaches it, so hash-flooding resistance buys nothing
+    /// here for the same reason it buys nothing on the pointer-keyed
+    /// registries `fast_hash` already serves.
+    descriptors: crate::fast_hash::PtrHashMap<u32, ShapeDescriptor>,
     /// Exact-facts reverse index. More than one id is legal when a worker
     /// minted a local descriptor before a process-global module id arrived.
+    ///
+    /// Deliberately NOT a `PtrHashMap`: `PtrHasher`'s `write_*` methods
+    /// OVERWRITE the accumulator instead of folding it, which is exactly right
+    /// for a single-word key and wrong for this five-field one — every
+    /// `ShapeFacts` would hash to its last field alone.
     ids_by_facts: HashMap<ShapeFacts, Vec<u32>>,
     /// Keys-array address -> every descriptor id that currently names it.
     /// Same-address key-count retirement uses this index instead of scanning
-    /// every shape ever observed by the agent.
-    ids_by_keys: HashMap<u64, Vec<u32>>,
+    /// every shape ever observed by the agent. Single-word key, so `PtrHasher`
+    /// (#8125).
+    ids_by_keys: crate::fast_hash::PtrHashMap<u64, Vec<u32>>,
 }
 
 pub(crate) struct ShapeTable {
@@ -91,9 +113,9 @@ impl ShapeTable {
         ShapeTable {
             inner: RefCell::new(ShapeTableInner {
                 indices: crate::fast_hash::new_ptr_hash_map(),
-                descriptors: HashMap::new(),
+                descriptors: crate::fast_hash::new_ptr_hash_map(),
                 ids_by_facts: HashMap::new(),
-                ids_by_keys: HashMap::new(),
+                ids_by_keys: crate::fast_hash::new_ptr_hash_map(),
             }),
         }
     }
@@ -137,14 +159,31 @@ fn remove_id_from_facts_index(inner: &mut ShapeTableInner, facts: ShapeFacts, id
 fn rebuild_descriptor_reverse_indices(inner: &mut ShapeTableInner) {
     let mut ids_by_facts: HashMap<ShapeFacts, Vec<u32>> =
         HashMap::with_capacity(inner.descriptors.len());
-    let mut ids_by_keys: HashMap<u64, Vec<u32>> = HashMap::new();
+    let mut ids_by_keys: crate::fast_hash::PtrHashMap<u64, Vec<u32>> =
+        crate::fast_hash::new_ptr_hash_map();
     for (&id, &descriptor) in &inner.descriptors {
         ids_by_facts
             .entry(descriptor_facts(descriptor))
             .or_default()
             .push(id);
         ids_by_keys.entry(descriptor.keys).or_default().push(id);
     }
+    // #8125: the rebuild walks `descriptors` in HASH order, and
+    // `shape_descriptor_ensure_with_generation` reuses `ids.first()` — so
+    // without this sort, WHICH id a facts key resolves to after a GC rewrite
+    // depends on the hasher. Two objects with identical facts, one born before
+    // a collection and one after, would then carry different ShapeIds, and
+    // every id-keyed consumer (the typed shape-layout install, the emitted
+    // PICs) splits its population. Ascending is the stable canonical choice:
+    // ids are minted monotonically, so the smallest is the oldest — the one
+    // already-published objects and already-installed layouts carry, and in
+    // practice the module-init id `install_external_shape_id` prefers.
+    for ids in ids_by_facts.values_mut() {
+        ids.sort_unstable();
+    }
+    for ids in ids_by_keys.values_mut() {
+        ids.sort_unstable();
+    }
     inner.ids_by_facts = ids_by_facts;
     inner.ids_by_keys = ids_by_keys;
 }