gc: make the below-HEAP_MIN growth-stub skip loud, and split from-space offenders by snapshot coverage - #7043
Merged
Merged
Conversation
…ce offenders by snapshot coverage Two diagnostics salvaged from the #7022 investigation, both landable on their own. 1. js_array_grow skips installing the growth forwarding stub when the array sits below the platform heap floor. That is an address-conditional silent divergence: behaviour depends on where the allocator happened to place memory, with no signal. Now a debug_assert plus a once-per-process stderr line. This has already cost debugging time. An experiment replacing arena blocks with mmap'd guard-paged blocks used a NULL hint, landed below macOS's 2 TB floor, silently disabled every growth stub, and came back falsely clean -- it was only caught by re-running with a high MAP_FIXED hint. 2. The from-space scan (#7041) now splits offenders by whether their page was in this cycle's dirty snapshot: not_in_snapshot -- the in-cycle remembered-set scan never looked at the page in_snapshot -- the scan looked and still did not rewrite the slot That split is what established #7022's failure as in-cycle, rather than an edge recorded and later dropped. Refs #7022, #7041.
📝 WalkthroughWalkthroughAdds runtime diagnostics for below- ChangesGC diagnostics
Possibly related PRs
Suggested reviewers: 🚥 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-runtime/src/gc/fromspace_scan.rs`:
- Around line 317-319: The from-space scan coverage must not be initialized from
dirty_old_pages, because the scan also visits surviving malloc-tracked objects
and other slots outside dirty old-page headers. Update the coverage tracking in
fromspace_scan.rs around SNAPSHOT_PAGES and the actual scan path to record every
inspected slot or narrow not_in_snapshot wording so it does not claim unexamined
coverage; update changelog.d/7043-heap-min-diagnostic.md lines 11-15 to match
the corrected semantics.
🪄 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: ab8a7429-e1e8-4707-b532-d1f890f8f138
📒 Files selected for processing (4)
changelog.d/7043-heap-min-diagnostic.mdcrates/perry-runtime/src/array/push_pop.rscrates/perry-runtime/src/gc/copying.rscrates/perry-runtime/src/gc/fromspace_scan.rs
Comment on lines
+317
to
+319
| SNAPSHOT_PAGES.with(|c| { | ||
| *c.borrow_mut() = snapshot.dirty_old_pages.iter().copied().collect(); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -A120 -B20 \
'scan_remembered_dirty_slots_copying|external_dirty_entries|dirty_pages|fallback_headers' \
crates/perry-runtime/src/gcRepository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file sizes =="
wc -l crates/perry-runtime/src/gc/fromspace_scan.rs
wc -l changelog.d/7043-heap-min-diagnostic.md
echo "== fromspace_scan relevant sections =="
ast-grep outline crates/perry-runtime/src/gc/fromspace_scan.rs --match scan_remembered_dirty_slots_copying --view expanded || true
rg -n -A35 -B35 'scan_remembered_dirty_slots_copying|SNAPSHOT_PAGES|RememberedDirtySnapshot|dirty_old_pages|dirty_pages|external_dirty_entries' crates/perry-runtime/src/gc/fromspace_scan.rs
echo "== snapshot structs and construction in gc =="
rg -n -A80 -B30 'struct RememberedDirtySnapshot|create.*remembered|remembered_dirty_snapshot|dirty_old_pages:' crates/perry-runtime/src/gcRepository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== fromspace_scan scan_heap_for_fromspace_refs area =="
sed -n '200,280p' crates/perry-runtime/src/gc/fromspace_scan.rs
echo "== scanner implementation around scan_remembered_dirty_slots_copying =="
rg -n -A160 -B30 'fn scan_remembered_dirty_slots_copying|scan_header|external_dirty_entries|fallback_headers|dirty_pages\.contains' crates/perry-runtime/src/gc/copying.rs
echo "== barrier scan header implementation =="
rg -n -A120 -B30 'DirtyHeaderSlotScan::step|fn next_dirty_header_addr|dirty_pages\.contains|old_object_page_overlaps|object_pages|scan.*header' crates/perry-runtime/src/gc/barrier.rsRepository: PerryTS/perry
Length of output: 22281
Do not use dirty_old_pages as from-space scan coverage.
dirty_old_pages only covers old-page dirty headers; the from-space scan walks every surviving slot in the heap, including malloc tracked objects, so slots whose owner is not on a dirty page can still be classified as not inspected from snapshot coverage. Build coverage from the actual scan path, or narrow this wording so not_in_snapshot does not claim the slot was never examined.
📍 Affects 2 files
crates/perry-runtime/src/gc/fromspace_scan.rs#L317-L319(this comment)changelog.d/7043-heap-min-diagnostic.md#L11-L15
🤖 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/fromspace_scan.rs` around lines 317 - 319, The
from-space scan coverage must not be initialized from dirty_old_pages, because
the scan also visits surviving malloc-tracked objects and other slots outside
dirty old-page headers. Update the coverage tracking in fromspace_scan.rs around
SNAPSHOT_PAGES and the actual scan path to record every inspected slot or narrow
not_in_snapshot wording so it does not claim unexamined coverage; update
changelog.d/7043-heap-min-diagnostic.md lines 11-15 to match the corrected
semantics.
This was referenced Jul 31, 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.
Two diagnostics salvaged from the #7022 investigation. Neither is a fix for #7022 — both are landable on their own, and the second is actively in use by the ongoing work.
Depends on #7041 (merged).
1. The below-
HEAP_MINgrowth-stub skip is no longer silentjs_array_growinstalls a forwarding stub on the old array so stale pre-grow references keep resolving (#233's mechanism). It skips that when the array sits below the platform heap floor — an address-conditional silent divergence: behaviour depends on where the allocator happened to place memory, and nothing says so.It now fires a
debug_assertand a once-per-process line on stderr (stderr specifically, so it cannot perturb a stdout parity comparison).This has already cost real time. During #7022 an experiment replaced arena blocks with
mmap'd, guard-paged blocks to catch stale accesses.mmapwith a NULL hint lands well below macOS's 2 TB floor, so this branch silently disabled every growth stub — and the experiment came back falsely clean. It was caught only by re-running with a highMAP_FIXEDhint. The next person gets a signal instead.The
debug_assertdeliberately fires on every occurrence while the stderr line is once-per-process: hard failure in debug, one-line warning in release. Reviewers may want to weigh in on whether the assert is too aggressive for a condition an unusual-but-legitimate allocator could reach.2. From-space offenders split by snapshot coverage
#7041's scan reports which live objects still hold from-space pointers after the rewrite pass. It now also says why each was missed:
not_in_snapshotin_snapshotThose are different defects with different fixes, and the split is what established #7022's failure as in-cycle rather than an edge recorded and later dropped. Without it,
lost_dirtycounts alone were consistent with both stories.Provenance, stated plainly
Both changes were written by an agent investigating #7022 that stalled before committing them — they existed only as uncommitted edits in a worktree on a remote box. I transferred the diff, applied it to current
main(clean apply), and am landing it unchanged apart from the changelog fragment. I did not write the code and have not independently re-derived the#233reasoning behind the stub; the diagnostic is additive and cannot change behaviour except by printing, which bounds the risk.Validation
cargo check -p perry-runtimeclean;cargo fmt --all -- --checkclean. No behavioural change beyond diagnostics — the only non-diagnostic edit is threading the existing&snapshotintorun_fromspace_scan, which is off by default behindPERRY_GC_FROMSPACE_SCAN.Not run: the matrix. This adds no code to any default path, and #7041's own tests still pass.
Summary by CodeRabbit
Bug Fixes
Documentation