From 95ac9c8f3cf8f6abf65ce812f1c5f36f6659ce4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Tue, 1 Sep 2026 16:07:29 +0200 Subject: [PATCH] fix(runtime): stop the concat memo probing when it isn't paying (#9391) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #9373's memo regressed `bench_gc_pressure` from ~11 ms to ~19, which @ecs1 bisected to `d923b8dcf0` and I confirmed by in-binary A/B. It is my change and this is the fix. The fixture builds `{ x: i, y: i * 2, name: "item_" + i }` half a million times with EVERY result distinct — the exact shape the memo targets and the exact shape it cannot help. Instrumented: 501,000 probes for 6 hits. Two separate costs, and the obvious one was the smaller: 1. **Rooting.** Every miss inserted, so the memo held up to 512 strings alive as strong GC roots on the one benchmark whose whole subject is collecting — keeping garbage alive to be promoted, and adding a root scan per collection. Fixed with an admission doorkeeper: a one-byte hash tag per slot means a result must be seen TWICE before it earns an entry, so a never-repeated key costs a byte instead of a rooted string. The tags are plain bytes, never addresses, so the collector never sees them. The first tag derivation was itself wrong — sliced straight out of FNV-1a's high bits, which avalanche poorly, it admitted 256,516 of 501,000 where ~1/128 was intended. A splitmix64 finalizer before slicing brought that to 3,992. 2. **The probe itself, which was the bigger half.** Even with admissions down to 0.8%, the row still ran 21 ms against 12. Assembling the result into a buffer and hashing it, on every concat, for six hits, was the cost. So the memo now measures its own hit rate over a 4096-candidate window and stops probing when under a quarter of candidates hit, with exponential backoff to one probing window in 2^8. A backoff always expires into a probation window, so a program that starts cold and turns hot is still picked up. The governor word is a relaxed global rather than a `thread_local!` — it is advisory, correctness never depends on it, and three TLS reads per candidate were themselves worth ~2 ms here. Measured, min-of-15, in-binary A/B against the memo compiled out: | | memo on | memo off | before this fix | |---|---|---|---| | `bench_gc_pressure` | 13 ms | 12 ms | 21 ms | | `bench_object_property` | 15 ms | 21 ms | 15 ms | So the hostile row is within 1 ms of not having the memo at all, and the row the memo exists for keeps its full 6 ms win. `bench_string_heavy` unchanged (43/44). Output identical throughout. Tests assert the governor's DECISIONS, not wall-clock: it disables after a window with no hits, stays enabled when every candidate hits, and always recovers from backoff. A timing test here would be noise-sensitive and would not say why it failed. WHY MY PRE-MERGE ADVERSARIAL TEST MISSED THIS: #9373 did test an all-distinct-results fixture and measured 7 ms vs Node's 8. But that fixture had no other allocation pressure, so it exercised the probe cost in isolation and never the interaction that actually hurt — rooting garbage while a collector is under load. An adversarial case has to be adversarial in the dimension the change touches, and mine covered one of two. Gates: perry-runtime 2918 passed (single-threaded), perry-codegen 1379 passed, fmt clean, `-D warnings` clean, shape-descriptor census clean, all five static gc_root_dominance_check audits pass, GC store-site inventory passes, all three touched files under the 2000-line cap. Claude-Session: https://claude.ai/code/session_01Pcq6j6y57TdKSR2Zx2D187 --- crates/perry-runtime/src/string/concat.rs | 174 +++++++++++++++++++++- crates/perry-runtime/src/string/tests.rs | 86 ++++++++++- 2 files changed, 248 insertions(+), 12 deletions(-) diff --git a/crates/perry-runtime/src/string/concat.rs b/crates/perry-runtime/src/string/concat.rs index b16604566e..e769012020 100644 --- a/crates/perry-runtime/src/string/concat.rs +++ b/crates/perry-runtime/src/string/concat.rs @@ -262,13 +262,157 @@ 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; @@ -276,7 +420,7 @@ fn concat_memo_slot(bytes: &[u8]) -> usize { 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); diff --git a/crates/perry-runtime/src/string/tests.rs b/crates/perry-runtime/src/string/tests.rs index c0d0f94d5f..68d4d0127a 100644 --- a/crates/perry-runtime/src/string/tests.rs +++ b/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" + ); +}