gc: close the strhandle, derived-mask and new.target unrooted hazards in the native lowering (#7664) - #7667
Conversation
📝 WalkthroughWalkthroughThe PR extends root reload analysis to handle string-handle globals and derived values. It adds rooted ChangesGC rooting and stale-register handling
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant root_reload
participant Facts
participant materialize_recipe
participant stale_instruction
root_reload->>Facts: analyze reloadable globals and transparent operations
Facts->>root_reload: return bounded derivation recipes
root_reload->>materialize_recipe: clone recipe and rename operands
materialize_recipe->>stale_instruction: insert fresh instructions before stale use
🚥 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 |
70b2434 to
e6f874a
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: 2
🧹 Nitpick comments (1)
crates/perry-codegen/src/root_reload.rs (1)
1695-1756: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for a multi-step recipe inserted at or above the entry-init boundary.
the_masked_receiver_is_re_derived_not_just_the_loadpins the three-instruction recipe.an_insertion_below_the_post_init_splice_does_not_move_itat Line 1438 pins the boundary arithmetic, but only for a single-instruction reload.The combination is untested: a masked-receiver rewrite at or above
entry_init_boundarynow advances the boundary byrecipe.len()rather than by one (Line 610-617). That arithmetic is what the comment at Line 599-605 calls out as the regression that cost the acceptance arm 30/30 → 0/30, and it changed in this PR.Two smaller soundness branches are also unpinned:
- Line 449-451 declines a recipe longer than
MAX_RECIPE.- Line 430-437 declines a transparent op whose operands come from two different roots.
Both are one-sided rejections, so a regression in either widens the pass silently rather than failing a test.
🤖 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 1695 - 1756, Add regression coverage alongside the existing masked-receiver tests for a multi-instruction recipe inserted at or above entry_init_boundary, asserting the boundary advances by recipe.len() and the full recipe is re-emitted correctly. Add tests covering rejection of recipes longer than MAX_RECIPE and transparent operations whose operands originate from different roots, verifying neither is widened or rewritten. Reuse the existing test helpers and preserve the current positive masked_receiver behavior.
🤖 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 @.github/workflows/gc-root-dominance.yml:
- Line 515: Update the diagnostic text associated with the gated arm near the
max-unrooted configuration to report 7 hazards, matching --max-unrooted 7.
Preserve 21 only in the historical pre-fix count near the earlier baseline.
In `@crates/perry-codegen/src/root_reload.rs`:
- Around line 655-690: Update materialize to return None on any step lacking an
instruction result, abandoning the rewrite without returning partial steps or a
stale register. In the rewrite call site, skip failed materializations and only
rename operands and append steps for successful results. Ensure entry_inserts
and note_entry_block_insertions count only steps actually emitted, using the
same success condition as materialize or the final reloads length.
---
Nitpick comments:
In `@crates/perry-codegen/src/root_reload.rs`:
- Around line 1695-1756: Add regression coverage alongside the existing
masked-receiver tests for a multi-instruction recipe inserted at or above
entry_init_boundary, asserting the boundary advances by recipe.len() and the
full recipe is re-emitted correctly. Add tests covering rejection of recipes
longer than MAX_RECIPE and transparent operations whose operands originate from
different roots, verifying neither is widened or rewritten. Reuse the existing
test helpers and preserve the current positive masked_receiver behavior.
🪄 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: 8cc7f228-fdf8-4627-86f7-093cfb6055c5
📒 Files selected for processing (5)
.github/workflows/gc-root-dominance.ymlcrates/perry-codegen/src/inst.rscrates/perry-codegen/src/lower_call/new.rscrates/perry-codegen/src/root_reload.rscrates/perry-codegen/src/rooting.rs
| --min-live-bundles 8000 \ | ||
| --min-relocates 20000 \ | ||
| --max-unrooted 21 \ | ||
| --max-unrooted 7 \ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the diagnostic baseline from 21 to 7.
This step now gates --max-unrooted 7, but the diagnostic text at Line 528 still says that the gated arm has 21 hazards. Change that text to 7. Keep 21 only as the historical pre-fix count at Line 475.
Proposed text update
- # ... against the gated arm's 21.
+ # ... against the gated arm's 7.🤖 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 @.github/workflows/gc-root-dominance.yml at line 515, Update the diagnostic
text associated with the gated arm near the max-unrooted configuration to report
7 hazards, matching --max-unrooted 7. Preserve 21 only in the historical pre-fix
count near the earlier baseline.
| /// Re-emit a derivation with fresh registers, returning the instructions in | ||
| /// order and the register the last one defines. | ||
| /// | ||
| /// A recipe is self-contained by construction — a load from the root location | ||
| /// plus pure bit ops whose every register operand is an earlier step — so the | ||
| /// only rewriting needed is step-to-step: each step's operands are renamed to | ||
| /// the fresh names of the steps it consumed. The root pointer (`%slot` or | ||
| /// `@…handle`) is not a step, is never in the map, and is therefore carried | ||
| /// through untouched, which is exactly what makes this a RE-READ. | ||
| fn materialize( | ||
| recipe: &[LlInst], | ||
| counter: &std::rc::Rc<crate::block::RegCounter>, | ||
| ) -> (Vec<LlInst>, String) { | ||
| let mut out: Vec<LlInst> = Vec::with_capacity(recipe.len()); | ||
| let mut renames: Vec<(String, String)> = Vec::with_capacity(recipe.len()); | ||
| let mut last = String::new(); | ||
| for step in recipe { | ||
| let mut step = step.clone(); | ||
| for (old, new) in &renames { | ||
| rename_operand(&mut step, old, new); | ||
| } | ||
| let old_dst = match inst_result(&step) { | ||
| Some(d) => d, | ||
| // Only loads and pure bit ops become recipe steps, and all three | ||
| // define a register. Bail rather than emit a step whose result | ||
| // nothing can name. | ||
| None => return (Vec::new(), last), | ||
| }; | ||
| let fresh = format!("%r{}", counter.next()); | ||
| set_inst_result(&mut step, &fresh); | ||
| renames.push((old_dst, fresh.trim_start_matches('%').to_string())); | ||
| last = fresh; | ||
| out.push(step); | ||
| } | ||
| (out, last) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make the materialize bail abandon the rewrite instead of returning an unusable register.
The bail at Line 681 returns (Vec::new(), last). Two things go wrong at the call site.
outis discarded, so any steps already materialised are lost.lastis returned anyway. On the first step it isString::new(); on a later step it names an instruction that was just discarded.
The caller at Line 638-640 uses that value unconditionally:
let (steps, fresh) = materialize(&r.recipe, &counter);
rename_operand(&mut insts[insn], &r.from, fresh.trim_start_matches('%'));The stale operand is renamed to % or to an undefined register, and no defining instruction is inserted. The emitted IR does not verify.
The same bail also breaks the entry-boundary count at Line 610-617. entry_inserts sums r.recipe.len(), but a bailed rewrite inserts zero instructions. note_entry_block_insertions then advances entry_init_boundary past the real insertion count — the over-count failure the comment at Line 599-605 and the test at Line 1437 describe.
Today's admission rules make the bail hard to reach, because raw_facts and inst_result parse the same LHS shape. The bail exists as a guard, so it should fail safe.
🛠️ Proposed fix: return `Option` and skip the rewrite on failure
fn materialize(
recipe: &[LlInst],
counter: &std::rc::Rc<crate::block::RegCounter>,
-) -> (Vec<LlInst>, String) {
+) -> Option<(Vec<LlInst>, String)> {
let mut out: Vec<LlInst> = Vec::with_capacity(recipe.len());
let mut renames: Vec<(String, String)> = Vec::with_capacity(recipe.len());
let mut last = String::new();
for step in recipe {
let mut step = step.clone();
for (old, new) in &renames {
rename_operand(&mut step, old, new);
}
- let old_dst = match inst_result(&step) {
- Some(d) => d,
- // Only loads and pure bit ops become recipe steps, and all three
- // define a register. Bail rather than emit a step whose result
- // nothing can name.
- None => return (Vec::new(), last),
- };
+ // Only loads and pure bit ops become recipe steps, and all three
+ // define a register. Abandon the whole rewrite rather than emit a
+ // step whose result nothing can name — a partial recipe would leave
+ // the stale operand renamed to an undefined register.
+ let old_dst = inst_result(&step)?;
let fresh = format!("%r{}", counter.next());
set_inst_result(&mut step, &fresh);
renames.push((old_dst, fresh.trim_start_matches('%').to_string()));
last = fresh;
out.push(step);
}
- (out, last)
+ Some((out, last))
}Then skip the rewrite at the call site and count only what is emitted:
// lines 636-641
let mut reloads: Vec<LlInst> = Vec::new();
for r in &rewrites[i..j] {
let Some((steps, fresh)) = materialize(&r.recipe, &counter) else {
continue;
};
rename_operand(&mut insts[insn], &r.from, fresh.trim_start_matches('%'));
reloads.extend(steps);
}entry_inserts is computed before this loop, so also derive it from the same predicate materialize uses, or move the boundary note after the loop and count reloads.len() for block-0 insertions at or above the boundary.
🤖 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 655 - 690, Update
materialize to return None on any step lacking an instruction result, abandoning
the rewrite without returning partial steps or a stale register. In the rewrite
call site, skip failed materializations and only rename operands and append
steps for successful results. Ensure entry_inserts and
note_entry_block_insertions count only steps actually emitted, using the same
success condition as materialize or the final reloads length.
e6f874a to
ceab0f0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/perry-codegen/src/root_reload.rs (1)
1863-1890: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider two more cases: the
MAX_RECIPEbound and two same-root operands.Two changed behaviours have no coverage.
MAX_RECIPEat Line 236 and therecipe.len() > MAX_RECIPErejection at Line 449 are untested. A chain of nine transparent steps must be declined. An off-by-one in that bound would ship silently.- The comment at Line 600-604 describes one instruction reading two values of the SAME root, and
seen_hereat Line 605 exists for it. The existingboth_stale_operands_of_one_instruction_are_reloadedtest uses two different slots, so the same-root path is not exercised. A fixture that passes both the load and the mask of one slot tojs_object_set_field_by_namewould cover it.I can draft both tests if that is useful.
🤖 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 1863 - 1890, Add coverage for the MAX_RECIPE limit by constructing a chain of nine transparent derivation steps and asserting apply_to_function declines it, including the recipe.len() > MAX_RECIPE rejection path. Add a same-root fixture alongside both_stale_operands_of_one_instruction_are_reloaded where js_object_set_field_by_name receives both the loaded value and mask derived from the same slot, exercising seen_here and asserting both operands are handled correctly.
🤖 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/src/root_reload.rs`:
- Around line 466-468: Update the size guard in the root reload construction
around the fixpoint-derived values so it uses the root-load count rather than
values.len(). Preserve the existing MAX_BLOCK_LOAD_PRODUCT threshold and
saturating multiplication, while ensuring the guard reflects the grouping cost
described near the grouping logic.
- Around line 808-814: Update the LlInst::Load handling in the reload-root
detection path to record load_of only for non-volatile, non-atomic loads,
matching raw_facts. Bring LoadFlavor into scope if needed and gate the existing
reloadable_ptr logic on the plain load flavor, while preserving register and use
tracking for all loads.
---
Nitpick comments:
In `@crates/perry-codegen/src/root_reload.rs`:
- Around line 1863-1890: Add coverage for the MAX_RECIPE limit by constructing a
chain of nine transparent derivation steps and asserting apply_to_function
declines it, including the recipe.len() > MAX_RECIPE rejection path. Add a
same-root fixture alongside both_stale_operands_of_one_instruction_are_reloaded
where js_object_set_field_by_name receives both the loaded value and mask
derived from the same slot, exercising seen_here and asserting both operands are
handled correctly.
🪄 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: 6f40a1fe-a755-4c78-b629-bb8292978783
📒 Files selected for processing (3)
changelog.d/7667-native-lowering-unrooted-hazards.mdcrates/perry-codegen/src/root_reload.rscrates/perry-codegen/src/rooting.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/perry-codegen/src/rooting.rs
| if blocks.len().saturating_mul(values.len()) > MAX_BLOCK_LOAD_PRODUCT { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
The size guard now counts derived values, so it bails earlier than before.
values previously held only root loads. It now also holds every derived value admitted by the fixpoint, so blocks.len() * values.len() can exceed MAX_BLOCK_LOAD_PRODUCT on functions that passed the guard before this change. Those functions lose the slot reloads they used to get, which is a silent coverage regression rather than a cost saving.
The comment at Line 519 states that grouping by root load puts the cost back at O(blocks × loads). The reachability walk runs once per group, and the group count equals the root-load count, so the guard can use that number instead of values.len().
♻️ Proposed change: measure the guard against the root-load count
- if blocks.len().saturating_mul(values.len()) > MAX_BLOCK_LOAD_PRODUCT {
+ // The walk below runs once per ROOT LOAD, not once per reloadable value,
+ // so the cost bound is stated in root loads. Counting derived values here
+ // would decline functions the pre-#7664 pass handled.
+ let root_loads = values
+ .iter()
+ .filter(|v| v.recipe.len() == 1)
+ .count();
+ if blocks.len().saturating_mul(root_loads) > MAX_BLOCK_LOAD_PRODUCT {
return 0;
}📝 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.
| if blocks.len().saturating_mul(values.len()) > MAX_BLOCK_LOAD_PRODUCT { | |
| return 0; | |
| } | |
| // The walk below runs once per ROOT LOAD, not once per reloadable value, | |
| // so the cost bound is stated in root loads. Counting derived values here | |
| // would decline functions the pre-#7664 pass handled. | |
| let root_loads = values | |
| .iter() | |
| .filter(|v| v.recipe.len() == 1) | |
| .count(); | |
| if blocks.len().saturating_mul(root_loads) > MAX_BLOCK_LOAD_PRODUCT { | |
| return 0; | |
| } |
🤖 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 466 - 468, Update the
size guard in the root reload construction around the fixpoint-derived values so
it uses the root-load count rather than values.len(). Preserve the existing
MAX_BLOCK_LOAD_PRODUCT threshold and saturating multiplication, while ensuring
the guard reflects the grouping cost described near the grouping logic.
| LlInst::Load { dst, ty, ptr, .. } => { | ||
| result = reg(dst); | ||
| use_op(&mut uses, ptr); | ||
| if let (Some(d), Some(p)) = (reg(dst), reg(ptr)) { | ||
| if slots.contains(&p) { | ||
| load_of = Some((d, *ty, p)); | ||
| } | ||
| if let (Some(d), Some(p)) = (reg(dst), reloadable_ptr(ptr, slots)) { | ||
| load_of = Some((d, *ty, p)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exclude volatile and atomic loads here, as the raw parser does.
raw_facts at Line 918 requires !rhs.contains("volatile") && !rhs.contains("atomic") before it records load_of. This arm applies no flavor check, so a LoadFlavor::Volatile or LoadFlavor::AtomicSeqCst load becomes a reload root and is re-executed at every stale use. Re-executing a volatile load duplicates an observable operation.
No current lowering emits a volatile load of a shadow slot or a handle global, so this is latent. Close it here so the two parsers state the same rule.
🛡️ Proposed fix: gate on the plain flavors
- LlInst::Load { dst, ty, ptr, .. } => {
+ LlInst::Load {
+ dst,
+ ty,
+ ptr,
+ flavor,
+ } => {
result = reg(dst);
use_op(&mut uses, ptr);
- if let (Some(d), Some(p)) = (reg(dst), reloadable_ptr(ptr, slots)) {
- load_of = Some((d, *ty, p));
- }
+ // Same exclusion the Raw parser states: re-executing a volatile or
+ // atomic load would duplicate an observable operation.
+ let re_readable = !matches!(
+ flavor,
+ LoadFlavor::Volatile | LoadFlavor::AtomicSeqCst(_)
+ );
+ if re_readable {
+ if let (Some(d), Some(p)) = (reg(dst), reloadable_ptr(ptr, slots)) {
+ load_of = Some((d, *ty, p));
+ }
+ }
}LoadFlavor needs to be in scope; import it alongside LlInst if it is not already.
📝 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.
| LlInst::Load { dst, ty, ptr, .. } => { | |
| result = reg(dst); | |
| use_op(&mut uses, ptr); | |
| if let (Some(d), Some(p)) = (reg(dst), reg(ptr)) { | |
| if slots.contains(&p) { | |
| load_of = Some((d, *ty, p)); | |
| } | |
| if let (Some(d), Some(p)) = (reg(dst), reloadable_ptr(ptr, slots)) { | |
| load_of = Some((d, *ty, p)); | |
| } | |
| } | |
| LlInst::Load { | |
| dst, | |
| ty, | |
| ptr, | |
| flavor, | |
| } => { | |
| result = reg(dst); | |
| use_op(&mut uses, ptr); | |
| // Same exclusion the Raw parser states: re-executing a volatile or | |
| // atomic load would duplicate an observable operation. | |
| let re_readable = !matches!( | |
| flavor, | |
| LoadFlavor::Volatile | LoadFlavor::AtomicSeqCst(_) | |
| ); | |
| if re_readable { | |
| if let (Some(d), Some(p)) = (reg(dst), reloadable_ptr(ptr, slots)) { | |
| load_of = Some((d, *ty, p)); | |
| } | |
| } | |
| } |
🤖 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 808 - 814, Update the
LlInst::Load handling in the reload-root detection path to record load_of only
for non-volatile, non-atomic loads, matching raw_facts. Bring LoadFlavor into
scope if needed and gate the existing reloadable_ptr logic on the plain load
flavor, while preserving register and use tracking for all loads.
…#7664) `gc-root-dominance-statepoints`' `--max-unrooted` ratchet goes 21 -> 7. #7663 pointed the root-dominance rule at the NATIVE root lowering -- the one that ships since #7370 -- and reported 21 `unrooted` hazards. Fourteen were shapes `root_reload.rs` looked straight through, because its rule is stated over the load's own register and in both shapes the value at risk lives somewhere else. 1. The root is a GLOBAL, not an alloca (10 hits). A string literal lowers to `load double, ptr @<mod>_.str.N.handle`; the handle global is a registered root, so the string is never swept, and an evacuating cycle REWRITES the global while a register loaded beforehand keeps the pre-move address. #7240's shape, whose fix covered call operands only. 2. The stale register is DERIVED from the load (3 of 7 unmasked receivers). `this.count++` holds the unmasked receiver across the property GET; the load's only use is the bitcast ABOVE the collecting call, so the window was empty and the function took zero reloads. 3. `new.target`'s saved previous value (1 hit). `new.rs` saved `js_new_target_get()` in a bare register across the whole constructor body; the cell is a registered mutable root, so the restore publishes a pre-move address back INTO a root the collector scans. #7226's `prev_this` bug for `new.target`. The window is anchored at the ROOT LOAD, not at the derived value. Anchoring at the derivation looks more precise and is wrong: `main`'s class-object read has the scope-end shadow-slot clear landing between the load and the mask, so a walk starting at the mask never sees it and re-read a slot the program had just nulled -- `(makeAnon(77) as any).v` became `undefined`. Caught by an A/B against the branch point on `test_gap_class_expr_identity`, not by the dominance checker, which cannot see a value-correctness bug. The reload rule is restated over the value's derivation rather than its register: for a value read out of a collector-rewritten location -- a shadow slot or a string-handle global -- and any value derived from it by pure bit ops, every use a collection point can reach re-materialises the whole derivation. A recipe is extended only through ops that are pure functions of their operands and whose every register operand is already in the same single root's recipe, which makes it self-contained and materialisable anywhere. Grouping by root load also puts the cost back at O(blocks x loads). `new.target` gets `new_target_save`/`new_target_restore` in `crate::rooting`, structurally `implicit_this_save`/`implicit_this_restore`. Re-reading the cell would be the wrong repair: `js_new_target_set` has already overwritten it. Measured on `Counter__increment`: before, all three statepoints carried an EMPTY live set, so the receiver was marked by nothing; after, each carries a "gc-live" bundle and a `gc.relocate`, and the SET reads a mask re-derived from the relocated pointer plus a fresh load of the handle global. Remaining 7, each its own slice: 4 unmasked are phi-mediated (the reload has to go in the predecessor, on the edge); 2 `@perry_global_*` are module-level variables the program assigns, so they need rooting rather than reloading (pinned by `a_module_global_is_not_a_reload_source`); 1 capture read. #7664 stays open as the budget's referent. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
Audit — merging as v0.5.1382. Ratchet 21 → 7.The root cause is the finding. All 14 fixes trace to one thing: The IR evidence checked: The regression you introduced, caught, and pinnedAnchoring each derived value's window at its own definition was wrong: Two things about that are worth more than the fix. You found it by A/B against the branch point, not by the dominance checker — and correctly noted the checker cannot find it, because it sees rooting, not value-correctness. And you pinned it with the controlled twin of the positive case: same frame plus one store, verdict flips 1 → 0. I verified the shipped version myself: Both corrections acceptedThe issue's shape-1 heading says 9 The remaining 7, each correctly its own slice4 phi-mediated (no instruction can go above a phi; the reload belongs in the predecessor on the edge — a different insertion model), 2 Not allowlisted, #7664 stays open as the budget's referent, and the budget is exact — Gates: 22/22 lint, fmt clean, |
ceab0f0 to
6ae6795
Compare
#7667 added new_target_save using crate::expr::temp_root while this slice moved the module to crate::rooting::temp_root. The two PRs were developed in parallel; the break only appears once both are on the same tree. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
…#7670) * refactor(codegen): split the instance allocation out of lower_call/new.rs (#7615) `new.rs` was 1,988 lines against `scripts/check_file_size.sh`'s 2,000-line cap, which blocked the Layer 1 rooting migration (#7615 slice 8) — that migration has to ADD lines to the file, replacing `refresh_rooted_args` and the `temp_root_scope_*` marker with a `RootedGroup`. Pure move, no behaviour change: `lower_new_impl_inner`'s field-count computation and its three-arm object allocation become `new_alloc::emit_instance_alloc(ctx, class_name, class) -> String`. `new_site_is_in_loop` moves with them (its only caller is the inline bump-allocator arm). The boundary is a boundary rather than a cut because none of the locals the block defines — `field_count`, `cid_str`, `parent_cid_str`, `n_str`, `packed_keys`, `alloc_field_count` — is read anywhere below the allocation. No rooting decision moves with it: everything the extracted block emits sits ABOVE the instance root, whose push is the caller's next act on the returned handle. new.rs 1,988 -> 1,501; new_alloc.rs 531. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix * refactor(codegen): split string concat out of lower_string_method.rs (#7615) `lower_string_method.rs` was 1,957 lines against the 2,000-line cap, and the Layer 1 rooting migration adds closure scopes to five of its functions — `with_operands_rooted` and `with_rooted_accumulator` both re-indent the body they own, which is line growth on a file with 43 lines of headroom. Pure move at the boundary the file already had: everything above `lower_string_self_append` dispatches a `str.<method>(...)` call, everything from it down lowers `a + b` / `s += x` on strings. `str_operand_handle_tag_dispatched` becomes `pub(crate)` because three dispatch arms above still call it. lower_string_method.rs 1,957 -> 1,368; lower_string_concat.rs 612. Also lands the first four module migrations of slice 8 (they share the `expr/binary.rs` import line with the move): * `expr/binary.rs` — five `lower_operand_pair_rooted` + `temp_root_release` pairs collapse into one `lower_rooted_dynamic_binary` helper over `with_operands_rooted`. * `expr/math_simple.rs` — `MapSet` becomes a `RootedGroup` (two operands, unequal windows, eight arm-specific re-read points); `MapGet`/`MapHas` become `with_operands_rooted`. `Expr::ArrayMap` gains the root it never had: the receiver was lowered, the callback was lowered, and only THEN was the receiver unboxed — the unbox sat below its own window. * `expr/static_field_meta.rs` — `ClassExprFresh` becomes a `RootedGroup` over the class object plus a nested `with_rooted_accumulator` for the `__perry_ctor_caps` snapshot array, which was threaded through a bare SSA register. * `expr/dyn_extern_i18n.rs` — the namespace-object build becomes `with_rooted_accumulator`. * `lower_call/new.rs` — `refresh_rooted_args` and the `temp_root_scope_begin`/`_end` marker become one escaping `RootedGroup`; the null marker slot is gone with them. `RootedGroup::adopt_emitted` gains a `protect` flag (the WINDOW, not the strategy) and `RootedGroup::is_rooted` returns whether a slot exists. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix * fix(gc): Layer 1 rooting slice 8 — the raw API becomes unreachable (#7615) The campaign's terminal condition, made true: `expr/temp_root.rs` is now `crate::rooting::temp_root`, declared with a PRIVATE `mod temp_root;` and with every accessor additionally carrying `pub(in crate::rooting)`. The plan spelled the condition as "`expr/temp_root.rs` going `pub(in crate::rooting)`", which is not expressible in Rust — `pub(in path)` requires `path` to be an ancestor module of the item (E0742), and `crate::rooting` is not an ancestor of `crate::expr::temp_root`. Hence the move. Both belts are worn because either alone is one keyword from being undone. Two items keep `pub(crate)` and are re-exported from `rooting/mod.rs`; neither is an accessor and neither can be called in the wrong order: `TempRootPool` (compile-time slot bookkeeping `FnCtx` owns) and `expr_is_inert_primitive` (the shared "can evaluating this run user code?" predicate the loop back-edge poll consults). Fourteen items are DELETED rather than narrowed, because the migration left them with no caller: `lower_exprs_rooted`, `lower_operand_pair_rooted`, `any_later_ref_may_trigger_gc`, `RootedOperands::is_rooted`, the whole `StoreOperandGuard` family and the whole `RootedHandle` family, and `temp_root_scope_begin`/`_end`. CLAUDE.md's kill-policy: the losing mode should stop compiling. Eight modules migrate (seven load-bearing on the committed source, one — `lower_call/new_alloc.rs` — vacuous and listed anyway so an unlisted sibling of a listed module cannot become the place a raw push goes): `expr/binary.rs`, `expr/math_simple.rs`, `expr/static_field_meta.rs`, `expr/dyn_extern_i18n.rs`, `lower_string_method.rs`, `lower_string_concat.rs`, `lower_call/new.rs`, `lower_call/new_alloc.rs`. Nine further files mention the raw API and make no rooting decision, so they are deliberately NOT listed: `expr/mod.rs` (module declaration and a field type, both gone with the move), the four `FnCtx` constructors (`TempRootPool::default()`), `stmt/loops.rs` (one purity predicate), `loop_purity.rs` (a doc link only), and `root_reload.rs` / `gc_call_effects.rs` / `runtime_decls/arrays.rs` plus five test files, whose `js_gc_temp_root_*` occurrences are runtime SYMBOL NAMES. One live bug fixed: `Expr::ArrayMap` lowered the receiver, lowered the callback, and only then unboxed the receiver — the unbox sat below its own window and masked a stale box rather than repairing it (#7280 taxonomy (c)). New: a terminal-condition test over `temp_root.rs`'s own source, with its own sabotage arm. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix * test(gc): pin the slice-8 windows, and record two ways the pin was vacuous (#7615) Four lowering tests over emitted IR, plus the terminal-condition test's ledger entry and the campaign's close-out in docs/engine-plan.md. ★ Both vacuities were MEASURED by the sabotage arm (restore the pre-fix `Expr::ArrayMap` lowering, require red), not reasoned about: 1. Slice 7's `assert_operand_survives_the_window` compares the operand register's OWN definition line against the window. For `ArrayMap` that register is `and i64 %stale, POINTER_MASK` — emitted BELOW the window while masking a value loaded above it. A one-level check cannot see "the unbox sits below its own window", which is the bug. These tests chase the definition chain through pure bit-twiddling to the first real producer. 2. An array-typed LOCAL receiver has no window at all: codegen's `ptr addrspace(1)` retype pass rematerialises the load from the local's own root slot at the use site, so the pre-fix code re-read the receiver by accident. The windows that are real — verified by A/B on emitted IR against a `main` baseline — are the receivers with no slot to rematerialise from: a module global, a class-field read and a closure capture. The tests use the field read. The window is anchored on the LATER OPERAND'S producer rather than on "the last object allocation above the call", because which helper an `Expr::Object` lowering reaches for is not this module's property. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix * docs(codegen): repoint the TempRootPool doc link after the move (#7615) Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix * fix(codegen): repoint new_target_save at the moved temp_root module #7667 added new_target_save using crate::expr::temp_root while this slice moved the module to crate::rooting::temp_root. The two PRs were developed in parallel; the break only appears once both are on the same tree. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix * chore: bump version to 0.5.1384 Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
1 participant
Closes the last piece of #7480 and
docs/engine-plan.mditem 6: elementPtr<Shape>for object-literal element types.Measured
Pinned quiet mini, release, 7 interleaved rounds, 200k elements x 50 sweeps,
runtime-derived bounds, checksums equal in every cell,
rustc/cargoat zeroand 94.6–94.9% idle before and after.
keep: {v, w}[]— #7480's own kernelkeep: Node[]— what #7612 covered{v, w}[](#7034 §3, E1–E5)34x on the object-literal arm, to parity with both engines. The
named-class arm is unchanged, and so is the region-local arm — that one was
already covered by
collectors/ptr_shape_elements.rs, which is why thischange extends the versioned-loop consumer (the parameter/global case) rather
than that pass.
Both arms link byte-identical
libperry_runtime.a/libperry_stdlib.a(sha256 equal), so this is a codegen-only change and the meaningful control is
the emitted IR, below.
What changed
stmt/element_shape_loop.rs::element_class_nameresolvedArray(Named(C))only. It now also resolves a declared object type to the
__AnonShape_<hash>class its literals allocate, by matching the declared property order against
the module's anon shapes. The hash cannot be recomputed —
mint_anon_shape_classkeys it on the literal's inferred value types (
{v: 1}tagsi, notn)while the annotation says
number— so the class is found, not derived.Ambiguity declines rather than guessing, because
ctx.classesis aHashMapand "first match wins" would make the emitted code depend oniteration order.
receiver_class_nameis not widened. That is the #6377 blast radius #7612deliberately refused; instead the clone is made self-contained. Its
ElementShapeLoopFactalready carried the class name and packed slot index, sothe three sites that would otherwise re-derive the class from the receiver now
go through one predicate,
expr::element_shape_loop_fact_for_property_get:lower_raw_f64_class_field_get_for_number_context— the interception movedabove the
receiver_class_namegate;type_analysis::is_numeric_expr'sPropertyGetarm;expr::binary's arithmetic-operand router (as a disjunct, rather than bywidening
expr_may_return_boxed_value_from_raw_f64_fallback, which wouldhave been a lie — this read has no boxed fallback).
All three are scoped to the fast clone: outside one the fact vector is empty,
so
keep[j].vanywhere else is byte-for-byte what it was.The issue's cost model was wrong, and the correction is load-bearing
#7480 records "no out-of-line guard calls, the cost is stacked inline
diamonds". The object-literal arm actually carried three calls per
iteration, the third being
js_dynamic_string_or_number_add: with noresolvable class the accumulator loses its numeric proof, so
+is not anfadd. The plan called that "a second, separable lever".It is not separable. The clone is admitted only if it is provably
call-free (
LlBlock::contains_gc_unsafe_callcounts every non-llvm.call),so resolving the element class without restoring the numeric proof emits the
clone, fails the call-free test, branches unconditionally to the slow arm and
buys exactly zero at a cost in code size. Both had to land together, and the
numeric claim inside the clone is stronger than the annotation it replaces:
the residual per-element check already proves
GC_OBJ_TYPED_LAYOUT_INTACT,i.e. that the slot holds a raw double.
IR evidence (
--trace llvm, release,PERRY_NO_AUTO_OPTIMIZE=1)sweep, object-literal kernel, before — no clone at all, zerofadd:After — the fast clone is
gep+ load, a three-load residual check,fadd:The remaining calls in the function are the preheader's
(
js_array_refresh_local_head,js_array_ensure_element_shape) and the slowclone's, which is unchanged.
Correctness against the #7660 shape
Every new gap case that reads a
{v, w}[]crossesMIN_ARRAY_CAPACITY, so thegrowth-forwarding stub the #7660 repair exists for is live on this arm too:
callee-built-and-returned, callee-filled-caller-owned, a 17-element prefix, and
a module-global array read from inside a function (the write-back's
module_globalsarm). No new preheader or base derivation was added — theelement-shape preheader is the one #7660 fixed, unchanged.
The gap test also pins the hazards specific to this arm:
rows.lengthbound, which the matcher rejects (it is aPropertyGet, not anInteger/LocalGet), so a kernel written the obviousway gets no clone and must still print the same number;
re-runs the current iteration and the accumulator turns into a string exactly
where JS says it does;
{v: number, w: number}vs{v: string, w: string}), so a mis-resolved shape would cost the clone andnever the answer;
test_gap_repsel_element_shape_loop_cloneis byte-identical to the Node 26.5.1oracle on both arms, and an IR census confirms the new sections are live:
sumRow,sumRow$spec_b_i32andsumGlobalRowsgain the clone, where onmainonlysumFieldandmainhad it.An existing gate that could not fail
fast_clone_sliceinelement_shape_loop_tests.rssliced from the firstsubstring occurrence of
for.element_shape_fast.cond— which is thebr label %…terminator of the fast preheader, four lines above the slowpreheader — and every assertion made against the result is a negative
(
!fast.contains(" call "),!fast.contains("js_array_get_f64"), …). So theIR census that exists to prove the clone is call-free had been vacuous since
#7612, on the code that then shipped a SIGBUS. It now finds the block
definition and asserts the slice contains the cloned body and its element
load, so it cannot pass on an empty subject again. Same family as #7024/#7025:
the gate ran, its subject did not.
Tests
crates/perry-codegen/src/stmt/element_shape_loop_tests.rs, 7 new (17 total inthe module, all green). One positive — #7480's kernel reaches the clone, the
clone is call-free, and the accumulate is an
fadd(the two halves assertedtogether, because either alone is inert) — and six sabotage cases: an ambiguous
shape, a shape a field-type tie can break, an optional property, a shape no
literal allocates, a reordered shape, and a read outside the clone that must
stay on the by-name path.
Gates
lint's 22 extracted commands (22 extracted, 22 ran, 0 failed),cargo fmt --all -- --check,cargo check --all-targets,cargo test -p perry-codegen --liband-p perry-runtime --lib(
--no-fail-fast), the 14native_root_coveragetests, bothgc_root_dominancearms (--moving-only --seeded-violations 40and--statepoints --moving-only), and the full gap suite againsttest-parity/gap_snapshot.json.Validation
Release build, Node 26.5.1 (matches
.node-version),PERRY_GC_MOVING_LOOP_POLLS=1on both corpus arms.
gc-root-dominance-statepoints— 7 unrooted / 0 stale, 40/40 seededviolations caught, empty allowlist honoured. Budget of 7 is exact: the same
run with
--max-unrooted 6exits 1, so the ratchet still has a referent itcan hit.
gc-root-dominance(shadow arm) — 0 violations over 2458 functions /9833 root stores, 40/40 seeded caught. Unchanged.
--self-test+ all three audits (--audit-alloc-re,--audit-poll-capable,--audit-immovable-sources) — pass.test.yml(includingcheck_file_size.sh,addr_class_inventory.py,gc_gate_wiring_check.py) —0 failed;
cargo fmt --all -- --checkclean.cargo test -p perry-codegen --lib748 passed / 0 failed (19 inroot_reload, 14native_root_coverage).cargo test -p perry-runtime --lib1917 passed / 0 failed.cargo check --all-targetsclean.reported failure is an existing
test-parity/gap_snapshot.json/known_failures.jsonentry except one,test_gap_zlib_4917_level(
compile_fail), which fails identically on the branch point — A/B'd witha
perry-devbuild of7bde3de24in the same tree. It is the auto-optimizerelink dropping the zlib feature from its stdlib archive (host-local); with
PERRY_NO_AUTO_OPTIMIZE=1the test compiles, runs, and is byte-identical tonode. Clearing
target/perry-auto-*does not change it, so it is notstaleness either.
node_fail -> parity_failstatus changes(
enum_forward_ref,backoff_options,cron_cronjob, …). A codegen changecannot alter node's exit status, and each is an existing snapshot entry
whose recorded reason (
ERR_UNSUPPORTED_TYPESCRIPT_SYNTAX, or an npm importwith no local
node_modules) reproduces here — local classification, not aregression.
test_gap_iterator_helpers_2874: parity_fail -> pass. Not investigated.test_gap_class_expr_identity, the regression the anchor bug caused, is nowbyte-identical to node and back to passing.
The snapshot is deliberately not regenerated in this PR: the only real
delta is host-local.
Summary by CodeRabbit
Bug Fixes
Documentation