perf(hir): admit method shorthand into the closed-shape record route (method literals 671→19 ns, −97%) - #9134
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughStatic method shorthand properties can now use anonymous-shape allocation when their keys and bodies meet the closed-shape rules. Directly exported methods and methods using ChangesStatic method closed-shape lowering
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This localized compiler optimization preserves existing fallback behavior for unsupported method forms and has supporting correctness and performance coverage. No actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is detailed, relevant, and covers the implementation, measurements, correctness checks, tests, and binary-size impact. It does not use the template headings and omits explicit Related issue and Checklist sections, but the core information is present.
✨ Finishing Touches🧪 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 |
|
Full battery on this branch: |
6e9c15a to
04f490f
Compare
|
Rebased onto current main (#9128 in): perry-hir passed=586 failed=0, codegen 1832/0 on the new base. Ready for review. |
is_closed_shape gated the anonymous-shape record lowering (fixed-slot
reads, preinstalled shape stamp, class allocation) to key:value and
shorthand props. A method-shorthand prop m() {} — the same value as
m: function () {} for everything except super — fell to the Expr::Object
route, where the literal's own field reads degrade to by-name dispatch
and each this-method needs a post-construction patch. Mini, 3-prop
literal: m: function(){ this.a } 44 ns; m(){ 1 } 114 ns; m(){ this.a }
234 ns; two methods 461 ns (node 5-17).
Admit static-key, non-async, non-generator, super-free method shorthand:
the shared lower_method_prop lowers the body, and the closure is taken as
a dynamic-this value (captures_this: false, no enclosing_class), exactly
the function-expression spelling, so t.m() and f.call(other) bind the
call receiver per spec and no patch exists to get wrong. super is
detected by an swc Visit over the whole body (nested functions and
classes included): with no object home slot on the stack, super.x would
otherwise fall to class-super lowering. Directly exported method literals
keep the seeded IIFE route that imported-object capabilities key on.
The PerryTS#8793 routing test now pins the record form for the static case
(source-ordered record fields, dynamic-this closure argument) and keeps
its super/computed fail-closed assertions.
Claude-Session: https://claude.ai/code/session_01F1dt1jfzK2cheMZyus6y6p
04f490f to
da2ae4c
Compare
|
Merged as part of a six-PR merge train: all six were cherry-picked onto one branch and validated together in a single build rather than six separate ones, at the maintainer's request to speed up a backlog. Combined validation: hir 361 passed, codegen 1352, runtime 2822 (exit 0, 0 abort markers), perry --bins 1066, and perry-hir only; covered by the combined hir + codegen suites above. |
1 participant
What
is_closed_shape(the gate for lowering an object literal to anew __AnonShape_…record class — fixed-slot reads, preinstalled shape stamp, class allocation) admitted onlykey: valueand shorthand props. A method-shorthand propm() { … }— semantically the same value asm: function () { … }for everything exceptsuper— fell to theExpr::Objectroute, where the literal's own field reads degrade to by-name dispatch and eachthis-method needs a post-construction patch. Measured on the mini for a 3-prop literal:m: function(){ this.a }44 ns,m(){ 1 }114 ns,m(){ this.a }234 ns, two methods 461 ns (node 5–17).This admits static-key, non-async, non-generator,
super-free method shorthand into the closed-shape route: the sharedlower_method_proplowers the body, and the resulting closure is taken as a dynamic-thisvalue (captures_this: false, noenclosing_class) exactly like the function-expression spelling — sot.m()andf.call(other)bind the call receiver per spec and no patch is needed.superis detected with an swcVisitover the whole body (nested functions/classes included) because with no object home slot on the stack,super.xwould otherwise fall to class-superlowering.Measurements
Mac mini (quiet host), 3 interleaved rounds each, median ns/op for a 3-prop escaping literal (build + one field read);
main= current main routing,nodeon the same box:{ a, b, m() { return this.a + 1 } }{ a, b, m() { return 1 } }{ a, b, m1() { this.a }, m2() { this.b } }{ a, b, inc: function () { this.a + 1 } }(already a record){ a, b, f: (x) => x + k }(captured arrow field, already a record)Spread on every row ≤0.1 ns across rounds. The function-expression row also moved and is not attributed to a code path this PR touches — reported as measured. The residual on the moved rows is now the per-instance closure allocation (~46 ns for a capturing closure vs ~4 in node — its own family) plus the record class allocation.
Correctness
thisbinding (params, arrays, extracted.call), the perf(codegen): shape-cache path for object literals with captures_this methods #9122 method-literal set, the 300k-birth churn set, and a function-expressionthisset — all byte-identical. Two probes covering the exotic surface (Object.keys/for…inorder, spread andObject.assigncopies,deletethen call → TypeError, later shadowing, nested literal methods, quoted/numeric keys,__proto__+super,this-returning chains, generators/async/getters/toPrimitivemethods) are byte-identical between the old and new routes; the only lines where either differs from node are pre-existing on main — method.prototypeexistence,new obj.m()constructibility, and captureless-method identity ({n(){}}.n === {n(){}}.nistrue, theFuncRefsingleton) — and are unchanged by this PR.thisclosure argument) and keeps itssuper/computed fail-closed assertions; directly exported method literals stay on the seeded IIFE route that imported-object capabilities key on.Binary size
HIR-level routing only: a method literal now compiles as a record class allocation (one
new) instead of a shape-cache allocation plus per-method patch sequence — fewer emitted instructions per literal site, no new inline sequences. Fixture (five method-literal loops, arm64,size -m):__text10,962,516 → 10,959,316 = −3,200 B (−0.03%).https://claude.ai/code/session_01F1dt1jfzK2cheMZyus6y6p
Summary by CodeRabbit
Performance
Behavior
thisbehavior when these methods are called.super, async/generator methods, computed keys, and directly exported method literals continue using their existing handling.Tests