Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions changelog.d/9862-regexp-descriptor-summary-probe.md
Original file line number Diff line number Diff line change
@@ -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.
14 changes: 13 additions & 1 deletion crates/perry-runtime/src/hot_diag.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<usize, PatStat>,
}

Expand Down Expand Up @@ -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,
Expand All @@ -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.
Expand Down
119 changes: 93 additions & 26 deletions crates/perry-runtime/src/object/descriptor_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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;
Expand All @@ -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)
Expand All @@ -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;
}
Expand All @@ -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;
}
Expand Down Expand Up @@ -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;
Expand Down
2 changes: 2 additions & 0 deletions crates/perry-runtime/src/object/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
64 changes: 64 additions & 0 deletions crates/perry-runtime/src/regex/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::<StringHeader, _>(|pattern| {
flags.with_mut_ptr::<StringHeader, _>(|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::<StringHeader, _>(|pattern| {
flags.with_mut_ptr::<StringHeader, _>(|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"
);
}
Loading