From f2e2c8d609a00fb6921281d76f2b7b6a5b9acc75 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Thu, 27 Aug 2026 16:25:39 +0200 Subject: [PATCH 1/2] perf(shapes): drop SipHash from ids_by_facts, the last one on the shape path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ids_by_facts was the only shape-table map still on std's RandomState. 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 and retire. Its sibling maps already moved off SipHash (#8125). The standing comment argued only against PtrHasher, whose write_* methods OVERWRITE the accumulator — correct for a single-word key, and wrong for this five-field one, which would collapse to its last field. 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. Test pins the folding property by varying one field at a time and requiring a distinct hash. Sabotage-checked against PtrHasher: it fails with 'changing keys alone must change the hash'. Suite 2717 passed. --- crates/perry-runtime/src/object/shapes.rs | 20 ++++- .../perry-runtime/src/object/shapes_tests.rs | 84 +++++++++++++++++++ 2 files changed, 102 insertions(+), 2 deletions(-) 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"); +} From 2c06e3f60cb2d5c5422b4f734377a6fc9cd36138 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Thu, 27 Aug 2026 16:37:17 +0200 Subject: [PATCH 2/2] changelog: add fragment for the ids_by_facts hasher change --- changelog.d/8882-shape-facts-hasher.md | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 changelog.d/8882-shape-facts-hasher.md 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".