gc: reload js_closure_get_capture_bits results across a collection point (#7725) - #7732
Conversation
📝 WalkthroughWalkthroughThe reload pass now rematerializes closure capture reads, tracks invalidation by capture index, and handles capture calls in typed and raw instructions. New tests cover capture reloads and broader reload behavior. The workflow, changelog, and package version were updated. ChangesClosure capture reload support
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
…int (#7725) The last 2 unrooted:capture hits gc-root-dominance-statepoints' budget was carrying: js_closure_get_capture_bits(ptr, idx)'s return value is never re-entered into the RS4GC-tracked domain or a temp root, unlike %this_closure itself (already protected via current_closure_slot, #7055). root_reload.rs's Facts now recognizes a capture-bits GET call whose ptr operand already belongs to a reloadable recipe as an extension of that recipe -- the same treatment string-handle globals and shadow slots already get -- with a synthetic per-index store side-condition so a same-index SET still suppresses the reload. Native corpus: 2 -> 0 unrooted hazards, stale held at 0. gc-root-dominance-statepoints' --max-unrooted is deleted (the flag already defaults to 0) rather than set to 0, so a future hit can't be silently absorbed by a stale budget. root_reload.rs crossed the 2,000-line file-size cap gaining this; its test module moved to a sibling root_reload_tests.rs (the existing linker.rs/type_analysis.rs #[path] idiom), which is also where the four new capture-bits tests landed.
2843fbc to
6a91539
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: 4
🧹 Nitpick comments (3)
crates/perry-codegen/src/root_reload.rs (1)
811-815: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider narrowing the call arm to the capture-bits callee.
inst_resultandset_inst_resultnow accept every result-bearingLlInst::Call. Today onlyCAPTURE_GET_CALLEEreachesmaterialize, because it is the only call thatfacts_ofmarkstransparent. That coupling is implicit. If a future change marks another call transparent,materializewill duplicate that call with no further review gate.A callee check here makes the invariant local and self-checking.
♻️ Proposed narrowing
- LlInst::Call { dst: Some(dst), .. } => dst, + LlInst::Call { + dst: Some(dst), + callee, + .. + } if callee == CAPTURE_GET_CALLEE => dst,🤖 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/root_reload.rs` around lines 811 - 815, Narrow the result-bearing LlInst::Call arm in materialize to accept only the CAPTURE_GET_CALLEE callee, while preserving the existing dst extraction for that call. Add an explicit callee check so other transparent calls cannot be materialized accidentally.crates/perry-codegen/src/root_reload_tests.rs (2)
843-881: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a fixture for a capture SET with a non-literal index.
The two fixtures cover literal indices only.
literal_capture_idxreturnsNonefor a register index, and the SET then records nostores_to. No test pins the behavior for that input. A fixture that passes a register as the index operand ofjs_closure_set_capture_bitswould lock down whichever fallback you choose for the gap raised incrates/perry-codegen/src/root_reload.rs.🤖 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/root_reload_tests.rs` around lines 843 - 881, Add a fixture alongside capture_get_chain_with_set that invokes js_closure_set_capture_bits with a register-valued, non-literal index instead of set_idx. Use it in a test covering the root-reload fallback for dynamic capture indices, ensuring the expected stores_to behavior is explicitly pinned.
813-823: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the stale-operand check unconditional.
If
positionreturnsNone, theif letskips the assertion and the test still passes. That is the half of the test that proves the consumer stopped reading the pre-call register. Useexpectso a missing original bitcast fails the test instead of disabling the check.💚 Proposed change
- let orig_bitcast = lines + let stale = lines .iter() .position(|l| l.contains("= bitcast i64 %r") && !recipe.contains(l)) - .map(|i| lines[i].split_whitespace().next().unwrap()); - if let Some(stale) = orig_bitcast { - assert!( - !lines[use_idx].contains(&format!("double {stale},")), - "the consumer must NOT still read the pre-call capture read" - ); - } + .map(|i| lines[i].split_whitespace().next().unwrap()) + .expect("the pre-call bitcast must still be in the IR"); + assert!( + !lines[use_idx].contains(&format!("double {stale},")), + "the consumer must NOT still read the pre-call capture read\n{ir}" + );🤖 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/root_reload_tests.rs` around lines 813 - 823, Make the stale original-bitcast lookup in the test unconditional: replace the optional result handling around `orig_bitcast` with an expectation that fails when no matching pre-call bitcast is found, then always assert that the consumer line does not contain the stale operand. Preserve the existing diagnostic message and operand check.
🤖 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 `@Cargo.toml`:
- Line 318: Revert the per-PR release metadata updates: restore
[workspace.package].version in Cargo.toml (line 318) to 0.5.1424 and restore
Current Version in CLAUDE.md (line 11) to 0.5.1424; leave the changelog fragment
unchanged.
In `@crates/perry-codegen/src/root_reload.rs`:
- Around line 493-499: The capture-chain walk in root_reload.rs around the
capture_get_key handling must preserve both the inherited root_ptr invalidation
key and the capture-index key, invalidating the chain when either the
closure-ptr slot is stored to or the capture index is set; do not replace
root_ptr with capture_get_key. In root_reload_tests.rs around the existing
fixture, add coverage that stores a new value into the closure-ptr shadow slot
during the window and asserts the capture reload is suppressed.
- Around line 915-928: The typed LlInst::Call arm at
crates/perry-codegen/src/root_reload.rs:915-928 and raw call arm at
crates/perry-codegen/src/root_reload.rs:1077-1102 must assign a wildcard capture
key to stores_to when literal_capture_idx or raw_call_literal_arg cannot
determine the SET index. Update the stores_to comparisons in apply_to_function
at lines 615, 633, and 692 so that wildcard invalidation matches every key
produced by capture_slot_key; preserve precise matching for known indices and
GET’s opaque behavior.
- Around line 1148-1152: Update the argument parsing in the surrounding
capture-index function to split `arg.trim()` at the last space rather than the
first, so multi-token types leave only the operand token in `val`. Preserve the
existing empty-value and `%` filtering and returned string behavior.
---
Nitpick comments:
In `@crates/perry-codegen/src/root_reload_tests.rs`:
- Around line 843-881: Add a fixture alongside capture_get_chain_with_set that
invokes js_closure_set_capture_bits with a register-valued, non-literal index
instead of set_idx. Use it in a test covering the root-reload fallback for
dynamic capture indices, ensuring the expected stores_to behavior is explicitly
pinned.
- Around line 813-823: Make the stale original-bitcast lookup in the test
unconditional: replace the optional result handling around `orig_bitcast` with
an expectation that fails when no matching pre-call bitcast is found, then
always assert that the consumer line does not contain the stale operand.
Preserve the existing diagnostic message and operand check.
In `@crates/perry-codegen/src/root_reload.rs`:
- Around line 811-815: Narrow the result-bearing LlInst::Call arm in materialize
to accept only the CAPTURE_GET_CALLEE callee, while preserving the existing dst
extraction for that call. Add an explicit callee check so other transparent
calls cannot be materialized accidentally.
🪄 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: 9acc7cbc-f00b-48a2-bd6b-52f29d0a950e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
.github/workflows/gc-root-dominance.ymlCLAUDE.mdCargo.tomlchangelog.d/7725-capture-bits-reload.mdcrates/perry-codegen/src/root_reload.rscrates/perry-codegen/src/root_reload_tests.rs
|
|
||
| [workspace.package] | ||
| version = "0.5.1424" | ||
| version = "0.5.1425" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Revert the per-PR release metadata updates.
The PR already includes changelog.d/7725-capture-bits-reload.md. Keep version metadata unchanged in the PR. Let maintainers apply the release update during merge or release.
Cargo.toml#L318-L318: restore[workspace.package].versionto0.5.1424.CLAUDE.md#L11-L11: restoreCurrent Versionto0.5.1424.
As per coding guidelines, external contributors must not modify [workspace.package].version. Based on learnings, maintainers own Cargo.toml and CLAUDE.md version metadata when the PR-keyed changelog fragment is present.
📍 Affects 2 files
Cargo.toml#L318-L318(this comment)CLAUDE.md#L11-L11
🤖 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 `@Cargo.toml` at line 318, Revert the per-PR release metadata updates: restore
[workspace.package].version in Cargo.toml (line 318) to 0.5.1424 and restore
Current Version in CLAUDE.md (line 11) to 0.5.1424; leave the changelog fragment
unchanged.
Sources: Coding guidelines, Learnings
| // #7725: a capture-GET call whose ptr operand is already reloadable extends the | ||
| // SAME recipe, but its own store side-condition is "was THIS INDEX set", not | ||
| // "was the closure-ptr slot stored to" — the ptr slot never is (it is written | ||
| // once, at closure entry). Swap the key here so every step from this call | ||
| // onward — including a further transparent cast like `bitcast i64 to double` — | ||
| // is invalidated by the right condition. | ||
| let root = f.capture_get_key.clone().unwrap_or(root); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
A capture chain loses the store condition of its own root load, and no test pins that case. Line 499 replaces the inherited root_ptr with the capture key. root_ptr is the single invalidation key the group walk uses, so the chain stops checking stores to the closure-ptr slot even though recipe[0] still loads from that slot and materialize re-emits that load.
crates/perry-codegen/src/root_reload.rs#L493-L499: carry both keys instead of replacing one with the other, so the chain is invalidated by a store to the inherited root OR by a set to the capture index.crates/perry-codegen/src/root_reload_tests.rs#L883-L911: add a fixture that stores a new value into the closure-ptr shadow slot inside the window, and assert the capture reload is suppressed.
📍 Affects 2 files
crates/perry-codegen/src/root_reload.rs#L493-L499(this comment)crates/perry-codegen/src/root_reload_tests.rs#L883-L911
🤖 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/root_reload.rs` around lines 493 - 499, The
capture-chain walk in root_reload.rs around the capture_get_key handling must
preserve both the inherited root_ptr invalidation key and the capture-index key,
invalidating the chain when either the closure-ptr slot is stored to or the
capture index is set; do not replace root_ptr with capture_get_key. In
root_reload_tests.rs around the existing fixture, add coverage that stores a new
value into the closure-ptr shadow slot during the window and asserts the capture
reload is suppressed.
| // #7725: the two capture-bits halves. GET extends a derivation like a transparent | ||
| // bit op (see CAPTURE_GET_CALLEE); SET's side effect on the capture slot has to | ||
| // invalidate that derivation the way a store does, via the shared `stores_to` | ||
| // machinery. | ||
| if callee == CAPTURE_GET_CALLEE { | ||
| if let Some(idx) = literal_capture_idx(args) { | ||
| transparent = true; | ||
| capture_get_key = Some(capture_slot_key(idx)); | ||
| } | ||
| } else if callee == CAPTURE_SET_CALLEE { | ||
| if let Some(idx) = literal_capture_idx(args) { | ||
| stores_to = Some(capture_slot_key(idx)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
A non-literal capture SET index records no invalidation in either analysis arm. literal_capture_idx and raw_call_literal_arg both return None for a register operand. The GET side handles that safely by staying opaque. The SET side does not: it leaves stores_to as None, so the write becomes invisible to the group walk and a capture reload can be re-materialized below it. An unknown index must invalidate every capture key, not none of them.
crates/perry-codegen/src/root_reload.rs#L915-L928: in the typedLlInst::Callarm, setstores_toto a wildcard capture key whenliteral_capture_idx(args)returnsNone.crates/perry-codegen/src/root_reload.rs#L1077-L1102: apply the same wildcard fallback in the raw arm whenraw_call_literal_arg(rhs, &name, 1)returnsNone.
The two stores_to comparisons in apply_to_function (Line 615, Line 633, Line 692) must then treat the wildcard as matching any key produced by capture_slot_key.
📍 Affects 1 file
crates/perry-codegen/src/root_reload.rs#L915-L928(this comment)crates/perry-codegen/src/root_reload.rs#L1077-L1102
🤖 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/root_reload.rs` around lines 915 - 928, The typed
LlInst::Call arm at crates/perry-codegen/src/root_reload.rs:915-928 and raw call
arm at crates/perry-codegen/src/root_reload.rs:1077-1102 must assign a wildcard
capture key to stores_to when literal_capture_idx or raw_call_literal_arg cannot
determine the SET index. Update the stores_to comparisons in apply_to_function
at lines 615, 633, and 692 so that wildcard invalidation matches every key
produced by capture_slot_key; preserve precise matching for known indices and
GET’s opaque behavior.
| let args_text = &rhs[start..end?]; | ||
| let arg = args_text.split(',').nth(index)?; | ||
| let (_, val) = arg.trim().split_once(' ')?; | ||
| let val = val.trim(); | ||
| (!val.is_empty() && !val.starts_with('%')).then(|| val.to_string()) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Take the last token of the argument, not the tail after the first space.
split_once(' ') assumes the type is a single token. A multi-token type such as ptr addrspace(1) %x makes val become addrspace(1) %x. That string is non-empty and does not start with %, so the function returns it as a literal index. The caller then builds a wrong capture key. On the SET side a wrong key means the write is recorded under an index nobody checks, and the invalidation is lost.
The two capture callees use single-token operand types today, so this does not fire now. rsplit_once removes the assumption at no cost.
🛡️ Proposed fix
- let (_, val) = arg.trim().split_once(' ')?;
+ // The VALUE is the last token; a type can be several tokens
+ // (`ptr addrspace(1)`, `i64 (i64)*`).
+ let (_, val) = arg.trim().rsplit_once(' ')?;
let val = val.trim();📝 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.
| let args_text = &rhs[start..end?]; | |
| let arg = args_text.split(',').nth(index)?; | |
| let (_, val) = arg.trim().split_once(' ')?; | |
| let val = val.trim(); | |
| (!val.is_empty() && !val.starts_with('%')).then(|| val.to_string()) | |
| let args_text = &rhs[start..end?]; | |
| let arg = args_text.split(',').nth(index)?; | |
| // The VALUE is the last token; a type can be several tokens | |
| // (`ptr addrspace(1)`, `i64 (i64)*`). | |
| let (_, val) = arg.trim().rsplit_once(' ')?; | |
| let val = val.trim(); | |
| (!val.is_empty() && !val.starts_with('%')).then(|| val.to_string()) |
🤖 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/root_reload.rs` around lines 1148 - 1152, Update the
argument parsing in the surrounding capture-index function to split `arg.trim()`
at the last space rather than the first, so multi-token types leave only the
operand token in `val`. Preserve the existing empty-value and `%` filtering and
returned string behavior.
Merging as v0.5.1425 — the native lowering is now at ZERO unrooted hazardsI verified the headline claim independently rather than trusting the PR, because a wrong budget makes 21 → 0, and Why the harder fix was the right oneThe obvious alternative — root every capture read at its own site via The bug caught during implementation is the best partInheriting the closure-ptr-slot's invalidation key would have made invalidation vacuous — that slot is written once, at entry, so nothing would ever invalidate the recipe. Caught before shipping, and fixed by switching a capture-GET recipe to a synthetic A reload pass whose invalidation never fires is exactly the shape of bug this project keeps finding — it would have passed every test while doing nothing. Verification
Gates 21/21. (One gate run showed a |
1 participant
Summary
Closes #7725, the follow-up #7724 split out: the last 2
unrooted:capturehits ongc-root-dominance-statepoints, bothjs_closure_get_capture_bits's return value never being re-entered into a protected domain (unlike%this_closureitself, already protected viacurrent_closure_slot, #7055).test_gap_class_expr_dynamic_parent_ctor::__closure_21— a captured dynamic-parent-class reference read at the top of a synthesized implicit constructor, used ~60 lines later asjs_new_function_construct's callee, acrossjs_object_alloc_class_inline_keysand the class's own user constructor.test_gap_computed_key_method_nested_this::__closure_9— a captured numeric local held acrossjs_number_coerce(which can run a userSymbol.toPrimitive).Confirming the framing before implementing
The task was to verify #7724's diagnosis — that this needs
root_reload.rs'sFactsto model a call as a reloadable source, not just a load — rather than assume it and start coding.I checked the alternative: root every capture read at its own call site via the existing
TempRootPool(rooting/temp_root.rs). Rejected — it would pay a store + shadow-slot bind on every capture read, including the overwhelming majority never held across a collection point, whereroot_reload.rs's whole design point is to cost nothing where the reload turns out to be unnecessary (LLVM CSEs a redundant one away; a needed one stays because the intervening call is opaque). The generic-read call sites (literals_vars.rs,closure.rs,array_push.rs,instance_misc1.rs,lower_array_method.rs,expr/mod.rs) also have no way to know at emission time whether this particular read will cross a later collection — that's exactly the questionroot_reload.rs's post-hoc CFG walk exists to answer cheaply, and it's the same question it already answers for string-handle globals and shadow-slot masks. So theFacts-as-call extension is the right layer, confirmed rather than assumed.The fix
js_closure_get_capture_bits(ptr, idx)calls whoseptroperand already belongs to a reloadable recipe (the closure-ptr shadow-slot load chain #7055 protects) now extend that recipe exactly like the existing transparentand/bitcaststeps —Factsgainedcapture_get_key, set when the call's index argument is a compile-time literal (every real emission site formats one directly, confirmed by grep). A reload point re-materializes the whole chain: a fresh slot load, a fresh mask, a fresh call tojs_closure_get_capture_bitswith the same index, and any further cast — not just a load.The store side-condition needed its own key rather than the inherited closure-ptr-slot one (that slot is written exactly once, at closure entry, so checking it there would make the invalidation vacuous — a real soundness gap I caught while implementing, not something I noticed after). A capture-GET recipe's
root_ptrswitches to a synthetic$closure_capture:<idx>key from that call onward;js_closure_set_capture_bitspopulates the existingstores_tofield with the same key, reusing the store-invalidation machinery a plainstorealready has rather than adding a parallel one. That required widening the reload pass's grouping key fromrecipe[0]alone to(recipe[0], root_ptr), since before this a chain'sroot_ptrnever changed mid-derivation — every pre-existing chain has oneroot_ptrthroughout, so the wider key is additive and provably doesn't change any existing (non-capture) grouping.raw_facts(theinvoke/tryrendering path) gets the samestores_todetection for the SET half; the GET half is deliberately gated on!is_invoke, sincematerializere-emits a recipe step as a plain mid-block instruction and aninvokeis a terminator — cloning one in would be invalid IR. In practice this call is never seen there except as an invoke (every real emission goes through the typedLlInst::Callform otherwise), so the guard is defensive rather than load-bearing today.Before / after (gated mode, both lowerings)
--statepoints --moving-only, native corpus--lowering shadow(dominance /--unrooted-allocas/--stale-registers)--stale-registershits, unrelated to capture-bits, unchanged, well under the 39 budget)Reached 0, so
gc-root-dominance-statepoints'--max-unrootedis deleted (not set to0) — the flag already defaults to 0, and a budget nobody re-measures is the exact failure mode #7706 fixed for a different gate on this same file: it silently absorbs the next real hazard instead of going red.Test plan
python3 scripts/gc_root_dominance_check.py --self-test— clean, including gc: fix root-dominance phi false positives + 3 of 5 real hits, lower --max-unrooted to 2 (#7664) #7724'sphi_safe_edge/phi_hazard_edgefixtures (confirms this change doesn't touch the checker)PERRY_RUNTIME_DIRpinned to the build under test both times):--statepoints --moving-only --max-unrooted 999 --max-stale 999→ 2 → 0 unrooted, stale held at 0--min-statepoints 15000 --min-live-bundles 8000 --min-relocates 20000 --max-unrooted 0 --max-stale 0 --seeded-violations 40) passes; all 40 planted violations still caught--lowering shadowcorpus (131/131 compiled): dominance,--unrooted-allocas --moving-only,--stale-registers --moving-onlyall cleancargo test -p perry-codegen --lib— 809 passed (805 onorigin/main+ 4 new tests), 0 failedcargo test -p perry-codegen --no-fail-fast(the full suite includingcrates/*/tests/*.rs, which per-PR CI does not run) — the same 6 pre-existing failures across the same 4 targets (large_object_barriers,native_proof_buffer_views,native_proof_regressions,typed_shape_descriptors) reproduce byte-identically on a freshorigin/maincheckout; zero new failurescargo fmt --all -- --check/scripts/check_file_size.sh— cleanroot_reload.rscrossed the 2,000-line file-size cap gaining this fix; its#[cfg(test)] mod testsmoved to a siblingroot_reload_tests.rs(the existinglinker.rs/type_analysis.rs#[path]idiom), which is also where the four new capture-bits tests landed.Summary by CodeRabbit
Bug Fixes
Tests
Documentation
Chores