From dd83a64649a27d99511a556979f4bd32999e4325 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Fri, 18 Sep 2026 21:34:49 +0200 Subject: [PATCH 1/2] perf(runtime): stop double-scanning ASCII-ness and buffering the concat memo probe concat_byte_parts (the s + t fast path for two statically-typed string operands) scanned both operands for ASCII-ness twice - once via bytes_all_ascii up front, again on the heap path via l_slice.is_ascii() && r_slice.is_ascii() - with the first scan's answer sitting unused in scope. A new sibling of str_bytes_from_jsvalue, str_bytes_ascii_from_jsvalue, computes the bit once and threads it through; bytes_all_ascii itself switches from a byte-at-a-time loop to <[u8]>::is_ascii() (word-at-a-time, total over arbitrary byte strings). An earlier version of this change also tried to read a heap string's ASCII-ness straight off its header (utf16_len == byte_len, free) instead of scanning at all. That is unsound and was caught in review before landing: Perry heap-string payloads are not guaranteed valid UTF-8 (WTF-8 lone surrogates, Buffer.toString of arbitrary bytes, FFI blobs - #6085), and a payload ending in a truncated multi-byte lead byte can coincide on utf16_len == byte_len without being ASCII (compute_utf16_len_wtf8 charges a truncated lead its full nominal unit count while the payload holds fewer bytes than that sequence declares - string/compare.rs's utf16_cmp_bytes doc names the identical hazard). The header check survives only as a negative filter (utf16_len != byte_len soundly proves non-ASCII, unconditionally, not just for well-formed input); utf16_len == byte_len is ambiguous and always falls back to a real scan. js_string_concat_value's memo-admission gate had the identical exposure independently and is fixed the same way. No TypeScript-reachable path that constructs such a payload was found: Buffer.toString (all seven encodings), TextDecoder.decode, and every bun:ffi string-returning path all validate via str::from_utf8/from_utf8_lossy (or are fed a Rust &str, valid by construction) before ever calling js_string_from_bytes. The fix stands regardless, since js_string_from_bytes is a pub extern "C" entry point whose own contract must hold for any bytes. Regression tests are therefore Rust-level against hand-built malformed StringHeaders (the same technique string/compare.rs's own corpus and tests_guard_page.rs already use) rather than a gap test - all three fail against the reverted code, confirmed by temporarily reintroducing it. The short-concat memo's probe also assembled both operands into a stack buffer just to hand the hash/lookup helpers one contiguous slice. FNV-1a is a streaming hash and the byte compare can run in two parts, so concat_memo_hash_parts / concat_memo_slot_and_tag_parts / concat_memo_lookup_parts replace the buffer with direct two-slice hashing and lookup; the single-slice js_string_concat_value memo probe now goes through the same two-slice primitives. The memo probe's own break-even hit rate (governed by the probe's hash/lookup/admit cost, not by the ASCII-determination fix) barely moved: ~54% before this fix, ~50.6% after, both measured by forcing the governor on/off via a temporary env knob (since removed). MEMO_MIN_HIT_SHIFT stays 1 (50%, still the closest power-of-two floor to either number) - it moved from the original 2 (25%, never measured) in this same change. Measured (differential instruction-count probe, bare-loop control flat in both arms, base and arm built in the same session): 640-distinct short concat 548 -> 403 instructions/concat (-26.5%), a 73-byte memo-ineligible concat 644 -> 471 (-26.8%), a 100%-memo-hit workload 319 -> 318 (unchanged, within noise). Covered by test-files/test_gap_string_concat_memo_ascii_header.ts (byte-for-byte against node) and GC stress on a concat-heavy fixture (117,923 copying minors, 14,739 retired from-space sets quarantined, no fault) confirming the memo's GC roots survive evacuation under the new two-slice storage. --- .../string-concat-memo-ascii-header.md | 104 ++++++++ crates/perry-runtime/src/string/concat.rs | 229 ++++++++++++------ crates/perry-runtime/src/string/mod.rs | 64 +++++ crates/perry-runtime/src/string/tests.rs | 131 ++++++++++ ...est_gap_string_concat_memo_ascii_header.ts | 186 ++++++++++++++ 5 files changed, 637 insertions(+), 77 deletions(-) create mode 100644 changelog.d/string-concat-memo-ascii-header.md create mode 100644 test-files/test_gap_string_concat_memo_ascii_header.ts diff --git a/changelog.d/string-concat-memo-ascii-header.md b/changelog.d/string-concat-memo-ascii-header.md new file mode 100644 index 0000000000..c99f6ac1ef --- /dev/null +++ b/changelog.d/string-concat-memo-ascii-header.md @@ -0,0 +1,104 @@ +**String concat: stop re-scanning bytes for ASCII-ness twice, and stop +building a scratch buffer just to hash it** — 640-distinct short-string +concat down 26.5% (548 → 403 instructions/concat), a memo-ineligible 73-byte +concat down 26.8% (644 → 471), a 100%-memo-hit workload unchanged (319 → +318, within noise), measured with a differential instruction-count probe +(median-of-7, base vs arm built in the same session; bare-loop control flat +at ~3 in both). + +`concat_byte_parts` (the `s + t` fast path for two statically-typed string +operands, `perry-runtime/src/string/concat.rs`) had three defects, all in the +same neighborhood: + +1. It scanned both operands for ASCII-ness via `bytes_all_ascii` up front, + then scanned them *again* on the heap path via `l_slice.is_ascii() && + r_slice.is_ascii()` — with the first scan's answer (`both_ascii`) sitting + in scope, unused. Fixed by computing the bit once, in the caller (the new + `str_bytes_ascii_from_jsvalue`, a sibling of `str_bytes_from_jsvalue`), + and threading it through. +2. `bytes_all_ascii` scanned byte-at-a-time (`.iter().all(|&b| b < 0x80)`). + `<[u8]>::is_ascii()` inspects the same bytes word-at-a-time and is total + over arbitrary byte strings, valid or not (see the soundness note below — + that "total over arbitrary bytes" property is why it's the only sound + choice here, not just the faster one). Switching to it — `bytes_all_ascii`'s + body, and `str_bytes_ascii_from_jsvalue`'s scan — is most of this change's + win on `long73`: that workload's improvement is mostly the scan itself + getting faster over ~140 bytes/concat, not any trick that avoids it. +3. The short-concat memo's probe assembled both operands into a 12-byte stack + buffer just to hand the hash/lookup helpers one contiguous slice. FNV-1a is + a streaming hash (`fnv(a ++ b)` needs no buffer, just fold `a` then `b`), + and the byte compare on a hit/miss can run in the same two parts against + the cached entry. `concat_memo_hash_parts` / `concat_memo_slot_and_tag_parts` + / `concat_memo_lookup_parts` replace the buffer with direct two-slice + hashing and lookup; the single-slice `js_string_concat_value` ("prefix" + + i) memo probe is now implemented in terms of the same two-slice + primitives. + +**An earlier version of this change also tried to skip the scan entirely**, +by reading a heap string's ASCII-ness straight off its header (`utf16_len == +byte_len`, already computed at construction, so free). That is unsound, and +was caught in review before landing: Perry heap-string payloads are not +guaranteed valid UTF-8 (WTF-8 lone surrogates, `Buffer.toString` of +arbitrary bytes, FFI blobs — #6085), and a payload ending in a truncated +multi-byte lead byte can coincide on `utf16_len == byte_len` without being +ASCII — `compute_utf16_len_wtf8` charges a truncated lead its full nominal +unit count while the payload holds fewer bytes than that sequence declares +(`[0xC3]`, a lone 2-byte lead, records `utf16_len == 1 == byte_len`; +`string/compare.rs`'s `utf16_cmp_bytes` doc names the identical hazard and +pins the identical corpus for its own ASCII fast path — this change's +`str_bytes_ascii_from_jsvalue` doc now cross-references it). The header +check survives only as a NEGATIVE filter: `utf16_len != byte_len` soundly +proves non-ASCII with no scan needed, because `compute_utf16_len_wtf8` +counts exactly one unit per byte for any run of bytes `< 0x80` — the +contrapositive holds unconditionally, not just for well-formed input — but +`utf16_len == byte_len` is ambiguous and always falls back to a real +`is_ascii()` scan. `js_string_concat_value`'s memo-admission gate had the +identical exposure independently: `prefix_u16 == prefix_blen` was treated as +sufficient on its own and the `bytes_all_ascii` check right after it deleted +as redundant with it; restored, with the same negative-filter-then-scan +reasoning documented at the call site. + +No TypeScript-reachable path that constructs such a payload was found: +`Buffer.toString` (all seven encodings, `buffer/encode.rs`), +`TextDecoder.decode` (`text.rs`), and every `bun:ffi` string-returning path +(`read_cstring_value`, `dlopen.rs`'s `CString`/`cstring` conversions) all +validate via `str::from_utf8`/`from_utf8_lossy` (or are fed a Rust `&str`, +valid by construction) before ever calling `js_string_from_bytes` — `#609` +closed these same construction sites for a related UB hazard, and the fix +happens to guarantee well-formed output too. The fix stands regardless: +relying on an invariant this tree already documents as unsound is the wrong +foundation, and `js_string_from_bytes` is a `pub extern "C"` entry point +whose own contract must hold for any bytes, whether or not today's call +graph happens to always validate first. Regression tests are therefore +Rust-level, against hand-built malformed `StringHeader`s — the same +technique `string/compare.rs`'s own corpus and `tests_guard_page.rs` already +use — rather than a gap test: +`ascii_probe_falls_back_to_a_scan_when_the_header_lies`, +`concat_memo_declines_a_prefix_whose_header_lies_about_being_ascii`, +`concat_box_reports_not_well_formed_for_a_malformed_operand_either_side` +(`perry-runtime/src/string/tests.rs`) — all three fail against the reverted +(unsound) code, confirmed by temporarily reintroducing it and reverting +back. + +The memo probe's own break-even hit rate (governed by defect 3's mechanics — +hash/lookup/admit cost — not by the ASCII determination defects 1/2 changed) +barely moved between the unsound and sound paths: ~54% with the unsound +header shortcut, ~50.6% with the sound negative-filter-then-scan, both +measured by forcing the governor on/off via a temporary env knob (since +removed). `MEMO_MIN_HIT_SHIFT` stays `1` (50%, still the closest +power-of-two floor to either number) — it moved from the original `2` (25%, +chosen as a plausible fraction, never measured against the probe's own cost) +in this same change, which is what made the floor worth re-deriving at all. + +Covered by `test-files/test_gap_string_concat_memo_ascii_header.ts` +(byte-for-byte against node): ASCII boundary lengths crossing the SSO (5) and +memo (12) ceilings, 2/3/4-byte non-ASCII operands, a surrogate pair formed +*across* the join boundary and one that deliberately isn't (reverse order), +empty operands, and repeated-identical concats (both split two different +ways) to exercise the memo's "seen twice" admission and its `===` identity. +GC stress (`PERRY_GC_SCHEDULE_SEED=1` and `=42`, `RATE=1`, +`PROTECT_FROMSPACE=1`, `VERIFY_EVACUATION=1`, `FROMSPACE_SCAN_ABORT=1`) on a +concat-heavy fixture ran 117,923 copying minors and quarantined 14,739 +retired from-space sets with no fault and output still matching node, +confirming the memo's GC roots (`scan_concat_memo_roots_mut`) survive +evacuation under the new two-slice storage. diff --git a/crates/perry-runtime/src/string/concat.rs b/crates/perry-runtime/src/string/concat.rs index 3f4c1ef80f..ce30ed6839 100644 --- a/crates/perry-runtime/src/string/concat.rs +++ b/crates/perry-runtime/src/string/concat.rs @@ -104,15 +104,18 @@ pub(crate) fn canonicalize_surrogate_pairs(ptr: *mut StringHeader) -> *mut Strin /// True when the `len` bytes at `data` are all ASCII (`< 0x80`), or the slice /// is empty/null. Used to decide whether a concat result may be stored inline -/// through the concat helpers' ASCII SSO fast path. +/// through the concat helpers' ASCII SSO fast path. `<[u8]>::is_ascii` +/// inspects the bytes word-at-a-time and is total over arbitrary byte +/// strings — the only sound way to answer this for a Perry heap-string +/// payload, which is not guaranteed valid UTF-8 (see +/// [`str_bytes_ascii_from_jsvalue`](super::str_bytes_ascii_from_jsvalue)'s +/// doc for why the header's `utf16_len == byte_len` cannot stand in for it). #[inline] fn bytes_all_ascii(data: *const u8, len: u32) -> bool { if data.is_null() || len == 0 { return true; } - unsafe { std::slice::from_raw_parts(data, len as usize) } - .iter() - .all(|&b| b < 0x80) + unsafe { std::slice::from_raw_parts(data, len as usize) }.is_ascii() } /// `ptr::copy_nonoverlapping` with a byte loop for short payloads: the libc @@ -194,21 +197,24 @@ pub extern "C" fn js_string_concat_box(l_value: f64, r_value: f64) -> f64 { // NaN-boxed — keeps the dynamic arm. One side must still be a REAL // string, so the annotation-lie semantics of the dynamic arm are // unchanged for number+number. + // Digits from `fast_itoa_u32` are always ASCII (`'0'..='9'`, no sign — the + // admission range below is non-negative), so this arm's third tuple + // element is a constant `true`, never a scan. #[inline] - fn itoa_operand(bits_value: f64, buf: &mut [u8; 32]) -> Option<(*const u8, u32)> { + fn itoa_operand(bits_value: f64, buf: &mut [u8; 32]) -> Option<(*const u8, u32, bool)> { let bits = bits_value.to_bits(); let tag = bits >> 48; let is_plain_f64 = tag < 0x7FF8 || (tag == 0x7FF8 && (bits & 0x000F_FFFF_FFFF_FFFF) == 0); if is_plain_f64 && bits_value.fract() == 0.0 && (0.0..=999_999_999.0).contains(&bits_value) { let len = fast_itoa_u32(bits_value as u32, buf); - Some((buf.as_ptr(), len as u32)) + Some((buf.as_ptr(), len as u32, true)) } else { None } } - let l_str = str_bytes_from_jsvalue(l_value, &mut scratch_l); - let r_str = str_bytes_from_jsvalue(r_value, &mut scratch_r); + let l_str = str_bytes_ascii_from_jsvalue(l_value, &mut scratch_l); + let r_str = str_bytes_ascii_from_jsvalue(r_value, &mut scratch_r); if let (Some(l), Some(r)) = (l_str, r_str) { // Two real strings: straight to assembly, no number buffer touched // (the itoa scratch below would cost this path a 32-byte memset). @@ -231,9 +237,9 @@ pub extern "C" fn js_string_concat_box(l_value: f64, r_value: f64) -> f64 { } _ => {} } - // `str_bytes_from_jsvalue` returns `None` for exactly the non-string - // values, so every remaining pair — number+number included — is the - // annotation-lie arm and nothing else. + // `str_bytes_ascii_from_jsvalue` returns `None` for exactly the + // non-string values, so every remaining pair — number+number included — + // is the annotation-lie arm and nothing else. unsafe { crate::value::js_dynamic_string_or_number_add(l_value, r_value) } } @@ -266,8 +272,34 @@ 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; +// Earn the probe: at least half a window's candidates must hit. +// +// This was `2` (a 25% floor) — chosen as a plausible fraction, never measured +// against the probe's own cost. A differential instruction-count probe +// (`"abcdefgN" + "xJJ"`, N ∈ 8, JJ ∈ 0..79 — 640 distinct 11-byte results +// against the memo's 512 slots, vs an 8-distinct 10-byte set that hits +// ~100%) against the SAME binary with the governor's decision forced ON/OFF +// via a temporary env knob (since removed) put the break-even — the hit rate +// at which the memo's probe cost equals its allocation savings — at: +// +// before F0/F1/F2 (double ASCII scan + stack-buffer memo probe): ~69-72% +// after F0/F1 + F2's UNSOUND positive header-ASCII path (since reverted, +// see `str_bytes_ascii_from_jsvalue`'s doc): ~54% +// after F0/F1 + F2 corrected to a sound header-negative-filter +// -then-scan (current code): ~50.6% +// +// (`cost / (cost + save)`, reading `cost` off the low-hit-rate workload and +// `save` off the ~100%-hit one — both relative to the same memo-off +// baseline, which the `long73` memo-ineligible control confirmed was flat +// across the forced on/off runs, so the two workloads' allocation-path costs +// are comparable). The break-even barely moved between the unsound and +// sound versions of F2, because this governor times the MEMO PROBE itself +// (hash/lookup/admit, F1's concern) — not the ASCII determination that +// gates whether `concat_byte_parts` reaches the probe at all, which F2 +// changed. `1` (a 50% floor) was already the closest power-of-two to the +// unsound path's ~54%, and it is still the closest power-of-two to the +// sound path's ~50.6% — no change from the F2 fix. +const MEMO_MIN_HIT_SHIFT: u32 = 1; // A hostile workload ends up probing one window in 2^8 rather than one in two. const MEMO_MAX_BACKOFF: u32 = 8; @@ -393,11 +425,33 @@ crate::perry_thread_local! { 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. +/// FNV-1a over concatenated content `a ++ b`, without materialising the +/// concatenation. FNV-1a is a streaming hash — folding in `a`'s bytes then +/// `b`'s bytes is bit-identical to folding in `(a ++ b)`'s bytes — so a +/// two-operand walk needs no scratch buffer at all. This is the memo's +/// analogue of the intern table's `fnv1a_concat`, over raw byte slices +/// instead of `StringHeader` pointers (the memo's operands may be an SSO +/// scratch view, not a heap header). #[inline] -fn concat_memo_slot_and_tag(bytes: &[u8]) -> (usize, u8) { - let h = concat_memo_hash(bytes); +fn concat_memo_hash_parts(a: &[u8], b: &[u8]) -> u64 { + let mut h: u64 = 0xcbf2_9ce4_8422_2325; + for &byte in a { + h ^= byte as u64; + h = h.wrapping_mul(0x100_0000_01b3); + } + for &byte in b { + h ^= byte as u64; + h = h.wrapping_mul(0x100_0000_01b3); + } + h +} + +/// Slot and admission tag from one hash walk over `a ++ b`. 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_and_tag_parts(a: &[u8], b: &[u8]) -> (usize, u8) { + let h = concat_memo_hash_parts(a, b); // 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, @@ -413,38 +467,45 @@ fn concat_memo_slot_and_tag(bytes: &[u8]) -> (usize, u8) { ) } +/// A cached string whose content is exactly `a ++ b`, or null. The compare is +/// done in the same two parts, against the cached entry's payload — no +/// scratch buffer, and a hash collision is a miss, never a wrong answer. #[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 -} - -/// A cached string with exactly these bytes, or null. The byte compare makes -/// a hash collision a miss, never a wrong answer. -#[inline] -fn concat_memo_lookup(slot: usize, bytes: &[u8]) -> *mut StringHeader { +fn concat_memo_lookup_parts(slot: usize, a: &[u8], b: &[u8]) -> *mut StringHeader { let cached = CONCAT_MEMO.with(|c| unsafe { (*c.get())[slot] }); if cached.is_null() { return std::ptr::null_mut(); } unsafe { - if (*cached).byte_len as usize != bytes.len() { + if (*cached).byte_len as usize != a.len() + b.len() { return std::ptr::null_mut(); } let data = crate::string::string_data(cached); - if std::slice::from_raw_parts(data, bytes.len()) != bytes { + if !a.is_empty() && std::slice::from_raw_parts(data, a.len()) != a { + return std::ptr::null_mut(); + } + if !b.is_empty() && std::slice::from_raw_parts(data.add(a.len()), b.len()) != b { return std::ptr::null_mut(); } } cached } +/// Single-slice callers (the `"prefix" + i` arm in +/// [`js_string_concat_value`], which already has its two pieces contiguous +/// in a scratch buffer by the time it probes) go through the two-slice +/// primitives with an empty second operand — one hash/lookup definition, +/// not two. +#[inline] +fn concat_memo_slot_and_tag(bytes: &[u8]) -> (usize, u8) { + concat_memo_slot_and_tag_parts(bytes, &[]) +} + +#[inline] +fn concat_memo_lookup(slot: usize, bytes: &[u8]) -> *mut StringHeader { + concat_memo_lookup_parts(slot, bytes, &[]) +} + #[inline] fn concat_memo_insert(slot: usize, ptr: *mut StringHeader) { CONCAT_MEMO.with(|c| unsafe { @@ -482,16 +543,39 @@ pub(crate) fn test_clear_concat_memo() { }); } +/// Byte view over a `(ptr, len)` operand, empty for a null/zero-length one. +/// `slice::from_raw_parts` requires a non-null, aligned pointer even at +/// length 0, so the null check must come first. +/// +/// # Safety +/// `ptr` must be valid for `len` bytes when non-null. +#[inline(always)] +unsafe fn operand_byte_slice<'a>(ptr: *const u8, len: u32) -> &'a [u8] { + if ptr.is_null() || len == 0 { + &[] + } else { + std::slice::from_raw_parts(ptr, len as usize) + } +} + /// Shared tail of [`js_string_concat_box`]: assemble two raw byte slices /// (each a real string's payload or an itoa'd integer) into an SSO immediate /// when the total fits five ASCII bytes, a heap `StringHeader` otherwise. +/// +/// The third tuple element is whether that operand is pure ASCII, computed +/// once by the caller — see +/// [`str_bytes_ascii_from_jsvalue`](super::str_bytes_ascii_from_jsvalue) for +/// how (a sound header-filter-then-scan for a heap string, a plain scan for +/// an SSO one, or a constant `true` for an itoa'd operand). Taking it as a +/// precomputed bit here, instead of re-deriving it with a fresh byte scan, is +/// F0: this function used to scan both operands for ASCII-ness twice (once +/// via `bytes_all_ascii` up front, again via `l_slice.is_ascii() && +/// r_slice.is_ascii()` on the heap path below with `both_ascii` sitting +/// unused in scope) — one real scan per operand now, not two. #[inline(always)] -fn concat_byte_parts(l: (*const u8, u32), r: (*const u8, u32)) -> f64 { +fn concat_byte_parts(l: (*const u8, u32, bool), r: (*const u8, u32, bool)) -> f64 { let total_blen = l.1 + r.1; - - // Keep the existing ASCII-only concat fast path. Non-ASCII results use - // the heap path, which also handles WTF-8 surrogate-pair boundaries. - let both_ascii = bytes_all_ascii(l.0, l.1) && bytes_all_ascii(r.0, r.1); + let both_ascii = l.2 && r.2; // SSO fast path — assemble the result inline when it fits (≤ 5 // bytes). Pure bit arithmetic, no heap touch. @@ -509,33 +593,28 @@ fn concat_byte_parts(l: (*const u8, u32), r: (*const u8, u32)) -> f64 { } } - // Memo probe, ahead of the allocation: assemble the result into a stack - // buffer and look it up by content. Restricted to short ASCII results, so - // `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. + // Byte views over both operands — used by the memo probe below and by + // the heap path's copy (and, on the non-ASCII arm only, its UTF-16/flags + // walk). Built once and shared, rather than re-derived per use. + let l_slice: &[u8] = unsafe { operand_byte_slice(l.0, l.1) }; + let r_slice: &[u8] = unsafe { operand_byte_slice(r.0, r.1) }; + + // Memo probe, ahead of the allocation: hash and look up `l_slice ++ + // r_slice` directly (F1 — no stack buffer to materialise the + // concatenation just to ask about it; FNV-1a is a streaming hash and the + // compare runs in the same two parts against the cached entry). Restricted + // to short ASCII results, so `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 && 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 { - std::ptr::copy_nonoverlapping(l.0, memo_buf.as_mut_ptr(), l.1 as usize); - } - if r.1 > 0 { - std::ptr::copy_nonoverlapping( - r.0, - memo_buf.as_mut_ptr().add(l.1 as usize), - r.1 as usize, - ); - } - } - let bytes = &memo_buf[..total_blen as usize]; - let (slot, tag) = concat_memo_slot_and_tag(bytes); + let (slot, tag) = concat_memo_slot_and_tag_parts(l_slice, r_slice); memo_slot = slot; - let hit = concat_memo_lookup(memo_slot, bytes); + let hit = concat_memo_lookup_parts(memo_slot, l_slice, r_slice); if !hit.is_null() { concat_memo_note_hit(); return f64::from_bits(crate::value::JSValue::string_ptr(hit).bits()); @@ -546,26 +625,11 @@ fn concat_byte_parts(l: (*const u8, u32), r: (*const u8, u32)) -> f64 { } // Heap path — allocate a StringHeader and memcpy. Decode both - // operands' byte slices via `str_bytes_from_jsvalue` (already done + // operands' byte slices via `str_bytes_ascii_from_jsvalue` (already done // above) and write directly into the new header's payload region. let (ptr, data_ptr) = string_storage_alloc(total_blen); unsafe { - // ASCII-fast utf16 length: count bytes < 0x80 in both slices in - // one pass. Most concat results are pure ASCII (number formatting, - // ID building, slug construction, etc.); falling back to the - // full Grisu-style codepoint walk for non-ASCII keeps spec - // compliance for the edge case. - let l_slice = if !l.0.is_null() { - std::slice::from_raw_parts(l.0, l.1 as usize) - } else { - &[] - }; - let r_slice = if !r.0.is_null() { - std::slice::from_raw_parts(r.0, r.1 as usize) - } else { - &[] - }; - let (utf16_len, flags) = if l_slice.is_ascii() && r_slice.is_ascii() { + let (utf16_len, flags) = if both_ascii { (total_blen, 0) } else { // Sum each operand's UTF-16 length independently (concatenating two @@ -794,6 +858,17 @@ pub extern "C" fn js_string_concat_value( // and heap-allocates. Restricted to a plain ASCII prefix so the cached // string is bit-identical to what the block below would build // (flags == 0, utf16_len == byte_len). + // + // `prefix_u16 == prefix_blen` is NOT the runtime's ASCII predicate — + // it is necessary but not sufficient: a truncated multi-byte lead + // byte can make a non-ASCII `prefix` coincide on `utf16_len == + // byte_len` too (see `str_bytes_ascii_from_jsvalue`'s doc in + // `string/mod.rs`, and `string/compare.rs`'s `utf16_cmp_bytes` doc, + // for the exact mechanism and a concrete payload). It DOES soundly + // rule out non-ASCII when the lengths differ, so it stays first in + // the chain as a free short-circuit — but when it's true, the + // `bytes_all_ascii` scan below is still required, not redundant + // with it. let memoizable = total_blen <= CONCAT_MEMO_MAX_BYTES as usize && is_valid_string_ptr(prefix) && prefix_u16 == prefix_blen diff --git a/crates/perry-runtime/src/string/mod.rs b/crates/perry-runtime/src/string/mod.rs index 670455727d..b82f892d51 100644 --- a/crates/perry-runtime/src/string/mod.rs +++ b/crates/perry-runtime/src/string/mod.rs @@ -926,6 +926,70 @@ pub fn str_bytes_from_jsvalue( None } +/// Sibling of [`str_bytes_from_jsvalue`] that additionally reports whether the +/// operand is pure ASCII. +/// +/// - Heap `STRING_TAG`: Perry heap-string payloads are **not guaranteed valid +/// UTF-8** (WTF-8 lone surrogates, `Buffer.toString` of arbitrary bytes, FFI +/// blobs — #6085), so the header's `utf16_len == byte_len` can only be used +/// as a one-directional filter, never as the answer: +/// - `utf16_len != byte_len` ⟹ **definitely not ASCII**, no scan needed. +/// This direction is unconditional, not a well-formedness assumption: +/// [`compute_utf16_len_wtf8`] advances exactly one byte and adds exactly +/// one unit per iteration whenever it sees a byte `< 0x80`, so a payload +/// of nothing but such bytes always produces `utf16_len == byte_len` +/// exactly — the contrapositive holds for *any* byte content, valid or +/// not. +/// - `utf16_len == byte_len` does **not** imply ASCII: a truncated +/// multi-byte lead byte is charged its full nominal unit count by +/// [`compute_utf16_len_wtf8`] while the payload holds fewer bytes than +/// that sequence would need, so a short malformed payload can coincide — +/// `[0xC3]` (a lone 2-byte lead) records `utf16_len == 1 == byte_len`, +/// and `[0xF0, 0x41]` (a truncated 4-byte lead followed by an unrelated +/// byte) records `utf16_len == 2 == byte_len` — both non-ASCII. See +/// `string/compare.rs`'s `utf16_cmp_bytes` doc, which documents the same +/// hazard for the same reason. When the header is this ambiguous, fall +/// back to an actual byte scan (`<[u8]>::is_ascii`, word-at-a-time, total +/// over arbitrary bytes — no validity precondition at all). +/// - Inline `SHORT_STRING_TAG`: [`JSValue::try_short_string`] stores whatever +/// bytes it's given verbatim, with no ASCII requirement, and there is no +/// header standing in for the scan — always run `is_ascii()` on the +/// already-materialised ≤5-byte scratch, which is trivial at that size. +/// +/// Left as a separate function (not a shared implementation with +/// `str_bytes_from_jsvalue`) so the latter's ~50 other call sites pay no new +/// cost for a bit they don't use. +#[inline] +pub fn str_bytes_ascii_from_jsvalue( + value: f64, + scratch: &mut [u8; crate::value::SHORT_STRING_MAX_LEN], +) -> Option<(*const u8, u32, bool)> { + let bits = value.to_bits(); + let jsval = crate::value::JSValue::from_bits(bits); + unsafe { + if jsval.is_short_string() { + let n = jsval.short_string_to_buf(scratch); + let ascii = scratch[..n].is_ascii(); + return Some((scratch.as_ptr(), n as u32, ascii)); + } + if jsval.is_string() { + let hdr = jsval.as_string_ptr(); + if hdr.is_null() { + return Some((std::ptr::null(), 0, true)); + } + let data = string_data(hdr); + let byte_len = (*hdr).byte_len; + // `!=` proves non-ASCII outright (see doc above); `==` is + // ambiguous — a truncated/malformed lead byte can coincidentally + // match — so only THAT arm pays for the real scan. + let ascii = (*hdr).utf16_len == byte_len + && std::slice::from_raw_parts(data, byte_len as usize).is_ascii(); + return Some((data, byte_len, ascii)); + } + } + None +} + /// Fast path: create a string from bytes known to be pure ASCII. /// Skips the `compute_utf16_len` byte scan — sets utf16_len = byte_len directly. #[inline] diff --git a/crates/perry-runtime/src/string/tests.rs b/crates/perry-runtime/src/string/tests.rs index f581570e00..362925c21b 100644 --- a/crates/perry-runtime/src/string/tests.rs +++ b/crates/perry-runtime/src/string/tests.rs @@ -1197,6 +1197,137 @@ fn concat_memo_declines_non_ascii_prefixes() { } } +// ── #6085-class regression: a header that LIES about being ASCII ────────── +// +// Perry heap-string payloads are not guaranteed valid UTF-8 (WTF-8 lone +// surrogates, `Buffer.toString` of arbitrary bytes, FFI blobs — #6085). The +// header's `utf16_len == byte_len` predicate is sound as a NEGATIVE filter +// (unequal ⟹ definitely not ASCII) but not as a positive one: a payload +// ending in a truncated multi-byte lead byte can coincide on equal lengths +// without being ASCII — `compute_utf16_len_wtf8` charges a truncated lead +// its full nominal unit count while the payload holds fewer bytes than that +// sequence declares. `[0xC3]` (a lone 2-byte lead) and `[0xF0, 0x41]` (a +// truncated 4-byte lead followed by an unrelated byte) both report +// `utf16_len == byte_len` while being non-ASCII — the exact pair +// `string/compare.rs`'s `cached_utf16_len_predicate_would_misclassify_these` +// pins for the same reason. (`[0x80]`, a bare continuation byte, is NOT in +// this class: `compute_utf16_len_wtf8` skips it as "continuation byte in +// lead position" without counting a unit, so it reports `utf16_len == 0 != +// byte_len == 1` — the negative filter already catches it correctly, no +// scan needed.) +// +// I could not find a TypeScript-reachable path that constructs such a +// payload today: every raw-bytes-to-string channel that could plausibly +// carry attacker/arbitrary bytes — `Buffer.toString` (all seven encodings, +// `buffer/encode.rs`), `TextDecoder.decode` (`text.rs::decode_bytes`), and +// every `bun:ffi` string-returning path (`read_cstring_value`, +// `dlopen.rs`'s `CString`/`cstring` conversions) — validates via +// `str::from_utf8`/`from_utf8_lossy` (or is fed a Rust `&str`, valid by +// construction) before ever calling `js_string_from_bytes`; `#609` closed +// the same construction sites for a related UB hazard and the fix happens +// to guarantee well-formed output too. So these are Rust-level regression +// tests against `js_string_from_bytes` directly (the same technique +// `string/compare.rs`'s own corpus and `tests_guard_page.rs` use) rather +// than a gap test: `js_string_from_bytes` is a `pub extern "C"` entry point +// whose own contract must hold for any bytes, whether or not today's call +// graph happens to always validate first. + +/// [`str_bytes_ascii_from_jsvalue`] must not trust the header's equal-lengths +/// coincidence — it must fall back to a real (word-at-a-time, always sound) +/// byte scan whenever the header is this ambiguous. +#[test] +fn ascii_probe_falls_back_to_a_scan_when_the_header_lies() { + for bytes in [&[0xC3u8][..], &[0xF0u8, 0x41][..]] { + let hdr = js_string_from_bytes(bytes.as_ptr(), bytes.len() as u32); + unsafe { + assert_eq!( + (*hdr).utf16_len, + (*hdr).byte_len, + "{bytes:?}: header must (wrongly) report equal lengths, \ + or this test is not exercising the hazard" + ); + } + let value = f64::from_bits(crate::value::JSValue::string_ptr(hdr).bits()); + let mut scratch = [0u8; crate::value::SHORT_STRING_MAX_LEN]; + let (_ptr, len, ascii) = str_bytes_ascii_from_jsvalue(value, &mut scratch) + .expect("a real string operand must decode"); + assert_eq!(len, bytes.len() as u32); + assert!(!ascii, "{bytes:?} is not ASCII"); + } +} + +/// The `"prefix" + i` memo probe ([`js_string_concat_value`]'s memoizable +/// gate) must not memoize off a header-lying prefix either: `prefix_u16 == +/// prefix_blen` is a necessary pre-filter, not the ASCII predicate — the +/// `bytes_all_ascii` scan after it is what actually decides. +#[test] +fn concat_memo_declines_a_prefix_whose_header_lies_about_being_ascii() { + let _lock = crate::gc::global_side_table_test_lock(); + crate::string::concat::test_clear_concat_memo(); + crate::string::concat::test_reset_memo_governor(); + + let malformed: &[u8] = &[0xC3]; + let prefix = js_string_from_bytes(malformed.as_ptr(), malformed.len() as u32); + unsafe { + assert_eq!( + (*prefix).utf16_len, + (*prefix).byte_len, + "premise: header must (wrongly) report equal lengths" + ); + } + // Same 3-call shape as `concat_memo_returns_one_object_for_equal_results`: + // the doorkeeper admits a result only on its SECOND sighting, so a + // 2-call probe cannot distinguish "declined outright" from "memoizable, + // just not admitted yet" — the second and third calls are the pair that + // would share identity if this prefix were (wrongly) memoized. + let _first = crate::string::js_string_concat_value(prefix, 1.0); + let second = crate::string::js_string_concat_value(prefix, 1.0); + let third = crate::string::js_string_concat_value(prefix, 1.0); + assert_ne!( + second as usize, third as usize, + "a header-lying malformed prefix must not be memoized" + ); +} + +/// The observable divergence the (now-fixed) header-trick bug produced: +/// concatenating a malformed operand with an ordinary ASCII string, on +/// either side, must come out `isWellFormed() === false` — the same answer +/// [`js_string_concat`] (the general, always-scanning path) gives — instead +/// of silently taking the ASCII fast path and reporting well-formed. +#[test] +fn concat_box_reports_not_well_formed_for_a_malformed_operand_either_side() { + let heap_bytes = |b: &[u8]| { + let p = js_string_from_bytes(b.as_ptr(), b.len() as u32); + f64::from_bits(crate::value::JSValue::string_ptr(p).bits()) + }; + let heap_str = |s: &str| heap_bytes(s.as_bytes()); + + for malformed in [&[0xC3u8][..], &[0xF0u8, 0x41][..]] { + for (l, r, order) in [ + (heap_bytes(malformed), heap_str("hello"), "malformed+ascii"), + (heap_str("hello"), heap_bytes(malformed), "ascii+malformed"), + ] { + let result = js_string_concat_box(l, r); + let jsval = crate::value::JSValue::from_bits(result.to_bits()); + assert!( + jsval.is_string(), + "{malformed:?} {order}: both operands are real strings, \ + concat must not fall through to the dynamic-add arm" + ); + let ptr = jsval.as_string_ptr(); + assert!( + !ptr.is_null(), + "{malformed:?} {order}: empty-string sentinel unexpected here" + ); + let well_formed = crate::value::js_is_truthy(js_string_is_well_formed(ptr)); + assert_eq!( + well_formed, 0, + "{malformed:?} {order}: concat result must report isWellFormed() === false" + ); + } + } +} + /// #9391: the memo must stop PROBING when it stops paying. /// /// `bench_gc_pressure` builds half a million distinct `"item_" + i` strings. diff --git a/test-files/test_gap_string_concat_memo_ascii_header.ts b/test-files/test_gap_string_concat_memo_ascii_header.ts new file mode 100644 index 0000000000..92d35f057f --- /dev/null +++ b/test-files/test_gap_string_concat_memo_ascii_header.ts @@ -0,0 +1,186 @@ +// Coverage for the string-concat perf fix (perry-runtime/src/string/concat.rs +// + string/mod.rs): the ASCII-ness of a concat operand is now read off the +// StringHeader (`utf16_len == byte_len`) instead of re-scanning bytes, the +// short-concat memo probe hashes/looks-up two operand slices directly +// instead of assembling them into a scratch buffer first, and the memo +// governor's minimum-hit-rate floor changed. None of that may change any +// observable result: every case here is compared byte-for-byte against +// `node --experimental-strip-types`. +// +// Values are built from runtime state (array/loop indices), never folded to +// a compile-time constant, so codegen must actually reach the runtime concat +// paths under test. + +function codes(s: string): string { + let out = ""; + for (let i = 0; i < s.length; i++) out += (i ? "+" : "") + s.charCodeAt(i).toString(16); + return out; +} + +// --------------------------------------------------------------------------- +// 1. ASCII string+string concat across the SSO (5) and memo (12) byte +// ceilings — exercises concat_byte_parts's SSO fast path, memo path, and +// heap path in one sweep. +// --------------------------------------------------------------------------- +const lensA = [0, 0, 2, 3, 6, 6, 10, 36]; +const lensB = [0, 1, 3, 3, 6, 7, 10, 37]; +for (let i = 0; i < lensA.length; i++) { + const a = "x".repeat(lensA[i]); + const b = "y".repeat(lensB[i]); + const r = a + b; + console.log("ss", lensA[i], lensB[i], r.length, r); +} + +// Same boundary set, but string+number (js_string_concat_value / +// js_value_concat_string) and number+string, so the "prefix" + i arm's +// memoizable gate (also touched by this fix) is covered too. +const prefixLens = [0, 1, 4, 5, 6, 11, 12, 13, 20]; +for (let i = 0; i < prefixLens.length; i++) { + const prefix = "p".repeat(prefixLens[i]); + const withNum = prefix + i; + const numWith = i + prefix; + console.log("sn", prefixLens[i], withNum.length, withNum, numWith.length, numWith); +} + +// --------------------------------------------------------------------------- +// 2. Non-ASCII, valid (well-formed) UTF-16 — 2-byte, 3-byte and 4-byte +// (astral, via a literal, not a joined surrogate pair) UTF-8 operands. +// These print directly: valid Unicode encodes identically to UTF-8 in +// both engines, so a byte-for-byte diff is a meaningful check on its own. +// --------------------------------------------------------------------------- +const twoByte = "é"; // U+00E9, 2 UTF-8 bytes, 1 UTF-16 unit +const threeByte = "€"; // U+20AC, 3 UTF-8 bytes, 1 UTF-16 unit +const fourByte = "😀"; // U+1F600, 4 UTF-8 bytes, 2 UTF-16 units (already a pair) +const nonAsciiCases: [string, string][] = [ + ["2b+ascii", twoByte + "ab"], + ["ascii+2b", "ab" + twoByte], + ["2b+2b", twoByte + twoByte], + ["3b+ascii", threeByte + "ab"], + ["ascii+3b", "ab" + threeByte], + ["3b+3b", threeByte + threeByte], + ["4b+ascii", fourByte + "ab"], + ["ascii+4b", "ab" + fourByte], + ["4b+4b", fourByte + fourByte], + ["mixed", "a" + twoByte + "b" + threeByte + "c" + fourByte + "d"], +]; +for (const [name, s] of nonAsciiCases) { + console.log("na", name, s.length, s, s.isWellFormed()); +} + +// string+number and number+string with a non-ASCII prefix/suffix, to hit +// js_string_concat_value / js_value_concat_string's non-ASCII path. +for (let i = 0; i < 3; i++) { + const withNum = threeByte + i; + const numWith = i + fourByte; + console.log("nan", i, withNum.length, withNum, numWith.length, numWith); +} + +// --------------------------------------------------------------------------- +// 3. Lone surrogates and a surrogate pair formed ACROSS the join boundary. +// Raw lone-surrogate content is reported via charCodeAt (codes()) or +// JSON.stringify — both are byte-safe (JSON.stringify escapes an +// unpaired surrogate as \uXXXX rather than emitting it raw), matching +// the pattern used elsewhere in this suite (#9431). A properly merged +// astral pair is well-formed Unicode and is printed directly. +// --------------------------------------------------------------------------- +const hi = "\uD83D"; // lone high surrogate +const lo = "\uDE00"; // lone low surrogate — hi+lo is exactly 😀 (U+1F600) + +// Pair formed directly across the join boundary: canonicalize_surrogate_pairs +// must merge it, so this is well-formed and safe to print raw. +const paired = hi + lo; +console.log("pair-direct", paired.length, paired, paired.isWellFormed(), paired.codePointAt(0)); + +// Reverse order never forms a pair (low-before-high is not a valid pair) — +// must remain two lone surrogates. +const reversed = lo + hi; +console.log( + "pair-reversed", + reversed.length, + codes(reversed), + reversed.isWellFormed(), + JSON.stringify(reversed), +); + +// A pair split across TWO separate concatenations, then joined by a THIRD: +// "a" + hi built first, lo + "b" built second, then those two results +// concatenated — the pair only becomes adjacent at the last join. +const left = "a" + hi; +const right = lo + "b"; +const rejoined = left + right; +console.log( + "pair-split-rejoin", + left.length, + right.length, + rejoined.length, + rejoined, + rejoined.isWellFormed(), + rejoined.codePointAt(1), +); + +// A lone surrogate with ASCII on both sides never forms a pair — stays lone, +// flag preserved through the concat. +const loneMid = "x" + hi + "y"; +console.log("lone-mid", loneMid.length, codes(loneMid), loneMid.isWellFormed(), JSON.stringify(loneMid)); + +// Two highs in a row: no valid pair (high+high is not low-after-high). +const twoHighs = hi + hi; +console.log("two-highs", twoHighs.length, codes(twoHighs), twoHighs.isWellFormed()); + +// --------------------------------------------------------------------------- +// 4. Empty operands on both sides of both concat forms. +// --------------------------------------------------------------------------- +console.log("empty-both", ("" + "").length, JSON.stringify("" + "")); +console.log("empty-left", ("" + "z").length, "" + "z"); +console.log("empty-right", ("z" + "").length, "z" + ""); +console.log("empty-num", ("" + 0).length, "" + 0, (0 + "").length, 0 + ""); + +// --------------------------------------------------------------------------- +// 5. Repeated identical concat results — forces the memo doorkeeper's +// "seen twice" admission and then real hits, and checks `===` identity +// across independently-built equal results (the memo must never change +// observable semantics: value equality is unaffected either way, but a +// hash-collision or admission bug would surface as a wrong `.length` or +// a `false` here). +// --------------------------------------------------------------------------- +let memoFailures = 0; +const memoResults: string[] = []; +for (let i = 0; i < 40; i++) { + // Same content, two different operand splits — "ab" + "cdef" and + // "abc" + "def" both yield "abcdef". + const viaSplitA = "ab" + "cdef".slice(0); + const viaSplitB = "abc".slice(0) + "def"; + if (viaSplitA !== viaSplitB) memoFailures++; + if (viaSplitA.length !== 6) memoFailures++; + memoResults.push(viaSplitA); +} +for (let i = 1; i < memoResults.length; i++) { + if (memoResults[i] !== memoResults[0]) memoFailures++; +} +console.log("memo-repeat-failures", memoFailures, memoResults.length, memoResults[0]); + +// A heap-forced (>SSO, <=memo-ceiling) equal pair built two different ways, +// repeated enough to admit, then compared for identity and content. +let memoHeapFailures = 0; +for (let i = 0; i < 40; i++) { + const a = "field_" + "ab".slice(0); // "field_ab", 8 bytes + const b = "field" + "_ab".slice(0); + if (a !== b || a.length !== 8 || a !== "field_ab") memoHeapFailures++; +} +console.log("memo-heap-repeat-failures", memoHeapFailures); + +// The "prefix" + i shape repeated with a REPEATED i, so the SAME result +// recurs (as opposed to section 1's sweep, which never repeats a value). +let memoNumFailures = 0; +const memoNumResults: string[] = []; +for (let rep = 0; rep < 30; rep++) { + const k = "row_" + 7; + if (k.length !== 5 || k !== "row_7") memoNumFailures++; + memoNumResults.push(k); +} +for (let i = 1; i < memoNumResults.length; i++) { + if (memoNumResults[i] !== memoNumResults[0]) memoNumFailures++; +} +console.log("memo-num-repeat-failures", memoNumFailures, memoNumResults[0]); + +console.log("done"); From 8ce4d9308f31b15c442b8a1e62897f9fe0dbdcc9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Fri, 18 Sep 2026 23:39:51 +0200 Subject: [PATCH 2/2] changelog: key fragment to #10672 --- ...o-ascii-header.md => 10672-string-concat-memo-ascii-header.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changelog.d/{string-concat-memo-ascii-header.md => 10672-string-concat-memo-ascii-header.md} (100%) diff --git a/changelog.d/string-concat-memo-ascii-header.md b/changelog.d/10672-string-concat-memo-ascii-header.md similarity index 100% rename from changelog.d/string-concat-memo-ascii-header.md rename to changelog.d/10672-string-concat-memo-ascii-header.md