From 31ebc72f8c8ca65ff80b7c9d279990a76e062d86 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 12 Aug 2026 03:00:56 +0200 Subject: [PATCH 1/2] perf(runtime): gate property-tail probes by header --- .../perry-runtime/src/object/field_get_set.rs | 3 + .../get_field_by_name_probe_tests.rs | 128 ++++++++++++++++++ .../field_get_set/get_field_by_name_tail.rs | 106 ++++----------- .../object/field_get_set/probe_dispatch.rs | 80 +++++++++++ scripts/addr_class_ratchet_baseline.txt | 2 +- 5 files changed, 236 insertions(+), 83 deletions(-) create mode 100644 crates/perry-runtime/src/object/field_get_set/get_field_by_name_probe_tests.rs create mode 100644 crates/perry-runtime/src/object/field_get_set/probe_dispatch.rs diff --git a/crates/perry-runtime/src/object/field_get_set.rs b/crates/perry-runtime/src/object/field_get_set.rs index f264e41ac6..986d591c4a 100644 --- a/crates/perry-runtime/src/object/field_get_set.rs +++ b/crates/perry-runtime/src/object/field_get_set.rs @@ -239,10 +239,13 @@ mod crypto_key; mod enumeration; mod field_ops; mod get_field_by_name; +#[cfg(test)] +mod get_field_by_name_probe_tests; mod get_field_by_name_tail; mod has_property; mod ic_miss; mod map_set_receiver; +mod probe_dispatch; /// Size of the direct-mapped `(keys_ptr, key_hash, field_index)` inline /// cache backing `js_object_get_field_by_name`'s slow tail. diff --git a/crates/perry-runtime/src/object/field_get_set/get_field_by_name_probe_tests.rs b/crates/perry-runtime/src/object/field_get_set/get_field_by_name_probe_tests.rs new file mode 100644 index 0000000000..ac04e9adbb --- /dev/null +++ b/crates/perry-runtime/src/object/field_get_set/get_field_by_name_probe_tests.rs @@ -0,0 +1,128 @@ +//! Probe-dispatch invariants for the by-name property-get slow tail (#7867). +//! +//! Buffer and small TypedArray allocations can be headerless, so their +//! registry probes must stay ahead of the first `GcHeader` read. Sets cannot: +//! every registered Set is `GC_TYPE_SET`. Symbols straddle both storage +//! classes, but every one carries `SYMBOL_MAGIC` in its own first word. These +//! tests pin the resulting split instead of merely checking the read result. + +use super::*; + +fn key(bytes: &[u8]) -> *const crate::StringHeader { + crate::string::js_string_from_bytes(bytes.as_ptr(), bytes.len() as u32) +} + +fn leaked_symbol(description: &str) -> usize { + let description = crate::string::js_string_from_str(description); + let description = f64::from_bits(crate::value::js_nanbox_string(description as i64).to_bits()); + let bits = unsafe { crate::symbol::js_symbol_for(description) }.to_bits(); + let ptr = crate::value::js_nanbox_get_pointer(f64::from_bits(bits)) as usize; + assert_ne!(ptr, 0, "test premise: Symbol.for allocated a symbol"); + assert!( + crate::symbol::is_registered_symbol(ptr), + "test premise: the leaked symbol is registered on this thread" + ); + ptr +} + +fn fresh_symbol(description: &str) -> usize { + let description = crate::string::js_string_from_str(description); + let description = f64::from_bits(crate::value::js_nanbox_string(description as i64).to_bits()); + let bits = unsafe { crate::symbol::js_symbol_new(description) }.to_bits(); + let ptr = crate::value::js_nanbox_get_pointer(f64::from_bits(bits)) as usize; + assert_ne!( + ptr, 0, + "test premise: Symbol(description) allocated a symbol" + ); + assert!( + crate::symbol::is_registered_symbol(ptr), + "test premise: the fresh symbol is registered on this thread" + ); + ptr +} + +fn tail(addr: usize, property: &[u8]) -> JSValue { + get_field_by_name_object_tail(addr as *const ObjectHeader, key(property)) +} + +#[test] +fn plain_object_miss_skips_set_and_symbol_registries() { + leaked_symbol("perry-7867-arm-symbol"); + let _set = crate::set::js_set_alloc(4); + let object = crate::object::js_object_alloc(0, 0) as usize; + + // Warm unrelated lazy state before taking the counters. + assert!(tail(object, b"missing").is_undefined()); + let set_before = crate::set::test_set_registry_probe_count(); + let symbol_before = crate::symbol::test_symbol_registry_probe_count(); + + assert!(tail(object, b"missing").is_undefined()); + assert_eq!( + crate::set::test_set_registry_probe_count(), + set_before, + "GC_TYPE_OBJECT rules a Set out; a property miss must not probe SET_REGISTRY" + ); + assert_eq!( + crate::symbol::test_symbol_registry_probe_count(), + symbol_before, + "a plain-object property miss must not take the process-global symbol mutex" + ); + + // Sabotage the cheap SYMBOL_MAGIC screen. The authoritative registry probe + // must return, while the answer and the Set counter stay unchanged. + let restore = crate::symbol::test_disable_symbol_magic_screen(true); + let set_before = crate::set::test_set_registry_probe_count(); + let symbol_before = crate::symbol::test_symbol_registry_probe_count(); + let answer = tail(object, b"missing"); + let symbol_after = crate::symbol::test_symbol_registry_probe_count(); + crate::symbol::test_disable_symbol_magic_screen(restore); + + assert!(answer.is_undefined()); + assert!( + symbol_after > symbol_before, + "with the magic screen defeated the symbol registry probe must return" + ); + assert_eq!( + crate::set::test_set_registry_probe_count(), + set_before, + "defeating the symbol screen must not reintroduce the Set probe" + ); +} + +#[test] +fn set_and_both_symbol_storage_classes_still_dispatch() { + let set = crate::set::js_set_alloc(4) as usize; + let set_before = crate::set::test_set_registry_probe_count(); + let size = tail(set, b"size"); + assert_eq!(f64::from_bits(size.bits()), 0.0); + assert!( + crate::set::test_set_registry_probe_count() > set_before, + "GC_TYPE_SET must still enter the authoritative Set registry" + ); + + let symbol = leaked_symbol("perry-7867-headerless-symbol"); + assert!( + unsafe { crate::symbol::may_be_symbol_header(symbol as *const u8) }, + "a Box-leaked symbol must carry SYMBOL_MAGIC without a GcHeader" + ); + let symbol_before = crate::symbol::test_symbol_registry_probe_count(); + let description = tail(symbol, b"description"); + assert!(description.is_string()); + assert!( + crate::symbol::test_symbol_registry_probe_count() > symbol_before, + "a possible Symbol must still enter the authoritative symbol registry" + ); + + let symbol = fresh_symbol("perry-7867-gc-symbol"); + assert!( + unsafe { crate::symbol::may_be_symbol_header(symbol as *const u8) }, + "a GC-allocated symbol must carry SYMBOL_MAGIC too" + ); + let symbol_before = crate::symbol::test_symbol_registry_probe_count(); + let description = tail(symbol, b"description"); + assert!(description.is_string()); + assert!( + crate::symbol::test_symbol_registry_probe_count() > symbol_before, + "a GC-allocated Symbol must still enter the authoritative symbol registry" + ); +} diff --git a/crates/perry-runtime/src/object/field_get_set/get_field_by_name_tail.rs b/crates/perry-runtime/src/object/field_get_set/get_field_by_name_tail.rs index 2ba040bea5..f8c7ce9968 100644 --- a/crates/perry-runtime/src/object/field_get_set/get_field_by_name_tail.rs +++ b/crates/perry-runtime/src/object/field_get_set/get_field_by_name_tail.rs @@ -451,88 +451,22 @@ pub(crate) fn get_field_by_name_object_tail( } return JSValue::undefined(); } - // Sets: detect via the SET_REGISTRY, which is authoritative and - // dereference-free. Route `.size` to `js_set_size` and synthesize - // method values for prototype functions such as `.has`, which Node - // exposes through ordinary property reads. - // - // (This used to say a `SetHeader` comes from a raw `alloc()` with no - // `GcHeader`, so the preceding byte could not be read. That has not - // been true since `js_set_alloc` moved to - // `arena_alloc_gc(_, _, GC_TYPE_SET)`: a registered Set IS a GC - // allocation and its `obj_type` classifies it. `js_array_get_f64` and - // `js_array_length` gate their probes on exactly that byte (#7765); - // this receiver is not proven to carry a header at this point, so it - // still asks the registry.) - if crate::set::is_registered_set(obj as usize) { - if !key.is_null() { - let key_ptr = (key as *const u8).add(std::mem::size_of::()); - let key_len = (*key).byte_len as usize; - let key_bytes = std::slice::from_raw_parts(key_ptr, key_len); - if key_bytes == b"size" { - let s = obj as *const crate::set::SetHeader; - return JSValue::number(crate::set::js_set_size(s) as f64); - } - if let Some(name) = set_method_value_name(key_bytes) { - // Return the SAME brand-checking thunk installed on - // Set.prototype so `const m = s.forEach; m.call(badThis)` - // throws a TypeError (and `m === Set.prototype.forEach`). - // Falls back to the legacy instance-bound closure if the - // prototype thunk isn't available. - if let Ok(method_name) = std::str::from_utf8(name) { - if let Some(v) = - super::super::collection_proto_thunks::collection_proto_method_value( - "Set", - method_name, - ) - { - return JSValue::from_bits(v.to_bits()); - } - } - let this_f64 = - f64::from_bits(crate::value::js_nanbox_pointer(obj as i64).to_bits()); - let result = js_class_method_bind(this_f64, name.as_ptr(), name.len()); - return JSValue::from_bits(result.to_bits()); - } - // User expando keys (`s.tag = x`) live in the exotic side - // table (`ExoticKind::Set`); see the Map/Set arm below. - if let Ok(name) = std::str::from_utf8(key_bytes) { - let receiver = - f64::from_bits(crate::value::js_nanbox_pointer(obj as i64).to_bits()); - if let Some(v) = crate::object::exotic_expando::exotic_get_own_property( - obj as usize, - crate::object::exotic_expando::ExoticKind::Set, - name, - receiver, - ) { - return JSValue::from_bits(v.to_bits()); - } - } - } - return JSValue::undefined(); - } - // Symbols: registered in SYMBOL_POINTERS by symbol.rs. Symbols - // allocated via Symbol.for(...) are Box-leaked (no GcHeader), so - // reading the byte before would be UB. Detect via the side table. - if crate::symbol::is_registered_symbol(obj as usize) { - if !key.is_null() { - let key_ptr = (key as *const u8).add(std::mem::size_of::()); - let key_len = (*key).byte_len as usize; - let key_bytes = std::slice::from_raw_parts(key_ptr, key_len); - let sym_f64 = - f64::from_bits(0x7FFD_0000_0000_0000u64 | (obj as u64 & 0x0000_FFFF_FFFF_FFFF)); - if key_bytes == b"description" { - return JSValue::from_bits( - crate::symbol::js_symbol_description(sym_f64).to_bits(), - ); - } + // Symbols straddle two storage classes: fresh Symbol() values carry a + // GC_TYPE_STRING header, while Symbol.for / well-known / Intl symbols + // are Box-leaked and carry no GcHeader at all. Every one does carry + // SYMBOL_MAGIC in its own first word, so use that exact-false screen + // before the authoritative registry. A plain object now pays one + // 4-byte load instead of the process-global symbol Mutex + SipHash. + if crate::symbol::may_be_symbol_header(obj as *const u8) { + if let Some(value) = super::probe_dispatch::symbol_property_if_registered(obj, key) { + return value; } - return JSValue::undefined(); } - // Validate this is an ObjectHeader, not some other heap type. - // Check GcHeader first (reliable for heap objects), then fallback to ObjectHeader.object_type - // for static/const objects that don't have GcHeaders. - // Guard: ensure we can safely read GC_HEADER_SIZE bytes before obj + + // Buffer and TypedArray were the two headerless allocations that had + // to be classified first. A possible headerless Symbol returned above; + // every remaining supported receiver can now be classified once by + // the GcHeader the rest of this function already switches on. if (obj as usize) < crate::gc::GC_HEADER_SIZE + 0x1000 || !is_valid_obj_ptr(obj as *const u8) { @@ -541,8 +475,16 @@ pub(crate) fn get_field_by_name_object_tail( let gc_header = (obj as *const u8).sub(crate::gc::GC_HEADER_SIZE) as *const crate::gc::GcHeader; let gc_type = (*gc_header).obj_type; - if gc_type != crate::gc::GC_TYPE_ARRAY && !is_valid_obj_ptr(obj as *const u8) { - return JSValue::undefined(); + + // Sets are arena_alloc_gc(_, _, GC_TYPE_SET) allocations. Let the + // header rule every other receiver out before entering SET_REGISTRY; + // keep the registry authoritative for a genuine Set and for stale + // address reuse. Route `.size` and prototype method-value reads exactly + // as before. + if gc_type == crate::gc::GC_TYPE_SET { + if let Some(value) = super::probe_dispatch::set_property_if_registered(obj, key) { + return value; + } } // Issue #618: closures have their own GC type (GC_TYPE_CLOSURE=4) // distinct from GC_TYPE_OBJECT, but support dynamic-property storage diff --git a/crates/perry-runtime/src/object/field_get_set/probe_dispatch.rs b/crates/perry-runtime/src/object/field_get_set/probe_dispatch.rs new file mode 100644 index 0000000000..6776929820 --- /dev/null +++ b/crates/perry-runtime/src/object/field_get_set/probe_dispatch.rs @@ -0,0 +1,80 @@ +//! Cold Set/Symbol property dispatch reached from the by-name slow tail. + +use super::*; + +#[cold] +#[inline(never)] +pub(super) unsafe fn symbol_property_if_registered( + obj: *const ObjectHeader, + key: *const crate::StringHeader, +) -> Option { + if !crate::symbol::is_registered_symbol(obj as usize) { + return None; + } + if !key.is_null() { + let key_ptr = (key as *const u8).add(std::mem::size_of::()); + let key_len = (*key).byte_len as usize; + let key_bytes = std::slice::from_raw_parts(key_ptr, key_len); + let sym_f64 = + f64::from_bits(0x7FFD_0000_0000_0000u64 | (obj as u64 & 0x0000_FFFF_FFFF_FFFF)); + if key_bytes == b"description" { + return Some(JSValue::from_bits( + crate::symbol::js_symbol_description(sym_f64).to_bits(), + )); + } + } + Some(JSValue::undefined()) +} + +#[cold] +#[inline(never)] +pub(super) unsafe fn set_property_if_registered( + obj: *const ObjectHeader, + key: *const crate::StringHeader, +) -> Option { + if !crate::set::is_registered_set(obj as usize) { + return None; + } + if !key.is_null() { + let key_ptr = (key as *const u8).add(std::mem::size_of::()); + let key_len = (*key).byte_len as usize; + let key_bytes = std::slice::from_raw_parts(key_ptr, key_len); + if key_bytes == b"size" { + let set = obj as *const crate::set::SetHeader; + return Some(JSValue::number(crate::set::js_set_size(set) as f64)); + } + if let Some(name) = set_method_value_name(key_bytes) { + // Return the SAME brand-checking thunk installed on Set.prototype + // so `const m = s.forEach; m.call(badThis)` throws a TypeError (and + // `m === Set.prototype.forEach`). Fall back to the legacy + // instance-bound closure if the prototype thunk isn't available. + if let Ok(method_name) = std::str::from_utf8(name) { + if let Some(value) = + super::super::collection_proto_thunks::collection_proto_method_value( + "Set", + method_name, + ) + { + return Some(JSValue::from_bits(value.to_bits())); + } + } + let this_f64 = f64::from_bits(crate::value::js_nanbox_pointer(obj as i64).to_bits()); + let result = js_class_method_bind(this_f64, name.as_ptr(), name.len()); + return Some(JSValue::from_bits(result.to_bits())); + } + // User expando keys (`s.tag = x`) live in the exotic side table + // (`ExoticKind::Set`); see the Map/Set arm in the caller. + if let Ok(name) = std::str::from_utf8(key_bytes) { + let receiver = f64::from_bits(crate::value::js_nanbox_pointer(obj as i64).to_bits()); + if let Some(value) = crate::object::exotic_expando::exotic_get_own_property( + obj as usize, + crate::object::exotic_expando::ExoticKind::Set, + name, + receiver, + ) { + return Some(JSValue::from_bits(value.to_bits())); + } + } + } + Some(JSValue::undefined()) +} diff --git a/scripts/addr_class_ratchet_baseline.txt b/scripts/addr_class_ratchet_baseline.txt index 3511cb4ac7..f1765f9eee 100644 --- a/scripts/addr_class_ratchet_baseline.txt +++ b/scripts/addr_class_ratchet_baseline.txt @@ -235,7 +235,7 @@ lone-valid-obj-ptr | crates/perry-runtime/src/object/descriptors.rs | 2 lone-valid-obj-ptr | crates/perry-runtime/src/object/field_get_set/accessors.rs | 2 lone-valid-obj-ptr | crates/perry-runtime/src/object/field_get_set/enumeration.rs | 3 lone-valid-obj-ptr | crates/perry-runtime/src/object/field_get_set/field_ops.rs | 1 -lone-valid-obj-ptr | crates/perry-runtime/src/object/field_get_set/get_field_by_name_tail.rs | 2 +lone-valid-obj-ptr | crates/perry-runtime/src/object/field_get_set/get_field_by_name_tail.rs | 1 lone-valid-obj-ptr | crates/perry-runtime/src/object/field_get_set/has_property.rs | 1 lone-valid-obj-ptr | crates/perry-runtime/src/object/field_get_set/ic_miss.rs | 1 lone-valid-obj-ptr | crates/perry-runtime/src/object/field_set_by_name/attr_variants.rs | 2 From 761969671282d5b43df294dd2c6ec5d3c01e24f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 12 Aug 2026 03:40:34 +0200 Subject: [PATCH 2/2] docs(changelog): record property-probe validation --- .../7897-header-gated-property-tail.md | 57 +++++++++++++++++++ 1 file changed, 57 insertions(+) create mode 100644 changelog.d/7897-header-gated-property-tail.md diff --git a/changelog.d/7897-header-gated-property-tail.md b/changelog.d/7897-header-gated-property-tail.md new file mode 100644 index 0000000000..e364fb36b6 --- /dev/null +++ b/changelog.d/7897-header-gated-property-tail.md @@ -0,0 +1,57 @@ +### Performance + +**Property-get IC misses no longer ask the Set and Symbol registries about an +ordinary object (#7867).** `get_field_by_name_object_tail` already classified +Buffer and TypedArray before reading the receiver's `GcHeader`, then switched +on that header for the rest of its exotic-receiver cases. Set and Symbol were +the exceptions: every miss consulted their address-keyed registries first, +including the process-global Symbol mutex and hash table. + +Those two exceptions need different gates. Sets are allocated with +`GC_TYPE_SET`, so the header can rule them out before `SET_REGISTRY` is touched. +Symbols span two storage classes: fresh `Symbol()` values are GC-backed strings, +while `Symbol.for`, well-known, and Intl symbols are Box-leaked and have no +`GcHeader`. Every Symbol does carry `SYMBOL_MAGIC` in its first word, so the +existing exact-false `may_be_symbol_header` screen rules ordinary receivers out +without excluding either storage class. In both positive cases the registry +remains authoritative, including for stale address reuse. + +The ordering remains deliberate. Buffer and TypedArray are headerless and stay +before the first `GcHeader` read. A possible headerless Symbol returns from the +magic-gated dispatch before that read. Only then does the tail validate the +pointer, read the header once, and use `GC_TYPE_SET` to decide whether Set +dispatch is possible. + +The registered-receiver bodies are `#[cold]` and `#[inline(never)]`. This is not +cosmetic: an initial version left them inline, grew +`get_field_by_name_object_tail` by 40 bytes, shifted every following runtime +function, and regressed `pipeline_big` by **+4.636%** paired geomean (30 +alternating quiet-M1-mini pairs, 95% CI +4.589% to +4.681%). Sampling showed no +new hot leaf inside the changed tail; the regression tracked the text-layout +shift. That version was rejected rather than hidden behind the intended probe +saving. + +With the cold dispatch outlined, the same locked-host A/B measured 30 +alternating pairs at 1.693549 s base median and 1.688705 s fixed median: +**-0.315% paired geomean**, bootstrap 95% CI -0.355% to -0.273%. Both arms +exited zero and produced the exact `pipeline_big` oracle output +`556260000 3 3`. + +The issue's earlier 1.8% `is_registered_symbol_slow` attribution is no longer +present on current `main`: fresh debug-symbol samples contain zero such samples +in either arm. The structural cost is still directly reproducible. The new +test arms both registries, performs plain-object misses, and asserts the +test-only entry counters do not move; before this change the Set counter moves +from 801 to 803. It then sabotages the Symbol magic screen after a successful +lookup to prove the Symbol dispatch actually ran and the Set registry remained +untouched. A companion test exercises Set `.size`, a Box-leaked +`Symbol.for(...).description`, and a GC-backed `Symbol(...).description`, so +deleting a positive path or accidentally requiring every receiver to have a +header turns the suite red. + +Validation includes the full serialized runtime suite (2,154 passed, 4 +ignored), its doc tests, test-registration and thread-local policy checks, the +address-class audit, formatting, file-size, and whitespace gates. The +address-class audit also reports the pre-existing stale `dyn_index.rs` ratchet +entry (`baseline 2, found 1`); that adjacent cleanup is intentionally not folded +into this PR.