Fix GC freeing live objects referenced by untraced fresh BiBOP objects - #5442
Merged
Conversation
The fresh-page-stack grace scheme introduced in #5436 queued a page onto an alternating per-epoch stack on its FIRST allocation in a GC epoch and consumed both stacks once, mid-mark. The queue-once-per-epoch dedup left a wide uncovered window: every allocation into an already-consumed page for the REST of that epoch -- the remainder of the mark plus the entire unbarriered inter-cycle gap -- was skipped, and if the page received no next-epoch allocation before the next grace pass, its fresh (gcMark==-1) slots were never grace-traced. The sweep then freed any object reachable only through such an untraced fresh object while the fresh object itself survived via grace: a dangling reference inside a surviving object. With compact strings inlining the byte payload into the String's BiBOP slot, recycling those slots rewrites word bytes in place -- the corrupted dictionary entries and impossible NPE reported in issue 5425. Replace the queue with a full-registry walk pruned by the existing gcAllocedSinceSweep flag. The pruning invariant is exact and race-free: a mark==-1 slot can only exist on a page allocated into since that page's last sweep (the sweep converts every -1 it sees), every allocation path already sets the flag, pre-mark stores are published by the mark-start thread sync, and only the sweep -- which never touches an owned page -- clears it. Flag-FALSE pages (the retained-survivor bulk on exactly the workloads #5436 targets) are skipped without touching their slots, so the pause win of #5436 is preserved while the whole fresh-page queueing machinery (two page-header fields, the epoch mirror check and queue call in three allocation paths) is deleted from the hot path. Add a QA-only grace-completeness gate (-DCN1_GRACE_AUDIT): snapshot each page's bump cursor at mark start and, right before the sweep, full-walk the registry tracing any pre-snapshot slot still fresh. It reports missedFresh (fresh slots the grace pass never visited) and doomedChildren (objects that became marked ONLY through them -- each one would otherwise be swept while still referenced; any nonzero value is a collector bug). The new GraceAudit driver allocates dropped fresh nodes holding sole references to older objects WHILE the concurrent mark runs (System.gc is asynchronous), then goes quiet a cycle: against the fresh-page stacks it reports 100-370 missed / 100-250 doomed per cycle (12,178 doomed in one run); with this fix doomedChildren is zero across the suite. StormAB and LoadLoop are the matching perf A/B drivers: wall time and RSS are unchanged vs the pre-fix tree (storm 5-8x faster than pre-#5436, repeated dictionary loads flat), run-bibop-adaptive.sh stays green (adaptive 0.94x time / 0.62x RSS vs legacy), and the full gauntlet passes byte-identical in both thread-stop modes. Fixes the corruption regression reported in #5425. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
✅ ByteCodeTranslator Quality ReportTest & Coverage
Benchmark Results
Static Analysis
Generated automatically by the PR CI workflow. |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a BiBOP concurrent-GC heap corruption regression (issue #5425) by replacing the “fresh-page stack” grace scan with a full registry walk that is pruned by the existing gcAllocedSinceSweep page flag, and adds QA-only benchmark/audit drivers to gate grace-pass completeness going forward.
Changes:
- Replace per-epoch fresh-page queueing with a full
bibopAllPagesregistry walk (skipping pages not allocated into since last sweep). - Remove fresh-page queue bookkeeping from allocation paths and page headers.
- Add QA-only grace completeness audit (
CN1_GRACE_AUDIT) plus new benchmark drivers (GraceAudit,StormAB,LoadLoop) and document them.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| vm/ByteCodeTranslator/src/cn1_globals.m | Switches grace scan to registry walk pruned by gcAllocedSinceSweep; removes fresh-page stack; adds CN1_GRACE_AUDIT pre-sweep verifier. |
| vm/ByteCodeTranslator/src/cn1_globals.h | Removes fresh-page stack fields/prototypes and updates allocation-path comments around the new pruning flag. |
| vm/benchmarks/src/com/bench/StormAB.java | Adds sustained allocation storm A/B driver for pacing/perf comparisons. |
| vm/benchmarks/src/com/bench/LoadLoop.java | Adds repeated dictionary build/drop workload to detect per-round degradation. |
| vm/benchmarks/src/com/bench/GraceAudit.java | Adds regression-shaped driver intended to break incomplete grace schemes when audit is enabled. |
| vm/benchmarks/README.md | Documents the new grace audit flag and new benchmark drivers/gates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1306
to
+1310
| #ifndef CN1_BIBOP_NO_FASTSWEEP | ||
| if(gp->gcAllocedSinceSweep == JAVA_FALSE) { | ||
| gp = atomic_load_explicit(&gp->nextAll, memory_order_acquire); | ||
| continue; | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
Valid on the formal C11 point, addressed in bc7b403: the concurrent pair (mutator set-on-allocation / grace-pass read) now uses relaxed __atomic ops -- identical machine code (plain str/ldr) to the previous access, no hot-path cost. Sweep and format keep plain access because they only touch retired (owner==0) or pooled pages no mutator holds, so those accesses never race.
The "stale FALSE skips a page with fresh slots" half does not hold, and no ordering stronger than relaxed is needed:
- Pre-mark allocations are ordered ahead of the grace pass by the mark-start thread pause: the mark waits for every lightweight thread to park (
while(t->threadActive)) and migrates every thread's pending allocations underlockCriticalSection()/lockThreadHeapMutex()before the drain and grace pass run. Any flag store made before the mark began is visible by then. - A store the grace pass can still miss is therefore by definition a during-mark allocation. Its links are covered by the armed SATB barrier this cycle, and the flag store is never lost -- only the sweep clears the flag, and the sweep never runs concurrently with mutators touching the page -- so the page is re-observed dirty and slot-scanned next cycle. The object itself survives this cycle's sweep via the mark==-1 grace.
This is also validated empirically: the committed -DCN1_GRACE_AUDIT gate full-walks the registry pre-sweep ignoring the flag and reports any fresh object the grace pass failed to trace whose children would be swept while referenced (doomedChildren). It reports zero across GraceAudit (which allocates fresh objects concurrently with the mark specifically to attack this window), the torture suite, and the gauntlet in both thread-stop modes.
Comment on lines
+2007
to
+2016
| #ifdef CN1_GRACE_AUDIT | ||
| // QA builds only: snapshot every page's cursor at mark start. Slots below the | ||
| // snapshot existed before the grace pass ran, so a complete grace pass must | ||
| // have traced every one of them that is still fresh at pre-sweep time. | ||
| { | ||
| CN1BibopPage* ap = atomic_load_explicit(&bibopAllPages, memory_order_acquire); | ||
| while(ap != 0) { | ||
| ap->gcAuditSnapshot = atomic_load_explicit(&ap->bumpIndex, memory_order_acquire); | ||
| ap = atomic_load_explicit(&ap->nextAll, memory_order_acquire); | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
The under-snapshot is intentional and is now documented at the snapshot site (bc7b403). Key point: trailing the true cursor makes the audit conservative in the right direction -- it can only shrink the audited set, never produce a false positive.
Slots racing mark start are during-mark allocations, and those are exactly the class the grace guarantee does not (and need not) cover in the current cycle: their reference stores happen under the armed SATB barrier, and the sticky gcAllocedSinceSweep flag re-traces them next cycle. The population the audit must cover -- and the one the issue-5425 bug dropped -- is the clearly-pre-mark slots, all of which sit below the snapshot: each per-page bumpIndex is acquire-loaded, so every value is a genuinely published cursor that includes all allocations from before the mark began.
Snapshotting later (after the thread pause) would widen coverage by only those boundary slots while turning benign mid-mark allocations into missedFresh noise, weakening the gate's signal. The audit found the real bug at 100-370 missed / 100-250 doomed per cycle with this placement, and reports zero doomed on the fixed collector.
Comment on lines
+47
to
+54
| // deterministic LCG so runs are comparable without java.util.Random | ||
| static long seed = 42; | ||
|
|
||
| static int next(int bound) { | ||
| seed = seed * 6364136223846793005L + 1442695040888963407L; | ||
| int v = (int) (seed >>> 33) % bound; | ||
| return v < 0 ? v + bound : v; | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
Valid -- the LCG was left over from an earlier randomized-phase version of the driver; the final driver gets its mark/allocation overlap from System.gc() being asynchronous plus fixed slice timing, so the helper went unused. Removed in bc7b403, along with the vestigial sink field.
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Contributor
Cloudflare Preview
|
Collaborator
Author
|
Compared 146 screenshots: 146 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 146 screenshots: 146 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 181 screenshots: 181 matched. |
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. |
- gcAllocedSinceSweep: the concurrent pair (mutator set on allocation / grace-pass read) now uses relaxed __atomic ops -- identical machine code to the previous plain access, but removes the formal C11 data race. Sweep/format keep plain access: they only touch retired or pooled pages no mutator holds. Comments spell out why relaxed suffices: pre-mark stores are ordered ahead of the grace pass by the mark-start thread pause, a store the pass can still miss is by definition a during-mark allocation (SATB-covered this cycle), and only the sweep clears the flag so a missed store is re-observed next cycle. - Document that the CN1_GRACE_AUDIT mark-start snapshot deliberately under-approximates: boundary slots racing mark start are during-mark allocations, the class the grace guarantee does not cover this cycle, and excluding them keeps the audit free of false positives. - GraceAudit: drop the unused LCG helper and vestigial sink field, reword stale fresh-stack phrasing to be scheme-agnostic. Revalidated: GraceAudit doomedChildren=0, run-gauntlet.sh GREEN (both stop modes), run-bibop-adaptive.sh GREEN (0.93x time / 0.60x RSS vs legacy), StormAB wall time unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
Author
|
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
vm/ByteCodeTranslator/src/cn1_globals.m:1992
- cn1BibopFormatPage() clears gcAllocedSinceSweep with a plain store, but the grace pass concurrently reads this field via __atomic_load_n() while mutators are running. Mixing atomic and non-atomic accesses to the same object is a C11 data race (UB). Use a relaxed __atomic_store_n() here to match the concurrent reader.
#ifndef CN1_BIBOP_NO_FASTSWEEP
p->gcAllocedSinceSweep = JAVA_FALSE;
p->gcNeedsReclaim = JAVA_FALSE;
vm/ByteCodeTranslator/src/cn1_globals.m:2028
- In CN1_GRACE_AUDIT builds, gcAuditSnapshot is accessed while mutators may be reformatting pooled pages concurrently. This write should use an atomic builtin to pair with the other concurrent accesses (and to allow cn1GraceAuditPreSweep() to use an atomic load), otherwise the field has a C11 data race.
ap->gcAuditSnapshot = atomic_load_explicit(&ap->bumpIndex, memory_order_acquire);
| } | ||
| } | ||
| #ifdef CN1_GRACE_AUDIT | ||
| p->gcAuditSnapshot = 0; |
Collaborator
Author
There was a problem hiding this comment.
Valid -- fixed in 1c7f68b. The distinction that matters: the new-page path (cn1BibopNewPage) formats BEFORE registry insertion, so nothing can observe it; but the acquire path (cn1BibopAcquirePage) reformats a FREE-pool page that is already in the registry, on a mutator thread, possibly mid-mark -- that one can genuinely overlap the GC's audit accesses. All three gcAuditSnapshot sites (format reset, mark-start snapshot store, pre-sweep read) now use relaxed __atomic ops.
Comment on lines
+1308
to
+1311
| // Alloc paths set it / the grace pass reads it | ||
| // via relaxed __atomic ops (concurrent pair); | ||
| // sweep/format access it plain -- they only | ||
| // touch retired/pooled pages no mutator holds |
Collaborator
Author
There was a problem hiding this comment.
Correct -- the comment lumped format in with the sweep, and format does not share the sweep's justification. cn1BibopAcquirePage reformats a registry-visible FREE-pool page on a mutator thread, so its gcAllocedSinceSweep reset can race the grace pass's atomic read. Fixed in 1c7f68b: the format reset is now a relaxed __atomic store (value-identical -- the sweep already reset the flag before pooling the page, so any interleaving reads FALSE; the atomic only removes the formal race), and the struct comment now states the precise reason the sweep's three accesses stay plain: the sweep runs on the GC thread after mark completes, program-ordered against the grace pass, on retired pages no mutator holds, with the pool-handoff mutex ordering it against the next owner's stores.
Revalidated after the change: GraceAudit doomedChildren=0, gauntlet GREEN in both thread-stop modes, run-bibop-adaptive GREEN, StormAB wall time unchanged.
cn1BibopAcquirePage reformats a FREE-pool page that is already in the page registry, on a mutator thread, possibly during a concurrent mark -- so its resets of gcAllocedSinceSweep (and gcAuditSnapshot in audit builds) can overlap the grace pass / audit reads. Convert that pair to relaxed __atomic ops: the flag store is value-identical (the sweep already reset it before pooling), so this only removes the formal race. The new-page format path is untouched by observers (it runs before registry insertion) but shares the same code, and format is cold either way. The audit snapshot field now uses relaxed atomics at all three sites (format reset, mark-start snapshot, pre-sweep read). The sweep's three plain accesses remain intentionally plain and the header comment now states the precise reason: the sweep runs on the GC thread after mark completes (program-ordered against the grace pass) on retired pages no mutator holds, and the pool-handoff mutex orders it against the next owner's stores. Revalidated: GraceAudit doomedChildren=0, gauntlet GREEN in both stop modes, run-bibop-adaptive GREEN (0.95x time / 0.61x RSS vs legacy), StormAB unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment on lines
+94
to
+114
| compiles in a QA-only pre-sweep pass that snapshots every page's bump cursor | ||
| at mark start and, right before the sweep, full-walks the registry tracing | ||
| any pre-snapshot slot that is still fresh. It reports per cycle: | ||
|
|
||
| - `missedFresh` — fresh slots the grace pass did not visit. Small counts can | ||
| be benign (a free-list slot re-allocated mid-mark, below the snapshot, after | ||
| the grace pass ran — SATB covers its links this cycle and the sticky | ||
| `gcAllocedSinceSweep` flag re-traces it next cycle). | ||
| - `doomedChildren` — objects that became marked ONLY by tracing those missed | ||
| slots. **Any nonzero value is a collector bug**: without the audit pass the | ||
| sweep would free each of them while a surviving object still references it. | ||
|
|
||
| `GraceAudit` is the driver shaped to break queue/dedup-based grace schemes: | ||
| `System.gc()` is asynchronous, so a single thread allocates dropped fresh | ||
| nodes (each holding the only reference to an older object) WHILE the mark | ||
| runs, then goes quiet across the next cycle. Gate: | ||
|
|
||
| ```bash | ||
| ./translate-and-build.sh GraceAudit target/grace-audit -DCN1_GRACE_AUDIT | ||
| ./target/grace-audit # stderr must show doomedChildren=0 on every line | ||
| ``` |
Collaborator
Author
There was a problem hiding this comment.
Correct -- the wording predated the switch to conditional printing. Fixed in a592510: the README now states that a clean cycle prints nothing (silence is success), that benign missedFresh-only lines may still appear, and that the gate is precisely "no line reports doomedChildren != 0".
The audit prints a [GRACE-AUDIT] line only when a cycle misses something; the README implied a line per cycle. State that an empty stderr is a fully clean run and the gate is that no line reports doomedChildren != 0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3 tasks
Collaborator
Author
|
Compared 217 screenshots: 217 matched. |
Collaborator
Author
|
Compared 144 screenshots: 144 matched. |
Collaborator
Author
|
Compared 143 screenshots: 143 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Contributor
|
There is evidence that this problem isn't completely solved. Once again this was one-time-only and not reproducable. |
Collaborator
Author
|
Did you build this after the release of 262? |
Contributor
|
currently building against master |
Contributor
|
I've made another build against master and it looks sick. mysterious crashes. |
shai-almog
added a commit
that referenced
this pull request
Jul 26, 2026
#5471) * Trace grace subtrees on the legacy heap, and gate heap integrity in CI The grace-subtree pass added in #5442 covers BiBOP pages only. The legacy sweep grants a fresh (gcMark == -1) legacy object exactly the same one-cycle grace -- codenameOneGCSweep promotes it to the current epoch instead of freeing it -- but nothing traced its subtree, so an older object reachable ONLY through such an object was freed while it was still referenced. That is the same defect #5442 fixed, on the other heap. Everything above CN1_BIBOP_MAX_OBJECT lands there (the retained large byte[] blocks and Hashtable bucket arrays of issue 5425), as does every allocation the adaptive survivor-heavy bypass diverts off the page heap, and every matured survivor, whose table entry is what the sweep consults. Measured with the audit half added below: ~65,800 untraced fresh legacy objects per cycle on a bypass-heavy workload, with children reachable only through them. Mirror the page walk over allObjectsInHeap. Only entries already migrated into the table can be fresh at that point -- pending allocations are not swept until the mark that migrates them, and migration happens with the owning thread paused, upstream of this pass -- so the pass is exact. Cost is one extra walk of an array the sweep already walks in full, and only fresh entries are traced: StormAB and LoadLoop wall time and RSS are unchanged and LargeArrayLoad still collects in 5 cycles. CN1_DISABLE_LEGACY_GRACE is the A/B escape hatch, mirroring CN1_DISABLE_SATB. Add the gate that would have caught both halves. Checksums are structurally blind to this failure: when a sweep frees memory a survivor still references, nothing diverges at the point of the bug -- the dangling reference reads whatever object recycled the slot, so the damage surfaces later, elsewhere, as corrupted data. That is how #5436's regression reached a user as "non word" dictionary entries and an impossible NPE instead of as a failing test. -DCN1_GC_VERIFY makes the invariant observable by destroying the plausible replacement: - POISON every reclaimed page slot and legacy block. This includes the O(1) all-dead page reclaim, which is where nearly all page memory is actually reclaimed and which normally drops a page without writing a single slot, leaving every dead object with an intact-looking header -- the reason a dangling read in this VM finds plausible data rather than crashing. - QUARANTINE freed legacy blocks in a ring instead of returning them to the C allocator, so a poisoned block stays mapped and recognizable. - VERIFY after every sweep: walk each survivor through its own generated mark function with the collector in verify mode, classifying every reference field against the page registry, the live-extent index and the quarantine set. A field pointing into reclaimed memory is reported with the holder's class, the victim's class and the field's mark call site, then aborts at the cycle that created it. The gate holds CURRENT-EPOCH survivors to the invariant, where a dangling field is unambiguous: the sweep either marked the object reachable (marking traces children) or promoted it by the grace rule (tracing the subtree was the grace pass's job). References it cannot place are skipped, so a violation is never a false alarm. CN1_GC_VERIFY_AGING extends it to previous-epoch survivors as a census rather than a gate. run-gc-verify.sh runs it over nine drivers and then re-injects the #5442 defect (CN1_GC_FAULT=nograce disables the grace pass) and REQUIRES the verifier to catch it -- a gate nobody has watched fail is not a gate, and a build where the verification silently compiled out would otherwise report a permanent, meaningless pass. GcHeapIntegrityIntegrationTest is the CI twin and asserts both halves. Two supporting pieces, both born from the same investigation: - LegacyGrace is the legacy-path twin of GraceAudit, plus the [GRACE-AUDIT-LEGACY] half of -DCN1_GRACE_AUDIT. Writing it exposed why drivers in this area come back green while the defect is present: the hazard has to be built with no mark in flight (during a mark the SATB barriers cover the very reference move under test) and the driver has to scrub its own native stack afterwards, because the conservative root scan marks whatever a returned frame's leftover word still points at. Both are documented at the driver and in the README. - CN1_GC_TRACE_MARK names the mark pass that keeps a class alive, which is how that retention was identified; CN1_GC_VERIFY_CENSUS reports whether a driver's hazard set actually ages out instead of being pinned. Validation: gauntlet GREEN in both stop modes, run-gc-verify GREEN over all nine drivers with the fault self-test firing, GraceAudit clean under -DCN1_GRACE_AUDIT, and the new integration test green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Sanitize the collector's env knobs in the heap-integrity gate Both halves of the gate are decided by environment variables, and anyone debugging the collector has exactly those exported: CN1_GC_VERIFY_SOFT downgrades the abort the faulted half asserts on, and CN1_GC_FAULT injects the defect the clean half asserts is absent. An inherited knob would invert a result rather than fail loudly. Drop CN1_* from the child environment in the integration test and unset the knobs at the top of run-gc-verify.sh, so both start from a known state and see only what they set themselves. Verified by running each with CN1_GC_FAULT=nograce and CN1_GC_VERIFY_SOFT=1 exported: the test passes and the script reports GC-VERIFY GREEN. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Report the mark site from the frame that actually has it cn1GcVerifyChild read __builtin_return_address(0) itself, which resolves to the return address in gcMarkObject -- one frame too deep, and in a different object file from the generated mark function the label claimed it pointed into. Anyone following a violation back to a field would have landed in the collector rather than at the field read. Capture the address in gcMarkObject instead, where this frame's return address IS the instruction inside the generated mark function (or gcMarkArrayObject for an element), and pass it down. Report it as an offset from the holder's own mark function so the line is self-verifying without symbols and survives ASLR: markSite= 0x102fa66c0 = markFn+36 (the field read, inside the holder's mark function) A small positive offset means the frame is right; add it to the mark function's symbol to reach the exact field. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Make every driver in the gate carry its weight, and fix the CI self-test on Linux Three problems, all the same shape: a check that reports success without having checked anything. 1. The verifier never reported whether it RAN. MtStress, MapTorture and SbTorture exit before any sweep completes, so their "clean" results in run-gc-verify.sh meant only that nothing was ever verified -- exactly the hollow-gate failure the fault self-test exists to prevent, sitting inside the same script. Count completed passes, print them at exit, and fail any driver reporting zero. Those three now end with one collection over the heap they built (printing nothing, so the gauntlet's byte-identical comparison is unaffected) and contribute real coverage. 2. The CI self-test failed on Linux: the fault was injected and the workload completed, but no dangling reference appeared, so the test declared the gate inert. Reproduced in a linux/amd64 container -- the platform is fine (GraceAudit detects the fault there), the app's hazard was too weak. Its during-mark phase now follows the shape that provably breaks a missing grace pass: refill, kick a mark, then allocate dropped fresh nodes in sleep-separated slices so they land across the mark rather than racing past it, then go quiet for a full cycle so the untraced children age past the free threshold. Verified in the container: clean 0 violations / 23 passes, faulted aborts with 20 reports, three runs, no variance. 3. The integration test now also requires a nonzero pass count, so a workload that stops driving collection fails instead of passing silently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Add the Codename One header to the three tortures this PR touches MapTorture, MtStress and SbTorture predate the copyright gate and carried no header at all. The gate only inspects files a PR adds or modifies, so giving them a trailing collection in the previous commit is what pulled them into scope. Header text copied verbatim from LegacyGrace.java, added in this PR and already passing. Comment-only: scripts/check-copyright-headers.sh passes over the branch, and the gauntlet still matches the host JVM byte for byte. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Correct the documented contract of cn1GcVerifyQuarantineFree The comment described an earlier design in which the function handed the evicted block back for the caller to release. It returns a boolean and frees the displaced block itself, and the call site in codenameOneGcFree already relies on that boolean -- so the comment was the only thing out of date, and the one part a future caller would have read first. State what the return value means instead: TRUE when the block was quarantined and must NOT be freed, FALSE when the quarantine could not be allocated and the caller should free normally. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Resolve the injected fault before the first grace pass consults it cn1GcFaultNoGrace was initialized only by cn1GcVerifyHeap, which runs after the sweep. The first cycle of every faulted run therefore traced grace subtrees normally, and a workload completing a single GC cycle never had the defect injected at all -- the self-test would have reported a gate that "cannot fail" purely because nothing ever faulted it. Resolve the switch at its first use in the mark instead; cn1GcFaultInit is idempotent and GC-thread only, so the call in cn1GcVerifyHeap stays as a harmless second one. Observable: [GC-FAULT] now prints before the first verify pass rather than after a sweep. Checking that turned up a matching gap in GcStress, which reported 1 verify pass on one run and 0 on the next: its churn triggers collections, but whether the last one reaches its sweep before the process exits is a race, so the new vacuity check would have flaked in CI. It gets the same trailing collection as the other drivers (prints nothing, so the gauntlet comparison is unaffected). Two consecutive gate runs now report identical pass counts across all nine drivers, and the gauntlet stays byte-identical to the host JVM. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * State LegacyGrace's real allocation volume against the trigger The hazard comment still described the driver's first shape -- 256 parents of ~800 bytes -- after it was scaled to KEEP=10000 arrays of 65 references. The number matters rather than being decoration: the window only works while nothing in it starts a collection, so a reader checking that property was being handed the wrong figure by an order of magnitude. Give the real one (about 560 bytes each, 5.6 MB total, against the 24 MB allocation-volume trigger) and say which constants would break the window if raised. Same for the other precondition, that no mark is in flight, since both are equally easy to lose when editing the driver. Comment-only; LegacyGrace still reports 120 clean verify passes with the fault self-test firing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Add the Codename One header to GcStress GcStress is the fourth torture in this tree without a header, and the trailing collection added for the vacuity check pulled it into the copyright gate's scope. Missed on the first pass because the earlier commit added headers to the three drivers touched at that point, and GcStress was edited after that check ran. Audited every source file this branch touches rather than fixing one report at a time: all eight now carry the header and scripts/check-copyright-headers.sh passes over the full branch range. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
shai-almog
added a commit
that referenced
this pull request
Aug 25, 2026
The filter's soundness argument ends "its non-fresh children are still logged by this same barrier as they are stored". That step has a precondition I did not state and did not check: the INSERTION half has to exist. Under CN1_NURSERY it does not. CN1_WRITE_BARRIER is the nursery remembered-set update there and enqueues nothing at all (cn1_globals.h:1020-1039), so a fresh container that takes an older child after the grace pass has that child recorded nowhere -- and dropping the deletion entry for the container then lets the sweep reclaim a child the grace-surviving container still references. That is a use-after-free, in the class of defect #5425 and #5442 were about. The filter is an optimisation and not a correctness requirement, so a build without the insertion half simply does not get it: the condition is now !defined(CN1_SATB_LOG_FRESH) && !defined(CN1_NURSERY). Adding SATB insertion to the nursery barrier was the other option offered and is the riskier one -- it changes barrier behaviour in a configuration nothing exercises, and would have to be justified by measurements no one can take. Latent rather than live: CN1_NURSERY is not defined anywhere in-tree, so no shipping or CI build takes that path. It is a documented, reachable flag, and the comment now records the dependency so the next person to enable it is not relying on an argument that quietly stopped holding. Verified: five ablation combinations compile including -DCN1_NURSERY; run-gc-verify.sh green with both fault self-tests; steady-state and heap-integrity gates green. Reported by chatgpt-codex-connector on #5599. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
shai-almog
added a commit
that referenced
this pull request
Aug 25, 2026
… (issue #5537) (#5599) * Stop the SATB barrier logging fresh references (issue #5537) Four merged fixes (#5540, #5563, #5573, #5585) each named a mechanism and the reporter's build still climbed 500MB to 5GB in five minutes on the iOS Simulator with a live set of a few hundred objects, GC pauses lengthening until they were continuous. The reason none of them settled it is structural: every GC workload in vm/tests measures a PEAK under load, and a heap that grows forever at a modest rate passes "peak < 2GB over 50 rounds" without difficulty. Nothing measured whether the VM ever gives the memory back. The instrument comes first, and it is what found this. -DCN1_GC_CONFORM adds a probe that PARTITIONS the footprint -- resident pages, legacy blocks, the legacy table, the allocator's side tables -- and prints the residual the four do not account for, plus a per-phase breakdown of the mark. It deliberately is not CN1_GC_VERIFY: that flag forces cn1BibopReleaseOffset() to 0, which compiles out the page-release path, the major sweep and every madvise call, so the paths a footprint investigation is about cannot be measured in a verifier build. It changes no allocator behaviour, and the emitters are gated at RUNTIME on CN1_GC_PROBE so probe-on and probe-off are the same binary. On the reported shape -- a deep game-tree search on four workers, tiny short-lived reference-carrying objects, a constant live set -- it named the cost immediately: of a 327ms mark, 282ms was SATB termination, draining 2,718,448 logged references in one cycle. Of those, 2,718,413 were references to FRESH objects. A mark == -1 object was allocated after the cycle's snapshot was taken, so it is not in the snapshot the barrier exists to preserve, and both sweeps keep it anyway -- the grace rule promotes a fresh slot to the current epoch instead of freeing it. Its own outgoing references to non-fresh objects are still logged by the same barrier as they are stored, so nothing reachable only through a fresh object is lost, which is the hazard the insertion half was added for. Without that filter the log is a feedback loop rather than a cost: its size is mutation rate times cycle duration, draining it is part of the cycle, so a longer cycle logs more and logging more lengthens the cycle. Both reported symptoms fall out of the one loop -- the footprint climbs because the collector never catches up, and the pauses climb because the log it has to drain keeps growing. Measured, three repetitions each, interleaved in one session: footprint drift before 306,684 / 241,493 / 224,237 KB/min after -31,430 / 36,866 / 18,993 KB/min (noise around zero) page count before 3,947 -> 5,995 over 40s and still climbing after flat at 11,687 for 40s mark time before 38ms -> 180ms; after 9-68ms, no trend under a simulated 1.4GB per-process ceiling: 3.5x the search throughput (237.8M nodes vs 67.6M), peak 1271MB, no kill Throughput, interleaved A/B, checksums bit-identical: geomean 0.944 -- 5.6% faster overall, objectAllocation 1.73x (56.3ms -> 32.5ms). The barrier was that expensive. -DCN1_SATB_LOG_FRESH restores the old behaviour for A/B. GcSteadyStateIntegrationTest is the gate. It asserts the SATB log stays sized by the live set rather than by the allocation rate, and that the page heap stops growing in the second half of the run; then it rebuilds with -DCN1_SATB_LOG_FRESH and requires both to fail, so it cannot go inert. Two pre-existing defects found on the way and fixed here: * -DCN1_DISABLE_CONSERVATIVE_GC_ROOTS, the revert path cn1_globals.h documents, did not compile at all: the grace passes use CN1_GC_TRUSTED_BEGIN/END/SUSPEND/ RESUME unconditionally and those are only defined with conservative roots on. No-op definitions restore it, which is what makes it usable as an A/B arm. * [GC-INSTR] allocs= is not an allocation count -- CN1_FAST_NEW's inlined bump path never reaches that counter, so on a small-object workload it understates allocation by orders of magnitude. Renamed to outOfLineAllocs= with a note. Verified: 520 vm/tests non-benchmark tests green; all six GC benchmark tests green; run-gc-verify.sh green including both fault self-tests; run-gauntlet.sh green with every checksum matching; grace audit reports doomedChildren=0 with and without the filter; and the probe compiles across nine ablation flag combinations. Not addressed, and pre-existing: under a per-process ceiling the process still rides to ceiling-minus-64MB, which #5585 flagged as open. That is now a bounded plateau rather than unbounded growth, but the margin is thin on a device where the renderer shares the same budget. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Defend a headroom reserve under a per-process ceiling (issue #5537) The previous commit stopped the heap growing without bound. This one stops the process parking itself on the kill line, which #5585 flagged as open and which is what turns a native spike into a jetsam kill. Budget headroom is not a footprint bound. Admission against os_proc_available_memory answers only "is there budget left", so it keeps saying yes until the budget is gone. Measured on the issue-5537 game-tree shape under a simulated 1.4GB ceiling, seven times: 1,271MB resident and 63MB of headroom left, every time, against a live set of a few hundred objects. That repeatability is the tell -- it is not an accident of the workload, it is the policy converging on ceiling minus CN1_PACING_HEADROOM_MARGIN by construction. The ceiling is not special either: give the same workload an 8GB budget and it rides to 7.5GB. There is no footprint TARGET anywhere in the design. 63MB is the whole margin, and the renderer spends out of the same budget -- #5598 measured one screen texture at 30MB. So the collector now also bounds how far the mutator may run ahead of it, but only once headroom drops inside a reserve of a quarter of the budget (CN1_PACING_RESERVE_SHIFT). Inside the reserve the mutator is clamped to the static cap, the collector gets ahead, and the footprint falls back out. Gating on HEADROOM rather than on footprint is what makes this affordable: it is a control loop that engages only inside the reserve, not a tax on every allocation, and volumeParks in the [PACING] report is 0 for a run that never enters it. Both allocation paths are charged against ONE figure. Bounding them separately is a defect this code has had before -- each running a full cap ahead of a cap derived from the same budget -- and the reserve is derived from the BUDGET, never from the device's free RAM, which is the defect #5563 fixed. cn1BibopPacingCap is deliberately not reused for that reason. Measured, builds interleaved within one session (-DCN1_PACING_NO_RESERVE is the same binary with the bound compiled out), simulated 1.4GB ceiling, four workers: peak footprint smallest headroom seen no reserve 1271MB, x7 63MB, x7 reserve limit>>2 1027-1036MB 298-304MB 4.8x the margin. Throughput across seven interleaved pairs came out at 0.90 to 0.99 of the unbounded build, median 0.94; the spread is session drift, not the bound, and the sign never changed. A single repetition each of the tighter reserves put >> 3 at 1183MB/150MB and >> 4 at 1207MB/127MB, both slower -- a smaller reserve engages later and thrashes closer to the edge -- so a quarter is the knee rather than a compromise. Roughly 6% for that margin is a different trade from the volume brakes #5573 and #5585 measured at 2-4x and rejected. It cannot touch a platform with no per-process budget, because the whole branch is unreachable there: vm/benchmarks measures geomean 0.9398 against master, i.e. still 6% FASTER from the previous commit's SATB fix, with no benchmark regressing and every checksum identical. cn1PacingPastGrowthFloor's rate-limited footprint probe is factored out as cn1PacingFootprintNow so both bounds read through it. Behaviour-preserving: each of its three early returns previously answered FALSE, and the fast path above already established that the cached value is under the floor. GcSteadyStateIntegrationTest gains a third scenario asserting the process defends its reserve under a simulated ceiling, and a fourth that rebuilds with -DCN1_PACING_NO_RESERVE and requires the third to fail -- otherwise a gate that never engages would report green forever. Verified: 520 vm/tests non-benchmark tests green; all seven GC benchmark tests green (ProcessBudgetPacingIntegrationTest included, which exercises the same budgeted path); run-gc-verify.sh green with both fault self-tests; run-gauntlet.sh green with every checksum matching; nine ablation flag combinations compile. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Give the benchmark driver its GPL header, and scope two helpers to their use check-copyright-headers rejects a new source file without the complete Codename One GPLv2 + Classpath Exception header, and vm/benchmarks/src is in scope. cn1PacingUncollectedBytes and cn1PacingReserveBytes are used only from the reserve bound, so they are guarded on the same condition it is -- otherwise compiling the bound out with -DCN1_PACING_NO_RESERVE leaves them as unused statics. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Count benchmark nodes per worker, not through a shared racy counter The driver incremented one static long from four workers with an unsynchronised read-modify-write, and the sampler read it concurrently. That is not merely imprecise: the rate at which increments are lost depends on CONTENTION, and contention is exactly what differs between the builds this benchmark compares -- a build whose threads park more loses fewer increments and so reports a throughput advantage it has not got. The per-round `nodes = localNodes` writeback also overwrote the shared total instead of combining the workers' counts. Each worker now counts into its own slot, and NODES= is summed after join(), which gives it a happens-before edge to every worker's last write. The SAMPLE series sums the same slots while they are still being written, so it is renamed nodes~= and documented as a progress indicator rather than a measurement. The CI fixture (GcSteadyStateApp) never had a node counter -- its assertions come from the [GCPROBE] series -- so nothing the gate asserts is affected. Reported by chatgpt-codex-connector on #5599. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Keep every probe row a single-cycle row, publish node counts live Two review findings on #5599, both real. cn1GcProbeCycle returned early on a skipped cycle without clearing the phase accumulators, so with CN1_GC_PROBE>1 snapMs/graceMs/satbMs and friends carried a whole interval while markMs and sweepMs described only the cycle that just ran -- two time bases in one row, which would attribute an interval's worth of a phase to a single cycle's pause. The resets move into cn1GcProbeResetPhases and run on every cycle, printed or not. The cumulative counters (matured, consWords, staleSkips) are deliberately left alone: those are running totals the reader diffs. The benchmark driver published each worker's node count only after the run stopped, so every SAMPLE line reported zero. It now republishes once per round; a worker that stalls stops publishing and its slot going flat is the signal. Neither affected any measurement reported so far -- every run used CN1_GC_PROBE=1, where the skip path is unreachable, and the throughput figures come from NODES=, which is summed after join(). Also corrects the reserve's throughput figures, which came from the racy counter the previous commit replaced. Re-measured with the exact one, four interleaved pairs: 0.97-1.05 of the unbounded build, median 0.99, two of four faster with the bound on. The previous "median 0.94" overstated the cost. Peak footprint and headroom are unchanged (1271MB/63MB against 1015-1027MB/306-308MB) -- those come from the probe and Runtime, not the counter. The claim that a smaller reserve is "slower" is withdrawn; >>3 and >>4 buy less on peak and headroom, which is the argument that survives. Reported by chatgpt-codex-connector on #5599. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Load the mark word atomically in the barrier, and harden the ceiling scenario Three findings, two from review and one the review's tighter test surfaced. The SATB filter read __codenameOneGcMark with a plain load while the marker reaches the same field through __atomic_*. That is a mixed atomic/non-atomic access to one object -- undefined in C, and the same bug class #5598 fixed in the constant pool. The concrete hazard is not tearing but the compiler caching a -1 across several inlined barriers in one loop, which would keep suppressing entries after the object had aged into a genuine snapshot object. Now __ATOMIC_RELAXED, and the comment says why relaxed and not acquire: nothing is published through this read, both stale answers are safe, and what relaxed buys is that the load happens at all. An acquire fence on every object store buys nothing over that and is not free on arm64. The two CN1_GC_CONFORM census reads of the same field move with it. The fault-injected runs' measurements were accepted without checking exit status or the completion marker, so a build that crashed after emitting enough probe rows would have satisfied the assertions and turned a memory-safety regression into a green gate. Both now go through assertHealthy first. The ceiling scenario used a 1400MB budget, which needs the mutator to actually outrun the collector by 1.3GB -- and how far it outruns depends on how many cores it has to itself, so a two-core runner might never get there and the fourth scenario would go quietly inert. It now uses 768MB, which admission converges on by construction rather than by winning a race. The threshold between the two regimes becomes ABSOLUTE, twice CN1_PACING_HEADROOM_MARGIN, because the margin does not scale with the budget: a proportional threshold silently stops separating them as the budget shrinks, which is exactly what happened at 400MB (reserve 100MB, margin still 63MB, half the reserve below it). Reported by chatgpt-codex-connector on #5599. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Do not size an adopted BiBOP slot as if it were a malloc block The probe sized every non-null allObjectsInHeap entry with malloc_size / malloc_usable_size. A MATURED object is in that table but its storage is a slot inside a posix_memalign'd BiBOP arena, so the pointer is interior: glibc's malloc_usable_size reads the chunk header immediately below it and returns a garbage figure, and CI runs this gate on Linux. Its bytes are also already counted in residentPgBytes, so anything it did return double-counted into the residual that is this probe's whole point. Only an object the table INDEXES (__heapPosition >= 0) owns an individual block. The rest are counted as legAdopted instead -- the same population as matured - maturedDied but measured from the table rather than from the counters, so the two disagreeing is itself a finding. Not a small corner: on the game-tree workload legAdopted is 32,907 of a legUsed of 33,164, so 99% of the table was being sized this way. It was harmless on macOS only because malloc_size answers 0 for an interior pointer, which is also why legBlockKb read flat through the original investigation and correctly never carried the drift. Verified after the change: run-gc-verify.sh green with both fault self-tests, and vm/benchmarks geomean 0.9422 against master (0.9398 before the previous commit's atomic load, i.e. that load costs nothing), no benchmark regressing, checksums identical. Reported by chatgpt-codex-connector on #5599. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Close the wait timer at the wait, read the cycle counter atomically Three findings from review; two fixed, one measured and answered in the code. waitMs was opened before the safepoint wait and closed only after the allocation migration and both stack scans, so it double-counted work already attributed to migrateMs and stackMs -- a phase breakdown that overlaps reads a long root scan as mutator wait time, which is the opposite of what it exists to say. It now opens and closes around the wait alone, inside the lightweightThread branch, so a native thread (which is never waited for) contributes 0 instead of everything up to markStatics. The 1Hz emitter read currentGcMarkValue with a plain load while the collector increments that ordinary int -- a data race, in the one emitter documented as "atomics only" and built to keep reporting exactly when the collector is stalled. Now an atomic relaxed load, as is the mutator-side comparison in the SATB census. Not taken: requiring the -DCN1_SATB_LOG_FRESH build to also blow the second-half page-growth bound. Measured across two runs of that build, its second-half growth is 0.446 and then 0.033 -- a runaway's page pool sometimes saturates before the midpoint and the ratio then reads flat while the heap is enormous. That assertion would fail about half the time, and a coin-flip gate is worse than the inertness it guards against. The reasoning, the numbers and what does have teeth (the SATB metric, five orders of magnitude, every time) are recorded on the constant. Both series are now printed on every run so the ratio stays auditable rather than merely asserted. Verified after these changes: phases sum to markMs with no overlap (16.0 of 16.3); 520 vm/tests non-benchmark tests green; all seven GC benchmark tests green. Reported by chatgpt-codex-connector on #5599. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Read the collector's atomic epoch mirror, and claim a matured page with one edge Two follow-ups from review, both correct. The previous commit made the 1Hz emitter's read of currentGcMarkValue atomic while codenameOneGCMark still increments it with a plain ++. That is half a fix: an atomic read of a plainly-written object is still a mixed access and still undefined. Both sides now go through bibopGcEpoch, the collector's own _Atomic mirror of the same value, published at cycle start -- which is what the reviewer offered as the alternative and what should have been used first. The mutator-side comparison in the SATB census moves with it. Where there is no page heap there is no mirror, so the emitter reports cyc=-1 rather than a figure read through a data race. cn1MaturedPages tested gcHasAdopted and then let the existing plain store set it. The CAS above guarantees one thread matures a given OBJECT, but two markers can mature two different objects on the SAME page, so both could observe FALSE and both count it -- and the plain store is itself a data race the moment gcMarkResolveThreadCount stops returning 1. Now one __atomic_exchange_n: exactly one thread sees the FALSE->TRUE edge, and it does the counting. That the ratio is read chiefly in the CN1_GC_MARK_THREADS>1 arm is the point -- it would have been wrong exactly where it is used. Verified in that arm: maturedPages=2121 of pgTotal=11214, a plausible ratio rather than an inflated one. run-gc-verify.sh green with both fault self-tests; the steady state, heap integrity and process budget gates green; seven ablation combinations compile. Reported by chatgpt-codex-connector on #5599. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Make every collector-side write to the mark word atomic The barrier's read was made atomic two commits ago while gcMarkObject still stamped the same field with a plain store, so the pair was still a mixed access. The field already had an atomic convention here -- gcMarkObject's own read is __ATOMIC_ACQUIRE, the BiBOP publish is __ATOMIC_RELEASE -- and the plain writes were the inconsistency, not the new read. Every write that can run concurrently with a mutator is now a relaxed atomic store: gcMarkObject's stamp, both sweeps' grace promotion, both free-mark stores, the nursery promotion and the CN1_GC_VERIFY poison. Relaxed compiles to the same instruction on every target we build; what it buys is that the write is a write the reader is allowed to observe. Header INITIALISATION deliberately stays plain, in codenameOneGcMalloc and in cn1FusedInstallPrimArray. Those are not concurrent with anything: the barrier only ever reads the mark of an object the mutator holds a reference to, so one already published, and the publishing store orders the initialisation against any reader. That distinction is not free-floating -- making those two atomic as well cost 1.2 points of benchmark geomean (0.9550 against 0.9432, with arraySequential, quicksort and valueEscape all moving and returning), because they sit on the allocation fast path. The reasoning is recorded at the site so the next person does not reintroduce it for symmetry. Verified: vm/benchmarks geomean 0.9432 against master, six rounds interleaved, no benchmark regressing and checksums identical; run-gc-verify.sh green with both fault self-tests; all seven GC gates green; five ablation combinations compile including -DCN1_NURSERY. Reported by chatgpt-codex-connector on #5599. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Emit the generated mark chain's root store atomically too The previous commit converted every hand-written collector-side write of the mark word and missed the one that matters most, because it is not in the C sources at all: ByteCodeClass emits the root of every generated mark chain, and that store was still plain. It runs on the GC thread for every object marked while the SATB barrier atomically loads the same field from mutators, so the pair stayed a mixed atomic/non-atomic access -- the exact defect the previous commit was for, in the one place a grep of cn1_globals.m could not see. Costs nothing, as the hand-written conversions did not: vm/benchmarks geomean 0.9387 against master over six interleaved rounds (0.9432 before this change, so inside the noise), no benchmark regressing, checksums identical. A codegen change touches every translated class rather than one runtime path, so it is verified against the shapes rather than the sites: run-gc-verify.sh green with both fault self-tests, and run-gauntlet.sh green with every checksum matching. Reported by chatgpt-codex-connector on #5599. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Publish the sampler's counters, and keep the page partition valid under a race Two findings, one taken as offered and one taken but answered differently. The benchmark driver's per-worker slots were published with a plain long[] write against a concurrent reader: no visibility guarantee, and Java 8 permits a 64-bit element to be observed torn, so the live series could sit stale or jump nonsensically exactly when a stalled worker is what it is meant to show. Publication and sumNodes() now share SUM_LOCK. Once per round is about once a second per worker, so it costs nothing, and NODES= after join() remains the authoritative figure regardless. The probe's page walk is a different case. It reads plain page counters while mutators run, which is a race, but it is the same deliberate sample cn1HeapAccounting takes beside it -- "a diagnostic wants the shape, not the last digit" -- and both offered remedies cost more than the unsoundness. Stopping the page owners would perturb collector/mutator timing, which is the quantity this probe reports, and would cost CN1_GC_CONFORM the behaviour-neutrality that is the only reason it is a separate flag from CN1_GC_VERIFY. Making the page fields _Atomic would put atomic accesses on the inlined bump path in cn1_globals.h, the hottest code in the VM, to improve a diagnostic. What is worth fixing is the harm actually named: an internally inconsistent partition. Only an owned page can move under the walk -- at most one per size class per thread out of many thousands -- so freeCount is clamped into [0, bumpIndex] and a stale pair can no longer make live and dead slots sum past the page. Verified: 517295 + 25326 KB against a 776448 KB reservation. The reasoning is recorded at the walk so the next reader does not have to rediscover which of the three options was chosen and why. Verified: run-gc-verify.sh green with both fault self-tests; steady-state, heap integrity and process budget gates green; the sampler now tracks progress live (nodes~=28,697,812 mid-run against a final NODES=34,360,526); and the 520-test non-benchmark suite is green on the regenerated code from the previous commit. Reported by chatgpt-codex-connector on #5599. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Survive a stall: publish inside the traversal, bound the run Both findings are the same blind spot from two directions -- a stalled collector is one of the things this gate exists to CATCH, and neither the progress series nor the runner survived one. Publishing between rounds was not enough. One depth-14 traversal is millions of nodes, so if the collector stalls badly enough that no round completes inside the window, nothing is ever published and the series reads zero -- silent in exactly the case it is for. It now also publishes every 1<<20 nodes: a power of two so the test is an AND, coarse enough (about a fifth of a second of work) that the lock traffic is negligible against the sampler's 4Hz. Verified live: 0 -> 4,194,304 at 1s -> 33,554,432 at 9.8s, against a final NODES=35,255,230. The runner read the child's output to EOF on the test thread and only then called waitFor(), so a hung workload would block until the CI job's global timeout -- the guard would stop reporting a regression and start eating the build. It now drains on a background thread and waits with a bound, killing the child on expiry and failing with whatever it printed, which is the only diagnostic a stalled run leaves. That is not a new invention: GcOverflowSpiralIntegrationTest and ProcessBudgetPacingIntegrationTest both already do exactly this, and the naive pattern came from copying GcHeapIntegrityIntegrationTest, which is the one that does not. Reported by chatgpt-codex-connector on #5599. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Do not filter fresh SATB entries where there is no insertion barrier The filter's soundness argument ends "its non-fresh children are still logged by this same barrier as they are stored". That step has a precondition I did not state and did not check: the INSERTION half has to exist. Under CN1_NURSERY it does not. CN1_WRITE_BARRIER is the nursery remembered-set update there and enqueues nothing at all (cn1_globals.h:1020-1039), so a fresh container that takes an older child after the grace pass has that child recorded nowhere -- and dropping the deletion entry for the container then lets the sweep reclaim a child the grace-surviving container still references. That is a use-after-free, in the class of defect #5425 and #5442 were about. The filter is an optimisation and not a correctness requirement, so a build without the insertion half simply does not get it: the condition is now !defined(CN1_SATB_LOG_FRESH) && !defined(CN1_NURSERY). Adding SATB insertion to the nursery barrier was the other option offered and is the riskier one -- it changes barrier behaviour in a configuration nothing exercises, and would have to be justified by measurements no one can take. Latent rather than live: CN1_NURSERY is not defined anywhere in-tree, so no shipping or CI build takes that path. It is a documented, reachable flag, and the comment now records the dependency so the next person to enable it is not relying on an argument that quietly stopped holding. Verified: five ablation combinations compile including -DCN1_NURSERY; run-gc-verify.sh green with both fault self-tests; steady-state and heap-integrity gates green. Reported by chatgpt-codex-connector on #5599. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Let CN1_WL_LEGACY=0 actually ablate the legacy population Setting the documented knob to 0 built a zero-length legacyLiveSet and then indexed [-1] on the last line, so the driver threw AFTER the entire timed run had been paid for -- losing RESULT and GC_STEADY_STATE_DONE, which is everything the run was for. Running without the retained legacy population is a legitimate ablation, so it now works rather than crashing: the fold is skipped when there is nothing to fold. Two neighbouring values that would produce a wasted or silently empty run are clamped at the same time. A negative CN1_WL_LEGACY reached new Object[n][]; a CN1_WL_THREADS below one started no workers at all and reported that only by printing zero nodes, which is the exact failure mode -- a measurement that looks like a result -- this whole change has been about. WLCONFIG prints the clamped values, so the log says what actually ran. Verified: CN1_WL_LEGACY=0, CN1_WL_LEGACY=-5 with CN1_WL_THREADS=0, and the defaults all reach RESULT and GC_STEADY_STATE_DONE. Reported by chatgpt-codex-connector on #5599. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Check the answer, not just the telemetry, in every scenario Three of the four runs checked exit status and the completion marker but never that the workload still computed the right thing. That gap matters most exactly where it was left: the ceiling scenarios exercise the budgeted pacing path -- the code this change touches most -- under an environment the clean run never sees, so a worker could die early or compute a wrong sum while the process still exited cleanly and emitted plenty of [PACING] telemetry for the policy assertions to pass. None of the variants changes what the program computes: the faults injected are a barrier filter and a pacing bound, and the workload is deterministic by construction (fixed rounds, fixed seeds, an order-independent checksum). So RESULT must equal the host JVM's in all of them, and assertHealthy now requires it -- which also picks up the -DCN1_SATB_LOG_FRESH run, which had the same gap. Reported by chatgpt-codex-connector on #5599. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Normalise every workload knob, not just the one that was reported CN1_WL_MOVES=0 left the move chain null and the next-seed derivation dereferenced it, so the leaf-only ablation died with an NPE on the first node. That is the second knob found this way, so this fixes the class rather than the instance: all eight are normalised in one place before the timed run, and WLCONFIG prints the normalised values so the log says what actually ran rather than what was asked for. Auditing the rest turned up one more that was worse than the reported one. A negative CN1_WL_DEPTH never matches the d == 0 base case, so it recursed until the stack gave out. CN1_WL_SECONDS and CN1_WL_BRANCH below their floors produced runs that measured nothing and said so only by reporting zero -- the failure mode this entire change is about. Zero stays meaningful where it means something, and both cases are real ablations: no retained legacy population, and no reference-carrying Move per node. The second is worth having, because only a non-leaf object reaches the grace pass's worklist or maturation, so leaf-only allocation is a genuinely different workload for the parts of the collector under test. Verified: CN1_WL_MOVES=0, CN1_WL_MOVES=-3, CN1_WL_DEPTH=-1, CN1_WL_BRANCH=0, CN1_WL_SECONDS=0 and CN1_WL_LEGACY=0 all reach RESULT and GC_STEADY_STATE_DONE, and the default configuration is unchanged. Reported by chatgpt-codex-connector on #5599. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Flag the probe row when the collection cycle threw gcMarkSweep wraps mark and sweep in a catch-all so a throwing finalizer cannot wedge the collector. On that path control jumps past the timing assignments, so the probe emitted a row carrying the PREVIOUS cycle's markMs and sweepMs beside the partial current cycle's phase counters -- two cycles in one row, and it concealed the exceptional cycle, which is the one a reader most wants to see. This is the same defect as the CN1_GC_PROBE>1 skip path fixed earlier, on a different route out. The timings are now cleared BEFORE the protected region, so a throw cannot inherit them, and the row carries threw=1 rather than being suppressed: hiding it would defeat the reason this probe has a wall-clock emitter at all. The three carriers are file scope, so the setjmp/longjmp indeterminate-local rule does not apply to them. Verified: five ablation combinations compile; run-gc-verify.sh green with both fault self-tests; steady-state and heap-integrity gates green; probe rows carry threw=0 on a healthy run. Reported by chatgpt-codex-connector on #5599. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Re-evaluate the reserve throughout the wait, and stop the driver perturbing itself Two review findings, and a third defect the first one's verification exposed. The wait loop's copy of the volume bound was guarded on the thread having already been refused, so it could only transition refused->allowed. A thread that parked on BUDGET while outside the reserve then held a stale "allowed" for its whole wait and could be admitted on headroom alone after other mutators had pushed the uncollected total past the cap and the process into the reserve. There is now ONE definition, cn1PacingVolumeOk, called from both sites and recomputed every iteration -- the two copies drifted precisely because they were two. The gate parsed only the per-cycle [GCPROBE] rows, so a collector that completes its early cycles and then never finishes another was invisible to it: the rows stop, the generated main returns as soon as the workers do, and the process exits cleanly with the marker while the heap is still growing. [GCPROBE-T] was added for exactly that state and then not asserted on. The outcome check now covers the wall-clock series too, with its own anti-vacuous row count. And the driver had started perturbing its own experiment. The periodic publication added two commits ago took SUM_LOCK inside the search, and monitorEnter is a GC SAFEPOINT in this VM -- so the workers were being stopped far more often than the workload otherwise permits and the runaway stopped reproducing: peak footprint fell from 1271MB to 126MB with the reserve compiled out, in BOTH builds, which is what gave it away. Publication is now a volatile long per worker: not a safepoint, not a lock, and JLS 17.7 makes volatile long access atomic, so it also answers the visibility and tearing that the plain long[] had. With the runaway restored, the reserve's throughput cost is re-measured across four interleaved pairs at 0.875-1.035, median 0.90 -- about a tenth, not the ~1% the previous figure claimed. Peak and headroom are unchanged (1271/63 against 1022-1064/272-304). This is the third throughput figure this comment has carried and the first two were both apparatus rather than signal, so the comment now says which were which. Verified: four ablation combinations compile; run-gc-verify.sh green with both fault self-tests; the steady-state gate green with all five checks. Reported by chatgpt-codex-connector on #5599. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Attach evidence to the ceiling assertions The first vm-tests run that ever completed on this branch failed scenario 3 -- "the smallest headroom seen was 62MB" under a 768MB budget -- and reported nothing else. Every other assertion in this gate appends the run's output; this one, the only one that has actually failed, did not. The probe rows that would explain it were captured and then discarded. Both ceiling assertions now carry the [PACING] counters, the last [GCPROBE] footprint partition and the wall-clock summary. That partition is the whole point of the probe: it says whether a footprint the reserve did not defend is even in the Java heap. No behaviour change, and the gate still passes locally on macOS -- which is itself the open question, since the failure is on the Linux runner and the two measure different quantities (phys_footprint against RSS). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Assert the reserve's mechanism, report its outcome The first vm-tests run that completed on this branch failed scenario 3 on the Linux runner: 62MB of headroom under a 768MB budget. With the evidence attached, the diagnosis is not what I guessed. I expected allocator retention -- glibc arenas holding freed legacy blocks, which RSS counts and phys_footprint would not. Wrong: residKb was 7MB of a 518MB footprint, so the footprint was the Java heap almost exactly. That is the residual bucket earning its place; it killed the hypothesis in one line. What the runner actually shows is a collector that cannot keep up, with the bound working: volumeParks=879, so it engaged and parked repeatedly, while mark ran 407-545ms per cycle -- 235ms of conservative stack scan, 122-252ms waiting for mutators to reach a safepoint -- against ~170MB of allocation per cycle. With the grace rule holding a cycle's allocation two more cycles, the smallest working set that machine can hold is already above the reserve line at that budget. satbMs was 0 throughout, so the earlier fix is holding and the stack scan is simply the next cost. So an absolute headroom assertion was testing the runner rather than the collector. Scenario 3 now asserts the contract, which is true on any machine: either the process never entered its reserve, or the bound engaged when it did. The headroom achieved is printed either way, so the outcome stays visible without being asserted. A regression that stops the bound engaging fails here; a machine that is merely slow does not. Scenario 4 gains a second half for the same reason -- with the reserve compiled out the process must land on the bare admission margin, or the ceiling is not pressuring the workload and scenario 3's "never entered" branch would pass for the wrong reason -- plus volumeParks == 0, since the bound is not in that build at all. Locally: headroom 161MB inside a 192MB reserve with volumeParks=350, against 63MB and volumeParks=0 with the reserve compiled out. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
Fixes the heap-corruption regression reported in #5425 (comment) -- corrupted dictionary words plus an "impossible" NPE -- introduced by the fresh-page-stack grace pass in #5436 (df5719e).
The bug
The grace pass exists because a fresh BiBOP object (
gcMark == -1) survives the sweep via grace, so an old object reachable ONLY through it must be traced during the same cycle or the sweep frees it while it is still referenced. #5436 replaced the full-registry grace walk with alternating per-epoch fresh-page stacks, deduped by queueing a page only on its FIRST allocation in each epoch.That dedup leaves a wide uncovered window: once the grace pass consumes a page's stack entry mid-mark, every later allocation in the SAME epoch -- the rest of the mark plus the entire unbarriered inter-cycle gap -- skips re-queueing. If the page then receives no next-epoch allocation before the next grace pass (a bursty size class going quiet, exactly the Dtest load-then-lookup shape), its fresh slots are never grace-traced. The sweep frees their only-referenced children; the fresh parents survive pointing at recycled memory. Compact strings inline the byte payload into the String's BiBOP slot, so slot recycling rewrites word bytes in place -- the "non word" symptoms in the issue.
Empirical proof
A new QA-only audit (
-DCN1_GRACE_AUDIT) snapshots each page's bump cursor at mark start and, right before the sweep, full-walks the registry tracing any pre-snapshot slot that is still fresh.doomedChildrencounts objects that became marked ONLY through those missed objects -- each one would otherwise be swept while still referenced. Against the fresh-page stacks, the newGraceAuditdriver (allocates dropped fresh nodes holding sole references to older objects WHILE the concurrent mark runs, then goes quiet a cycle) reports:The fix
Walk the full page registry, pruned by the existing
gcAllocedSinceSweepflag. The invariant is exact and race-free:mark==-1slot can only exist on a page allocated into since that page's last sweep (the sweep converts every-1it sees);owner==0) pages -- no mutator/GC race exists.Flag-FALSE pages (the retained-survivor bulk on exactly the workloads #5436 targets) are skipped without touching a slot, preserving the pause win, and the entire fresh-page queueing machinery (two page-header fields, epoch-mirror check + queue call in three allocation paths) is deleted from the hot allocation path. Allocations landing mid-mark after the grace pass remain covered: SATB protects their links this cycle and the sticky flag re-traces them next cycle.
Validation
GraceAuditwith-DCN1_GRACE_AUDIT: doomedChildren=0 on every cycle, repeated runs (was 100-250/cycle).run-gauntlet.sh: GREEN -- all 9 tortures byte-identical to the host JVM, GC stress in cooperative AND forced-signal stop modes.run-bibop-adaptive.sh: GREEN -- issue-5425 retained-array correctness, all adaptive-policy checks, adaptive 0.94x time / 0.62x RSS vs legacy.MapTorture,GcStress,MtStress,ThreadChurn,FusedTest,AdoptDeath) with the audit enabled: zero doomed, all checksums pass.StormAB/LoadLoopdrivers: parity (storm remains 5-8x faster than pre-Adapt BiBOP GC policy to survivor-heavy allocation #5436; repeated 120k-entry dictionary loads stay flat).GraceAudit,StormAB,LoadLoopand the audit flag are committed as the regression gate for future grace-pass changes (documented invm/benchmarks/README.md).Fixes the corruption regression from #5425.
🤖 Generated with Claude Code