diff --git a/changelog.d/10570-dynamic-key-receiver-requeries.md b/changelog.d/10570-dynamic-key-receiver-requeries.md new file mode 100644 index 0000000000..1fd2514dbe --- /dev/null +++ b/changelog.d/10570-dynamic-key-receiver-requeries.md @@ -0,0 +1,33 @@ +Stop re-deriving the receiver on every dynamic-key property read. `o[k]` cost +674 instructions per access — in a loop, with a constant key, on a +two-property object — against node's ~4.5, and it did not amortize. A +symbol-resolved profile showed three per-access re-derivations, the same shape +as the array-push work: 674.1 -> 546.0, **-19.0%**. + +`keys_find_slot_by_bytes` ran `clean_arr_ptr` on `descriptor.keys` (16.2% of +the loop) although that field is maintained by the COLLECTOR — +`shapes::scan_shape_table_rekey_mut` writes the forwarded address back into +every descriptor record when the keys array moves, and prunes descriptors whose +keys array died. `try_data_get_bytes` read a thread-local on every access to +ask whether the receiver is `process.env`; a sticky latch means that is touched +only once such an object exists. And `is_anon_shape_class_id` took an `RwLock` +read guard per access (16.1%, the largest frame left) to consult a set written +only from module init; it is now answered from a lock-free open-addressed +mirror, sound because the set is insert-only, and living in the class IMAGE +beside `parent_dense` because `ImageTable` resolves every access through +`current()`. + +A sticky "is the anon-shape set empty?" latch was tried first and measured +ZERO — object literals ARE anon shapes, so the flag is true in essentially +every real program. Caching the resolved slot against the key was dropped: a +cached key pointer can be recycled into a WRONG slot rather than a miss, and +would need its own GC root scanner. + +New fixture `test_gap_dynamic_key_read_paths.ts` covers the receiver shapes and +the mutations that must invalidate what the fast path reads — key added and +deleted after a first read, 64 keys to cross the indexed-lookup threshold, +declared-class instances, prototype-chain reads, accessors, and the +`.constructor`/`getPrototypeOf` verdicts the mirror answers. Byte-identical to +node; two GC-stress seeds with from-space protection and +PERRY_GC_FROMSPACE_SCAN_ABORT=1 ran 2,117 and 2,450 copying minors with +dangling=0 and missing_rewrites=0. diff --git a/crates/perry-runtime/src/object/class_image.rs b/crates/perry-runtime/src/object/class_image.rs index 33e13bce26..50c3a61ae5 100644 --- a/crates/perry-runtime/src/object/class_image.rs +++ b/crates/perry-runtime/src/object/class_image.rs @@ -73,7 +73,7 @@ use crate::fast_hash::{PtrHashMap, PtrHashSet}; use std::cell::OnceCell; use std::collections::HashMap; -use std::sync::atomic::{AtomicU32, Ordering}; +use std::sync::atomic::{AtomicBool, AtomicU32, Ordering}; use std::sync::{Arc, LockResult, OnceLock, RwLock, RwLockReadGuard, RwLockWriteGuard}; use std::thread::ThreadId; @@ -150,6 +150,16 @@ pub struct ClassImageTables { pub(crate) names: RwLock>>, pub(crate) lengths: RwLock>>, pub(crate) anon_shape_class_ids: RwLock>>, + /// Lock-free read mirror of `anon_shape_class_ids`, open-addressed. + /// `0` is an empty slot; `js_register_anon_shape_class_id` rejects id 0, + /// so the sentinel is unambiguous. Per IMAGE, like `parent_dense` — a + /// process-global mirror would answer one image's question out of + /// another's registrations, since `ImageTable` resolves every access + /// through `current()`. + pub(crate) anon_shape_fast: OnceLock>, + /// Set when an insert exhausted its probe run: from then on an empty slot + /// no longer proves absence, so a miss must consult the locked set. + pub(crate) anon_shape_fast_overflow: AtomicBool, } impl ClassImageTables { @@ -176,6 +186,8 @@ impl ClassImageTables { names: RwLock::new(None), lengths: RwLock::new(None), anon_shape_class_ids: RwLock::new(None), + anon_shape_fast: OnceLock::new(), + anon_shape_fast_overflow: AtomicBool::new(false), } } } @@ -335,6 +347,77 @@ pub(crate) fn parent_dense_store(idx: usize, biased_parent: u32) { .store(biased_parent, Ordering::Release); } +/// Slot count of the per-image anon-shape mirror. Power of two. +pub(crate) const ANON_FAST_SLOTS: usize = 4096; +const ANON_FAST_MASK: usize = ANON_FAST_SLOTS - 1; +const ANON_FAST_MAX_PROBE: usize = 8; + +#[inline] +fn anon_fast_index(class_id: u32) -> usize { + // splitmix32 finaliser: anon ids come from a counter, so the low bits + // alone would cluster one module's ids into a single probe run. + let mut z = class_id ^ 0x9E37_79B9; + z = (z ^ (z >> 16)).wrapping_mul(0x85EB_CA6B); + z = (z ^ (z >> 13)).wrapping_mul(0xC2B2_AE35); + ((z ^ (z >> 16)) as usize) & ANON_FAST_MASK +} + +#[inline] +fn anon_fast_table() -> &'static [AtomicU32] { + current() + .anon_shape_fast + .get_or_init(|| (0..ANON_FAST_SLOTS).map(|_| AtomicU32::new(0)).collect()) +} + +/// `Some(verdict)` when the calling image's mirror can answer; `None` when the +/// caller must fall back to the locked set. +#[inline] +pub(crate) fn anon_fast_lookup(class_id: u32) -> Option { + let table = anon_fast_table(); + let mut i = anon_fast_index(class_id); + for _ in 0..ANON_FAST_MAX_PROBE { + let v = table[i].load(Ordering::Acquire); + if v == class_id { + return Some(true); + } + if v == 0 { + return if current().anon_shape_fast_overflow.load(Ordering::Acquire) { + None + } else { + Some(false) + }; + } + i = (i + 1) & ANON_FAST_MASK; + } + None +} + +/// Publish `class_id` into the calling image's mirror. Idempotent. +pub(crate) fn anon_fast_insert(class_id: u32) { + let table = anon_fast_table(); + let mut i = anon_fast_index(class_id); + for _ in 0..ANON_FAST_MAX_PROBE { + let v = table[i].load(Ordering::Acquire); + if v == class_id { + return; + } + if v == 0 + && table[i] + .compare_exchange(0, class_id, Ordering::AcqRel, Ordering::Acquire) + .is_ok() + { + return; + } + if table[i].load(Ordering::Acquire) == class_id { + return; + } + i = (i + 1) & ANON_FAST_MASK; + } + current() + .anon_shape_fast_overflow + .store(true, Ordering::Release); +} + #[cfg(test)] mod tests { use super::*; diff --git a/crates/perry-runtime/src/object/class_registry/class_meta.rs b/crates/perry-runtime/src/object/class_registry/class_meta.rs index cc1692ccd3..62325eb83e 100644 --- a/crates/perry-runtime/src/object/class_registry/class_meta.rs +++ b/crates/perry-runtime/src/object/class_registry/class_meta.rs @@ -590,11 +590,37 @@ pub static ANON_SHAPE_CLASS_IDS: ImageTable>>> = /// Mark `class_id` as a synthetic anon-shape class so `.constructor` /// reads on instances of that class return the global `Object` /// constructor rather than the synthetic class ref. +/// The lock-free read mirror lives in the class IMAGE +/// (`class_image::anon_fast_lookup` / `anon_fast_insert`), next to +/// `parent_dense`, because `ANON_SHAPE_CLASS_IDS` is an `ImageTable` and every +/// access resolves through `current()`. A process-global mirror would answer +/// one image's question out of another image's registrations. +/// +/// Why a mirror at all: `is_anon_shape_class_id` is asked once per dynamic +/// property read from `native_get::try_data_get_bytes`, and taking an `RwLock` +/// read guard to do it was 16.1% of an `o[k]` loop — the largest single frame +/// left in that profile. A guard is an atomic read-modify-write on a lock word +/// shared by every thread in the image, paid to consult a set written only +/// from module init. +/// +/// A sticky "is the set empty" flag does NOT work here, and that was measured +/// before this was written: object literals ARE anon shapes, so such a flag is +/// true in essentially every real program and the lock is taken anyway. +/// +/// The set is INSERT-ONLY — the registrar below only ever calls `insert`, and +/// nothing removes — which is what makes an open-addressed mirror sound with +/// no reclamation scheme: an entry, once published, stays valid for the life +/// of the image. #[no_mangle] pub unsafe extern "C" fn js_register_anon_shape_class_id(class_id: u32) { if class_id == 0 { return; } + // Mirror FIRST: a reader that sees the id here is right, and one that does + // not yet see it falls through to the locked set below, which the write + // guard is about to update. The reverse order would let a reader miss in + // both. + super::super::class_image::anon_fast_insert(class_id); let mut guard = ANON_SHAPE_CLASS_IDS.write().unwrap(); if guard.is_none() { *guard = Some(new_ptr_hash_set()); @@ -629,6 +655,9 @@ pub fn is_anon_shape_class_id(class_id: u32) -> bool { if class_id == 0 { return false; } + if let Some(verdict) = super::super::class_image::anon_fast_lookup(class_id) { + return verdict; + } if let Ok(guard) = ANON_SHAPE_CLASS_IDS.read() { if let Some(set) = guard.as_ref() { return set.contains(&class_id); diff --git a/crates/perry-runtime/src/object/keys_lookup.rs b/crates/perry-runtime/src/object/keys_lookup.rs index 50e827606c..fcff25538c 100644 --- a/crates/perry-runtime/src/object/keys_lookup.rs +++ b/crates/perry-runtime/src/object/keys_lookup.rs @@ -111,6 +111,70 @@ pub(crate) unsafe fn keys_find_slot_by_bytes( None } +/// [`keys_array_dense_slots`] for a keys array the caller read out of a LIVE +/// `ShapeDescriptor`. +/// +/// `descriptor.keys` is maintained by the COLLECTOR. When the keys array +/// moves, `shapes::scan_shape_table_rekey_mut` writes the forwarded address +/// back into every descriptor record in that family — +/// `unsafe { (*record).keys = addr as u64 }` — and a descriptor whose keys +/// array died is pruned in the same pass (`shape_keys_address_is_recycled`). +/// So the pointer read out of a live descriptor already IS the resolved live +/// head, and `clean_arr_ptr` on it re-derives a guarantee the collector has +/// already made. +/// +/// Measured: `keys_array_dense_slots` was 16.2% of an `o[k]` read loop, and +/// `clean_arr_ptr` is what it spends that on. +/// +/// # Safety +/// +/// `keys` must be `ShapeDescriptor::keys` from a descriptor read on this same +/// straight-line path, with no allocation or safepoint since that read. +#[inline] +pub(crate) unsafe fn keys_array_dense_slots_resolved( + keys: *const crate::array::ArrayHeader, +) -> (*const f64, usize) { + if keys.is_null() { + return (std::ptr::null(), 0); + } + let len = (*keys).length.min((*keys).capacity) as usize; + (crate::array::array_elements_ptr(keys) as *const f64, len) +} + +/// [`keys_find_slot_by_bytes`] for a keys array obtained from a live +/// descriptor — see [`keys_array_dense_slots_resolved`] for why the receiver +/// needs no second resolution. +/// +/// # Safety +/// +/// As [`keys_array_dense_slots_resolved`]. +pub(crate) unsafe fn keys_find_slot_by_bytes_resolved( + keys: *const crate::array::ArrayHeader, + key_count: u32, + key_bytes: &[u8], +) -> Option { + if key_count >= KEYS_INDEX_THRESHOLD { + // The indexed path owns its own receiver handling; hand it the + // unresolved entry so its behaviour is bit-for-bit what it was. + return keys_find_slot_by_bytes(keys, key_count, key_bytes); + } + let (slots, slot_len) = keys_array_dense_slots_resolved(keys); + if slots.is_null() { + return None; + } + let n = (key_count as usize).min(slot_len); + let mut sso = [0u8; crate::value::SHORT_STRING_MAX_LEN]; + for i in 0..n { + let v = crate::JSValue::from_bits((*slots.add(i)).to_bits()); + if let Some(stored) = crate::string::js_string_key_bytes(v, &mut sso) { + if stored == key_bytes { + return Some(i as u32); + } + } + } + None +} + /// [`keys_find_slot_by_bytes`] for a key held as a `StringHeader`. pub(crate) unsafe fn keys_find_slot_by_key_ptr( keys: *const crate::array::ArrayHeader, diff --git a/crates/perry-runtime/src/object/native_get.rs b/crates/perry-runtime/src/object/native_get.rs index c674092e5b..072b8eb435 100644 --- a/crates/perry-runtime/src/object/native_get.rs +++ b/crates/perry-runtime/src/object/native_get.rs @@ -108,7 +108,15 @@ pub(crate) unsafe fn try_data_get_bytes(receiver: JSValue, key: &[u8]) -> Option let keys = descriptor.keys as usize as *const crate::array::ArrayHeader; if !keys.is_null() { if let Some(slot) = - super::keys_find_slot_by_bytes(keys, descriptor.logical_key_count, key) + // `keys` came straight out of `descriptor` above with no + // allocation in between, and the collector maintains that + // field — so the resolved entry skips a `clean_arr_ptr` + // that re-derives it. + super::keys_find_slot_by_bytes_resolved( + keys, + descriptor.logical_key_count, + key, + ) { let value = super::field_get_set::object_field_at_with_live( object, diff --git a/crates/perry-runtime/src/process/env_misc.rs b/crates/perry-runtime/src/process/env_misc.rs index e3023f35b9..681121e159 100644 --- a/crates/perry-runtime/src/process/env_misc.rs +++ b/crates/perry-runtime/src/process/env_misc.rs @@ -1317,7 +1317,22 @@ pub fn is_process_env_object(value: f64) -> bool { /// /// The pointer form of [`is_process_env_object`], for call sites that have /// already unboxed the target (`Object.assign`'s write funnel). +/// Sticky: has a `process.env` object ever been materialised in this process? +/// See [`is_process_env_ptr`]. +static ANY_PROCESS_ENV_OBJECT: std::sync::atomic::AtomicBool = + std::sync::atomic::AtomicBool::new(false); + pub fn is_process_env_ptr(addr: usize) -> bool { + // Asked once per dynamic property read, from `native_get::try_data_get_bytes`. + // Reading `CACHED_ENV` is a thread-local access — `_tlv_get_addr` was 14.1% + // of an `o[k]` loop with half of it from here — and in a program that never + // materialises `process.env` the answer is always `false`. The latch is set + // the moment such an object is created, so the TLS is touched only once one + // exists. Monotone and never cleared: a stale `true` costs the old TLS read, + // never a wrong answer. `AtomicBool` holds no heap pointer, so not a GC root. + if !ANY_PROCESS_ENV_OBJECT.load(std::sync::atomic::Ordering::Acquire) { + return false; + } let cached = CACHED_ENV.with(|c| c.get()); if cached == 0.0 { return false; @@ -1429,6 +1444,7 @@ fn js_process_env_impl() -> f64 { } let boxed = f64::from_bits(JSValue::pointer(obj as *const u8).bits()); CACHED_ENV.with(|c| c.set(boxed)); + ANY_PROCESS_ENV_OBJECT.store(true, std::sync::atomic::Ordering::Release); boxed } diff --git a/test-files/test_gap_dynamic_key_read_paths.ts b/test-files/test_gap_dynamic_key_read_paths.ts new file mode 100644 index 0000000000..a0eecfc0b7 --- /dev/null +++ b/test-files/test_gap_dynamic_key_read_paths.ts @@ -0,0 +1,60 @@ +// Dynamic-key property reads, `o[k]`, across the receiver shapes the fast path +// in `native_get::try_data_get_bytes` classifies — and the mutations that must +// invalidate what it reads. +// +// Three per-access re-derivations were removed behind this: the keys array was +// re-resolved through `clean_arr_ptr` although the collector maintains +// `ShapeDescriptor::keys`; a thread-local was read to answer "is this +// process.env?"; and an RwLock read guard was taken to ask whether the +// receiver's class id is an anon shape. The last is answered from a per-IMAGE +// lock-free mirror, so anything that depends on the anon-shape verdict +// (`.constructor`, `Object.getPrototypeOf`) is exercised here too. +const plain: any = { a: 1, b: "two", c: true }; +for (const k of ["a", "b", "c", "missing"]) console.log("plain", k, String(plain[k])); + +// Anon shape: an object literal's `.constructor` must still be Object, which +// is the verdict the mirror now answers. +console.log("ctor is Object:", plain.constructor === Object); +console.log("proto is Object.prototype:", Object.getPrototypeOf(plain) === Object.prototype); + +// Key added after first read: the shape changes, so the cached keys array must +// not be reused. +const grown: any = { x: 1 }; +console.log("before add", String(grown.y)); +grown.y = 42; +console.log("after add", String(grown.y)); + +// Key deleted after first read. +const shrunk: any = { p: 1, q: 2 }; +console.log("before delete", String(shrunk.q)); +delete shrunk.q; +console.log("after delete", String(shrunk.q)); + +// Many keys, to cross the indexed-lookup threshold the resolved entry +// deliberately delegates back to the original path for. +const wide: any = {}; +for (let i = 0; i < 64; i++) wide["k" + i] = i; +let sum = 0; +for (let i = 0; i < 64; i++) sum += wide["k" + i]; +console.log("wide sum", sum); + +// A declared class instance is NOT an anon shape. +class Holder { v: number; constructor(v: number) { this.v = v; } } +const inst: any = new Holder(7); +console.log("class read", inst["v"], "ctor", inst.constructor === Holder); + +// A prototype-chain read through a dynamic key. +const parent: any = { inherited: "yes" }; +const child: any = Object.create(parent); +child.own = "mine"; +console.log("own", child["own"], "inherited", child["inherited"]); + +// Accessors must not be answered from the data fast path. +const acc: any = { get computed() { return "from-getter"; } }; +console.log("accessor", acc["computed"]); + +// Hot loop over one key, which is the shape the removals were measured on. +const hot: any = { k: 1.5, other: 2 }; +let t = 0; +for (let i = 0; i < 500; i++) t += hot["k"]; +console.log("hot total", t);