diff --git a/changelog.d/7861-class-field-subclass-chain.md b/changelog.d/7861-class-field-subclass-chain.md new file mode 100644 index 0000000000..6e8af0698b --- /dev/null +++ b/changelog.d/7861-class-field-subclass-chain.md @@ -0,0 +1,108 @@ +### 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. + +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. + +#### 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`). 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..fd7e8c49a0 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"); @@ -395,9 +532,32 @@ pub(crate) fn emit_class_field_inline_precheck( // 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, &fc_ok); - acc = blk.and(I1, &acc, &ka_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-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, 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/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()); +} 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,