fix(runtime): stop the concat memo probing when it isn't paying (fixes #9391)

crates/perry-runtime/src/string/concat.rs

@@ -262,21 +262,165 @@ pub extern "C" fn js_string_concat_box(l_value: f64, r_value: f64) -> f64 {
 const CONCAT_MEMO_SIZE: usize = 512;
 const CONCAT_MEMO_MAX_BYTES: u32 = 12;
 
+// Candidates per governor window.
+const MEMO_WINDOW: u32 = 4096;
+// Earn the probe: at least a quarter of a window's candidates must hit.
+const MEMO_MIN_HIT_SHIFT: u32 = 2;
+// A hostile workload ends up probing one window in 2^8 rather than one in two.
+const MEMO_MAX_BACKOFF: u32 = 8;
+
+const GOV_ENABLED: u64 = 1 << 63;
+const GOV_HIT_SHIFT: u32 = 32;
+const GOV_COUNT_MASK: u64 = 0xFFFF_FFFF;
+
+static MEMO_GOV: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(1 << 63);
+static MEMO_SKIP_WINDOWS: std::sync::atomic::AtomicU32 = std::sync::atomic::AtomicU32::new(0);
+static MEMO_BACKOFF: std::sync::atomic::AtomicU32 = std::sync::atomic::AtomicU32::new(0);
+
+/// Count this candidate and report whether the memo path is worth walking.
+/// Closes a window every `MEMO_WINDOW` candidates.
+#[inline]
+fn concat_memo_should_probe() -> bool {
+    use std::sync::atomic::Ordering::Relaxed;
+    let v = MEMO_GOV.load(Relaxed);
+    let count = (v & GOV_COUNT_MASK) + 1;
+    if count < MEMO_WINDOW as u64 {
+        MEMO_GOV.store((v & !GOV_COUNT_MASK) | count, Relaxed);
+        return v & GOV_ENABLED != 0;
+    }
+    let hits = (v >> GOV_HIT_SHIFT) & 0x7FFF_FFFF;
+    let now_enabled = if v & GOV_ENABLED != 0 {
+        if (hits << MEMO_MIN_HIT_SHIFT) >= count {
+            MEMO_BACKOFF.store(0, Relaxed);
+            true
+        } else {
+            let b = (MEMO_BACKOFF.load(Relaxed) + 1).min(MEMO_MAX_BACKOFF);
+            MEMO_BACKOFF.store(b, Relaxed);
+            MEMO_SKIP_WINDOWS.store(1u32 << b, Relaxed);
+            false
+        }
+    } else {
+        // Serving out a backoff; expire it into a probation window so a
+        // workload that turns hot later is still picked up.
+        let left = MEMO_SKIP_WINDOWS.load(Relaxed).saturating_sub(1);
+        MEMO_SKIP_WINDOWS.store(left, Relaxed);
+        left == 0
+    };
+    MEMO_GOV.store(if now_enabled { GOV_ENABLED } else { 0 }, Relaxed);
+    now_enabled
+}
+
+#[inline]
+fn concat_memo_note_hit() {
+    MEMO_GOV.fetch_add(1u64 << GOV_HIT_SHIFT, std::sync::atomic::Ordering::Relaxed);
+}
+
+#[cfg(test)]
+pub(crate) fn test_reset_memo_governor() {
+    use std::sync::atomic::Ordering::Relaxed;
+    MEMO_GOV.store(GOV_ENABLED, Relaxed);
+    MEMO_SKIP_WINDOWS.store(0, Relaxed);
+    MEMO_BACKOFF.store(0, Relaxed);
+    CONCAT_MEMO_TAGS.with(|t| unsafe { (*t.get()).fill(0) });
+}
+
+#[cfg(test)]
+pub(crate) fn test_memo_enabled() -> bool {
+    MEMO_GOV.load(std::sync::atomic::Ordering::Relaxed) & GOV_ENABLED != 0
+}
+
+#[cfg(test)]
+pub(crate) fn test_memo_window() -> u32 {
+    MEMO_WINDOW
+}
+
+#[cfg(test)]
+pub(crate) fn test_memo_should_probe() -> bool {
+    concat_memo_should_probe()
+}
+
+#[cfg(test)]
+pub(crate) fn test_memo_note_hit() {
+    concat_memo_note_hit();
+}
+
+// Admission doorkeeper (#9391). A result must be observed TWICE before it
+// earns a memo entry.
+//
+// Without this the memo is a net loss on exactly the workload it looks most
+// applicable to. `bench_gc_pressure` builds `"item_" + i` half a million
+// times with every result distinct: the memo missed 100% of the time, yet
+// inserted on every miss — keeping up to 512 strings alive as strong GC
+// roots, promoting garbage that should have died young, and adding a root
+// scan per collection, all for zero hits. Measured 18 ms with the memo
+// against 12 ms without.
+//
+// A one-byte hash tag per slot fixes it by construction rather than by
+// governor: a never-repeated key writes its tag and leaves, so it never costs
+// an allocation or a root, while a reused key matches its own tag on the
+// second occurrence and is admitted then. No windows, no hit-rate counters,
+// and no disabled state that can get stuck off. The tags are plain bytes —
+// never addresses — so they are invisible to the collector.
+//
+// A tag collision between two distinct keys only admits one of them a little
+// early; the entry itself is still content-compared on lookup, so admission
+// can never produce a wrong string.
+thread_local! {
+    static CONCAT_MEMO_TAGS: std::cell::UnsafeCell<[u8; CONCAT_MEMO_SIZE]> =
+        const { std::cell::UnsafeCell::new([0u8; CONCAT_MEMO_SIZE]) };
+}
+
+/// Returns true when this result has been seen before and may be admitted.
+/// Otherwise records it and declines, so the first sighting costs nothing but
+/// a byte.
+#[inline]
+fn concat_memo_admit(slot: usize, tag: u8) -> bool {
+    CONCAT_MEMO_TAGS.with(|t| unsafe {
+        let slot_tag = &mut (*t.get())[slot];
+        if *slot_tag == tag {
+            true
+        } else {
+            *slot_tag = tag;
+            false
+        }
+    })
+}
+
 thread_local! {
     static CONCAT_MEMO: std::cell::UnsafeCell<[*mut StringHeader; CONCAT_MEMO_SIZE]> =
         const { std::cell::UnsafeCell::new([std::ptr::null_mut(); CONCAT_MEMO_SIZE]) };
 }
 
+/// Slot and admission tag from one hash walk. The tag is a different slice of
+/// the same digest, so two keys sharing a slot rarely share a tag.
 #[inline]
-fn concat_memo_slot(bytes: &[u8]) -> usize {
+fn concat_memo_slot_and_tag(bytes: &[u8]) -> (usize, u8) {
+    let h = concat_memo_hash(bytes);
+    // FNV-1a avalanches poorly in its high bits, so slicing a tag straight out
+    // of `h >> 32` gave two distinct keys the same tag about half the time —
+    // measured 256,516 admissions in 501,000 probes where ~1/128 was intended,
+    // which defeats the doorkeeper entirely. A splitmix64 finalizer first.
+    let mut z = h;
+    z = (z ^ (z >> 30)).wrapping_mul(0xbf58_476d_1ce4_e5b9);
+    z = (z ^ (z >> 27)).wrapping_mul(0x94d0_49bb_1331_11eb);
+    z ^= z >> 31;
+    (
+        (z as usize) & (CONCAT_MEMO_SIZE - 1),
+        // Never 0: that is the "no key seen here yet" sentinel.
+        (((z >> 40) as u8) | 1),
+    )
+}
+
+#[inline]
+fn concat_memo_hash(bytes: &[u8]) -> u64 {
     // FNV-1a over the result bytes. Content-addressed, so two different
     // operand splits that produce the same string share one entry.
     let mut h: u64 = 0xcbf2_9ce4_8422_2325;
     for &b in bytes {
         h ^= b as u64;
         h = h.wrapping_mul(0x100_0000_01b3);
     }
-    (h as usize) & (CONCAT_MEMO_SIZE - 1)
+    h
 }
 
 /// A cached string with exactly these bytes, or null. The byte compare makes
@@ -371,9 +515,11 @@ fn concat_byte_parts(l: (*const u8, u32), r: (*const u8, u32)) -> f64 {
     // `flags`/`utf16_len` are trivially `0`/`total_blen` and the surrogate
     // canonicalization below is a no-op — the cached string is bit-identical
     // to what the heap path would have built.
-    let memoizable = both_ascii && total_blen <= CONCAT_MEMO_MAX_BYTES;
+    let memoizable =
+        both_ascii && total_blen <= CONCAT_MEMO_MAX_BYTES && concat_memo_should_probe();
     let mut memo_buf = [0u8; CONCAT_MEMO_MAX_BYTES as usize];
     let mut memo_slot = 0usize;
+    let mut memo_admitted = false;
     if memoizable {
         unsafe {
             if l.1 > 0 {
@@ -388,11 +534,16 @@ fn concat_byte_parts(l: (*const u8, u32), r: (*const u8, u32)) -> f64 {
             }
         }
         let bytes = &memo_buf[..total_blen as usize];
-        memo_slot = concat_memo_slot(bytes);
+        let (slot, tag) = concat_memo_slot_and_tag(bytes);
+        memo_slot = slot;
         let hit = concat_memo_lookup(memo_slot, bytes);
         if !hit.is_null() {
+            concat_memo_note_hit();
             return f64::from_bits(crate::value::JSValue::string_ptr(hit).bits());
         }
+        // Same admission rule as the `"prefix" + i` arm: a first sighting
+        // costs a tag byte, never a rooted string. See `concat_memo_admit`.
+        memo_admitted = concat_memo_admit(memo_slot, tag);
     }
 
     // Heap path — allocate a StringHeader and memcpy. Decode both
@@ -449,7 +600,7 @@ fn concat_byte_parts(l: (*const u8, u32), r: (*const u8, u32)) -> f64 {
         // Merge any surrogate pair newly formed across the join boundary
         // (no-op unless the result carries the lone-surrogate flag).
         let ptr = canonicalize_surrogate_pairs(ptr);
-        if memoizable {
+        if memoizable && memo_admitted {
             // Publish only after the header and payload are fully written:
             // the memo is a GC root, so a half-built entry would be traced.
             concat_memo_insert(memo_slot, ptr);
@@ -651,9 +802,11 @@ pub extern "C" fn js_string_concat_value(
             && is_valid_string_ptr(prefix)
             && prefix_u16 == prefix_blen
             && unsafe { (*prefix).flags == 0 }
-            && bytes_all_ascii(string_data(prefix), prefix_blen);
+            && bytes_all_ascii(string_data(prefix), prefix_blen)
+            && concat_memo_should_probe();
         let mut memo_buf = [0u8; CONCAT_MEMO_MAX_BYTES as usize];
         let mut memo_slot = 0usize;
+        let mut memo_admitted = false;
         if memoizable {
             unsafe {
                 if prefix_blen > 0 {
@@ -670,11 +823,16 @@ pub extern "C" fn js_string_concat_value(
                 );
             }
             let bytes = &memo_buf[..total_blen];
-            memo_slot = concat_memo_slot(bytes);
+            let (slot, tag) = concat_memo_slot_and_tag(bytes);
+            memo_slot = slot;
             let hit = concat_memo_lookup(memo_slot, bytes);
             if !hit.is_null() {
+                concat_memo_note_hit();
                 return hit;
             }
+            // Missed: admit only a result we have seen before, so a stream of
+            // never-repeated keys costs a tag byte instead of a rooted string.
+            memo_admitted = concat_memo_admit(memo_slot, tag);
         }
 
         let (ptr, data_ptr, prefix) =
@@ -718,7 +876,7 @@ pub extern "C" fn js_string_concat_value(
             );
         }
 
-        if memoizable {
+        if memoizable && memo_admitted {
             // Publish only after header and payload are written: the memo is a
             // GC root, so a half-built entry would be traced.
             concat_memo_insert(memo_slot, ptr);

crates/perry-runtime/src/string/tests.rs

@@ -973,18 +973,24 @@ fn concat_memo_returns_one_object_for_equal_results() {
     let _lock = crate::gc::global_side_table_test_lock();
     crate::string::concat::test_clear_concat_memo();
 
+    crate::string::concat::test_reset_memo_governor();
     let prefix = crate::string::js_string_from_bytes(b"field_".as_ptr(), 6);
-    let first = crate::string::js_string_concat_value(prefix, 7.0);
+    // #9391's doorkeeper admits a result on its SECOND sighting, so the third
+    // evaluation is the first that can share. That ordering is the point: a
+    // result seen once never costs a rooted entry.
+    let _first = crate::string::js_string_concat_value(prefix, 7.0);
+    let second = crate::string::js_string_concat_value(prefix, 7.0);
     // A DIFFERENT prefix object with the same bytes must still reach the entry:
     // the memo is keyed on result content, not on operand identity.
     let other_prefix = crate::string::js_string_from_bytes(b"field_".as_ptr(), 6);
     assert_ne!(prefix as usize, other_prefix as usize);
-    let second = crate::string::js_string_concat_value(other_prefix, 7.0);
+    let third = crate::string::js_string_concat_value(other_prefix, 7.0);
 
     assert_eq!(
-        first as usize, second as usize,
-        "equal concat results must share one memoized string"
+        second as usize, third as usize,
+        "once admitted, equal concat results must share one memoized string"
     );
+    let first = third;
     unsafe {
         assert_eq!((*first).byte_len, 7);
         // Shared, so the in-place `+=` append can never mutate it under a
@@ -1045,3 +1051,75 @@ fn concat_memo_declines_non_ascii_prefixes() {
         assert_eq!((*a).utf16_len, 3);
     }
 }
+
+/// #9391: the memo must stop PROBING when it stops paying.
+///
+/// `bench_gc_pressure` builds half a million distinct `"item_" + i` strings.
+/// Before the governor the memo probed every one of them for six hits, and the
+/// probe alone — buffer assembly plus hashing — cost the row 21 ms against
+/// 12 ms with the memo compiled out.
+///
+/// This asserts the governor's DECISION, not a wall-clock number: a timing
+/// test here would be noise-sensitive and would not say why it failed.
+#[test]
+fn concat_memo_governor_disables_itself_when_nothing_hits() {
+    crate::string::concat::test_reset_memo_governor();
+    assert!(
+        crate::string::concat::test_memo_enabled(),
+        "governor starts enabled"
+    );
+
+    // One full window of candidates, none of which hit.
+    let window = crate::string::concat::test_memo_window();
+    for _ in 0..window {
+        crate::string::concat::test_memo_should_probe();
+    }
+    assert!(
+        !crate::string::concat::test_memo_enabled(),
+        "a window with no hits must turn the probe off"
+    );
+}
+
+/// The other half: a workload that DOES hit keeps the memo on, so the governor
+/// cannot silently disable the case the memo exists for
+/// (`bench_object_property`, which hits 211,960 times out of 212,000).
+#[test]
+fn concat_memo_governor_stays_on_when_hits_are_frequent() {
+    crate::string::concat::test_reset_memo_governor();
+    let window = crate::string::concat::test_memo_window();
+    for _ in 0..window {
+        crate::string::concat::test_memo_should_probe();
+        crate::string::concat::test_memo_note_hit();
+    }
+    assert!(
+        crate::string::concat::test_memo_enabled(),
+        "a window that hits every time must keep the probe on"
+    );
+}
+
+/// And it recovers: a backoff always expires into a probation window, so a
+/// program whose first phase misses and whose second phase hits is picked up
+/// rather than left permanently disabled.
+#[test]
+fn concat_memo_governor_recovers_after_backoff() {
+    crate::string::concat::test_reset_memo_governor();
+    let window = crate::string::concat::test_memo_window();
+    for _ in 0..window {
+        crate::string::concat::test_memo_should_probe();
+    }
+    assert!(!crate::string::concat::test_memo_enabled());
+
+    // Serving the backoff out must eventually re-enable. The exact number of
+    // windows is a tuning detail; that it terminates is the contract.
+    let mut windows = 0;
+    while !crate::string::concat::test_memo_enabled() && windows < 8 {
+        for _ in 0..window {
+            crate::string::concat::test_memo_should_probe();
+        }
+        windows += 1;
+    }
+    assert!(
+        crate::string::concat::test_memo_enabled(),
+        "backoff must expire into a probation window, got stuck for {windows} windows"
+    );
+}