Stop one compute-only thread from freezing the whole VM (issue #5537) - #5631
Conversation
The mark phase stops each lightweight thread cooperatively -- it raises threadBlockedByGC and spins on while(t->threadActive) -- and the translator emits no safepoint polls in generated code, neither on method entry nor on loop back-edges. Every safepoint lives inside a runtime function: the allocator handshakes, contended monitorEnter, Thread.sleep/Object.wait and the native-call bracket. cn1BibopMaybeGc is reached once per 64KB PAGE, not per object. So a Java loop that allocates nothing new and enters no contended monitor reaches no safepoint at all, and that spin never ends. It is not a slow GC, it is a whole-VM freeze: every other thread parks at its next allocation waiting for a cycle that can never start. The reporter's debugger caught the collector at totalwait = 609491500 microseconds -- 10 minutes 9 seconds -- while a game-tree search ran a compute-only evaluation loop. Bound the spin at CN1_GC_SAFEPOINT_WAIT_MAX_US (250ms) and past it freeze the thread with the same SIGUSR2 stop the collector already uses for genuine native threads, retried on the same cadence because that stop can time out on a descheduled handler. Windows keeps the unbounded spin: it has no POSIX signals, and scanning a running thread and then sweeping under it is worse than a hang. A thread frozen wherever it happened to be, rather than at a point it chose, constrains what the rest of the iteration may do, and each constraint is argued at its site: nothing may allocate while the freeze is held (the root snapshot is built before it and skipped at both later call sites), the pending-allocation table is not migrated (the pending[size]=o; size++ window would orphan an object and then hand its slot back out), the freeze is released as soon as roots are captured rather than after the mark drain because a signal-frozen thread busy-spins where a parked one sleeps, and a thread already frozen must not be signalled again. Also fix the diagnostic that was supposed to catch this and never could: time(0) is in SECONDS, and the code compared the elapsed value against 10000 and printed it divided by 1000, so the warning first became eligible after 2.8 hours and would have understated by a factor of 1000. The ten-minute freeze above printed nothing. totalwait was an int as well, which is signed overflow at about 36 minutes of waiting -- reachable only by the wedge the counter exists to report. gcMarkForcedStop is initialized explicitly because ThreadLocalData is malloc'd and never zeroed; garbage there would have told the scanner a running thread was frozen, scanned it from a garbage SP, and never stopped it. GcUncooperativeThreadIntegrationTest gates both halves and rebuilds the same translated project with -DCN1_GC_NO_FORCE_STOP to prove it can fail. One 6s compute-only spin against a churning allocator, idle host: the ablation arm stalls a mutator 6140ms of a 6151ms spin, this build 383ms of 6052ms. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 8c147c0c1c
ℹ️ 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".
| #if defined(__OBJC__) | ||
| NSLog(@"[GC] force-stopped thread %d after %lldus at a safepoint it never reached (%ld so far)", | ||
| (int)t->threadId, totalwait, cn1GcForcedStops); | ||
| #else | ||
| fprintf(stderr, "[GC] force-stopped thread %d after %lldus at a safepoint it never reached (%ld so far)\n", | ||
| (int)t->threadId, totalwait, cn1GcForcedStops); |
There was a problem hiding this comment.
When the escalation catches a thread inside malloc, NSLog, or a write to stderr, the stopped thread may own allocator or logging locks. Calling NSLog/fprintf here while that thread remains frozen can wait for the same lock and permanently wedge the collector—the failure this change is intended to prevent. Record the diagnostic data while stopped, but emit it only after cn1GcMarkReleaseForced().
Useful? React with 👍 / 👎.
| @@ -2283,7 +2371,19 @@ void codenameOneGCMark() { | |||
| long long __mg0 = cn1GcNowNs(); | |||
| #endif | |||
| lockCriticalSection(); | |||
There was a problem hiding this comment.
A force-stop can arrive at any instruction, including after the target enters markDeadThread() or another runtime path holding criticalSection; there is also a race between the last threadActive read and signal delivery. In that case this blocking lock acquisition waits on a mutex whose owner cannot run until cn1GcMarkReleaseForced(), but release is reached only after this section, so GC deadlocks. The forced path must not acquire this mutex while the target is suspended (or must release and revalidate the target before doing so).
Useful? React with 👍 / 👎.
|
Compared 163 screenshots: 163 matched. Benchmark ResultsDetailed Performance Metrics
|
|
Compared 163 screenshots: 163 matched. Benchmark ResultsDetailed Performance Metrics
|
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Cloudflare Preview
|
|
Compared 163 screenshots: 163 matched. |
|
Compared 163 screenshots: 163 matched. |
|
Compared 163 screenshots: 163 matched. Benchmark ResultsDetailed Performance Metrics
|
|
Compared 181 screenshots: 181 matched. |
✅ ByteCodeTranslator Quality ReportTest & Coverage
Benchmark Results
Static Analysis
Generated automatically by the PR CI workflow. |
|
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
|
Compared 149 screenshots: 149 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
|
Compared 217 screenshots: 217 matched. |
|
Compared 144 screenshots: 144 matched. |
|
Compared 143 screenshots: 143 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Two P1 review findings on the escalation, both correct, plus a third instance of the same class that the review did not flag. All three are the rule the change states for cn1GcBuildRootSnapshots -- nothing may block on a lock the stopped thread might own -- applied inconsistently: the rule was written and then broken three lines below it. A thread force-stopped by signal halts at an arbitrary instruction, so it can own the libc allocator's lock, stdio's, os_log's, or criticalSection. Blocking on any of those before cn1GcMarkReleaseForced makes the collector wait on a thread that cannot run until the collector releases it: the same permanent wedge this change exists to remove, only rarer and harder to place. "It is only held briefly" is not a defence -- the target can enter that code in the window between the last threadActive read and the signal landing. - The escalation's own NSLog/fprintf ran under the freeze. The counter values are captured there now and printed after the release. - The pending-migration block took criticalSection unconditionally and guarded only its body, which deadlocks just as thoroughly. The forced path skips the lock as well as the body. - The aggressive-allocator hold calls get_free_memory() and NSLog, and the EDT is an ordinary candidate for the escalation -- a long computation on the event thread is the shape issue #5537 reported. Skipped on the forced path; its allocSize input describes a table this cycle deliberately did not migrate anyway. Every step between freeze and release is now enumerated and classified at the top of the per-thread block, so the next addition there has to answer the question rather than rediscover it. GcUncooperativeThreadIntegrationTest unchanged: 336ms of a 5955ms spin (0.06) against 6575ms of 6571ms (1.00) for the -DCN1_GC_NO_FORCE_STOP ablation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb71d94c6d
ℹ️ 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".
| if(base == 0 || ssz == 0) return; | ||
|
|
||
| #ifdef CN1_GC_CAN_FORCE_STOP | ||
| if(t->gcMarkForcedStop) { |
There was a problem hiding this comment.
On Linux this forced-stop branch is reached only after the target is suspended, but cn1GcScanThreadNativeStack() first calls cn1GcStackBase(), whose pthread_getattr_np() implementation uses internal locks and performs realloc/free. If the signal stopped the target inside malloc or while it held the relevant pthread lock, the collector can deadlock here before reaching cn1GcMarkReleaseForced(), recreating the VM freeze this escalation is intended to prevent. Cache the stack bounds before cn1GcMarkForceStopUncooperative() and reuse them in this branch.
Useful? React with 👍 / 👎.
Third finding of the same class, and the most instructive one: I classified cn1GcStackBase as safe to call under a held freeze after reading only its Apple branch, which is pthread_get_stackaddr_np plus pthread_get_stacksize_np -- two plain accessors. The Linux branch of the same function is pthread_getattr_np, which mallocs (and reads /proc/self/maps for the initial thread) and frees again through pthread_attr_destroy. Calling it while the target is frozen can block on the allocator lock the target owns, which is the deadlock the previous commit removed from three other sites. A cross-platform helper has to be classified on its WORST platform. Checking one spelling and letting the conclusion cover the rest is how this reached the "SAFE, lock-free" line of the enumeration; the enumeration now says so. cn1GcMarkForceStopUncooperative resolves the bounds while the thread is still running and stashes them on the TLD -- they cannot change for a live pthread -- and cn1GcScanThreadNativeStack's forced branch moves ABOVE the cn1GcStackBase call so the frozen path never reaches it. Unresolvable bounds now decline the freeze rather than take one: without them the conservative scan could not read the thread's native stack even once stopped, so freezing would skip its roots for nothing. gcSigStackBase existed but was never read; it is now the pre-freeze base, with gcSigStackSize alongside it. Both are explicitly initialized, because ThreadLocalData is malloc'd and never zeroed. Note CI runs parparvm-tests on ubuntu-latest and passed twice with the defect present -- it is a race, so a green Linux run was never evidence of absence. GcUncooperativeThreadIntegrationTest unchanged: 365ms of a 6193ms spin (0.06) against 6092ms of 6081ms (1.00) for the ablation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a8ddbfbdf
ℹ️ 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".
| cn1GcBuildRootSnapshots(); | ||
| forcedStop = cn1GcMarkForceStopUncooperative(t); |
There was a problem hiding this comment.
When the forced stop catches a mutator inside malloc, the subsequent precise/native root scans call gcMarkObject, and with the default CN1_ADOPT_POLICY == 1 a surviving non-leaf BiBOP root reaches cn1MatureObject, whose adoption-buffer growth calls realloc. In particular, the first adoption uses this path because the buffer capacity starts at zero, so the collector can block on the allocator lock held by the frozen mutator and never reach cn1GcMarkReleaseForced(). Preallocate/defer adoption or otherwise ensure root marking cannot allocate while this freeze is held.
Useful? React with 👍 / 👎.
Fourth finding of the same class, and it refutes the claim the previous two commits leaned on. I asserted the root scans are malloc-free having checked gcMarkWorklistPush (fixed array) and cn1ForceVisitedTestAndSet (force path only, which no root scan takes) and stopped there. gcMarkObject has a third allocating edge: with the default CN1_ADOPT_POLICY == 1 a surviving non-leaf BiBOP root reaches cn1MatureObject, whose adoption buffer grows by realloc -- and gcAdoptCap starts at zero, so the first adoption of the process always takes it. Checking two of three paths and generalising is the same mistake as reading one platform's branch of cn1GcStackBase. This one is NOT introduced here. Master already marks between cn1GcSignalStopOne and cn1GcSignalReleaseOne in cn1GcScanThreadNativeStack, so the collector could already hang against a frozen NATIVE thread; the escalation widens the exposure a long way, because lightweight threads now take that path, far more often, and with the precise object stack scanned under the freeze as well. Both sites are fixed. cn1GcFreezeHeld is raised for the duration of either freeze, and cn1MatureObject declines BEFORE its claim CAS when the flag is up and the buffer would have to grow. Declining before the CAS is the whole point: claiming and then bailing is what the existing OOM path does, and it leaves the object flagged -4 and unregistered forever, which is a leak, because the CAS can never fire again. A declined object stays -3, is still marked and traced this cycle (the worklist push below is unconditional), and simply graduates in a later one. The buffer is also given headroom before each freeze, while allocating is still legal, so the decline should stay rare. GcUncooperativeThreadIntegrationTest 348ms of a 6015ms spin (0.06) against 5847ms of 5865ms (1.00) for the ablation; GcHeapIntegrity and GcOverflowSpiral green, which is where an adoption mistake would show. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ded343848
ℹ️ 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".
| if(forcedStop) { | ||
| cn1GcMarkReleaseForced(t); | ||
| forcedStop = JAVA_FALSE; |
There was a problem hiding this comment.
When the documented -DCN1_DISABLE_SATB fallback is used, this release lets the mutator resume before its grey root worklist is drained even though both insertion and deletion barriers compile to no-ops. The resumed thread can read a child from a captured root into a local and clear the root's field; the drain then misses the child, while the pre-release stack snapshot cannot contain the new local, allowing sweep to reclaim an object the thread is still using. Either retain the freeze through gcMarkDrainParallel() in this configuration or exclude force-stop support when SATB is disabled.
Useful? React with 👍 / 👎.
| assertTrue(fixed.output.contains(FORCE_STOP_MARKER), | ||
| "The collector never reported a forced stop, so the spinner parked on its own" | ||
| + " and this run did not exercise the escalation at all. Output: " | ||
| + tail(fixed.output)); |
There was a problem hiding this comment.
On Windows, CN1_GC_CAN_FORCE_STOP is explicitly not defined and the runtime intentionally retains the unbounded cooperative wait, so the fixed arm can never emit FORCE_STOP_MARKER; this assertion therefore deterministically fails whenever the benchmark group is run on Windows, regardless of the implementation's correctness on supported POSIX targets. Guard the test with a Windows/platform-capability assumption or restrict it to platforms where signal stopping is compiled.
Useful? React with 👍 / 👎.
…Windows (review) Two P2 review findings, both correct. The early release is justified by the SATB deletion barrier -- that is what lets a released mutator stay sound before the drain, and it is already the only thing keeping genuine native threads honest, since they are never blocked at all. The argument was stated and then not guarded: under the documented -DCN1_DISABLE_SATB ablation both barriers compile to no-ops and it evaporates. The resumed thread can read a child out of a captured root into a local the pre-release stack snapshot cannot contain, clear the field, and have the sweep reclaim an object it is still using. That build now keeps the freeze until after gcMarkDrainParallel, which costs the busy spin the early release exists to avoid -- the right trade in an ablation build, and none at all in a shipping one. The deferred log moves to the later point so it stays after the release in both. The gate asserted the force-stop marker unconditionally. CN1_GC_CAN_FORCE_STOP is deliberately undefined on Windows -- no POSIX signals, so the runtime keeps the unbounded cooperative wait there -- which made the assertion fail on Windows for a reason that says nothing about the code under test. Skipped by assumption rather than weakened: the assertions are what make it a gate, and the platform that can satisfy them is the platform the feature exists on. Compiles clean on -DCN1_DISABLE_SATB along with the other five arms. Gate unchanged: 367ms of a 5967ms spin (0.06) against 6274ms of 6256ms (1.00). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4174c6557f
ℹ️ 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".
| // worklist is a fixed-size array; the force-visited side table | ||
| // is only touched on the force path, which no root scan takes). | ||
| cn1GcBuildRootSnapshots(); | ||
| forcedStop = cn1GcMarkForceStopUncooperative(t); |
There was a problem hiding this comment.
When CN1_NURSERY is enabled and a minor collection lasts past the 250 ms escalation threshold, cn1NurseryMinorCollect() deliberately leaves threadActive true while setting t->nurseryPromoting. This line can therefore freeze it mid-collection, after which the precise/native root scans call gcMarkObject(t, ...); because t->nurseryPromoting is true, gcMarkObject() takes its minor-GC branch and returns without globally marking any non-nursery object. The following global sweep can then reclaim mature objects that are live only from this thread. Avoid escalating while the target is performing its minor collection, or ensure forced root marking uses collector rather than target nursery state.
Useful? React with 👍 / 👎.
The previous commit wrapped the early release in #if !defined(CN1_DISABLE_SATB) and left the old unconditional-release comment stranded above the new one, so the block carried two descriptions of itself and the stale one no longer matched the code under it. Verified comment-only: preprocessing both revisions with -fpreprocessed -dD -E -P and diffing them is empty. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f47025f23e
ℹ️ 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".
| if(forcedStop) { | ||
| cn1GcMarkReleaseForced(t); | ||
| forcedStop = JAVA_FALSE; |
There was a problem hiding this comment.
When built with the documented -DCN1_DISABLE_SATB fallback, this conditional omits the release and leaves the mutator frozen across markStatics(d) and gcMarkDrainParallel(d). Those paths are not allocation-free: generated markStatics() calls gcMarkObject(..., JAVA_TRUE), whose force-visited table can call malloc, and the first parallel drain lazily creates worker threads with pthread_create. If the signal caught the target while it held the allocator or pthread-internal lock, the collector can block here and never reach the later cn1GcMarkReleaseForced(), recreating a permanent VM wedge. Reserve/initialize these resources before freezing or otherwise keep the no-SATB frozen interval allocation-free.
Useful? React with 👍 / 👎.
…SATB is off (review) Two more findings. The first is a use-after-free this change introduced; the second is one my previous fix introduced while fixing something else. NURSERY. The root scans mark through gcMarkObject(t, ...) -- the TARGET's thread state, not the collector's. cn1NurseryWriteBarrier raises nurseryPromoting and deliberately leaves threadActive TRUE for the whole minor collection, which makes such a thread a prime candidate for a 250ms escalation; and under that flag gcMarkObject's first act is to promote-or- return WITHOUT marking. Freezing there hands the sweep a thread whose roots were every one of them silently skipped, and mature objects live only from it are reclaimed. A cooperatively parked thread never has the flag set, which is why this could not happen before. cn1GcMarkForceStopUncooperative now declines such a thread -- checked AFTER the stop, because a read taken while the thread still runs can be raised in the window before the signal lands, whereas a frozen thread's flag cannot change. SATB. The previous commit answered "the early release is only sound because of the barrier" by holding the freeze through the drain when the barrier is compiled out. That is worse: it drags markStatics -- which force-marks, and so reaches the force-visited table's malloc -- and gcMarkDrainParallel's lazy pthread_create inside a window where the frozen thread may own the allocator or pthread lock. A wedge in the middle of the fix for a wedge. The frozen window has to stay small and enumerable and a full parallel drain is neither, so -DCN1_DISABLE_SATB now simply does not get the escalation and keeps master's unbounded cooperative wait, which is the behaviour that ablation exists to measure against. The release and the deferred log go back to one site each, and the dependency is enforced in the CN1_GC_CAN_FORCE_STOP guard instead of being asserted in a comment. Clean on eight compile arms including -DCN1_NURSERY and -DCN1_NURSERY -DCN1_DISABLE_SATB. Gate 355ms of a 5843ms spin (0.06) against 7046ms of 7025ms (1.00); GcHeapIntegrity green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Follow-up to #5609 on issue #5537, and a different root cause: the reporter's
last comment ("the GC is waiting for this thread. This thread is compute bound
and will never yield") is correct.
What was wrong
The mark phase stops each lightweight thread cooperatively -- it raises
threadBlockedByGCand spins onwhile(t->threadActive)-- and the translatoremits no safepoint polls in generated code, neither on method entry nor on loop
back-edges. Every safepoint lives inside a runtime function, and
cn1BibopMaybeGcis reached once per 64KB page, not per object.A Java loop that allocates nothing new and enters no contended monitor therefore
reaches no safepoint at all, and that spin never ends. It is not a slow GC, it is
a whole-VM freeze: every other thread parks at its next allocation waiting for a
cycle that can never start. The reporter's debugger caught the collector at
totalwait = 609491500microseconds -- 10 minutes 9 seconds -- while a game-treesearch ran a compute-only evaluation loop.
The diagnostic meant to catch exactly this was dead for its whole life:
time(0)is in seconds, and the code compared the elapsed value against
10000and printedit divided by
1000, so it first became eligible after 2.8 hours and would haveunderstated by 1000x. That freeze printed nothing.
What this does
Bounds the spin at
CN1_GC_SAFEPOINT_WAIT_MAX_US(250ms) and past it freezes thethread with the same SIGUSR2 stop the collector already uses for genuine native
threads. Windows keeps the unbounded spin -- no POSIX signals, and scanning a
running thread and then sweeping under it is worse than a hang.
The reasoning for each constraint a signal freeze imposes is in comments at the
site it applies to, plus a new section in
vm/CLAUDE.md.Measured
GcUncooperativeThreadIntegrationTest, one 6s compute-only spin against achurning allocator, idle host. Thresholds are ratios of the workload's own two
measurements, not wall-clock constants.
-DCN1_GC_NO_FORCE_STOP(ablation)The gate requires the ablation arm to reproduce the wedge, so it cannot go inert.
Not in this change
monitorEnter's first-creation branch locks withthreadActivestill TRUEwhere the contended branch parks first -- a genuine three-way deadlock that
this escalation rescues on POSIX and not on Windows. Different bug, hot path,
wants its own change and gate. Documented in
vm/CLAUDE.md.answer and costs throughput in every loop the VM ever runs.
Note on local testing
GcSteadyStateIntegrationTest's 768MB-ceiling scenario fails on a high-core-countdeveloper machine and passes in CI (2837s there). A/B'd on an idle host: master
859.9s, this branch 883.5s -- both fail identically, so it is pre-existing and
unrelated. That scenario's dynamics depend on the mutator/collector core ratio,
as its own comments note.
🤖 Generated with Claude Code