fix(gc): minor sweep reclaimed array-growth forwarding stubs still referenced from old-gen parents#6471
Merged
proggeramlug merged 1 commit intoJul 16, 2026
Conversation
…ferenced from old-gen parents Array growth installs a PERMANENT forwarding stub at the pre-growth address (PerryTS#6228): stale pre-growth pointers keep resolving for reads, and references to stubs are never rewritten. But the minor sweep reclaimed any FORWARDED stub that was neither marked this cycle nor inside the recent-allocation block window, on the assumption that unmarked stubs are "stale array-growth remnants". That assumption is unsound in a minor: old-gen parents are black leaves — their slots are visited only via remembered-set dirty pages — so a stale pre-growth pointer held in an old parent whose page is not dirty (a long-lived Map's entries buffer that hasn't been written since) never marks the stub. "Unmarked" does NOT imply "unreferenced". Once the stub's block aged out of the recent window, the sweep freed it; reads through the still-live stale pointer then returned reused-memory garbage. Observed in a large esbuild-bundled TUI app: a render cell-cache Map value (a grown cells array reached via its pre-growth pointer) read `.length` back as a heap pointer after a few minors, blowing up every subsequent frame — typed input never appeared on screen. The failure needed an old parent, a non-dirty page, and window expiry to line up, which is why smaller reproductions kept passing. Fix: thread `retain_all_forwarded_stubs` from `step_sweep` into the arena sweep. Minor sweeps retain EVERY forwarding stub — they cannot prove one unreferenced. Full traces visit every live parent, so their mark-based stub reclaim stays as-is and bounds the accumulation. The synchronous sweep wrappers use `do_age_bump` (true exactly for minor sweeps) as the same signal. Also adds two env-gated, non-fatal GC diagnostics (`PERRY_GC_VERIFY_MARK`) used to isolate this: a marked-heap mark-invariant reporter on the copied-minor path, and a pre-sweep old-parent → unmarked-young-child reporter at the budgeted minor's AtomicFinalize boundary. Regression test drives `IncrementalSweepState` exactly as a minor builds it, with a genuinely unmarked, window-expired growth stub (small-allocation churn to advance the general window; the test-build conservative-scan default masks the scenario otherwise): pre-fix the sweep strips FORWARDED and reclaims the stub; post-fix it is retained and reads through the stale pointer still resolve. Claude-Session: https://claude.ai/code/session_01XRHKpxgDnVB3GJdsV9ud7g
📝 WalkthroughWalkthroughMinor GC sweep configuration now retains forwarding stubs through arena cleanup. Optional verification probes report marked-to-unmarked references at minor collection boundaries, with regression coverage for stale growth pointers and updated sweep-state construction. ChangesMinor GC retention and verification
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GcCycleState
participant IncrementalSweepState
participant ArenaSweepObjectsState
participant ForwardingStub
GcCycleState->>IncrementalSweepState: configure minor sweep with stub retention
IncrementalSweepState->>ArenaSweepObjectsState: pass retention flag
ArenaSweepObjectsState->>ForwardingStub: process forwarded object
ArenaSweepObjectsState-->>ForwardingStub: retain stub when flag is set
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: 1
🧹 Nitpick comments (1)
crates/perry-runtime/src/gc/verify.rs (1)
791-794: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSort the histogram entries for deterministic diagnostic output.
HashMapiteration is non-deterministic. Sorting the entries by parent and child object types before logging will ensure consistent output across different test runs or executions.♻️ Proposed refactor
let mut hist_str = String::new(); - for ((pt, ct), n) in &hist { - hist_str.push_str(&format!(" {}({})->{}({})={}", tn(*pt), pt, tn(*ct), ct, n)); + let mut entries: Vec<_> = hist.iter().collect(); + entries.sort_unstable_by_key(|(&(pt, ct), _)| (pt, ct)); + for (&(pt, ct), n) in entries { + hist_str.push_str(&format!(" {}({})->{}({})={}", tn(pt), pt, tn(ct), ct, n)); }🤖 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/verify.rs` around lines 791 - 794, Sort the entries from hist by parent and child object types before building hist_str in the histogram formatting loop. Preserve the existing tn formatting and counts, but iterate over the sorted entries so diagnostic output is deterministic despite hist being a HashMap.
🤖 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-runtime/src/gc/verify.rs`:
- Around line 786-788: Update the minor-sweep old-parent verification around
old_arena_walk_objects and include MALLOC_STATE in the objects passed to
visit_parent. Ensure malloc-registry old-generation objects, including large
closures, are traversed so their child edges receive the same verification as
arena-backed objects.
---
Nitpick comments:
In `@crates/perry-runtime/src/gc/verify.rs`:
- Around line 791-794: Sort the entries from hist by parent and child object
types before building hist_str in the histogram formatting loop. Preserve the
existing tn formatting and counts, but iterate over the sorted entries so
diagnostic output is deterministic despite hist being a HashMap.
🪄 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: 8791663d-8b37-4c70-a08a-dbe70d7df187
📒 Files selected for processing (6)
crates/perry-runtime/src/gc/copying.rscrates/perry-runtime/src/gc/cycle.rscrates/perry-runtime/src/gc/oldgen.rscrates/perry-runtime/src/gc/tests/debt_pacer.rscrates/perry-runtime/src/gc/tests/incremental_sweep_reclaim.rscrates/perry-runtime/src/gc/verify.rs
Comment on lines
+786
to
+788
| crate::arena::old_arena_walk_objects(|hp| { | ||
| visit_parent(hp as *mut GcHeader); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Include MALLOC_STATE in the minor-sweep old-parent verification.
The diagnostic probe walks old_arena_walk_objects to verify that no surviving old-gen parents have unmarked sweep-eligible children. However, it currently omits MALLOC_STATE, meaning any old-gen objects allocated via the malloc registry (like large closures) are skipped and their edges are not verified.
🛠️ Proposed fix to verify old-gen malloc objects
crate::arena::old_arena_walk_objects(|hp| {
visit_parent(hp as *mut GcHeader);
});
+ MALLOC_STATE.with(|s| {
+ let s = s.borrow();
+ for &header in s.objects.iter() {
+ visit_parent(header);
+ }
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| crate::arena::old_arena_walk_objects(|hp| { | |
| visit_parent(hp as *mut GcHeader); | |
| }); | |
| crate::arena::old_arena_walk_objects(|hp| { | |
| visit_parent(hp as *mut GcHeader); | |
| }); | |
| MALLOC_STATE.with(|s| { | |
| let s = s.borrow(); | |
| for &header in s.objects.iter() { | |
| visit_parent(header); | |
| } | |
| }); |
🤖 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/verify.rs` around lines 786 - 788, Update the
minor-sweep old-parent verification around old_arena_walk_objects and include
MALLOC_STATE in the objects passed to visit_parent. Ensure malloc-registry
old-generation objects, including large closures, are traversed so their child
edges receive the same verification as arena-backed objects.
This was referenced Jul 18, 2026
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.
Array growth installs a PERMANENT forwarding stub at the pre-growth address (#6228): stale pre-growth pointers keep resolving for reads, and references to stubs are never rewritten. But the minor sweep reclaimed any
FORWARDEDstub that was neither marked this cycle nor inside the recent-allocation block window, assuming unmarked stubs are "stale array-growth remnants".That assumption is unsound in a minor: old-gen parents are black leaves — their slots are visited only via remembered-set dirty pages — so a stale pre-growth pointer held in an old parent whose page is not dirty (a long-lived Map's entries buffer that hasn't been written since) never marks the stub. "Unmarked" does NOT imply "unreferenced". Once the stub's block aged out of the recent window, the sweep freed it; reads through the still-live stale pointer then returned reused-memory garbage.
Observed in a large esbuild-bundled TUI app: a render cell-cache Map value (a grown cells array reached via its pre-growth pointer) read
.lengthback as a heap pointer after a few minors — pointer-shaped, varying per run, elements at higher offsets still intact — blowing up every subsequent frame, so typed input never appeared on screen. The failure needs an old parent + non-dirty page + window expiry to line up, which is why smaller reproductions kept passing.Fix
Thread
retain_all_forwarded_stubsfromstep_sweepinto the arena sweep:do_age_bump(true exactly for minor sweeps) as the same signal.Diagnostics
Adds two env-gated, non-fatal reporters (
PERRY_GC_VERIFY_MARK) used to isolate this: a marked-heap mark-invariant reporter on the copied-minor path, and a pre-sweep old-parent → unmarked-young-child reporter at the budgeted minor's AtomicFinalize boundary.Test
minor_sweep_retains_window_expired_growth_stubdrivesIncrementalSweepStateexactly as a minor builds it, with a genuinely unmarked, window-expired growth stub (small-allocation churn to advance the general window past the stub's block; the test-build full-conservative-scan default and large-allocation routing both mask the scenario otherwise — validated failing-then-passing: pre-fix the sweep stripsFORWARDEDand reclaims the stub; post-fix it is retained,retained_forwarded_stub_objectsreports it, and reads through the stale pointer still resolve).Full
perry-runtimesuite: 1315 passed / 0 failed (single-threaded).https://claude.ai/code/session_01XRHKpxgDnVB3GJdsV9ud7g
Summary by CodeRabbit
Bug Fixes
Diagnostics
PERRY_GC_VERIFY_MARKis enabled.