diff --git a/changelog.d/8157-shape-descriptor-siphash.md b/changelog.d/8157-shape-descriptor-siphash.md new file mode 100644 index 0000000000..bc0db96430 --- /dev/null +++ b/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` 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. diff --git a/crates/perry-runtime/src/fast_hash.rs b/crates/perry-runtime/src/fast_hash.rs index 90aaa2ce7d..9962c062ed 100644 --- a/crates/perry-runtime/src/fast_hash.rs +++ b/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- diff --git a/crates/perry-runtime/src/object/shapes.rs b/crates/perry-runtime/src/object/shapes.rs index 430e747032..a57d1d543a 100644 --- a/crates/perry-runtime/src/object/shapes.rs +++ b/crates/perry-runtime/src/object/shapes.rs @@ -72,14 +72,36 @@ struct ShapeFacts { struct ShapeTableInner { indices: crate::fast_hash::PtrHashMap, - descriptors: HashMap, + /// #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, /// 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>, /// 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>, + /// every shape ever observed by the agent. Single-word key, so `PtrHasher` + /// (#8125). + ids_by_keys: crate::fast_hash::PtrHashMap>, } 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,7 +159,8 @@ 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> = HashMap::with_capacity(inner.descriptors.len()); - let mut ids_by_keys: HashMap> = HashMap::new(); + let mut ids_by_keys: crate::fast_hash::PtrHashMap> = + crate::fast_hash::new_ptr_hash_map(); for (&id, &descriptor) in &inner.descriptors { ids_by_facts .entry(descriptor_facts(descriptor)) @@ -145,6 +168,22 @@ fn rebuild_descriptor_reverse_indices(inner: &mut ShapeTableInner) { .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; }