perf(codegen): shape-cache path for object literals with captures_this methods - #9122
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; 6 remain after this review. 📝 WalkthroughWalkthroughChangesCapturing method closures now use the object-literal shape path by default. The compiler stores properties by index, rereads method closures, and patches their reserved Object literal shape lowering
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🟡 Moderate · up to Object literals with Sequence Diagram(s)sequenceDiagram
participant lower_object_literal
participant js_object_set_field
participant js_object_get_field
participant js_closure_set_capture_bits
lower_object_literal->>js_object_set_field: store object properties by index
lower_object_literal->>js_object_get_field: reread each method closure
lower_object_literal->>js_closure_set_capture_bits: patch reserved this capture
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description provides detailed technical context, measurements, correctness claims, and test results, but it does not follow the required template. It omits the required Resolution Rewrite the description using the repository template. Add the required headings, summarize the change under
✨ 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 |
…s methods
A captures_this method used to force the WHOLE literal onto the by-name
path (all-or-nothing gate): this_patches needs each method closure value
after every later initializer has run, and the by-name arm keeps those
values alive through nested rooted accumulators. But the shape allocator
pre-populates keys_array, so by-INDEX stores are equally valid for method
props — and the patch loop can simply RE-READ each method closure from its
own field slot at patch time (js_object_get_field, declared for IR here):
the rooted object keeps the closure alive, and the re-read observes any
evacuation that moved it, with no per-closure rooting at all.
Kill switch: PERRY_OBJECT_LITERAL_SHAPE_METHODS=0 restores the old routing.
Mac mini kill-switch pairs: {a, b, inc()} literal 853.6 -> 426.2 ns/op
(-50.1%), 5-prop 2-method literal 1585.5 -> 765.6 (-51.7%), plain literal
flat. Differential vs node (methods + this-binding, 200k-literal churn
under allocation pressure, methods-only literals, nested literal methods,
JSON round-trip): byte-identical.
Claude-Session: https://claude.ai/code/session_01F1dt1jfzK2cheMZyus6y6p
796f50c to
c40c321
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 I added one commit: |
…che input (#9144) #9122 introduced the knob; its registration was prepared but did not make it into the merged commit, leaving codegen_env_vars_are_build_cache_inputs red on main. The knob selects between two literal-birth emission sequences, so it is a cache key. Co-authored-by: Ralph Küpper <ralph@skelpo.com>
1 participant
What
A
captures_thismethod used to force the WHOLE object literal onto the by-name path (all-or-nothing gate at the shape-cache fast path):this_patchesneeds each method closure value after every later initializer has run, and the by-name arm keeps those values alive through nested rooted accumulators. But the shape allocator pre-populateskeys_array, so by-INDEX stores are equally valid for method props — and the patch loop can simply re-read each method closure from its own field slot at patch time (js_object_get_field, declared for IR here): the rooted object keeps the closure alive, and the re-read observes any evacuation that moved it, with no per-closure rooting at all.Kill switch:
PERRY_OBJECT_LITERAL_SHAPE_METHODS=0restores the old routing (read per literal, uncached, so the tests below can pin either arm).Measurements
Mac mini kill-switch pairs (7 runs, medians; node for scale):
{a, b, inc()}literal build+callThe residual (~257 ns build-only, ~63 ns per method call) is per-instance closure allocation and the dynamic method-call family — separate levers, sized in the follow-up investigation (the fn_param receiver finding is filed with the team).
Correctness
this-binding, alias preservation, methods-only literals, method-first literals, nested literal methods,JSON.stringifyround-trip, and a 200k-literal churn loop under allocation pressure (evacuating minors between build and patch): byte-identical, both switch states.by_name_method_closure_tests(the gc-root-dominance: 7/3 gated (8/4 unfiltered) NEW root-dominance regressions landed 2026-08-15..25 while the gate was red #8809 rooting-hazard pins) now pin their arm explicitly via the kill switch under a module-local mutex, and three newshape_method_literal_teststwins pin the shape path's invariants: patches below every store, each patch re-reads its closure from its field slot (a kept register is stale after an evacuating initializer), and source-order patching.-D warnings0, codegen 1833/0, full runtime suite 2819/0 (runtime untouched by this change; suite run on the same tree), lints clean, integration 8655 2/2 / 8690 3/3 / 8897 3/3.https://claude.ai/code/session_01F1dt1jfzK2cheMZyus6y6p
Summary by CodeRabbit
Bug Fixes
this, enabling safer and more consistent property initialization.thiscontext.Tests