Stop the O(1) page reclaim from freeing slots the per-slot walk keeps - #5474
Merged
Conversation
Follow-up to #5471, which flagged kept objects referencing reclaimed memory on the issue-5425 workload and left the cause open. The O(1) whole-page reclaim tests gcLastMarkedEpoch != V, but the per-slot walk it claims to reproduce ("byte-identical outcome") frees on m < V-1 -- so a slot marked at V-1 SURVIVES the walk while the shortcut drops the entire page holding it. Measured on LargeArrayLoad, the final issue-5425 shape: 26,924 slots freed a full cycle early in one run. The other drivers report zero, which fits: it needs a large retained survivor set sharing pages with garbage, which is exactly what that workload builds. That is what produced the dangling pairs. The legacy sweep ages on the same m < V-1 rule, so a matured Hashtable.Entry kept at V-1 was left pointing at a page-resident byte[] payload the shortcut had already reclaimed -- the reporter's dictionary shape precisely. Test gcLastMarkedEpoch < V-1 instead. Both bounds are then exact and neither implies the other: gcLastMarkedEpoch covers slots marked since the last full walk, gcGraceEpoch covers slots the sweep itself promoted out of grace. earlyFreed goes 26,924 -> 0 across the suite, and it is free: LoadLoop and StormAB wall time and RSS unchanged, LargeArrayLoad still 5 cycles, peak RSS 93.1 vs 92.6 MB. On the resurrection half of that report, the measurement says the hazard is real but essentially never fires: across all nine drivers, 1 resurrection total and 0 of them holding a dangling reference. For the conservative scan to revive an object a stale stack word must point at it, and such a word generally keeps marking it EVERY cycle -- retention, not resurrection. The audit stays in as a gate so that stops being an assumption. Three things keep this honest: - resurrected / resurrectedDangling / earlyFreed are reported at exit, and run-gc-verify.sh fails on a nonzero earlyFreed or resurrectedDangling. - CN1_GC_FAULT=earlyfree restores the old bound, and a second self-test requires the gate to reject it (26,924 slots), so the new check cannot go inert the way an unexercised assertion does. - CN1_GC_VERIFY_AGING is documented as PERTURBING: it doubles the post-sweep walk and the extra GC-thread time moves page ageing by orders of magnitude (early-freed 0 vs 205,958 from one binary). The 30k-60k figures quoted in #5471 came from that mode and overstated the problem; every number here is from the default configuration. Validation: gauntlet GREEN in both stop modes, run-gc-verify GREEN over nine drivers with both self-tests firing, GcHeapIntegrityIntegrationTest green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a ParparVM GC correctness issue where the O(1) whole-page reclaim could free BiBOP page slots that the per-slot sweep would have kept, which could leave surviving objects holding references into reclaimed memory. The PR also strengthens the GC-verify harness with counters/audits and fault-injection to keep the new invariant check exercised.
Changes:
- Adjust BiBOP fast-sweep eligibility to match the per-slot keep/free rule (
gcLastMarkedEpoch < V-1), and add anearlyfreefault-injection mode for A/B validation. - Add
earlyFreedand resurrection auditing/counters to the GC-verify summary output to detect (and gate on) the “freed a cycle early” condition and resurrected-dangling hazards. - Update
run-gc-verify.shandvm/benchmarks/README.mdto parse/report the new counters and enforce a second self-test for the early-free regression.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
vm/ByteCodeTranslator/src/cn1_globals.m |
Fixes the O(1) page reclaim bound, adds early-free tracking, resurrection auditing, and summary reporting for GC verification. |
vm/benchmarks/run-gc-verify.sh |
Gates driver runs on earlyFreed/resurrectedDangling, and adds a second self-test for the early-free fault injection. |
vm/benchmarks/README.md |
Documents new diagnostics and clarifies that CN1_GC_VERIFY_AGING perturbs collector behavior/counters. |
Comments suppressed due to low confidence (1)
vm/ByteCodeTranslator/src/cn1_globals.m:3078
- This comment says the issue-5425 workload freed "232,882 slots" early, but the repo documentation and self-test mention 26,924 early-freed slots for LargeArrayLoad. Aligning the number (or avoiding a hard-coded count) will prevent confusion when comparing runs.
// cycle earlier than the rule it claims to reproduce. Measured on the
// issue-5425 workload: 232,882 slots freed early in one run.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The bullet list above the O(1) page decision still documented
gcLastMarkedEpoch != V -- the exact bound this PR replaces -- so the file
stated both the old rule and the new one a few lines apart, with the wrong
one first. The bullet now carries the real criterion and says why != V is
insufficient; the paragraph below no longer restates it.
getenv("CN1_GC_DEBUG_EARLY") also ran once per early-freed slot, and the
earlyfree self-test drives 26,924 of them through that loop. Cached in a
static, like the other QA env reads in this file.
Gate re-run: clean, with both self-tests firing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Cloudflare Preview
|
The resurrection audit runs at the end of the mark but drives the same per-field reporter as the post-sweep gate, so anything it found announced itself as happening "after sweep" -- pointing a reader at the wrong phase of the collector while they try to reconstruct what freed the memory. Carry the calling context in the report instead of suppressing the per-field detail, which is the part worth having: the holder and victim classes and the mark site are what map a resurrected object's dangling field back to source. The audit also now says which resolver snapshot it classifies against (the mark's, correct there because the memory it looks for was reclaimed by earlier cycles). Post-sweep reports still read "after sweep"; verified against the nograce self-test, and the gate stays green with both self-tests firing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
✅ ByteCodeTranslator Quality ReportTest & Coverage
Benchmark Results
Static Analysis
Generated automatically by the PR CI workflow. |
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Collaborator
Author
|
Compared 146 screenshots: 146 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 147 screenshots: 147 matched. |
Collaborator
Author
|
Compared 147 screenshots: 147 matched. |
Collaborator
Author
|
Compared 146 screenshots: 146 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 181 screenshots: 181 matched. |
Collaborator
Author
|
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 217 screenshots: 217 matched. |
Collaborator
Author
|
Compared 143 screenshots: 143 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 144 screenshots: 144 matched. |
Collaborator
Author
|
Compared 146 screenshots: 146 matched. Benchmark ResultsDetailed Performance Metrics
|
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
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
Closes the finding #5471 left open: kept objects referencing reclaimed memory on the issue-5425 workload. Chasing it gave a different answer than the one that PR guessed at, so the numbers below matter more than the hypothesis did.
What was actually wrong
The O(1) whole-page reclaim tests
gcLastMarkedEpoch != V. The per-slot walk it claims to reproduce -- its comment says "Byte-identical outcome WITHOUT touching a single slot" -- frees onm < V-1. A slot marked atV-1therefore survives the walk, while the shortcut drops the entire page holding it.Measured on
LargeArrayLoad, the final issue-5425 shape: 26,924 slots freed a full cycle early in one run. Every other driver reports zero, which fits -- it needs a large retained survivor set sharing pages with garbage, which is exactly what that workload builds.That is the source of the dangling pairs. The legacy sweep ages on the same
m < V-1rule, so a maturedHashtable.Entrykept atV-1was left pointing at a page-residentbyte[]payload the shortcut had already reclaimed -- the reporter's dictionary shape precisely.Fix: test
gcLastMarkedEpoch < V - 1. Both bounds are then exact, and neither implies the other:gcLastMarkedEpochcovers slots marked since the last full walk,gcGraceEpochcovers slots the sweep itself promoted out of grace.Free, measured against master: LoadLoop 0.16s both, StormAB 0.59 vs 0.58s, LargeArrayLoad 5 collection cycles both, peak RSS 93.1 vs 92.6 MB.
On the resurrection hypothesis
#5471 speculated that the conservative native-stack scan resurrecting unreachable objects would turn those dangling pairs into corruption. Measured directly with a new pre-sweep audit -- record every object marked live again after ageing past the keep threshold, then check its fields before the sweep acts:
Across all nine drivers: 1 resurrection total, 0 holding a dangling reference.
The hazard is real but essentially never fires, for a structural reason: for the scan to revive an object a stale stack word must point at it, and such a word generally keeps marking it every cycle. That is retention, not resurrection -- the object never ages out to be revived. The audit stays in as a gate so this stops being an assumption.
Keeping it honest
earlyFreed,resurrectedandresurrectedDanglingare reported in the exit summary;run-gc-verify.shfails on a nonzeroearlyFreedorresurrectedDangling.CN1_GC_FAULT=earlyfreerestores the old bound, and a second self-test requires the gate to reject it (26,924 slots). Neither new check can go inert the way an unexercised assertion does.CN1_GC_DEBUG_EARLY=1dumps the offending pages' ageing bounds, since which bound went stale is the whole diagnosis.Correction to #5471
That PR reported ~30k-60k dangling references per cycle from
CN1_GC_VERIFY_AGING=1. That mode perturbs the collector: it roughly doubles the post-sweep walk, and the extra GC-thread time changes page ageing by orders of magnitude -- early-freed slots read 0 or 205,958 from the same binary depending only on that flag. Those figures overstated the problem. The README now says so, and every number in this PR comes from the default configuration.Validation
run-gauntlet.sh, both stop modesrun-gc-verify.sh, 9 drivers + both self-testsGcHeapIntegrityIntegrationTest🤖 Generated with Claude Code