Fix GC Map and Set teardown leaks - #6387
Conversation
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughMap and Set side-allocation registries now own buffer metadata through growth, relocation, finalization, and thread teardown. Runtime exit paths expose and invoke cleanup, executable codegen places it at process exit, and tests validate ownership transfer and exactly-once release. ChangesGC side-allocation teardown
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ExecutableEntry
participant RuntimeGC
participant MapRegistry
participant SetRegistry
ExecutableEntry->>RuntimeGC: js_gc_release_current_thread_collection_side_allocations()
RuntimeGC->>MapRegistry: release_current_thread_map_side_allocations()
RuntimeGC->>SetRegistry: release_current_thread_set_side_allocations()
RuntimeGC-->>ExecutableEntry: return after cleanup
Possibly related PRs
🚥 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.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/perry-runtime/src/map.rs (1)
644-659: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBulk registry clear on thread teardown skips GC external-side-byte accounting. Both
release_current_thread_map_side_allocationsandrelease_current_thread_set_side_allocationsfree their side buffers viaRefCell::clear()'s implicitDrop, but neither callscrate::gc::gc_note_external_side_freethe waydrop_map_index/finalize_map_side_allocation_for_gc(map.rs) anddrop_set_index/finalize_set_side_allocation_for_gc/test_clear_set_roots(set.rs) do.GC_EXTERNAL_SIDE_LIVE_BYTESis left stale for whatever remains of the thread's lifetime after this call. Today this is harmless — the only caller,js_gc_release_current_thread_collection_side_allocations, runs immediately beforelibc::_exit/std::process::exit— but it's a latent foot-gun if this helper is ever invoked without an immediate thread/process exit.
crates/perry-runtime/src/map.rs#L644-L659: inrelease_current_thread_map_side_allocations, drainMAP_REGISTRYand callcrate::gc::gc_note_external_side_free(allocation.byte_len())per entry before dropping, mirroringtest_clear_set_roots's pattern.crates/perry-runtime/src/set.rs#L460-L473: apply the same drain-and-notify fix torelease_current_thread_set_side_allocations.🤖 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 644 - 659, Update release_current_thread_map_side_allocations in crates/perry-runtime/src/map.rs:644-659 to drain MAP_REGISTRY, call crate::gc::gc_note_external_side_free with each allocation’s byte_len(), then drop the drained allocations before clearing the indexes. Apply the same drain-and-notify behavior to release_current_thread_set_side_allocations in crates/perry-runtime/src/set.rs:460-473, preserving the existing index cleanup.crates/perry-runtime/src/set.rs (1)
259-358: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
test_clear_set_rootscorrectly drains-and-accounts; contrast withrelease_current_thread_set_side_allocationsbelow.Good pattern here: explicit
drain()+gc_note_external_side_freeper allocation before drop, unlike the bulkclear()at Line 470. See consolidated comment for the cross-file inconsistency.🤖 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/set.rs` around lines 259 - 358, Update release_current_thread_set_side_allocations to drain each allocation from the registry, call gc_note_external_side_free with its byte length, and then drop it, matching the per-allocation cleanup pattern in test_clear_set_roots instead of bulk-clearing without accounting.
🤖 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/process/env_misc.rs`:
- Around line 120-127: Update exit_after_current_thread_collection_teardown to
use the same platform-specific no-atexit termination helper as js_process_exit
instead of std::process::exit, while preserving release of current-thread
collection side allocations before termination. Factor the hard-exit logic into
a shared helper if it is currently embedded in js_process_exit, then call that
helper from both paths.
In `@plans/008-fix-gc-map-set-teardown-leak.md`:
- Around line 257-266: Update the focused-runner expectations in this plan,
including the corresponding done criteria, to require the current 462-test
inventory after adding the three teardown tests. Keep the instruction to inspect
inventory drift and prohibit hard-coded exclusions; only document an alternative
if the new tests are intentionally outside Plan 001’s runner.
---
Nitpick comments:
In `@crates/perry-runtime/src/map.rs`:
- Around line 644-659: Update release_current_thread_map_side_allocations in
crates/perry-runtime/src/map.rs:644-659 to drain MAP_REGISTRY, call
crate::gc::gc_note_external_side_free with each allocation’s byte_len(), then
drop the drained allocations before clearing the indexes. Apply the same
drain-and-notify behavior to release_current_thread_set_side_allocations in
crates/perry-runtime/src/set.rs:460-473, preserving the existing index cleanup.
In `@crates/perry-runtime/src/set.rs`:
- Around line 259-358: Update release_current_thread_set_side_allocations to
drain each allocation from the registry, call gc_note_external_side_free with
its byte length, and then drop it, matching the per-allocation cleanup pattern
in test_clear_set_roots instead of bulk-clearing without accounting.
🪄 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: 187a247a-ba86-44be-9ff3-673ebeff2329
📒 Files selected for processing (13)
crates/perry-codegen/src/codegen/entry.rscrates/perry-codegen/src/codegen/entry/tests.rscrates/perry-codegen/src/runtime_decls/strings.rscrates/perry-runtime/src/gc/mod.rscrates/perry-runtime/src/gc/tests/copying.rscrates/perry-runtime/src/gc/tests/mod.rscrates/perry-runtime/src/gc/tests/teardown.rscrates/perry-runtime/src/map.rscrates/perry-runtime/src/process.rscrates/perry-runtime/src/process/env_misc.rscrates/perry-runtime/src/promise/rejection.rscrates/perry-runtime/src/set.rsplans/008-fix-gc-map-set-teardown-leak.md
08532c3 to
cbcda22
Compare
2 participants
Summary
Release registry-owned Map and Set side buffers at thread and executable teardown so arena destruction cannot orphan external allocations.
Changes
atexitafter GC teardown, matchingprocess.exit()safety.Related issue
n/a
Test plan
Before the fix, the exact local Linux LeakSanitizer reproduction passed its Rust test but exited with a 6,144-byte leak across 128 allocations. After the fix and the review adjustments, the same command exits successfully with leak detection enabled and no sanitizer report.
cargo build --releaseclean (via the parity runner)cargo 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-windowspassestest-files/or a#[test]in the affected cratedocs/src/(not applicable; internal teardown contract only)-p perry-ui-<backend>locally on that platform (not applicable)Additional verification:
RUST_TEST_THREADS=1 cargo test --lib -p perry-runtime -- --test-threads=1— 1,312 passed, 1 ignoredcargo test -p perry-codegen --lib codegen::entry::tests— 2 passedcargo test -p perry-codegen --test shadow_slot_hygiene— 9 passedcargo test -p perry --test issue_6185_thread_worker_closure_guards— 12 passedpython3 scripts/gc_store_site_inventory.py --self-testpython3 scripts/gc_store_site_inventory.pypython3 scripts/addr_class_inventory.py --self-testpython3 scripts/addr_class_inventory.pycargo fmt --all -- --checkgit diff --check./run_parity_tests.sh --filter test_gap_fs_fd_2749— passed four consecutive local runs after one CI mismatchScreenshots / output
Before: LeakSanitizer reported 4,096 bytes from 64 Map allocations and 2,048 bytes from 64 Set allocations.
After: the exact test exits 0 with
detect_leaks=1and no AddressSanitizer or LeakSanitizer findings.Checklist
feat:/fix:/docs:/chore:prefix convention used in the logSummary by CodeRabbit
Map/Setside-allocation memory at thread exit and at the real process-exit boundary.Map/Setside allocations stay correctly tracked through garbage-collection relocation and capacity growth.