perf(shapes): drop SipHash from ids_by_facts, the last one on the shape path

changelog.d/8882-shape-facts-hasher.md

@@ -0,0 +1,20 @@
+Dropped SipHash from `ids_by_facts`, the last shape-table map still using it.
+
+Profiling `claude -p` put `RandomState::hash_one` at 17 self-samples inside
+`shapes::` alone (57 across the process) — pure hashing overhead on a lookup
+that runs on every descriptor install and retire.
+
+Its sibling maps already moved off SipHash (#8125). The standing comment on
+this one argued only against `PtrHasher`, whose `write_*` methods OVERWRITE the
+accumulator — right for a single-word key, wrong for this five-field one, which
+would collapse to its last field and collide every descriptor sharing it. That
+objection does not apply to `FastKeyHasher`: it implements only `write`, so the
+derived `Hash`'s `write_u32` / `write_u64` calls all forward there and FOLD with
+FNV-1a, reaching every field.
+
+The key is internal shape state, never program input, so DoS-resistant hashing
+buys nothing — the same rationale already applied to the descriptor side tables.
+
+The new test pins the folding property directly: vary one field at a time and
+require a distinct hash each time. Sabotage-checked against `PtrHasher`, where
+it fails with "changing `keys` alone must change the hash".

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

@@ -167,7 +167,23 @@ struct ShapeTableInner {
     /// 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>>,
+    ///
+    /// It is a `FastKeyHashMap` rather than the SipHash default, though: that
+    /// objection is to `PtrHasher` specifically, and leaving std's
+    /// `RandomState` here made this the only SipHash map left on the shape
+    /// path. Profiling `claude -p` showed `RandomState::hash_one` at 17
+    /// self-samples inside `shapes::` alone (57 across the process) — pure
+    /// hashing overhead on a lookup that runs on every descriptor
+    /// install/retire.
+    ///
+    /// `FastKeyHasher` is the right third option: it implements only `write`,
+    /// so every `write_u32` / `write_u64` from the derived `Hash` forwards
+    /// there and FOLDS with FNV-1a. All five fields reach the accumulator,
+    /// which is exactly the property `PtrHasher` lacks. The key is built from
+    /// internal shape state (never program input), so DoS-resistant hashing
+    /// buys nothing here — the same rationale already applied to the
+    /// descriptor side tables and to `indices` (#8125).
+    ids_by_facts: crate::fast_hash::FastKeyHashMap<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. Single-word key, so `PtrHasher`
@@ -185,7 +201,7 @@ impl ShapeTable {
             inner: RefCell::new(ShapeTableInner {
                 indices: crate::fast_hash::new_ptr_hash_map(),
                 descriptors: crate::fast_hash::new_ptr_hash_map(),
-                ids_by_facts: HashMap::new(),
+                ids_by_facts: crate::fast_hash::new_fast_key_hash_map(),
                 ids_by_keys: crate::fast_hash::new_ptr_hash_map(),
             }),
         }

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

@@ -776,3 +776,87 @@ mod descriptor_tests_8067 {
         }
     }
 }
+
+/// `ids_by_facts` moved from std's SipHash `RandomState` to `FastKeyHasher`.
+///
+/// The hazard that motivated the original "deliberately NOT a `PtrHashMap`"
+/// note is real: `PtrHasher`'s `write_*` methods OVERWRITE the accumulator, so
+/// a five-field `ShapeFacts` would collapse to its last field and every
+/// descriptor sharing that field would collide into one bucket.
+///
+/// `FastKeyHasher` avoids this by implementing only `write` — the derived
+/// `Hash`'s `write_u32`/`write_u64` calls all forward there and FOLD with
+/// FNV-1a. This test pins that property directly: vary ONE field at a time and
+/// require a distinct hash each time. It fails loudly against any hasher that
+/// overwrites instead of folding.
+#[test]
+fn shape_facts_hash_folds_every_field() {
+    use crate::fast_hash::FastKeyHasher;
+    use std::hash::{BuildHasher, Hash, Hasher};
+
+    fn h(f: &ShapeFacts) -> u64 {
+        let mut hasher = FastKeyHasher.build_hasher();
+        f.hash(&mut hasher);
+        hasher.finish()
+    }
+
+    let base = ShapeFacts {
+        keys: 0x1111_2222_3333_4444,
+        logical_key_count: 7,
+        live_inline_slot_count: 3,
+        semantic_generation: 9,
+        object_kind: ShapeObjectKind::Ordinary,
+    };
+
+    let variants = [
+        (
+            "keys",
+            ShapeFacts {
+                keys: 0x5555_6666_7777_8888,
+                ..base
+            },
+        ),
+        (
+            "logical_key_count",
+            ShapeFacts {
+                logical_key_count: 8,
+                ..base
+            },
+        ),
+        (
+            "live_inline_slot_count",
+            ShapeFacts {
+                live_inline_slot_count: 4,
+                ..base
+            },
+        ),
+        (
+            "semantic_generation",
+            ShapeFacts {
+                semantic_generation: 10,
+                ..base
+            },
+        ),
+        (
+            "object_kind",
+            ShapeFacts {
+                object_kind: ShapeObjectKind::Class,
+                ..base
+            },
+        ),
+    ];
+
+    let base_hash = h(&base);
+    for (field, v) in &variants {
+        assert_ne!(
+            h(v),
+            base_hash,
+            "changing `{field}` alone must change the hash — a hasher that \
+             overwrites instead of folding would collapse ShapeFacts to its \
+             last field and collide every descriptor that shares it"
+        );
+    }
+
+    // Same facts must still hash the same, or lookups would miss.
+    assert_eq!(h(&base), h(&base.clone()), "hashing must be deterministic");
+}