diff --git a/changelog.d/9862-regexp-descriptor-summary-probe.md b/changelog.d/9862-regexp-descriptor-summary-probe.md new file mode 100644 index 0000000000..a615dfa760 --- /dev/null +++ b/changelog.d/9862-regexp-descriptor-summary-probe.md @@ -0,0 +1,46 @@ +A `RegExp` — and every other cell type that owns a metadata edge — can now +answer a descriptor-summary probe from its own header instead of hashing its way +into the descriptor tables. + +`set_last_index_throwing` asks `get_property_attrs(re, "lastIndex")` on every +global or sticky `test()`/`exec()`, because a user can make `lastIndex` +non-writable and the spec's `Set(R, "lastIndex", n, true)` must then throw +(test262 `prototype/{exec,test}/y-fail-lastindex-no-write`). #6759 phase C2 +added a per-object meta summary precisely so that question could be answered +without touching the tables, but `may_have_descriptor_entry` reached that +summary through `meta_capable_object`, which answers only for `GC_TYPE_OBJECT`. +A `RegExp` is its own cell type, so the filter returned the conservative "maybe" +for every RegExp receiver and the slow probe ran every time: a `String` +allocation for the key, a SipHash of `(usize, String)`, and a map lookup that +was always going to miss. + +The capability was already present and merely unwired. #6759 phase 1 unified the +metadata edge behind `cell_meta_slot`, which answers for Object, Error, Map, +Set, RegExp, Promise and Date, and `RegExpHeader::meta` is traced by +`GcLayoutSlotKind::RegExpFields` and moves with its header. The five +descriptor-summary sites now share one predicate built on that edge, so the +change adds no state and no new invariant — it asks the narrower question the +summary actually needs rather than the `ObjectHeader`-shaped one +`meta_capable_object`'s other callers need. + +The predicate's answer is deliberately three-way. `None` means the cell type has +no metadata edge and the caller must stay conservative; `Some(null)` means the +edge exists and no record was ever installed, which *proves* the tables hold no +entry for this owner; `Some(meta)` means read the summary words. Collapsing the +first two would turn a conservative *maybe* into a false *no* for the types that +still lack an edge (Temporal, the typed-array views). + +Install and probe move together, which is what keeps the fast negative safe: the +installing twin of the predicate is used by `note_meta_descriptor_key`, so an +owner whose install set the key bit is always found, and +`Object.defineProperty(re, "lastIndex", { writable: false })` still makes the +next `test()` throw. `js_regexp_new` writes `meta = null` on every construction, +so a fresh header at a recycled address cannot inherit a dead tenant's bits. + +Measured on one 400-character claude-code reply with `PERRY_REGEX_DIAG` armed: +424,035 descriptor-summary probes with a `RegExp` owner, **100.0 % of which the +meta summary now proves absent** — 5.02 per global `test()` call, each one a +`String` allocation, a SipHash and a missing map lookup that no longer happen. +Two diagnostic counters (`desc_regexp_probes`, `desc_regexp_meta_negative`) are +added behind that same environment variable; the second was 0 by construction +before this change, so a single binary measures both arms. diff --git a/crates/perry-runtime/src/hot_diag.rs b/crates/perry-runtime/src/hot_diag.rs index b4bd5238ee..c3109233cf 100644 --- a/crates/perry-runtime/src/hot_diag.rs +++ b/crates/perry-runtime/src/hot_diag.rs @@ -125,6 +125,15 @@ pub struct RegexDiag { pub replace_calls: u64, pub replace_matches: u64, pub split_calls: u64, + /// `may_have_descriptor_entry` calls whose owner is a `GC_TYPE_REGEXP` + /// cell — the `lastIndex` writability question `set_last_index_throwing` + /// asks on every global/sticky `test()`/`exec()`. + pub desc_regexp_probes: u64, + /// Of those, the ones the per-object meta summary proved absent, so no + /// `key.to_string()` and no SipHash of `(usize, String)` ran. Before the + /// meta edge was wired for RegExp this was 0 by construction: the filter + /// answered "maybe" for every one of them. + pub desc_regexp_meta_negative: u64, per_pattern: HashMap, } @@ -238,7 +247,8 @@ impl RegexDiag { "[regex-diag] t={secs:.1}s new={} validated_hit={} site_hit={} pattern_bytes={} \ compiles std={} fancy={} repeat={} cache_clears={} lazy_builds={} lazy_cache_hits={} \ exec={} exec_matched={} capture_slots={} capture_bytes={} test={} test_global={} \ - match={} replace={} replace_matches={} split={}", + match={} replace={} replace_matches={} split={} \ + desc_regexp_probes={} desc_regexp_meta_negative={}", self.new_calls, self.new_validated_hit, self.new_site_hit, @@ -259,6 +269,8 @@ impl RegexDiag { self.replace_calls, self.replace_matches, self.split_calls, + self.desc_regexp_probes, + self.desc_regexp_meta_negative, ); // Merge by content (prefix, len, flags): distinct literal sites with // the same pattern are one row. diff --git a/crates/perry-runtime/src/object/descriptor_state.rs b/crates/perry-runtime/src/object/descriptor_state.rs index 109ef1e75e..5c91593430 100644 --- a/crates/perry-runtime/src/object/descriptor_state.rs +++ b/crates/perry-runtime/src/object/descriptor_state.rs @@ -612,6 +612,40 @@ pub(crate) fn test_descriptor_key_bit(key: &str) -> u64 { descriptor_key_bit(key) } +/// #6759 phase 1 follow-up: the owner's meta record for descriptor-summary +/// purposes, for ANY cell type that owns one. +/// +/// [`super::prototype_chain::meta_capable_object`] answers only for +/// `GC_TYPE_OBJECT`, because its other callers need an `ObjectHeader` to work +/// with. The descriptor summary does not — it needs the `ObjectMeta` edge and +/// nothing else — and every exotic cell has carried that edge since #6759 +/// phase 1 unified it behind [`super::cell_meta_slot`]. Asking the narrower +/// question is what lets a `RegExp` receiver answer a summary probe at all. +/// +/// * `None` — the cell type has no meta edge, so the caller must stay +/// conservative and probe the tables. +/// * `Some(null)` — the cell HAS the edge and no record was ever installed, +/// which proves the tables hold no entry for this owner. +/// * `Some(meta)` — read the summary words. +/// +/// The three-way answer is the whole contract: collapsing "no edge" and "edge, +/// but null" into one `None` would turn a conservative *maybe* into a false +/// *no* for the cell types that still lack an edge. +#[inline] +unsafe fn descriptor_summary_meta(owner: usize) -> Option<*mut ObjectMeta> { + Some(*super::cell_meta_slot(owner)?) +} + +/// Installing twin of [`descriptor_summary_meta`]. Install and probe MUST use +/// the same predicate: a probe that admits a cell type whose installs do not +/// set the key bits would answer a proven-absent for an owner that really has +/// a descriptor — e.g. `Object.defineProperty(re, "lastIndex", {writable:false})` +/// would stop throwing (test262 prototype/{exec,test}/y-fail-lastindex-no-write). +#[inline] +unsafe fn descriptor_summary_meta_ensure(owner: usize) -> Option<*mut ObjectMeta> { + super::object_meta_ensure_for_cell(owner) +} + /// #6759 Phase C2: record `key` in the owner's per-object meta summary so /// hot-path probes for OTHER keys can skip the descriptor tables. No-op for /// owners that cannot carry a meta record (handle-band ids, typed arrays, @@ -628,13 +662,11 @@ pub(crate) fn test_descriptor_key_bit(key: &str) -> u64 { /// owner left behind can no longer be misread as the new tenant's. fn note_meta_descriptor_key(owner: usize, key: &str, accessor: bool) { unsafe { - if let Some(obj) = super::prototype_chain::meta_capable_object(owner) { - // No-move window: `object_meta_ensure` allocates, and a - // triggered collection could MOVE `owner` — installers - // (freeze/seal loops, defineProperty) hold raw owner pointers - // across repeated installs. - let _no_gc = crate::gc::GcSuppressScope::new(); - let meta = super::object_meta_ensure(obj); + // No-move window: the ensure below allocates, and a triggered + // collection could MOVE `owner` — installers (freeze/seal loops, + // defineProperty) hold raw owner pointers across repeated installs. + let _no_gc = crate::gc::GcSuppressScope::new(); + if let Some(meta) = descriptor_summary_meta_ensure(owner) { let bit = descriptor_key_bit(key); if accessor { (*meta).accessor_key_bits |= bit; @@ -652,22 +684,60 @@ fn note_meta_descriptor_key(owner: usize, key: &str, accessor: bool) { #[inline] pub(crate) fn may_have_descriptor_entry(owner: usize, key: &str, accessor: bool) -> bool { unsafe { - match super::prototype_chain::meta_capable_object(owner) { - Some(obj) => { - let meta = (*obj).meta; + let answer = match descriptor_summary_meta(owner) { + Some(meta) => { if meta.is_null() { - return false; - } - let word = if accessor { - (*meta).accessor_key_bits + false } else { - (*meta).attr_key_bits - }; - word & descriptor_key_bit(key) != 0 + let word = if accessor { + (*meta).accessor_key_bits + } else { + (*meta).attr_key_bits + }; + word & descriptor_key_bit(key) != 0 + } } None => true, + }; + // Diagnostic only, and only when the instrument is armed: one relaxed + // load otherwise. Counts the RegExp receivers this filter sees and how + // many it now proves absent — before the meta edge was wired for + // RegExp the second number was 0 by construction. + if crate::hot_diag::regex_on() { + note_regexp_descriptor_probe(owner, answer); } + answer + } +} + +/// Test-only view of [`may_have_descriptor_entry`], so a test can assert the +/// FILTER's answer rather than only the value it filters to. Without this a +/// test can see that `get_property_attrs` returns `None`, which is equally +/// true when the fast negative never fired — it would pass against a change +/// that did nothing. +#[cfg(test)] +pub(crate) fn test_may_have_descriptor_entry(owner: usize, key: &str, accessor: bool) -> bool { + may_have_descriptor_entry(owner, key, accessor) +} + +/// Diagnostic counter for [`may_have_descriptor_entry`]: is this owner a +/// RegExp cell, and did the summary prove the key absent? Split out and marked +/// cold so the armed check costs the hot path a predictable branch and nothing +/// else. +#[cold] +unsafe fn note_regexp_descriptor_probe(owner: usize, answer: bool) { + let Some(header) = crate::value::addr_class::try_read_gc_header(owner) else { + return; + }; + if header.obj_type != crate::gc::GC_TYPE_REGEXP { + return; } + crate::hot_diag::regex_with(|d| { + d.desc_regexp_probes += 1; + if !answer { + d.desc_regexp_meta_negative += 1; + } + }); } /// #6759 Phase C2: can an OWN string-keyed descriptor (attr or accessor) @@ -682,9 +752,8 @@ unsafe fn own_descriptor_may_cover_key(addr: usize, key: f64) -> bool { ) else { return true; }; - match super::prototype_chain::meta_capable_object(addr) { - Some(obj) => { - let meta = (*obj).meta; + match descriptor_summary_meta(addr) { + Some(meta) => { if meta.is_null() { return false; } @@ -702,9 +771,8 @@ unsafe fn own_descriptor_may_cover_key(addr: usize, key: f64) -> bool { #[inline] pub(crate) fn owner_may_have_descriptor_entries(owner: usize, accessor: bool) -> bool { unsafe { - match super::prototype_chain::meta_capable_object(owner) { - Some(obj) => { - let meta = (*obj).meta; + match descriptor_summary_meta(owner) { + Some(meta) => { if meta.is_null() { return false; } @@ -1148,11 +1216,10 @@ fn owner_index_push_proven_new( /// single-kind form's no-op arm). fn note_meta_descriptor_key_both(owner: usize, key: &str) -> Option<(bool, bool)> { unsafe { - let obj = super::prototype_chain::meta_capable_object(owner)?; - // No-move window: `object_meta_ensure` allocates (see + // No-move window: the ensure allocates (see // `note_meta_descriptor_key`). let _no_gc = crate::gc::GcSuppressScope::new(); - let meta = super::object_meta_ensure(obj); + let meta = descriptor_summary_meta_ensure(owner)?; let bit = descriptor_key_bit(key); let accessor_bit_was_set = (*meta).accessor_key_bits & bit != 0; let attr_bit_was_set = (*meta).attr_key_bits & bit != 0; diff --git a/crates/perry-runtime/src/object/mod.rs b/crates/perry-runtime/src/object/mod.rs index c7b48fc87d..b5a73cca57 100644 --- a/crates/perry-runtime/src/object/mod.rs +++ b/crates/perry-runtime/src/object/mod.rs @@ -275,6 +275,8 @@ pub(crate) use descriptor_state::{ set_builtin_property_attrs, set_property_attrs, transfer_descriptor_owner, AccessorDescriptor, DescriptorTables, PropertyAttrs, }; +#[cfg(test)] +pub(crate) use descriptor_state::test_may_have_descriptor_entry; pub(crate) use field_get_set::FieldLookupCaches; pub(crate) use field_get_set::{ private_evaluation_brand_value, private_lexical_brand_pop, private_lexical_brand_push, diff --git a/crates/perry-runtime/src/regex/tests.rs b/crates/perry-runtime/src/regex/tests.rs index d82dea78e9..9ff488a547 100644 --- a/crates/perry-runtime/src/regex/tests.rs +++ b/crates/perry-runtime/src/regex/tests.rs @@ -1830,3 +1830,67 @@ fn a_single_program_cache_clear_cannot_disarm_a_lookbehind_literal() { "the literal must still match after an unrelated cache reached capacity" ); } + +/// #6759 phase 1 follow-up: a `RegExp` receiver can now answer the +/// descriptor-summary probe. Before the meta edge was wired for +/// `GC_TYPE_REGEXP`, `may_have_descriptor_entry` answered the conservative +/// `true` for every RegExp, so `set_last_index_throwing` built a `String` and +/// SipHashed `(usize, String)` on every global/sticky `test()`/`exec()`. +#[test] +fn a_fresh_regexp_proves_lastindex_absent_without_probing_the_tables() { + let _lock = crate::gc::global_side_table_test_lock(); + let scope = crate::gc::RuntimeHandleScope::new(); + let pattern = scope.root_string_ptr(make_string("x")); + let flags = scope.root_string_ptr(make_string("g")); + let re = pattern.with_mut_ptr::(|pattern| { + flags.with_mut_ptr::(|flags| js_regexp_new(pattern, flags)) + }); + // Premise: this really is the dedicated RegExp cell, not a shaped object + // that would have answered through the ordinary `GC_TYPE_OBJECT` path. + let gc = unsafe { crate::value::addr_class::try_read_gc_header(re as usize) } + .expect("RegExp must be a GC allocation"); + assert_eq!(gc.obj_type, crate::gc::GC_TYPE_REGEXP); + + assert!( + !crate::object::test_may_have_descriptor_entry(re as usize, "lastIndex", false), + "a fresh RegExp has no descriptors, so the meta summary must prove \ + `lastIndex` absent instead of sending the caller to the table" + ); + assert!( + crate::object::get_property_attrs(re as usize, "lastIndex").is_none(), + "and the answer the fast path skips must be the same one" + ); +} + +/// The other half, and the one that makes the fast negative safe: an owner +/// that DOES have a descriptor must still be found. Install and probe share +/// one predicate, so a probe widened without its install would answer +/// "proven absent" here and `set_last_index_throwing` would silently stop +/// throwing (test262 prototype/{exec,test}/y-fail-lastindex-no-write). +#[test] +fn a_regexp_with_a_non_writable_lastindex_is_still_found_by_the_probe() { + let _lock = crate::gc::global_side_table_test_lock(); + let scope = crate::gc::RuntimeHandleScope::new(); + let pattern = scope.root_string_ptr(make_string("x")); + let flags = scope.root_string_ptr(make_string("g")); + let re = pattern.with_mut_ptr::(|pattern| { + flags.with_mut_ptr::(|flags| js_regexp_new(pattern, flags)) + }); + let attrs = crate::object::PropertyAttrs::new(false, true, true); + crate::object::set_property_attrs(re as usize, "lastIndex".to_string(), attrs); + + assert!( + crate::object::test_may_have_descriptor_entry(re as usize, "lastIndex", false), + "the install set the key bit, so the probe must send the caller to the table" + ); + let found = crate::object::get_property_attrs(re as usize, "lastIndex") + .expect("the descriptor the test installed must be readable back"); + assert!(!found.writable(), "and it must still read as non-writable"); + + // A DIFFERENT key on the same owner stays proven-absent: the summary is + // per key, not per owner, so widening it must not blunt it. + assert!( + !crate::object::test_may_have_descriptor_entry(re as usize, "source", false), + "an unrelated key on the same RegExp must still take the fast negative" + ); +}