docs(plan): restore the corrected size figure and close the adoption fork - #7371
Merged
Conversation
…fork 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'.
📝 WalkthroughWalkthroughThe GC engine plan now records statepoints as adopted by default. It updates runtime-rooting backlog counts, binary-size measurements, rollout sequencing, completed adoption gates, and target-aware native-root or shadow-stack behavior. ChangesGC adoption plan
Estimated code review effort: 1 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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: 3
🤖 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 `@docs/engine-plan.md`:
- Around line 31-32: Reconcile the Layer 3 backlog counts in the `perry-runtime`
row and the `#7341` worklist: either change the 54-item reference to 41, or
explicitly label 54 as the historical total and 41 as the remaining open
catches.
- Around line 31-32: Update the statepoints entry in the engine plan to remove
the inaccurate claim that it is the default; describe statepoints as adopted
while accurately retaining the opt-in/shadow-stack default behavior. Keep the
surrounding `perry-runtime` and issue references unchanged unless implementation
defaults are also intentionally updated to match.
- Around line 426-435: Update the root-density paragraph’s `97%` `__text` claim
to match the table’s calculation, using 96% or 96.2%, unless you explicitly
document a different denominator that produces 97%.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+31
to
+32
| | 2 | emitted code's liveness | #7280, #7271, #7252, #7243 | statepoints (#7108, #7174) | ✅ **THE DEFAULT** (#7370); landed #7314, made usable by #7339/#7340 | | ||
| | 3 | `perry-runtime` hand-written Rust | #7249, #7239, #7226, #7231 | `RuntimeHandleScope`, non-optional | mechanism exists (675 uses), **still optional**; **41** open catches (#7341) | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reconcile the Layer 3 backlog count.
Line 32 reports 41 open catches, but Lines 436-438 still describe a 54-item worklist under #7341. Update 54 to 41, or state that 54 is the historical total and 41 is the remaining count.
Also applies to: 436-438
🤖 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 `@docs/engine-plan.md` around lines 31 - 32, Reconcile the Layer 3 backlog
counts in the `perry-runtime` row and the `#7341` worklist: either change the
54-item reference to 41, or explicitly label 54 as the historical total and 41
as the remaining open catches.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 \
'PERRY_RS4GC|native_stack_roots_enabled|shadow.?stack|gc_map|statepoint.*default|default.*statepoint' \
.Repository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== docs/engine-plan relevant lines =="
sed -n '1,70p;426,465p' docs/engine-plan.md | cat -n
echo "== changelog snippets =="
for f in changelog.d/7314-statepoint-native-roots.md changelog.d/7345-delete-statepoint-bridge.md; do
if [ -f "$f" ]; then
echo "--- $f ---"
sed -n '1,220p' "$f" | cat -n
fi
done
echo "== PERRY_RS4GC references, focused =="
rg -n -C 3 '(^|[^A-Za-z0-9_])PERRY_RS4GC|RS4GC|native.*root|fallback|llvm-inprocess|target-aware|PERRY_SHADOW_STACK|PERRY_GC_SAFEPOINT_ONLY' --glob '!**/*.lock' --glob '!**/target/**' --glob '!**/dist/**' | sed -n '1,240p'Repository: PerryTS/perry
Length of output: 14554
Remove the inaccurate default-rooting claim.
#7370 does not change PERRY_RS4GC=1 from opt-in, and the runtime notes still say the default path leaves native stack-root behavior unchanged. Change line 31 to describe statepoints as adopted while keeping the default path opt-in/shadow-stack, or update the implementation and related docs so the claim matches the actual runtime default.
🤖 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 `@docs/engine-plan.md` around lines 31 - 32, Update the statepoints entry in
the engine plan to remove the inaccurate claim that it is the default; describe
statepoints as adopted while accurately retaining the opt-in/shadow-stack
default behavior. Keep the surrounding `perry-runtime` and issue references
unchanged unless implementation defaults are also intentionally updated to
match.
Comment on lines
+426
to
+435
| 2. **Reduce root density — worth doing, NOT a prerequisite.** Measured at | ||
| dependency scale, statepoints cost **+1.86% binary size** against **−1–2% | ||
| runtime**: a trade worth taking, and adoption shipped in #7370 without this. | ||
| Statepoints carry **zero fixed cost** — a function with nothing live across a | ||
| safepoint pays nothing at all — so a program's exposure is exactly its root | ||
| density, and 97% of the growth is `__text` (the per-root relocation | ||
| sequence), not metadata. #7314's compact map closed the metadata objection | ||
| completely; metadata was never the dominant term. Fewer roots is still the | ||
| same lever #7296 proved worth 9.9× on `matmul`, so size and speed pull | ||
| together rather than trading off. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the stated __text percentage.
Using the values in the table, 519,012 / 539,392 = 96.2%, not 97%. Change the statement to 96% or 96.2%, or document the different denominator used for 97%.
🧰 Tools
🪛 LanguageTool
[style] ~430-~430: Consider an alternative for the overused word “exactly”.
Context: ...ing at all — so a program's exposure is exactly its root density, and 97% of the gro...
(EXACTLY_PRECISELY)
🤖 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 `@docs/engine-plan.md` around lines 426 - 435, Update the root-density
paragraph’s `97%` `__text` claim to match the table’s calculation, using 96% or
96.2%, unless you explicitly document a different denominator that produces 97%.
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.
Three corrections to
docs/engine-plan.md, one of which matters more than a docs PR usually would.1. The plan carries a number it tells you not to quote
Only +18.95% appears on main. That is a synthetic worst case — three heap values live across an allocation in every one of 2000 functions — and it overstates real exposure by an order of magnitude. The dependency-scale measurement is +1.86% (
zodfrom source, 81 native modules, 29 MB binary).The correction was written when the synthetic was retracted, but it never reached main: #7345 squash-merged as 24 insertions — the first commit only — so the follow-up correction commit was dropped on the floor.
That is the same failure this document already records for #7321: "was WRONG, and is recorded as wrong because it survived into an issue and a job name before anyone measured it." A retracted figure sitting in the authoritative plan is how the next person plans from it. The real number now leads; the worst case stays, marked do-not-quote, because it isolates the mechanism (root-free code costs exactly zero).
2. Sequencing step 2 called root density a prerequisite
It reasoned directly from the retracted figure — "adopting them as the default today therefore regresses the owner's stated goal of minimal binary size." Adoption shipped in #7370 without it. Root reduction is still worth doing and still the same lever #7296 proved worth 9.9× on
matmul, but it gates nothing.3. The adoption fork is closed
Every gate it listed is shut:
llvm-inprocessdefault (#7353), x86-64 (#7349), Windows (#7355), bridge deleted (#7348), and the full 479-test suite with no env set matching the shadow baseline exactly — 447 / 19 / 13, zero regressions.The target-aware shape is recorded, because that is the part that generalises: native roots where the runtime can walk, shadow stack where it cannot. A blanket flip would hard-fail every watchOS
arm64_32and ARM64-Windows compile.Also: layer 2 reads THE DEFAULT rather than "landed opt-in", layer 3's count is 41 (not 54) after #7363, and the status header no longer says "not yet adopted".
Docs only.
Summary by CodeRabbit