feat(gc): make native roots (statepoints) the default - #7370
Merged
Conversation
PERRY_RS4GC=1 is no longer needed. PERRY_RS4GC=0 reverts to the shadow stack for bisection. TARGET-AWARE, not blanket. gc_map REFUSES to emit a map for a target whose frame bases the runtime cannot resolve, because a map nothing reads loses roots silently -- so a global flip would turn every watchOS arm64_32 and ARM64-Windows compile into a hard error. The default is therefore native roots where the runtime can walk, shadow stack where it cannot. That is only expressible because #7340 split the root-set analysis from its lowering: falling back is not 'no roots', it is the other lowering of the same analysis. A test pins the support matrix in both directions, because the one way this breaks a platform is if the predicate is LOOSER than gc_map's refusals. An explicit PERRY_RS4GC=1 still reaches that refusal rather than being silently downgraded, so an A/B arm measures what it asked for. Evidence, full 479-test gap suite with no env set: pass 447 (shadow baseline: 447) diff 19 (pre-existing, unchanged) node_fail 13 regressions 0 compile failures 0 All 128 try-carrying tests compiled -- the class the deleted bridge (#7348) could never handle. All 10 gc_ratchet probes byte-identical to Node. Runtime -1-2%; binary size +1.86% measured on zod's 81 modules. Eight codegen tests assert on shadow-stack IR and now pin that lowering through a thread-local guard, mirroring arena::quarantine's ProtectionModeGuard. They were right about what they asserted -- they had just never needed to name a lowering, because there was only one.
📝 WalkthroughWalkthroughChangesNative root lowering
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant compile_module
participant codegen_helpers
participant target_runtime_support
compile_module->>codegen_helpers: set_native_roots_for_target(target)
codegen_helpers->>target_runtime_support: check frame-walker support
target_runtime_support-->>codegen_helpers: supported or unsupported
codegen_helpers-->>compile_module: select native roots or shadow stack
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/codegen/helpers.rs`:
- Around line 1577-1632: Refactor the target-default decision used by
rs4gc_enabled() into a pure helper that accepts an explicit optional PERRY_RS4GC
override and the target capability result, preserving override precedence.
Update native_roots_default_matches_the_targets_gc_map_will_emit_for() to
evaluate defaults with None so the process environment cannot affect assertions,
and replace the current tautological override check in
the_target_default_is_a_default_not_a_veto() with assertions that explicit
Some(false) and Some(true) override the target default.
🪄 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: 65e79afd-d78e-46ec-ba62-7255f3b703eb
📒 Files selected for processing (6)
changelog.d/7366-statepoints-default.mdcrates/perry-codegen/src/codegen/closure.rscrates/perry-codegen/src/codegen/helpers.rscrates/perry-codegen/src/codegen/mod.rscrates/perry-codegen/src/collectors/proven_this_routing_tests.rscrates/perry-codegen/src/expr/shadow_inline.rs
Comment on lines
+1577
to
+1632
| #[test] | ||
| fn native_roots_default_matches_the_targets_gc_map_will_emit_for() { | ||
| for triple in [ | ||
| "arm64-apple-macosx", | ||
| "aarch64-apple-darwin", | ||
| "aarch64-apple-ios", | ||
| "aarch64-unknown-linux-gnu", | ||
| "aarch64-unknown-linux-musl", | ||
| "x86_64-apple-darwin", | ||
| "x86_64-unknown-linux-gnu", | ||
| "x86_64-pc-windows-msvc", | ||
| ] { | ||
| set_native_roots_for_target(triple); | ||
| assert!( | ||
| rs4gc_enabled(), | ||
| "{triple} has a working walker — native roots should be the default" | ||
| ); | ||
| } | ||
|
|
||
| for triple in [ | ||
| // ILP32: 32-bit pointers, and the runtime's map loader is gated to | ||
| // 64-bit Apple, so a map here would be read by nothing. | ||
| "arm64_32-apple-watchos", | ||
| // COFF + ARM64: no Windows walker for that register model, so no | ||
| // frame would ever be visited. | ||
| "aarch64-pc-windows-msvc", | ||
| // Architectures with no walker at all. | ||
| "riscv64gc-unknown-linux-gnu", | ||
| "wasm32-unknown-unknown", | ||
| ] { | ||
| set_native_roots_for_target(triple); | ||
| assert!( | ||
| !rs4gc_enabled(), | ||
| "{triple} has no walker — must fall back to the shadow stack, \ | ||
| not hard-fail in gc_map" | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| /// An explicit `PERRY_RS4GC=1` must still reach `gc_map`'s refusal for an | ||
| /// unsupported target. Turning that into a silent shadow-stack fallback | ||
| /// would hide exactly what the arm was set to measure. | ||
| #[test] | ||
| fn the_target_default_is_a_default_not_a_veto() { | ||
| set_native_roots_for_target("riscv64gc-unknown-linux-gnu"); | ||
| assert!( | ||
| !rs4gc_enabled(), | ||
| "unset env + unsupported target = fall back" | ||
| ); | ||
| // The override path is env-driven and process-cached, so it is asserted | ||
| // by the CI arms rather than re-read here; this pins the shape that the | ||
| // target decision is consulted ONLY when there is no explicit answer. | ||
| assert!( | ||
| rs4gc_env_override().is_none() || rs4gc_env_override().is_some(), | ||
| "override is a tri-state" | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -uo pipefail
cargo check -p perry --profile perry-dev || exit 1
status=0
for override in 0 1; do
if ! env PERRY_RS4GC="$override" \
cargo test -p perry-codegen --profile perry-dev \
native_roots_target_tests -- --test-threads=1; then
status=1
fi
done
exit "$status"Repository: PerryTS/perry
Length of output: 143
🏁 Script executed:
#!/usr/bin/env bash
set -uo pipefail
printf 'Repository files around helpers.rs:\n'
fd -a 'helpers.rs$' . | sed 's#^\./##'
printf '\nTarget symbols in crates/perry-codegen/src/codegen/helpers.rs:\n'
rg -n "rs4gc_(enabled|env_override|policy)|NATIVE_ROOTS_TARGET_OK|NATIVE_ROOTS_OVERRIDE|set_native_roots_for_target" crates/perry-codegen/src/codegen/helpers.rs || true
printf '\nRelevant source section:\n'
sed -n '1520,1645p' crates/perry-codegen/src/codegen/helpers.rs
printf '\nRelevant section above definitions:\n'
rg -n -C 8 "pub.*rs4gc|NATIVE_ROOTS_TARGET_OK|NATIVE_ROOTS_OVERRIDE|set_native_roots_for_target" crates/perry-codegen/src/codegen/helpers.rsRepository: PerryTS/perry
Length of output: 15193
Isolate the native-roots target tests from PERRY_RS4GC.
rs4gc_enabled() uses cached PERRY_RS4GC before reading NATIVE_ROOTS_TARGET_OK, so PERRY_RS4GC=0 fails the walking-target assertions and PERRY_RS4GC=1 fails the unsupported-target assertions. Move the precedence decision into a pure helper, then test target defaults with explicit None and assert explicit overrides separately.
🤖 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/codegen/helpers.rs` around lines 1577 - 1632,
Refactor the target-default decision used by rs4gc_enabled() into a pure helper
that accepts an explicit optional PERRY_RS4GC override and the target capability
result, preserving override precedence. Update
native_roots_default_matches_the_targets_gc_map_will_emit_for() to evaluate
defaults with None so the process environment cannot affect assertions, and
replace the current tautological override check in
the_target_default_is_a_default_not_a_veto() with assertions that explicit
Some(false) and Some(true) override the target default.
Source: Coding guidelines
proggeramlug
pushed a commit
that referenced
this pull request
Aug 4, 2026
799,604,736 -> 245,055,488 bytes across the 8 gc_ratchet probes at +2%
wall, all 8 byte-identical to Node.
Measured as a 2x2, because the one-armed version of this measurement is
misleading:
no scavenge scavenge
no cap baseline +0% RSS
cap 16 MB -33%/+23% -69%/+3%
Scavenge alone moves nothing; the cap alone trades a third of the
footprint for a quarter of the wall time. Together the cap makes
collections frequent and scavenge makes them evacuating (O(live) copying)
instead of O(heap) sweeps, so the frequency is cheap. #7056 measured the
cap and recommended decoupling it -- but it was gated behind two knobs
that both defaulted OFF, so it had never been active in a shipped build,
and following that recommendation literally ships the bad arm.
Enabling scavenge also defers alloc-point collections to a precise
safepoint instead of collecting behind a forced conservative scan, which
became reasonable only when #7370 made native roots the default.
TEST WORK, which was the real cost: this first produced 23 gc:: failures.
10 of them were one bug -- force_legacy_gc_pacing() pinned only the
moving-loop-polls flag, which used to be enough because the cap and the
deferral branch both hung off it. With the cap unconditional and scavenge
default-on the guard silently stopped pinning anything, so tests that
correctly declared their pacing mode were running in the wrong one. It
now pins all three. The other 13 drive the budgeted/incremental stepper
without any guard; they pin it explicitly now, since the shipped default
bypasses that path by design.
Remaining suite variance (3-4 failures) is the pre-existing flake in
#7365 -- clean main gives 1/2/2/3/4 on the same runs.
proggeramlug
added a commit
that referenced
this pull request
Aug 4, 2026
* prototype(gc): nursery cap + scavenge on by default — NOT landable as-is Measured -69% RSS at +3% wall over the 8 gc_ratchet probes, 11/11 probes byte-identical to Node, gap suite tracking the 447/19/13 baseline. See #7372 for the full 2x2 and why neither half is worth shipping alone. Blocked on 26 gc::tests failures (baseline flake is 1-4, #7365), concentrated in the budgeted/incremental path that scavenge's deferral bypasses. Includes the one cause already understood: force_legacy_gc_pacing un-capped the trigger by pinning the polls flag, which stops working once the cap is unconditional -- the guard now suppresses the cap directly. Mechanically pinning legacy pacing across incremental_sweep_reclaim fixed only 3 of 10, so the rest need individual judgement. Pushed as a reference for #7372, not for merge. * perf(gc): nursery cap + scavenge on by default — peak RSS -69% 799,604,736 -> 245,055,488 bytes across the 8 gc_ratchet probes at +2% wall, all 8 byte-identical to Node. Measured as a 2x2, because the one-armed version of this measurement is misleading: no scavenge scavenge no cap baseline +0% RSS cap 16 MB -33%/+23% -69%/+3% Scavenge alone moves nothing; the cap alone trades a third of the footprint for a quarter of the wall time. Together the cap makes collections frequent and scavenge makes them evacuating (O(live) copying) instead of O(heap) sweeps, so the frequency is cheap. #7056 measured the cap and recommended decoupling it -- but it was gated behind two knobs that both defaulted OFF, so it had never been active in a shipped build, and following that recommendation literally ships the bad arm. Enabling scavenge also defers alloc-point collections to a precise safepoint instead of collecting behind a forced conservative scan, which became reasonable only when #7370 made native roots the default. TEST WORK, which was the real cost: this first produced 23 gc:: failures. 10 of them were one bug -- force_legacy_gc_pacing() pinned only the moving-loop-polls flag, which used to be enough because the cap and the deferral branch both hung off it. With the cap unconditional and scavenge default-on the guard silently stopped pinning anything, so tests that correctly declared their pacing mode were running in the wrong one. It now pins all three. The other 13 drive the budgeted/incremental stepper without any guard; they pin it explicitly now, since the shipped default bypasses that path by design. Remaining suite variance (3-4 failures) is the pre-existing flake in #7365 -- clean main gives 1/2/2/3/4 on the same runs. --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
proggeramlug
added a commit
that referenced
this pull request
Aug 4, 2026
…fork (#7371) Three corrections, one of which is a number the plan explicitly warns against quoting and was carrying anyway. 1. THE SIZE FIGURE. Only +18.95% appears on main -- a synthetic worst case with three heap values live across an allocation in EVERY one of 2000 functions. The dependency-scale measurement is +1.86% (zod, 81 native modules, 29 MB binary), an order of magnitude lower. The correction was written when the synthetic was retracted but never reached main: #7345 squash-merged as 24 insertions, the first commit only, so the follow-up correction commit was dropped. That is the same failure mode this document records for #7321 -- a wrong explanation outliving its own disproof -- so the real number now leads and the worst case is explicitly marked do-not-quote. 2. SEQUENCING STEP 2 said root density was a PREREQUISITE for adoption, reasoning from that retracted figure. Adoption shipped in #7370 without it. Still worth doing, and still the same lever #7296 proved worth 9.9x, but it gates nothing. 3. THE ADOPTION FORK IS CLOSED. Every gate shut: llvm-inprocess default (#7353), x86-64 (#7349), Windows (#7355), bridge deleted (#7348), and the 479-test suite with no env matching the shadow baseline exactly. The target-aware shape is recorded because it is the part that generalises: native roots where the runtime can walk, shadow stack where it cannot. Also: layer 2 now reads THE DEFAULT rather than landed opt-in, layer 3's count is 41 rather than 54 after #7363, and the 2026-08-03 status header no longer says 'not yet adopted'. Co-authored-by: Ralph Küpper <ralph@skelpo.com>
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.
Native GC roots are now the default.
PERRY_RS4GC=1is no longer needed;PERRY_RS4GC=0reverts to the shadow stack for bisection.This is the fork
docs/engine-plan.mdcalled "the plan's next real fork, and it is the owner's — but it should be made on a schedule, not left to drift." Every blocker it listed is now closed.Evidence
Full 479-test gap suite, no env set, diffed against the pinned Node oracle:
All 128 try-carrying tests compiled — the class the deleted bridge (#7348) could never handle at all. All 10
gc_ratchetprobes byte-identical to Node. Runtime −1–2%; binary size +1.86% measured on a real dependency (zod, 81 native modules) rather than a synthetic.The part that made this non-trivial
A blanket flip would have been wrong.
gc_mapdeliberately refuses to emit a map for a target whose frame bases the runtime cannot resolve — a map nothing reads loses roots silently, which is the exact failure this backend exists to prevent. So flipping globally turns every watchOSarm64_32and ARM64-Windows compile into a hard error.The default is therefore native roots where the runtime can walk, shadow stack where it cannot. Falling back is not "no roots" — it is the other lowering of the same root-set analysis, which is only expressible because #7340 split analysis from lowering. Wired per module beside
set_jscvt_for_target, which already had this exact shape.PERRY_RS4GC=1still reachesgc_map's refusal rather than being quietly downgraded to a fallback, because an A/B arm must measure what it asked for.A test pins the support matrix in both directions. The one way this change breaks a platform is if my predicate is looser than
gc_map's refusals — then the compile hard-fails instead of falling back — so that is what is under test.Eight tests had to name their lowering
They assert on shadow-stack IR (
js_shadow_slot_bind, frame pushes) and broke when the default moved. They were correct about what they assert; they had simply never had to say which lowering, because there was only one default. Each now pins it through a thread-local guard, mirroringarena::quarantine'sProtectionModeGuard— thread-local and restoring, so one test's pin cannot change another's.That guard is deliberately separate from the per-target cell:
compile_modulesets the target decision per module, so a pin that wrote that cell would be erased the moment the test invoked codegen.What this unlocks
With native roots as the default, the shadow stack's lowering becomes removable — the analysis stays, since both mechanisms consume it. That is the "delete the shadow stack" goal, now a mechanical follow-up rather than an open question.
Caveat
My sweep is aarch64-macOS. Linux and Windows correctness rests on
gc-native-roots' ELF and PE arms, which are queued behind a deep runner backlog and have not reported yet.Summary by CodeRabbit
New Features
Documentation