fix(gc): scalar-replaced object/array locals are not precise roots (#6968) - #7007
Conversation
|
Warning Review limit reached
Next review available in: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughScalar-replaced object fields and array elements are now bound to precise GC shadow slots when storing heap values. Shadow frames grow lazily, numeric-only stores avoid bindings, and codegen plus runtime regression tests cover the supported lowering paths. ChangesScalar-replaced GC rooting
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 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 |
8b4fcb8 to
b96fcb5
Compare
![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
🧹 Nitpick comments (1)
crates/perry-codegen/tests/scalar_replaced_slot_roots.rs (1)
249-328: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftCover
split()and anonymous-shape constructor stores.The stated rooting scope includes these paths, but neither the IR contracts nor the runtime repro exercises them.
crates/perry-codegen/tests/scalar_replaced_slot_roots.rs#L249-L328: add IR assertions that heap-valuedsplit()parts and anonymous-shape constructor arguments emit scalar-slot bindings.test-files/test_gap_repsel_scalar_replaced_locals.ts#L41-L65: add churn-based runtime cases that read those values after allocation pressure.Based on PR objectives, instrumentation covers “
split()parts” and “anonymous-shape constructor arguments.”🤖 Prompt for 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. In `@crates/perry-codegen/tests/scalar_replaced_slot_roots.rs` around lines 249 - 328, The scalar-replaced rooting tests omit split() parts and anonymous-shape constructor stores. In crates/perry-codegen/tests/scalar_replaced_slot_roots.rs:249-328, add IR assertions confirming heap-valued split parts and anonymous-shape constructor arguments emit scalar-slot bindings; in test-files/test_gap_repsel_scalar_replaced_locals.ts:41-65, add churn-based runtime cases that read those values after allocation pressure.
🤖 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/tests/scalar_replaced_slot_roots.rs`:
- Around line 209-213: Update the assertion in the scalar-replacement test so it
verifies the additional shadow-frame slot reserved for the scalar binding,
rather than merely checking that the frame is nonempty. Compare
frame_slot_count(&ir) with an equivalent numeric-only control or assert the
expected increment, while preserving the existing diagnostic context.
---
Nitpick comments:
In `@crates/perry-codegen/tests/scalar_replaced_slot_roots.rs`:
- Around line 249-328: The scalar-replaced rooting tests omit split() parts and
anonymous-shape constructor stores. In
crates/perry-codegen/tests/scalar_replaced_slot_roots.rs:249-328, add IR
assertions confirming heap-valued split parts and anonymous-shape constructor
arguments emit scalar-slot bindings; in
test-files/test_gap_repsel_scalar_replaced_locals.ts:41-65, add churn-based
runtime cases that read those values after allocation pressure.
🪄 Autofix (Beta)
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: cf4b9e4b-879e-463e-93d1-21b19caf48f2
📒 Files selected for processing (13)
changelog.d/7007-scalar-replaced-local-rooting.mdcrates/perry-codegen/src/codegen/closure.rscrates/perry-codegen/src/codegen/entry.rscrates/perry-codegen/src/codegen/function.rscrates/perry-codegen/src/codegen/method.rscrates/perry-codegen/src/expr/mod.rscrates/perry-codegen/src/expr/property_set.rscrates/perry-codegen/src/expr/scalar_slot_root.rscrates/perry-codegen/src/function.rscrates/perry-codegen/src/stmt/let_stmt.rscrates/perry-codegen/tests/scalar_replaced_slot_roots.rstest-files/test_gap_repsel_scalar_replaced_locals.tstest-parity/gc_repsel_corpus.txt
b96fcb5 to
53f3b71
Compare
Correction: re-measured after the rebase — the per-file table in the PR body is staleThe body's per-file results were measured against
The five red cells at baseline: #6981's 13 distinct files, as of
|
| outcome | count | files |
|---|---|---|
| already repaired before this PR, by #6983 / #6990 / #6994 / #6997 | 11 | repsel_canonical_i32, specabi_polymorphic_coexist, specabi_reassign, ta_param_numeric_read, typedarray_param_read, repsel_ptr_shape_locals, repsel_ptr_shape_barriers, repsel_p4a_inline_tiers, repsel_p4a_logical_numeric, repsel_p4b_field_store_elision, repsel_proven_this_frozen |
| repaired by this PR | 1 | repsel_gc_stress |
| still red, not this defect | 2 | repsel_p4a3_numarray_barriers, repsel_p4a3_ptr_numarray |
So the honest count is 1 of #6981's files, not the 7 the issue hypothesised and not the "1 of 14" the body reported against the older base — the denominator itself had already shrunk. The body's supporting evidence still holds and is what made the conclusion safe to state either way: 10 of the 13 compiled to byte-identical LLVM IR with and without this change, so #6968 could never have been their cause.
Everything else re-verified against d30f3bea8
All on the evacuating precise-roots arm, copied_objects > 0 on every run, diffed against the pinned Node 26.5.0 oracle (not exit codes):
- Issue repro, both forms — baseline MISMATCH / fixed MATCH,
moved≈1.08–1.10 M. Compiled with auto-optimize (the shipped link path), unlike the body's first measurement. test_gap_repsel_scalar_replaced_locals— 3 repeats × 2 arms, baseline MISMATCH ×6 / fixed MATCH ×6;cons_scan_off(a PR arm)moved=0but 35 cycles,evac_minormoved=1 501 975.- PR arm set —
--arms pr: FAIL=0, 29 PASS. The new member is one of only two corpus files that collect at all, so it PASSes rather than UNVERs on all four. - Full gap-file A/B (measured pre-rebase): all 433
test_gap_*.tsproduce byte-identical stdout and exit code with and without the fix under the default configuration.
Unit-test teeth, re-verified three ways
cargo test --test scalar_replaced_slot_roots -p perry-codegen, with crates/perry-codegen/src reverted to d30f3bea8 (sanity-checked: 0 occurrences of root_scalar_replaced_slot / reserve_shadow_slot in the reverted tree):
- fix reverted → the 4 positive tests FAIL, the 2 gate tests pass (they assert absence of rooting, which is also true pre-fix — that is why they need the third direction below);
- gate deliberately coarsened (the
expr_is_known_non_pointer_shadow_valueearly-return removed) → the 2 gate tests FAIL, and the differential frame-slot assertion FAILS withheap-field literal has 3 slots, the numeric-only control has 3; - correct code → all 6 pass.
Note the earlier teeth run in this thread reported all 6 passing against "reverted" source. That run was bogus: it reverted to origin/main, which by then already was this PR. Recording it because the failure mode — a teeth check that silently reverts to nothing — looks exactly like a test with teeth.
Replies to the CodeRabbit review🔴 Major —
|
1 participant
Fixes #6968.
The defect
With the conservative native-stack scan disabled — i.e. with precise (shadow-stack) roots only — a heap value stored into a scalar-replaced object field or array element was swept out from under the alloca that held it.
Scalar replacement deletes the object and keeps one entry-block alloca per field/element. Those allocas belong to no HIR local, so
collect_pointer_typed_locals— which assigns shadow slots by walkingStmt::Let— never saw them, and nothing ever calledjs_shadow_slot_bindfor them:#6951/#6972's object-literal fix cannot reach this shape. That path roots the object handle; here there is no handle. The object local does get a shadow slot reserved (it is pointer-typed), but lowering only ever clears it —
main's frame in the repro pushed 7 slots and emittedjs_shadow_slot_set(i, 0)for them and nothing else.Reproduction (confirmed,
aa1c15028)Both the object and the array form. Isolated with four variants, all on the same acceptance arm:
{ const o = { a: fresh(k), b: churn(N) }; log(o.a, o.b) }const a = [fresh(k), churn(N)]; log(a[0], a[1])const o = {…}; const s = o.a; const n = o.b; log(s, n)console.logargument pathconst s0 = fresh(k); const n0 = churn(N); const o = { a: s0, b: n0 }const s = fresh(k); const n = churn(N); log(s, n)console.logargument temporaries are fine (#6972 holds)The design choice, and what it costs
Three shapes were on the table. Suppressing scalar replacement when a replaced slot is pointer-capable and live across a safepoint is provably sound but gives the optimization back exactly where it earns most (measured below: ~32× on the shape in question). Rooting only across collecting safepoints needs a liveness analysis over storage that has no HIR name.
This PR takes the third: shadow-bind the replaced slot, lazily and only when the store can be a heap reference — the same treatment
emit_shadow_slot_update_for_expralready gives an ordinary pointer-typed local, which is what the issue asks for.Two mechanisms make it fit:
LlFunction::reserve_shadow_slotrewrites the slot-count operand of the already-emittedjs_shadow_frame_pushline (and creates the frame if the pre-lowering count was 0). The alternative — teachingcollect_pointer_typed_localsto predict every scalar-replacement decision — would have to re-derive conditions the collector never evaluates, and the escape facts are not even computed until after the frame is sized.The gate is decided from the lowering, never the declared type (#6997):
expr_is_known_non_pointer_shadow_value, the same predicate that decides whether an ordinary pointer local's slot is bound or cleared. Nothing here consults a field's: string/: numberannotation, which Perry does not enforce. The one place a declared type is consulted isnumeric_storeinexpr::property_set, and that arm is skipped, not trusted: those bits are a canonicalized rawf64by construction.Sites instrumented: object-literal fields, array-literal elements, scalar-replaced
split()parts (js_string_split_part_valuereturns a fresh heap string with nothing else referring to it), anonymous-shape constructor arguments, and bothproperty_setarms (o.f = …and an inlined ctor'sthis.f = …).Measured cost
Release build, macOS arm64, 5 iterations,
perry-baselinevsperry-fixedfrom the same tree:{ x: i & 1023, y: (i>>3) & 1023 }, 40 M iters[i & 1023, (i>>3) & 1023], 40 M iters{ s: mkStr(i), y: (i>>3) & 1023 }, 10 M itersOnly
sis rooted in the third loop;yis skipped, so the emitted bind sites go 3 → 4 for the whole file.Context for that +2.6 ns — the same literal with the object made to escape, i.e. the heap object scalar replacement removes:
Scalar replacement still wins by ~32× after paying for the root. That is the honest reason this PR keeps the optimization rather than suppressing it.
What it does not do: the bind is emitted per store site, so a pointer-capable field store inside a loop re-binds each iteration. Hoisting it to entry setup (persistent-slot style: one bind + a cheap incremental root barrier per store) would remove that call, but the barrier helper lives in
expr/shadow_slot.rs, which #6997 is currently editing. Left as a follow-up rather than merged into a fenced file.Results against #6981's corpus
scripts/gc_repsel_matrix.sh --arms evac_minor,cons_scan_off --pressure 8, release, pinned Node 26.5.0, same tree, baseline vs fixed:Exactly one cell flips:
test_gap_repsel_gc_stressonevac_minor, FAIL → PASS. No cell regresses. Deterministic over 3 repeats (baseline MISMATCH ×3, fixed MATCH ×3,moved=1 230 900in every run — the evacuating minor relocated the same volume in both arms, so this is a rooting fix, not an evacuation that stopped happening).Per file, for the 13 distinct files #6981 lists as red — with the emitted LLVM IR diffed between the two compilers:
repsel_gc_stressrepsel_ptr_shape_localsrepsel_p4b_field_store_elision4010890→NaN, byte-identical to baseline)repsel_canonical_i32specabi_polymorphic_coexistspecabi_reassignta_param_numeric_readtypedarray_param_readrepsel_ptr_shape_barriersrepsel_p4a_inline_tiersrepsel_p4a_logical_numericrepsel_p4a3_numarray_barriersrepsel_p4a3_ptr_numarrayrepsel_proven_this_frozen10 of the 13 compile to byte-identical LLVM IR with and without this change, so #6968 provably cannot be their cause. The hypothesis that #6968 accounted for 7 of them does not survive measurement — #6981's own minimal reproducer for
specabi_reassignis a typed array passed under the specialized ABI, which is the argument-passing family (#6969/#6970/#6971), not scalar replacement. The two files this PR does touch without fixing produce output byte-identical to the baseline, so their remaining redness is a different defect layered on the same file (p4b's is a corrupted number, which no missing root produces).Acceptance evidence
Every claim above was measured on the evacuating precise-roots arm:
with
[gc-copy-minor] ran copied_objects=N, N > 0, on every run — 8 cycles up tocopied_objects=157 385on the issue repro,moved=1 230 900ongc_stress,moved=1 501 960on the new corpus member. No green-because-nothing-moved runs are reported here.Tests
test-files/test_gap_repsel_scalar_replaced_locals.ts— new corpus member, registered intest-parity/gc_repsel_corpus.txt. Object literal, array literal, post-constructiono.a = …, plus the numeric-only twin that must stay free. Teeth verified in both directions, 3 repeats each:cons_scan_off(a PR arm)evac_minormoved=1 501 960default/shipped_defaultIt is GC-live by construction: one of only two corpus members that collect at all (35 cycles under
--pressure 8), so it PASSes rather than UNVERs on every PR arm.scripts/gc_repsel_matrix.sh --arms pr --pressure 8is FAIL=0, 88/88 cells byte-exact vs Node 26.5.0.crates/perry-codegen/tests/scalar_replaced_slot_roots.rs— 5 codegen-contract tests, run per-PR by thee2e-scopedjob (#5960). Teeth verified in both directions:crates/perry-codegen/srcreverted toorigin/main, the 3 positive tests FAIL and pass with the fix;numeric_only_*_emits_no_rooting) cannot fail pre-fix — nothing was rooted then — so their teeth were verified the other way: with theexpr_is_known_non_pointer_shadow_valuegate deliberately removed, both FAIL.Not fixed here, found along the way
An escaping object literal loses
o.aon the same arm ({ const o = { a: fresh(k), b: churn(N) }; keep.push(o); log(o.a, o.b) }, first 2 of 6 iterations). That is the heap object-literal path, not scalar replacement — this PR does not change it and does not claim it.scripts/check_file_size.shis red on this tree for 9 files, all byte-identical toorigin/mainand none touched here.Summary by CodeRabbit
Bug Fixes
Tests
js_shadow_slot_bind/shadow frame behavior for scalar-replaced roots, including precise-root mode coverage.