diff --git a/changelog.d/keystroke-segmenter-shared-shape.md b/changelog.d/keystroke-segmenter-shared-shape.md new file mode 100644 index 0000000000..2394b1b422 --- /dev/null +++ b/changelog.d/keystroke-segmenter-shared-shape.md @@ -0,0 +1,15 @@ +### Fixed + +- `Intl.Segmenter` segment records now share one shape. They were built + property-by-property with `set_field`, which allocates a fresh key string + per call and clones the object's key list before each write, so every record + got its own keys array — and, because the shape table is keyed on that + array's address, its own ShapeId. That made every read of `.segment` / + `.index` / `.input` a guaranteed inline-cache miss and added one descriptor + to the shape table per record. Grapheme-aware text measurement segments + every string a terminal UI renders: one 400-character reply in the compiled + claude-code TUI produces 175,797 segment records, and `PERRY_IC_DIAG` + attributes 175,797 of that turn's 2,589,696 IC misses to the `.segment` read + site alone. The two record shapes (with and without `isWordLike`) now share + one `GC_FLAG_SHAPE_SHARED` keys array each, built at most twice per thread — + the same construction #7564 used for `{ value, done }` iterator results. diff --git a/crates/perry-runtime/src/gc/mod.rs b/crates/perry-runtime/src/gc/mod.rs index 2bbc530e0a..365f4b7ba1 100644 --- a/crates/perry-runtime/src/gc/mod.rs +++ b/crates/perry-runtime/src/gc/mod.rs @@ -1099,6 +1099,9 @@ pub fn gc_init() { // REWRITES: an evacuating collection moves them like any other array, and // the thread-local slot is the only place the new address can be recorded. reg_scanner!(crate::iter_result::scan_iter_result_keys_roots_mut); + // Same shape as the line above: the shared keys arrays behind + // `Intl.Segmenter`'s segment records are referenced only by this cache. + reg_scanner!(crate::intl::segmenter::scan_segment_record_keys_roots_mut); reg_scanner!(small_int_cache_mutable_root_scanner); reg_scanner!(concat_memo_mutable_root_scanner); reg_scanner!(crate::builtins::scan_console_log_singleton_roots_mut); diff --git a/crates/perry-runtime/src/gc/tests/runtime_roots.rs b/crates/perry-runtime/src/gc/tests/runtime_roots.rs index fc67e7a3bf..b398f80ab7 100644 --- a/crates/perry-runtime/src/gc/tests/runtime_roots.rs +++ b/crates/perry-runtime/src/gc/tests/runtime_roots.rs @@ -9,6 +9,7 @@ mod generator_attach_prototype; mod hook_dispatch_handles; mod interned_string_caches; mod iter_result_keys; +mod segment_record_keys; mod json_shape_template; mod native_module_name; mod old_defrag_contract; diff --git a/crates/perry-runtime/src/gc/tests/runtime_roots/segment_record_keys.rs b/crates/perry-runtime/src/gc/tests/runtime_roots/segment_record_keys.rs new file mode 100644 index 0000000000..44b4b3471c --- /dev/null +++ b/crates/perry-runtime/src/gc/tests/runtime_roots/segment_record_keys.rs @@ -0,0 +1,179 @@ +//! The per-thread `{ segment, index, input(, isWordLike) }` keys arrays every +//! `Intl.Segmenter` segment record shares. +//! +//! Exactly the shape `iter_result_keys.rs` guards, for the same reason and +//! with the same failure mode: `scripts/gc_root_dominance_check.py` reads +//! emitted LLVM IR, so a thread-local holding a `*mut ArrayHeader` into the +//! heap is structurally invisible to it, and the runtime scanner is the only +//! thing between this cache and a use-after-free. Being a cache rather than a +//! register, it would go bad at collection #0 and stay bad — corrupting every +//! later segment record on the thread instead of failing intermittently. +//! +//! Marking alone is not enough: a marked but un-rewritten slot still hands out +//! a pre-move address after a copying minor, so there is a MARK test and a +//! REWRITE test, plus a registration check (a scanner a test can call directly +//! is a no-op in production until `gc_init` names it). + +use super::*; +use crate::array::ArrayHeader; +use crate::intl::segmenter::{SegmentRecordShape, SEGMENT_RECORD_SHAPE_LIST}; + +/// Empties the cache on entry and exit and pins the GC triggers for the body, +/// exactly as `IterResultKeysGuard` does. +struct SegmentRecordKeysGuard { + _triggers: GcTriggerThresholdTestGuard, +} + +impl SegmentRecordKeysGuard { + fn new() -> Self { + let triggers = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); + crate::intl::segmenter::reset_shared_segment_keys_for_test(); + Self { + _triggers: triggers, + } + } +} + +impl Drop for SegmentRecordKeysGuard { + fn drop(&mut self) { + crate::intl::segmenter::reset_shared_segment_keys_for_test(); + } +} + +fn evacuate_array(from: *mut ArrayHeader) -> *mut ArrayHeader { + let to = crate::arena::arena_alloc_gc_old(64, 8, GC_TYPE_ARRAY); + unsafe { + set_forwarding_address(header_from_user_ptr(from as *const u8), to); + } + to as *mut ArrayHeader +} + +/// MARK. The cache is the ONLY reference to these arrays — the records that +/// point at them are short-lived while the cache outlives them — so an +/// unmarked slot is a swept slot, and every later segment record installs a +/// freed keys array as its shape. +#[test] +fn segment_record_keys_cache_is_marked_by_the_collector() { + let _guard = SegmentRecordKeysGuard::new(); + clear_marks(); + clear_mark_seeds(); + + let arrays = crate::intl::segmenter::populate_shared_segment_keys_for_test(); + let valid_ptrs = build_valid_pointer_set(); + + crate::intl::segmenter::scan_segment_record_keys_roots_mut(&mut RuntimeRootVisitor::for_mark( + &valid_ptrs, + )); + + for (i, arr) in arrays.iter().enumerate() { + assert!(!arr.is_null(), "keys slot {i} should have been populated"); + assert_marked_user_ptr( + *arr as usize, + &format!("segment-record keys array {i} (nothing else references it)"), + ); + } + + clear_marks(); + clear_mark_seeds(); +} + +/// REWRITE, every slot. Marking keeps the array alive; only the rewrite makes +/// the slot name the surviving copy. +#[test] +fn every_segment_record_keys_slot_is_rewritten_by_the_collector() { + let _guard = SegmentRecordKeysGuard::new(); + + let before = crate::intl::segmenter::populate_shared_segment_keys_for_test(); + let valid_ptrs = build_valid_pointer_set(); + let expected: Vec<*mut ArrayHeader> = before.iter().map(|p| evacuate_array(*p)).collect(); + + crate::intl::segmenter::scan_segment_record_keys_roots_mut( + &mut RuntimeRootVisitor::for_rewrite(&valid_ptrs), + ); + + for (i, shape) in SEGMENT_RECORD_SHAPE_LIST.into_iter().enumerate() { + assert_eq!( + crate::intl::segmenter::shared_segment_keys_peek_for_test(shape), + expected[i], + "segment-record keys slot {i} ({shape:?}) must be rewritten to the \ + relocated array. A marked-but-stale slot goes bad at collection #0 \ + and then EVERY segment record on this thread installs a from-space \ + keys array as its shape." + ); + } +} + +/// An empty cache is the state between process start and the first +/// `Intl.Segmenter` use, and every cycle in that window scans it. A null slot +/// must be skipped, not treated as an address. +#[test] +fn scanning_an_empty_segment_record_keys_cache_is_a_no_op() { + let _guard = SegmentRecordKeysGuard::new(); + let valid_ptrs = build_valid_pointer_set(); + + crate::intl::segmenter::scan_segment_record_keys_roots_mut( + &mut RuntimeRootVisitor::for_rewrite(&valid_ptrs), + ); + + for shape in SEGMENT_RECORD_SHAPE_LIST { + assert!( + crate::intl::segmenter::shared_segment_keys_peek_for_test(shape).is_null(), + "scanning must not populate the {shape:?} keys slot" + ); + } +} + +/// …and it must actually be REGISTERED: an unregistered scanner is a no-op in +/// production, which is precisely the bug this cache would introduce. +#[test] +fn segment_record_keys_scanner_is_registered() { + crate::gc::gc_init(); + let registered = |scanner: MutableRootScanner| { + crate::gc::roots::MUTABLE_ROOT_SCANNERS.with(|scanners| { + scanners + .borrow() + .iter() + .any(|entry| entry.scanner as usize == scanner as usize) + }) + }; + + assert!( + registered( + crate::intl::segmenter::scan_segment_record_keys_roots_mut as MutableRootScanner + ), + "scan_segment_record_keys_roots_mut must be registered in gc_init — unregistered, \ + the shared segment-record keys arrays are swept by the first minor and every later \ + segment record installs a freed array as its shape" + ); +} + +/// The cache must be STABLE: the second segment record reuses the array the +/// first one built. If it did not, nothing would have been saved — and, +/// because `shape_id_for_keys_ensure` keys the shape table on the array's +/// ADDRESS, a fresh array per record is also a fresh shape id per record, +/// which is what made every read of `.segment` an inline-cache miss. +#[test] +fn segment_record_keys_are_built_once_per_shape() { + let _guard = SegmentRecordKeysGuard::new(); + + let first = crate::intl::segmenter::populate_shared_segment_keys_for_test(); + let second = crate::intl::segmenter::populate_shared_segment_keys_for_test(); + + assert_eq!( + first, second, + "the shared keys arrays must be built once per thread per shape; rebuilding them \ + per record restores both the per-record allocations and the one-shape-id-per-record \ + inline-cache miss" + ); + assert_eq!( + first.len(), + SEGMENT_RECORD_SHAPE_LIST.len(), + "every segment-record shape needs its own shared array" + ); + assert_ne!( + first[SegmentRecordShape::Plain as usize], + first[SegmentRecordShape::WordLike as usize], + "the two shapes must not share one array: `isWordLike` is present only for \ + word granularity" + ); +} diff --git a/crates/perry-runtime/src/intl.rs b/crates/perry-runtime/src/intl.rs index ae6c46176d..88084c0db4 100644 --- a/crates/perry-runtime/src/intl.rs +++ b/crates/perry-runtime/src/intl.rs @@ -63,7 +63,7 @@ mod number_format_options; mod numbering_system; use numbering_system::{is_well_formed_numbering_system, resolve_numbering_system}; mod canon_aliases; -mod segmenter; +pub(crate) mod segmenter; use canon_aliases::canonicalize_unicode_extension_types; pub(crate) use date_collator::{ diff --git a/crates/perry-runtime/src/intl/segmenter.rs b/crates/perry-runtime/src/intl/segmenter.rs index 6ba71870ec..f0f8ec7d08 100644 --- a/crates/perry-runtime/src/intl/segmenter.rs +++ b/crates/perry-runtime/src/intl/segmenter.rs @@ -65,21 +65,195 @@ unsafe fn segmenter_input_text(ptr: *const StringHeader) -> String { text } +/// The two shapes a segment record can have. ECMA-402 18.5.1 attaches +/// `isWordLike` only to word granularity, so there are exactly two. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub(crate) enum SegmentRecordShape { + /// `{ segment, index, input }` + Plain = 0, + /// `{ segment, index, input, isWordLike }` + WordLike = 1, +} + +/// Every shape, for the root-scanner tests. +#[cfg(test)] +pub(crate) const SEGMENT_RECORD_SHAPE_LIST: [SegmentRecordShape; SEGMENT_RECORD_SHAPES] = + [SegmentRecordShape::Plain, SegmentRecordShape::WordLike]; + +const SEGMENT_RECORD_SHAPES: usize = 2; + +crate::perry_thread_local! { + /// Per-thread shared keys arrays for segment records, indexed by + /// [`SegmentRecordShape`]. + /// + /// Same construction, and the same reason, as `iter_result`'s + /// `ITER_RESULT_KEYS` (#7564): `set_field`-by-name clones an object's key + /// list before writing, so building a record property-by-property gave + /// EVERY record its own keys array — a fresh array address per record, + /// therefore a fresh ShapeId per record (`shape_id_for_keys_ensure` keys + /// the shape table on the array's address), therefore a guaranteed inline + /// -cache miss on every `.segment` / `.index` / `.input` read and one more + /// descriptor in the shape table per segment. On the claude-code TUI, + /// whose text measurement segments every string it renders, that was + /// 175,797 misses on `.segment` alone in one 400-character reply + /// (`PERRY_IC_DIAG`). One shared array per shape means one ShapeId for + /// every segment record in the program. + /// + /// Per-thread and not process-global for the same reason the intern table + /// is: each `perry/thread` worker has its own arena. + /// + /// GC-visible through [`scan_segment_record_keys_roots_mut`], which both + /// MARKS (nothing else references these arrays; the records that use them + /// are short-lived and the cache outlives them) and REWRITES them. + static SEGMENT_RECORD_KEYS: std::cell::UnsafeCell<[*mut crate::array::ArrayHeader; SEGMENT_RECORD_SHAPES]> = + std::cell::UnsafeCell::new([std::ptr::null_mut(); SEGMENT_RECORD_SHAPES]); +} + +#[inline(always)] +fn cached_segment_keys(shape: SegmentRecordShape) -> *mut crate::array::ArrayHeader { + SEGMENT_RECORD_KEYS.with(|c| unsafe { (*c.get())[shape as usize] }) +} + +/// NaN-boxed bits of an interned constant property name. +#[inline] +fn interned_key_bits(bytes: &[u8]) -> u64 { + JSValue::string_ptr(crate::string::intern_ascii_literal(bytes) as *mut _).bits() +} + +/// Build this thread's shared keys array for `shape`, if it has none. +/// +/// Cold and at most twice per thread for the program's lifetime, so it is +/// written for obviousness rather than speed: every intermediate is rooted +/// across every allocation that follows it. Interning makes the names +/// pointer-identical to the `"segment"` / `"index"` / `"input"` the READ side +/// hashes, and gives them a second independent root in the intern table. +#[cold] +unsafe fn build_shared_segment_keys(shape: SegmentRecordShape) { + const NAMES: [&[u8]; 4] = [b"segment", b"index", b"input", b"isWordLike"]; + let n = match shape { + SegmentRecordShape::Plain => 3usize, + SegmentRecordShape::WordLike => 4usize, + }; + + let scope = crate::gc::RuntimeHandleScope::new(); + let keys_h = scope.root_raw_mut_ptr(js_array_alloc(n as u32)); + + // Each intern ALLOCATES on a first-call-per-thread miss, so the array's + // address is taken back out of its handle ACROSS them. + let mut handles = Vec::with_capacity(n); + for name in NAMES.iter().take(n) { + let (bits, _) = keys_h.across_mut::(|| interned_key_bits(name)); + handles.push(scope.root_nanbox_u64(bits)); + } + let keys = keys_h.get_raw_mut_ptr::(); + + (*keys).length = n as u32; + for (i, h) in handles.iter().enumerate() { + crate::array::store_array_slot(keys, i, h.get_nanbox_u64()); + } + crate::array::rebuild_array_layout_exact(keys); + + // Copy-on-write marker. Without it, `record.extra = 1` on ONE record would + // append to the array every other record shares. + crate::gc::mark_shape_shared(keys as *mut u8); + + SEGMENT_RECORD_KEYS.with(|c| (*c.get())[shape as usize] = keys); + crate::gc::runtime_write_barrier_root_raw_ptr(keys); +} + +/// GC root scanner for the shared segment-record keys arrays. See +/// `SEGMENT_RECORD_KEYS`. +pub fn scan_segment_record_keys_roots_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) { + SEGMENT_RECORD_KEYS.with(|c| unsafe { + for slot in (*c.get()).iter_mut() { + visitor.visit_raw_mut_ptr_slot(slot); + } + }); +} + +/// Drop the cached arrays. The unit-test harness resets arenas between tests +/// while thread-locals persist, which would leave these pointing into a +/// deallocated block. +#[cfg(test)] +pub(crate) fn populate_shared_segment_keys_for_test() -> Vec<*mut crate::array::ArrayHeader> { + for shape in SEGMENT_RECORD_SHAPE_LIST { + if cached_segment_keys(shape).is_null() { + unsafe { build_shared_segment_keys(shape) }; + } + } + SEGMENT_RECORD_SHAPE_LIST + .iter() + .map(|shape| cached_segment_keys(*shape)) + .collect() +} + +#[cfg(test)] +pub(crate) fn shared_segment_keys_peek_for_test( + shape: SegmentRecordShape, +) -> *mut crate::array::ArrayHeader { + cached_segment_keys(shape) +} + +#[cfg(test)] +pub(crate) fn reset_shared_segment_keys_for_test() { + SEGMENT_RECORD_KEYS.with(|c| unsafe { + (*c.get()) = [std::ptr::null_mut(); SEGMENT_RECORD_SHAPES]; + }); +} + pub(crate) fn make_segment_record( segment_value: f64, index: u32, input_value: f64, word_like: Option, ) -> f64 { - let obj = js_object_alloc(0, 4); - set_field(obj, "segment", segment_value); - // `index` is a plain Number (UTF-16 code-unit offset into the input). - set_field(obj, "index", index as f64); - set_field(obj, "input", input_value); - if let Some(word_like) = word_like { - set_field(obj, "isWordLike", bool_value(word_like)); + let shape = if word_like.is_some() { + SegmentRecordShape::WordLike + } else { + SegmentRecordShape::Plain + }; + let n = match shape { + SegmentRecordShape::Plain => 3usize, + SegmentRecordShape::WordLike => 4usize, + }; + unsafe { + let scope = crate::gc::RuntimeHandleScope::new(); + // Both caller-supplied values are heap pointers; the allocations below + // can collect and move them. + let segment_h = scope.root_nanbox_f64(segment_value); + let input_h = scope.root_nanbox_f64(input_value); + + // Fill the keys cache BEFORE the record exists, so its cold + // allocations cannot invalidate a pointer already being held. + if cached_segment_keys(shape).is_null() { + build_shared_segment_keys(shape); + } + + let obj_h = scope.root_nanbox_f64(js_nanbox_pointer( + js_object_alloc(0, n as u32) as i64, + )); + // Everything below re-reads through storage the collector rewrites: + // the record from its handle, the keys array from the scanned + // thread-local. No address here predates the allocation above. + let obj = || crate::js_nanbox_get_pointer(obj_h.get_nanbox_f64()) as *mut ObjectHeader; + crate::object::js_object_set_keys(obj(), cached_segment_keys(shape)); + crate::object::js_object_set_field( + obj(), + 0, + JSValue::from_bits(segment_h.get_nanbox_f64().to_bits()), + ); + // `index` is a plain Number (UTF-16 code-unit offset into the input). + crate::object::js_object_set_field(obj(), 1, JSValue::number(index as f64)); + crate::object::js_object_set_field( + obj(), + 2, + JSValue::from_bits(input_h.get_nanbox_f64().to_bits()), + ); + if let Some(word_like) = word_like { + crate::object::js_object_set_field(obj(), 3, JSValue::bool(word_like)); + } + js_nanbox_pointer(obj() as i64) } - js_nanbox_pointer(obj as i64) } /// Build the segment list for `input` under `granularity`. The backing array