From 8e146393b6578c672657d6fa0fe0cfdc521a9fb6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sun, 23 Aug 2026 19:45:15 +0200 Subject: [PATCH] perf(codegen): restore the fast store for class field initializers #8630 replaced field-initializer lowering with an unconditional js_class_field_add -- a full [[DefineOwnProperty]] behind a handle scope, per field per construction. A bare `x: number;` still gets a synthesized `undefined` initializer, so an ordinary class pays it for every field of every instance: shapes.ts measured 3.11x the pre-#8630 instruction count. DefineField and a plain store agree when neither of the two differences can arise, and both are statically decidable: no accessor on the chain (which class_field_global_index already proves) and no constructor able to hand back a replacement `this` (the only route to a Proxy receiver). Take the PropertySet path then; keep the full DefineField call otherwise. Conservative on every unseen edge -- native base, dynamic extends, unknown parent. shapes: 3,662,616,604 -> 1,320,393,329 instructions (baseline 1,179,031,124), 94% of the regression recovered. Node 26.5.1 differential: inherited-setter shadowing prints d.v=5 as Node does (pre-#8630 printed "SETTER RAN" / undefined); a two-level getter chain matches; a value-returning parent ctor still emits js_class_field_add. Refs #8648. --- .../8648-class-field-init-fast-path.md | 40 +++++++++++ crates/perry-codegen/src/collectors/mod.rs | 4 +- .../perry-codegen/src/collectors/mutation.rs | 38 +++++++++++ .../src/lower_call/field_init.rs | 67 ++++++++++++++++++- 4 files changed, 147 insertions(+), 2 deletions(-) create mode 100644 changelog.d/8648-class-field-init-fast-path.md diff --git a/changelog.d/8648-class-field-init-fast-path.md b/changelog.d/8648-class-field-init-fast-path.md new file mode 100644 index 0000000000..6fcc49ead0 --- /dev/null +++ b/changelog.d/8648-class-field-init-fast-path.md @@ -0,0 +1,40 @@ +Restored the optimized store for class field initializers. + +#8630 replaced the field-initializer lowering with an unconditional +`js_class_field_add` — a full `[[DefineOwnProperty]]` behind a handle scope, +per field, per construction. The motivation was correct: DefineField uses +CreateDataProperty, so an inherited setter must not run and a Proxy receiver +must observe its `defineProperty` trap. But a bare declaration (`x: number;`) +still gets a synthesized `undefined` initializer, so an ordinary class pays the +full define path for every field of every instance. `shapes.ts` (7 classes, +~2-3 fields each, 120k constructions) measured **3.11x** the instruction count +of the pre-#8630 compiler. + +The two semantics coincide when neither difference can arise, and both are +statically decidable: + +* **no accessor anywhere on the chain** — `class_field_global_index` already + answers exactly this, returning `None` the moment an accessor or a + re-declaration appears on the chain (the #5654 machinery); and +* **the receiver is provably the freshly allocated ordinary instance** — no + constructor on the chain returns a value, `js_ctor_return_override` being the + only route by which a Proxy can become the field-initializer receiver. + +When both hold, lower through the `PropertySet` path (inline shape precheck -> +direct slot store) as this did before #8630. Everything else keeps the full +DefineField call. The chain walk is conservative on every edge it cannot see: a +native base, a dynamic `extends`, an id-only parent edge, or a class missing +from `ctx.classes` all answer "unsafe". + +Measured (instructions retired, vs the pre-#8630 compiler at `00bddb34b`): +`shapes` 3,662,616,604 -> 1,320,393,329 against a 1,179,031,124 baseline — +**94% of the regression recovered**, 3.11x -> 1.12x. + +Verified against Node 26.5.1 that #8630's fix is preserved: for +`class Base { set v(x) {...} }` / `class Derived extends Base { v = 5 }`, the +pre-#8630 compiler printed `SETTER RAN` and `d.v=undefined`; this prints +`d.v=5`, matching Node. A getter two levels up the chain also matches, and a +value-returning parent constructor still emits `js_class_field_add`. + +Partial for #8648: `cycles`, `deeplist` and a two-class `new B(x, y)` loop are +essentially unmoved by this, so they have a second, independent cause. diff --git a/crates/perry-codegen/src/collectors/mod.rs b/crates/perry-codegen/src/collectors/mod.rs index 0ae01715ea..ad3f4b6dbc 100644 --- a/crates/perry-codegen/src/collectors/mod.rs +++ b/crates/perry-codegen/src/collectors/mod.rs @@ -82,7 +82,9 @@ pub(crate) use integer_locals::{ collect_flat_row_aliases, is_int32_producing_expr, static_index_window, }; pub(crate) use local_refs::{expr_contains_local_get, mark_all_candidate_refs_in_expr}; -pub(crate) use mutation::{body_contains_call, body_contains_closure, has_any_mutation}; +pub(crate) use mutation::{ + body_contains_call, body_contains_closure, body_returns_value, has_any_mutation, +}; pub(crate) use number_by_construction::collect_number_by_construction_locals; pub(crate) use param_ranges::{collect_param_int_ranges, ParamIntRanges}; pub(crate) use pointer_locals::collect_pointer_typed_locals; diff --git a/crates/perry-codegen/src/collectors/mutation.rs b/crates/perry-codegen/src/collectors/mutation.rs index 18bfdd1f4e..9f5b4015b9 100644 --- a/crates/perry-codegen/src/collectors/mutation.rs +++ b/crates/perry-codegen/src/collectors/mutation.rs @@ -57,6 +57,44 @@ pub fn body_contains_closure(stmts: &[perry_hir::Stmt]) -> bool { any_top_level_expr(stmts, &mut expr_contains_closure) } +/// #8648: can this constructor body hand back a replacement `this`? +/// +/// ECMAScript lets a constructor `return` an object, which becomes the +/// construction's result (`js_ctor_return_override`). That replacement may be a +/// Proxy, and DefineField must reach a Proxy's `defineProperty` trap rather +/// than its `set`. With no value-returning `return` anywhere on the chain, the +/// instance a field initializer writes to is provably the freshly allocated +/// ordinary object, so `CreateDataProperty` and a plain own-slot store agree. +pub fn body_returns_value(stmts: &[perry_hir::Stmt]) -> bool { + stmts_have_value_return(stmts) +} + +/// `Stmt::Return(Some(_))` at any statement depth. +fn stmts_have_value_return(stmts: &[perry_hir::Stmt]) -> bool { + use perry_hir::Stmt; + for s in stmts { + let hit = match s { + Stmt::Return(Some(_)) => true, + Stmt::If { + then_branch, + else_branch, + .. + } => { + stmts_have_value_return(then_branch) + || else_branch + .as_ref() + .is_some_and(|b| stmts_have_value_return(b)) + } + Stmt::While { body, .. } | Stmt::For { body, .. } => stmts_have_value_return(body), + _ => false, + }; + if hit { + return true; + } + } + false +} + fn expr_contains_closure(expr: &perry_hir::Expr) -> bool { if matches!(expr, perry_hir::Expr::Closure { .. }) { return true; diff --git a/crates/perry-codegen/src/lower_call/field_init.rs b/crates/perry-codegen/src/lower_call/field_init.rs index c7d4a201d3..002f710568 100644 --- a/crates/perry-codegen/src/lower_call/field_init.rs +++ b/crates/perry-codegen/src/lower_call/field_init.rs @@ -816,7 +816,39 @@ pub(crate) fn apply_field_initializers_recursive( // DefineField uses CreateDataProperty semantics: an inherited // setter must not run, while a Proxy receiver must observe its - // `defineProperty` trap. + // `defineProperty` trap. `js_class_field_add` provides both, but it + // is a full [[DefineOwnProperty]] behind a handle scope — per field, + // per construction. #8648: `shapes.ts` pays it ~2M times (7 classes + // x ~2-3 fields x 120k constructions) and measured 3.14x. + // + // The two semantics coincide when neither difference can arise: + // + // * no accessor anywhere on the chain -- `class_field_global_index` + // already answers exactly this, returning `None` the moment an + // accessor (or a re-declaration) appears on the chain + // (`class_field_inline_guard`, #5654); and + // * the receiver is provably the freshly allocated ordinary + // instance -- no constructor on the chain hands back a + // replacement `this` via `js_ctor_return_override`, which is the + // only way a Proxy can become the field-initializer receiver. + // + // Both hold for an ordinary class, so lower through the optimized + // `PropertySet` path (inline shape precheck -> direct slot store) + // exactly as this did before #8630. Anything else keeps the full + // DefineField call. + let chain_can_replace_this = chain_constructor_returns_value(ctx, &class_name_in_chain); + let no_accessor_on_chain = + crate::type_analysis::class_field_global_index(ctx, &class_name_in_chain, &prop) + .is_some(); + if no_accessor_on_chain && !chain_can_replace_this { + let set_expr = Expr::PropertySet { + object: Box::new(Expr::This), + property: prop, + value: Box::new(init_expr), + }; + let _ = lower_expr(ctx, &set_expr)?; + continue; + } let value = lower_expr(ctx, &init_expr)?; let this_val = ctx .this_stack @@ -882,3 +914,36 @@ pub(crate) fn apply_field_initializers_recursive( #[cfg(test)] mod tests; + +/// #8648: does any constructor on `leaf`'s inheritance chain `return` a value? +/// +/// A value-returning constructor can hand back a replacement `this` +/// (`js_ctor_return_override`), and that replacement may be a Proxy — which +/// DefineField must reach through `defineProperty`, not `set`. Conservative on +/// every edge it cannot see: a native base, a dynamic `extends`, or a class +/// missing from `ctx.classes` all answer `true`. +fn chain_constructor_returns_value(ctx: &crate::expr::FnCtx<'_>, leaf: &str) -> bool { + let mut name = leaf.to_string(); + for _ in 0..32 { + let Some(class) = ctx.classes.get(&name).copied() else { + return true; + }; + if class.native_extends.is_some() || class.extends_expr.is_some() { + return true; + } + if let Some(ctor) = class.methods.iter().find(|m| m.name == "constructor") { + if crate::collectors::body_returns_value(&ctor.body) { + return true; + } + } + // `extends` is a class id; `extends_name` is the textual parent. Only + // the latter can be followed through `ctx.classes`, so an id-only edge + // is treated as unknown. + match class.extends_name.as_ref() { + Some(parent) => name = parent.clone(), + None if class.extends.is_some() => return true, + None => return false, + } + } + true +}