diff --git a/changelog.d/8882-shape-facts-hasher.md b/changelog.d/8882-shape-facts-hasher.md new file mode 100644 index 0000000000..50629d57ac --- /dev/null +++ b/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". diff --git a/crates/perry-runtime/src/object/shapes.rs b/crates/perry-runtime/src/object/shapes.rs index d348857534..09c6a8be28 100644 --- a/crates/perry-runtime/src/object/shapes.rs +++ b/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>, + /// + /// 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>, /// 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(), }), } diff --git a/crates/perry-runtime/src/object/shapes_tests.rs b/crates/perry-runtime/src/object/shapes_tests.rs index 30ea3ea124..8bf3279bb4 100644 --- a/crates/perry-runtime/src/object/shapes_tests.rs +++ b/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"); +}