perf(gc): emit the shadow-slot root store inline - #7088
Open
proggeramlug wants to merge 6 commits into
Open
Conversation
added 5 commits
July 30, 2026 20:03
Prepares the inline slot store: Vec layout is unspecified and cannot be a codegen contract, so the three buffer words become explicit #[repr(C)] fields with published, offset_of!-asserted offsets. Dropping the Vec also drops the TLS drop glue (and its per-op lazy-registration check). Claude-Session: https://claude.ai/code/session_018ZFER8EEg8K7ez2n6oDrT9
The first version passed with the guard removed: after a balanced pop len is 0, so the bounds check alone skips the write. Forcing frame_top = usize::MAX while the frame's entries are still live reproduces the wrap the guard exists for -- idx-1 lands on the frame header -- and the test now fails when the guard is deleted. Claude-Session: https://claude.ai/code/session_018ZFER8EEg8K7ez2n6oDrT9
📝 WalkthroughWalkthroughThe PR replaces vector-backed shadow-stack state with a fixed raw-buffer layout, adds ChangesInline shadow-slot storage and code generation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant GeneratedCode
participant js_shadow_frame_enter
participant ShadowStackState
participant ShadowEntry
participant GC
GeneratedCode->>js_shadow_frame_enter: enter frame
js_shadow_frame_enter-->>GeneratedCode: return state pointer
GeneratedCode->>ShadowStackState: check frame and slot bounds
GeneratedCode->>ShadowEntry: write inline bind or clear
GC->>ShadowStackState: scan and rewrite active roots
Possibly related PRs
Suggested labels: 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
crates/perry-codegen/src/function.rs (1)
319-323: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
ShadowFramePush::handle_regnow holds the state register, not a handle.
shadow_frame_push_linerenders%reg = call ptr@js_shadow_frame_enter(...), so this field is the state pointer register; the name will mislead the next reader ofreserve_shadow_slot. Consider renaming tostate_reg(field doc at Line 190 too).🤖 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/function.rs` around lines 319 - 323, Rename the ShadowFramePush field handle_reg to state_reg and update all references, including reserve_shadow_slot and shadow_frame_push_line, so the field accurately describes the state pointer register. Update the field documentation near the ShadowFramePush definition to use the new name and terminology.crates/perry-runtime/src/gc/roots/shadow_stack.rs (1)
283-288: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale SAFETY comment contradicts the code.
The comment says the length is passed as 0, then immediately says the real length is re-declared; the code passes
s.len. Trim it to the accurate rationale.📝 Suggested comment fix
- // SAFETY: `ptr`/`cap` came from a `Vec<ShadowEntry>` built here. Length - // is passed as 0 because `ShadowEntry: Copy` has no drop glue, and the - // live prefix is copied by `reserve` from the raw allocation anyway — - // so re-declare the real length to keep the data. + // SAFETY: `ptr`/`cap` came from a `Vec<ShadowEntry>` built here. The + // real `len` is re-declared so `reserve` copies the live prefix into + // the new allocation.🤖 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-runtime/src/gc/roots/shadow_stack.rs` around lines 283 - 288, Update the SAFETY comment immediately above Vec::from_raw_parts in the shadow-stack reconstruction to accurately describe that s.len is passed as the vector length; remove the contradictory claim that length is passed as 0 while retaining only the valid rationale about the Vec allocation, Copy element type, and preserved live prefix.crates/perry-codegen/src/lib.rs (1)
11-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRevert
exprto crate-local visibility.
perryonly needsperry_codegen::expr_shadow_layout;pub use crate::expr::shadow_inline::{…}works whileexprremainspub(crate), so widening it makes the entire expression lowering namespace part ofperry-codegen’s public API unnecessarily.♻️ Proposed change
-pub mod expr; +pub(crate) mod expr;🤖 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/lib.rs` at line 11, Change the expr module declaration back to crate-local visibility by replacing the public export in the module declarations. Preserve the existing public re-export of expr_shadow_layout through the established shadow_inline path so perry can access only that API without exposing the full expression-lowering namespace.crates/perry-codegen/src/expr/shadow_inline.rs (1)
428-547: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBrittle hardcoded SSA register numbers in three unit tests.
frame_push_uses_frame_enter_and_derives_the_handle(Line 440, 447),inline_store_keeps_the_sentinel_and_bounds_guards(Lines 507, 512, 518), andinline_bind_keeps_the_gated_root_shading_barrier(Line 543) assert against exact register names (%r3,%r5,%r10,%r13,%r15,%r17,%r23). Any unrelated instruction added/removed earlier inemit_inline_slot_write(or in shared codegen helpers) will shift numbering and break these tests without the guarded property actually regressing. The file already demonstrates a more robust technique for other assertions — content-based block extraction (store_blocks/bind_block/clear_block/entry_reg) and a wildcard-register fallback for the frame_top offset check at Line 442 — that could be applied consistently here too (e.g., match"icmp eq i64 %", ", -1"and"sub i64 %", &SHADOW_STACK_HEADER_SLOTS.to_string()without pinning the exact register).♻️ Example direction for one of the assertions
- assert!( - body.contains(&format!("sub i64 %r5, {}", SHADOW_STACK_HEADER_SLOTS)), - "pop handle must be frame_top - {SHADOW_STACK_HEADER_SLOTS}; body:\n{body}" - ); + assert!( + body.contains("sub i64 %") && body.contains(&format!(", {}\n", SHADOW_STACK_HEADER_SLOTS)), + "pop handle must be frame_top - {SHADOW_STACK_HEADER_SLOTS}; body:\n{body}" + );🤖 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/shadow_inline.rs` around lines 428 - 547, Replace hardcoded SSA register names in frame_push_uses_frame_enter_and_derives_the_handle, inline_store_keeps_the_sentinel_and_bounds_guards, and inline_bind_keeps_the_gated_root_shading_barrier with content-based or wildcard-register assertions. Preserve each test’s validation of the relevant operation and constants, using existing helpers such as bind_block and entry_reg where appropriate, so unrelated codegen changes do not break the tests.
🤖 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.
Nitpick comments:
In `@crates/perry-codegen/src/expr/shadow_inline.rs`:
- Around line 428-547: Replace hardcoded SSA register names in
frame_push_uses_frame_enter_and_derives_the_handle,
inline_store_keeps_the_sentinel_and_bounds_guards, and
inline_bind_keeps_the_gated_root_shading_barrier with content-based or
wildcard-register assertions. Preserve each test’s validation of the relevant
operation and constants, using existing helpers such as bind_block and entry_reg
where appropriate, so unrelated codegen changes do not break the tests.
In `@crates/perry-codegen/src/function.rs`:
- Around line 319-323: Rename the ShadowFramePush field handle_reg to state_reg
and update all references, including reserve_shadow_slot and
shadow_frame_push_line, so the field accurately describes the state pointer
register. Update the field documentation near the ShadowFramePush definition to
use the new name and terminology.
In `@crates/perry-codegen/src/lib.rs`:
- Line 11: Change the expr module declaration back to crate-local visibility by
replacing the public export in the module declarations. Preserve the existing
public re-export of expr_shadow_layout through the established shadow_inline
path so perry can access only that API without exposing the full
expression-lowering namespace.
In `@crates/perry-runtime/src/gc/roots/shadow_stack.rs`:
- Around line 283-288: Update the SAFETY comment immediately above
Vec::from_raw_parts in the shadow-stack reconstruction to accurately describe
that s.len is passed as the vector length; remove the contradictory claim that
length is passed as 0 while retaining only the valid rationale about the Vec
allocation, Copy element type, and preserved live prefix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b9028ae2-20d2-4f88-8724-e73741398b84
📒 Files selected for processing (20)
changelog.d/7086-inline-shadow-slot-store.mdcrates/perry-codegen/src/codegen/closure.rscrates/perry-codegen/src/codegen/helpers.rscrates/perry-codegen/src/expr/mod.rscrates/perry-codegen/src/expr/shadow_inline.rscrates/perry-codegen/src/expr/shadow_slot.rscrates/perry-codegen/src/function.rscrates/perry-codegen/src/lib.rscrates/perry-codegen/src/module.rscrates/perry-codegen/src/runtime_decls/arrays.rscrates/perry-codegen/tests/shadow_slot_hygiene.rscrates/perry-runtime/src/gc/roots.rscrates/perry-runtime/src/gc/roots/shadow_stack.rscrates/perry-runtime/src/gc/tests/debt_pacer.rscrates/perry-runtime/src/gc/tests/shadow_stack_ops.rscrates/perry-runtime/src/gc/tests/support.rscrates/perry/src/commands/compile/build_cache.rscrates/perry/src/commands/compile/object_cache.rscrates/perry/src/main.rscrates/perry/src/shadow_layout_contract.rs
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.
A ceiling measurement (#7079) put the shadow stack at +63.7 % instructions
on application-shaped code (
w6_records, Pi 5) versus not emitting it at all,and concluded — labelled as a reading, not a measurement — that what remained
was per-store
extern "C"calls that only a codegen change can touch. Thereare two codegen changes that touch it: replacing the shadow stack with stack
maps, and inlining the store. This PR is the second one.
It removes 56 % of that ceiling.
w6_recordsgoes from +60.8 % to+26.7 % against an identical toolchain with only the store form changed.
Why a call, not the work, was the cost
An
extern "C"call costs twice: the call itself, and the fact that it isopaque to LLVM, which forces a spill of every live value around it and
blocks hoisting across it. Read out of the shipped
aarch64archive,js_shadow_slot_bind's fast path was ~35 instructions, of which about six didany work:
bl+ 4-instruction prologue/epilogue pairadrp/ldr/add+ indirectblrinto the resolver,mrs tpidr_el0ldrb/cmp/b.eq+ apanic_access_erroredgestp, gated barrierHow the thread-local block is reached
Not by re-deriving the TLS address in generated code. That would mean
modelling Rust's TLS model per platform (TLSDESC on this Linux build,
tlvonmacOS) and would be a second, unverified path to the same memory.
Instead the address is obtained from the runtime and cached for the
activation.
js_shadow_frame_enterisjs_shadow_frame_pushreturning theaddress of this thread's
ShadowStackStateinstead of the frame handle, socodegen pays exactly the one thread-local lookup per activation that the push
already paid. The handle the matching
js_shadow_frame_popneeds is recoveredas
frame_top - SHADOW_STACK_HEADER_SLOTS, so the pop side is untouched.Caching it is sound because it is the address of a
const-initialized,drop-free
thread_local!: fixed for the thread's lifetime, never reallocated.The buffer it points at does move when a deeper frame grows it — which is
exactly why no frame base is cached and
ptr/len/frame_toparere-loaded from the state at every store. One activation of a compiled function
runs entirely on one thread (
perry/threadhands a whole call to a worker; aresumed async state machine re-enters through the function entry and calls
js_shadow_frame_enteragain), so a cached pointer never escapes its thread.Because the pointer comes from the runtime's own
SHADOW.with, the inlinestore addresses the same memory
js_shadow_slot_setwrites by construction.js_shadow_state_addrmakes that testable from Rust: a test writes an entrythrough the published offsets and reads it back with
js_shadow_slot_get, andvice versa.
ShadowStackStateis now#[repr(C)]with an explicitptr/len/capbuffer, because
Vec's layout is not a contract — in the archive read at thetime of writing it placed
capat 0,ptrat 8 andlenat 16, and a silentreorder would have codegen writing live GC roots through the wrong word.
Dropping the
Vecalso drops the type's drop glue, which is what forced theper-op lazy destructor-registration check; the buffer is freed at thread exit by
a separate guard thread-local armed only from the cold growth path.
Soundness, per root property
Liveness — the inline store writes the same
ShadowEntry.valueand sets thesame
SLOT_ACTIVEbit ofmeta, at the same index, sovisit_shadow_stack_root_slotsmarks it identically.Rewritability —
metastill carries the bound compiled-local address, withbound_slot_meta's alignment fallback intact (a tag-colliding address isrecorded active-but-unbound, never truncated), so an evacuating collection
rewrites the alloca the mutator reads after the safepoint, not just the mirror.
The value the mutator stored — the value is read from the local slot at the
store site, in the position the call occupied, and written immediately. Nothing
re-reads a slot at a later safepoint.
The incremental-mark root shading barrier is emitted inline behind the same
PERRY_INCREMENTAL_MARK_BARRIER_ACTIVE_COUNTgate the runtime andemit_persistent_shadow_root_barrieralready use.Guards. Both of the runtime function's guards are emitted: the
frame_top == usize::MAXsentinel and theslot < lenbounds check. Honestscope: the sentinel is unreachable in a balanced program (that state implies
len == 0, so the bounds check alone would skip). It is kept because if the twoever diverge the failure mode is silent corruption, not a skip —
usize::MAX + idxwraps toidx - 1, an in-bounds index into the frameheader, unlinking every outer frame from the root scan. Same wrap-around class
#7079 fixed in
frame_pop.Bounds and capacity are not dropped. Growth stays in the runtime's cold
path; the inline sequence never grows the buffer, it re-reads
lenand skipswhen out of range, exactly as
js_shadow_slot_setdoes.Each store also emits a null-state fallback arm calling the original runtime
function.
js_shadow_frame_enteris declarednonnull, so LLVM folds that armaway wherever the push dominates — verified in the linked binary, where
js_shadow_slot_setno longer appears in the symbol table at all.Measured
Pi 5 (Cortex-A76, governor
performance), instructions retired underperf stat,taskset -c 3, 12 interleaved reps per arm, runs dropped if the armclock moved >1 % or any throttle bit set. 0 of 300 runs dropped, 0 oracle
mismatches, clock 2 400 017–2 400 037 kHz (within 0.001 %), throttle word
0x0throughout, temp ≤ 67.5 °C, load average 0.93/2.54/3.34 at launch.Every arm's stdout diffed against pinned Node 26.5.0. Instruction-count cv is
0.00 % on every workload arm, which is also why the decaying 5/15-minute
load does not threaten these figures — instruction counts are load-insensitive,
unlike the cycles and wall columns.
Single-variable isolation — same compiler, same runtime archives, only
PERRY_INLINE_SHADOW_SLOTdiffers (it is in both cache keys, so the arms cannotshare a cached
.o; binary distinctness asserted per workload):w6_records(application-shaped)w1_calldepthw3_treewalkw5_purity(floor)w5_purityis exactly inert, as it must be: a pure numeric loop reserves noshadow slots, so there is nothing to inline.
The ceiling, measured directly rather than inferred.
PERRY_SHADOW_STACK=1vs
=0re-run with the change in. The #7079 reference arms were re-runinterleaved in the same session and reproduce their published numbers, which
is what makes the two columns comparable:
w6_recordsw3_treewalkw1_calldepthNot inlined, deliberately
The frame push/pop pair (per activation, not per store), and the parameter /
closure-prologue / persistent entry-setup binds. Those are emitted while block 0
is being built directly, before the lowering context that can create the guard
and barrier basic blocks exists.
What that leaves is checkable rather than asserted. In
w6_records' emitted IR:ss.slowarmsjs_shadow_slot_setjs_shadow_slot_bindjs_shadow_frame_enterjs_shadow_frame_popEvery opaque shadow call left in
w6is per-activation; none is per-store.js_shadow_slot_setis gone from the linked binary's symbol table entirely, andthe 11 surviving
js_shadow_slot_bindsites are all parameter and prologuebinds. Since
alwaysinlineleaf functions turn per-activation intoper-iteration, those are the natural next target — and they are why
w3_treewalk(deep recursion, few stores per activation) improves least here.Gates
scripts/gc_repsel_matrix.sh --arms all --pressure 8, pinned Node 26.5.0:PASS=339 UNVER=100 XFAIL=1 FAIL=0over 440 cells — the required numbersexactly, byte-exact against the oracle on 439/440 (the one non-match is the
registered
XFAIL,repsel_ptr_shape_localsunderrep_ptr_shape_off). Thearms were live, not inert: every
requires=movearm ran 21/22 rows withmoved-objectsandcopy-minorboth confirmed, and therequires=scavengearms 12/22 — so the evacuating configurations actually evacuated with the
inline stores in place.
gc-ratchet: all 72 gated retention/evacuation counters bit-identical topristine
origin/main. Rather than trust the pinned baseline artifact —which was captured on
darwin-arm64and which the harness itself refuses togate against a
linux-aarch64run —origin/main@404c1c948was built inthe same worktree, with the same package set, and measured on the same host.
heap_used_bytes,heap_total_bytes,minor_cycles,step_cycles,copied_objects,copied_bytes,promoted_objects,promoted_bytesandfreed_bytesare equal across all 8 probes: 0 of 72 differ. Nothing is keptalive or dropped that was not before.
The comparison is on the whole metric object — median, min, max, spread, stdev
and every sample — not just medians.
And the A/B was not vacuous: on the same two runs the ungated metrics
differ on 20 of 24 rows,
wall_msandrss_byteson all 8 probes (branchfaster on 6/8, median ≈ −1.7 % wall). Identical counters, demonstrably
different binaries. The four identical
peak_rss_bytesvalues arepage-quantised.
(Against the pinned artifact, 71 of 72 gated rows match to the byte and one —
05_closure_captureheap_used_bytes, +6.47 % — does not. That is theplatform, not this PR: the pre-#7088 pinned toolchain reproduces the branch's
exact figure, 1,107,552, on this host, and the branch reproduces
main'sexactly. The harness flags the platform mismatch itself and refuses to gate
across it.)
Tests — 6 new runtime, 5 new codegen, 3 new cross-crate contract. Every one
was verified to fail under a targeted sabotage of the thing it covers:
shadow_layout_contract_matches_the_runtimeSLOT_ACTIVEinline_bound_slot_survives_and_is_rewritten_by_a_copying_minor,inline_write_and_runtime_accessor_address_the_same_entryinline_write_with_no_frame_installed_is_skipped_not_wrappedinline_bind_keeps_the_gated_root_shading_barriermetainstead of maskingdead_local_clear_is_inline_and_preserves_the_bindingThe sentinel-guard test needed a second pass: the first version passed with the
guard deleted, because after a balanced pop
lenis 0 and the bounds checkalone skips the write. It now forces
frame_top = usize::MAXwhile the frame'sentries are still live, which is the state the guard actually exists for, and
fails when the guard is removed.
PERRY_INLINE_SHADOW_SLOT=0/off/falsereverts to the calls for bisection.