Skip to content

Commit 4c4ef98

Browse files
author
Ralph Küpper
committed
perf(codegen): reach user-written constructors with the dead-field-init elision (#7512)
#7469's `ctor_prologue_param_assigned_fields` matched the constructor prologue on `Expr::PropertySet`. No user syntax lowers to that node — `lower_expr`'s assignment arm turns every source `obj.prop = value` into `Expr::PutValueSet`, and `PropertySet` is emitted only by synthesized HIR (the anon-shape object-literal ctor). The elision therefore fired on object literals and was structurally unreachable for the declared class its changelog claimed to cover. That is the #7512 anomaly: `new Node(v, w)` with two declared `number` fields emitted FOUR field-store IC diamonds per construction against the equivalent `{v, w}` literal's two — two of them storing a compile-time-constant `undefined` that the next two statements overwrite. Both dead diamonds took the cold arm on every construction, not occasionally: a fresh instance has no typed-shape descriptor yet (`js_gc_init_typed_shape_layout` runs after the ctor returns), so a `requires_raw_f64` set-guard cannot pass and `js_class_field_set_fallback` — record-fallback plus a linear-key-search by-name store — ran twice per object. One recognizer, `prologue_assigned_field`, now accepts both spellings of `this.<field> = <plain parameter>`. The proof obligation is unchanged and is about the operands rather than the store opcode: `This` and `LocalGet` of a plain parameter cannot throw, allocate, or observe `this`. `PutValueSet` additionally requires a constant string key and a `This` receiver. Emitted-IR census, same workload and compiler: the class constructor drops from 314 to 157 IR lines and from 4 store diamonds to 2, matching the literal's 2; the literal's constructor is byte-identical to before. Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH
1 parent e4ab722 commit 4c4ef98

3 files changed

Lines changed: 464 additions & 22 deletions

File tree

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
### Codegen: the dead-default-field-init elision now reaches user-written constructors (#7512)
2+
3+
#7469 elides the default-`undefined` write for a class field the constructor's
4+
own prologue provably overwrites. Its changelog said it covered "plain user
5+
ctors like `constructor(a, b) { this.a = a; this.b = b }`". It did not, and
6+
could not: `ctor_prologue_param_assigned_fields` matched the prologue on
7+
`Expr::PropertySet`, and **no user syntax lowers to that node**.
8+
`perry-hir/src/lower/lower_expr/assignment.rs` turns every source-level
9+
`obj.prop = value``this.v = v` included — into the spec `PutValue` node
10+
`Expr::PutValueSet`. `Expr::PropertySet` is emitted only by *synthesized* HIR,
11+
which is exactly what the anon-shape object-literal constructor
12+
(`lower/context.rs::mint_anon_shape_class`) is built from. The elision was
13+
therefore measured on the one construction form it reached, and was
14+
structurally unreachable for the declared class it was documented as covering.
15+
16+
The visible consequence is the anomaly filed as #7512: `new Node(v, w)` with two
17+
declared `number` fields was **slower** than the equivalent `{v, w}` literal, so
18+
the most statically-known construction form in the language was the least
19+
optimized one. Emitted-IR census of the two constructors, same workload, same
20+
compiler (`--trace llvm`):
21+
22+
| per construction | `{v, w}` literal | `new Node(v, w)` before | after |
23+
|---|--:|--:|--:|
24+
| field-store IC diamonds | 2 | **4** | 2 |
25+
| `js_typed_feedback_class_field_set_guard` | 2 | 2 | 0 |
26+
| `js_class_field_set_fallback` | 2 | 2 | 0 |
27+
| `js_array_numeric_value_to_raw_f64` | 0 | 4 | 2 |
28+
| constructor body, IR lines | 111 | **314** | 157 |
29+
30+
The two extra diamonds each stored a compile-time-constant `undefined` that the
31+
next two statements overwrote. Both took the cold by-name arm on *every*
32+
construction rather than occasionally: a freshly allocated instance carries no
33+
typed-shape descriptor (`js_gc_init_typed_shape_layout` runs after the
34+
constructor returns), so a `requires_raw_f64` set-guard on a declared `number`
35+
field cannot pass, and `js_class_field_set_fallback` — a feedback-fallback
36+
record plus a linear-key-search `js_object_set_field_by_name` — ran twice per
37+
object. A class whose fields are declared `any` paid the same two dead diamonds.
38+
39+
The fix is one recognizer, `prologue_assigned_field`, accepting both spellings
40+
of `this.<field> = <plain parameter>`. The proof obligation is unchanged and is
41+
about the *operands*, not the store opcode: `This` and `LocalGet` of a plain
42+
parameter cannot throw, allocate, or observe `this`, so the prologue write is
43+
reached before any other effect of the constructor. `PutValueSet` additionally
44+
requires a constant string key and a `This` receiver. Every existing refusal
45+
still applies — derived classes, field initializers, computed keys, parameter
46+
defaults, setter-shadowed fields, and any statement that breaks the leading run.
47+
48+
Behaviour is unchanged: an 11-case semantics probe (unassigned fields still read
49+
`undefined`, `Object.keys`/JSON shape, prototype-setter shadowing, derived
50+
classes, post-construction reassignment, 200-instance shared-shape consistency)
51+
produces byte-identical output before and after, and matches Node on 10 of 11 —
52+
the eleventh is a pre-existing, unrelated divergence in how a declared field
53+
interacts with a same-named prototype accessor, identical on both compilers.
54+
55+
**Not fixed here, and worth its own ticket:** the residual gap is an ordering
56+
one. `lower_call/new.rs` emits `js_gc_init_typed_shape_layout` *after* the
57+
constructor call, so no raw-f64-declared class-field store inside any
58+
constructor can ever pass its guard — the surviving real stores still take
59+
`js_put_value_set`. That belongs with #7510's construction-path item.

crates/perry-codegen/src/lower_call/field_init.rs

Lines changed: 91 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -13,15 +13,87 @@ use crate::expr::{lower_expr, FnCtx};
1313
use crate::nanbox::{double_literal, POINTER_MASK_I64};
1414
use crate::types::{DOUBLE, I32, I64};
1515

16+
/// The field name a constructor-prologue statement assigns from a plain
17+
/// parameter, or `None` if the statement is not of that shape.
18+
///
19+
/// **Two HIR shapes mean the same thing here, and matching only one of them is
20+
/// what #7512 was** (`new Node(v, w)` measured 63% slower than the equivalent
21+
/// `{v, w}` literal, the reverse of the expected ordering):
22+
///
23+
/// - `Expr::PropertySet` is what the compiler SYNTHESIZES. Every anon-shape
24+
/// object-literal constructor (`lower/context.rs::mint_anon_shape_class`)
25+
/// and the destructuring lowering emit it directly.
26+
/// - `Expr::PutValueSet` is what USER SOURCE lowers to. `lower_expr`'s
27+
/// assignment arm (`perry-hir/src/lower/lower_expr/assignment.rs`) turns
28+
/// *every* source-level `obj.prop = value` — `this.v = v` in a hand-written
29+
/// constructor included — into the spec `PutValue` node. Nothing a user can
30+
/// type produces `Expr::PropertySet`.
31+
///
32+
/// So #7469's elision, which only ever matched `PropertySet`, fired on the
33+
/// synthesized literal ctor and was structurally unreachable for the declared
34+
/// class it was documented as covering. The class paid two extra full
35+
/// class-field-set IC diamonds per construction — a guard call plus a
36+
/// by-name `js_class_field_set_fallback` each, since a fresh instance has no
37+
/// typed-shape descriptor yet and the raw-f64 guard therefore cannot pass —
38+
/// writing a compile-time-constant `undefined` that the next two statements
39+
/// overwrite.
40+
///
41+
/// The proof obligation is identical for both shapes and is entirely about the
42+
/// *operand* expressions, not the store opcode: `This` and `LocalGet(<plain
43+
/// param>)` cannot throw, allocate, or observe `this`, so the assignment is
44+
/// reached before any other effect of the constructor. Both nodes' misses
45+
/// route through a `[[Set]]` that honours an inherited setter, so neither adds
46+
/// prototype-chain exposure the other lacks — and the caller separately
47+
/// refuses any field the class itself declares a setter for.
48+
///
49+
/// `PutValueSet` additionally requires a constant string key (a computed key
50+
/// is an arbitrary expression that can run user code) and `receiver` to be
51+
/// `This` as well, since codegen evaluates both.
52+
fn prologue_assigned_field<'a>(
53+
stmt: &'a Stmt,
54+
param_ids: &std::collections::HashSet<u32>,
55+
) -> Option<&'a str> {
56+
let is_plain_param = |e: &Expr| matches!(e, Expr::LocalGet(id) if param_ids.contains(id));
57+
match stmt {
58+
// Synthesized (anon-shape ctor, destructuring lowering).
59+
Stmt::Expr(Expr::PropertySet {
60+
object,
61+
property,
62+
value,
63+
}) if matches!(object.as_ref(), Expr::This) && is_plain_param(value.as_ref()) => {
64+
Some(property.as_str())
65+
}
66+
// User-written `this.f = p;`.
67+
Stmt::Expr(Expr::PutValueSet {
68+
target,
69+
key,
70+
value,
71+
receiver,
72+
strict: _,
73+
}) if matches!(target.as_ref(), Expr::This)
74+
&& matches!(receiver.as_ref(), Expr::This)
75+
&& is_plain_param(value.as_ref()) =>
76+
{
77+
match key.as_ref() {
78+
Expr::String(property) => Some(property.as_str()),
79+
_ => None,
80+
}
81+
}
82+
_ => None,
83+
}
84+
}
85+
1686
/// Field names whose default-`undefined` initializer write is provably dead
1787
/// because the class's own constructor unconditionally overwrites them before
18-
/// anything can observe `this` (#7469).
88+
/// anything can observe `this` (#7469; extended to user-written constructors
89+
/// by #7512).
1990
///
2091
/// A field declared without an initializer must normally be written as
2192
/// `undefined` in the init phase (#486: `new C().x === undefined` is spec, not
2293
/// zero-bytes-from-the-allocator). But the most common constructor shape —
23-
/// including every synthesized anon-shape literal ctor
24-
/// (`lower/context.rs::mint_anon_shape_class`) — opens with a run of plain
94+
/// every synthesized anon-shape literal ctor
95+
/// (`lower/context.rs::mint_anon_shape_class`), and the hand-written
96+
/// `constructor(v, w) { this.v = v; this.w = w }` — opens with a run of plain
2597
/// `this.f = <param>` statements. For those fields the `undefined` write is a
2698
/// dead store: it is overwritten before any code that could read `this.f`
2799
/// runs. On `churn.ts` that dead store was 2 of the 4 guarded field-store
@@ -49,18 +121,18 @@ use crate::types::{DOUBLE, I32, I64};
49121
/// - **Every constructor parameter is plain**: no default (a default expression
50122
/// evaluates before the prologue and, in the general lowering, could observe
51123
/// `this`), no rest, no decorators, no `arguments` materialization.
52-
/// - **No setter shares a name with a prologue-assigned field** — the
53-
/// PropertySet would dispatch to the setter instead of writing the slot, and
54-
/// the elided `undefined` write was the only slot write.
124+
/// - **No setter shares a name with a prologue-assigned field** — the store
125+
/// would dispatch to the setter instead of writing the slot, and the elided
126+
/// `undefined` write was the only slot write.
55127
/// - The field itself is public and non-computed (`is_private` false,
56128
/// `key_expr` none).
57129
///
58-
/// The prologue is the maximal leading run of
59-
/// `Stmt::Expr(PropertySet { object: This, property, value: LocalGet(<param>) })`
60-
/// statements. A `LocalGet` of a plain parameter cannot throw, allocate, or
61-
/// observe `this`, so every field it assigns is written before ANY other
62-
/// effect of the constructor — which is exactly the guarantee that makes the
63-
/// earlier `undefined` write dead.
130+
/// The prologue is the maximal leading run of statements that
131+
/// [`prologue_assigned_field`] recognizes as `this.<f> = <plain param>`. A
132+
/// `LocalGet` of a plain parameter cannot throw, allocate, or observe `this`,
133+
/// so every field it assigns is written before ANY other effect of the
134+
/// constructor — which is exactly the guarantee that makes the earlier
135+
/// `undefined` write dead.
64136
fn ctor_prologue_param_assigned_fields(
65137
class: &perry_hir::Class,
66138
) -> std::collections::HashSet<String> {
@@ -91,17 +163,11 @@ fn ctor_prologue_param_assigned_fields(
91163
let param_ids: std::collections::HashSet<_> = ctor.params.iter().map(|p| p.id).collect();
92164
let mut assigned = std::collections::HashSet::new();
93165
for stmt in &ctor.body {
94-
match stmt {
95-
Stmt::Expr(Expr::PropertySet {
96-
object,
97-
property,
98-
value,
99-
}) if matches!(object.as_ref(), Expr::This)
100-
&& matches!(value.as_ref(), Expr::LocalGet(id) if param_ids.contains(id)) =>
101-
{
102-
assigned.insert(property.clone());
166+
match prologue_assigned_field(stmt, &param_ids) {
167+
Some(property) => {
168+
assigned.insert(property.to_string());
103169
}
104-
_ => break,
170+
None => break,
105171
}
106172
}
107173
if assigned.is_empty() {
@@ -449,3 +515,6 @@ pub(crate) fn apply_field_initializers_recursive(
449515
}
450516
Ok(())
451517
}
518+
519+
#[cfg(test)]
520+
mod tests;

0 commit comments

Comments
 (0)