End the GC stall on issue #5537: answer the collector's demand signal, and stop it paying for work that finds nothing - #5609
Merged
Merged
Conversation
3 tasks
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e20eb755c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Collaborator
Author
|
Compared 163 screenshots: 163 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 163 screenshots: 163 matched. Benchmark ResultsDetailed Performance Metrics
|
Contributor
Cloudflare Preview
|
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Collaborator
Author
|
Compared 163 screenshots: 163 matched. |
Collaborator
Author
|
Compared 163 screenshots: 163 matched. |
Collaborator
Author
|
Compared 163 screenshots: 163 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 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
|
Collaborator
Author
|
Compared 217 screenshots: 217 matched. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 877eff7f3d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4871124034
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f42fd6deb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c4f697fd0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
shai-almog
force-pushed
the
gc-stall-5537
branch
from
August 28, 2026 15:34
1b73a39 to
799a0d3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 799a0d324c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49f55f24f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dcf40e72d7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44775f19e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…sue #5537) Five fixes have been merged against #5537 and the reporter's build is still unusable, but the symptom has changed: "no long term memory buildup, and no crashes, but the pauses for GC become very frequent and very long". The footprint work is done. What is left is latency, and nothing in this runtime measured latency -- [GCPROBE] times the COLLECTOR, waitMs is its inverse, and [PACING]/[LOWMEM] count parks without recording how long any of them lasted. A build could stop every worker for most of a run with every gate green. THE INSTRUMENT COMES FIRST, and it named this in one run. [GCSTALL] brackets every site where a mutator can be stopped and charges the duration to a cause -- pacingVolume, pacingBudget, lowMemory, handshake, pendingFull, nativeResume, signalStop -- with a log2-microsecond histogram behind p50/p99/max, and prints dutyPct, the share of wall time the mutator threads were RUNNING. Like the footprint probe it is -DCN1_GC_CONFORM only, and it costs two clock_gettime calls per PARK, never per allocation. On the churn workload it reported a 40ms mark against a 212ms mutator park. WHAT IT FOUND. bibopBytesSinceGc is zeroed at cycle START, so under sustained churn a mutator re-crosses the collection trigger throughout every cycle -- and cn1BibopMaybeGc discarded all of those crossings behind a !gcCurrentlyRunning gate. By the time a cycle ended and the gate lifted, every mutator was parked on the run-ahead cap and therefore allocating nothing, so no crossing was left to raise the request. forceGc was false, and the GC thread took its 200ms idle wait with the whole application blocked on it, waiting for the next cycle to begin. The legacy trigger in codenameOneGcMalloc had already solved exactly this, and says so in its own comment: level-triggered, latched to one request per cycle window, and "deliberately no !gcCurrentlyRunning suppression -- the latch makes it redundant anyway". The BiBOP side simply never got the same treatment. It does now, with the same latch. The other half is that the request was not answered either. The GC loop read if(forceGc || isHighFrequencyGC()) { forceGc = false; LOCK.wait(200); } which CLEARS a pending request and then sleeps anyway -- System.gc()'s notify is lost because nobody is waiting yet. forceGc means a collection is owed, so it is now answered: java_lang_System_gcIdleWaitMillis___R_int returns 0 and the collector starts the next cycle at once. It answers only while the request STILL STANDS. Both byte counters are zeroed at cycle start, so what they hold at the end is what the mutator produced DURING that cycle: at or above the trigger it is outrunning the collector and the next cycle is genuinely owed; below it the collector is keeping up and the ordinary idle is the right answer. Answering unconditionally measured 8-9% on the two allocation-heavy microbenchmarks -- a real cost paid by applications that were never blocked. The 200ms/30s idles are unchanged for the cases that really are idle. isHighFrequencyGC() is called unconditionally and exactly once because it resets allocationsSinceLastGC as part of answering. MEASURED, interleaved in one session, GcSteadyState, four workers, median of 3: search throughput 43.0M -> 121.5M nodes (2.82x) mutator duty cycle 51% -> 90% mean mutator stall 213ms -> 15ms (14.6x shorter) footprint 603MB -> 505MB (-16%) The footprint falls because a collector that runs when it is asked keeps less garbage; this does not trade memory for latency. Under a simulated 1.4GB per-process ceiling -- the reporter's iPad regime, where the budgeted pacing park already re-requested a collection every 200ms and the starvation was therefore partial: +16% throughput, duty 88% -> 93%, total stall -42%, and more headroom left below the ceiling (285MB -> 311MB). Under the harder shape, CN1_WL_BIGARRAY=256 (arrays over CN1_BIBOP_MAX_OBJECT, so the legacy calloc + allObjectsInHeap path a real game-tree search hits with a 15x15 int board): +78% throughput, -38% footprint. Duty only reaches ~55% there, because the per-cycle legacy costs are large and are NOT what this addresses. vm/benchmarks geomean 1.011 against the ablation, interleaved, nine reps, checksums bit-identical. The residual is hashMapChurn at 1.075: an allocation -heavy microbenchmark paying honestly for a collector that no longer sleeps through its garbage. GATES. GcSteadyStateIntegrationTest gains two scenarios. The assertion is on the MECHANISM, as scenario 3's comment argues it must be: cyclesOnDemand / cyclesAfterIdle say how the collector decided to start each cycle, and unlike any pause threshold that means the same thing on a slow runner -- fewer cores make cycles longer, they do not make a collector idle through demand. The outcome is asserted only relative to the fault twin in the same session. -DCN1_GC_NO_DEMAND_SIGNAL re-injects both halves and scenario 6 requires them to fail. Measured by the gate itself: fixed onDemandShare=1.00 dutyPct=85.8 meanParkUs=16325 faulted onDemandShare=0.00 dutyPct=39.8 meanParkUs=220427 run-gauntlet.sh GREEN (nine tortures byte-identical to the host JVM, GcStress and MtStress in both stop modes), run-gc-verify.sh GREEN including both fault self-tests, and the other eight vm/tests integration tests pass. cn1_globals.m compile-checked for real iOS arm64 in the release shape and under CN1_GC_CONFORM, CN1_GC_NO_DEMAND_SIGNAL, CN1_GC_VERIFY and CN1_PACING_NO_RESERVE; check-native-signatures.sh reports 0 fatal. Also here, because it cost an investigation: translate-and-build.sh cached vm/JavaAPI classes on a presence check alone and never invalidated them. A cache predating Thread.sleep(long) becoming Java still declared it native, so the translator emitted a call to java_lang_Thread_sleep___long that nothing defined -- an undefined-symbol link error in generated code with no hint that a stale directory was the cause. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up to the demand-signal fix, which by making the collector actually run
exposed what a cycle spends its time on. Under the legacy-heavy shape a 159ms
mark was 39% grace pass, 36% conservative-root snapshot and 19% per-thread
drain -- against a 1.5MB LIVE SET with a 1.9M-slot legacy table.
THE PER-THREAD DRAIN. gcMarkDrain ends every call with a linear walk of
allObjectsInHeap that re-pushes each already-marked object so its mark function
runs again. That is an OVERFLOW recovery: gcMarkObject pushes every object it
marks that has a mark function, and gcMarkWorklistPush drops a push only on
overflow, which sets the sticky gcMarkOverflowSeen. With no overflow, draining
the worklist to empty IS the fixed point. But the walk ran unconditionally, on
every one of the (threads + 3 + SATB rounds) calls a cycle makes.
Measured before the gate: 8.9 passes per cycle over the table, 16.5 MILLION
slot visits and 290,000 mark functions re-run per cycle -- and across 883
passes it found something new exactly ZERO times. The BiBOP half of the same
loop has always been gated on overflow ("First time we observe an overflow,
start also rescanning page slots"); the legacy half simply never was. It is now.
Worth +27% throughput and -21% footprint on that shape.
-DCN1_GC_ALWAYS_RESCAN_LEGACY restores it, and scenario 8 requires the restored
walk to visit slots and still find nothing -- which is what makes the gate's
soundness argument testable rather than asserted.
THE SNAPSHOT. cn1ConsExt is rebuilt and re-sorted every cycle and the qsort
alone was 34ms of a 57ms build. The comment above it lists replacing "the libc
qsort (a function-pointer comparator call per compare) with an inlined/radix
sort" as one of three directions; this is that one, as an in-place introsort
(median-of-3, insertion sort under 16, heapsort past 2*log2(n), recursion on the
smaller side only). A radix sort would be O(n) but needs a scratch buffer the
size of the array -- 18MB at the measured extent count -- and footprint is the
other half of this issue.
An ordering bug there is silent: the array backs the binary search that resolves
INTERIOR pointers, so it does not crash, it returns the wrong object and the
collector frees something live. CN1_CONS_EXT_SORT_TEST compares the replacement
against qsort element for element across the shapes that break naive quicksorts
(random, sorted, reverse, all-equal, few-distinct, clustered) at seven sizes,
and CN1_GC_VERIFY builds re-check sortedness after every sort. 1.28x qsort on
the same data.
THE SEARCH IN FRONT OF IT. That binary search is the last resort in
cn1ConservativeResolve, reached only for a word that is neither a BiBOP address
nor a legacy object base -- i.e. almost always for something that is not a heap
pointer at all. Measured: 2,298,610 searches over 748,000 extents with ZERO
hits. A Bloom filter over the 64KB address block now answers those in one load
(1.5M searches -> 66k). It can say "maybe" for a block it does not hold, which
only costs the search that would have run anyway; it can never say "no" for a
block it holds, which is the direction that would be a use-after-free, and
CN1_GC_VERIFY builds run the search it skipped and abort if it was ever wrong.
-DCN1_CONS_EXT_NO_BLOOM.
A MUTATOR COULD WAIT A WHOLE COLLECTION FOR TABLE SPACE. Legacy allocations go
into a per-thread pending table that only the collector empties, at mark start.
When it filled, the thread waited for any RUNNING cycle to finish, then
requested another and waited for that one too -- when a running cycle is
precisely the thing that migrates the table. It now asks once and waits only for
the migration, bounded, falling through to growing the table if the bound
expires so bounding it cannot overflow anything.
Both thresholds involved are derived from a single free-RAM reading taken at the
first collection, which is tens of millions of slots on any machine CI runs on
and small on a memory-tight device -- the same "testable nowhere CI can run"
shape that let the rest of #5537 survive five fixes. CN1_SIMULATE_FREE_MEMORY
now pins that reading too, exactly as CN1_SIMULATE_PROC_MEMORY_LIMIT pins the
process budget. With it pinned to a device-like 16MB the WORST pending-table
stall goes from 1579ms to 136ms and the mean from 169ms to 52ms.
-DCN1_GC_PENDING_WAIT_FULL_CYCLE.
TRIED AND REJECTED, recorded because it looked obviously right: also treating a
parked mutator as demand in gcIdleWaitMillis. A thread parked because the
process BUDGET is exhausted is waiting for memory that collecting will not
produce, and counting it as demand ran the collector back-to-back at 100% and
starved the threads it was serving -- the -DCN1_PACING_NO_RESERVE arm under a
simulated ceiling stopped finishing its fixed round count at all. The counter is
kept for the duty-cycle figure; the idle decision does not read it.
GATES. GcSteadyStateIntegrationTest goes from 6 scenarios to 11, each with a
compiled-out fault twin, and the legacy-path pair translate a second copy of the
fixture with one constant rewritten -- the churn cannot be on for everyone,
because those throwaway arrays multiply the legacy population by twenty and
scenario 2 expresses the SATB budget PER LIVE OBJECT with that population as its
denominator. Turning it on globally would not have found a defect, it would have
made an existing gate unfalsifiable.
Scenario 2's fault twin is re-expressed on the log's TOTAL size for a related
reason: satbRefsPerLiveObject divides by the cycle count, and answering the
demand signal roughly tripled that, so the same unfiltered barrier spread the
same log over three times as many cycles and measured 2.4 against a threshold of
4. The fixed arm's per-cycle budget is unchanged -- that is the assertion that
states the property -- and the twin is now checked at five orders of magnitude
instead of one.
fixed duty 93.0% meanStall 10.7ms rescanSlots 0 maxPending 136ms
faulted duty 62.3% meanStall 206ms rescanSlots 121,566,892 maxPending 1579ms
(faulted rescan re-ran 106,165,318 mark functions and found 0)
run-gc-verify.sh's second self-test also stops conflating three outcomes: it
reported BROKEN once with a message accusing the fix, and re-running the same
binary twelve times gave earlyFreed=13452 and exit 0 every time. It now
distinguishes "the fault did not fire" from "the faulted run died before it
could report" and prints the evidence.
VERIFIED: run-gauntlet.sh GREEN, run-gc-verify.sh GREEN with both fault
self-tests, all 11 gate scenarios pass, vm/benchmarks geomean 0.999 over 13
interleaved reps with bit-identical checksums, and cn1_globals.m compile-checked
for real iOS arm64 in the release shape and under all ten macro arms.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ier gap (issue #5537) Three things, all from investigating the BiBOP grace pass -- the largest item left in a mark and the one I had said needed its own investigation. A DEADLOCK, introduced by the demand-signal fix. Under a simulated per-process ceiling the fixed-round fixture wedged: collector frozen in codenameOneGCMark's while(t->threadActive) safepoint wait, all four workers inside cn1PacingPark, 80MB uncollected, nothing progressing for ten minutes. True master ran the same workload in 19 seconds. java_lang_System_gc__ enters synchronized(LOCK), and monitorEnter is a GC SAFEPOINT. The budgeted pacing wait re-requests a collection every 200ms from INSIDE its park -- after publishing threadActive = FALSE -- so the request takes the thread back out of the parked state the collector is spinning on, and can block it in the monitor while the collector waits for it to go quiescent. A circular wait. The calloc-failure path had the same shape. The window is not new. It survived because the collector used to sit inside LOCK.wait() with the monitor released; answering the demand signal removed that idle and made it acquire LOCK once per cycle at several hundred cycles a second, which turned a theoretical race into a reliable hang. Both sites now set forceGc directly (cn1RequestGcFromParkedThread) -- a plain store the collector re-reads at the top of every loop pass, entering no Java at all. The only thing lost is the notify, and only while the collector is inside LOCK.wait(), which when a mutator is parked on the pacing cap is the 200ms high-frequency wait. Ablation macros bisected this badly: every arm hung sometimes, which reads as "not this one" for each in turn. `sample` on the wedged process named it in one shot. 18 of 18 soak runs under the ceiling now complete in ~20s. THE GRACE PASS ITSELF IS NOT CHANGED, and that is the result. Instrumented (gracePagesWalked/graceSlotsWalked/graceSlotsFresh/graceMarked) it turns out to be efficient, not wasteful: the gcAllocedSinceSweep prune skips ~9,950 pages per cycle and walks ~1,900, and 82-91% of the slots it touches are genuinely fresh. The cost is 828,000-1,520,000 gcMarkObject calls per cycle at ~35ns each -- the pass treats the entire fresh generation as roots, because the sweep's one-cycle grace keeps every fresh object whether or not it is reachable and an OLD object reachable only through one of them would otherwise be swept under it. The optimization that would work is allocate-black: objects allocated while the SATB barrier is armed need no tracing, because every reference stored into them is logged by the insertion half -- which exists for exactly that case. Skipping them would drop ~60-70% of the walk. It depends on the barrier being COMPLETE, and auditing that found two bulk copies of object references that bypass the per-element setter and so fire no insertion barrier at all: java_lang_System_arraycopy on an object array (deletion half only) and cloneArray (neither half). Both are fixed here; the fix is free (geomean 0.994 against the ablation) because it only runs during a mark. Then the part that decided it: THE VERIFIER CANNOT SEE THIS WINDOW. Two purpose-built drivers -- single-threaded and four-threaded, ~100 verify passes each, the destination made unreachable so only the grace rule keeps it -- report violations=0 WITH the barrier deliberately compiled out. The window is real by inspection and too narrow for any gate here to open. Making the grace pass depend on an invariant no gate can falsify would trade a measured 50% of mark time for a correctness risk that surfaces as silent heap corruption in a customer app days later with no reproducer, so it is not here. BulkCopyBarrier joins the verifier's driver list because it exercises both bulk paths; it is deliberately NOT a self-test, because a self-test that cannot fail is worse than none. The fixture's ROUNDS goes 24 -> 44. Making the collector faster made the fixed round count finish in nine seconds, and MIN_WALL_ROWS then failed on the SUCCESS; the right answer is to lengthen the workload, not to lower the floor. VERIFIED: run-gauntlet.sh GREEN, run-gc-verify.sh GREEN with both fault self-tests and the new driver, all 11 gate scenarios pass, 18/18 soaks under the ceiling complete, vm/benchmarks geomean 0.996 over 13 interleaved reps with bit-identical checksums, and cn1_globals.m + nativeMethods.m compile-checked for real iOS arm64 across seven macro arms. fixed duty 74.3% meanStall 22ms rescanSlots 0 maxPending 51ms faulted duty 35.1% meanStall 219ms rescanSlots 155,600,819 maxPending 867ms Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…5537) BOTH REVIEW FINDINGS WERE CORRECT and are fixed as raised. 1. The native force-GC request was a data race. forceGc is a non-volatile Java static, so the translator emits plain storage for it; a plain store from a parked mutator against the collector's plain read is unordered, and nothing stops a compiler keeping the collector's copy. The Java System.gc() path does not have that problem only because it writes the field under synchronized(LOCK) -- which is the one monitor a parked thread must not enter. The parked path now has its own release/acquire flag (cn1GcNativeGcRequest) that gcIdleWaitMillis consumes alongside forceGc. 2. The calloc-failure path should not have been downgraded at all. The bug there was requesting the collection AFTER publishing threadActive = FALSE; moving the request ahead of the park is the whole fix, and once it is ahead of the park the REAL java_lang_System_gc__ is both safe and necessary. The bare flag does none of what that path needs: it does not notify, so a collector inside its 30s idle stays there, and it does not call startGCThread(), so a collector that was never started never starts. Without either, threadBlockedByGC is still false, the wait falls straight through, and the function retries the allocation with nothing collected. Restored. The retry itself is left alone and now says so in place: it is a tail call that every optimised build turns into a jump, it is unchanged from before this work, and bounding it is a separate question about what a VM with no way to fail an allocation should do when it truly runs out. STOP ASSERTING THE RUNNER. CI failed on scenario 6, this change's own gate, and the failure was the gate's fault rather than the code's: it demanded the faulted arm stall 2x longer and the two-core runner measured 1.74x -- on a run where the two arms completed 947 and 946 cycles. Four workers on two cores leave the collector CPU-saturated rather than demand-starved, so answering the demand signal cannot shorten a park that is already just "one cycle"; a developer machine measures 10x for the same code. That is asserting the runner, which is exactly what scenario 3's comment in this file argues against. Scenarios 6 and 11 now assert the SIGN of the difference -- the faulted arm must not be faster, which is a property of the collector on any machine -- and REPORT the magnitude. What makes both twins non-vacuous is unchanged and mechanical: cyclesOnDemand is 0 in the faulted arm and non-zero in the fixed one, and scenario 10 still asserts the pending-table path is exercised at all. A replacement mechanism counter for scenario 11 was built, measured and REJECTED: collection epochs spanned per pending-table wait, on the theory that the old shape waits a running cycle out and then asks for another. It measured 1.04 against 1.00, because that shape's while(gcCurrentlyRunning) exits immediately whenever no cycle happens to be running. Removed rather than shipped inert, and recorded in the test so nobody rebuilds it. Verified: all 11 gate scenarios pass (stall ratio 11.71x, pending tail ratio 15.31x, both now reported), ProcessBudgetPacingIntegrationTest passes, run-gauntlet.sh GREEN, run-gc-verify.sh GREEN with both fault self-tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The review said the calloc-failure path "repeatedly consum[es] stack", and the comment I wrote dismissing that was WRONG. I claimed the retry was a tail call that every optimised build turns into a jump. It is not: CN1_GC_PARK_CAPTURE takes the address of a local a few lines above, which blocks the optimisation, and clang -O2 emits a plain `bl`. Checked in the generated assembly this time instead of asserted -- one recursive `bl` before, zero after. So a process that was genuinely out of memory answered by recursing until it ran out of stack as well. A backward goto does exactly what the recursion did -- same arguments, same re-execution of the counters and the class registration at the top of the function -- without the frame. The retry stays unbounded, because this VM has no way to fail an allocation; what changed is that failing repeatedly no longer costs stack. run-gauntlet.sh GREEN, run-gc-verify.sh GREEN with both fault self-tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…owed" on who asked (issue #5537) Both review findings hold. The second one is fixed differently from how it was raised, and better. BIND gcIdleWaitMillis IN THE JAVASCRIPT BACKEND. The JS port binds gcMarkSweep and isHighFrequencyGC explicitly; moving the collector's idle decision into a native added a third one it does not bind, so a JS-targeted app's GC thread would reach a missing-native stub on its first loop pass. It now returns 30000 -- the wait the Java code chose for exactly this case before the decision moved, which keeps that port's GC thread as idle as it has always been rather than spinning it every 200ms over a gcMarkSweep that is a no-op there. Worth recording: JavascriptNativeAuditTest passed throughout, because its inspector records no symbols at all ("intentionally records no uncategorized symbols in the ParparVM JS backend mode"). It is a gate that cannot fail, which is why this got through. Un-inerting it is not this change -- the blast radius is every uncategorized native in JavaAPI, not just the one added here -- but it is the reason a human review found this and CI did not. STOP GATING "A COLLECTION IS OWED" ON WHO ASKED. Review found a window: the request latch is cleared just AFTER bibopBytesSinceGc is zeroed in cn1BibopBeginGcCycle, so a collector descheduled between those two exchanges lets mutators cross the fresh trigger while the stale latch suppresses every CAS. Level-triggering normally retries that on the next page acquire -- but if the mutators have parked on the run-ahead cap by then there is no next allocation to do the retrying, and the collector idles with everyone blocked on it. That is the exact stall this change exists to remove. The suggested fix was a handshake between the counter and the latch. What is actually wrong is narrower: whether a collection is owed is a property of the BYTE COUNTERS, and it does not depend on anyone having remembered to ask. The uncollected >= trigger test moves out of the if(forced) branch and runs unconditionally, which closes that window and every other lost-request window with it, for one comparison the collector was making anyway. No handshake, no new state. It costs nothing: the trigger test is what made this cheap in the first place, so removing the forced gate does not reintroduce the 8-9% that answering every request unconditionally cost. vm/benchmarks geomean 1.003 against master over 11 interleaved reps, with objectAllocation 0.978, hashMapChurn 0.994 and stringBuilding 0.998 -- the three that would show it. Verified: all 11 gate scenarios pass (stall ratio 13.23x), ProcessBudgetPacing passes, every JavaScript test passes (audit, port smoke, cn1 core completeness, runtime facade), run-gauntlet.sh GREEN, run-gc-verify.sh GREEN with both fault self-tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#5537) Review finding: the out-of-memory retry waits only on threadBlockedByGC, which System.gc() does not set synchronously, so the loop falls through and retries at whatever rate it can -- worst in the two seconds after startGCThread(), whose first act is LOCK.wait(2000) and during which nothing collects at all. It was survivable before only because the retry recursed and ran out of stack; turning that into a loop removed the accidental brake. This path could not be reached on a developer machine -- macOS ignores `ulimit -v`, so calloc cannot be made to fail -- and it has now been the subject of two findings that could only be argued about. CN1_SIMULATE_ALLOC_FAILURES=<n> fails the next n legacy allocations, gated on CN1_GC_CONFORM like the rest of the QA instrumentation so no shipping build can be told to fail one. It found a crash on the FIRST injected failure, before any of the above: java_lang_System_gc__ reaches startGCThread(), which touches System's statics, and this was the one GC-trigger site in the file without the `constantPoolObjects != 0` guard that every other one carries. A real out-of-memory during bootstrap dereferenced null inside startGCThread. It now sleeps and retries instead, because there is nothing to collect that early. Then it measured the actual fix, 200 consecutive failures on one allocation: no delay 0.37s wall 0.17s CPU wait for the cycle 40.51s wall 0.33s CPU 10ms backoff 1.87s wall 0.20s CPU The first row says the concern is milder than it looks: 0.85ms of CPU per iteration, because the monitor round-trip in System.gc() already throttles it. The second row is what I wrote first, and it is worse than the problem -- a full collection per failed allocation, 100x slower to recover, because each retry sits through the collector's 200ms idle. The third is what shipped: a 10ms backoff caps the retry rate at a hundred a second and costs a hundredth of waiting for the cycle, and the loop still exits the moment a collection actually starts. -DCN1_GC_NO_ALLOC_WAIT removes the backoff for that A/B. Verified: run-gauntlet.sh GREEN, run-gc-verify.sh GREEN with both fault self-tests and BulkCopyBarrier clean, all 11 gate scenarios pass, ProcessBudgetPacing and GcHeapIntegrity pass, every ablation arm compiles. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review finding, and correct. The hook added last commit overloaded -1 as both "environment not probed yet" and a possible value of the counter, which does not survive two threads: both see a positive budget, one decrements 1 -> 0 and the other 0 -> -1, and the next call reads -1 as the sentinel, re-reads the environment and re-arms. A budget that silently refills invalidates the very experiment it exists for. A separate pthread_once initialiser now owns the probing, and the decrement is a CAS that only fires while the count is positive, so it cannot go below zero however many threads race. Checked against MtStress, which allocates from several threads: requested 25 / 100 / 400 failures, observed exactly 25 / 100 / 400 retries. That measurement is also the reason to re-state last commit's numbers rather than leave them resting on a racy counter. Re-run with this fix, 200 consecutive failures on one allocation: no delay 0.34s wall 0.13s CPU 10ms backoff 1.59s wall 0.14s CPU against 0.37/0.17 and 1.87/0.20 before, so the conclusion the backoff was chosen on stands unchanged. Every line of this is inside the CN1_GC_CONFORM block, so a shipping build is unchanged; run-gauntlet.sh GREEN and run-gc-verify.sh GREEN with both fault self-tests confirm it, and every ablation arm compiles. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ence cn1SatbEnqueue locks and unlocks gcSatbMutex for every reference it accepts. That is right for the per-store barrier the translator emits, and wrong for the two bulk paths this issue's grace-pass audit added a barrier to: cloning or arraycopying a large object array turned one memcpy into an acquisition per element, contending with the collector's own drain for the length of the array. It also got likelier with the demand-signal work, because a collector that runs when asked means a mark is in progress far more of the time. cn1SatbEnqueueRange filters unlocked and flushes 256 at a time. Chunked rather than one hold for the whole range: a single acquisition across a million-element array would block cn1SatbTake for the entire walk, trading many short stalls for one long one. Measured with BulkCopyCost -- 400 arraycopy+clone rounds over a 200,000-element Object[] of old, unfiltered references while a second thread keeps the collector busy -- five interleaved reps against -DCN1_SATB_NO_BULK, which restores the per-element shape: the copy loop's median goes 576ms to 494ms, and its spread tightens from 556-648 to 492-500. The collector side of that A/B reads like a regression and is not, which is why the numbers are in the code: markMs 477 to 648, satbMs 243 to 453, because the bulk arm logs twice the references (37.9M to 74.5M) -- a faster mutator gets through more copies inside the same mark. Per logged entry the drain costs 6.4ns before and 6.1ns after. Scope, checked rather than assumed: on the ordinary churn workload the log holds 0-6 entries a cycle, because the fresh-reference filter already rejects essentially everything. This path is neutral for normal code and matters only for the bulk copies that now go through it. Gauntlet green, gc-verify green with both fault self-tests firing, and the 11-scenario steady-state gate green. Issue #5537
shai-almog
force-pushed
the
gc-stall-5537
branch
from
August 28, 2026 17:09
a9c8859 to
9da3fef
Compare
cn1StallSumThreads excludes the GC thread now, but the numbers written down before that correction were never restated. threadRunner sets lightweightThread = JAVA_TRUE on every Java thread, the collector included, so a four-worker run divided the aggregate stall by six thread-seconds instead of five. Re-measured with the corrected denominator: the demand-signal headline is 38% -> 86%, not 51% -> 90%, and the legacy-heavy shape reaches ~53%, not ~55% (52.6 / 53.3 / 51.8 over three 25s reps at CN1_WL_BIGARRAY=256). Throughput, stall and footprint figures are unaffected -- the bug was only in the divisor. Records the trap next to the instrument, and flags that the branch's commit messages and the pull request description still carry the pre-correction pair. Issue #5537
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9da3fef9cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5c870f523
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…wn cleanly Three defects in the instrument this issue's headline rests on, and one window in the bulk barrier added alongside it. The stall clock died with the thread that earned it. It was summed from a per-thread counter over the LIVE threads, and markDeadThread() drops a TLD out of allThreads on exit, so the next sample's delta went negative, got clamped to zero, and the line reported 100% duty exactly at a thread-generation boundary -- then kept reporting it until the survivors climbed back past the vanished total. Measured at exit the live-thread walk returns 0 against a process-wide 9.4-35.2 SECONDS. The total now comes from the process-wide per-cause counters, which nothing removes; the per-thread field is deleted. That substitution is exact only because the collector never records a stall of its own, so this does not assume it: instrumenting cn1StallRecord to attribute by thread gives gcThreadNs=0 and nullTsRecords=0 on the churn, legacy-heavy and thread-churn shapes alike. All seven CN1_STALL_ADD sites are mutator paths. The "1Hz" line was not 1Hz. usleep returns early on EINTR and the signal-based thread stop delivers to the probe thread too, so the series collapsed to ~20ms windows -- and four threads accrue more stall than 20ms of one thread's wall clock, which is how the line came to print a NEGATIVE duty. It now sleeps in slices until a second of monotonic time has really passed, and integrates the live mutator count across those slices, so the denominator is thread-time rather than one end-of-window count times elapsed wall time. That integral is also what makes it right when threads come and go inside the window. The peak-thread track moves into the slice loop so a run shorter than one interval still reports. MutatorChurnDuty is the driver. Before: "threads=1 stallMs=0 duty=100.0" at a generation boundary, windows 20-150ms apart. After: a clean 1s cadence, no phantom, duty 25-38% throughout. The bulk barrier's flag check and its append are two steps, so the collector can clear gcSatbActive and run its final cn1SatbTake() between chunks and strand entries in a log this cycle never drains again. For a single store that window is argued harmless where the flag is cleared; for cloneArray it is not, because the copy publishes into a brand new array the grace pass has ALREADY walked past, so a dropped reference is one the sweep can free under a live pointer. cn1SatbBulkQuiesce closes it -- enqueuer registers before re-reading the flag, collector clears the flag then waits for zero before the final take, both sides seq_cst. Safe to spin on because nothing between register and deregister can block or reach a safepoint. 3.2%, inside this host's noise. -DCN1_SATB_NO_BULK_HANDSHAKE compiles it out. It deliberately does NOT close the same window on the per-store barrier, where the handshake would have to be an unconditional atomic on every reference store. Also corrects the previous commit's claim for the chunked enqueue. It quoted a 14% throughput win measured in a quieter session; interleaved medians over seven reps now put the two arms 3.7% apart, which this host cannot resolve -- the same reps threw 1323ms and 1016ms outliers against a ~650ms median. The stable measure is the new satbLocks counter: 1.155 enqueue-side mutex acquisitions per logged reference becomes 0.0055, 210x fewer. Both figures have to be normalised per logged reference, because the arms do not log the same amount. Gauntlet green, gc-verify green with both fault self-tests firing, all ten ablation arms compile. Issue #5537
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76126e22b5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
arraycopy logs two ranges -- the deletion half off the destination and the insertion half off the source -- and the previous shape registered each one separately with the SATB termination handshake. The in-flight count therefore fell to zero between them, which is a full re-opening of the window for the second half: the collector can clear gcSatbActive, observe zero, and finish its final drain in that gap, after which the insertion half logs nothing and the memmove publishes the source's references into the destination regardless. If the source is then dropped after the grace scan, the sweep can reclaim referents the destination still points at. The bracket now spans the whole barrier operation. cn1SatbBulkEnter() registers and then re-reads the flag, returning false when the mark has already terminated; cn1SatbEnqueueRangeLocked() logs one range and states in its name that the caller must hold the registration; cn1SatbBulkExit() releases it. The outer gcSatbActive test stays as the cheap predicted-not-taken pre-filter, with cn1SatbBulkEnter doing the authoritative registered re-check. This is also strictly cheaper: arraycopy now pays two atomics for the operation instead of four for the two ranges. Gauntlet green, gc-verify green with both fault self-tests firing, all ten ablation arms compile. Issue #5537
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4fa589394
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Clearing gcSatbActive and then running the closing catch meant gcMarkDrain could scan an object that catch had just discovered -- grey, and unwatched. A mutator moving an old child out of that grey object into a fresh container in the window logs nothing on either side: the drain then scans the object the child has already left, the grace pass is long past the destination, and the child ends up unmarked, not fresh, and reachable only from the fresh container, so the sweep takes it. The clear is now a TRIAL. If the catch comes back empty the snapshot is closed and the barrier stays down. If it does not, the barrier goes back UP before that batch is marked, so anything it discovers is scanned under a live barrier, and the fixpoint runs again. It terminates for the reason the inner fixpoint does: an outer pass repeats only when it marked something NEW, and marks are monotonic and bounded by the live set. The new satbReopens counter says how often that happens, so the question is answered by the instrument rather than by argument -- 0 on the churn workload, where the cost is one extra empty cn1SatbTake per cycle, and 1 on BulkCopyCost. That 1 is worth stating plainly: the window is reachable, not theoretical, and the bulk copy paths this issue added a barrier to are where it shows up. Gauntlet green, gc-verify green with both fault self-tests firing, ten ablation arms compile. Issue #5537
The window where a newly discovered object is scanned grey with the barrier already down, why the clear is provisional, and the measured re-arm counts that say it is reachable rather than theoretical.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: efa53aba1b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
cn1StallNs[] is the obvious process-wide total and the wrong one. cn1GcSignalHandler charges CN1_STALL_SIGNAL_STOP to whatever thread the signal interrupted, and under conservative roots that includes NATIVE threads -- which are not lightweightThread, so cn1StallSumThreads cannot count them and they contribute no thread-time. Their stalls therefore raised the numerator against a denominator that did not move. Measured by attributing every cn1StallRecord call: native threads are 1.9% and 4.2% of the total on the churn and thread-churn shapes, and 2.4% and 15.4% of it under CN1_GC_SIGNAL_STOP=1. That biases duty down, and codex is right that it can drive it negative on a native-thread-heavy workload. cn1StallMutatorNs replaces it -- still process-wide, so a thread exiting loses nothing, but charged only for lightweight, non-collector threads, so numerator and denominator describe one population. Both emitters use it; the per-cause table below still reports every stall the process took, including the native ones this figure deliberately leaves out. The collector is excluded by comparing thread-local POINTERS rather than resolving System.gcThreadInstance, because the filter runs inside a signal handler where an atomic load is safe and reaching into Java statics is not. cn1StallSumThreads publishes that pointer as it walks, since it has to identify the thread anyway. With the populations matched the two stop modes agree -- 81.9/81.9/82.9 cooperative against 81.4/82.2 under signal stop, where the signal-stop arm previously carried the whole 15.4%. The headline pair is re-derived on the corrected instrument, interleaved, median of three: 37% -> 85%, against the 38% -> 86% quoted before, so the correction does not move it materially -- it biases both arms alike. Gauntlet green, gc-verify green with both fault self-tests firing. Issue #5537
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1fdd221eb2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The reopened epoch had a shortcut: if the caught batch marked nothing new, the barrier came down and the loop exited without taking again. Entries logged while the barrier was back up were therefore never taken at all, so an unmarked non-fresh reference stored into a live or fresh container during that window could be swept. The shortcut is gone -- re-arm, drain, and go round to the fixpoint again, and leave only when a trial clear produces an empty catch. That means saying where the regress ends, because it does not end on its own. Every take leaves a window after it in which a store can still log, so "drain what was logged during the last drain" has no fixed point a concurrent collector can reach without holding the mutators still -- which is the stop-the-world pause this collector exists to avoid. It converges in practice because a cleared flag stops mutators logging within one barrier's worth of instructions and cn1SatbBulkQuiesce holds the bulk writers outright. CN1_SATB_MAX_REOPENS bounds the loop only against a mutator storming references, and reaching it falls back on the invariant the sweep has always relied on: a reference stored after the mark reaches its fixpoint is already marked or fresh, and the sweep keeps both. Measured rather than argued, worst case per cycle via satbReopens: 0 on the churn workload in both stop modes and on the legacy-heavy shape, and 4 on BulkCopyCost -- up from 1, which is the shortcut's removal doing exactly what it should. The cap is 32, set well above that rather than just above it, because reaching it silently substitutes the weaker invariant and the headroom is the whole point. Gauntlet green, gc-verify green with both fault self-tests firing, nine ablation arms compile. Issue #5537
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 926dc96d69
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two findings on the latency instrument and the idle decision behind it. Reading the native request and entering LOCK.wait(idle) back in Java are two steps with nothing joining them. A park's in-park re-request that lands between them sets the flag on a collector that is already asleep, and it cannot notify -- a parked thread must not enter a Java monitor, which is the deadlock cn1RequestGcFromParkedThread exists to avoid. With the byte demand and the high-frequency test both quiet, which is exactly what a parked mutator produces because it allocates nothing, the collector then slept 30 SECONDS on a request already standing. The regime-B comment predicted this failure and the periodic re-request was its mitigation; this is the race that loses the re-request. gcIdleWaitMillis now refuses the long idle while a pacing park is recent, which costs a lost request 200ms instead of 30s. It returns a SHORT IDLE, not 0: returning 0 whenever a mutator was parked is the documented mistake -- a thread parked on the process BUDGET waits for memory collecting will not produce, and treating it as demand ran the collector back to back at 100%. A short idle only re-reads the request sooner and forces no cycle. This bounds the race rather than closing it, and the comment says so. Closing it means moving the collector's sleep off the Java monitor onto something a parked thread may signal, and that is the one mechanism that has already deadlocked this collector twice in this issue alone. Recency stamp rather than a parked-thread gauge, because cn1PacingPark has five early returns and a counter leaking on any of them would pin the collector at the short idle for the rest of the run. A stamp cannot leak; it ages out. Separately, the whole-run duty figure divided by peak threads times wall time, which assumes every thread that ever existed did so for the entire run. On a population that changes the denominator is too big and the duty too high, hiding the pauses the instrument exists to report. It now divides by the accumulated thread-time integral the probe loop already computes for the 1Hz line, and prints it as threadMs so the two can be compared. Honest about the size: on these workloads the populations are nearly constant and it moves duty 0.1-0.6pp; it matters where a population actually varies. It falls back to the old approximation only when the probe thread never ran, with the peak count printed beside it so a reader can tell which they have. Gauntlet green, gc-verify green with both fault self-tests firing. Both ceiling scenarios still complete under a simulated 768MB budget (23s and 30s) with identical RESULT. Issue #5537
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06d7afc3ec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
gcSatbActive is not a safe thing to sample outside the handshake. Mark termination clears it and puts it back up during the trial-clear protocol, so an object-array copy that read 0 in that window skipped the barrier entirely and then published its references into a mark the collector reopened a moment later -- registering first is the whole point of the increment-and-recheck protocol, and consulting the flag outside it defeats it. gcSatbTerminating stays up across every trial and drops only once the mark is genuinely over, after the last quiesce, so the pair is never both zero while a mark can still reopen. The call sites test both and cn1SatbBulkEnter re-reads both while REGISTERED, which is what makes the answer authoritative. Kept as a precheck rather than entering the handshake unconditionally because that would put three seq_cst atomics on every arraycopy and clone in the program; two relaxed loads is what the off-mark path pays now. Also records, at the reopen bound, why the textbook fallback is not available here. Review asked for a brief stop-the-world instead of the barrier-down drain. This collector cannot do that: codenameOneGCMark pauses only lightweightThread states and says why -- a native thread is never waited for -- and native threads mutate references, which is the reason the SATB barrier exists at all (CN1_SATB_DELETE: it covers native threads, "which thread-pausing structurally cannot"). A stop-the-world remark there would be weaker than the barrier it replaced, and dead code besides, reached only in a state measurement says never occurs. The note also says why draining beats leaving the batch: draining loses an object only if a mutator moves a reference out of one specific object during one specific scan, where not draining loses it with certainty. Gauntlet green, gc-verify green with both fault self-tests firing, eight ablation arms compile. satbReopens unchanged at 4 worst case on BulkCopyCost and 0 on the churn workload, and BulkCopyCost under the heap verifier is clean over three runs (violations, earlyFreed, resurrectedDangling all 0). Issue #5537
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b474db565
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two findings, one shipped and one in the instrument. Mark startup armed gcSatbActive and walked on. A bulk copy that had already passed the flag check decided not to log, so the rest of its memcpy published references into a destination the mark could walk past with nothing logged for them; if the source was then dropped, the sweep took them. Native threads make that reachable rather than theoretical -- they are never cooperatively paused, so nothing else in the mark waits for them. codenameOneGCMark now calls cn1SatbBulkQuiesce after arming, the same handshake termination already used, and the flag precheck at the call sites is gone: sampling gcSatbActive outside the protocol is unsafe at BOTH ends of a mark, and registering before reading it is the entire point of increment-and-recheck. Removing the precheck was free. ObjCopyCost -- 40 million object-array copies with no allocation, so no collection runs and every copy takes the entry protocol and finds the mark inactive -- measures 625ms median either way, interleaved over five reps, against -DCN1_SATB_NO_BULK_HANDSHAKE for the arm without the atomics. The cost lands only on OBJECT arrays anyway: cls->primitiveType rejects the byte[]/char[] traffic that dominates arraycopy before any atomic executes. The duty denominator was still sampled. Polling the live population once per 100ms slice missed any thread that both started and exited inside one slice -- its stalls stayed in the numerator while its lifetime was never counted, which understates duty and can drive it negative on short thread bursts. It is now computed exactly: each thread stamps gcThreadStartMs at registration, markDeadThread banks its lifetime as it exits, and the integral is that plus the live threads' time so far. No polling interval appears in the answer, and it no longer depends on the probe thread having run. Sanity check on the trivially-correct case: a stable 4-worker run reports threadMs=125486 against 5 threads x 25102ms = 125510. Gauntlet green, gc-verify green with both fault self-tests firing, eight ablation arms compile. Issue #5537
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2249bed1de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The bracket released before the memmove, and the "nothing to log" path released immediately. Either way the copy itself ran unregistered, so a mutator could observe both SATB flags clear, deregister, and then publish while the collector armed the mark, saw zero in cn1SatbBulkQuiesce and began scanning the destination. References written after that scan were logged by nobody, and if the source was then dropped the sweep took them. Native threads make it reachable: nothing else in the mark waits for them. Registration is what the quiesce waits on, so it has to span the publication. cn1SatbBulkBegin now registers unconditionally and only REPORTS whether logging is needed; cn1SatbBulkEnd is called after the memmove/memcpy either way. Mark startup and mark termination therefore both block until every in-flight copy has finished publishing, and no scan can interleave with one. Checked rather than assumed, because a leaked registration would hang the collector outright: both brackets are straight-line, with no early return, no allocation and no safepoint between Begin and End -- cloneArray's allocArray happens before the bracket opens, and arraycopy's two argument-check returns are well above it. Still free on the off-mark path: ObjCopyCost's 40 million object-array copies measure the same as before the change, and primitive arrays skip the bracket entirely since they publish no references. Gauntlet green, gc-verify green with both fault self-tests firing, nine ablation arms compile, and BulkCopyCost completes three times in each stop mode -- the case that would hang if a registration ever leaked. Issue #5537
The previous commit said ObjCopyCost "measures the same as before the change". It does not, and the way that slipped through is worth recording next to the number: the comparison was against a median from an EARLIER session, which is exactly the comparison the measurement notes in vm/CLAUDE.md say never to make on this host. Interleaved in one session, seven reps, against -DCN1_SATB_NO_BULK_HANDSHAKE for the arm without the atomics: 634ms against 651ms, so 2.7%. As a magnitude that is under this host's 5% resolution, but the sign is not in doubt -- the registered arm was slower in all seven pairs, which a coin would manage about once in 128 tries. The change stays. 2.7% is the worst case on a loop that does nothing but copy object arrays, primitive arrays never reach the bracket at all, and what it buys is closing a window through which the sweep can reclaim a live object. Issue #5537
Contributor
✅ ByteCodeTranslator Quality ReportTest & Coverage
Benchmark Results
Static Analysis
Generated automatically by the PR CI workflow. |
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.
Closes the latency half of #5537.
After five merged fixes (#5540, #5563, #5573, #5585, #5599) the reporter's build had no leak and no crashes, but was still unusable: "the pauses for GC become very frequent and very long." Nothing in the runtime measured a pause, so nothing could have caught this —
[GCPROBE]times the collector,waitMsis its inverse, and[PACING]/[LOWMEM]count parks without recording a duration. A build could stop every worker for most of a run with every gate green.The instrument came first and named the cause in one run: a 40 ms mark against a 212 ms mutator park.
What was wrong
The collector was told not to collect.
bibopBytesSinceGcis zeroed at cycle start, so under sustained churn a mutator re-crosses the trigger throughout every cycle — andcn1BibopMaybeGcdiscarded every one of those crossings behind a!gcCurrentlyRunninggate. By the time a cycle ended, all mutators were parked on the run-ahead cap and allocating nothing, so nobody was left to ask.forceGcwas false and the GC thread took its 200 ms idle wait with the whole application blocked on it. The legacy trigger had already solved this with a per-cycle latch and says so in its own comment; the BiBOP side never got the same treatment.gcMarkDrainre-marked the whole legacy table on every call. It is an overflow recovery —gcMarkObjectpushes everything it marks, so with no overflow the worklist is the fixpoint — but it ran on all(threads + 3 + SATB rounds)calls a cycle makes: 16.5 M slot visits and 290 K mark functions re-run per cycle, finding something new 0 times in 883 passes.A mutator could wait a whole collection for pending-table space. It waited for a running cycle to finish, then asked for another — when the running cycle is exactly what migrates the table.
Two bulk copies bypassed the SATB insertion barrier:
System.arraycopyon an object array (deletion half only) andcloneArray(neither). That barrier exists specifically to keep an object alive when the container it is stored into is a fresh grace object.The extent snapshot: the per-cycle
qsortwas 34 ms of a 57 ms build, and the binary search behind it ran 2.3 M times with zero hits.Measured
Footprint falls — this does not trade memory for latency. Under a simulated 1.4 GB ceiling (the reporter's iPad regime): +16 %, duty 88 -> 93 %.
Two hangs this work exposed, both fixed here
Removing the collector's idle woke a dormant race:
java_lang_System_gc__enters a Java monitor andmonitorEnteris a GC safepoint, so re-requesting a collection from inside a park takes the thread back out of the parked statecodenameOneGCMarkis spinning on. Circular wait. Then fixing that lost the notify, which needed both a pre-park request and restoring the guarantee that a consumedforceGcnever drops to the 30 s idle.Both are recorded in
vm/CLAUDE.md, along with the deliberate decision not to ship allocate-black grace narrowing: it is the remaining ~50 % of a mark, I fixed the two barrier gaps it would need, and then established that the verifier cannot falsify the resulting invariant — two purpose-built drivers reportviolations=0with the barrier compiled out.Gates
GcSteadyStateIntegrationTestgoes from 6 scenarios to 11, each with a compiled-out fault twin. Assertions are on the mechanism (cyclesOnDemand/cyclesAfterIdle,rescanSlots), which mean the same thing on a slow runner; outcomes are asserted only relative to the twin in the same session.Verified locally:
run-gauntlet.shGREEN,run-gc-verify.shGREEN with both fault self-tests and a newBulkCopyBarrierdriver, all 9 benchmark-tagged tests pass, 520 non-benchmark tests pass,check-native-signatures.sh0 fatal,check-cast-semantics.shclean,vm/benchmarksgeomean 1.009 over 11 interleaved reps with bit-identical checksums, andcn1_globals.m+nativeMethods.mcompile-checked for real iOS arm64 across seven macro arms plus clang and gcc.What is not verified
The reporter's own app has never been run against this.
ddyer0/boardspace.netcannot be built from GitHub —Sthread.javaandbridge/BSClip.javaare referenced byboardspace coreand exist nowhere in the repo. Everything here rests on the synthetic workload and the fixture, which is the same structural gap that let the previous five fixes through. Windows and Linux are covered by CI only.🤖 Generated with Claude Code