perf(codegen): lower temp roots onto pooled frame allocas — three FFI calls per temporary become a store and a load - #7487
Merged
Conversation
proggeramlug
pushed a commit
that referenced
this pull request
Aug 6, 2026
|
Warning Review limit reached
Next review available in: 18 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 ignored due to path filters (1)
📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThe code generator now pools temporary GC roots in frame allocas, routes rooted operations through the pool, elides selected class-field stores, and skips redundant string and GC layout bookkeeping for guarded non-pointer stores. ChangesCode generation rooting and store optimization
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant call_rooted
participant TempRootPool
participant emit_shadow_slot_bind_ptr
participant RuntimeFFI
call_rooted->>TempRootPool: push call result as temporary root
TempRootPool->>emit_shadow_slot_bind_ptr: bind pooled frame slot
emit_shadow_slot_bind_ptr->>RuntimeFFI: use FFI fallback when shadow-slot binding is unavailable
call_rooted->>TempRootPool: get rooted value and truncate released slots
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 |
added 3 commits
August 6, 2026 08:52
…ls per temporary become a store and a load The #6951 temp-root contract cost three runtime calls per protected temporary — js_gc_temp_root_push, a mandatory re-read, truncate — and after #7474 those three were 206 of the 542 remaining _tlv_get_addr-attributed samples on churn.ts. But named locals already demonstrate the cheap form of the same root: an entry alloca bound to a shadow-frame slot, written and re-read with plain stores and loads, upgraded by the RS4GC/stack-map lowering into a relocated addrspace(1) slot. A temp needs nothing a local does not. TempRootPool (per-function, compile-time-only) lowers the same API onto that mechanism: push = store + the identical bind/root-shading emission a named local's store uses (emit_shadow_slot_bind_ptr, extracted from emit_shadow_slot_bind_for_local); get = load; set = store+bind; truncate = zero the slot, clear the frame mirror, release the pool entries at and above it — the same drop-everything-above contract the FFI stack imposed. Handles keep the String currency, so every caller (RootedOperands, StoreOperandGuard, rooting.rs's call_rooted) compiles unchanged. The pool reuses slots by stack watermark; frame slots are reserved on demand via reserve_shadow_slot, whose in-place slot-count rewrite is what rules out the #7184 out-of-frame-bounds shape. The store-then-bind order at the push site is the #7192 dominance invariant, stated at the emission. When shadow-stack emission is off (reserve_shadow_slot -> None) the whole function falls back to the FFI stack byte-for-byte; ShadowSavepoint's temp-depth restore keeps working on both arms (the FFI stack simply stays empty in alloca mode). js_array_push_f64_temp_rooted remains for the fallback arm only — in alloca mode the fused form is load + js_array_push_f64 + store, which is cheaper than the call it was fusing. Refs #7469.
proggeramlug
force-pushed
the
perf/7469-temp-root-allocas
branch
from
August 6, 2026 06:53
f862e24 to
b051649
Compare
This was referenced Aug 6, 2026
proggeramlug
pushed a commit
that referenced
this pull request
Aug 6, 2026
…es (#7493) #7370 made native roots (RS4GC statepoints) the default lowering. `NativeRootsPin` was added so a test asserting on shadow-stack IR could say so, the in-crate unit tests were repaired with it — and the five integration suites that assert the same mechanics had no pin to reach for, because `NativeRootsPin` is `#[cfg(test)]` and `tests/*.rs` link this crate as an external consumer. They run nightly/at-tag only, so nothing went red at merge time and `shadow_slot_hygiene` sat at 0/12 on `main`. The pin is now reachable as `perry_codegen::testing::NativeRootsPin`, behind a `testing` cargo feature that only this crate's own `[dev-dependencies]` entry enables. With the feature off the pin, its thread-local and the branch it adds to `rs4gc_enabled()` are `#[cfg]`-ed out of the artifact — not merely private, absent — and cargo builds dev-dependencies for test/bench targets only, so no production profile can reach it. `NativeRootsPin::native()` joins `shadow()`, because a pin also outranks `PERRY_RS4GC` and that is what keeps an assertion meaning the same thing during a `PERRY_RS4GC=0` sweep. Per-test classification (not per file — the files disagree internally): * `shadow_slot_hygiene` — 12/12 shadow. The file's subject IS the shadow frame; 0/12 -> 11/12. * `scalar_replaced_slot_roots` — 11/11 shadow (every test counts `js_shadow_slot_bind` sites). 2/11 -> 5/11. * `temp_root_operand_temporaries` — 2 shadow, the rest unpinned and lowering-independent. 12/19 -> 13/19. * `temp_root_argument_temporaries` — none. `PERRY_RS4GC=0` moves it not at all; its failures are #7487's, not #7370's. * `native_proof_regressions` — 2 shadow, 15 native in `invalidation`. 249/253 -> 253/255 single-threaded, 198/253 -> 253/255 in parallel. * `native_proof_buffer_views` — 1 native. 28/30 -> 30/32. Two tests were pinned though they were PASSING: `numeric_only_scalar_replaced_{object,array}_emits_no_rooting` and `a_collection_free_construction_emits_no_this_slot_root` assert `bind_calls(&ir) == 0` / `!contains("@js_shadow_slot_bind")`, which under the native default is true of every program. They were green without their subject running (CLAUDE.md hazard 4). Pinned, the first two now fail for a real reason (#7497). Also fixed here because it hid this suite's real signal: `native_proof_ regressions` reported 55 failures under default parallelism and 4 under `--test-threads=1`. 51 of the 55 were `PoisonError` — #7490's shape again. The `PERRY_NATIVE_REPS*` env vars are process-global and the restore was hand-written after the compile, so a panic inside `compile_module` left them installed and every later unlocked compile wrote artifact JSON into a directory another test was reading; the torn read panicked inside the lock and poisoned it. The harness is now one shared `tests/native_proof_support/mod.rs`: a poison-tolerant accessor, an RAII env guard, and an artifact reader that treats a foreign or half-written neighbour as noise. Two sabotage tests plant each failure shape and assert the fix is what prevents it. Finally, a tripwire in `src/` — so it runs in the REQUIRED `cargo-test` job, which is the tier this whole issue is about not being in: `host_target_lowering_default_is_native_roots` fails the moment the default flips again, naming the suites that then need re-pinning. It asserts its subject is live (both pins must give different answers; the unsupported-target arm must give the opposite default), so a constant-folded `rs4gc_enabled()` fails it rather than passing it. A second gate scans every workspace manifest and fails if a non-dev dependency edge ever enables the `testing` feature. Refs #7493.
proggeramlug
added a commit
that referenced
this pull request
Aug 6, 2026
…es, and say which lowering each asserts (#7493) (#7509) * fix(test): make the root-lowering pin reachable from integration suites (#7493) #7370 made native roots (RS4GC statepoints) the default lowering. `NativeRootsPin` was added so a test asserting on shadow-stack IR could say so, the in-crate unit tests were repaired with it — and the five integration suites that assert the same mechanics had no pin to reach for, because `NativeRootsPin` is `#[cfg(test)]` and `tests/*.rs` link this crate as an external consumer. They run nightly/at-tag only, so nothing went red at merge time and `shadow_slot_hygiene` sat at 0/12 on `main`. The pin is now reachable as `perry_codegen::testing::NativeRootsPin`, behind a `testing` cargo feature that only this crate's own `[dev-dependencies]` entry enables. With the feature off the pin, its thread-local and the branch it adds to `rs4gc_enabled()` are `#[cfg]`-ed out of the artifact — not merely private, absent — and cargo builds dev-dependencies for test/bench targets only, so no production profile can reach it. `NativeRootsPin::native()` joins `shadow()`, because a pin also outranks `PERRY_RS4GC` and that is what keeps an assertion meaning the same thing during a `PERRY_RS4GC=0` sweep. Per-test classification (not per file — the files disagree internally): * `shadow_slot_hygiene` — 12/12 shadow. The file's subject IS the shadow frame; 0/12 -> 11/12. * `scalar_replaced_slot_roots` — 11/11 shadow (every test counts `js_shadow_slot_bind` sites). 2/11 -> 5/11. * `temp_root_operand_temporaries` — 2 shadow, the rest unpinned and lowering-independent. 12/19 -> 13/19. * `temp_root_argument_temporaries` — none. `PERRY_RS4GC=0` moves it not at all; its failures are #7487's, not #7370's. * `native_proof_regressions` — 2 shadow, 15 native in `invalidation`. 249/253 -> 253/255 single-threaded, 198/253 -> 253/255 in parallel. * `native_proof_buffer_views` — 1 native. 28/30 -> 30/32. Two tests were pinned though they were PASSING: `numeric_only_scalar_replaced_{object,array}_emits_no_rooting` and `a_collection_free_construction_emits_no_this_slot_root` assert `bind_calls(&ir) == 0` / `!contains("@js_shadow_slot_bind")`, which under the native default is true of every program. They were green without their subject running (CLAUDE.md hazard 4). Pinned, the first two now fail for a real reason (#7497). Also fixed here because it hid this suite's real signal: `native_proof_ regressions` reported 55 failures under default parallelism and 4 under `--test-threads=1`. 51 of the 55 were `PoisonError` — #7490's shape again. The `PERRY_NATIVE_REPS*` env vars are process-global and the restore was hand-written after the compile, so a panic inside `compile_module` left them installed and every later unlocked compile wrote artifact JSON into a directory another test was reading; the torn read panicked inside the lock and poisoned it. The harness is now one shared `tests/native_proof_support/mod.rs`: a poison-tolerant accessor, an RAII env guard, and an artifact reader that treats a foreign or half-written neighbour as noise. Two sabotage tests plant each failure shape and assert the fix is what prevents it. Finally, a tripwire in `src/` — so it runs in the REQUIRED `cargo-test` job, which is the tier this whole issue is about not being in: `host_target_lowering_default_is_native_roots` fails the moment the default flips again, naming the suites that then need re-pinning. It asserts its subject is live (both pins must give different answers; the unsupported-target arm must give the opposite default), so a constant-folded `rs4gc_enabled()` fails it rather than passing it. A second gate scans every workspace manifest and fails if a non-dev dependency edge ever enables the `testing` feature. Refs #7493. * docs(test): point the lowering notes at the filed follow-ups (#7502, #7503, #7504, #7505, #7506) * docs(changelog): fragment for the root-lowering pin and the poison cascade (#7509) * chore: bump version to 0.5.1291 --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
This was referenced Aug 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Item #3 of the #7469 mutator-cost breakdown: expression temporaries stop paying three runtime calls each. Stacked on the dead-store-elision PR (merge that first).
The mechanism
The #6951 temp-root contract emitted
js_gc_temp_root_push/ mandatory re-read /js_gc_temp_root_truncatearound every protected temporary — 206 of the 542 remaining_tlv_get_addr-attributed samples onchurn.tsafter #7474. But named locals already demonstrate the cheap form of the same root: an entry alloca bound to a shadow-frame slot, written and re-read with plain stores/loads, upgraded by the RS4GC/stack-map lowering into a relocatedaddrspace(1)slot. A temp needs nothing a local doesn't.TempRootPool(per-function, compile-time bookkeeping only — no runtime state, nothing forShadowSavepoint's unwind restore to do) lowers the same API onto that mechanism:emit_shadow_slot_bind_ptr, extracted verbatim fromemit_shadow_slot_bind_for_local) — store-before-bind-before-collecting-call is the fix(codegen): make every GC root store dominate the collection points after it #7192 dominance invariant, stated at the siteStringcurrency → every caller (RootedOperands,StoreOperandGuard,rooting.rs::call_rooted) compiles unchangedreserve_shadow_slot, whose in-place slot-count rewrite rules out the fix(codegen): never let a shadow-frame slot index escape the pushed frame (#7154) #7184 out-of-bounds shapejs_array_push_f64_temp_rootedsurvives for that arm onlychurn.ts's hot function: 9 temp-root FFI calls → 0. The GC trace confirms the roots moved home:compiled_nativestack-map roots appear (9 pointer roots/cycle) exactly asruntime_mutable_scannersslots shrink, with retention equivalent (+3 objects of 254k on tree).Results
Against post-#7474 main, interleaved best-of-5 user CPU — incremental over the dead-store PR, and composed:
Cumulative since #7469 was filed: churn 4.33 → 2.81 s,
_tlv_get_addr34.2% → 13.9% of self time, temp-root self-samples zero.One flagged cell: tree peak RSS 193 → 216 MB (+12%), deterministic (5/5 runs). Investigated to the bottom: per-cycle GC traces are field-identical on every retention metric (43 cycles, same copied/promoted/freed bytes, live objects +3 of 254k), peak arena accounting byte-identical (105.9 MB reserved / 68.1 MB in-use both arms), binaries same size, malloc zones flat — the delta lives outside every collector metric. This bench is the documented regime-sensitive RSS cell (#7438 recorded 193↔102 MB swings on it from scavenge-timing policy alone); a 21% faster mutator shifts that interleaving. Note deeplist RSS moves the other way (163.8 → 150.7 MB) and
09_try_catch_rootsin the ratchet retains 14% less heap under this PR — frame-slot temps are zeroed at release instead of lingering on the runtime stack. If the pinned-host ratchet disagrees with my read, it wins.Validation
{ ...src }with an accessor source SIGSEGVs under PERRY_GC_MOVING_LOOP_POLLS=1 #7200/codegen: ClassExprFresh leaves the class object unrooted across its own js_object_set_field_by_name stores #7211 reproducer shapes match Node byte-for-byte: default GC;PERRY_CONSERVATIVE_STACK_SCAN=off(the mode where an unrooted temp is a live use-after-free);PERRY_GC_ZEAL=1 + PERRY_GC_PROTECT_FROMSPACE=1on aPERRY_GC_MOVING_LOOP_POLLS=1build — with the instrument proven live (5 quarantined from-space sets per run: survivors genuinely moved, slots genuinely rewritten). Same probes underPERRY_RS4GC=0shadow mode, plus zeal there too.gc_root_dominancecorpus (124 sources, 144 modules): 0 violations with the empty allowlist, 2,379 functions / 9,145 root stores scanned; stale-register budget 21 ≤ 39.--self-testpasses.heap_used_bytesdiffs — one −0.01% wobble, and09_try_catch_roots−14.4% (the retention improvement above).cargo test -p perry-codegen(695) green, incl.rooting.rs's compile-fail doctests.PERRY_RS4GC=0) is a wash by design: 10 bind FFI calls replace 9 temp-root FFI calls in churn's hot fn (binds are cheaper per call — no Vec traffic). In the shipped RS4GC pipeline the binds are erased at stack-map lowering, so the win is real where it matters.No version bump; changelog fragment included.
Refs #7469.
Summary by CodeRabbit
Performance
Compatibility