From 8c147c0c1c832e512f8d3f5860baccff82585b62 Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Sun, 30 Aug 2026 17:16:18 +0300 Subject: [PATCH 1/7] Stop one compute-only thread from freezing the whole VM (issue #5537) 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) --- vm/ByteCodeTranslator/src/cn1_globals.h | 55 +++ vm/ByteCodeTranslator/src/cn1_globals.m | 193 ++++++++- vm/ByteCodeTranslator/src/nativeMethods.m | 6 + vm/CLAUDE.md | 73 ++++ .../GcUncooperativeThreadIntegrationTest.java | 405 ++++++++++++++++++ .../translator/GcUncooperativeThreadApp.java | 192 +++++++++ 6 files changed, 914 insertions(+), 10 deletions(-) create mode 100644 vm/tests/src/test/java/com/codename1/tools/translator/GcUncooperativeThreadIntegrationTest.java create mode 100644 vm/tests/src/test/resources/com/codename1/tools/translator/GcUncooperativeThreadApp.java diff --git a/vm/ByteCodeTranslator/src/cn1_globals.h b/vm/ByteCodeTranslator/src/cn1_globals.h index 79072bfc220..a75cbca8861 100644 --- a/vm/ByteCodeTranslator/src/cn1_globals.h +++ b/vm/ByteCodeTranslator/src/cn1_globals.h @@ -93,6 +93,53 @@ #endif #endif +// ========================================================================= +// UNCOOPERATIVE-MUTATOR ESCALATION (issue #5537) +// ========================================================================= +// The mark phase stops each lightweight thread by setting threadBlockedByGC and +// then spinning on threadActive until the thread parks itself. That handshake is +// purely COOPERATIVE, and ParparVM emits no safepoint polls in generated code -- +// not on method entry, not on loop back-edges (grep the translator for +// threadBlockedByGC: no hits). Every safepoint lives inside a runtime function: +// the codenameOneGcMalloc handshake, cn1BibopMaybeGc (reached only once per BiBOP +// PAGE, not per object), contended monitorEnter, Thread.sleep/Object.wait and the +// CN1_YIELD_THREAD native bracket. +// +// So a Java loop that allocates nothing new and enters no contended monitor never +// reaches a safepoint, and the collector's spin never ends. That 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. Issue #5537 caught it in the debugger with the +// collector 10 minutes 9 seconds into that spin (totalwait = 609,491,500us) while a +// game-tree search thread ran a compute-only evaluation loop. +// +// The escalation bounds the spin and then FREEZES the thread with the same SIGUSR2 +// stop the collector already uses for genuine native threads (cn1GcSignalStopOne), +// which needs no cooperation at all. Only available where that machinery is +// compiled: conservative roots on, and not Windows (no POSIX signals -- there the +// spin stays unbounded, exactly as today, because proceeding without stopping the +// thread would miss its roots and free live objects). +// +// The proper long-term answer is a safepoint poll on loop back-edges in the +// translator, which costs throughput in every loop the VM ever runs; this makes the +// pathological case survivable without paying that everywhere. +// +// -DCN1_GC_NO_FORCE_STOP restores the unbounded cooperative spin. It is the ablation arm +// GcUncooperativeThreadIntegrationTest builds to prove the gate can fail -- without a +// build that still wedges, an assertion that the VM does not wedge proves nothing. +#if defined(CN1_CONSERVATIVE_GC_ROOTS) && !defined(_WIN32) && !defined(CN1_GC_NO_FORCE_STOP) +#define CN1_GC_CAN_FORCE_STOP 1 +#endif + +// How long the cooperative safepoint handshake may spin before escalating, in +// microseconds. A thread that is going to park does so in microseconds, so this is +// pure headroom -- it only has to exceed the longest legitimate gap between a +// mutator's safepoints, which is one BiBOP page (64KB) of allocation or one +// unbracketed native call. Raising it lengthens the freeze in the pathological case +// and buys nothing in the normal one. +#ifndef CN1_GC_SAFEPOINT_WAIT_MAX_US +#define CN1_GC_SAFEPOINT_WAIT_MAX_US 250000 +#endif + //#define DEBUG_GC_ALLOCATIONS #define NUMBER_OF_SUPPORTED_THREADS 1024 @@ -1234,6 +1281,14 @@ struct ThreadLocalData { void* volatile gcSigStackBase; // [sp,base) high bound (filled by GC/handler) char gcSigRegs[4096]; // raw copy of the interrupted ucontext (GPRs) volatile sig_atomic_t gcSigRegsLen; // valid bytes in gcSigRegs + // Set while the MARK LOOP owns a signal freeze it took because this thread would + // not reach a safepoint (see CN1_GC_CAN_FORCE_STOP). It tells + // cn1GcScanThreadNativeStack to reuse that freeze rather than take its own: a + // second SIGUSR2 aimed at a thread already spinning inside the handler stays + // pending until the handler returns, so the nested stop would spin out its whole + // timeout and then report failure on a thread that is in fact frozen. GC-thread + // owned, cleared by cn1GcMarkReleaseForced. + JAVA_BOOLEAN gcMarkForcedStop; #endif #ifdef CN1_GC_CONFORM // Monotonic ms at which this thread was registered. The duty denominator is the diff --git a/vm/ByteCodeTranslator/src/cn1_globals.m b/vm/ByteCodeTranslator/src/cn1_globals.m index bfcd4be756b..3cebed7b3d6 100644 --- a/vm/ByteCodeTranslator/src/cn1_globals.m +++ b/vm/ByteCodeTranslator/src/cn1_globals.m @@ -1605,6 +1605,12 @@ static void cn1DrainDeadThreadPending() { static void cn1GcScanOwnStack(CODENAME_ONE_THREAD_STATE); static void cn1GcSignalStopThreads(struct ThreadLocalData* self); static void cn1GcSignalReleaseThreads(struct ThreadLocalData* self); +#ifdef CN1_GC_CAN_FORCE_STOP +// Escalation for a lightweight thread that will not reach a safepoint (issue #5537). +// See the definitions next to cn1GcSignalStopOne for the contract. +static JAVA_BOOLEAN cn1GcMarkForceStopUncooperative(struct ThreadLocalData* t); +static void cn1GcMarkReleaseForced(struct ThreadLocalData* t); +#endif void cn1GcBuildRootSnapshots(void); JAVA_OBJECT cn1ConservativeResolve(void* w); #ifdef CN1_CONSERVATIVE_GC_SELFCHECK @@ -2223,6 +2229,10 @@ void codenameOneGCMark() { } if(t != d) { struct elementStruct* objects = t->threadObjectStack; + // TRUE once the cooperative handshake below gave up and froze this + // thread with a signal instead. It changes what the rest of this + // iteration may touch -- see each use. + JAVA_BOOLEAN forcedStop = JAVA_FALSE; #ifdef CN1_CONSERVATIVE_GC_ROOTS // PHASE 3b: demand a FRESH native-stack capture this round. Only a @@ -2239,8 +2249,19 @@ void codenameOneGCMark() { // we don't have much control and who barely call into Java anyway if(t->lightweightThread) { t->threadBlockedByGC = JAVA_TRUE; - int totalwait = 0; + // 64-bit: at 500us a spin, an int overflowed after ~36 minutes of + // waiting, and signed overflow is undefined -- the one input that + // can reach it is precisely the wedge this loop is trying to report. + long long totalwait = 0; long now = time(0); + long lastReport = 0; +#ifdef CN1_GC_CAN_FORCE_STOP + // Next totalwait at which to try the escalation. A running total + // rather than a modulo: CN1_GC_SAFEPOINT_WAIT_MAX_US is overridable, + // and `totalwait % bound == 0` silently never fires for any bound that + // is not a multiple of the 500us step. + long long nextEscalation = (long long)CN1_GC_SAFEPOINT_WAIT_MAX_US; +#endif #ifdef CN1_GC_CONFORM // Opened and closed around the safepoint wait ALONE. Closing it later // -- after the migration and the stack scans -- would fold their cost @@ -2252,16 +2273,83 @@ void codenameOneGCMark() { while(t->threadActive) { usleep(500); totalwait += 500; - if((totalwait%10000)==0) - { long later = time(0)-now; - if(later>10000) - { + // REPORTING, fixed. time(0) is in SECONDS; this compared the + // elapsed value against 10000 and then printed it divided by + // 1000, so the warning first became eligible after 2.8 HOURS and + // would have understated what it found by a factor of 1000. The + // collector in issue #5537 sat here for 10 minutes 9 seconds + // (totalwait 609,491,500us, read out of the debugger) and never + // printed a line -- the one diagnostic aimed at this failure was + // silent for the whole of it. Report from 10 seconds on, once + // every 10, naming the thread while it is still stuck. Kept live + // even where the escalation below exists, because that escalation + // can itself fail and this is then the only thing that says so. + if((totalwait % 100000) == 0) { + long later = time(0) - now; + if(later >= 10 && later - lastReport >= 10) { + lastReport = later; #if defined(__OBJC__) - NSLog(@"GC trapped for %d seconds waiting for thread %d in slot %d (%d)", - (int)(later/1000),(int)t->threadId,iter,t->threadKilled); + NSLog(@"[GC] trapped for %ld seconds waiting for thread %d in slot %d (killed=%d)", + later, (int)t->threadId, iter, (int)t->threadKilled); +#else + fprintf(stderr, "[GC] trapped for %ld seconds waiting for thread %d in slot %d (killed=%d)\n", + later, (int)t->threadId, iter, (int)t->threadKilled); #endif } } +#ifdef CN1_GC_CAN_FORCE_STOP + // ESCALATION. Past the bound this thread is not going to park -- + // typically because it is running generated code that reaches no + // safepoint at all (see CN1_GC_CAN_FORCE_STOP in cn1_globals.h). + // Freeze it with the signal stop instead, which needs no + // cooperation. RETRIED on the same cadence rather than attempted + // once: cn1GcSignalStopOne can time out on a descheduled handler, + // and one transient failure must not sentence the collector to the + // unbounded wait for the rest of the cycle. Each failed attempt + // already costs about its own timeout, so the retry rate is + // self-limiting. gcPthreadValid is the one PERMANENT reason the + // stop cannot work, and retrying past it would rebuild the root + // snapshot every 250ms for the rest of a wait that is already + // never going to end. + if(totalwait >= nextEscalation && t->gcPthreadValid) { + nextEscalation += (long long)CN1_GC_SAFEPOINT_WAIT_MAX_US; + // The snapshot rebuild MUST happen here, BEFORE the freeze: it + // reallocs, and a thread frozen mid-malloc holds the libc + // allocator lock. Everything this iteration does while the + // freeze is held is malloc-free for the same reason (the mark + // 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); + if(forcedStop) { + // Reported on the 1st, 2nd, 4th, 8th ... escalation of the + // process. Self-limiting without a rate-limit clock, and + // the running count still tells you the order of magnitude + // -- one escalation is a thread that happened to sit in a + // long native, thousands is a program with a compute loop + // in it, and that difference is what a reader needs. Not + // behind an env var: a collector that had to shoot a + // mutator to make progress is abnormal, and the reason + // issue #5537 took so long to place is that this loop had + // nothing to say for itself. + static long cn1GcForcedStops = 0; // GC thread only + cn1GcForcedStops++; + if((cn1GcForcedStops & (cn1GcForcedStops - 1)) == 0) { +#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); +#endif + } + // threadActive is still TRUE and stays that way -- the + // thread is frozen, not parked. Everything below reads + // forcedStop rather than threadActive for that reason. + break; + } + } +#endif } #ifdef CN1_GC_CONFORM cn1GcWaitNs += cn1GcNowNs() - __wt0; @@ -2283,7 +2371,19 @@ void codenameOneGCMark() { long long __mg0 = cn1GcNowNs(); #endif lockCriticalSection(); - if(allThreads[iter] == t) { + // A force-stopped thread is SKIPPED here, and its heapAllocationSize is + // left alone below. The append is `pending[size] = o; size++`, so a thread + // frozen between those two stores has an object in the table that the + // count does not cover yet: migrating [0,size), zeroing those slots and + // resetting size to 0 would leave that object referenced by nothing the + // collector ever walks, and the thread's own `size++` on resume would then + // hand the next allocation a slot the migration had already emptied. The + // table is not a root source -- it only decides which objects the SWEEP + // may consider -- so leaving it intact for one cycle costs a deferred + // reclaim and nothing else. It also cannot grow without bound: a table + // over its threshold parks its own thread at a safepoint, which is the + // cooperative stop this escalation was standing in for. + if(allThreads[iter] == t && !forcedStop) { if (!t->lightweightThread) { // For native threads, we need to actually lock them while we traverse the // heap allocations because we can't use the usual locking mechanisms on @@ -2328,14 +2428,21 @@ void codenameOneGCMark() { } - t->heapAllocationSize = 0; + if(!forcedStop) { + t->heapAllocationSize = 0; + } int stackSize = t->threadObjectStackOffset; #ifdef CN1_CONSERVATIVE_GC_ROOTS // Refresh the page/extent snapshot for the VALIDATED precise scan // below (also rebuilt in cn1GcScanThreadNativeStack before any // signal-stop; building here first only makes it fresher). - cn1GcBuildRootSnapshots(); + // NOT while a forced stop is held: this reallocs, and the frozen thread + // may hold the allocator lock. The escalation built the snapshot before + // it froze the thread, so the one this would refresh already exists. + if(!forcedStop) { + cn1GcBuildRootSnapshots(); + } #endif #ifdef CN1_GC_VERIFY { extern const char* cn1GcMarkPhase; cn1GcMarkPhase = "precise-thread-stack"; } @@ -2403,6 +2510,24 @@ void codenameOneGCMark() { #ifdef CN1_GC_CONFORM cn1GcStackNs += cn1GcNowNs(); #endif +#ifdef CN1_GC_CAN_FORCE_STOP + // Release the forced freeze the moment this thread's roots are captured, + // which is HERE and not at the threadBlockedByGC clear further down. A + // cooperatively parked thread waits out the mark drain in a usleep; a + // signal-frozen one waits in an async-signal-safe BUSY spin, so holding it + // across the drain would burn a core for the length of a full mark. What + // the drain-before-unblock is protecting -- snapshot-at-the-beginning -- + // is supplied for a released mutator by the SATB deletion barrier, which + // is armed for the whole mark and is already the only thing keeping + // genuine native threads honest; they are never blocked at all, and + // cn1GcScanThreadNativeStack releases its own signal stops at exactly this + // point for the same reason. threadBlockedByGC stays set, so if this + // thread does reach a safepoint it still parks. + if(forcedStop) { + cn1GcMarkReleaseForced(t); + forcedStop = JAVA_FALSE; + } +#endif #ifdef CN1_CONSERVATIVE_GC_SELFCHECK cn1GcSelfCheckThreadStack(t, stackSize); #endif @@ -8219,6 +8344,33 @@ static void cn1GcSignalReleaseOne(struct ThreadLocalData* t) { #endif } +#ifdef CN1_GC_CAN_FORCE_STOP +// Freeze a lightweight thread that would not reach a safepoint within +// CN1_GC_SAFEPOINT_WAIT_MAX_US (issue #5537 -- see CN1_GC_CAN_FORCE_STOP in +// cn1_globals.h for why such a thread exists at all). Returns JAVA_TRUE once the thread +// is parked inside the SIGUSR2 handler, and the CALLER then owns that freeze until +// cn1GcMarkReleaseForced -- including the obligation to run nothing that allocates while +// it is held, because the thread can have been frozen mid-malloc. The root snapshots must +// already be built for the same reason. +static JAVA_BOOLEAN cn1GcMarkForceStopUncooperative(struct ThreadLocalData* t) { + if(cn1GcSignalStopOne(t) == 0) { + // Timed out or the thread has no valid pthread. cn1GcSignalStopOne has already + // pre-released the generation, so nothing is left stranded. + return JAVA_FALSE; + } + t->gcMarkForcedStop = JAVA_TRUE; + return JAVA_TRUE; +} + +static void cn1GcMarkReleaseForced(struct ThreadLocalData* t) { + if(!t->gcMarkForcedStop) { + return; + } + t->gcMarkForcedStop = JAVA_FALSE; + cn1GcSignalReleaseOne(t); +} +#endif + // Scan ONE thread's native C stack [sp, base) + its register snapshot, marking every // resolved live object. threadStateData = the GC thread; t = the thread being scanned. static void cn1GcScanThreadNativeStack(CODENAME_ONE_THREAD_STATE, struct ThreadLocalData* t) { @@ -8227,6 +8379,27 @@ static void cn1GcScanThreadNativeStack(CODENAME_ONE_THREAD_STATE, struct ThreadL char* base = cn1GcStackBase(t->gcPthread, &ssz); if(base == 0 || ssz == 0) return; +#ifdef CN1_GC_CAN_FORCE_STOP + if(t->gcMarkForcedStop) { + // The mark loop already froze this thread (the cooperative handshake timed out) + // and still owns the freeze. Reuse its capture: do NOT stop again -- a second + // SIGUSR2 to a thread spinning inside the handler stays pending until the handler + // returns, so the nested stop would spin out its full timeout and then report + // failure on a thread that is demonstrably stopped -- and do NOT release, because + // the precise object-stack scan the caller runs alongside this needs the same + // freeze. Also no cn1GcBuildRootSnapshots here: it reallocs, the caller built the + // snapshot before freezing, and a frozen thread may hold the allocator lock. + char* fsp = (char*)t->gcSigStackPointer; + if(fsp != 0 && fsp >= base - (long)ssz && fsp < base) { + cn1ConservativeMarkRange(threadStateData, fsp, base); + } + if(t->gcSigRegsLen > 0) { + cn1ConservativeMarkRange(threadStateData, t->gcSigRegs, t->gcSigRegs + t->gcSigRegsLen); + } + return; + } +#endif + // Snapshot rebuilt BEFORE any signal-stop (realloc-while-frozen would deadlock). cn1GcBuildRootSnapshots(); diff --git a/vm/ByteCodeTranslator/src/nativeMethods.m b/vm/ByteCodeTranslator/src/nativeMethods.m index 35354f8196d..8df74848c7a 100644 --- a/vm/ByteCodeTranslator/src/nativeMethods.m +++ b/vm/ByteCodeTranslator/src/nativeMethods.m @@ -1831,6 +1831,12 @@ JAVA_INT java_lang_Object_hashCode___R_int(CODENAME_ONE_THREAD_STATE, JAVA_OBJEC i->gcSigStopGen = 0; i->gcSigStackPointer = 0; i->gcSigRegsLen = 0; + // ThreadLocalData is malloc'd, NOT zeroed (see the notes on nativeStackLimit and + // bibopBytesLocal above). Garbage-nonzero here would tell + // cn1GcScanThreadNativeStack that the mark loop already froze this thread, so it + // would scan a RUNNING thread's stack from a garbage SP and never signal-stop it + // -- missed roots, then a use-after-free on whatever the sweep took. + i->gcMarkForcedStop = JAVA_FALSE; cn1TlsSelf = i; #endif pthread_setspecific(threadIdKey, i); diff --git a/vm/CLAUDE.md b/vm/CLAUDE.md index 2cb98ab271b..0d483dcaf84 100644 --- a/vm/CLAUDE.md +++ b/vm/CLAUDE.md @@ -341,6 +341,79 @@ If this is ever revisited, the thing to build FIRST is a way to drive an allocat residual window on purpose -- the phases after the grace walk and before `gcSatbActive` is cleared -- because without that, no version of this change can be validated. +### There are no safepoint polls in generated code + +The mark phase stops each lightweight thread cooperatively: it raises `threadBlockedByGC` +and spins on `while(t->threadActive)` until the thread parks itself. **Nothing in the +translator emits a safepoint poll** -- not on method entry, not on loop back-edges; grep +`vm/ByteCodeTranslator/src/com/codename1/tools/translator` for `threadBlockedByGC` and +there are no hits. Every safepoint in this VM lives inside a *runtime function*: + +- `codenameOneGcMalloc`'s handshake (legacy allocations), +- `cn1BibopMaybeGc`, reached **once per 64KB PAGE**, not per object -- a thread bumping + inside a page it already holds passes no safepoint, +- contended `monitorEnter`, +- `Thread.sleep` / `Object.wait`, and the `CN1_YIELD_THREAD` native bracket. + +`monitorEnter`'s **first-creation** branch is the odd one out and is worth fixing on its +own: it `pthread_mutex_lock`s with `threadActive` still TRUE, where the contended branch +right below it parks first. A thread that publishes the monitor into the side table, drops +the critical section and is then beaten to the mutex by a second thread -- which parks and +holds it across the GC handshake -- blocks there while still counted active, which is a +three-way deadlock (collector waits for it, it waits for the mutex, the holder waits for the +collector). The escalation below rescues that on POSIX and does not on Windows. Left alone +here deliberately: it is a different bug on a hot path and wants its own change and gate. + +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. +Issue #5537's reporter caught it in the debugger with the collector `totalwait = +609491500` microseconds -- **10 minutes 9 seconds** -- into that spin while a game-tree +search ran a compute-only evaluation loop. + +`CN1_GC_SAFEPOINT_WAIT_MAX_US` (250ms) bounds the spin, and past it the collector freezes +the thread with the **same SIGUSR2 stop it already uses for genuine native threads** +(`cn1GcSignalStopOne`), which needs no cooperation. Measured by +`GcUncooperativeThreadIntegrationTest`, one 6s compute-only spin against a churning +allocator: **maxStall 6186ms of a 6187ms spin without it, 339ms of 5948ms with it**. The +ablation arm is `-DCN1_GC_NO_FORCE_STOP`, and the gate requires it to reproduce the wedge. + +Four things the escalation has to respect, all of them consequences of a thread being +frozen wherever it happened to be rather than at a point it chose: + +- **Nothing may allocate while the freeze is held.** The thread can be frozen mid-`malloc` + holding the libc allocator lock. `cn1GcBuildRootSnapshots` reallocs, so it runs BEFORE + the freeze and is skipped at both of its usual call sites for the rest of that thread's + iteration. The scans themselves are malloc-free (fixed-size mark worklist; the + force-visited side table is only touched on the `force` path, which no root scan takes). +- **The pending-allocation table is not migrated.** The append is `pending[size] = o; + size++`, so a thread frozen between the two stores has an object the count does not + cover; migrating and resetting `size` to 0 would orphan it and then hand its slot back + out. The table is not a root source -- it only decides what the SWEEP may consider -- so + skipping it for a cycle costs a deferred reclaim and nothing else. +- **The freeze is released as soon as the roots are captured**, not at the + `threadBlockedByGC` clear. A parked thread waits in `usleep`; a signal-frozen one waits + in an async-signal-safe BUSY spin, so holding it across the mark drain would burn a core. + SATB is what makes an early release safe, and it is already the only thing keeping + genuine native threads honest. +- **Do not signal a thread that is already frozen.** A second SIGUSR2 aimed at a thread + spinning inside the handler stays pending until the handler returns, so a nested stop + spins out its whole timeout and then reports failure on a thread that is demonstrably + stopped. `gcMarkForcedStop` tells `cn1GcScanThreadNativeStack` to reuse the capture. + +Windows has no POSIX signals, so there the spin stays unbounded -- proceeding without +stopping the thread would miss its roots and free live objects, which is worse than a hang. +The proper long-term answer is a back-edge poll in the translator; it costs throughput in +every loop the VM ever runs, and this makes the pathological case survivable without paying +that everywhere. + +**The diagnostic aimed at exactly this was dead for its whole life.** The spin's warning +computed `long later = time(0) - now` -- SECONDS -- then tested `later > 10000` and printed +`later / 1000` as "seconds". It first became eligible after 2.8 hours and would have +understated by 1000x, so the ten-minute freeze above printed nothing and had to be read out +of a debugger. `totalwait` was also an `int`, which is signed overflow at ~36 minutes of +waiting. If an instrument has never been seen firing, assume it does not. + ### Never call into Java from a parked thread `java_lang_System_gc__` enters `synchronized(LOCK)`, and `monitorEnter` is a GC safepoint. diff --git a/vm/tests/src/test/java/com/codename1/tools/translator/GcUncooperativeThreadIntegrationTest.java b/vm/tests/src/test/java/com/codename1/tools/translator/GcUncooperativeThreadIntegrationTest.java new file mode 100644 index 00000000000..0b88ceeb201 --- /dev/null +++ b/vm/tests/src/test/java/com/codename1/tools/translator/GcUncooperativeThreadIntegrationTest.java @@ -0,0 +1,405 @@ +/* + * Copyright (c) 2012, Codename One and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. Codename One designates this + * particular file as subject to the "Classpath" exception as provided + * by Oracle in the LICENSE file that accompanied this code. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Codename One through http://www.codenameone.com/ if you + * need additional information or have any questions. + */ +package com.codename1.tools.translator; + +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; + +import java.io.BufferedReader; +import java.io.InputStreamReader; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.concurrent.TimeUnit; +import java.util.stream.Collectors; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; + +/** + * The uncooperative-mutator gate for the collector (issue #5537). + * + *

The mark phase stops each lightweight thread by raising {@code threadBlockedByGC} and + * spinning on {@code threadActive}, and the translator emits no safepoint polls in + * generated code -- not on method entry, not on loop back-edges. Every safepoint lives + * inside a runtime function, so a Java loop that allocates nothing and enters no contended + * monitor reaches none of them and that spin never ends. The reporter's debugger caught it + * at {@code totalwait = 609491500} microseconds -- ten minutes with the whole VM stopped + * behind one compute loop, because every other thread parks at its next allocation waiting + * for a cycle that can never start.

+ * + *

{@code cn1_globals.m} now bounds that spin at {@code CN1_GC_SAFEPOINT_WAIT_MAX_US} and + * then freezes the thread with the same SIGUSR2 stop the collector already uses for genuine + * native threads. This gate asserts the outcome and the mechanism, and then rebuilds the + * same translated project with the escalation compiled out ({@code -DCN1_GC_NO_FORCE_STOP}) + * and requires both to fail -- a gate that has never been watched failing proves + * nothing.

+ * + *

Every threshold is a RATIO of the workload's own two measurements rather than a + * wall-clock constant, because the failure is "a mutator was stalled for as long as the + * spinner ran" and that duration is whatever the machine makes it. See + * {@code GcUncooperativeThreadApp} for how the two are produced.

+ */ +@Tag("benchmark") +class GcUncooperativeThreadIntegrationTest { + + /** + * Share of the spin a mutator may be stalled for when the escalation works. The stall + * it should actually see is one safepoint bound (CN1_GC_SAFEPOINT_WAIT_MAX_US, 250ms) + * plus a mark, repeated per cycle -- so this is loose on purpose. The two regimes are + * "a fraction of a second" and "the entire spin", and a threshold tight enough to + * flake would be measuring the machine rather than the bug. + * + *

Measured on this arm: 0.06 alone, 0.13 with the whole benchmark suite running in + * parallel on the same host. The ablation arm measures 0.98-1.00 in both, so the two + * are separated by roughly an order of magnitude and the margin here is deliberate + * headroom for a loaded runner rather than a number tuned against one.

+ */ + private static final double MAX_STALL_SHARE_FIXED = 0.25; + + /** + * Share of the spin the ABLATION arm must exceed. A wedged VM stalls the allocator + * from whenever the first cycle starts until the spinner ends, so the only thing that + * keeps this below 1.0 is how long the workload takes to get going -- measured 0.98 + * and 1.00. Set at half that, because this half of the gate only has to establish + * that the wedge still reproduces, not to measure it. + */ + private static final double MIN_STALL_SHARE_FAULTED = 0.5; + + /** + * Floor on the spin itself. Below this the ratios above stop separating anything: a + * spin shorter than a collection cycle cannot express a stall that lasts one. The + * fixture calibrates for 6s, so this only catches a machine or a build on which the + * calibration collapsed. + */ + private static final long MIN_SPIN_MS = 2000; + + /** + * Ceiling on one translated run. The ablation arm's spinner is bounded by an iteration + * count, so even a fully wedged VM finishes -- but reading the child to EOF on this + * thread would still hang the fork if that ever stopped being true. + */ + private static final long VM_RUN_TIMEOUT_SECONDS = 600; + + /** What the collector prints when it has to shoot a mutator. */ + private static final String FORCE_STOP_MARKER = "force-stopped thread"; + + @Test + void aComputeOnlyThreadDoesNotStallEverybodyElse() throws Exception { + Parser.cleanup(); + List tempDirs = new ArrayList<>(); + try { + runGate(tempDirs); + } finally { + for (Path dir : tempDirs) { + deleteRecursively(dir); + } + } + } + + private void runGate(List tempDirs) throws Exception { + Path sourceDir = Files.createTempDirectory("gc-uncoop-sources"); + Path classesDir = Files.createTempDirectory("gc-uncoop-classes"); + Path javaApiDir = Files.createTempDirectory("gc-uncoop-javaapi"); + tempDirs.add(sourceDir); + tempDirs.add(classesDir); + tempDirs.add(javaApiDir); + + Path source = sourceDir.resolve("GcUncooperativeThreadApp.java"); + Files.write(source, loadAppSource().getBytes(StandardCharsets.UTF_8)); + + CompilerHelper.CompilerConfig config = selectCompiler(); + if (config == null) { + fail("No compatible compiler available for the GC uncooperative-thread test"); + } + CompilerHelper.compileJavaAPI(javaApiDir, config); + + List compileArgs = new ArrayList<>(); + compileArgs.add("-source"); + compileArgs.add(config.targetVersion); + compileArgs.add("-target"); + compileArgs.add(config.targetVersion); + if (CompilerHelper.useClasspath(config)) { + compileArgs.add("-classpath"); + compileArgs.add(javaApiDir.toString()); + } else { + compileArgs.add("-bootclasspath"); + compileArgs.add(javaApiDir.toString()); + compileArgs.add("-Xlint:-options"); + } + compileArgs.add("-d"); + compileArgs.add(classesDir.toString()); + compileArgs.add(source.toString()); + assertEquals(0, CompilerHelper.compile(config.jdkHome, compileArgs), + "GcUncooperativeThreadApp should compile. " + CompilerHelper.getLastErrorLog()); + + String javaResult = extractLine(runJavaMain(config, classesDir, javaApiDir), "RESULT="); + assertTrue(javaResult.startsWith("RESULT="), "JavaSE should produce RESULT="); + + CompilerHelper.copyDirectory(javaApiDir, classesDir); + Path outputDir = Files.createTempDirectory("gc-uncoop-output"); + tempDirs.add(outputDir); + CleanTargetIntegrationTest.runTranslator(classesDir, outputDir, "GcUncooperativeThreadApp"); + Path distDir = outputDir.resolve("dist"); + Path cmakeLists = distDir.resolve("CMakeLists.txt"); + assertTrue(Files.exists(cmakeLists), "Translator should emit a CMake project"); + CleanTargetIntegrationTest.replaceLibraryWithExecutableTarget( + cmakeLists, "GcUncooperativeThreadApp-src"); + + // ---- 1. the gate ------------------------------------------------------ + Run fixed = run(build(distDir, tempDirs, "fixed", ""), distDir); + assertEquals(0, fixed.exit, "The workload must finish. Output: " + tail(fixed.output)); + assertTrue(fixed.output.contains("GC_UNCOOPERATIVE_DONE"), + "The workload should run to completion. Output: " + tail(fixed.output)); + assertEquals(javaResult, extractLine(fixed.output, "RESULT="), + "JavaSE and ParparVM should agree on the workload result"); + + long fixedSpin = value(fixed.output, "SPINMS="); + long fixedStall = value(fixed.output, "MAXSTALL="); + report("fixed", fixedSpin, fixedStall, fixed.output.contains(FORCE_STOP_MARKER)); + assertTrue(fixedSpin >= MIN_SPIN_MS, + "The spin only lasted " + fixedSpin + "ms, so the stall ratio below compared" + + " nothing. Output: " + tail(fixed.output)); + + // MECHANISM: the collector had to shoot the spinner. Without this the outcome check + // would also pass on a VM where the spinner happened to reach a safepoint for some + // unrelated reason, and the gate would quietly stop testing the escalation. + 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)); + + // OUTCOME: nobody waited for the spinner. + assertTrue(fixedStall <= fixedSpin * MAX_STALL_SHARE_FIXED, + "A mutator was stalled " + fixedStall + "ms of a " + fixedSpin + "ms spin (" + + "share " + share(fixedStall, fixedSpin) + "), i.e. it spent the spin waiting" + + " for a collector waiting for a thread that reaches no safepoint." + + " Output: " + tail(fixed.output)); + + // ---- 2. proof that the gate can fail ---------------------------------- + Run faulted = run(build(distDir, tempDirs, "faulted", "-DCN1_GC_NO_FORCE_STOP"), distDir); + assertEquals(0, faulted.exit, + "The ablation build must still finish -- its spinner is bounded by an" + + " iteration count. Output: " + tail(faulted.output)); + long faultedSpin = value(faulted.output, "SPINMS="); + long faultedStall = value(faulted.output, "MAXSTALL="); + report("faulted", faultedSpin, faultedStall, faulted.output.contains(FORCE_STOP_MARKER)); + assertTrue(faultedSpin >= MIN_SPIN_MS, + "The ablation arm's spin only lasted " + faultedSpin + "ms. Output: " + + tail(faulted.output)); + assertFalse(faulted.output.contains(FORCE_STOP_MARKER), + "-DCN1_GC_NO_FORCE_STOP still reported a forced stop, so the ablation did not" + + " remove the thing part 1 asserts. Output: " + tail(faulted.output)); + assertTrue(faultedStall >= faultedSpin * MIN_STALL_SHARE_FAULTED, + "Without the escalation a mutator was stalled only " + faultedStall + "ms of a " + + faultedSpin + "ms spin (share " + share(faultedStall, faultedSpin) + "). The" + + " wedge did not reproduce, so part 1 proved nothing -- the workload" + + " has stopped reaching the state it is meant to create. Output: " + + tail(faulted.output)); + } + + /** + * Both arms are printed on every run, passing or failing. The gate's whole claim is a + * comparison between two numbers, and a comparison nobody can read afterwards is an + * assertion on trust -- the same reason the steady-state gate prints its series. + */ + private static void report(String arm, long spinMs, long stallMs, boolean forced) { + System.out.println("[GC-UNCOOP] arm=" + arm + " spinMs=" + spinMs + " maxStallMs=" + + stallMs + " stallShare=" + share(stallMs, spinMs) + " forcedStop=" + forced); + } + + private static String share(long part, long whole) { + if (whole <= 0) { + return "n/a"; + } + return String.format("%.2f", (double) part / whole); + } + + private static long value(String output, String prefix) { + for (String line : output.split("\\R")) { + if (line.startsWith(prefix)) { + try { + return Long.parseLong(line.substring(prefix.length()).trim()); + } catch (NumberFormatException e) { + return -1; + } + } + } + return -1; + } + + /** One build of the already-translated project, with its own flags and build dir. */ + private Path build(Path distDir, List tempDirs, String name, String cFlags) throws Exception { + Path buildDir = Files.createTempDirectory("gc-uncoop-build-" + name); + tempDirs.add(buildDir); + List cmake = new ArrayList<>(Arrays.asList( + "cmake", "-S", distDir.toString(), "-B", buildDir.toString(), + "-DCMAKE_BUILD_TYPE=Release")); + cmake.addAll(CompilerHelper.cmakeToolchainArgs()); + // CMAKE_C_FLAGS composes with the target's own options, so the mandatory + // -fwrapv / -fno-strict-aliasing the generated project adds are kept. + cmake.add("-DCMAKE_C_FLAGS=" + cFlags); + CleanTargetIntegrationTest.runCommand(cmake, distDir); + CleanTargetIntegrationTest.runCommand( + Arrays.asList("cmake", "--build", buildDir.toString()), distDir); + Path exe = buildDir.resolve(CompilerHelper.executableName("GcUncooperativeThreadApp")); + assertTrue(Files.exists(exe), "ParparVM build should produce a runnable executable at " + exe); + return exe; + } + + private static final class Run { + final int exit; + final String output; + + Run(int exit, String output) { + this.exit = exit; + this.output = output; + } + } + + private Run run(Path executable, Path workingDir) throws Exception { + ProcessBuilder builder = new ProcessBuilder(executable.toString()); + builder.directory(workingDir.toFile()); + // A developer debugging the collector has CN1_* knobs exported, and several of them + // would invert this result rather than fail loudly. Start from a known state. + builder.environment().keySet().removeIf(key -> key.startsWith("CN1_")); + builder.redirectErrorStream(true); + final Process process = builder.start(); + + // Drained concurrently: a child that fills the pipe buffer blocks in write() while + // we block in waitFor(), and a wedged collector is precisely what this gate is + // about -- blocking on EOF would turn a caught regression into a hung build. + final StringBuilder captured = new StringBuilder(); + Thread drain = new Thread(() -> { + try (BufferedReader reader = new BufferedReader( + new InputStreamReader(process.getInputStream(), StandardCharsets.UTF_8))) { + String line; + while ((line = reader.readLine()) != null) { + synchronized (captured) { + captured.append(line).append('\n'); + } + } + } catch (Exception e) { + // The stream ends abruptly when a timed-out child is destroyed; whatever was + // captured before that is exactly what should be reported. + } + }); + drain.setDaemon(true); + drain.start(); + + boolean exited = process.waitFor(VM_RUN_TIMEOUT_SECONDS, TimeUnit.SECONDS); + if (!exited) { + process.destroyForcibly(); + process.waitFor(10, TimeUnit.SECONDS); + } + drain.join(10_000); + String output; + synchronized (captured) { + output = captured.toString(); + } + assertTrue(exited, + "The workload did not finish within " + VM_RUN_TIMEOUT_SECONDS + "s. For this" + + " gate that is a result and not an infrastructure problem. Output so" + + " far:\n" + tail(output)); + return new Run(exited ? process.exitValue() : -1, output); + } + + private String tail(String output) { + String[] lines = output.split("\\R"); + int from = Math.max(0, lines.length - 25); + return String.join("\n", Arrays.copyOfRange(lines, from, lines.length)); + } + + private String loadAppSource() throws Exception { + java.io.InputStream in = GcUncooperativeThreadIntegrationTest.class + .getResourceAsStream("/com/codename1/tools/translator/GcUncooperativeThreadApp.java"); + assertNotNull(in, "GcUncooperativeThreadApp.java test resource should exist"); + try (BufferedReader reader = new BufferedReader(new InputStreamReader(in, StandardCharsets.UTF_8))) { + return reader.lines().collect(Collectors.joining("\n")) + "\n"; + } + } + + private String runJavaMain(CompilerHelper.CompilerConfig config, Path classesDir, Path javaApiDir) + throws Exception { + String javaExe = config.jdkHome.resolve("bin").resolve("java").toString(); + if (System.getProperty("os.name").toLowerCase().contains("win")) { + javaExe += ".exe"; + } + ProcessBuilder pb = new ProcessBuilder(javaExe, "-cp", + classesDir + System.getProperty("path.separator") + javaApiDir, + "GcUncooperativeThreadApp"); + pb.redirectErrorStream(true); + Process process = pb.start(); + String output; + try (BufferedReader reader = new BufferedReader( + new InputStreamReader(process.getInputStream(), StandardCharsets.UTF_8))) { + output = reader.lines().collect(Collectors.joining("\n")); + } + assertEquals(0, process.waitFor(), "JVM run should exit cleanly. Output: " + output); + return output; + } + + private String extractLine(String output, String prefix) { + for (String line : output.split("\\R")) { + if (line.startsWith(prefix)) { + return line.trim(); + } + } + return ""; + } + + private CompilerHelper.CompilerConfig selectCompiler() { + String[] preferredTargets = {"11", "17", "21", "25", "1.8"}; + for (String target : preferredTargets) { + for (CompilerHelper.CompilerConfig config : CompilerHelper.getAvailableCompilers(target)) { + if (CompilerHelper.isJavaApiCompatible(config)) { + return config; + } + } + } + return null; + } + + private static void deleteRecursively(Path root) { + if (root == null || !Files.exists(root)) { + return; + } + try (java.util.stream.Stream walk = Files.walk(root)) { + walk.sorted(java.util.Comparator.reverseOrder()).forEach(p -> { + try { + Files.deleteIfExists(p); + } catch (java.io.IOException ignored) { + // best effort; the OS reclaims the temp tree + } + }); + } catch (java.io.IOException ignored) { + // best effort + } + } +} diff --git a/vm/tests/src/test/resources/com/codename1/tools/translator/GcUncooperativeThreadApp.java b/vm/tests/src/test/resources/com/codename1/tools/translator/GcUncooperativeThreadApp.java new file mode 100644 index 00000000000..65447a0fda5 --- /dev/null +++ b/vm/tests/src/test/resources/com/codename1/tools/translator/GcUncooperativeThreadApp.java @@ -0,0 +1,192 @@ +/* + * Copyright (c) 2012, Codename One and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. Codename One designates this + * particular file as subject to the "Classpath" exception as provided + * by Oracle in the LICENSE file that accompanied this code. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Codename One through http://www.codenameone.com/ if you + * need additional information or have any questions. + */ + +/** + * The uncooperative-mutator half of issue #5537: what happens to everybody else while one + * thread runs code that reaches no GC safepoint. + * + *

ParparVM's mark phase stops each lightweight thread cooperatively -- it raises + * {@code threadBlockedByGC} and then spins on {@code threadActive} until the thread parks + * itself -- 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 {@code monitorEnter}, {@code Thread.sleep}, and the + * native-call bracket. A Java loop that allocates nothing and enters no contended monitor + * therefore reaches no safepoint at all, and the collector's spin never ends -- taking + * every other thread with it, because they park at their next allocation waiting for a + * cycle that can never start. The reporter's debugger caught the collector ten minutes + * into that spin while a game-tree search ran a compute-only evaluation loop.

+ * + *

The shape here is that reduced to its two essential threads:

+ * + *
    + *
  • A spinner that runs one uninterrupted long-arithmetic loop. No allocation, no + * monitor, no native call -- so on a VM without the escalation it is unstoppable for + * its whole duration.
    • + *
  • An allocator that churns small objects and times every chunk, reporting the + * LONGEST it was ever stalled. That number is the whole measurement: a mutator blocked + * behind a collector blocked behind the spinner.
    • + *
+ * + *

Both figures are printed, and the gate compares them against each other rather than + * against a wall-clock constant: {@code MAXSTALL} is a small fraction of {@code SPINMS} + * when the collector can force the spinner to stop, and essentially all of it when it + * cannot. Self-calibrating, so one pair of thresholds holds on a fast developer machine + * and a slow CI runner alike.

+ * + *

The spin length is calibrated at runtime for the same reason -- a fixed iteration + * count is seconds on one machine and milliseconds on another, and a spin shorter than a + * collection cycle cannot express the failure. Calibration runs BEFORE the allocator + * starts, so it measures the machine rather than the contention.

+ * + *

Note that the allocator cannot escape the wedge by finishing early: once the + * collector is stuck on the spinner, the allocator blocks at its next page acquisition and + * stays there until the spinner ends. Its chunk count therefore only has to be enough to + * still be running when the spin begins.

+ * + *

Declares no natives and uses no Codename One API, so a stock JVM runs it unchanged + * and {@code RESULT} can be compared across the two. {@code RESULT} deliberately excludes + * everything derived from the calibration, which is wall-clock dependent and so differs + * between any two runs. On a stock JVM the spinner is stoppable by construction (HotSpot + * polls at loop back-edges), which is the behaviour this gate asks ParparVM to + * approximate.

+ */ +public class GcUncooperativeThreadApp { + + /** How long the uninterruptible spin should last. Comfortably longer than a collection + * cycle on any runner, so a wedge is unmistakable next to a legitimate GC pause. */ + private static final long TARGET_SPIN_MS = 6000; + + /** Iterations used to measure the machine before sizing the real spin. Large enough to + * take milliseconds rather than to be swallowed by clock granularity. */ + private static final long CALIBRATE_ITERS = 20000000L; + + /** Floor on the calibrated spin, in case the calibration lands inside one clock tick. */ + private static final long MIN_SPIN_ITERS = 200000000L; + + /** Objects per timed chunk on the allocator. A chunk has to cross several BiBOP page + * acquisitions -- the page acquire is where the allocator's own safepoint lives, so a + * chunk that fitted inside one page would never park and would time nothing. */ + private static final int CHUNK_OBJECTS = 20000; + + /** Timed chunks. The measurement is a MAXIMUM, so this only has to keep the allocator + * running into the spin; see the class comment on why it cannot finish during one. */ + private static final int CHUNKS = 1500; + + /** Retained set, so the collector has something real to trace and cycles cost time. */ + private static final int LIVE_SET = 4000; + + /** A small reference-carrying object: reaches the mark worklist, unlike a leaf. */ + private static final class Node { + Node next; + long a; + long b; + + Node(Node next, long a) { + this.next = next; + this.a = a; + this.b = a * 31; + } + } + + /** Iterations for the spin, published before the spinner starts. */ + private static long spinIterations; + + /** Written by the spinner, read only after join. */ + private static volatile long spinResult; + private static volatile long spinMillis; + + /** Held so the retained set cannot be optimized into nothing. */ + private static Node[] liveSet; + + /** + * The unstoppable loop. Pure long arithmetic on locals: no allocation (so no allocator + * handshake), no monitor, no native call, and on ParparVM no back-edge poll either. + * Deliberately NOT split into chunks with a check between them -- a chunk boundary that + * touches the runtime is a safepoint, and the point of this method is to have none. + */ + private static long burn(long iterations) { + long a = 1; + for (long i = 0; i < iterations; i++) { + a = a * 31 + i; + a ^= (a >>> 7); + } + return a; + } + + private static final class Spinner implements Runnable { + public void run() { + long t0 = System.currentTimeMillis(); + long r = burn(spinIterations); + spinMillis = System.currentTimeMillis() - t0; + spinResult = r; + } + } + + public static void main(String[] args) throws Exception { + // Retained set first: a collection that finds nothing to do finishes instantly and + // would not hold the allocator long enough for the comparison to mean anything. + liveSet = new Node[LIVE_SET]; + for (int i = 0; i < LIVE_SET; i++) { + liveSet[i] = new Node(i > 0 ? liveSet[i - 1] : null, i); + } + + // Size the spin against THIS machine, before any contention exists. + long calStart = System.currentTimeMillis(); + long calAcc = burn(CALIBRATE_ITERS); + long calMs = System.currentTimeMillis() - calStart; + long iters = calMs <= 0 ? MIN_SPIN_ITERS : (CALIBRATE_ITERS * TARGET_SPIN_MS) / calMs; + if (iters < MIN_SPIN_ITERS) { + iters = MIN_SPIN_ITERS; + } + spinIterations = iters; + + Thread spinner = new Thread(new Spinner()); + spinner.start(); + + // The allocator. Every chunk is timed; the maximum is the report. + long maxStallMs = 0; + long checksum = 0; + for (int chunk = 0; chunk < CHUNKS; chunk++) { + long t0 = System.currentTimeMillis(); + Node head = null; + for (int i = 0; i < CHUNK_OBJECTS; i++) { + head = new Node(head, chunk * 31L + i); + } + checksum += head.b; + // Keep the retained set churning so marking has real work every cycle. + liveSet[chunk % LIVE_SET] = new Node(null, chunk); + long dt = System.currentTimeMillis() - t0; + if (dt > maxStallMs) { + maxStallMs = dt; + } + } + + spinner.join(); + + System.out.println("SPINMS=" + spinMillis); + System.out.println("MAXSTALL=" + maxStallMs); + System.out.println("SPINACC=" + spinResult); + System.out.println("RESULT=" + (checksum ^ calAcc ^ liveSet[0].b)); + System.out.println("GC_UNCOOPERATIVE_DONE"); + } +} From cb71d94c6d62d4930a18847eddd3c0ca7487a654 Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Sun, 30 Aug 2026 21:04:09 +0300 Subject: [PATCH 2/7] Take no lock a frozen mutator could be holding (review) 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) --- vm/ByteCodeTranslator/src/cn1_globals.m | 152 +++++++++++++++++------- 1 file changed, 109 insertions(+), 43 deletions(-) diff --git a/vm/ByteCodeTranslator/src/cn1_globals.m b/vm/ByteCodeTranslator/src/cn1_globals.m index 3cebed7b3d6..5d328ec979a 100644 --- a/vm/ByteCodeTranslator/src/cn1_globals.m +++ b/vm/ByteCodeTranslator/src/cn1_globals.m @@ -2232,7 +2232,33 @@ void codenameOneGCMark() { // TRUE once the cooperative handshake below gave up and froze this // thread with a signal instead. It changes what the rest of this // iteration may touch -- see each use. + // + // WHAT A HELD FREEZE FORBIDS. The thread is stopped at an arbitrary + // instruction, so it may own ANY lock a mutator can hold -- the libc + // allocator's, stdio's, os_log's, criticalSection. Nothing between the + // freeze and cn1GcMarkReleaseForced may block on one of those, or the + // collector waits for a thread that cannot run until the collector + // releases it: the same permanent wedge this whole change exists to + // remove, just rarer and harder to place. Note this is not only about + // how long a mutator holds such a lock -- there is a window between the + // last threadActive read and the signal landing in which the target can + // enter any of that code, so "it would have to be unlucky" is not an + // argument. Every step below is therefore classified: + // + // pending migration ....... TAKES criticalSection -> skipped entirely + // aggressive-allocator hold allocates + logs -> skipped entirely + // root snapshot rebuild ... reallocs -> skipped (built pre-freeze) + // precise stack scan ...... resolve + mark bit -> SAFE, lock-free + // native stack scan ....... resolve + mark bit -> SAFE, lock-free + // this escalation's log ... stdio / os_log -> DEFERRED past release + // + // The mark worklist mutex is fine to take: only collector threads ever + // hold it, so a frozen mutator can never be the owner. JAVA_BOOLEAN forcedStop = JAVA_FALSE; + // Deferred report for the escalation (see above). Zero means nothing to + // report; the values are captured under the freeze and printed after it. + long long forcedStopWaitUs = 0; + long forcedStopSeq = 0; #ifdef CN1_CONSERVATIVE_GC_ROOTS // PHASE 3b: demand a FRESH native-stack capture this round. Only a @@ -2332,16 +2358,18 @@ void codenameOneGCMark() { // mutator to make progress is abnormal, and the reason // issue #5537 took so long to place is that this loop had // nothing to say for itself. + // + // CAPTURED here, PRINTED after the release. NSLog and + // fprintf both take locks a frozen mutator can be holding + // (os_log's, stdio's, and malloc's underneath either), so + // logging here would deadlock the collector against the + // very thread it just stopped. Counting is two integer + // stores and is safe. static long cn1GcForcedStops = 0; // GC thread only cn1GcForcedStops++; if((cn1GcForcedStops & (cn1GcForcedStops - 1)) == 0) { -#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); -#endif + forcedStopSeq = cn1GcForcedStops; + forcedStopWaitUs = totalwait; } // threadActive is still TRUE and stays that way -- the // thread is frozen, not parked. Everything below reads @@ -2370,41 +2398,56 @@ void codenameOneGCMark() { #ifdef CN1_GC_CONFORM long long __mg0 = cn1GcNowNs(); #endif - lockCriticalSection(); - // A force-stopped thread is SKIPPED here, and its heapAllocationSize is - // left alone below. The append is `pending[size] = o; size++`, so a thread - // frozen between those two stores has an object in the table that the - // count does not cover yet: migrating [0,size), zeroing those slots and - // resetting size to 0 would leave that object referenced by nothing the - // collector ever walks, and the thread's own `size++` on resume would then - // hand the next allocation a slot the migration had already emptied. The - // table is not a root source -- it only decides which objects the SWEEP - // may consider -- so leaving it intact for one cycle costs a deferred - // reclaim and nothing else. It also cannot grow without bound: a table - // over its threshold parks its own thread at a safepoint, which is the - // cooperative stop this escalation was standing in for. - if(allThreads[iter] == t && !forcedStop) { - if (!t->lightweightThread) { - // For native threads, we need to actually lock them while we traverse the - // heap allocations because we can't use the usual locking mechanisms on - // them. - lockThreadHeapMutex(); - } - for(int heapTrav = 0 ; heapTrav < t->heapAllocationSize ; heapTrav++) { - JAVA_OBJECT obj = (JAVA_OBJECT)t->pendingHeapAllocations[heapTrav]; - if(obj) { - t->pendingHeapAllocations[heapTrav] = 0; - placeObjectInHeapCollection(obj); -#ifdef CN1_GC_CONFORM - cn1GcMigrated++; -#endif + // A force-stopped thread skips this ENTIRELY -- the lock included, not + // just the body. Two independent reasons, and the first one is fatal: + // + // 1. lockCriticalSection() would BLOCK. The frozen thread is stopped at an + // arbitrary instruction and may hold this very mutex (markDeadThread, + // monitorEnter's monitor-creation branch, placeObjectInHeapCollection + // all take it), and it cannot release it until this cycle releases the + // freeze -- which happens after this point. Guarding only the body, + // which is what this did first, still takes the lock and still + // deadlocks. There is also no "it is only held briefly" defence: the + // target can enter that code in the window between the last + // threadActive read and the signal landing. + // 2. Migrating would be wrong anyway. The append is `pending[size] = o; + // size++`, so a thread frozen between those two stores has an object in + // the table that the count does not cover yet: migrating [0,size), + // zeroing those slots and resetting size to 0 would leave that object + // referenced by nothing the collector ever walks, and the thread's own + // `size++` on resume would hand the next allocation a slot the + // migration had already emptied. + // + // The table is not a root source -- it only decides which objects the + // SWEEP may consider -- so leaving it intact for one cycle costs a + // deferred reclaim and nothing else. It cannot grow without bound either: + // a table over its threshold parks its own thread at a safepoint, which is + // the cooperative stop this escalation was standing in for. + if(!forcedStop) { + lockCriticalSection(); + if(allThreads[iter] == t) { + if (!t->lightweightThread) { + // For native threads, we need to actually lock them while we traverse the + // heap allocations because we can't use the usual locking mechanisms on + // them. + lockThreadHeapMutex(); + } + for(int heapTrav = 0 ; heapTrav < t->heapAllocationSize ; heapTrav++) { + JAVA_OBJECT obj = (JAVA_OBJECT)t->pendingHeapAllocations[heapTrav]; + if(obj) { + t->pendingHeapAllocations[heapTrav] = 0; + placeObjectInHeapCollection(obj); + #ifdef CN1_GC_CONFORM + cn1GcMigrated++; + #endif + } + } + if (!t->lightweightThread) { + unlockThreadHeapMutex(); } } - if (!t->lightweightThread) { - unlockThreadHeapMutex(); - } + unlockCriticalSection(); } - unlockCriticalSection(); #ifdef CN1_GC_CONFORM cn1GcMigrateNs += cn1GcNowNs() - __mg0; #endif @@ -2413,11 +2456,20 @@ void codenameOneGCMark() { JAVA_INT allocSize = t->heapAllocationSize; JAVA_BOOLEAN agressiveAllocator = JAVA_FALSE; + // Skipped under a held freeze, with the diagnostic below it. get_free_memory() + // and NSLog both take locks the stopped thread may own, and the EDT is a + // perfectly ordinary candidate for the escalation -- a long computation on + // the event thread is exactly the shape issue #5537 reported. The hold + // decision is also meaningless here: allocSize is read from a table this + // cycle deliberately did not migrate, so it describes the previous cycle's + // backlog rather than what this thread just produced. #ifndef CN1_NO_AGGRESSIVE_HOLD - if (isEdt(t->threadId) && !lowMemoryMode) { - agressiveAllocator = allocSize > CN1_AGRESSIVE_ALLOCATOR_THREAD_HEAP_ALLOCATIONS_THRESHOLD_EDT; - } else { - agressiveAllocator = allocSize > CN1_AGRESSIVE_ALLOCATOR_THREAD_HEAP_ALLOCATIONS_THRESHOLD; + if(!forcedStop) { + if (isEdt(t->threadId) && !lowMemoryMode) { + agressiveAllocator = allocSize > CN1_AGRESSIVE_ALLOCATOR_THREAD_HEAP_ALLOCATIONS_THRESHOLD_EDT; + } else { + agressiveAllocator = allocSize > CN1_AGRESSIVE_ALLOCATOR_THREAD_HEAP_ALLOCATIONS_THRESHOLD; + } } #endif if (CN1_EDT_THREAD_ID == t->threadId && agressiveAllocator) { @@ -2527,6 +2579,20 @@ void codenameOneGCMark() { cn1GcMarkReleaseForced(t); forcedStop = JAVA_FALSE; } + // Only now, with the thread running again, is it safe to take a logging + // lock: until the release the target could have owned os_log's, stdio's or + // malloc's, and printing would have hung the collector on the thread it + // had just stopped. Captured at the escalation, printed here. + if(forcedStopSeq != 0) { +#if defined(__OBJC__) + NSLog(@"[GC] force-stopped thread %d after %lldus at a safepoint it never reached (%ld so far)", + (int)t->threadId, forcedStopWaitUs, forcedStopSeq); +#else + fprintf(stderr, "[GC] force-stopped thread %d after %lldus at a safepoint it never reached (%ld so far)\n", + (int)t->threadId, forcedStopWaitUs, forcedStopSeq); +#endif + forcedStopSeq = 0; + } #endif #ifdef CN1_CONSERVATIVE_GC_SELFCHECK cn1GcSelfCheckThreadStack(t, stackSize); From 4a8ddbfbdf2b435ea3ef8ebb8bca1b462d69c70f Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Sun, 30 Aug 2026 23:09:19 +0300 Subject: [PATCH 3/7] Resolve the stack bounds before freezing, not after (review) 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) --- vm/ByteCodeTranslator/src/cn1_globals.h | 9 +++- vm/ByteCodeTranslator/src/cn1_globals.m | 65 +++++++++++++++++------ vm/ByteCodeTranslator/src/nativeMethods.m | 2 + 3 files changed, 60 insertions(+), 16 deletions(-) diff --git a/vm/ByteCodeTranslator/src/cn1_globals.h b/vm/ByteCodeTranslator/src/cn1_globals.h index a75cbca8861..c36a7fa8982 100644 --- a/vm/ByteCodeTranslator/src/cn1_globals.h +++ b/vm/ByteCodeTranslator/src/cn1_globals.h @@ -1278,7 +1278,14 @@ struct ThreadLocalData { volatile sig_atomic_t gcSigRelease; // GC publishes highest released gen (monotonic) volatile sig_atomic_t gcSigStopGen; // generation counter (GC thread writes only) void* volatile gcSigStackPointer; // SP captured inside the signal handler - void* volatile gcSigStackBase; // [sp,base) high bound (filled by GC/handler) + // [sp,base) high bound and stack size, resolved BEFORE a forced freeze and reused + // while it is held. cn1GcStackBase must not be called under one: it is two plain + // accessors on Apple, but on Linux it is pthread_getattr_np, which mallocs (and + // reads /proc/self/maps for the initial thread), so calling it while the target is + // stopped can wait on an allocator lock the target owns. Written by the GC thread in + // cn1GcMarkForceStopUncooperative, read by cn1GcScanThreadNativeStack. + void* volatile gcSigStackBase; + size_t gcSigStackSize; char gcSigRegs[4096]; // raw copy of the interrupted ucontext (GPRs) volatile sig_atomic_t gcSigRegsLen; // valid bytes in gcSigRegs // Set while the MARK LOOP owns a signal freeze it took because this thread would diff --git a/vm/ByteCodeTranslator/src/cn1_globals.m b/vm/ByteCodeTranslator/src/cn1_globals.m index 5d328ec979a..071f496f3f2 100644 --- a/vm/ByteCodeTranslator/src/cn1_globals.m +++ b/vm/ByteCodeTranslator/src/cn1_globals.m @@ -2250,10 +2250,17 @@ void codenameOneGCMark() { // root snapshot rebuild ... reallocs -> skipped (built pre-freeze) // precise stack scan ...... resolve + mark bit -> SAFE, lock-free // native stack scan ....... resolve + mark bit -> SAFE, lock-free + // stack-bounds lookup ..... mallocs on Linux -> done pre-freeze // this escalation's log ... stdio / os_log -> DEFERRED past release // // The mark worklist mutex is fine to take: only collector threads ever // hold it, so a frozen mutator can never be the owner. + // + // The stack-bounds entry is the one to learn from: cn1GcStackBase is two + // plain accessors on Apple and pthread_getattr_np on Linux, and only the + // Linux spelling allocates. Checking the Apple one and calling the + // function safe is how it got onto this list as SAFE the first time. A + // cross-platform helper has to be classified on its WORST platform. JAVA_BOOLEAN forcedStop = JAVA_FALSE; // Deferred report for the escalation (see above). Zero means nothing to // report; the values are captured under the freeze and printed after it. @@ -8419,9 +8426,29 @@ static void cn1GcSignalReleaseOne(struct ThreadLocalData* t) { // it is held, because the thread can have been frozen mid-malloc. The root snapshots must // already be built for the same reason. static JAVA_BOOLEAN cn1GcMarkForceStopUncooperative(struct ThreadLocalData* t) { + // Stack bounds FIRST, while the target is still running. cn1GcStackBase is two plain + // accessors on Apple and pthread_getattr_np on Linux, and the Linux one mallocs (plus + // reads /proc/self/maps for the initial thread) -- so resolving it under the freeze + // can block on an allocator lock the frozen thread owns. Reasoning about the Apple + // spelling and letting the conclusion cover both is how this got classified safe the + // first time. The bounds do not change for a live pthread, so resolving here and + // reusing them for the whole freeze loses nothing. + if(!t->gcPthreadValid) { + return JAVA_FALSE; + } + size_t ssz = 0; + char* base = cn1GcStackBase(t->gcPthread, &ssz); + if(base == 0 || ssz == 0) { + // No bounds means the conservative scan could not read this thread's native stack + // even once it was stopped, so freezing it would buy nothing and skip its roots. + // Report failure and let the caller fall back to waiting. + return JAVA_FALSE; + } + t->gcSigStackBase = base; + t->gcSigStackSize = ssz; if(cn1GcSignalStopOne(t) == 0) { - // Timed out or the thread has no valid pthread. cn1GcSignalStopOne has already - // pre-released the generation, so nothing is left stranded. + // Timed out. cn1GcSignalStopOne has already pre-released the generation, so + // nothing is left stranded. return JAVA_FALSE; } t->gcMarkForcedStop = JAVA_TRUE; @@ -8441,23 +8468,27 @@ static void cn1GcMarkReleaseForced(struct ThreadLocalData* t) { // resolved live object. threadStateData = the GC thread; t = the thread being scanned. static void cn1GcScanThreadNativeStack(CODENAME_ONE_THREAD_STATE, struct ThreadLocalData* t) { if(!t->gcPthreadValid) return; - size_t ssz = 0; - char* base = cn1GcStackBase(t->gcPthread, &ssz); - if(base == 0 || ssz == 0) return; #ifdef CN1_GC_CAN_FORCE_STOP + // FIRST, ahead of cn1GcStackBase: the mark loop may already be holding a signal + // freeze on this thread, and cn1GcStackBase is pthread_getattr_np on Linux, which + // mallocs -- calling it under the freeze can block on an allocator lock the frozen + // thread owns. cn1GcMarkForceStopUncooperative resolved the bounds before it froze + // the thread precisely so this path never has to. if(t->gcMarkForcedStop) { - // The mark loop already froze this thread (the cooperative handshake timed out) - // and still owns the freeze. Reuse its capture: do NOT stop again -- a second - // SIGUSR2 to a thread spinning inside the handler stays pending until the handler - // returns, so the nested stop would spin out its full timeout and then report - // failure on a thread that is demonstrably stopped -- and do NOT release, because - // the precise object-stack scan the caller runs alongside this needs the same - // freeze. Also no cn1GcBuildRootSnapshots here: it reallocs, the caller built the - // snapshot before freezing, and a frozen thread may hold the allocator lock. + // Reuse the mark loop's capture: do NOT stop again -- a second SIGUSR2 to a + // thread spinning inside the handler stays pending until the handler returns, so + // the nested stop would spin out its full timeout and then report failure on a + // thread that is demonstrably stopped -- and do NOT release, because the precise + // object-stack scan the caller runs alongside this needs the same freeze. No + // cn1GcBuildRootSnapshots either: it reallocs, and the caller built the snapshot + // before freezing for the same reason. + char* fbase = (char*)t->gcSigStackBase; + size_t fssz = t->gcSigStackSize; char* fsp = (char*)t->gcSigStackPointer; - if(fsp != 0 && fsp >= base - (long)ssz && fsp < base) { - cn1ConservativeMarkRange(threadStateData, fsp, base); + if(fbase != 0 && fssz != 0 && fsp != 0 + && fsp >= fbase - (long)fssz && fsp < fbase) { + cn1ConservativeMarkRange(threadStateData, fsp, fbase); } if(t->gcSigRegsLen > 0) { cn1ConservativeMarkRange(threadStateData, t->gcSigRegs, t->gcSigRegs + t->gcSigRegsLen); @@ -8466,6 +8497,10 @@ static void cn1GcScanThreadNativeStack(CODENAME_ONE_THREAD_STATE, struct ThreadL } #endif + size_t ssz = 0; + char* base = cn1GcStackBase(t->gcPthread, &ssz); + if(base == 0 || ssz == 0) return; + // Snapshot rebuilt BEFORE any signal-stop (realloc-while-frozen would deadlock). cn1GcBuildRootSnapshots(); diff --git a/vm/ByteCodeTranslator/src/nativeMethods.m b/vm/ByteCodeTranslator/src/nativeMethods.m index 8df74848c7a..2d50728d0c8 100644 --- a/vm/ByteCodeTranslator/src/nativeMethods.m +++ b/vm/ByteCodeTranslator/src/nativeMethods.m @@ -1830,6 +1830,8 @@ JAVA_INT java_lang_Object_hashCode___R_int(CODENAME_ONE_THREAD_STATE, JAVA_OBJEC i->gcSigRelease = 0; i->gcSigStopGen = 0; i->gcSigStackPointer = 0; + i->gcSigStackBase = 0; + i->gcSigStackSize = 0; i->gcSigRegsLen = 0; // ThreadLocalData is malloc'd, NOT zeroed (see the notes on nativeStackLimit and // bibopBytesLocal above). Garbage-nonzero here would tell From 4ded34384863b0d38002b5b6544d10471622efb8 Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Mon, 31 Aug 2026 00:16:16 +0300 Subject: [PATCH 4/7] Root marking must not allocate under a freeze either (review) 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) --- vm/ByteCodeTranslator/src/cn1_globals.m | 60 ++++++++++++++++++++++++- 1 file changed, 59 insertions(+), 1 deletion(-) diff --git a/vm/ByteCodeTranslator/src/cn1_globals.m b/vm/ByteCodeTranslator/src/cn1_globals.m index 071f496f3f2..c30b86be6f9 100644 --- a/vm/ByteCodeTranslator/src/cn1_globals.m +++ b/vm/ByteCodeTranslator/src/cn1_globals.m @@ -1448,12 +1448,52 @@ void placeObjectInHeapCollection(JAVA_OBJECT obj) { static long gcAdoptTop = 0, gcAdoptCap = 0; #endif +// Nonzero while the collector is holding a thread SIGNAL-FROZEN (either the escalation in +// codenameOneGCMark or the native-thread stop inside cn1GcScanThreadNativeStack). A frozen +// thread halts at an arbitrary instruction and may own the libc allocator lock, so nothing +// the collector runs in that window may allocate -- and root marking DOES allocate, through +// cn1MatureObject's adoption buffer. Written only by the GC thread, and only while root +// scanning, which is serial; the parallel drain runs after every freeze is released. +static _Atomic int cn1GcFreezeHeld = 0; + +#ifndef CN1_DISABLE_BIBOP +// Grow the adoption buffer to `headroom` free slots, called BEFORE a freeze is taken so the +// growth inside cn1MatureObject is unlikely to be needed while one is held. Best effort: +// failing to grow only makes the decline in cn1MatureObject more likely, never unsafe. +static void cn1GcAdoptReserve(long headroom) { + pthread_mutex_lock(&gcAdoptMutex); + if(gcAdoptTop + headroom > gcAdoptCap) { + long ncap = gcAdoptCap ? gcAdoptCap : 4096; + while(ncap < gcAdoptTop + headroom) { ncap *= 2; } + JAVA_OBJECT* n = (JAVA_OBJECT*)realloc(gcAdoptStack, (size_t)ncap * sizeof(JAVA_OBJECT)); + if(n != 0) { gcAdoptStack = n; gcAdoptCap = ncap; } + } + pthread_mutex_unlock(&gcAdoptMutex); +} +#endif + static void cn1MatureObject(JAVA_OBJECT obj) { #ifndef CN1_DISABLE_BIBOP // Claim the object for adoption exactly ONCE with a CAS -3 -> -4. Under parallel // markers two threads can both reach the same object; the CAS loser must not // double-buffer/double-register. The -4 flag takes effect immediately so the cascade, // the mark-stamp and the sweep-skip all see it during THIS mark. + // DECLINE BEFORE THE CLAIM if a signal freeze is held and the buffer below would have + // to grow: that growth is a realloc, and the frozen thread may own the allocator lock, + // which would hang the collector on the thread it just stopped and never reach the + // release. Checked before the CAS on purpose -- claiming and then bailing is what the + // OOM path below does, and it leaves the object flagged -4 and unregistered forever, + // i.e. leaked, because the CAS can never fire again. Declining leaves it at -3: it is + // still marked and traced this cycle (the push below is unconditional), it simply + // graduates in a later one. + // + // The unlocked read of gcAdoptTop/gcAdoptCap is exact where it matters: the flag is + // only ever set while the collector is root-scanning, which is serial on the GC + // thread. Ordered flag-last so the common path pays nothing but a predictable branch. + if(gcAdoptTop >= gcAdoptCap + && atomic_load_explicit(&cn1GcFreezeHeld, memory_order_relaxed) != 0) { + return; + } int expected = CN1_BIBOP_HEAP_POS; if(!__atomic_compare_exchange_n(&obj->__heapPosition, &expected, CN1_BIBOP_ADOPTED, 0, __ATOMIC_RELAXED, __ATOMIC_RELAXED)) { @@ -8446,11 +8486,18 @@ static JAVA_BOOLEAN cn1GcMarkForceStopUncooperative(struct ThreadLocalData* t) { } t->gcSigStackBase = base; t->gcSigStackSize = ssz; +#ifndef CN1_DISABLE_BIBOP + // Give the adoption buffer headroom while allocating is still legal, so the root scans + // under the freeze are unlikely to need cn1MatureObject's realloc (which they must + // decline). Sized well above one thread's plausible adoption count per cycle. + cn1GcAdoptReserve(16384); +#endif if(cn1GcSignalStopOne(t) == 0) { // Timed out. cn1GcSignalStopOne has already pre-released the generation, so // nothing is left stranded. return JAVA_FALSE; } + atomic_fetch_add_explicit(&cn1GcFreezeHeld, 1, memory_order_relaxed); t->gcMarkForcedStop = JAVA_TRUE; return JAVA_TRUE; } @@ -8461,6 +8508,7 @@ static void cn1GcMarkReleaseForced(struct ThreadLocalData* t) { } t->gcMarkForcedStop = JAVA_FALSE; cn1GcSignalReleaseOne(t); + atomic_fetch_sub_explicit(&cn1GcFreezeHeld, 1, memory_order_relaxed); } #endif @@ -8522,6 +8570,13 @@ static void cn1GcScanThreadNativeStack(CODENAME_ONE_THREAD_STATE, struct ThreadL // SIGNAL path: stop, scan, release. Used for native threads, for the forced // CN1_GC_SIGNAL_STOP=1 validation mode, or as a fallback for a stale capture. + // Same allocation ban as the escalation, and this path had it first: master already + // marked between the stop and the release here, so cn1MatureObject's realloc could + // already hang the collector against a frozen NATIVE thread. Counted rather than a + // boolean because the two freeze sites are independent. +#ifndef CN1_DISABLE_BIBOP + cn1GcAdoptReserve(16384); +#endif char* sp = cn1GcSignalStopOne(t); if(sp == 0) { // Could not stop the thread. If it is lightweight and cooperatively captured we @@ -8535,8 +8590,10 @@ static void cn1GcScanThreadNativeStack(CODENAME_ONE_THREAD_STATE, struct ThreadL (char*)&t->gcRegisterSnapshot + sizeof(t->gcRegisterSnapshot)); } } - return; + return; // nothing was frozen, so the counter was never raised } + // Raised only on the success path, so the sub below always pairs with an add. + atomic_fetch_add_explicit(&cn1GcFreezeHeld, 1, memory_order_relaxed); if(sp >= base - (long)ssz && sp < base) { cn1ConservativeMarkRange(threadStateData, sp, base); } @@ -8544,6 +8601,7 @@ static void cn1GcScanThreadNativeStack(CODENAME_ONE_THREAD_STATE, struct ThreadL cn1ConservativeMarkRange(threadStateData, t->gcSigRegs, t->gcSigRegs + t->gcSigRegsLen); } cn1GcSignalReleaseOne(t); + atomic_fetch_sub_explicit(&cn1GcFreezeHeld, 1, memory_order_relaxed); } // Scan the GC thread's OWN native stack (a root could be live only in a GC-thread C From 4174c6557ff0f32b299bf214255272bd69a750a7 Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Mon, 31 Aug 2026 01:19:55 +0300 Subject: [PATCH 5/7] Hold the freeze through the drain when SATB is off; skip the gate on 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) --- vm/ByteCodeTranslator/src/cn1_globals.m | 58 ++++++++++++++----- .../GcUncooperativeThreadIntegrationTest.java | 10 ++++ 2 files changed, 55 insertions(+), 13 deletions(-) diff --git a/vm/ByteCodeTranslator/src/cn1_globals.m b/vm/ByteCodeTranslator/src/cn1_globals.m index c30b86be6f9..4e8e3d6511a 100644 --- a/vm/ByteCodeTranslator/src/cn1_globals.m +++ b/vm/ByteCodeTranslator/src/cn1_globals.m @@ -2622,24 +2622,31 @@ void codenameOneGCMark() { // cn1GcScanThreadNativeStack releases its own signal stops at exactly this // point for the same reason. threadBlockedByGC stays set, so if this // thread does reach a safepoint it still parks. +#if !defined(CN1_DISABLE_SATB) + // Released as soon as this thread's roots are captured, which is HERE and + // not at the threadBlockedByGC clear below. A cooperatively parked thread + // waits out the mark drain in a usleep; a signal-frozen one waits in an + // async-signal-safe BUSY spin, so holding it across the drain burns a core + // for the length of a full mark. + // + // What the drain-before-unblock protects -- snapshot-at-the-beginning -- + // is supplied for a released mutator by the SATB deletion barrier, which + // is armed for the whole mark and is already the only thing keeping + // genuine native threads honest; they are never blocked at all, and + // cn1GcScanThreadNativeStack releases its own signal stops at exactly this + // point for the same reason. Hence the guard: under -DCN1_DISABLE_SATB + // both barriers compile to no-ops and that argument evaporates, so the + // freeze is instead held through the drain by the block after it. A + // released mutator could otherwise 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. + // threadBlockedByGC stays set either way, so if this thread does reach a + // safepoint it still parks. if(forcedStop) { cn1GcMarkReleaseForced(t); forcedStop = JAVA_FALSE; } - // Only now, with the thread running again, is it safe to take a logging - // lock: until the release the target could have owned os_log's, stdio's or - // malloc's, and printing would have hung the collector on the thread it - // had just stopped. Captured at the escalation, printed here. - if(forcedStopSeq != 0) { -#if defined(__OBJC__) - NSLog(@"[GC] force-stopped thread %d after %lldus at a safepoint it never reached (%ld so far)", - (int)t->threadId, forcedStopWaitUs, forcedStopSeq); -#else - fprintf(stderr, "[GC] force-stopped thread %d after %lldus at a safepoint it never reached (%ld so far)\n", - (int)t->threadId, forcedStopWaitUs, forcedStopSeq); #endif - forcedStopSeq = 0; - } #endif #ifdef CN1_CONSERVATIVE_GC_SELFCHECK cn1GcSelfCheckThreadStack(t, stackSize); @@ -2670,6 +2677,31 @@ void codenameOneGCMark() { { long long __t0 = cn1GcNowNs(); gcMarkDrainParallel(d); cn1GcTDrainNs += cn1GcNowNs() - __t0; } #else gcMarkDrainParallel(d); +#endif +#ifdef CN1_GC_CAN_FORCE_STOP + // Release point for the -DCN1_DISABLE_SATB build, where the block above + // deliberately kept the freeze (no barrier, so the drain has to finish + // before this thread may run again). A no-op when SATB is armed and the + // freeze was already dropped. + if(forcedStop) { + cn1GcMarkReleaseForced(t); + forcedStop = JAVA_FALSE; + } + // Only now, with the thread running again, is it safe to take a logging + // lock: while frozen the target could have owned os_log's, stdio's or + // malloc's, and printing would have hung the collector on the thread it + // had just stopped. Captured at the escalation, printed here, and placed + // after BOTH release points so it is correct in either build. + if(forcedStopSeq != 0) { +#if defined(__OBJC__) + NSLog(@"[GC] force-stopped thread %d after %lldus at a safepoint it never reached (%ld so far)", + (int)t->threadId, forcedStopWaitUs, forcedStopSeq); +#else + fprintf(stderr, "[GC] force-stopped thread %d after %lldus at a safepoint it never reached (%ld so far)\n", + (int)t->threadId, forcedStopWaitUs, forcedStopSeq); +#endif + forcedStopSeq = 0; + } #endif if(!agressiveAllocator) { t->threadBlockedByGC = JAVA_FALSE; diff --git a/vm/tests/src/test/java/com/codename1/tools/translator/GcUncooperativeThreadIntegrationTest.java b/vm/tests/src/test/java/com/codename1/tools/translator/GcUncooperativeThreadIntegrationTest.java index 0b88ceeb201..6ab368bb793 100644 --- a/vm/tests/src/test/java/com/codename1/tools/translator/GcUncooperativeThreadIntegrationTest.java +++ b/vm/tests/src/test/java/com/codename1/tools/translator/GcUncooperativeThreadIntegrationTest.java @@ -124,6 +124,16 @@ void aComputeOnlyThreadDoesNotStallEverybodyElse() throws Exception { } private void runGate(List tempDirs) throws Exception { + // The escalation needs POSIX signals, so CN1_GC_CAN_FORCE_STOP is deliberately not + // defined on Windows and the runtime keeps the unbounded cooperative wait there. + // The fixed arm therefore cannot emit the force-stop marker on Windows and this + // gate would fail for a reason that says nothing about the code under test. Skipped + // 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. + org.junit.jupiter.api.Assumptions.assumeFalse(CompilerHelper.isWindows(), + "The forced-stop escalation is not compiled on Windows (no POSIX signals)," + + " so there is nothing for this gate to assert there."); + Path sourceDir = Files.createTempDirectory("gc-uncoop-sources"); Path classesDir = Files.createTempDirectory("gc-uncoop-classes"); Path javaApiDir = Files.createTempDirectory("gc-uncoop-javaapi"); From f47025f23ed8100fe1ca6ddfedf48c71893a1d9a Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Mon, 31 Aug 2026 03:43:18 +0300 Subject: [PATCH 6/7] Drop the duplicated comment left by the SATB guard (no code change) 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) --- vm/ByteCodeTranslator/src/cn1_globals.m | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/vm/ByteCodeTranslator/src/cn1_globals.m b/vm/ByteCodeTranslator/src/cn1_globals.m index 4e8e3d6511a..754beb46181 100644 --- a/vm/ByteCodeTranslator/src/cn1_globals.m +++ b/vm/ByteCodeTranslator/src/cn1_globals.m @@ -2610,18 +2610,6 @@ void codenameOneGCMark() { cn1GcStackNs += cn1GcNowNs(); #endif #ifdef CN1_GC_CAN_FORCE_STOP - // Release the forced freeze the moment this thread's roots are captured, - // which is HERE and not at the threadBlockedByGC clear further down. A - // cooperatively parked thread waits out the mark drain in a usleep; a - // signal-frozen one waits in an async-signal-safe BUSY spin, so holding it - // across the drain would burn a core for the length of a full mark. What - // the drain-before-unblock is protecting -- snapshot-at-the-beginning -- - // is supplied for a released mutator by the SATB deletion barrier, which - // is armed for the whole mark and is already the only thing keeping - // genuine native threads honest; they are never blocked at all, and - // cn1GcScanThreadNativeStack releases its own signal stops at exactly this - // point for the same reason. threadBlockedByGC stays set, so if this - // thread does reach a safepoint it still parks. #if !defined(CN1_DISABLE_SATB) // Released as soon as this thread's roots are captured, which is HERE and // not at the threadBlockedByGC clear below. A cooperatively parked thread From 25d569e0c805085046d45960abe4f96833d54933 Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Mon, 31 Aug 2026 04:49:46 +0300 Subject: [PATCH 7/7] Never freeze a thread mid minor-collection; drop the escalation when 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) --- vm/ByteCodeTranslator/src/cn1_globals.h | 17 ++++- vm/ByteCodeTranslator/src/cn1_globals.m | 98 +++++++++++++++---------- vm/CLAUDE.md | 18 ++++- 3 files changed, 90 insertions(+), 43 deletions(-) diff --git a/vm/ByteCodeTranslator/src/cn1_globals.h b/vm/ByteCodeTranslator/src/cn1_globals.h index c36a7fa8982..d2ba08f9cba 100644 --- a/vm/ByteCodeTranslator/src/cn1_globals.h +++ b/vm/ByteCodeTranslator/src/cn1_globals.h @@ -126,7 +126,22 @@ // -DCN1_GC_NO_FORCE_STOP restores the unbounded cooperative spin. It is the ablation arm // GcUncooperativeThreadIntegrationTest builds to prove the gate can fail -- without a // build that still wedges, an assertion that the VM does not wedge proves nothing. -#if defined(CN1_CONSERVATIVE_GC_ROOTS) && !defined(_WIN32) && !defined(CN1_GC_NO_FORCE_STOP) +// +// ALSO OFF UNDER -DCN1_DISABLE_SATB, which is the interesting one. A freeze is only +// releasable early -- before the mark drain -- because the SATB deletion barrier covers a +// mutator released mid-mark, exactly as it already covers native threads, which are never +// blocked at all. With the barrier compiled out that argument is gone, and the two ways to +// keep the escalation would both be worse than not having it: releasing early would let +// the resumed thread move a child out of a captured root into a local no snapshot contains +// and have the sweep take it, while holding the freeze through the drain puts markStatics +// (force-marking, which mallocs through the force-visited table) and gcMarkDrainParallel +// (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 this ablation keeps +// master's unbounded cooperative wait, which is the behaviour it is there to measure +// against anyway. +#if defined(CN1_CONSERVATIVE_GC_ROOTS) && !defined(_WIN32) \ + && !defined(CN1_GC_NO_FORCE_STOP) && !defined(CN1_DISABLE_SATB) #define CN1_GC_CAN_FORCE_STOP 1 #endif diff --git a/vm/ByteCodeTranslator/src/cn1_globals.m b/vm/ByteCodeTranslator/src/cn1_globals.m index 754beb46181..da921e8b653 100644 --- a/vm/ByteCodeTranslator/src/cn1_globals.m +++ b/vm/ByteCodeTranslator/src/cn1_globals.m @@ -2296,6 +2296,19 @@ void codenameOneGCMark() { // The mark worklist mutex is fine to take: only collector threads ever // hold it, so a frozen mutator can never be the owner. // + // Note what is NOT on the list: markStatics and gcMarkDrainParallel, which + // run after the release. Both allocate -- markStatics force-marks, which + // reaches the force-visited table's malloc, and the first parallel drain + // creates its workers with pthread_create -- so the release must stay ahead + // of them. That is also why the escalation is compiled out when the SATB + // barrier is (see CN1_GC_CAN_FORCE_STOP): without the barrier the only way + // to keep it would be to hold the freeze across exactly those two. + // + // And a thread inside its own nursery minor collection is never frozen at + // all: cn1MarkForceStopUncooperative declines it, because the root scans + // mark through the TARGET's thread state and nurseryPromoting makes + // gcMarkObject return without marking anything. + // // The stack-bounds entry is the one to learn from: cn1GcStackBase is two // plain accessors on Apple and pthread_getattr_np on Linux, and only the // Linux spelling allocates. Checking the Apple one and calling the @@ -2610,31 +2623,42 @@ void codenameOneGCMark() { cn1GcStackNs += cn1GcNowNs(); #endif #ifdef CN1_GC_CAN_FORCE_STOP -#if !defined(CN1_DISABLE_SATB) // Released as soon as this thread's roots are captured, which is HERE and // not at the threadBlockedByGC clear below. A cooperatively parked thread // waits out the mark drain in a usleep; a signal-frozen one waits in an - // async-signal-safe BUSY spin, so holding it across the drain burns a core - // for the length of a full mark. + // async-signal-safe BUSY spin, so holding it across the drain would burn a + // core for the length of a full mark -- and would drag markStatics and + // gcMarkDrainParallel, neither of which is allocation-free, inside a window + // where this thread may own the allocator lock. // - // What the drain-before-unblock protects -- snapshot-at-the-beginning -- - // is supplied for a released mutator by the SATB deletion barrier, which - // is armed for the whole mark and is already the only thing keeping - // genuine native threads honest; they are never blocked at all, and + // What the drain-before-unblock protects -- snapshot-at-the-beginning -- is + // supplied for a released mutator by the SATB deletion barrier, which is + // armed for the whole mark and is already the only thing keeping genuine + // native threads honest; they are never blocked at all, and // cn1GcScanThreadNativeStack releases its own signal stops at exactly this - // point for the same reason. Hence the guard: under -DCN1_DISABLE_SATB - // both barriers compile to no-ops and that argument evaporates, so the - // freeze is instead held through the drain by the block after it. A - // released mutator could otherwise 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. - // threadBlockedByGC stays set either way, so if this thread does reach a - // safepoint it still parks. + // point for the same reason. That dependency is enforced rather than + // assumed: CN1_GC_CAN_FORCE_STOP is not defined when the barrier is + // compiled out (see cn1_globals.h), so this release can never run without + // it. threadBlockedByGC stays set, so if this thread does reach a safepoint + // it still parks. if(forcedStop) { cn1GcMarkReleaseForced(t); forcedStop = JAVA_FALSE; } + // Only now, with the thread running again, is it safe to take a logging + // lock: while frozen the target could have owned os_log's, stdio's or + // malloc's, and printing would have hung the collector on the thread it had + // just stopped. Captured at the escalation, printed here. + if(forcedStopSeq != 0) { +#if defined(__OBJC__) + NSLog(@"[GC] force-stopped thread %d after %lldus at a safepoint it never reached (%ld so far)", + (int)t->threadId, forcedStopWaitUs, forcedStopSeq); +#else + fprintf(stderr, "[GC] force-stopped thread %d after %lldus at a safepoint it never reached (%ld so far)\n", + (int)t->threadId, forcedStopWaitUs, forcedStopSeq); #endif + forcedStopSeq = 0; + } #endif #ifdef CN1_CONSERVATIVE_GC_SELFCHECK cn1GcSelfCheckThreadStack(t, stackSize); @@ -2665,31 +2689,6 @@ void codenameOneGCMark() { { long long __t0 = cn1GcNowNs(); gcMarkDrainParallel(d); cn1GcTDrainNs += cn1GcNowNs() - __t0; } #else gcMarkDrainParallel(d); -#endif -#ifdef CN1_GC_CAN_FORCE_STOP - // Release point for the -DCN1_DISABLE_SATB build, where the block above - // deliberately kept the freeze (no barrier, so the drain has to finish - // before this thread may run again). A no-op when SATB is armed and the - // freeze was already dropped. - if(forcedStop) { - cn1GcMarkReleaseForced(t); - forcedStop = JAVA_FALSE; - } - // Only now, with the thread running again, is it safe to take a logging - // lock: while frozen the target could have owned os_log's, stdio's or - // malloc's, and printing would have hung the collector on the thread it - // had just stopped. Captured at the escalation, printed here, and placed - // after BOTH release points so it is correct in either build. - if(forcedStopSeq != 0) { -#if defined(__OBJC__) - NSLog(@"[GC] force-stopped thread %d after %lldus at a safepoint it never reached (%ld so far)", - (int)t->threadId, forcedStopWaitUs, forcedStopSeq); -#else - fprintf(stderr, "[GC] force-stopped thread %d after %lldus at a safepoint it never reached (%ld so far)\n", - (int)t->threadId, forcedStopWaitUs, forcedStopSeq); -#endif - forcedStopSeq = 0; - } #endif if(!agressiveAllocator) { t->threadBlockedByGC = JAVA_FALSE; @@ -8517,6 +8516,25 @@ static JAVA_BOOLEAN cn1GcMarkForceStopUncooperative(struct ThreadLocalData* t) { // nothing is left stranded. return JAVA_FALSE; } +#ifdef CN1_NURSERY + // DECLINE a thread caught inside its own minor collection. cn1NurseryWriteBarrier + // raises nurseryPromoting and deliberately leaves threadActive TRUE for the duration, + // so it is a prime candidate for this escalation -- and the root scans below call + // gcMarkObject(t, ...) with the TARGET's thread state, whose first act under that flag + // is to promote-or-return WITHOUT marking. Freezing here would therefore hand the + // sweep a thread whose roots were all silently skipped, and mature objects live only + // from this thread would be reclaimed under it. + // + // Checked AFTER the stop, not before: read while the thread is running, the flag can + // be raised in the window between the read and the signal landing. A frozen thread's + // flag cannot change, so this is exact. Released and reported as a failure so the + // caller's retry simply tries again after the next interval, by which time the minor + // collection is normally over -- and the cooperative wait is still in force meanwhile. + if(t->nurseryPromoting) { + cn1GcSignalReleaseOne(t); // before any bookkeeping, so there is none to unwind + return JAVA_FALSE; + } +#endif atomic_fetch_add_explicit(&cn1GcFreezeHeld, 1, memory_order_relaxed); t->gcMarkForcedStop = JAVA_TRUE; return JAVA_TRUE; diff --git a/vm/CLAUDE.md b/vm/CLAUDE.md index 0d483dcaf84..59eb9d0396f 100644 --- a/vm/CLAUDE.md +++ b/vm/CLAUDE.md @@ -401,8 +401,22 @@ frozen wherever it happened to be rather than at a point it chose: spins out its whole timeout and then reports failure on a thread that is demonstrably stopped. `gcMarkForcedStop` tells `cn1GcScanThreadNativeStack` to reuse the capture. -Windows has no POSIX signals, so there the spin stays unbounded -- proceeding without -stopping the thread would miss its roots and free live objects, which is worse than a hang. +Two configurations deliberately do NOT get the escalation, and both are enforced in the +`CN1_GC_CAN_FORCE_STOP` guard rather than argued at the use site. **Windows**, which has no +POSIX signals -- proceeding without stopping the thread would miss its roots and free live +objects, which is worse than a hang. And **`-DCN1_DISABLE_SATB`**, because the early +release is what keeps the frozen window small, and only the barrier makes an early release +sound; holding the freeze through the drain instead drags `markStatics` (force-marking, +which mallocs through the force-visited table) and `gcMarkDrainParallel` (lazy +`pthread_create`) inside it, which is a wedge in the middle of the fix for a wedge. + +A thread inside its own **nursery minor collection** is never frozen either. +`cn1NurseryWriteBarrier` raises `nurseryPromoting` and leaves `threadActive` TRUE for the +duration, so it is a prime escalation candidate -- and the root scans mark through the +TARGET's thread state, where that flag makes `gcMarkObject` promote-or-return without +marking anything. Freezing one would hand the sweep a thread whose roots were all silently +skipped. The check runs AFTER the stop, because a flag read while the thread is still +running can be raised in the window before the signal lands. The proper long-term answer is a back-edge poll in the translator; it costs throughput in every loop the VM ever runs, and this makes the pathological case survivable without paying that everywhere.