diff --git a/crates/perry-runtime/src/string/concat.rs b/crates/perry-runtime/src/string/concat.rs index fd8080a26c..74e26de688 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. +crate::perry_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 + } + }) +} + crate::perry_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" + ); +} diff --git a/scripts/gc_runtime_root_holders.json b/scripts/gc_runtime_root_holders.json index c44d502fa4..406427cb7b 100644 --- a/scripts/gc_runtime_root_holders.json +++ b/scripts/gc_runtime_root_holders.json @@ -433,6 +433,12 @@ "verdict": "not_a_gc_pointer", "why": "Derived cache of the Set's own elements array, which is the canonical scanned storage (GcRewriteDescriptorKind::Set element-slot walk). Not root-scanner-shaped: the outer address key is rekeyed on relocation by GcMoveHookKind::SetSideTables -> set_header_moved_for_gc (set.rs:352), the JSValueKey inner keys are rebuilt post-rewrite by GcRewriteHookKind::SetIndex -> rebuild_set_index_for_gc (set.rs:315, fired from gc/copying.rs:669/798, gc/barrier/mod.rs:195, gc/verify.rs:114), and finalize_set_side_allocation_for_gc (set.rs:374) prunes dead owners — the same three-hook design as MAP_INDEX." }, + { + "file": "crates/perry-runtime/src/string/concat.rs", + "name": "CONCAT_MEMO_TAGS", + "verdict": "not_a_gc_pointer", + "why": "#9391 admission doorkeeper: one hash tag byte per CONCAT_MEMO slot, so a concat result must be observed twice before it earns a rooted entry. A [u8; 512] of plain bytes derived from a splitmix64-finalized hash — never an address, so the collector never sees a pointer here. The admitted strings live in CONCAT_MEMO, which scan_concat_memo_roots_mut visits." + }, { "file": "crates/perry-runtime/src/string/format.rs", "name": "POW10",