merge: land #8633 (spill estimate sees allocating literals + call temporaries) - #8634
Merged
Conversation
added 3 commits
August 23, 2026 08:12
… the RS4GC root-spill estimate (#8583) A minified bundle data table compiles as one giant array-of-arrays literal: the Claude Code 2.1.112 bundle's `__33499` lowered to 11,104 `js_array_from_values` allocations (one per sub-array) and ~20k GC safepoints. The shadow-frame spill estimate (`slot_count * safepoint_sites`, #8583) missed it on two counts, so the function stayed on native statepoints and `rewrite-statepoints-for-gc` fanned out for >3h / ~30GiB — never reaching the post-rewrite budget assertion, which only fires after a rewrite that here never finished: 1. `count_safepoint_sites` counted only Call/New-family expressions, not allocating object/array literals, which lower to a collecting runtime call (`js_array_from_values` / `js_object_*`) and each get an RS4GC statepoint. 2. `slot_count` counted only named pointer locals (the shadow-slot map), not the ~one pointer temporary each call result leaves live across later safepoints — the roots RS4GC actually relocates. The estimate now counts allocating literals as safepoints and adds per-safepoint temporaries to the root count, so a data-table-shaped function spills to the shadow frame like the module entry does. Over-approximation biased toward spilling (a false positive is a cheap shadow frame; a missed fan-out is not); a function needs ~2000+ allocating ops to cross the default threshold, i.e. only genuinely huge (usually module-init) functions. Verified on a 3,000-row nested-array synthetic: estimate 3001x3001 -> spilled, compiled in 64s instead of not finishing, byte-correct output. Claude-Session: https://claude.ai/code/session_01TwxRkALrR9HKSF1zKLSTAF
#8633 changes the estimate to (slot_count + sites) x sites, but #8623's two endpoint tests still called root_relocation_estimate(slot, sites) directly, so they pinned a formula production no longer uses. Both still reached the right verdict (moderate 8.0M vs 12.0M, catastrophic 84.3M vs 11.3B -- same side of the 32M threshold either way), but they would no longer catch a regression in the live_roots composition this PR introduces. Extract spill_live_root_count() so production and the tests share one definition, and route both endpoint tests through it.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe codegen now counts allocating object and array expressions as safepoints. Root-spill estimates include one pointer temporary per safepoint and use the expanded count for spill decisions and diagnostics. ChangesRoot spill estimation
Estimated code review effort: 3 (Moderate) | ~20 minutes ✨ 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 |
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.
Lands #8633 plus one test fix it needs.
Audit
The change is sound and well-evidenced. It closes a genuine two-axis under-count
in the RS4GC spill estimate:
minified data table (no
Expr::Callanywhere) looked safepoint-free — that isthe
__33499shape that fanned out for >3 h / ~30 GiB;slot_countcounted only named pointer locals, missing the call-resulttemporaries RS4GC actually relocates.
Both directions over-approximate toward spilling, which is the correct bias
here: a false-positive shadow frame is cheap, a missed fan-out is not.
Interaction with #8623 (which raised the threshold to 32M because spilling
moderate functions measured slower): this pushes the other way by raising
estimates, so it's worth stating where they meet. The crossover is roughly
4,000–5,700 allocating operations before a function starts spilling — module-init
data-table territory, not hot code. The PR's "~2000+" is a conservative
statement of the same bound.
The test fix
#8633 changes the production formula to
(slot_count + sites) × sites, but#8623's two endpoint tests still called
root_relocation_estimate(slot, sites)directly:
Both still land on the correct side of the 32M threshold, so nothing was broken
— but those tests exist specifically to pin both ends of the threshold, and they
would no longer catch a regression in the
live_rootscomposition this PRintroduces. Extracted
spill_live_root_count()so production and the testsshare one definition, and routed both endpoint tests through it.
Validation
cargo fmt --all -- --check: pass.perry-codegenlib: 1182 passed, 0 failed (1180 onmain+ the two newsafepoint tests).
array_and_object_literals_are_safepoints,nested_array_literals_recurse,and the three
root_spill_default_tests.Fork PR, so this lands as a branch rather than a push to the contributor's head ref.
Summary by CodeRabbit