perf(runtime): stop double-hashing Map's string content-hash side table

changelog.d/10813-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<u32>` 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<u32>` 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<K, V,
+S>::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 `<string, number>`-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.

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<usize, std::collections::HashMap<u64, Vec<u32>>>,
+        crate::fast_hash::PtrHashMap<usize, crate::fast_hash::PtrHashMap<u64, Vec<u32>>>,
     > = 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<K, V, S>::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

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<number, string>();
+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<number, string>();
+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<string, number | undefined>();
+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<string, number>();
+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<string, number>();
+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<string, string>();
+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<string, string>();
+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<string, number>();
+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<string, number>();
+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<string, number>();
+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<string, number>();
+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("")),
+);