From 70847c7d7f1754e40443e0d0a38d7ca725b22f82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Tue, 11 Aug 2026 14:55:56 +0200 Subject: [PATCH 1/9] perf(class): widen the class-field shape guard to the subclass closure WIP --- .../src/expr/class_field_inline_guard.rs | 153 +++++++++++++++++- crates/perry-codegen/src/expr/mod.rs | 2 +- crates/perry-codegen/src/expr/property_get.rs | 9 ++ .../src/expr/property_get/helpers.rs | 8 + crates/perry-codegen/src/expr/property_set.rs | 36 ++++- 5 files changed, 204 insertions(+), 4 deletions(-) diff --git a/crates/perry-codegen/src/expr/class_field_inline_guard.rs b/crates/perry-codegen/src/expr/class_field_inline_guard.rs index 0faacbbc9c..22cebd1948 100644 --- a/crates/perry-codegen/src/expr/class_field_inline_guard.rs +++ b/crates/perry-codegen/src/expr/class_field_inline_guard.rs @@ -54,6 +54,135 @@ const OBJ_FLAG_HAS_DESCRIPTORS_BIT: &str = "2048"; // OBJ_FLAG_HAS_DESCRIPTORS ( const OBJ_FLAG_FROZEN_OR_DESCRIPTORS: &str = "2049"; const F64_EXP_MASK: &str = "9218868437227405312"; // 0x7FF0_0000_0000_0000 +/// A widening arm for the class-field shape check: one concrete subclass whose +/// instances put `property` at the SAME packed slot as the declared class does. +/// +/// `keys_global` names the module global holding that subclass's canonical keys +/// array; `class_id` is its registered class id. +#[derive(Clone, Debug)] +pub(crate) struct ClassFieldSubclassArm { + pub class_id: u32, + pub keys_global: String, +} + +/// A hierarchy wider than this turns the shape check into a longer compare +/// chain than the by-name fallback it replaces. Matches the dispatch-side cap +/// in `lower_call/property_get/dynamic_dispatch.rs`. +const MAX_CLASS_FIELD_SUBCLASS_ARMS: usize = 8; + +/// Every transitive subclass of `class_name` that agrees with it about +/// `property`'s slot — i.e. every receiver the field fast path may accept +/// beyond the declared class itself. +/// +/// ## Why this exists +/// +/// `emit_class_field_inline_precheck` (and the runtime `class_field_fast_contract` +/// behind it) speculates that the receiver's dynamic class is EXACTLY the +/// expression's declared class. Inside a base class's own constructor or method +/// that bet is not merely unreliable, it is **guaranteed wrong**: `this` in +/// `Node2D`'s constructor is only ever reached through `super(...)` from a +/// subclass, so the class-id compare fails on every single store and each +/// `this.x = x` pays a full by-name `js_put_value_set`. The same holds for every +/// inherited read — a `Node2D` getter reading `this.x` misses 100% of the time. +/// +/// This is the field-side counterpart of the dispatch widening in +/// `lower_call/property_get/dynamic_dispatch.rs` (#7800): one shape probe, +/// several (class id, keys) pairs. +/// +/// ## Why it is sound +/// +/// `class_field_global_index` lays a class out as its init chain's keyable +/// fields, root → leaf — parent fields first — so an inherited field keeps its +/// index in every subclass. That is a property of the layout algorithm, not a +/// promise, so this **re-derives the index for each candidate** and drops any +/// subclass that disagrees (a shadowing re-declaration lands at its own slot, +/// and an accessor anywhere on the chain makes `class_field_global_index` +/// return `None`). The raw-f64 candidacy of the declared type is likewise +/// re-checked per subclass: the fast path reads/writes the slot as a bare +/// double, and the per-object typed-layout intact bit only licenses that for a +/// field the *matched* class declares as a raw-f64 candidate. +pub(crate) fn class_field_subclass_arms( + ctx: &FnCtx<'_>, + class_name: &str, + property: &str, + field_index: u32, + requires_raw_f64: bool, +) -> Vec { + let Some(&declared_id) = ctx.class_ids.get(class_name) else { + return Vec::new(); + }; + // Deterministic order: class id, then name. Codegen output must be + // byte-reproducible (the corpus `cmp` A/B depends on it). + let mut candidates: Vec<(&String, u32)> = ctx.class_ids.iter().map(|(k, &v)| (k, v)).collect(); + candidates.sort_unstable_by(|a, b| a.1.cmp(&b.1).then_with(|| a.0.cmp(b.0))); + + let mut arms: Vec = Vec::new(); + let mut seen_ids: Vec = vec![declared_id]; + for (sub_name, sub_id) in candidates { + if sub_name == class_name || sub_id == 0 || seen_ids.contains(&sub_id) { + continue; + } + if !is_transitive_subclass(ctx, sub_name, class_name) { + continue; + } + // A class with computed runtime members has keys the packed layout + // does not describe; its sets route through the by-name path anyway. + if super::property_set::class_has_computed_runtime_members(ctx, sub_name) { + continue; + } + // The layout algorithm SHOULD put an inherited field at the same index + // in every subclass. Verify rather than assume — a shadowing + // re-declaration or an accessor on the subclass chain breaks it. + if crate::type_analysis::class_field_global_index(ctx, sub_name, property) + != Some(field_index) + { + continue; + } + // The fast path's representation choice (raw double vs NaN-boxed) is + // fixed at this site, so a subclass whose declared type disagrees would + // have the slot read at the wrong representation. + let sub_raw_f64 = crate::type_analysis::class_field_declared_type(ctx, sub_name, property) + .as_ref() + .is_some_and(crate::typed_shape::type_is_raw_f64_candidate); + if sub_raw_f64 != requires_raw_f64 { + continue; + } + let Some(keys_global) = ctx.class_keys_globals.get(sub_name).cloned() else { + continue; + }; + seen_ids.push(sub_id); + arms.push(ClassFieldSubclassArm { + class_id: sub_id, + keys_global, + }); + if arms.len() > MAX_CLASS_FIELD_SUBCLASS_ARMS { + return Vec::new(); + } + } + arms +} + +/// Is `name` a transitive subclass of `ancestor`? Cycle- and depth-guarded: +/// heavily-modular packages declare same-named classes across modules, and the +/// name-keyed `ctx.classes` can then form a parent cycle (see +/// `type_analysis_class_fields.rs`). +fn is_transitive_subclass(ctx: &FnCtx<'_>, name: &str, ancestor: &str) -> bool { + let mut seen: std::collections::HashSet = std::collections::HashSet::new(); + let mut parent = ctx.classes.get(name).and_then(|c| c.extends_name.clone()); + let mut depth = 0usize; + while let Some(p) = parent { + depth += 1; + if depth > 64 || !seen.insert(p.clone()) { + return false; + } + if p == ancestor { + return true; + } + parent = ctx.classes.get(&p).and_then(|c| c.extends_name.clone()); + } + false +} + /// Emit the `i1` "plain finite number" predicate on a value's raw bits: true /// iff the exponent field is not all-ones. Rejects ±Inf, every NaN (canonical /// or boxed), and therefore every NaN-box tag — exactly the values the @@ -312,6 +441,13 @@ pub(crate) fn emit_proven_shape_recheck( /// adds the not-frozen and plain-finite-number checks the set fast contract /// requires (a non-number must downgrade through the boxed setter, never a raw /// store). +/// +/// `subclass_arms` widens the shape test from "is exactly the declared class" +/// to "is the declared class or one of these subclasses, each of which puts +/// this property at this same slot" — see [`class_field_subclass_arms`] for why +/// the narrow form misses 100% of the time in a base-class body. Pass an empty +/// slice to keep the single-pair check; a class with no subclasses emits +/// byte-identical IR either way. #[allow(clippy::too_many_arguments)] pub(crate) fn emit_class_field_inline_precheck( ctx: &mut FnCtx, @@ -323,6 +459,7 @@ pub(crate) fn emit_class_field_inline_precheck( require_raw_f64: bool, set_value_bits: Option<&str>, fast_label: &str, + subclass_arms: &[ClassFieldSubclassArm], ) -> String { let deref_idx = ctx.new_block("class_field_inline.deref"); let guardcall_idx = ctx.new_block("class_field_inline.guardcall"); @@ -391,13 +528,25 @@ pub(crate) fn emit_class_field_inline_precheck( let keys_array = blk.load(I64, &ka_ptr); let ka_ok = blk.icmp_eq(I64, &keys_array, expected_keys); + // The declared class's own (class id, keys) pair, OR any subclass arm's. + // Each arm is a full pair — matching a class id without its canonical + // keys array would accept an instance that has since grown a property + // and no longer has the packed layout this slot index describes. + let mut shape_ok = blk.and(I1, &cid_ok, &ka_ok); + for arm in subclass_arms { + let arm_cid_ok = blk.icmp_eq(I32, &class_id, &arm.class_id.to_string()); + let arm_keys = blk.load(I64, &format!("@{}", arm.keys_global)); + let arm_ka_ok = blk.icmp_eq(I64, &keys_array, &arm_keys); + let arm_ok = blk.and(I1, &arm_cid_ok, &arm_ka_ok); + shape_ok = blk.or(I1, &shape_ok, &arm_ok); + } + // (The process-global enable flag was already checked at the gate above, // before this dereference.) let mut acc = blk.and(I1, >ype_ok, ¬_fwd); acc = blk.and(I1, &acc, &ot_ok); - acc = blk.and(I1, &acc, &cid_ok); + acc = blk.and(I1, &acc, &shape_ok); acc = blk.and(I1, &acc, &fc_ok); - acc = blk.and(I1, &acc, &ka_ok); // #5654: a receiver that has ever had a property / accessor descriptor // installed on it (Object.defineProperty / freeze / seal) needs the diff --git a/crates/perry-codegen/src/expr/mod.rs b/crates/perry-codegen/src/expr/mod.rs index cab44dd775..cdd6adc2f3 100644 --- a/crates/perry-codegen/src/expr/mod.rs +++ b/crates/perry-codegen/src/expr/mod.rs @@ -1990,7 +1990,7 @@ mod new_dynamic; mod objects_arrays_lit; mod os_uri_dates; pub(crate) mod property_get; -mod property_set; +pub(crate) mod property_set; pub(crate) mod proxy_reflect; mod static_field_meta; mod static_method; diff --git a/crates/perry-codegen/src/expr/property_get.rs b/crates/perry-codegen/src/expr/property_get.rs index 5ecaaf38b8..11d4add386 100644 --- a/crates/perry-codegen/src/expr/property_get.rs +++ b/crates/perry-codegen/src/expr/property_get.rs @@ -1607,6 +1607,14 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // branches straight to the fast slot load, skipping the // cross-crate guard call; on a miss it leaves the current // block at the guard-call path below (unchanged). + let subclass_arms = + crate::expr::class_field_inline_guard::class_field_subclass_arms( + ctx, + &class_name, + property, + field_index, + requires_raw_f64, + ); let _guardcall_label = crate::expr::class_field_inline_guard::emit_class_field_inline_precheck( ctx, @@ -1618,6 +1626,7 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { requires_raw_f64, None, &fast_label, + &subclass_arms, ); let guard_ok = ctx.block().call( I32, diff --git a/crates/perry-codegen/src/expr/property_get/helpers.rs b/crates/perry-codegen/src/expr/property_get/helpers.rs index b05c912ee1..79f8d28694 100644 --- a/crates/perry-codegen/src/expr/property_get/helpers.rs +++ b/crates/perry-codegen/src/expr/property_get/helpers.rs @@ -736,6 +736,13 @@ pub(crate) fn lower_raw_f64_class_field_get_for_number_context( let fallback_label = ctx.block_label(fallback_idx); let merge_label = ctx.block_label(merge_idx); + let subclass_arms = crate::expr::class_field_inline_guard::class_field_subclass_arms( + ctx, + &class_name, + property, + field_index, + true, + ); let _guardcall_label = crate::expr::class_field_inline_guard::emit_class_field_inline_precheck( ctx, &obj_bits, @@ -746,6 +753,7 @@ pub(crate) fn lower_raw_f64_class_field_get_for_number_context( true, None, &fast_label, + &subclass_arms, ); let guard_ok = ctx.block().call( I32, diff --git a/crates/perry-codegen/src/expr/property_set.rs b/crates/perry-codegen/src/expr/property_set.rs index 1cb86f2f18..d053098ea9 100644 --- a/crates/perry-codegen/src/expr/property_set.rs +++ b/crates/perry-codegen/src/expr/property_set.rs @@ -54,7 +54,7 @@ fn canonicalize_raw_f64_numeric_store_value( ) } -fn class_has_computed_runtime_members(ctx: &FnCtx<'_>, class_name: &str) -> bool { +pub(crate) fn class_has_computed_runtime_members(ctx: &FnCtx<'_>, class_name: &str) -> bool { ctx.classes .get(class_name) .is_some_and(|class| !class.computed_members.is_empty()) @@ -280,6 +280,13 @@ pub(crate) fn try_lower_sloppy_class_field_store( // Emits the shape/flags/value precheck and branches to `fast_label` on a // hit; leaves `ctx.current_block` on the freshly created miss block. + let subclass_arms = crate::expr::class_field_inline_guard::class_field_subclass_arms( + ctx, + &class_name, + property, + field_index, + true, + ); let _miss_label = crate::expr::class_field_inline_guard::emit_class_field_inline_precheck( ctx, &obj_bits, @@ -290,6 +297,7 @@ pub(crate) fn try_lower_sloppy_class_field_store( true, Some(&val_bits), &fast_label, + &subclass_arms, ); // Miss: the strict-aware runtime with `strict = 0`, so a rejected write @@ -408,6 +416,13 @@ fn try_lower_sloppy_class_field_boxed_store( // `set_value_bits` is `Some` so the not-frozen check is emitted; // `require_raw_f64` is false, so the plain-finite value check is not. + let subclass_arms = crate::expr::class_field_inline_guard::class_field_subclass_arms( + ctx, + class_name, + property, + field_index, + false, + ); let _miss_label = crate::expr::class_field_inline_guard::emit_class_field_inline_precheck( ctx, &obj_bits, @@ -418,6 +433,7 @@ fn try_lower_sloppy_class_field_boxed_store( false, Some(&val_bits), &fast_label, + &subclass_arms, ); { @@ -1287,6 +1303,23 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // the plain-finite value test (a boxed slot accepts any // `JSValue`) but still proves not-frozen / no per-object // descriptors via `set_value_bits: Some`. + // + // #7861: and the shape test it emits is widened from the + // DECLARED class to that class's subclass closure. Without + // this the boxed arm #7854 just un-gated would still miss + // 100% of the time for a store in a base class's own + // constructor, where `this` is only ever a subclass. The + // arms are computed with `requires_raw_f64` rather than a + // literal, so a candidate whose declared type disagrees + // about the slot's representation is dropped. + let subclass_arms = + crate::expr::class_field_inline_guard::class_field_subclass_arms( + ctx, + &class_name, + property, + field_index, + requires_raw_f64, + ); let _guardcall_label = crate::expr::class_field_inline_guard::emit_class_field_inline_precheck( ctx, @@ -1298,6 +1331,7 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { requires_raw_f64, Some(&val_bits), &fast_label, + &subclass_arms, ); let guard_ok = ctx.block().call( I32, From 1282a9b5ad9e74e3a0cb430d664919d70a1d6d1a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Tue, 11 Aug 2026 15:28:39 +0200 Subject: [PATCH 2/9] wip: temporary class-field guard miss counters (to be reverted) --- crates/perry-runtime/src/proxy/put_value.rs | 1 + crates/perry-runtime/src/typed_feedback.rs | 1 + .../src/typed_feedback/guard_counters.rs | 71 +++++++++++++++++++ .../src/typed_feedback/guards.rs | 48 ++++++++++--- 4 files changed, 113 insertions(+), 8 deletions(-) create mode 100644 crates/perry-runtime/src/typed_feedback/guard_counters.rs diff --git a/crates/perry-runtime/src/proxy/put_value.rs b/crates/perry-runtime/src/proxy/put_value.rs index a07e9cadeb..bdec3829db 100644 --- a/crates/perry-runtime/src/proxy/put_value.rs +++ b/crates/perry-runtime/src/proxy/put_value.rs @@ -107,6 +107,7 @@ pub extern "C" fn js_put_value_set( receiver: f64, strict: i32, ) -> f64 { + crate::typed_feedback::guard_counters::bump(4); // Sloppy script assignment lowers to PutValue rather than the named-field // setter. Existing own data fields need none of PutValue's rooting, // ToPropertyKey, Proxy, typed-array, or receiver-aware prototype work. diff --git a/crates/perry-runtime/src/typed_feedback.rs b/crates/perry-runtime/src/typed_feedback.rs index 5d7bfc768a..354ed9f5ef 100644 --- a/crates/perry-runtime/src/typed_feedback.rs +++ b/crates/perry-runtime/src/typed_feedback.rs @@ -1076,6 +1076,7 @@ pub extern "C" fn js_typed_feedback_object_set_field_by_name_fast( } #[path = "typed_feedback/guards.rs"] +pub(crate) mod guard_counters; mod guards; pub use guards::{ js_typed_feedback_class_field_get_guard, js_typed_feedback_class_field_set_guard, diff --git a/crates/perry-runtime/src/typed_feedback/guard_counters.rs b/crates/perry-runtime/src/typed_feedback/guard_counters.rs new file mode 100644 index 0000000000..4af8beb065 --- /dev/null +++ b/crates/perry-runtime/src/typed_feedback/guard_counters.rs @@ -0,0 +1,71 @@ +//! TEMPORARY (shapes campaign): per-precondition miss counters for the +//! class-field IC, so a widened guard can be shown to be TAKEN rather than +//! merely EMITTED. +//! +//! Off unless `PERRY_CLASS_FIELD_COUNTERS=1`. Reverted before the PR. + +use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; + +pub const N: usize = 12; +pub static C: [AtomicU64; N] = [ + AtomicU64::new(0), + AtomicU64::new(0), + AtomicU64::new(0), + AtomicU64::new(0), + AtomicU64::new(0), + AtomicU64::new(0), + AtomicU64::new(0), + AtomicU64::new(0), + AtomicU64::new(0), + AtomicU64::new(0), + AtomicU64::new(0), + AtomicU64::new(0), +]; + +pub const NAMES: [&str; N] = [ + "set_guard_calls", // 0: runtime set guard entered (= inline precheck MISS) + "set_guard_pass", // 1: ... and the runtime contract accepted + "get_guard_calls", // 2 + "get_guard_pass", // 3 + "put_value_set_calls", // 4: sloppy-arm inline precheck MISS + "contract_notobj", // 5: fast_contract: not a heap object / bad header + "contract_cid", // 6: fast_contract: class_id mismatch + "contract_keys", // 7: fast_contract: keys_array mismatch + "contract_fieldcount", // 8: fast_contract: field_index >= field_count + "contract_rawf64", // 9: fast_contract: side table says slot is not raw-f64 + "set_frozen", // 10: set contract: frozen + "set_notplain", // 11: set contract: value is not a plain number (INT32-boxed etc.) +]; + +static ENABLED: AtomicBool = AtomicBool::new(false); +static INIT: std::sync::Once = std::sync::Once::new(); + +#[inline] +pub fn enabled() -> bool { + INIT.call_once(|| { + let on = std::env::var("PERRY_CLASS_FIELD_COUNTERS") + .map(|v| matches!(v.trim(), "1" | "true" | "on" | "yes")) + .unwrap_or(false); + ENABLED.store(on, Ordering::Relaxed); + if on { + extern "C" fn report() { + eprint!("[class-field]"); + for i in 0..N { + eprint!(" {}={}", NAMES[i], C[i].load(Ordering::Relaxed)); + } + eprintln!(); + } + unsafe { + libc::atexit(report); + } + } + }); + ENABLED.load(Ordering::Relaxed) +} + +#[inline] +pub fn bump(i: usize) { + if enabled() { + C[i].fetch_add(1, Ordering::Relaxed); + } +} diff --git a/crates/perry-runtime/src/typed_feedback/guards.rs b/crates/perry-runtime/src/typed_feedback/guards.rs index 621ac1be8a..d117bc4865 100644 --- a/crates/perry-runtime/src/typed_feedback/guards.rs +++ b/crates/perry-runtime/src/typed_feedback/guards.rs @@ -308,6 +308,18 @@ fn class_field_fast_contract( && (*obj).class_id == expected_class_id && std::ptr::eq((*obj).keys_array as *const ArrayHeader, expected_keys) && expected_field_index < (*obj).field_count; + if super::guard_counters::enabled() && !shape_ok { + use super::guard_counters::bump; + if (*obj).object_type != crate::error::OBJECT_TYPE_REGULAR { + bump(5); + } else if (*obj).class_id != expected_class_id { + bump(6); + } else if !std::ptr::eq((*obj).keys_array as *const ArrayHeader, expected_keys) { + bump(7); + } else { + bump(8); + } + } // #5093 self-check: the codegen-inlined fast path concludes "slot K is // raw-f64" purely from the per-object intact bit (plus a class_id/keys // match). Under PERRY_VERIFY_TYPED_INTACT=1, assert that whenever this @@ -329,12 +341,17 @@ fn class_field_fast_contract( std::process::abort(); } } + if shape_ok + && require_raw_f64 + && !crate::gc::layout_typed_raw_f64_slot_for_user( + object_addr, + expected_field_index as usize, + ) + { + super::guard_counters::bump(9); + return false; + } shape_ok - && (!require_raw_f64 - || crate::gc::layout_typed_raw_f64_slot_for_user( - object_addr, - expected_field_index as usize, - )) } } @@ -378,14 +395,19 @@ pub extern "C" fn js_typed_feedback_class_field_get_guard( expected_field_index: u32, require_raw_f64: i32, ) -> i32 { + super::guard_counters::bump(2); if !typed_feedback_enabled() && !crate::object::descriptors_in_use() { - return class_field_fast_contract( + let r = class_field_fast_contract( receiver, expected_class_id, expected_keys, expected_field_index, require_raw_f64 != 0, ) as i32; + if r != 0 { + super::guard_counters::bump(3); + } + return r; } let (shape_addr, class_id, gc_type, contract_valid) = class_field_get_contract( receiver, @@ -441,10 +463,15 @@ fn class_field_set_fast_contract( return false; }; if (*gc_header)._reserved & crate::gc::OBJ_FLAG_FROZEN != 0 { + super::guard_counters::bump(10); return false; } } - !require_raw_f64 || is_plain_number_bits(value_bits) + if require_raw_f64 && !is_plain_number_bits(value_bits) { + super::guard_counters::bump(11); + return false; + } + true } fn descriptor_blocks_class_field_set(obj_addr: usize, class_id: u32, key_name: &str) -> bool { @@ -550,8 +577,9 @@ pub extern "C" fn js_typed_feedback_class_field_set_guard( require_raw_f64: i32, ) -> i32 { let value_bits = value.to_bits(); + super::guard_counters::bump(0); if !typed_feedback_enabled() && !crate::object::descriptors_in_use() { - return class_field_set_fast_contract( + let r = class_field_set_fast_contract( receiver, expected_class_id, expected_keys, @@ -559,6 +587,10 @@ pub extern "C" fn js_typed_feedback_class_field_set_guard( require_raw_f64 != 0, value_bits, ) as i32; + if r != 0 { + super::guard_counters::bump(1); + } + return r; } let (shape_addr, class_id, gc_type, contract_valid) = class_field_set_contract( receiver, From 773679c9b5d5b187fd03c3f05bfb506091564f05 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Tue, 11 Aug 2026 15:50:55 +0200 Subject: [PATCH 3/9] wip: keep pre-widening and-chain when the closure is empty (byte-identical IR) --- .../src/expr/class_field_inline_guard.rs | 41 ++++++++++++------- crates/perry-runtime/src/typed_feedback.rs | 3 +- 2 files changed, 28 insertions(+), 16 deletions(-) diff --git a/crates/perry-codegen/src/expr/class_field_inline_guard.rs b/crates/perry-codegen/src/expr/class_field_inline_guard.rs index 22cebd1948..fd7e8c49a0 100644 --- a/crates/perry-codegen/src/expr/class_field_inline_guard.rs +++ b/crates/perry-codegen/src/expr/class_field_inline_guard.rs @@ -528,25 +528,36 @@ pub(crate) fn emit_class_field_inline_precheck( let keys_array = blk.load(I64, &ka_ptr); let ka_ok = blk.icmp_eq(I64, &keys_array, expected_keys); - // The declared class's own (class id, keys) pair, OR any subclass arm's. - // Each arm is a full pair — matching a class id without its canonical - // keys array would accept an instance that has since grown a property - // and no longer has the packed layout this slot index describes. - let mut shape_ok = blk.and(I1, &cid_ok, &ka_ok); - for arm in subclass_arms { - let arm_cid_ok = blk.icmp_eq(I32, &class_id, &arm.class_id.to_string()); - let arm_keys = blk.load(I64, &format!("@{}", arm.keys_global)); - let arm_ka_ok = blk.icmp_eq(I64, &keys_array, &arm_keys); - let arm_ok = blk.and(I1, &arm_cid_ok, &arm_ka_ok); - shape_ok = blk.or(I1, &shape_ok, &arm_ok); - } - // (The process-global enable flag was already checked at the gate above, // before this dereference.) let mut acc = blk.and(I1, >ype_ok, ¬_fwd); acc = blk.and(I1, &acc, &ot_ok); - acc = blk.and(I1, &acc, &shape_ok); - acc = blk.and(I1, &acc, &fc_ok); + if subclass_arms.is_empty() { + // Byte-for-byte the pre-widening and-chain. A class with no + // eligible subclass must emit IDENTICAL IR, so the corpus-wide + // `cmp` stays a usable no-regression instrument (a reordered + // and-chain alone made 17 of 19 corpus binaries differ for no + // behavioural reason). + acc = blk.and(I1, &acc, &cid_ok); + acc = blk.and(I1, &acc, &fc_ok); + acc = blk.and(I1, &acc, &ka_ok); + } else { + // The declared class's own (class id, keys) pair, OR any subclass + // arm's. Each arm is a full pair — matching a class id without its + // canonical keys array would accept an instance that has since + // grown a property and no longer has the packed layout this slot + // index describes. + let mut shape_ok = blk.and(I1, &cid_ok, &ka_ok); + for arm in subclass_arms { + let arm_cid_ok = blk.icmp_eq(I32, &class_id, &arm.class_id.to_string()); + let arm_keys = blk.load(I64, &format!("@{}", arm.keys_global)); + let arm_ka_ok = blk.icmp_eq(I64, &keys_array, &arm_keys); + let arm_ok = blk.and(I1, &arm_cid_ok, &arm_ka_ok); + shape_ok = blk.or(I1, &shape_ok, &arm_ok); + } + acc = blk.and(I1, &acc, &shape_ok); + acc = blk.and(I1, &acc, &fc_ok); + } // #5654: a receiver that has ever had a property / accessor descriptor // installed on it (Object.defineProperty / freeze / seal) needs the diff --git a/crates/perry-runtime/src/typed_feedback.rs b/crates/perry-runtime/src/typed_feedback.rs index 354ed9f5ef..b156e09f3c 100644 --- a/crates/perry-runtime/src/typed_feedback.rs +++ b/crates/perry-runtime/src/typed_feedback.rs @@ -1075,8 +1075,9 @@ pub extern "C" fn js_typed_feedback_object_set_field_by_name_fast( } } -#[path = "typed_feedback/guards.rs"] +#[path = "typed_feedback/guard_counters.rs"] pub(crate) mod guard_counters; +#[path = "typed_feedback/guards.rs"] mod guards; pub use guards::{ js_typed_feedback_class_field_get_guard, js_typed_feedback_class_field_set_guard, From dbafd5a0a1b7ef3c4f12828553606d60e8055097 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Tue, 11 Aug 2026 16:02:06 +0200 Subject: [PATCH 4/9] perf(class): declare the typed shape at allocation for subclassed chains (#7512 followup) --- .../src/lower_call/field_init.rs | 231 ++++++++++++++++-- .../src/lower_call/typed_shape_init.rs | 16 +- crates/perry-codegen/src/typed_shape.rs | 55 +++++ 3 files changed, 279 insertions(+), 23 deletions(-) diff --git a/crates/perry-codegen/src/lower_call/field_init.rs b/crates/perry-codegen/src/lower_call/field_init.rs index d6c0e7a328..16f0ae0d0d 100644 --- a/crates/perry-codegen/src/lower_call/field_init.rs +++ b/crates/perry-codegen/src/lower_call/field_init.rs @@ -211,51 +211,216 @@ fn prologue_rhs_cannot_observe_this( pub(crate) fn ctor_prologue_param_assigned_fields( class: &perry_hir::Class, ) -> std::collections::HashSet { - let empty = std::collections::HashSet::new(); - if class.extends.is_some() - || class.extends_name.is_some() - || class.native_extends.is_some() + ctor_prologue_assigned_fields_inner(class, false).unwrap_or_default() +} + +/// Does `expr`'s subtree contain `this`? +/// +/// Recurses through [`perry_hir::walker::walk_expr_children`], which is +/// exhaustive over `Expr` and drift-checked against its `_mut` twin by +/// `walker_arms_match` — so a new expression variant cannot silently smuggle a +/// `This` past this scan. That completeness is the whole reason the trailing +/// region is screened at the EXPRESSION level; there is no equivalent shared +/// `Stmt` walker in the HIR, and hand-rolling one would make a missed variant a +/// silent wrong answer. +fn expr_mentions_this(expr: &Expr) -> bool { + if matches!(expr, Expr::This) { + return true; + } + let mut found = false; + perry_hir::walker::walk_expr_children(expr, &mut |child: &Expr| { + if !found && expr_mentions_this(child) { + found = true; + } + }); + found +} + +/// May `stmt` follow the prologue run without endangering a raw-f64 slot that +/// a LATER constructor on the chain has yet to write? +/// +/// A whitelist, deliberately: only `Stmt::Expr(e)` with no `this` anywhere in +/// `e`. Everything else — `Return` (would skip a later assignment), `If` / +/// loops / `Try` (would make the assignments conditional), `Let`, `Throw` — is +/// refused by not being matched, so adding a `Stmt` variant cannot widen this +/// by accident. `Shape.made = Shape.made + 1` is the motivating admission: a +/// static-field bump between `this.tag = tag` and the subclass's `this.w = w`, +/// which cannot reach the half-built instance because `this` never appears in +/// it. +fn stmt_is_this_free_expr(stmt: &Stmt) -> bool { + match stmt { + Stmt::Expr(e) => !expr_mentions_this(e), + _ => false, + } +} + +/// Is `stmt` a `super(...)` whose arguments cannot observe `this`? +fn stmt_is_safe_super_call(stmt: &Stmt, param_ids: &std::collections::HashSet) -> bool { + match stmt { + Stmt::Expr(Expr::SuperCall(args)) => args + .iter() + .all(|a| prologue_rhs_cannot_observe_this(a, param_ids)), + _ => false, + } +} + +/// `None` = the class is DISQUALIFIED (its constructor's effect on `this` +/// cannot be bounded). `Some(set)` = qualified, and `set` is the field names +/// its prologue unconditionally assigns — possibly EMPTY, which is a real and +/// useful answer for a fieldless subclass like `Marker` whose whole body is +/// `super(x, y)`. The two are different facts and the pre-#7512-followup code +/// conflated them into one empty set, which is why a chain could not be +/// analysed a class at a time. +fn ctor_prologue_assigned_fields_inner( + class: &perry_hir::Class, + allow_heritage: bool, +) -> Option> { + // A dynamic, native, or lexically-shadowed parent is out of scope in both + // arms: `super()` then runs a built-in or a runtime-resolved constructor + // whose effect on `this` this analysis cannot see. + if class.native_extends.is_some() || class.extends_expr.is_some() + || class.heritage_lexically_shadowed || !class.decorators.is_empty() { - return empty; + return None; } - let Some(ctor) = class.constructor.as_ref() else { - return empty; - }; + let has_heritage = class.extends.is_some() || class.extends_name.is_some(); + if has_heritage && !allow_heritage { + return None; + } + // Checked BEFORE the missing-constructor early return: a field initializer + // runs during the init phase whether or not the class declares a ctor, and + // it may legally read `this.` of an earlier field. let all_fields_bare = class.fields.iter().all(|f| { f.init.is_none() && f.key_expr.is_none() && f.decorators.is_empty() && !f.is_private }); if !all_fields_bare { - return empty; + return None; } + let Some(ctor) = class.constructor.as_ref() else { + // No constructor of its own: it assigns nothing, but it also cannot + // observe anything. A chain containing it is still analysable — its + // raw-f64 fields simply go uncovered, which the caller's coverage test + // then rejects. + return Some(std::collections::HashSet::new()); + }; let params_plain = ctor.params.iter().all(|p| { p.default.is_none() && !p.is_rest && p.decorators.is_empty() && p.arguments_object.is_none() }); if !params_plain { - return empty; + return None; } let param_ids: std::collections::HashSet<_> = ctor.params.iter().map(|p| p.id).collect(); let mut assigned = std::collections::HashSet::new(); - for stmt in &ctor.body { + let mut body = ctor.body.as_slice(); + if allow_heritage { + // A leading `super(...)` is not a prologue assignment, so the maximal + // leading run used to truncate at statement 0. Skip it — the argument + // check is what keeps the parent from being handed the half-built + // instance. + if let Some(first) = body.first() { + if stmt_is_safe_super_call(first, ¶m_ids) { + body = &body[1..]; + } else if has_heritage { + // A derived constructor that opens with anything else may run + // arbitrary code before `super()`. + return None; + } + } else if has_heritage { + return None; + } + } + let mut rest = body; + while let Some(stmt) = rest.first() { match prologue_assigned_field(stmt, ¶m_ids) { Some(property) => { assigned.insert(property.to_string()); + rest = &rest[1..]; } None => break, } } - if assigned.is_empty() { - return empty; + if allow_heritage { + // Everything after the run runs BEFORE a subclass's own field writes, + // so it must not be able to read a raw-f64 slot that is still holding + // the allocator's `undefined` fill. + if !rest.iter().all(stmt_is_this_free_expr) { + return None; + } } if class .setters .iter() .any(|(name, _)| assigned.contains(name)) { - return empty; + return None; } - assigned + Some(assigned) +} + +/// [`ctor_prologue_param_assigned_fields`] for a class that DOES extend a plain +/// user class — the shape the no-heritage rule above refuses outright. +/// +/// Refusing it is #7512 repeating one level up. A subclass instance never gets +/// an at-allocation typed-shape declaration, so *every* raw-f64 field store in +/// *every* constructor on its chain — including the base class's own +/// `this.x = x`, which is textually in a heritage-free class — misses its +/// `GC_OBJ_TYPED_LAYOUT_INTACT` guard and falls back to `js_put_value_set`. +/// Measured on `shapes.ts`: 528 000 by-name field stores, and a two-class +/// probe (`gc-handoff/bench/shapes_baseclass_field.ts`) runs **2.0x** slower +/// than the hand-flattened single class doing identical work. +/// +/// The extra obligations heritage brings, and how each is discharged: +/// +/// * **A leading `super(...)` is not a prologue assignment**, so the maximal +/// leading run truncated at statement 0 and came back empty. It is skipped +/// here instead — but only when every argument satisfies +/// [`prologue_rhs_cannot_observe_this`], so the parent constructor cannot be +/// handed the half-built instance. +/// * **A non-leaf constructor's trailing statements run BEFORE the leaf's field +/// writes.** `Shape`'s body finishes before `Rect` assigns `this.w`, so a +/// trailing `this.w` read there would see a raw-f64-masked slot that still +/// holds `undefined`'s NaN-box bits and yield `NaN` instead of `undefined`. +/// The heritage arm therefore requires the ctor body to contain **no `This` +/// at all after the prologue run** — a whole-subtree scan, not a top-level +/// one. (The no-heritage arm keeps its existing, laxer rule: a class that is +/// never extended has no later writer, and one that IS extended is caught by +/// this same check when the chain is walked from the leaf.) +/// * **An early `return` would skip a later assignment**, leaving a raw-f64 +/// field unwritten, so any `Return` anywhere in the body is rejected. +/// Per-class prologue sets for `leaf`'s whole inheritance chain, root → leaf, +/// or `None` if ANY class on it is disqualified. +pub(crate) fn chain_prologue_assigned_fields( + classes: &std::collections::HashMap, + leaf: &str, +) -> Option)>> { + let mut chain: Vec<&perry_hir::Class> = Vec::new(); + let mut cur = classes.get(leaf).copied(); + let mut seen: std::collections::HashSet = std::collections::HashSet::new(); + while let Some(c) = cur { + if !seen.insert(c.name.clone()) || chain.len() > 64 { + return None; + } + chain.push(c); + cur = match c.extends_name.as_deref() { + // A named parent this module cannot resolve is a class whose + // constructor we cannot analyse — refuse rather than assume it is + // inert. + Some(parent) => match classes.get(parent).copied() { + Some(pc) => Some(pc), + None => return None, + }, + None => None, + }; + } + chain.reverse(); + let mut out = Vec::with_capacity(chain.len()); + for c in chain { + let assigned = ctor_prologue_assigned_fields_inner(c, true)?; + out.push((c.name.clone(), assigned)); + } + Some(out) } /// Walk the inheritance chain from the root down and apply each class's @@ -312,6 +477,14 @@ pub(crate) fn apply_field_initializers_recursive( // message` on a `PropertySignature`). The authoritative chain is root → // leaf and carries each ancestor's resolved fields, so we use both its // ORDER (for the mode filter) and its FIELDS (per class below). + // #7512-followup: computed once for the LEAF, then consulted per class in + // the chain below. `Some` only when the chain form is what authorizes the + // at-allocation declaration, so a chain that stays on the old path keeps + // exactly its old elision set. + let chain_prologue_assigned: Option)>> = + chain_prologue_assigned_fields(ctx.classes, class_name).filter(|chain| { + crate::typed_shape::class_chain_layout_declarable_at_allocation(ctx.classes, chain) + }); let mut chain_field_override: std::collections::HashMap> = std::collections::HashMap::new(); // Collect the inheritance chain from root down. @@ -443,12 +616,28 @@ pub(crate) fn apply_field_initializers_recursive( // obligations. Computed from the leaf-authoritative `ctx.classes` entry // (an ancestor resolved only through `chain_field_override` has no // visible ctor here and gets the conservative empty set). - let prologue_assigned = ctx - .classes - .get(&class_name_in_chain) - .copied() - .map(ctor_prologue_param_assigned_fields) - .unwrap_or_default(); + // #7512-followup: when the LEAF's whole chain is declarable at + // allocation, the two consumers of this set must agree — the declared + // raw-f64 mask is live from birth, so a field-init `undefined` write + // into one of those slots would fail `layout_raw_f64_bits` and + // downgrade the descriptor on the spot, making the declaration + // worthless. So use the chain-aware per-class set exactly when the + // chain form is what authorized the declaration. + let prologue_assigned = chain_prologue_assigned + .as_ref() + .and_then(|chain| { + chain + .iter() + .find(|(name, _)| *name == class_name_in_chain) + .map(|(_, set)| set.clone()) + }) + .unwrap_or_else(|| { + ctx.classes + .get(&class_name_in_chain) + .copied() + .map(ctor_prologue_param_assigned_fields) + .unwrap_or_default() + }); let mut init_pairs: Vec<(String, Expr)> = Vec::new(); let mut init_pairs_computed: Vec<(Expr, Expr)> = Vec::new(); for field in &class_fields { diff --git a/crates/perry-codegen/src/lower_call/typed_shape_init.rs b/crates/perry-codegen/src/lower_call/typed_shape_init.rs index 4b29accb8c..c3d09c42b6 100644 --- a/crates/perry-codegen/src/lower_call/typed_shape_init.rs +++ b/crates/perry-codegen/src/lower_call/typed_shape_init.rs @@ -38,10 +38,22 @@ pub(super) fn layout_declared_at_allocation(ctx: &FnCtx<'_>, class_name: &str) - if !ctx.class_keys_globals.contains_key(class_name) { return false; } - ctx.classes.get(class_name).is_some_and(|class| { + let single = ctx.classes.get(class_name).is_some_and(|class| { let prologue = super::field_init::ctor_prologue_param_assigned_fields(class); crate::typed_shape::class_layout_declarable_at_allocation(class, &prologue) - }) + }); + if single { + return true; + } + // #7512-followup: the single-class rule refuses every class with heritage, + // which denies an at-allocation declaration to every subclass instance and + // puts every constructor store on the whole chain — the base class's own + // included — on the by-name fallback. Try the chain form. + super::field_init::chain_prologue_assigned_fields(ctx.classes, class_name).is_some_and( + |chain| { + crate::typed_shape::class_chain_layout_declarable_at_allocation(ctx.classes, &chain) + }, + ) } /// #7834: is `class_name`'s at-allocation declaration expressible as a diff --git a/crates/perry-codegen/src/typed_shape.rs b/crates/perry-codegen/src/typed_shape.rs index 9a92a2333e..34e9ba6807 100644 --- a/crates/perry-codegen/src/typed_shape.rs +++ b/crates/perry-codegen/src/typed_shape.rs @@ -133,6 +133,61 @@ pub(crate) fn type_is_raw_f64_candidate(ty: &Type) -> bool { /// (`is_plain_number_bits`, and the inline path's finite-exponent test), falls /// back to the boxed setter, and downgrades the descriptor through /// `layout_note_slot` — the same path any post-install contradiction takes. +/// [`class_layout_declarable_at_allocation`] for a whole inheritance chain. +/// +/// The single-class rule refuses every class with heritage, which is #7512 +/// one level up: a `Rect extends Shape extends Node2D` instance never gets an +/// at-allocation declaration, so every raw-f64 store in *every* constructor on +/// its chain — `Node2D`'s own `this.x = x` included, though `Node2D` itself +/// extends nothing — misses `GC_OBJ_TYPED_LAYOUT_INTACT` and falls back to +/// `js_put_value_set`. Counted on `shapes.ts`: 528 000 by-name field stores per +/// run. A two-class probe measures **2.0x** against the hand-flattened class. +/// +/// `chain` is `chain_prologue_assigned_fields`' root → leaf answer, which is +/// `None` unless every class on the chain is individually analysable. The +/// obligations are the single-class ones, restated over the chain: +/// +/// 1. **Every raw-f64 field ANYWHERE on the chain is prologue-assigned** — by +/// its own class, since that is the only constructor that writes it. One +/// uncovered field would be read as a double while it still holds +/// `undefined`'s NaN-box bits, yielding `NaN` instead of `undefined`. +/// 2. **Nothing during construction can read a field before its write.** Each +/// class's prologue RHS and `super()` arguments are `This`-free by +/// `prologue_rhs_cannot_observe_this`, and everything after a class's +/// prologue run is `This`-free by `stmt_is_this_free_expr` — which matters +/// precisely because a *non-leaf* constructor's trailing statements run +/// before the leaf assigns its own fields. +/// 3. **The collector's view is true at birth** — unchanged from the +/// single-class case: every allocation path prefills `TAG_UNDEFINED`, a +/// pointer-masked slot holding it is rejected at `mark_field_into_worklist`'s +/// tag check, and a raw-f64-masked slot is not visited at all. +pub(crate) fn class_chain_layout_declarable_at_allocation( + classes: &std::collections::HashMap, + chain: &[(String, std::collections::HashSet)], +) -> bool { + let mut worth_declaring = false; + for (class_name, prologue) in chain { + let Some(class) = classes.get(class_name).copied() else { + return false; + }; + for field in &class.fields { + if field.key_expr.is_some() { + continue; + } + if type_is_pointer_bearing(&field.ty) { + worth_declaring = true; + } + if type_is_raw_f64_candidate(&field.ty) { + worth_declaring = true; + if !prologue.contains(&field.name) { + return false; + } + } + } + } + worth_declaring +} + pub(crate) fn class_layout_declarable_at_allocation( class: &perry_hir::Class, prologue: &std::collections::HashSet, From 41f5596979755a23300651a633c9ee7626f18541 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Tue, 11 Aug 2026 16:14:41 +0200 Subject: [PATCH 5/9] Revert "wip: temporary class-field guard miss counters (to be reverted)" --- crates/perry-runtime/src/proxy/put_value.rs | 1 - crates/perry-runtime/src/typed_feedback.rs | 2 - .../src/typed_feedback/guard_counters.rs | 71 ------------------- .../src/typed_feedback/guards.rs | 48 +++---------- 4 files changed, 8 insertions(+), 114 deletions(-) delete mode 100644 crates/perry-runtime/src/typed_feedback/guard_counters.rs diff --git a/crates/perry-runtime/src/proxy/put_value.rs b/crates/perry-runtime/src/proxy/put_value.rs index bdec3829db..a07e9cadeb 100644 --- a/crates/perry-runtime/src/proxy/put_value.rs +++ b/crates/perry-runtime/src/proxy/put_value.rs @@ -107,7 +107,6 @@ pub extern "C" fn js_put_value_set( receiver: f64, strict: i32, ) -> f64 { - crate::typed_feedback::guard_counters::bump(4); // Sloppy script assignment lowers to PutValue rather than the named-field // setter. Existing own data fields need none of PutValue's rooting, // ToPropertyKey, Proxy, typed-array, or receiver-aware prototype work. diff --git a/crates/perry-runtime/src/typed_feedback.rs b/crates/perry-runtime/src/typed_feedback.rs index b156e09f3c..5d7bfc768a 100644 --- a/crates/perry-runtime/src/typed_feedback.rs +++ b/crates/perry-runtime/src/typed_feedback.rs @@ -1075,8 +1075,6 @@ pub extern "C" fn js_typed_feedback_object_set_field_by_name_fast( } } -#[path = "typed_feedback/guard_counters.rs"] -pub(crate) mod guard_counters; #[path = "typed_feedback/guards.rs"] mod guards; pub use guards::{ diff --git a/crates/perry-runtime/src/typed_feedback/guard_counters.rs b/crates/perry-runtime/src/typed_feedback/guard_counters.rs deleted file mode 100644 index 4af8beb065..0000000000 --- a/crates/perry-runtime/src/typed_feedback/guard_counters.rs +++ /dev/null @@ -1,71 +0,0 @@ -//! TEMPORARY (shapes campaign): per-precondition miss counters for the -//! class-field IC, so a widened guard can be shown to be TAKEN rather than -//! merely EMITTED. -//! -//! Off unless `PERRY_CLASS_FIELD_COUNTERS=1`. Reverted before the PR. - -use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; - -pub const N: usize = 12; -pub static C: [AtomicU64; N] = [ - AtomicU64::new(0), - AtomicU64::new(0), - AtomicU64::new(0), - AtomicU64::new(0), - AtomicU64::new(0), - AtomicU64::new(0), - AtomicU64::new(0), - AtomicU64::new(0), - AtomicU64::new(0), - AtomicU64::new(0), - AtomicU64::new(0), - AtomicU64::new(0), -]; - -pub const NAMES: [&str; N] = [ - "set_guard_calls", // 0: runtime set guard entered (= inline precheck MISS) - "set_guard_pass", // 1: ... and the runtime contract accepted - "get_guard_calls", // 2 - "get_guard_pass", // 3 - "put_value_set_calls", // 4: sloppy-arm inline precheck MISS - "contract_notobj", // 5: fast_contract: not a heap object / bad header - "contract_cid", // 6: fast_contract: class_id mismatch - "contract_keys", // 7: fast_contract: keys_array mismatch - "contract_fieldcount", // 8: fast_contract: field_index >= field_count - "contract_rawf64", // 9: fast_contract: side table says slot is not raw-f64 - "set_frozen", // 10: set contract: frozen - "set_notplain", // 11: set contract: value is not a plain number (INT32-boxed etc.) -]; - -static ENABLED: AtomicBool = AtomicBool::new(false); -static INIT: std::sync::Once = std::sync::Once::new(); - -#[inline] -pub fn enabled() -> bool { - INIT.call_once(|| { - let on = std::env::var("PERRY_CLASS_FIELD_COUNTERS") - .map(|v| matches!(v.trim(), "1" | "true" | "on" | "yes")) - .unwrap_or(false); - ENABLED.store(on, Ordering::Relaxed); - if on { - extern "C" fn report() { - eprint!("[class-field]"); - for i in 0..N { - eprint!(" {}={}", NAMES[i], C[i].load(Ordering::Relaxed)); - } - eprintln!(); - } - unsafe { - libc::atexit(report); - } - } - }); - ENABLED.load(Ordering::Relaxed) -} - -#[inline] -pub fn bump(i: usize) { - if enabled() { - C[i].fetch_add(1, Ordering::Relaxed); - } -} diff --git a/crates/perry-runtime/src/typed_feedback/guards.rs b/crates/perry-runtime/src/typed_feedback/guards.rs index d117bc4865..621ac1be8a 100644 --- a/crates/perry-runtime/src/typed_feedback/guards.rs +++ b/crates/perry-runtime/src/typed_feedback/guards.rs @@ -308,18 +308,6 @@ fn class_field_fast_contract( && (*obj).class_id == expected_class_id && std::ptr::eq((*obj).keys_array as *const ArrayHeader, expected_keys) && expected_field_index < (*obj).field_count; - if super::guard_counters::enabled() && !shape_ok { - use super::guard_counters::bump; - if (*obj).object_type != crate::error::OBJECT_TYPE_REGULAR { - bump(5); - } else if (*obj).class_id != expected_class_id { - bump(6); - } else if !std::ptr::eq((*obj).keys_array as *const ArrayHeader, expected_keys) { - bump(7); - } else { - bump(8); - } - } // #5093 self-check: the codegen-inlined fast path concludes "slot K is // raw-f64" purely from the per-object intact bit (plus a class_id/keys // match). Under PERRY_VERIFY_TYPED_INTACT=1, assert that whenever this @@ -341,17 +329,12 @@ fn class_field_fast_contract( std::process::abort(); } } - if shape_ok - && require_raw_f64 - && !crate::gc::layout_typed_raw_f64_slot_for_user( - object_addr, - expected_field_index as usize, - ) - { - super::guard_counters::bump(9); - return false; - } shape_ok + && (!require_raw_f64 + || crate::gc::layout_typed_raw_f64_slot_for_user( + object_addr, + expected_field_index as usize, + )) } } @@ -395,19 +378,14 @@ pub extern "C" fn js_typed_feedback_class_field_get_guard( expected_field_index: u32, require_raw_f64: i32, ) -> i32 { - super::guard_counters::bump(2); if !typed_feedback_enabled() && !crate::object::descriptors_in_use() { - let r = class_field_fast_contract( + return class_field_fast_contract( receiver, expected_class_id, expected_keys, expected_field_index, require_raw_f64 != 0, ) as i32; - if r != 0 { - super::guard_counters::bump(3); - } - return r; } let (shape_addr, class_id, gc_type, contract_valid) = class_field_get_contract( receiver, @@ -463,15 +441,10 @@ fn class_field_set_fast_contract( return false; }; if (*gc_header)._reserved & crate::gc::OBJ_FLAG_FROZEN != 0 { - super::guard_counters::bump(10); return false; } } - if require_raw_f64 && !is_plain_number_bits(value_bits) { - super::guard_counters::bump(11); - return false; - } - true + !require_raw_f64 || is_plain_number_bits(value_bits) } fn descriptor_blocks_class_field_set(obj_addr: usize, class_id: u32, key_name: &str) -> bool { @@ -577,9 +550,8 @@ pub extern "C" fn js_typed_feedback_class_field_set_guard( require_raw_f64: i32, ) -> i32 { let value_bits = value.to_bits(); - super::guard_counters::bump(0); if !typed_feedback_enabled() && !crate::object::descriptors_in_use() { - let r = class_field_set_fast_contract( + return class_field_set_fast_contract( receiver, expected_class_id, expected_keys, @@ -587,10 +559,6 @@ pub extern "C" fn js_typed_feedback_class_field_set_guard( require_raw_f64 != 0, value_bits, ) as i32; - if r != 0 { - super::guard_counters::bump(1); - } - return r; } let (shape_addr, class_id, gc_type, contract_valid) = class_field_set_contract( receiver, From 97319b1dcfa8ed4e5383250db4c8a4a6d39d39f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Tue, 11 Aug 2026 16:34:48 +0200 Subject: [PATCH 6/9] test(codegen): pin the chain prologue analysis --- .../src/lower_call/field_init/tests.rs | 274 ++++++++++++++++++ 1 file changed, 274 insertions(+) diff --git a/crates/perry-codegen/src/lower_call/field_init/tests.rs b/crates/perry-codegen/src/lower_call/field_init/tests.rs index 37c95d3b92..1dd7a0517c 100644 --- a/crates/perry-codegen/src/lower_call/field_init/tests.rs +++ b/crates/perry-codegen/src/lower_call/field_init/tests.rs @@ -313,3 +313,277 @@ fn derived_class_refuses_the_elision() { c.extends_name = Some("Base".to_string()); assert!(ctor_prologue_param_assigned_fields(&c).is_empty()); } + +// ─────────────────────────────────────────────────────────────────────────── +// #7512-followup: the SAME predicate across an inheritance chain. +// +// The single-class rule bails to the empty set on any heritage, so a subclass +// instance never got an at-allocation typed-shape declaration and every +// raw-f64 store in every constructor on its chain — the BASE class's own +// `this.x = x` included — missed `GC_OBJ_TYPED_LAYOUT_INTACT` and fell back to +// the by-name `js_put_value_set`. Counted on `shapes.ts`: 528 000 by-name +// stores, and a two-class probe measured 2.0x against the flattened class. +// ─────────────────────────────────────────────────────────────────────────── + +fn named_class( + name: &str, + extends: Option<&str>, + fields: Vec, + ctor: Option, +) -> Class { + let mut c = class(fields, ctor); + c.name = name.to_string(); + c.extends_name = extends.map(str::to_string); + c +} + +fn super_call(args: Vec) -> Stmt { + Stmt::Expr(Expr::SuperCall(args)) +} + +fn chain_of(classes: &[Class]) -> std::collections::HashMap { + classes.iter().map(|c| (c.name.clone(), c)).collect() +} + +fn chain_sets( + map: &std::collections::HashMap, + leaf: &str, +) -> Option)>> { + chain_prologue_assigned_fields(map, leaf) + .map(|v| v.into_iter().map(|(n, s)| (n, sorted(s))).collect()) +} + +/// The `shapes.ts` shape: `Rect extends Base`, both constructors opening with +/// a plain prologue, the derived one after a `super(...)` whose arguments are +/// plain parameters. +#[test] +fn chain_prologue_covers_base_and_derived() { + let base = named_class( + "Base", + None, + vec![field("x")], + Some(func(vec![param(1, "x")], vec![user_this_assign("x", 1)])), + ); + let derived = named_class( + "Derived", + Some("Base"), + vec![field("w")], + Some(func( + vec![param(2, "x"), param(3, "w")], + vec![ + super_call(vec![Expr::LocalGet(2)]), + user_this_assign("w", 3), + ], + )), + ); + let all = vec![base, derived]; + let map = chain_of(&all); + assert_eq!( + chain_sets(&map, "Derived"), + Some(vec![ + ("Base".to_string(), vec!["x".to_string()]), + ("Derived".to_string(), vec!["w".to_string()]), + ]) + ); + let chain = chain_prologue_assigned_fields(&map, "Derived").unwrap(); + assert!(crate::typed_shape::class_chain_layout_declarable_at_allocation(&map, &chain)); +} + +/// A FIELDLESS subclass assigns nothing, and that is a QUALIFIED answer, not a +/// disqualification. The pre-followup API conflated "disqualified" and +/// "qualified but assigns nothing" into one empty set, which is exactly what +/// made a chain unanalysable a class at a time. `Marker extends Shape` in +/// `shapes.ts` is this case. +#[test] +fn fieldless_subclass_is_qualified_with_an_empty_set() { + let base = named_class( + "Base", + None, + vec![field("x")], + Some(func(vec![param(1, "x")], vec![user_this_assign("x", 1)])), + ); + let marker = named_class( + "Marker", + Some("Base"), + vec![], + Some(func( + vec![param(2, "x")], + vec![super_call(vec![Expr::LocalGet(2)])], + )), + ); + let all = vec![base, marker]; + let map = chain_of(&all); + assert_eq!( + chain_sets(&map, "Marker"), + Some(vec![ + ("Base".to_string(), vec!["x".to_string()]), + ("Marker".to_string(), vec![]), + ]) + ); + let chain = chain_prologue_assigned_fields(&map, "Marker").unwrap(); + assert!(crate::typed_shape::class_chain_layout_declarable_at_allocation(&map, &chain)); +} + +/// `Shape.made = Shape.made + 1` — a static bump AFTER the prologue run and +/// BEFORE the subclass's own field writes. Admitted because `this` appears +/// nowhere in it, which is what proves it cannot read a raw-f64 slot that is +/// still holding the allocator's `undefined` fill. +#[test] +fn this_free_trailing_statement_does_not_disqualify() { + let base = named_class( + "Base", + None, + vec![field("x")], + Some(func( + vec![param(1, "x")], + vec![ + user_this_assign("x", 1), + Stmt::Expr(Expr::Binary { + op: perry_hir::BinaryOp::Add, + left: Box::new(Expr::Number(1.0)), + right: Box::new(Expr::Number(2.0)), + }), + ], + )), + ); + let all = vec![base]; + let map = chain_of(&all); + assert_eq!( + chain_sets(&map, "Base"), + Some(vec![("Base".to_string(), vec!["x".to_string()])]) + ); +} + +/// ...but a trailing statement that DOES mention `this` disqualifies the whole +/// chain. This is the soundness pin: in `Base extends nothing`, `Derived`'s +/// `this.w` is still unwritten when `Base`'s body finishes, so a `this` read +/// there would see `undefined`'s NaN-box bits through a declared raw-f64 mask +/// and yield `NaN`. +#[test] +fn trailing_statement_mentioning_this_disqualifies() { + let base = named_class( + "Base", + None, + vec![field("x")], + Some(func( + vec![param(1, "x")], + vec![ + user_this_assign("x", 1), + Stmt::Expr(Expr::PropertyGet { + object: Box::new(Expr::This), + property: "w".to_string(), + byte_offset: 0, + }), + ], + )), + ); + let all = vec![base]; + let map = chain_of(&all); + assert_eq!(chain_sets(&map, "Base"), None); +} + +/// A raw-f64 field nobody's prologue assigns leaves a slot that would be read +/// as a double while it still holds `undefined`. The chain must be refused. +#[test] +fn uncovered_raw_f64_field_refuses_the_declaration() { + let base = named_class( + "Base", + None, + vec![field("x"), field("never_assigned")], + Some(func(vec![param(1, "x")], vec![user_this_assign("x", 1)])), + ); + let derived = named_class( + "Derived", + Some("Base"), + vec![], + Some(func( + vec![param(2, "x")], + vec![super_call(vec![Expr::LocalGet(2)])], + )), + ); + let all = vec![base, derived]; + let map = chain_of(&all); + let chain = chain_prologue_assigned_fields(&map, "Derived").unwrap(); + assert!(!crate::typed_shape::class_chain_layout_declarable_at_allocation(&map, &chain)); +} + +/// A derived constructor that runs anything before `super(...)` may observe +/// arbitrary state; refuse it rather than reason about it. +#[test] +fn statement_before_super_disqualifies() { + let base = named_class( + "Base", + None, + vec![field("x")], + Some(func(vec![param(1, "x")], vec![user_this_assign("x", 1)])), + ); + let derived = named_class( + "Derived", + Some("Base"), + vec![field("w")], + Some(func( + vec![param(2, "x"), param(3, "w")], + vec![ + Stmt::Expr(Expr::Number(1.0)), + super_call(vec![Expr::LocalGet(2)]), + user_this_assign("w", 3), + ], + )), + ); + let all = vec![base, derived]; + let map = chain_of(&all); + assert_eq!(chain_sets(&map, "Derived"), None); +} + +/// A `super(...)` argument that could observe `this` hands the parent +/// constructor the half-built instance. +#[test] +fn super_argument_mentioning_this_disqualifies() { + let base = named_class( + "Base", + None, + vec![field("x")], + Some(func(vec![param(1, "x")], vec![user_this_assign("x", 1)])), + ); + let derived = named_class( + "Derived", + Some("Base"), + vec![field("w")], + Some(func( + vec![param(2, "x"), param(3, "w")], + vec![super_call(vec![Expr::This]), user_this_assign("w", 3)], + )), + ); + let all = vec![base, derived]; + let map = chain_of(&all); + assert_eq!(chain_sets(&map, "Derived"), None); +} + +/// A named parent this module cannot resolve is a constructor we cannot +/// analyse — refuse rather than assume it is inert. +#[test] +fn unresolvable_parent_disqualifies() { + let derived = named_class( + "Derived", + Some("SomewhereElse"), + vec![field("w")], + Some(func(vec![param(3, "w")], vec![user_this_assign("w", 3)])), + ); + let all = vec![derived]; + let map = chain_of(&all); + assert_eq!(chain_sets(&map, "Derived"), None); +} + +/// The single-class predicate must be UNCHANGED by the followup: a class with +/// heritage still gets the empty set from it, so every existing caller keeps +/// its old answer and only the new chain form widens anything. +#[test] +fn single_class_predicate_still_refuses_heritage() { + let derived = named_class( + "Derived", + Some("Base"), + vec![field("w")], + Some(func(vec![param(3, "w")], vec![user_this_assign("w", 3)])), + ); + assert!(ctor_prologue_param_assigned_fields(&derived).is_empty()); +} From 1130a5e102269d83e4655f0cd799b21b4a1bcc08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Tue, 11 Aug 2026 16:41:27 +0200 Subject: [PATCH 7/9] docs: changelog fragment for the class-field chain fix --- .../7855-class-field-subclass-chain.md | 99 +++++++++++++++++++ 1 file changed, 99 insertions(+) create mode 100644 changelog.d/7855-class-field-subclass-chain.md diff --git a/changelog.d/7855-class-field-subclass-chain.md b/changelog.d/7855-class-field-subclass-chain.md new file mode 100644 index 0000000000..5bf908e375 --- /dev/null +++ b/changelog.d/7855-class-field-subclass-chain.md @@ -0,0 +1,99 @@ +### Performance + +**A subclassed class hierarchy paid a by-name hash store for every field +assignment in every constructor on its chain.** `gc-handoff/apps/shapes.ts` +issued **528 000** `js_put_value_set` calls per run; after this change it issues +**48 000**. + +Two independent defects, found in that order, with the second only visible once +the first was fixed. + +#### 1. The class-field shape guard bet on the DECLARED class + +`expr/class_field_inline_guard.rs`'s inline precheck (and the runtime +`class_field_fast_contract` behind it) compared the receiver's `class_id` and +`keys_array` against the *declared* class of the expression — one pair, exact +match. Inside a base class's own constructor or method that bet is not merely +unreliable, it is **guaranteed wrong**: `this` in `Node2D`'s constructor is only +ever reached through `super(...)` from a subclass, so both compares fail on every +single `this.x = x`, and every inherited read — `Node2D`'s `get originDist` +reading `this.x` — missed 100% of the time. + +`class_field_subclass_arms()` collects the base's transitive subclass closure and +the emitter turns the single equality into a disjunction over it, which is the +field-side counterpart of the dispatch widening in +`lower_call/property_get/dynamic_dispatch.rs` (#7800). Soundness does not rest on +the layout algorithm's root→leaf ordering: the field's slot index and its raw-f64 +candidacy are **re-derived per candidate subclass**, so a shadowing +re-declaration or an accessor on the subclass chain drops that arm. Capped at 8 +arms; a class with no eligible subclass emits **byte-identical IR** to before. + +Measured effect, with per-precondition counters on `shapes.ts`: the runtime get +guard goes from being called on every inherited read to **never being entered at +all** (`get_guard_calls=0`) — every read now takes the inline fast path. + +#### 2. #7512, one level up: no subclass instance ever got an at-allocation typed shape + +Fixing (1) left the *store* side almost unmoved, and the counters said why — +`contract_cid=0, contract_keys=0, contract_fieldcount=0, set_frozen=0, +set_notplain=0` but `contract_rawf64=144000`. Not the guard's class test: the +side table said the slot was not raw-f64 at all. + +`typed_shape::class_layout_declarable_at_allocation` consults +`ctor_prologue_param_assigned_fields`, which returns the empty set the moment a +class has `extends`. Empty prologue ⇒ no `js_gc_declare_typed_shape_layout` at +the allocation site ⇒ `GC_OBJ_TYPED_LAYOUT_INTACT` is clear for the whole +construction ⇒ every raw-f64 field store in every constructor on the chain +misses its guard. That is exactly #7512's mechanism ("declaring the fields +`number` is what makes the class slower — more type information selects a +representation whose guard the construction path has made unsatisfiable"), which +was fixed for a standalone class and never extended past it. + +It is not a base-class-only tax: `Node2D` extends nothing, yet its own +`this.x = x` misses too, because the eligibility question is asked of the +**allocated** class. Four TypeScript probes isolate it — a monomorphic class and +a hand-flattened two-field class take the fast path on 100% of constructor +stores; adding a single `extends`, even a *fieldless* one, puts every store on +the chain onto the by-name path. + +`chain_prologue_assigned_fields()` answers the same question for a whole chain, +and distinguishes **disqualified** from **qualified but assigns nothing** — the +old single-set API conflated the two, which is precisely what made a chain +unanalysable a class at a time (a fieldless `Marker extends Shape` is the second +case, and is fine). The extra obligations heritage brings: + +- A leading `super(...)` is skipped rather than truncating the prologue at + statement 0, but only when every argument is `This`-free, so the parent + constructor cannot be handed the half-built instance. +- Every statement **after** a class's prologue run must be a `Stmt::Expr` with no + `this` anywhere in it — a non-leaf constructor's trailing statements run + *before* the leaf writes its own fields, so a `this.w` read in `Shape`'s body + would see a raw-f64-masked slot still holding `undefined`'s NaN-box bits and + yield `NaN` instead of `undefined`. (`Shape.made = Shape.made + 1` is the + motivating admission.) The expression scan uses + `perry_hir::walker::walk_expr_children`, which is exhaustive and drift-checked + against its `_mut` twin; the statement side is a deliberate **whitelist**, + because the HIR has no shared statement walker and a missed variant here would + be a silent wrong answer rather than a missed optimization. +- Every raw-f64 field anywhere on the chain must be prologue-assigned by its own + class, or the declaration is refused. + +The field-init dead-`undefined`-write elision consumes the same chain set exactly +when the chain form is what authorized the declaration. The two must agree: with +the raw-f64 mask live from birth, a field-init `undefined` write into one of +those slots fails `layout_raw_f64_bits` and downgrades the descriptor on the +spot, which would make the declaration worthless. + +#### Validation + +Compiling the 19-program `gc-handoff` corpus with both arms against the **same** +runtime archives and `cmp`-ing the executables (output basename held constant): +**18 of 19 byte-identical, only `shapes` differs**, and all 19 outputs match +node byte-for-byte with exit 0. + +A semantics probe covering the shapes CLAUDE.md flags as weak — fieldless +subclass, indirect subclass, an un-assigned `number` field, a `string` field, and +a post-construction `d.x = "str"` downgrade — is byte-identical to node, +including `Object.keys` order and `JSON.stringify` output. Eight new unit tests +pin the chain analysis, including the two soundness refusals (a trailing +statement mentioning `this`, and a `super()` argument mentioning `this`). From 4424d153ae3d7a4dfe137f847156b00586018d47 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Tue, 11 Aug 2026 17:29:41 +0200 Subject: [PATCH 8/9] docs: key the changelog fragment to PR #7861 --- ...field-subclass-chain.md => 7861-class-field-subclass-chain.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changelog.d/{7855-class-field-subclass-chain.md => 7861-class-field-subclass-chain.md} (100%) diff --git a/changelog.d/7855-class-field-subclass-chain.md b/changelog.d/7861-class-field-subclass-chain.md similarity index 100% rename from changelog.d/7855-class-field-subclass-chain.md rename to changelog.d/7861-class-field-subclass-chain.md From 991bae3281675c1727bccf34e535f20efdd3d725 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Tue, 11 Aug 2026 17:35:37 +0200 Subject: [PATCH 9/9] docs: record the #7854 boxed-store interaction in the fragment --- changelog.d/7861-class-field-subclass-chain.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/changelog.d/7861-class-field-subclass-chain.md b/changelog.d/7861-class-field-subclass-chain.md index 5bf908e375..6e8af0698b 100644 --- a/changelog.d/7861-class-field-subclass-chain.md +++ b/changelog.d/7861-class-field-subclass-chain.md @@ -28,6 +28,15 @@ candidacy are **re-derived per candidate subclass**, so a shadowing re-declaration or an accessor on the subclass chain drops that arm. Capped at 8 arms; a class with no eligible subclass emits **byte-identical IR** to before. +All five `emit_class_field_inline_precheck` sites are widened, **including the +strict BOXED store arm #7854 un-gated**. That one matters on its own: #7854 +removed the `requires_raw_f64` gate so boxed declared fields stop paying an +unconditional guard call, but in a base class's own constructor the precheck it +newly emits would still have missed 100% of the time, because `this` there is +only ever a subclass. Its arms are computed with `requires_raw_f64` rather than a +literal, so a candidate subclass whose declared type disagrees about the slot's +representation is dropped. + Measured effect, with per-precondition counters on `shapes.ts`: the runtime get guard goes from being called on every inherited read to **never being entered at all** (`get_guard_calls=0`) — every read now takes the inline fast path.