Fix array side-table GC lifecycles - #6330
Merged
Merged
Conversation
📝 WalkthroughWalkthroughGC now updates and removes array-backed named-property and iterator-marker side tables during object movement and dead-owner cleanup. Runtime registration and six lifecycle tests cover pruning, address reuse, evacuation rekeying, pointer rewriting, and iterator branding. ChangesArray side-table lifecycle
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant GC
participant ArraySideTables
participant MapIteratorRegistry
participant SetIteratorRegistry
GC->>ArraySideTables: Rewrite moved named-property roots
GC->>MapIteratorRegistry: Rewrite moved iterator-array roots
GC->>SetIteratorRegistry: Rewrite moved iterator-array roots
GC->>ArraySideTables: Prune dead owners
GC->>MapIteratorRegistry: Prune dead owners
GC->>SetIteratorRegistry: Prune dead owners
Possibly related PRs
Suggested labels: 🚥 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: 2
🧹 Nitpick comments (1)
crates/perry-runtime/src/map.rs (1)
33-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate GC-lifecycle logic with
set.rs.
scan_map_iterator_array_roots_mut/prune_dead_map_iterator_array_ownersare identical in structure toscan_set_iterator_array_roots_mut/prune_dead_set_iterator_array_ownersincrates/perry-runtime/src/set.rs(lines 32-53), differing only by the thread-local name. Since this is subtle, correctness-critical GC rekeying code, a shared generic helper would avoid future divergence between the two copies.♻️ Suggested consolidation (illustrative)
// e.g. in a small shared gc side-table module pub(crate) fn rekey_metadata_address_set( set: &RefCell<HashSet<usize>>, visitor: &mut crate::gc::RuntimeRootVisitor<'_>, ) { let mut arrays = set.borrow_mut(); let mut moved = Vec::new(); for old_addr in arrays.iter().copied() { let mut new_addr = old_addr; if visitor.visit_metadata_usize_slot(&mut new_addr) { moved.push((old_addr, new_addr)); } } for (old_addr, new_addr) in moved { arrays.remove(&old_addr); arrays.insert(new_addr); } } pub(crate) fn prune_dead_address_set( set: &RefCell<HashSet<usize>>, is_dead_owner: &dyn Fn(usize) -> bool, ) { set.borrow_mut().retain(|owner| !is_dead_owner(*owner)); }-pub(crate) fn scan_map_iterator_array_roots_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) { - MAP_ITERATOR_ARRAYS.with(|r| { - let mut arrays = r.borrow_mut(); - let mut moved = Vec::new(); - for old_addr in arrays.iter().copied() { - let mut new_addr = old_addr; - if visitor.visit_metadata_usize_slot(&mut new_addr) { - moved.push((old_addr, new_addr)); - } - } - for (old_addr, new_addr) in moved { - arrays.remove(&old_addr); - arrays.insert(new_addr); - } - }); -} - -pub(crate) fn prune_dead_map_iterator_array_owners(is_dead_owner: &dyn Fn(usize) -> bool) { - MAP_ITERATOR_ARRAYS.with(|r| { - r.borrow_mut().retain(|owner| !is_dead_owner(*owner)); - }); -} +pub(crate) fn scan_map_iterator_array_roots_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) { + MAP_ITERATOR_ARRAYS.with(|r| crate::gc::rekey_metadata_address_set(r, visitor)); +} + +pub(crate) fn prune_dead_map_iterator_array_owners(is_dead_owner: &dyn Fn(usize) -> bool) { + MAP_ITERATOR_ARRAYS.with(|r| crate::gc::prune_dead_address_set(r, is_dead_owner)); +}🤖 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/map.rs` around lines 33 - 56, Consolidate the duplicated GC side-table logic used by scan_map_iterator_array_roots_mut and prune_dead_map_iterator_array_owners with the corresponding set.rs functions. Add shared helpers for rekeying metadata address sets and pruning dead owners, then update both map and set callers to use them while preserving the existing visitor and retention behavior.
🤖 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 `@plans/002-fix-array-side-table-lifecycles.md`:
- Around line 21-24: Update the Plan 002 dependency wording in the plan document
to explicitly state that Plan 001 is recommended but not required, and that Plan
002 may proceed independently without Plan 001. Align the wording with the
dependency guidance in plans/README.md without changing the lifecycle scope.
In `@plans/README.md`:
- Around line 21-24: Update the dependency statement for Plans 002 and 003 in
the plan ordering guidance to say they may run independently, while recommending
Plan 001 be completed first. Keep the existing ordering rationale and other plan
relationships unchanged.
---
Nitpick comments:
In `@crates/perry-runtime/src/map.rs`:
- Around line 33-56: Consolidate the duplicated GC side-table logic used by
scan_map_iterator_array_roots_mut and prune_dead_map_iterator_array_owners with
the corresponding set.rs functions. Add shared helpers for rekeying metadata
address sets and pruning dead owners, then update both map and set callers to
use them while preserving the existing visitor and retention behavior.
🪄 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: f744e746-87ad-4532-bbdc-b9cbdea04c6b
📒 Files selected for processing (9)
crates/perry-runtime/src/array/header.rscrates/perry-runtime/src/array/mod.rscrates/perry-runtime/src/gc/dead_owner.rscrates/perry-runtime/src/gc/mod.rscrates/perry-runtime/src/gc/tests/dead_owner_side_tables.rscrates/perry-runtime/src/map.rscrates/perry-runtime/src/set.rsplans/002-fix-array-side-table-lifecycles.mdplans/README.md
Comment on lines
+21
to
+24
| 3. remove the stale key before the old address can be reused. | ||
|
|
||
| The named-property table rewrites live owners and scans stored values, but does not prune dead owners. The iterator marker sets are written by production paths and currently have neither rewrite nor death hooks. | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify the Plan 002 dependency wording.
This plan says Plan 001 is recommended but not required, while plans/README.md says Plans 002 and 003 can run independently “after 001,” which implies 001 is a prerequisite. State explicitly that Plan 001 is recommended, but Plan 002 may proceed without it.
🤖 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 `@plans/002-fix-array-side-table-lifecycles.md` around lines 21 - 24, Update
the Plan 002 dependency wording in the plan document to explicitly state that
Plan 001 is recommended but not required, and that Plan 002 may proceed
independently without Plan 001. Align the wording with the dependency guidance
in plans/README.md without changing the lifecycle scope.
Comment on lines
+21
to
+24
| - 001 is first because the following plans change unsafe GC ownership, traversal, and heap-layout boundaries; deterministic ASan coverage lowers their regression risk. | ||
| - 002 and 003 can run independently after 001. | ||
| - 004 depends on 001 and is isolated from 002/003, but should land before performance work because it closes a resource-lifetime gap. | ||
| - 005 and 006 are separate optimizations. Execute 005 first because it reuses an already-proven filtered traversal and has the lower soundness risk. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify that Plan 001 is not a hard prerequisite.
The status table and Plan 002 state that Plan 001 is only recommended, but this sentence says Plans 002 and 003 run “after 001.” Change it to clarify that they may run independently, with Plan 001 recommended first.
🤖 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 `@plans/README.md` around lines 21 - 24, Update the dependency statement for
Plans 002 and 003 in the plan ordering guidance to say they may run
independently, while recommending Plan 001 be completed first. Keep the existing
ordering rationale and other plan relationships unchanged.
TheHypnoo
force-pushed
the
fix/gc-array-side-table-lifecycle
branch
from
July 12, 2026 21:48
615b6e2 to
796ff3d
Compare
TheHypnoo
force-pushed
the
fix/gc-array-side-table-lifecycle
branch
from
July 12, 2026 21:52
796ff3d to
2e40a45
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
2 participants
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
Fix raw-address array side tables so live metadata follows GC movement and dead metadata is removed before allocator address reuse. This prevents leaked expando values and stale Map/Set iterator brands.
Changes
Related issue
n/a
Test plan
cargo build --releaseclean — not run; no build-system changescargo test --workspace --exclude perry-ui-ios --exclude perry-ui-tvos --exclude perry-ui-watchos --exclude perry-ui-gtk4 --exclude perry-ui-android --exclude perry-ui-windowspasses — not run; focused runtime and root-contract suites used instead#[test]regressions in the affected runtime crateValidation performed:
RUST_TEST_THREADS=1 cargo test --lib -p perry-runtime array_named— 6 passedRUST_TEST_THREADS=1 cargo test --lib -p perry-runtime dead_owner_side_tables— 18 passedRUST_TEST_THREADS=1 cargo test --lib -p perry-runtime— 1270 passed, 1 ignoredpython3 scripts/gc_store_site_inventory.py --self-test— passedpython3 scripts/gc_store_site_inventory.py— passedcargo fmt --all -- --check— passedcargo test -p perry-codegen --test shadow_slot_hygiene— 9 passedScreenshots / output
Not applicable; there are no user-interface changes.
Checklist
feat:/fix:/docs:/chore:prefix conventionCONTRIBUTING.mdand agree to the Code of ConductSummary by CodeRabbit