fix(codegen): #6219 shadow-frame codegen must not blow up on deeply-nested closures#6253
Open
proggeramlug wants to merge 1 commit into
Open
fix(codegen): #6219 shadow-frame codegen must not blow up on deeply-nested closures#6253proggeramlug wants to merge 1 commit into
proggeramlug wants to merge 1 commit into
Conversation
…ested closures #6219 extended shadow-frame emission (GC exact-root spilling) from top-level functions to closures and methods, calling collect_pointer_typed_locals once per closure/method. That collector recurses into nested closure bodies, so on deeply-nested bundles — the Next.js standalone server: 700+ modules, whole webpack chunks lowered as single closures thousands of locals deep — codegen never reached LLVM emission (25 min+ and still climbing on a 105 MB bundle; CI missed it because its inputs are small). Three independent super-linear costs, all in collectors/pointer_locals.rs: 1. Re-walk. Each enclosing frame re-descended every nested closure subtree — O(nesting^2) walks. Memoize each closure body's collected writes by body identity (CLOSURE_WRITES_MEMO), so a subtree is walked once total. 2. Clone. The memo merged a closure's ENTIRE subtree writes into every ancestor frame, then pruned — O(nesting^2) Expr clones. Store only FREE writes (targets not declared within the subtree). HIR local ids are globally unique, so an ancestor only ever cares about writes to ITS OWN captured locals: a write to frame F's local L stays free relative to every intermediate closure and propagates up to F unchanged, while writes to a closure's own locals — which every ancestor discarded at the prune anyway — never get cloned. 3. Fixpoint. The type-refinement loop ran unbounded (while changed), O(locals x iterations): a long def-use chain needs one pass per link, and a thousands-of-locals webpack factory never converged. Cap iterations (16) and hard-gate refinement off above 8192 locals. All three are strictly SAFE. The fixpoint only ever GROWS non_pointer_locals to DROP slots, and every slot decision gates on `is_ptr_typed(declared_ty) && !non_pointer_locals.contains(id)` — so curtailing any of it only ever leaves an extra safe GC root, never removes a needed one. No under-rooting / use-after-free is possible; slot maps are identical to the uncapped analysis for all normal-sized frames. Result: the Next.js standalone server compiles in ~7 min with shadow frames ON (was: never). Validated 10/10 on closure/GC-heavy gap tests under PERRY_GC_FORCE_EVACUATE=1 PERRY_GC_VERIFY_EVACUATION=1 (no under-rooting).
📝 WalkthroughWalkthroughThe pointer-local collector now memoizes closure write analysis, distinguishes free writes from closure-owned locals, restricts refinement to current-frame locals, and caps the type-refinement fixpoint. ChangesPointer-local analysis
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 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/collectors/pointer_locals.rs`:
- Around line 11-32: Reset CLOSURE_WRITES_MEMO for every compile run to prevent
stale pointer/length entries from being reused after HIR lifetimes end. Add a
clear operation at the compile entry or exit, using the existing compilation
orchestration function as the integration point, and ensure cleanup also occurs
on failure paths or use a per-compile generation key.
🪄 Autofix (Beta)
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: 63fb9085-e7fa-43e2-9402-30c37b40be59
📒 Files selected for processing (1)
crates/perry-codegen/src/collectors/pointer_locals.rs
Comment on lines
+11
to
+32
| thread_local! { | ||
| /// #6219 perf: cache each closure body's collected local-writes, keyed by | ||
| /// the body slice's identity `(ptr, len)`, so a closure subtree is walked | ||
| /// ONCE total instead of once per enclosing frame. Writes are keyed by | ||
| /// GLOBAL local id, so the same cached set is correct for every ancestor | ||
| /// frame AND the closure's own frame — the collection is compositional. | ||
| /// | ||
| /// Without this, emitting a shadow frame per closure/method (#6219) made | ||
| /// each frame's `collect_pointer_typed_locals` re-descend the full nested | ||
| /// closure subtree, so a body nested `D` deep was walked `O(D)` times — | ||
| /// `O(nesting²)` overall, which never finished on deeply-nested bundles | ||
| /// (the Next.js standalone server, thousands of closures 5–10 deep). | ||
| /// | ||
| /// Safe because within a single `perry compile` the HIR is alive for the | ||
| /// whole (parallel) codegen — no body is freed, so no `(ptr, len)` is ever | ||
| /// reused for a different body; and each closure body is reachable from | ||
| /// exactly one enclosing tree, so its writes don't depend on the caller. | ||
| static CLOSURE_WRITES_MEMO: std::cell::RefCell< | ||
| HashMap<(usize, usize), std::rc::Rc<HashMap<u32, Vec<LocalWrite>>>>, | ||
| > = std::cell::RefCell::new(HashMap::new()); | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Is codegen thread pool / collect_pointer_typed_locals reused across multiple compiles in one process?
rg -nP '\bcollect_pointer_typed_locals\b' -C3
# Look for a compile-driver / worker-pool loop that could reuse threads across compiles
rg -nP 'fn\s+compile\b|rayon|thread(pool|::spawn)|par_iter|scope' crates/perry-codegen -C2
# Any existing per-compile thread_local reset patterns to mirror?
rg -nP 'with\(\|.*\|\s*.*\.borrow_mut\(\)\.clear\(\)' cratesRepository: PerryTS/perry
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate relevant symbols =="
rg -n "collect_pointer_typed_locals|CLOSURE_WRITES_MEMO|pointer_locals" crates/perry-codegen -S
echo
echo "== inspect pointer_locals.rs outline =="
ast-grep outline crates/perry-codegen/src/collectors/pointer_locals.rs --view expanded || true
echo
echo "== inspect nearby lines in pointer_locals.rs =="
sed -n '1,120p' crates/perry-codegen/src/collectors/pointer_locals.rs
echo
echo "== find compile entrypoints / thread-pool use =="
rg -n "fn\s+compile\b|rayon|thread_pool|ThreadPool|par_iter|spawn|scope" crates -SRepository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate relevant symbols =="
rg -n "collect_pointer_typed_locals|CLOSURE_WRITES_MEMO|pointer_locals" crates/perry-codegen -S
echo
echo "== inspect pointer_locals.rs outline =="
ast-grep outline crates/perry-codegen/src/collectors/pointer_locals.rs --view expanded || true
echo
echo "== inspect nearby lines in pointer_locals.rs =="
sed -n '1,140p' crates/perry-codegen/src/collectors/pointer_locals.rs
echo
echo "== find compile entrypoints / thread-pool use =="
rg -n "fn\s+compile\b|rayon|thread_pool|ThreadPool|par_iter|spawn|scope" crates -SRepository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== pointer_locals memo usage =="
rg -n "CLOSURE_WRITES_MEMO|collect_pointer_typed_locals|collect_expr_writes_in_closure" crates/perry-codegen/src/collectors/pointer_locals.rs -n -S
echo
echo "== pointer_locals.rs around memo + closure arm =="
sed -n '1,120p' crates/perry-codegen/src/collectors/pointer_locals.rs
printf '\n---\n'
sed -n '500,585p' crates/perry-codegen/src/collectors/pointer_locals.rs
echo
echo "== codegen entry outline =="
ast-grep outline crates/perry-codegen/src/codegen/entry.rs --view expanded || true
echo
echo "== codegen entry around compile entry / initialization =="
sed -n '1,220p' crates/perry-codegen/src/codegen/entry.rs
printf '\n---\n'
sed -n '220,420p' crates/perry-codegen/src/codegen/entry.rs
printf '\n---\n'
sed -n '420,620p' crates/perry-codegen/src/codegen/entry.rs
echo
echo "== search for cache resets or thread_local resets =="
rg -n "CLOSURE_WRITES_MEMO|thread_local!|borrow_mut\(\)\.clear\(|clear\(\);" crates/perry-codegen/src crates/perry-hir/src -SRepository: PerryTS/perry
Length of output: 45152
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== compile command / entrypoint =="
rg -n "pub\s+fn\s+compile\b|fn\s+compile\b|compile_module_entry|compile_ir|codegen::|perry-codegen" crates/perry/src crates/perry-codegen/src -S
echo
echo "== inspect perry compile command =="
sed -n '1,260p' crates/perry/src/commands/compile.rs
echo
echo "== inspect codegen mod around any thread-local cleanup =="
sed -n '1780,1865p' crates/perry-codegen/src/codegen/mod.rs
echo
echo "== search for any pointer_locals memo reset or compile-boundary cleanup =="
rg -n "CLOSURE_WRITES_MEMO|pointer_locals|clear\(\).*thread_local|thread_local!.*clear" crates/perry-codegen crates/perry-hir crates/perry -SRepository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== CLI / long-lived process entrypoints =="
rg -n "\bwatch\b|\bdaemon\b|\bserve\b|\blsp\b|\brepl\b|repeat|loop.*compile|run_with_parse_cache\(|commands::compile::run\(" crates/perry/src -S
echo
echo "== main binaries / command dispatch =="
fd -a 'main.rs' crates/perry/src crates/perry -t f
fd -a 'lib.rs' crates/perry/src crates/perry -t f
echo
echo "== compile command callers =="
rg -n "run_with_parse_cache\(|commands::compile::run\(" crates/perry/src -SRepository: PerryTS/perry
Length of output: 11490
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' crates/perry/src/commands/dev.rs
printf '\n---\n'
sed -n '220,420p' crates/perry/src/commands/dev.rsRepository: PerryTS/perry
Length of output: 13868
Reset CLOSURE_WRITES_MEMO between compile runs
This thread_local survives multiple compile runs in the same process, so a reused worker thread can hit a stale (ptr, len) from a dropped HIR and skip a needed shadow slot. Clear it at compile entry/exit, or key it by a per-compile generation.
🤖 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/collectors/pointer_locals.rs` around lines 11 - 32,
Reset CLOSURE_WRITES_MEMO for every compile run to prevent stale pointer/length
entries from being reused after HIR lifetimes end. Add a clear operation at the
compile entry or exit, using the existing compilation orchestration function as
the integration point, and ensure cleanup also occurs on failure paths or use a
per-compile generation key.
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.
Summary
Follow-up to #6219. That PR extended shadow-frame emission (GC exact-root spilling) from top-level functions to closures and methods, calling
collect_pointer_typed_localsonce per closure/method. The collector recurses into nested closure bodies, so on deeply-nested bundles codegen never reached LLVM emission — the Next.js standalone server (700+ modules, whole webpack chunks lowered as single closures thousands of locals deep, 105 MB bundle) sat in the collectors for 25 min+ and climbing. CI missed it because its inputs are small.Three independent super-linear costs, all in
crates/perry-codegen/src/collectors/pointer_locals.rs:O(nesting²)walksCLOSURE_WRITES_MEMO) — a subtree is walked once totalO(nesting²)Exprcloneswhile changed),O(locals × iterations); a long def-use chain needs one pass per link and a thousands-of-locals factory never convergedWhy it's safe (no under-rooting)
Every slot decision gates on
is_ptr_typed(declared_ty) && !non_pointer_locals.contains(id), and the fixpoint only ever growsnon_pointer_locals(to drop slots). So curtailing any of the three only ever leaves an extra safe GC root — it can never remove a needed one. No use-after-free is possible.F's localLstays free relative to every intermediate closure and propagates up toFunchanged; writes to a closure's own locals — which every ancestor discarded at the prune anyway — simply never get cloned. Slot maps are therefore identical to before for every frame.Validation
PERRY_GC_FORCE_EVACUATE=1 PERRY_GC_VERIFY_EVACUATION=1(stresses every marked nursery object through evacuation + panics on any stale slot) — confirms no under-rooting from the curtailed analysis:closures,5437_class_ref_shadows_captured_local,5437_class_ref_static_closure_method,in_operator_closure,array_callbacks_2797_2798_2799,map_set_extended,async_advanced,weakref_finalization,mapset_iter_2831_2830_2856,json_map_set.cargo fmtclean.Code-only; no version/changelog bump (maintainer folds metadata at merge).
Summary by CodeRabbit
Bug Fixes
Performance
Stability