gc(codegen): root the spread argument-bundle accumulator (#7664) - #7696
Conversation
📝 WalkthroughWalkthroughCall-spread lowering now uses a shared rooted accumulator helper across six lowering paths. LLVM IR tests verify accumulator ordering across allocating operations. The native statepoint gate documents remaining findings and lowers its unrooted threshold from 11 to 8. ChangesSpread argument rooting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CallSpreadLowering
participant bundle_args_rooted
participant LLVMCodegen
CallSpreadLowering->>bundle_args_rooted: collect regular and spread arguments
bundle_args_rooted->>LLVMCodegen: convert spread source and concatenate rooted accumulator
LLVMCodegen-->>CallSpreadLowering: source-ordered argument array
CallSpreadLowering->>LLVMCodegen: dispatch lowered call
Possibly related issues
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
🧹 Nitpick comments (2)
crates/perry-codegen/src/expr/call_spread_rooting_tests.rs (1)
144-204: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the non-closure arms.
The three tests exercise two of the six lowering arms: the interleaved closure arm and the multi-spread closure arm. The console arm, the
PropertyGetmethod-apply arm, theIndexGetmethod-apply arm, and the namespace rest-function arm are not covered.All six arms call
bundle_args_rooted, so the accumulator ordering is structurally identical and the current coverage does validate the helper. A per-arm test would instead catch an arm that stops using the helper. Add one case forconsole.log(...a, ...b)and one forrecv.m(...a, ...b)if you want that guard.🤖 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/src/expr/call_spread_rooting_tests.rs` around lines 144 - 204, Add coverage for the non-closure lowering arms in the call-spread rooting tests: add one case exercising console.log(...a, ...b) and one exercising recv.m(...a, ...b), using assert_every_fold_rereads_the_accumulator to verify bundle_args_rooted remains used. Preserve the existing test style and focus on the console and PropertyGet method-apply paths; the remaining arms do not require separate cases.crates/perry-codegen/src/expr/call_spread.rs (1)
461-468: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEscaped accumulator handles rely on an untested emission-order invariant.
Both closure arms return the accumulator register out of the
bundle_args_rootedrooted scope. Safety depends onjs_closure_call_apply_with_spreadat Line 519 being the next emission. I traced both paths and the invariant holds today. It is enforced only by the code comments, so a later insertion into either gap would create a stale handle with no failing test.
crates/perry-codegen/src/expr/call_spread.rs#L461-L468: the interleaved arm returnsacc_handle; add an IR assertion that no allocating runtime call appears between the last definition of this handle and thejs_closure_call_apply_with_spreadthat reads it.crates/perry-codegen/src/expr/call_spread.rs#L511-L514: the multi-spread arm returnsspread_handlethrough the same pattern; cover it with the same assertion, sinceregs_ptrandregs_lenare emitted above at Lines 472-493 and the ordering could change.🤖 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/src/expr/call_spread.rs` around lines 461 - 468, Add IR assertions in both accumulator escape paths of call-spread generation: the interleaved arm at crates/perry-codegen/src/expr/call_spread.rs:461-468 and the multi-spread arm at crates/perry-codegen/src/expr/call_spread.rs:511-514. Assert that no allocating runtime call is emitted between the final definition of acc_handle or spread_handle and the corresponding js_closure_call_apply_with_spread consumption, so later emissions cannot invalidate the rooted handle.
🤖 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 `@changelog.d/7696-spread-accumulator-rooting.md`:
- Line 25: Update the compound modifier in the changelog text around
bundle_args_rooted and rooting::with_rooted_accumulator from “two fold” to
“two-fold,” without changing the surrounding wording.
---
Nitpick comments:
In `@crates/perry-codegen/src/expr/call_spread_rooting_tests.rs`:
- Around line 144-204: Add coverage for the non-closure lowering arms in the
call-spread rooting tests: add one case exercising console.log(...a, ...b) and
one exercising recv.m(...a, ...b), using
assert_every_fold_rereads_the_accumulator to verify bundle_args_rooted remains
used. Preserve the existing test style and focus on the console and PropertyGet
method-apply paths; the remaining arms do not require separate cases.
In `@crates/perry-codegen/src/expr/call_spread.rs`:
- Around line 461-468: Add IR assertions in both accumulator escape paths of
call-spread generation: the interleaved arm at
crates/perry-codegen/src/expr/call_spread.rs:461-468 and the multi-spread arm at
crates/perry-codegen/src/expr/call_spread.rs:511-514. Assert that no allocating
runtime call is emitted between the final definition of acc_handle or
spread_handle and the corresponding js_closure_call_apply_with_spread
consumption, so later emissions cannot invalidate the rooted handle.
🪄 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: 5e452a1b-7827-4d40-bf55-41c489623aba
📒 Files selected for processing (5)
.github/workflows/gc-root-dominance.ymlchangelog.d/7696-spread-accumulator-rooting.mdcrates/perry-codegen/src/expr/call_spread.rscrates/perry-codegen/src/expr/call_spread_rooting_tests.rscrates/perry-codegen/src/expr/mod.rs
| URL one, and it is reachable from `a.splice(1, 0, ...src)`. | ||
|
|
||
| **The window is never empty here**, which is why the six sites share one helper | ||
| (`bundle_args_rooted`, a `rooting::with_rooted_accumulator` with the two fold |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Hyphenate “two-fold.”
Line 25 uses “two fold” as a compound modifier. Change it to “two-fold.”
Proposed wording
-(`bundle_args_rooted`, a `rooting::with_rooted_accumulator` with the two fold steps this lowering needs) rather than each asking `operand_protection`
+(`bundle_args_rooted`, a `rooting::with_rooted_accumulator` with the two-fold steps this lowering needs) rather than each asking `operand_protection`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (`bundle_args_rooted`, a `rooting::with_rooted_accumulator` with the two fold | |
| (`bundle_args_rooted`, a `rooting::with_rooted_accumulator` with the two-fold | |
| steps this lowering needs) rather than each asking `operand_protection` |
🧰 Tools
🪛 LanguageTool
[grammar] ~25-~25: Use a hyphen to join words.
Context: ...g::with_rooted_accumulator` with the two fold steps this lowering needs) rather t...
(QB_NEW_EN_HYPHEN)
🤖 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 `@changelog.d/7696-spread-accumulator-rooting.md` at line 25, Update the
compound modifier in the changelog text around bundle_args_rooted and
rooting::with_rooted_accumulator from “two fold” to “two-fold,” without changing
the surrounding wording.
Source: Linters/SAST tools
Six arms of `expr/call_spread.rs` bundle every argument -- regular and spread, in source order -- into one JS array before dispatching, and all six held the half-built array in a bare `i64` SSA register across the loop that fills it: %acc = call i64 @js_array_alloc(i32 0) %box = call double @perry_fn_...(...) ; user code %part = call i64 @js_array_like_to_array(double %box) ; ALLOCATES %acc2 = call i64 @js_array_concat(i64 %acc, i64 %part) ; %acc is stale `--statepoints --moving-only` reports that as `unrooted:alloc`: nothing in the register's cast chain appears in the `js_array_like_to_array` safepoint's live bundle, so an evacuating minor there neither marks nor rewrites the array and `js_array_concat` reads from-space. The window is never empty, which is why the six sites share one helper rather than each asking `operand_protection`: an `Expr::CallSpread` has at least one spread source by construction and `js_array_like_to_array` allocates unconditionally, so `f(...[1, 2])` -- every operand an inert literal -- is already the bug. An operands-only predicate answers `false` for it. Measured on the native corpus (150 modules, 32689 statepoints), same binary in both arms: 11 -> 8 unrooted hazards, the other eight byte-identical, `stale` still 0. The budget in `gc-root-dominance-statepoints` drops to 8 in the same commit. 71/71 gap tests containing a spread call are byte-identical to the pinned Node 26.5.1 oracle. The workflow comment also records what the remaining eight are, because four of them are NOT what it previously said: the `unmasked` population is a checker false positive. `"phi"` is in `TRANSPARENT_OPS`, so taint flows from a phi operand to the phi result and the use is located at the join -- but a phi operand is used on its own incoming edge, and in all four cases the safepoints are on the OTHER edge. Verified register-by-register.
Audit — merging as v0.5.1396. Budget 11 → 8.Sabotage verified independently: forcing The argument for one helper over six 150 modules / 32,689 statepoints, same binary both arms: exactly the three Your correction to my brief is the more valuable half
I inherited "phi-mediated, needs the reload in the predecessor on the edge" from the issue and passed it on as fact. So the real residual is 4, not 8, and the workflow comment was wrong too. Declining to ship the edge-sensitive rule in this PR is right for the reason you give — it lowers a reported count, so it must arrive with its own sabotage arm. A checker fix that quietly makes numbers smaller is the thing this whole campaign exists to prevent. The two capture hazards are correctly characterised as not reloadable: the recipe would re-derive from Gates: 24/24 lint, fmt clean, The rest of your report — #7341's triage (55 → 14 measured, bucket (b) empty), #7164's real defect (a populated pointer mask over a zero-length traced payload, because |
ada4046 to
a7438bd
Compare
1 participant
Closes part of #7664 and lowers
gc-root-dominance-statepoints's--max-unrootedfrom 11 to 8.The window
Six arms of
expr/call_spread.rsbundle every argument — regular and spread,in source order — into one JS array before dispatching (
console.*spread, therecv.m(...)andrecv[k](...)method-apply arms, the namespace REST export,and the closure-callee path's interleaved and multi-spread arms). All six wrote
the same loop and all six held the half-built array in a bare
i64SSA registeracross it:
--statepoints --moving-onlyreports it asunrooted:alloc— nothing in theregister's cast chain appears in the
js_array_like_to_arraysafepoint's livebundle, so an evacuating minor there neither marks nor rewrites the array and
js_array_concatreads from-space. It is #7453's shape (a fresh heap value in araw register across an allocating helper) in the spread lowering rather than the
URL one, reachable from
a.splice(1, 0, ...src).Why one helper rather than six
operand_protectionanswersThe window is never empty here. An
Expr::CallSpreadhas at least onespread source by construction, and
js_array_like_to_arrayallocatesunconditionally — so
f(...[1, 2]), every operand an inert literal, is alreadythe bug, and an operands-only "can anything here collect?" predicate answers
falsefor it.bundle_args_rooteddisjoins the spread-present test withrooting::any_operand_may_collectfor exactly that reason, andan_all_inert_spread_bundle_still_roots_its_accumulatorpins it.The fold itself is
RootedAcc::advance, so the accumulator never exists as aregister the loop holds across an emission: the re-read is fused to the
js_array_concat/js_array_push_f64that consumes it, below the allocatingconversion. Ordering is the fix — a re-read above
js_array_like_to_arraywould root an already-stale pointer, which is #7192's shape.
Measured
Native corpus (150 modules, 2538 functions, 32689 statepoints, 19593 live
bundles), the same binary in both arms:
The three that disappear are exactly the three
unrooted:allochits intest_gap_array_splice_spread::main; the other eight are byte-identical(4
unmasked, 2global, 2capture). No hit was added.Correctness: every gap test containing a spread call — 71 sources, selected
by
\w\(\s*\.\.\./,\s*\.\.\.rather than by hand — is byte-identical tothe pinned Node 26.5.1 oracle. 0 compile failures, 0
node_fail.Gates: all 24
lint-job commands green (each command's own exit statuschecked, and the extraction asserts it found 24).
cargo fmt --all --checkclean.
The tests, and the arm that proves they can fail
expr/call_spread_rooting_tests.rs, three cases, ordering only — never slotcounts. That is slice 8's lesson restated because this file would otherwise
have fallen into it: on the default build
reserve_shadow_slotreturns astack-map index and the pooled-alloca lowering emits a plain
store/load, sono
js_gc_temp_root_*call is emitted at all and atemp_root_calls(ir) > 0assertion reads zero and passes vacuously. Definition-line ordering is visible in
all three lowerings.
The helper walks every
js_array_concat, not the first — a bundle emits oneper spread source and
require_call_linewould leave a partially-fixed loopgreen.
Sabotage arm run and recorded: forcing
protect = false(which reproduces thepre-fix IR exactly) takes the suite to
0 passed; 3 failed— i.e. the tests ranand failed on the assertion, not on a compile error. Reverted, and the file
touched afterwards so the restored mtime is not older than the build.★ Four of the remaining eight are checker false positives
The workflow comment previously described them as "4 unmasked, all PHI-MEDIATED
… the reload has to go in the PREDECESSOR, on the edge, which is a different
insertion model". That is solving a problem that is not there, and this PR
corrects the comment rather than the code.
"phi"is inTRANSPARENT_OPS(gc_root_dominance_check.py:1007), so taintflows from a phi operand to the phi result and the use is located at the
join. But a phi operand is used on its own incoming edge. All four hits have
the identical shape — an
&&join — e.g.readCtx:Every safepoint is on the
%logical.then.1path, where the phi selects%r86.On the edge that carries
%r2nothing collects between its definition and thejoin. Verified register-by-register on all four (
readCtx,__closure_5,Readable,__obj_method_toLocaleString_3).So the real residual is 4, and the comment now says so and says what each is.
It also records why the capture population is not reloadable the way a
string-handle global is: the recipe would have to re-derive the closure pointer
from
%this_closure, ani64parameter RS4GC does not relocate, so there-read would address the pre-move closure.
An edge-sensitive phi rule is deliberately not in this PR: it lowers a
reported count, so it must arrive with a sabotage arm proving it still reports a
phi operand that is live across a safepoint on its own edge, and that belongs
with the checker change rather than riding along on a codegen fix.
Not fixed here, deliberately
Two arms lower
recv_box(andkey_box) before the bundle and use themafter — a second, distinct window on an operand rather than the accumulator.
Closing it needs the receiver and the array in one
RootedGroupscope so therelease post-dominates the dispatch; that is #7640's population and a different
acceptance test. Both sites now carry a comment saying so rather than being
half-fixed silently. The same applies to
cb_boxand the regular-argument stackbuffer on the closure-callee path (#7210 group 2).
Summary by CodeRabbit
Bug Fixes
Tests
Documentation