perf(codegen): reach user-written constructors with the dead-field-init elision (#7512) - #7515
Merged
Merged
Conversation
proggeramlug
force-pushed
the
diag/7512-class-vs-literal
branch
from
August 6, 2026 09:36
bd6c609 to
4c4ef98
Compare
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe constructor field-initialization analysis now recognizes user-written and synthesized assignments from plain parameters to ChangesConstructor field initialization elision
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-codegen/src/lower_call/field_init.rs`:
- Around line 67-80: Preserve inherited-setter effects by preventing the
PutValueSet optimization in
crates/perry-codegen/src/lower_call/field_init.rs:67-80 from eliding stores that
may dispatch through an inherited accessor, or use a field-definition path that
bypasses accessors. Update the proof statement in
crates/perry-codegen/src/lower_call/field_init.rs:41-51 to acknowledge that the
omitted initial [[Set]] is observable. Remove the prototype-setter parity claim
from changelog.d/7515-class-ctor-dead-field-init.md:39-53. Add a semantic
regression that installs a setter on C.prototype before construction and
verifies both expected setter invocations.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 03f5564b-86a4-4d9a-a436-0a3b4770bb40
📒 Files selected for processing (3)
changelog.d/7515-class-ctor-dead-field-init.mdcrates/perry-codegen/src/lower_call/field_init.rscrates/perry-codegen/src/lower_call/field_init/tests.rs
Contributor
Author
Broadened behaviour check: compiler-to-compiler A/B, whole constructor corpusSince this changes a predicate that fires on every user class with a The single compile failure is Alongside the Node-oracle runs already in the PR body:
Precision spot-checkThe elision is per-field, not per-class. A class with three declared fields |
added 3 commits
August 6, 2026 11:59
…it 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
…it (#7512) CodeRabbit's review reads the elided default-`undefined` field write as an observable `[[Set]]` that would fire an inherited setter, so removing it would take a prototype setter from two invocations to one — and the compile-time `class.setters` check structurally cannot see a setter installed on `C.prototype` after compilation. Measured instead of argued, and the premise does not hold. A class field declaration is a `CreateDataProperty` — a DEFINE, not a `[[Set]]` — so it never consults an inherited accessor, and it installs an own data property that the prologue assignment then writes directly rather than dispatching past. A setter installed on `C.prototype` after compilation runs ZERO times, on Node and on Perry, before and after this change. The "field init is a [[Set]], so the setter fires twice" reading is the legacy `useDefineForClassFields: false` behaviour, which neither this compiler nor Node implements. `test-files/test_class_field_init_proto_setter.ts` is the execution-level regression CodeRabbit asked for: it installs post-compilation prototype setters on a first- and a second-position prologue field, counts invocations, and also covers a field the prologue does not assign. It is byte-identical to Node on both compiler arms. The doc comment's `[[Set]]` sentence — which invited the reading — is replaced with the DEFINE account and a pointer to the test, and the changelog's parity claim now cites it. Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH
proggeramlug
force-pushed
the
diag/7512-class-vs-literal
branch
from
August 6, 2026 09:59
9ab2199 to
cff33c0
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
This was referenced Aug 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the bug half of #7512.
The anomaly
new Node(v, w)with two declarednumberfields measured 28.5× Node whilethe equivalent
{v, w}object literal measured 17.4× — the moststatically-known construction form in the language was the slowest one. #7512
filed that as a suspected defect rather than a missing optimisation. It is one.
Root cause
ctor_prologue_param_assigned_fields(crates/perry-codegen/src/lower_call/field_init.rs)— the #7469 predicate that proves a class field's default-
undefinedwrite isdead because the constructor prologue overwrites it — matched the prologue on
Expr::PropertySet:No user syntax lowers to that node.
perry-hir/src/lower/lower_expr/assignment.rs:86turns every source-level
obj.prop = value—this.v = vin a hand-writtenconstructor included — into the spec
PutValuenodeExpr::PutValueSet.Expr::PropertySetis emitted only by synthesized HIR, which is exactly whatthe anon-shape object-literal constructor (
lower/context.rs::mint_anon_shape_class)is built from.
So #7469 was measured on the one construction form it could reach, and was
structurally unreachable for the declared class its own changelog claimed to
cover ("plain user ctors like
constructor(a, b) { this.a = a; this.b = b }").Evidence: emitted-IR call census
Two probes differing only in construction form, same compiler,
--trace llvm,counting calls inside the constructor each
new/literal invokes:{v, w}literalnew Node(v, w)beforejs_typed_feedback_class_field_set_guardjs_class_field_set_fallbackjs_array_numeric_value_to_raw_f64The two extra diamonds each stored a compile-time-constant
undefined(
0x7FFC000000000001in the IR) that the next two statements overwrite. Theytook the cold arm on every construction rather than occasionally: a freshly
allocated instance carries no typed-shape descriptor —
js_gc_init_typed_shape_layoutis emitted after the constructor call — so a
requires_raw_f64set-guard on adeclared
numberfield cannot pass, andjs_class_field_set_fallback(afeedback-fallback record plus a linear-key-search
js_object_set_field_by_name)ran twice per object. A variant class with
anyfields paid the same two deaddiamonds, so this is not a raw-f64 interaction.
The literal's constructor IR is byte-identical before and after.
The fix
One recognizer,
prologue_assigned_field, accepting both spellings ofthis.<field> = <plain parameter>. The proof obligation is unchanged and isabout the operands, not the store opcode:
ThisandLocalGetof a plainparameter cannot throw, allocate, or observe
this, so the prologue write isreached before any other effect of the constructor.
PutValueSetadditionallyrequires a constant string key (a computed key evaluates arbitrary code) and a
Thisreceiver (codegen evaluates bothtargetandreceiver). Both nodes'miss paths route through a
[[Set]]that honours an inherited setter, soneither adds prototype-chain exposure the other lacks.
Every existing refusal still applies and is pinned by a test: derived classes,
field initializers, computed keys, parameter defaults, setter-shadowed fields,
and any statement that breaks the leading run.
Tests
crates/perry-codegen/src/lower_call/field_init/tests.rs, 11 cases visible tocargo test -p perry-codegen --lib(per CLAUDE.md,crates/*/tests/*.rsdoesnot run per-PR). The load-bearing one is
synthesized_and_user_ctor_prologues_agree,which pins the literal form and the class form of the same program to the same
elided field set — the ordering #7512 is about cannot silently invert again.
Sabotage-checked: with the
PutValueSetarm removed, exactly the three positivetests fail and the eight refusal tests still pass.
Validation
cargo test -p perry-codegen --lib --no-fail-fast— 650 passed, 0 failed.cargo test -p perry-runtime --no-fail-fast— 1748 passed, 0 failed.scripts/raw_handle_debt.py— 999 (baseline 999).scripts/check_file_size.sh— clean (run after the final commit).cargo fmt --all -- --check— clean.undefined,Object.keys/JSON shape, prototype-setter shadowing, derived classes,interrupted prologues, post-construction reassignment, 200-instance
shared-shape consistency): byte-identical before and after, and matches
Node on 10 of 11. The eleventh is a pre-existing divergence in how a declared
field interacts with a same-named prototype accessor, identical on both arms.
test-files/*{class,ctor,construct}*.tsagainst Node: 100 pass, 8 differ — and all 8 produce identical output on
both compiler arms, i.e. pre-existing and unrelated.
No timings are quoted: the numbers in #7512 came from a pinned quiet host and
this work was done on a loaded one. The evidence here is static.
Not fixed here
The residual gap is an ordering problem, and it is #7510's territory rather
than a second fix in this PR:
lower_call/new.rsemitsjs_gc_init_typed_shape_layoutafter the constructor call(
js_object_alloc_class_inline_keys->Node_constructor-> layout init ->js_ctor_return_override, visible in that order in the trace). A fresh instancetherefore has no typed descriptor and no
GC_OBJ_TYPED_LAYOUT_INTACTbit whilethe constructor runs, so no raw-f64-declared class-field store inside any
constructor can ever pass its guard — the two surviving real stores still take
js_put_value_set. Installing the descriptor at allocation is not a one-liner:init_typed_shape_layoutvalidates that each raw-f64 slot currently holds aplain double, and fresh slots hold
undefined. Recorded on #7512.https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH
Summary by CodeRabbit
Performance
Bug Fixes
Tests