From 9c8587ddac1d23fc884bace0f8dd24b06ebb42ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sun, 20 Sep 2026 15:31:15 +0200 Subject: [PATCH 1/2] perf(runtime): stop double-hashing Map's string content-hash side table MAP_STRING_INDEX's inner table (map_ptr -> content_hash -> Vec) hashed its `u64` key with std::collections::HashMap's default SipHash. That key is not raw input -- it is already the FNV-1a content hash computed one layer up, so every string-keyed Map.get/set/has/delete past SIDE_TABLE_THRESHOLD paid for a second, unrelated hash of an already-mixed value. Switched the inner table to PtrHasher (one multiply by the Fibonacci constant plus an xorshift avalanche), the same treatment the sibling NumericIndex.hashed table already gets for hashing a computed, non- adversarial u64. Not a HashDoS regression: two colliding u64 FNV-1a hashes land in the same Vec bucket under any hasher, so SipHash on the outer table could never have defended against a crafted FNV-1a collision -- the attack surface is FNV-1a itself, unchanged here. HashMap::get also re-checks K: Eq on every candidate regardless of S, so a hasher swap cannot change which entry a lookup resolves to, only how fast it gets there. New test `string_index_resolves_every_key_correctly_past_the_hashed_threshold` inserts 10,000 distinct string keys (forcing real bucket collisions at both the outer map-pointer and inner content-hash levels) and checks reverse-order reads, content-equal-but-freshly-allocated keys, adversarial shared-prefix misses, and delete-half-then-verify. Measured as a flat constant-factor win across an N-sweep from 16 to 4,096 map entries (well past the growth threshold): 115-121 instructions saved per lookup at every size tested, for interned hits, dynamically-built hits, and misses alike -- not a small-map-only effect. A first pass on one shape (dynamically-rebuilt keys) showed an apparent ~38-instruction-per- doubling growth with N; that was a probe artifact (the decimal key suffix grew a digit as N grew, correlating length with N) and vanished under a length-controlled follow-up. Not a fix for #10697 (string-keyed Map slow on small maps): that turned out to be a codegen type-proof gap in the generic dispatch path (find_key_index_cold / jsvalue_eq / string_view_from_bits re-deriving a key already proven to be a string), tracked and owned separately. Gap test: test_gap_map_get_string_key_perf.ts, byte-identical to node 26.5.1 -- SameValueZero (NaN, +0/-0), insertion-order iteration, has vs get for a stored undefined, delete-then-reinsert ordering, non-ASCII and lone-surrogate keys, content-equal-but-distinct-identity keys, a map past the growth threshold. --- changelog.d/map-get-string-index-ptrhasher.md | 52 +++++++ crates/perry-runtime/src/map.rs | 98 ++++++++++++- .../test_gap_map_get_string_key_perf.ts | 131 ++++++++++++++++++ 3 files changed, 277 insertions(+), 4 deletions(-) create mode 100644 changelog.d/map-get-string-index-ptrhasher.md create mode 100644 test-files/test_gap_map_get_string_key_perf.ts diff --git a/changelog.d/map-get-string-index-ptrhasher.md b/changelog.d/map-get-string-index-ptrhasher.md new file mode 100644 index 0000000000..c22d57be22 --- /dev/null +++ b/changelog.d/map-get-string-index-ptrhasher.md @@ -0,0 +1,52 @@ +`MAP_STRING_INDEX` (the content-hashed side table behind string-keyed +`Map.get`/`set`/`has`/`delete` once a map grows past `SIDE_TABLE_THRESHOLD`) +hashed its inner `u64 -> Vec` table with `std::collections::HashMap`'s +default SipHash. The `u64` key there is not raw input — it is already the +FNV-1a content hash computed above it, so SipHash was paying for a second, +unrelated hash of an already-well-mixed value on every string-keyed lookup. +Switched the inner table to `PtrHasher` (a single multiply-by-Fibonacci- +constant plus an xorshift avalanche), the same treatment the sibling +`NumericIndex.hashed` table already gets for the same reason (a computed, +non-adversarial `u64` key). + +This is not a HashDoS regression: two colliding `u64` FNV-1a hashes land in +the same `Vec` bucket under *any* hasher — SipHash on the outer table +could never have defended against a crafted FNV-1a collision, because the +attack surface is FNV-1a itself, which is unchanged. `HashMap::get` also re-checks `K: Eq` on every candidate regardless of `S`, so a +hasher swap cannot change which entry a lookup resolves to, only how fast it +gets there — a property pinned by a new test that inserts 10,000 distinct +string keys (forcing real bucket collisions at both the outer map-pointer +and inner content-hash levels) and asserts every key still resolves to its +own value, including reverse-order reads, content-equal-but-freshly- +allocated keys, adversarial shared-prefix misses, and delete-half-then- +verify. + +Measured as a flat constant-factor win across an N-sweep from 16 to 4,096 +map entries (well past the growth threshold): 115-121 instructions saved +per lookup at every size tested, for both hits (interned and dynamically +built keys) and misses — not a small-map-only effect. (Absolute figures +were taken under `PERRY_NO_AUTO_OPTIMIZE=1`, whose own overhead varies +wildly by workload -- ~0.3% on string concat, 8x on `String.prototype. +split` elsewhere -- so treat the absolutes as flag-qualified; the 115-121 +delta is unaffected, since both arms of every comparison shared the flag +and the same runtime archive otherwise.) An initial reading of one shape +(dynamically-rebuilt keys) showed an apparent ~38-instruction-per-doubling +growth with N; that turned out to be a probe artifact (the decimal key +suffix `"key" + (i % size)` grows a digit as `size` grows, so key length +was correlating with N) and vanished under a length-controlled follow-up. + +This change is **not** a fix for the separately-reported (#10697) +string-Map slowdown on small (below-`SIDE_TABLE_THRESHOLD`) maps: that +session profiled the generic dispatch path directly and found +`find_key_index_cold` running on every lookup of a four-entry map, with +`jsvalue_eq` doing ~149 instructions of generic equality on a key already +known at compile time to be a string, plus `string_view_from_bits` +re-deriving what codegen had already proven -- ~361 instructions against +node's ~50. A related but narrower observation surfaced while chasing that +repro (`m.get(arr[i])` dispatches to the specialized string-keyed path, +but binding the same expression through a local first, `let key = arr[i]; +m.get(key)`, does not) -- confirmed to require both an inline index +expression *and* a ``-annotated map, not the binding shape +alone. Neither is a hashing cost; both are codegen type-proof gaps, tracked +and owned separately from this change. diff --git a/crates/perry-runtime/src/map.rs b/crates/perry-runtime/src/map.rs index 7236eebea4..d4b76b895f 100644 --- a/crates/perry-runtime/src/map.rs +++ b/crates/perry-runtime/src/map.rs @@ -821,9 +821,23 @@ fn is_safe_numeric_key(bits: u64) -> bool { // Pre-fix `Map.set("key_" + i, …)` over 500k inserts was O(N²) because // each `set` did a linear `find_key_index` to dedup-check; with this // table the dedup probe is O(1) amortized. +// +// The inner map is keyed by `u64`, but that key is not raw input — it is +// already the FNV-1a content hash above, a well-avalanched 64-bit value. +// `std::collections::HashMap`'s default `RandomState` (SipHash) is built to +// resist adversarial *byte* input; hashing an already-mixed hash through it +// a second time buys nothing here and was costing every `Map.get`/`set`/ +// `has`/`delete` on a string-keyed map past `SIDE_TABLE_THRESHOLD` a second, +// unrelated hash computation. `NumericIndex.hashed` next door already uses +// `PtrHasher` for exactly this reason (u64-keyed, no adversarial input); this +// table gets the same treatment. `PtrHasher::write_u64` is one multiply plus +// an xorshift avalanche step — see `fast_hash.rs`'s `mix` doc comment for why +// the avalanche still matters even though FNV-1a is already well-distributed +// (HashMap reads bucket indices from the LOW bits, which a pure multiply +// under-mixes for some input distributions). crate::perry_thread_local! { static MAP_STRING_INDEX: RefCell< - crate::fast_hash::PtrHashMap>>, + crate::fast_hash::PtrHashMap>>, > = RefCell::new(crate::fast_hash::new_ptr_hash_map()); } @@ -1597,7 +1611,7 @@ pub extern "C" fn js_map_alloc(capacity: u32) -> *mut MapHeader { // and reached directly through the header above. MAP_STRING_INDEX.with(|idx| { idx.borrow_mut() - .insert(ptr as usize, std::collections::HashMap::new()); + .insert(ptr as usize, crate::fast_hash::new_ptr_hash_map()); }); MAP_PTR_INDEX.with(|idx| { idx.borrow_mut() @@ -2131,7 +2145,7 @@ unsafe fn map_set_string_key_value( let mut idx = idx.borrow_mut(); let slot = idx .entry(map as usize) - .or_insert_with(std::collections::HashMap::new); + .or_insert_with(crate::fast_hash::new_ptr_hash_map); slot.entry(h).or_insert_with(Vec::new).push(used); }); } @@ -2253,7 +2267,7 @@ fn map_set_resolved(map: *mut MapHeader, key: f64, value: f64) { let mut idx = idx.borrow_mut(); let slot = idx .entry(map as usize) - .or_insert_with(std::collections::HashMap::new); + .or_insert_with(crate::fast_hash::new_ptr_hash_map); slot.entry(h).or_insert_with(Vec::new).push(used); }); } @@ -3688,6 +3702,82 @@ mod tests { } } + /// MAP_STRING_INDEX's inner table switched from `std::collections:: + /// HashMap` (SipHash) to `PtrHashMap` (a cheap multiplicative hasher) so + /// every string-keyed `Map.get`/`set`/`has`/`delete` past + /// `SIDE_TABLE_THRESHOLD` stops paying for a second, redundant hash of + /// an already-hashed FNV-1a value. `HashMap::get` re-checks + /// `K: Eq` on every candidate regardless of `S`, so a hasher swap cannot + /// change *which* key a lookup resolves to -- only how fast it gets + /// there -- but this pins that down empirically at a scale (10,000+ + /// distinct keys, forced far past `SIDE_TABLE_THRESHOLD` and any small + /// std-HashMap capacity) where bucket collisions in BOTH the outer + /// (map-pointer-keyed) and inner (content-hash-keyed) tables are a + /// certainty, not a contrived edge case. If a bucket collision at either + /// level silently returned the wrong entry, or if switching hashers + /// somehow let two live keys shadow each other, this test fails. + #[test] + fn string_index_resolves_every_key_correctly_past_the_hashed_threshold() { + let map = js_map_alloc(4); + const COUNT: usize = 10_000; + let mut keys: Vec<*const StringHeader> = Vec::with_capacity(COUNT); + for i in 0..COUNT { + let content = format!("string-index-key-{i}"); + let key = js_string_from_bytes(content.as_ptr(), content.len() as u32); + js_map_set_string_number(map, key, i as f64); + keys.push(key); + } + assert_eq!(js_map_size(map), COUNT as u32); + assert!(COUNT as u32 > SIDE_TABLE_THRESHOLD); + + // Every inserted key still resolves to its OWN distinct value, in + // reverse-insertion order (exercises the hashed side table, not + // append-order luck). + for i in (0..COUNT).rev() { + assert_eq!( + js_map_get_string_key(map, keys[i]), + i as f64, + "key {i} resolved to the wrong value -- a bucket collision \ + returned a neighbor's entry instead of missing or matching" + ); + assert_eq!(js_map_has_string_key(map, keys[i]), 1); + } + + // A content-equal-but-freshly-allocated key (distinct pointer from + // the one stored at insert time) must still resolve by content -- + // the outer hasher change must not have started keying by identity. + for i in [0usize, COUNT / 2, COUNT - 1] { + let content = format!("string-index-key-{i}"); + let fresh = js_string_from_bytes(content.as_ptr(), content.len() as u32); + assert_ne!(fresh as usize, keys[i] as usize); + assert_eq!(js_map_get_string_key(map, fresh), i as f64); + } + + // Absent keys that share a long common prefix with real entries + // (adversarial-ish for a byte-at-a-time hash) must still miss. + for i in 0..50 { + let content = format!("string-index-key-{i}-absent"); + let missing = js_string_from_bytes(content.as_ptr(), content.len() as u32); + assert_eq!(js_map_get_string_key(map, missing).to_bits(), TAG_UNDEFINED); + assert_eq!(js_map_has_string_key(map, missing), 0); + } + + // Delete half the keys, then confirm the survivors are still exact + // and the deleted ones are definitively gone (forces + // `compact_map_entries`'s side-table rebuild at this scale too). + for i in (0..COUNT).step_by(2) { + assert_eq!(js_map_delete_string_key(map, keys[i]), 1); + } + assert_eq!(js_map_size(map), (COUNT / 2) as u32); + for i in 0..COUNT { + if i % 2 == 0 { + assert_eq!(js_map_has_string_key(map, keys[i]), 0); + } else { + assert_eq!(js_map_get_string_key(map, keys[i]), i as f64); + } + } + } + #[test] fn clear_resets_every_index_whatever_the_key_kinds() { // Numeric-only map: cleared without touching the side-tables; the diff --git a/test-files/test_gap_map_get_string_key_perf.ts b/test-files/test_gap_map_get_string_key_perf.ts new file mode 100644 index 0000000000..540856feef --- /dev/null +++ b/test-files/test_gap_map_get_string_key_perf.ts @@ -0,0 +1,131 @@ +// Gap test: Map.get(string) correctness under the fast string-keyed lookup +// path (content-hash side table + SameValueZero identity rules). Covers the +// shapes exercised while reducing Map.get(str) instruction cost: identity +// semantics (NaN, +0/-0), insertion-order iteration, has-vs-get with a +// stored `undefined`, delete-then-reinsert order, non-ASCII / lone-surrogate +// string keys, content-equal-but-distinct-identity string keys, and a map +// past the side-table growth threshold. +// Run: node --experimental-strip-types test_gap_map_get_string_key_perf.ts + +// --- SameValueZero: NaN keys collapse to one slot --- +const nanMap = new Map(); +nanMap.set(NaN, "first"); +nanMap.set(NaN, "second"); +console.log("nan size:", nanMap.size); +console.log("nan get:", nanMap.get(NaN)); +console.log("nan has:", nanMap.has(NaN)); + +// --- SameValueZero: +0 and -0 are the same key --- +const zeroMap = new Map(); +zeroMap.set(0, "plus"); +zeroMap.set(-0, "minus"); +console.log("zero size:", zeroMap.size); +console.log("zero get +0:", zeroMap.get(0)); +console.log("zero get -0:", zeroMap.get(-0)); + +// --- has() vs get() for a stored `undefined` value --- +const undefMap = new Map(); +undefMap.set("present", undefined); +console.log("undef has present:", undefMap.has("present")); +console.log("undef get present:", undefMap.get("present")); +console.log("undef has missing:", undefMap.has("missing")); +console.log("undef get missing:", undefMap.get("missing")); + +// --- Insertion-order iteration survives get()-only probing --- +const orderMap = new Map(); +orderMap.set("z", 1); +orderMap.set("a", 2); +orderMap.set("m", 3); +orderMap.get("a"); +orderMap.get("z"); +const orderKeys: string[] = []; +for (const k of orderMap.keys()) orderKeys.push(k); +console.log("order keys:", orderKeys); + +// --- delete() then re-insert moves a key to the end --- +const reinsMap = new Map(); +reinsMap.set("x", 1); +reinsMap.set("y", 2); +reinsMap.set("z", 3); +reinsMap.delete("x"); +reinsMap.set("x", 10); +const reinsKeys: string[] = []; +for (const k of reinsMap.keys()) reinsKeys.push(k); +console.log("reins keys:", reinsKeys); +console.log("reins get x:", reinsMap.get("x")); + +// --- Non-ASCII string keys --- +const uniMap = new Map(); +uniMap.set("héllo", "accent"); +uniMap.set("日本語", "japanese"); +uniMap.set("😀emoji", "emoji"); +console.log("uni get héllo:", uniMap.get("héllo")); +console.log("uni get 日本語:", uniMap.get("日本語")); +console.log("uni get emoji:", uniMap.get("😀emoji")); +console.log("uni get missing:", uniMap.get("héllo2")); + +// --- Lone-surrogate string keys (WTF-8) --- +const loneHigh = String.fromCharCode(0xd800); +const loneLow = String.fromCharCode(0xdc00); +const surrMap = new Map(); +surrMap.set(loneHigh, "high"); +surrMap.set(loneLow, "low"); +console.log("surr get high:", surrMap.get(loneHigh)); +console.log("surr get low:", surrMap.get(loneLow)); +console.log("surr high !== low:", loneHigh !== loneLow); +console.log( + "surr get rebuilt high:", + surrMap.get(String.fromCharCode(0xd800)), +); + +// --- Content-equal but distinct-identity keys (dynamically built) --- +function makeKey(prefix: string, n: number): string { + return prefix + n; +} +const dynMap = new Map(); +for (let i = 0; i < 20; i++) { + dynMap.set(makeKey("item", i), i * 10); +} +// Re-build the same content via a different allocation than the stored key. +console.log("dyn get item0 (rebuilt):", dynMap.get(makeKey("item", 0))); +console.log("dyn get item19 (rebuilt):", dynMap.get(makeKey("item", 19))); +console.log("dyn get item9 (rebuilt):", dynMap.get(makeKey("item", 9))); +console.log("dyn get missing:", dynMap.get(makeKey("item", 99))); + +// --- Map past the side-table growth threshold (small linear scan vs +// hashed side table) --- +const bigMap = new Map(); +for (let i = 0; i < 64; i++) { + bigMap.set("k" + i, i); +} +console.log("big size:", bigMap.size); +console.log("big get k0:", bigMap.get("k0")); +console.log("big get k63:", bigMap.get("k63")); +console.log("big get k32 (rebuilt):", bigMap.get(makeKey("k", 32))); +console.log("big get missing:", bigMap.get("nomatch")); +bigMap.delete("k10"); +bigMap.delete("k20"); +bigMap.set("k10", 1010); +console.log("big get k10 after delete+reinsert:", bigMap.get("k10")); +console.log("big has k20 after delete:", bigMap.has("k20")); +console.log("big size after churn:", bigMap.size); + +// --- Long (> 64 byte) string keys --- +const longMap = new Map(); +const longPrefix = "q".repeat(70); +for (let i = 0; i < 10; i++) { + longMap.set(longPrefix + i, i); +} +console.log("long get 0 (rebuilt):", longMap.get(longPrefix + 0)); +console.log("long get 9 (rebuilt):", longMap.get(longPrefix + 9)); +console.log("long get missing:", longMap.get(longPrefix + "zz")); + +// --- Interned literal keys get a pointer-equality shortcut but must +// still match content-equal keys built at runtime --- +const litMap = new Map(); +litMap.set("literal-key", 1); +console.log("lit get literal:", litMap.get("literal-key")); +console.log( + "lit get rebuilt:", + litMap.get(["li", "teral-key"].join("")), +); From a427b31796913f1a07a66d11c24066fdea643606 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sun, 20 Sep 2026 15:32:35 +0200 Subject: [PATCH 2/2] changelog: key fragment to #10813 --- ...index-ptrhasher.md => 10813-map-get-string-index-ptrhasher.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changelog.d/{map-get-string-index-ptrhasher.md => 10813-map-get-string-index-ptrhasher.md} (100%) diff --git a/changelog.d/map-get-string-index-ptrhasher.md b/changelog.d/10813-map-get-string-index-ptrhasher.md similarity index 100% rename from changelog.d/map-get-string-index-ptrhasher.md rename to changelog.d/10813-map-get-string-index-ptrhasher.md