Skip to content
Merged
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
1 change: 1 addition & 0 deletions changelog.d/6802-inline-guard-per-key-c5a.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
perf(runtime): #6759 C5a — the class-field inline-guard disable is now vetted per-key: a prototype-level descriptor install only flips the process-wide kill switch when its key names a declared instance field of a registered class (with a retro-check for classes that register after the install). Babel-style method installs on class prototypes no longer push every `this.field` access onto the out-of-line guard call.
33 changes: 25 additions & 8 deletions crates/perry-runtime/src/object/alloc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,31 @@ fn remember_class_keys_array(class_id: u32, field_count: u32, keys_array: *mut A
if class_id == 0 || keys_array.is_null() {
return;
}
let mut guard = CLASS_KEYS_BY_ID.write().unwrap();
if guard.is_none() {
*guard = Some(std::collections::HashMap::new());
}
guard
.as_mut()
.unwrap()
.insert(class_id, (keys_array as usize, field_count));
{
let mut guard = CLASS_KEYS_BY_ID.write().unwrap();
if guard.is_none() {
*guard = Some(std::collections::HashMap::new());
}
guard
.as_mut()
.unwrap()
.insert(class_id, (keys_array as usize, field_count));
}
// #6759 C5a: harvest this class's declared instance-field names into
// the process-wide name-hash set the per-key inline-guard vetting
// consults — and retro-check them against prototype-level descriptor
// keys installed BEFORE this class registered (module-init ordering
// must not create an unsound skip).
unsafe {
let count = field_count.min(crate::array::js_array_length(keys_array)) as usize;
let mut sso = [0u8; crate::value::SHORT_STRING_MAX_LEN];
for i in 0..count {
let v = crate::array::js_array_get(keys_array, i as u32);
if let Some(b) = crate::string::js_string_key_bytes(v, &mut sso) {
super::descriptor_state::note_declared_instance_field_name(b);
}
}
}
}

pub(crate) fn registered_class_keys_array(class_id: u32) -> Option<(*mut ArrayHeader, u32)> {
Expand Down
166 changes: 161 additions & 5 deletions crates/perry-runtime/src/object/descriptor_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,11 @@ pub(crate) fn class_field_inline_guard_enabled() -> bool {
PERRY_CLASS_FIELD_INLINE_GUARD_DISABLED.load(Ordering::Relaxed) == 0
}

#[cfg(test)]
pub(crate) fn test_reset_class_field_inline_guard() {
PERRY_CLASS_FIELD_INLINE_GUARD_DISABLED.store(0, Ordering::Relaxed);
}

Comment on lines +147 to +151

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Test reset helper doesn't clear the new per-key hash sets.

test_reset_class_field_inline_guard() only resets PERRY_CLASS_FIELD_INLINE_GUARD_DISABLED, leaving DECLARED_FIELD_NAME_HASHES (Line 205) and PROTO_DESCRIPTOR_KEY_HASHES (Line 212) accumulated forever across test invocations in the same process. Both are documented as "never pruned." Since the c5a_tests (Lines 1096-1174) rely on this helper for "deterministic unit tests," any future test that reuses a key/field name already inserted by an earlier test (e.g. "c5a_field_x", "c5a_render_method") will get a stale match and silently disable the guard regardless of that test's own setup — a source of order-dependent flakiness that the two current tests happen to avoid only because they use disjoint key names.

🔧 Proposed fix
 #[cfg(test)]
 pub(crate) fn test_reset_class_field_inline_guard() {
     PERRY_CLASS_FIELD_INLINE_GUARD_DISABLED.store(0, Ordering::Relaxed);
+    if let Ok(mut guard) = DECLARED_FIELD_NAME_HASHES.write() {
+        guard.take();
+    }
+    if let Ok(mut guard) = PROTO_DESCRIPTOR_KEY_HASHES.write() {
+        guard.take();
+    }
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/perry-runtime/src/object/descriptor_state.rs` around lines 147 - 151,
Update test_reset_class_field_inline_guard to also clear
DECLARED_FIELD_NAME_HASHES and PROTO_DESCRIPTOR_KEY_HASHES, preserving the
helper’s deterministic reset behavior for all class-field inline guard state.

/// #5654: flip the process-wide inline gate only when the descriptor target can
/// intercept a `this.field` access that the inline precheck cannot reject on
/// its own. Receiver-level installs are visible to the precheck via
Expand All @@ -165,13 +170,85 @@ pub(crate) fn class_field_inline_guard_enabled() -> bool {
///
/// The prototype-registry probes scan by value (O(#classes)); descriptor
/// installs are rare and never on the hot property path, so the scan cost is
/// acceptable. Finer granularity (flip only when the key collides with a
/// declared field of that class hierarchy) is possible follow-up work.
pub(crate) fn disable_class_field_inline_guard_for_target(obj: usize) {
/// acceptable.
///
/// #6759 C5a — per-KEY refinement (the follow-up the paragraph above used to
/// promise): the inline fast path only ever compiles accesses to DECLARED
/// instance fields, so a prototype-level install whose key names no declared
/// field of any registered class cannot affect anything the inline path
/// handles — babel-style method installs (`defineProperty(C.prototype,
/// "render", …)`) no longer poison the process. The vetting set holds FNV
/// hashes of every declared field name (harvested by
/// `remember_class_keys_array` at class registration); a collision merely
/// disables — never skips — so it stays conservative. Module-init ordering
/// is covered in both directions: installs that precede a class's
/// registration are recorded in [`PROTO_DESCRIPTOR_KEY_HASHES`] and
/// retro-checked by [`note_declared_instance_field_name`] when the class
/// arrives.
pub(crate) fn disable_class_field_inline_guard_for_target(obj: usize, key: &str) {
if crate::array::object_prototype_addr_matches(obj)
|| class_registry::is_registered_class_prototype_object(obj)
|| class_registry::class_id_for_decl_prototype_object(obj).is_some()
{
let hash = super::key_bytes_hash(key.as_ptr(), key.len());
note_proto_descriptor_key_hash(hash);
if declared_field_name_hash_exists(hash) {
disable_class_field_inline_guard();
}
}
}

/// #6759 C5a: FNV hashes of every declared instance-field name across all
/// registered classes. Written at class registration (cold), read at
/// prototype-level descriptor installs (rare). Never pruned — class
/// registrations are process-lifetime.
static DECLARED_FIELD_NAME_HASHES: std::sync::RwLock<Option<std::collections::HashSet<u64>>> =
std::sync::RwLock::new(None);

/// #6759 C5a: FNV hashes of every key installed on a prototype-level
/// descriptor target, so a class that registers AFTER such an install can
/// retro-trigger the disable (see
/// [`disable_class_field_inline_guard_for_target`]).
static PROTO_DESCRIPTOR_KEY_HASHES: std::sync::RwLock<Option<std::collections::HashSet<u64>>> =
std::sync::RwLock::new(None);

fn declared_field_name_hash_exists(hash: u64) -> bool {
DECLARED_FIELD_NAME_HASHES
.read()
.map(|g| g.as_ref().is_some_and(|s| s.contains(&hash)))
// Lock poisoned: be conservative (disable rather than skip).
.unwrap_or(true)
}

fn note_proto_descriptor_key_hash(hash: u64) {
if let Ok(mut guard) = PROTO_DESCRIPTOR_KEY_HASHES.write() {
guard
.get_or_insert_with(std::collections::HashSet::new)
.insert(hash);
} else {
// Lock poisoned: the retro-check can no longer see this key —
// take the conservative disable now.
disable_class_field_inline_guard();
}
}

/// #6759 C5a: called by `remember_class_keys_array` for each declared
/// instance-field name of a registering class. Records the name hash and
/// retro-checks it against prototype-level descriptor keys installed
/// earlier (which skipped the disable because no class had declared the
/// name yet).
pub(crate) fn note_declared_instance_field_name(name: &[u8]) {
let hash = super::key_bytes_hash(name.as_ptr(), name.len());
if let Ok(mut guard) = DECLARED_FIELD_NAME_HASHES.write() {
guard
.get_or_insert_with(std::collections::HashSet::new)
.insert(hash);
}
let installed_earlier = PROTO_DESCRIPTOR_KEY_HASHES
.read()
.map(|g| g.as_ref().is_some_and(|s| s.contains(&hash)))
.unwrap_or(true);
if installed_earlier {
disable_class_field_inline_guard();
}
}
Expand Down Expand Up @@ -582,7 +659,7 @@ pub(crate) fn set_property_attrs(obj: usize, key: String, attrs: PropertyAttrs)
let st = state();
st.descriptors.property_attrs_in_use.set(true);
GLOBAL_DESCRIPTORS_IN_USE.store(true, Ordering::Relaxed);
disable_class_field_inline_guard_for_target(obj);
disable_class_field_inline_guard_for_target(obj, &key);
note_meta_descriptor_key(obj, &key, false);
st.descriptors
.property_descriptors
Expand Down Expand Up @@ -752,7 +829,7 @@ pub(crate) fn set_accessor_descriptor(obj: usize, key: String, acc: AccessorDesc
let st = state();
st.descriptors.accessors_in_use.set(true);
GLOBAL_DESCRIPTORS_IN_USE.store(true, Ordering::Relaxed);
disable_class_field_inline_guard_for_target(obj);
disable_class_field_inline_guard_for_target(obj, &key);
note_accessor_descriptor_key(&key);
note_meta_descriptor_key(obj, &key, true);
st.descriptors
Expand Down Expand Up @@ -1016,3 +1093,82 @@ pub(crate) fn scan_descriptor_roots_mut(visitor: &mut crate::gc::RuntimeRootVisi
}
}
}

#[cfg(test)]
mod c5a_tests {
use super::*;

/// #6759 C5a: a prototype-level descriptor whose key names no declared
/// instance field must NOT flip the process-wide inline-guard disable;
/// one whose key IS a declared field must.
#[test]
fn inline_guard_disable_is_per_declared_field_key() {
let _lock = crate::gc::global_side_table_test_lock();
test_reset_class_field_inline_guard();

let proto = crate::object::js_object_alloc(0, 0);
class_registry::class_prototype_object_root_store(0x0666_0001, proto);
let proto_addr = proto as usize;

// Method-style install (babel output): key declared by no class.
set_accessor_descriptor(
proto_addr,
"c5a_render_method".to_string(),
AccessorDescriptor::default(),
);
assert!(
class_field_inline_guard_enabled(),
"a prototype install keyed by a non-field name must not poison \
the inline class-field fast path"
);

// Field-style install: key declared by a registered class.
note_declared_instance_field_name(b"c5a_field_x");
assert!(
class_field_inline_guard_enabled(),
"declaring the field alone (no matching install) must not disable"
);
set_property_attrs(
proto_addr,
"c5a_field_x".to_string(),
PropertyAttrs::new(false, true, true),
);
assert!(
!class_field_inline_guard_enabled(),
"a prototype install keyed by a DECLARED field must disable"
);

test_reset_class_field_inline_guard();
}

/// #6759 C5a ordering: an install that precedes the declaring class's
/// registration is retro-checked when the class arrives.
#[test]
fn inline_guard_retro_disable_on_late_class_registration() {
let _lock = crate::gc::global_side_table_test_lock();
test_reset_class_field_inline_guard();

let proto = crate::object::js_object_alloc(0, 0);
class_registry::class_prototype_object_root_store(0x0666_0002, proto);

set_accessor_descriptor(
proto as usize,
"c5a_late_field".to_string(),
AccessorDescriptor::default(),
);
assert!(
class_field_inline_guard_enabled(),
"no class declares the key yet — install must skip the disable"
);

// The declaring class registers AFTER the install.
note_declared_instance_field_name(b"c5a_late_field");
assert!(
!class_field_inline_guard_enabled(),
"late class registration must retro-trigger the disable for \
prototype keys installed earlier"
);

test_reset_class_field_inline_guard();
}
}
Loading