fix(gc): trace every overflow slot — the layout mask under-reports them#6506
Merged
proggeramlug merged 1 commit intoJul 17, 2026
Merged
Conversation
Contributor
Author
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughChangesOverflow field tracing
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 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/object/mod.rs (1)
1012-1017: 📐 Maintainability & Code Quality | 🔵 TrivialRebuild
perry-runtime-staticandperry-stdlib-staticbefore merge.This runtime change should be rebuilt through the static wrapper crates so the shipped archives stay in sync.
🤖 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/object/mod.rs` around lines 1012 - 1017, Rebuild the static wrapper crates perry-runtime-static and perry-stdlib-static after the runtime change, ensuring their shipped archives include the updated overflow-slot scanning behavior in the fields iteration.Source: Coding guidelines
🤖 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/tests/cycle_state.rs`:
- Around line 750-768: After seeding the overflow fields with
test_seed_overflow_fields_vec, directly invoke the object field-slot walker
visit_overflow_field_slots_mut and assert that it visits the expected child
slot/value. Keep the existing cycle-marking assertion unchanged so the test
independently verifies both the walker and scan_overflow_fields_roots_mut paths.
---
Nitpick comments:
In `@crates/perry-runtime/src/object/mod.rs`:
- Around line 1012-1017: Rebuild the static wrapper crates perry-runtime-static
and perry-stdlib-static after the runtime change, ensuring their shipped
archives include the updated overflow-slot scanning behavior in the fields
iteration.
🪄 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: 4db9136e-c5ed-4983-b906-15e19a200be7
📒 Files selected for processing (2)
crates/perry-runtime/src/gc/tests/cycle_state.rscrates/perry-runtime/src/object/mod.rs
Comment on lines
+750
to
+768
| let child = young_leaf(); | ||
| let mut values = vec![crate::value::TAG_UNDEFINED; 51]; | ||
| values[50] = string_bits(child); | ||
| crate::object::test_seed_overflow_fields_vec(owner as usize, values); | ||
|
|
||
| // Age the owner/child block out of the block-persistence window (that | ||
| // pass would otherwise force-mark the child as a register-holding | ||
| // candidate and mask the missing trace). | ||
| let aged_from = crate::arena::general_block_count(); | ||
| let mut filler_blocks = 0usize; | ||
| while filler_blocks < 7 { | ||
| for _ in 0..64 { | ||
| let _ = unsafe { crate::arena::arena_alloc_gc(4096, 8, GC_TYPE_STRING) }; | ||
| } | ||
| filler_blocks = crate::arena::general_block_count().saturating_sub(aged_from); | ||
| } | ||
|
|
||
| let mut state = GcCycleState::new_full(trace_snapshot(GcTriggerKind::Manual)); | ||
| run_cycle_until_phase(&mut state, GcCyclePhase::Sweep); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Directly verify the object field-slot walker too.
The final marking assertion can pass solely through scan_overflow_fields_roots_mut, because overflow fields are independently registered as mutable roots. Therefore, reverting visit_overflow_field_slots_mut to its mask-based behavior would not necessarily fail this test.
Add a direct assertion after seeding:
Proposed additional assertion
crate::object::test_seed_overflow_fields_vec(owner as usize, values);
+
+ let mut visited_slots = 0;
+ crate::object::visit_overflow_field_slots_mut(owner as usize, |_| {
+ visited_slots += 1;
+ });
+ assert_eq!(
+ visited_slots, 51,
+ "the object trace must visit every live overflow slot"
+ );This independently protects both walkers required by the regression objective.
📝 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
| let child = young_leaf(); | |
| let mut values = vec![crate::value::TAG_UNDEFINED; 51]; | |
| values[50] = string_bits(child); | |
| crate::object::test_seed_overflow_fields_vec(owner as usize, values); | |
| // Age the owner/child block out of the block-persistence window (that | |
| // pass would otherwise force-mark the child as a register-holding | |
| // candidate and mask the missing trace). | |
| let aged_from = crate::arena::general_block_count(); | |
| let mut filler_blocks = 0usize; | |
| while filler_blocks < 7 { | |
| for _ in 0..64 { | |
| let _ = unsafe { crate::arena::arena_alloc_gc(4096, 8, GC_TYPE_STRING) }; | |
| } | |
| filler_blocks = crate::arena::general_block_count().saturating_sub(aged_from); | |
| } | |
| let mut state = GcCycleState::new_full(trace_snapshot(GcTriggerKind::Manual)); | |
| run_cycle_until_phase(&mut state, GcCyclePhase::Sweep); | |
| let child = young_leaf(); | |
| let mut values = vec![crate::value::TAG_UNDEFINED; 51]; | |
| values[50] = string_bits(child); | |
| crate::object::test_seed_overflow_fields_vec(owner as usize, values); | |
| let mut visited_slots = 0; | |
| crate::object::visit_overflow_field_slots_mut(owner as usize, |_| { | |
| visited_slots += 1; | |
| }); | |
| assert_eq!( | |
| visited_slots, 51, | |
| "the object trace must visit every live overflow slot" | |
| ); | |
| // Age the owner/child block out of the block-persistence window (that | |
| // pass would otherwise force-mark the child as a register-holding | |
| // candidate and mask the missing trace). | |
| let aged_from = crate::arena::general_block_count(); | |
| let mut filler_blocks = 0usize; | |
| while filler_blocks < 7 { | |
| for _ in 0..64 { | |
| let _ = unsafe { crate::arena::arena_alloc_gc(4096, 8, GC_TYPE_STRING) }; | |
| } | |
| filler_blocks = crate::arena::general_block_count().saturating_sub(aged_from); | |
| } | |
| let mut state = GcCycleState::new_full(trace_snapshot(GcTriggerKind::Manual)); | |
| run_cycle_until_phase(&mut state, GcCyclePhase::Sweep); |
🤖 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/tests/cycle_state.rs` around lines 750 - 768,
After seeding the overflow fields with test_seed_overflow_fields_vec, directly
invoke the object field-slot walker visit_overflow_field_slots_mut and assert
that it visits the expected child slot/value. Keep the existing cycle-marking
assertion unchanged so the test independently verifies both the walker and
scan_overflow_fields_roots_mut paths.
proggeramlug
force-pushed
the
fix/6495-overflow-visit-full
branch
from
July 17, 2026 07:34
c9bf9ea to
57fe0ca
Compare
…em (PerryTS#6495) `visit_overflow_field_slots_mut` (mark-phase trace + rewrite walks) and `scan_overflow_fields_roots_mut` consulted the per-object layout slot mask to visit only pointer-bearing overflow slots. The mask is maintained by `layout_note_slot` at store time — but not every overflow write path notes: GC owner moves merge entries via `merge_overflow_fields` with no notes, so a usable-looking SIDE_MASK can claim pointer-bearing overflow slots are pointer-free. The trace then skips them, and a child reachable only through such a slot is swept while referenced. Observed at bundle scale in a large compiled TUI app (diagnostics from the 49..63 held live NaN-boxed heap pointers — ~3,200 skipped pointer-slot visits per run. Visit every overflow slot unconditionally. The Vec's length is the live overflow region, and objects with large overflow populations sit in UNKNOWN layout state in practice (every dynamic-shape store degrades the layout), so the mask fast path bought little exactly where it would cost. The inline-slot region keeps its layout-driven visit — its stores all funnel through the noting choke points. Regression test `overflow_slots_beyond_layout_mask_are_traced`: a rooted object with a SIDE_MASK claiming slots 0..=48 and the only reference to a live string sitting in overflow slot 50 (seeded without notes, the merge_overflow_fields shape); asserts the string is MARKED at sweep entry. Validated failing-then-passing against the old fast path (block-persistence window aged out so the recent-block resurrection pass cannot mask the miss). perry-runtime suite: 1322/0 single-threaded. Claude-Session: https://claude.ai/code/session_01M44HoYS3CAYZyagm3ZzYDG
proggeramlug
force-pushed
the
fix/6495-overflow-visit-full
branch
from
July 17, 2026 09:34
57fe0ca to
e90fe64
Compare
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.
Fixes #6495.
Problem
The mark-phase trace (
visit_overflow_field_slots_mut, also used by rewrite walks) and the overflow root scanner visited only the overflow slots the per-object layout mask claimed are pointer-bearing. That mask is maintained bylayout_note_slotat store time — but not every overflow write path notes.merge_overflow_fields(GC owner moves) writes merged values with no notes, so a usable-lookingSIDE_MASKcan under-report: pointer-bearing overflow slots invisible to the trace, their referents swept while referenced — the same live-sweep failure class as #6494, different mechanism.Observed at bundle scale in a large compiled TUI app (via the #6494 diagnostics): 17 objects whose masks capped at bit 48 while slots 49..63 held live NaN-boxed pointers, ~3,200 skipped pointer-slot visits per run.
Fix
Visit every overflow slot unconditionally in both walkers. The Vec's length is the live overflow region; objects with large overflow populations are in
UNKNOWNlayout state in practice (dynamic-shape stores degrade the layout), so the mask fast path bought little exactly where it would cost the most. The inline-slot region keeps its layout-driven visit — those stores all funnel through the noting choke points.Test
overflow_slots_beyond_layout_mask_are_traced: rooted object,SIDE_MASKclaiming slots 0..=48, the only reference to a live string seeded (note-free, themerge_overflow_fieldsshape) into overflow slot 50; asserts the string is MARKED at sweep entry. Validated failing-then-passing against the old fast path, with the block-persistence window aged out so the recent-block resurrection pass can't mask the miss.perry-runtime: 1322/0 single-threaded.https://claude.ai/code/session_01M44HoYS3CAYZyagm3ZzYDG
Summary by CodeRabbit
Bug Fixes
Tests