Fix GC storm on retained large-array loads (issue 5425, final Dtest shape)#5446
Open
shai-almog wants to merge 22 commits into
Open
Fix GC storm on retained large-array loads (issue 5425, final Dtest shape)#5446shai-almog wants to merge 22 commits into
shai-almog wants to merge 22 commits into
Conversation
…ape) Arrays above CN1_BIBOP_MAX_OBJECT always take the legacy allocation path, whose only byte-based GC signal was the pre-BiBOP 1MB isHighFrequencyGC re-arm -- 24x more aggressive than the BiBOP trigger. Retaining a few hundred 10K byte[] blocks (no garbage at all) kept full collection cycles starting every 200ms wait interval over the whole survivor set: "allocating the large arrays is triggering a global multiple times". Fix, all gated on the BiBOP build so -DCN1_DISABLE_BIBOP keeps the old shape verbatim: - cn1LegacyBytesSinceGc: per-cycle legacy allocation byte counter, reset in cn1BibopBeginGcCycle alongside bibopBytesSinceGc. - Event-driven trigger in codenameOneGcMalloc's legacy tail mirroring cn1BibopMaybeGc: async System.gc() when legacy volume crosses CN1_LEGACY_GC_TRIGGER_BYTES (24MB, -D overridable), so legacy-churn workloads still collect promptly. - isHighFrequencyGC re-arms at max(1MB, bibopGcTriggerBytes): with both paths event-driven on volume, the 200ms loop only needs to re-arm when the volume budget would have been crossed anyway. - CN1_GC_LOG_CYCLES env-gated cycle tracer in codenameOneGCMark (one stderr line per cycle) for CI assertions and on-device diagnosis. Guard: com.bench.LargeArrayLoad models the final Dtest (persistent small survivor set; retained large-array pass producing no garbage; wall-stretched phases so re-arm windows are machine-independent), and LargeArrayGcIntegrationTest gates on cycle count via the tracer -- measured 15 cycles unfixed vs 6 fixed, stable across runs, budget 10 -- plus RESULT parity with the host JVM. Cycle count is load-independent, unlike phase wall times which only inflate under mutator contention. Validated: integration test passes fixed / fails unfixed; gauntlet green (tortures byte-identical to host JVM, GcStress/MtStress in both cooperative and CN1_GC_SIGNAL_STOP modes). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0133QDXUjHwX9uprkupEgB29
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses the GC “storm” regression in ParparVM on iOS for workloads that retain many large byte[] arrays (legacy allocation path), by aligning legacy GC triggering/scheduling with the BiBOP volume-based policy and adding deterministic cycle-count instrumentation plus CI regression coverage.
Changes:
- Adds a legacy (non-BiBOP) allocation byte counter and an event-driven GC trigger for large/legacy allocations, and aligns the high-frequency GC re-arm threshold with the adaptive BiBOP trigger.
- Introduces an env-gated per-GC-cycle tracer (
CN1_GC_LOG_CYCLES) for deterministic cycle counting. - Adds a benchmark driver and a ParparVM integration test that gates on GC cycle count and validates
RESULT=parity vs host JVM.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
vm/ByteCodeTranslator/src/cn1_globals.m |
Adds legacy allocation volume tracking/trigger, aligns re-arm threshold with BiBOP trigger, and adds env-gated GC cycle tracing. |
vm/tests/src/test/java/com/codename1/tools/translator/LargeArrayGcIntegrationTest.java |
New integration test that builds/runs a translated executable and asserts GC cycle count stays below a regression threshold. |
vm/tests/src/test/resources/com/codename1/tools/translator/LargeArrayGcApp.java |
New test app resource modeling the “final Dtest shape” workload used by the integration test. |
vm/benchmarks/src/com/bench/LargeArrayLoad.java |
New benchmark app for local/manual reproduction and cycle-count inspection. |
vm/benchmarks/README.md |
Documents the new benchmark and how to use cycle tracing to detect regressions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
✅ ByteCodeTranslator Quality ReportTest & Coverage
Benchmark Results
Static Analysis
Generated automatically by the PR CI workflow. |
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Collaborator
Author
|
Compared 146 screenshots: 146 matched. Benchmark ResultsDetailed Performance Metrics
|
Contributor
Cloudflare Preview
|
Collaborator
Author
|
Compared 146 screenshots: 146 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 146 screenshots: 146 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
Native Linux port (x64)Compared 147 screenshots: 146 matched, 1 missing actual.
|
Collaborator
Author
Native Linux port (arm64)Compared 147 screenshots: 146 matched, 1 missing actual.
|
Collaborator
Author
|
Compared 181 screenshots: 181 matched. |
Collaborator
Author
|
Compared 144 screenshots: 144 matched. |
Collaborator
Author
|
Compared 217 screenshots: 217 matched. |
Collaborator
Author
|
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 143 screenshots: 143 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Contributor
|
It would be interesting to run the current test against the legacy parpavm behavior, but I don't have the |
check-copyright-headers flagged the two new test files; use the standard Codename One GPLv2 + Classpath Exception header. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0133QDXUjHwX9uprkupEgB29
Use the same atomic_exchange idiom as the BiBOP reset for cn1LegacyBytesSinceGc (racing adds land before the swap or charge the next cycle), and mirror cn1BibopMaybeGc's native-allocation-bracket gate on the legacy trigger: without conservative roots a bracketed thread must not trigger a cycle; with them, gating would starve the trigger for native-bracket allocators. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0133QDXUjHwX9uprkupEgB29
cn1LegacyBytesSinceGc is file-local -- make it static. The CN1_GC_LOG_CYCLES tracer stays outside the CN1_DISABLE_BIBOP guard on purpose (A/B builds need the same cycle-count observable); document that instead of gating it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0133QDXUjHwX9uprkupEgB29
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
vm/ByteCodeTranslator/src/cn1_globals.m:3966
- The legacy-volume trigger uses
long prevLegacyBytes = atomic_fetch_add_explicit(&cn1LegacyBytesSinceGc, ...). On Windows,longis 32-bit, so both the accumulator andprevLegacyBytescan overflow and incorrectly miss the edge-trigger crossing (or trigger spuriously). Use a 64-bit accumulator type consistently here (and in the cn1LegacyBytesSinceGc declaration/reset).
long prevLegacyBytes = atomic_fetch_add_explicit(&cn1LegacyBytesSinceGc, (long)size,
memory_order_relaxed);
if(prevLegacyBytes <= CN1_LEGACY_GC_TRIGGER_BYTES
&& prevLegacyBytes + (long)size > CN1_LEGACY_GC_TRIGGER_BYTES
&& constantPoolObjects != 0
Two review findings on the Windows (LLP64 / no-signal-stop) target: - cn1LegacyBytesSinceGc was _Atomic long, which is 32-bit under LLP64; the counter accumulates raw bytes between cycle resets, so it now uses long long (with the fetch_add operand and prevLegacyBytes widened to match) so the edge-trigger comparison can never see a wrapped value. - Thread.sleep published threadActive=FALSE without a cooperative park capture (a pre-existing pattern this PR's rewrite kept). Under CN1_CONSERVATIVE_GC_ROOTS the collector scans a parked thread's native stack either via the capture or a signal stop, and Windows has no signal-stop fallback -- an uncaptured sleeper's native-stack roots went unscanned for the cycle. sleep now runs CN1_GC_PARK_CAPTURE before parking, per the CN1_YIELD_THREAD contract, and drops the capture on resume exactly like CN1_RESUME_THREAD. Revalidated locally: LargeArrayLoad 6 GC cycles (x2), RESULT=1099 host parity, SLEEP_EDGE_OK, TIMER_LATENCY_OK (max_early_ms=0, min_sleep1500_ms=1500), MapTorture byte-identical to the host JVM, GcStress x4 + MtStress x3 in cooperative and CN1_GC_SIGNAL_STOP=1 modes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0133QDXUjHwX9uprkupEgB29
…umbs - The sleep deadline on the MSVC/clang-cl target was computed from gettimeofday, a wall clock: a system time step mid-sleep stretched or cut the remaining duration. cn1_win_compat gains cn1_monotonic_micros (QueryPerformanceCounter, overflow-safe split divide) and cn1SleepNowMicros now uses it, keeping windows.h out of translated compilation units per the compat layer's rule. - LargeArrayGcIntegrationTest's best-effort temp cleanup no longer fails silently: one stderr line per root reports the first failed deletion so leaked translated build trees leave a breadcrumb on self-hosted runners. - LargeArrayLoad and its CI twin LargeArrayGcApp now carry explicit KEEP IN SYNC notes in both directions, documenting that the shared host-JVM-asserted RESULT= value turns one-sided edits into a visible parity break rather than silent drift. Revalidated locally: LargeArrayLoad 6 GC cycles (x2), RESULT=1099, SLEEP_EDGE_OK, TIMER_LATENCY_OK; vm/tests module test-compiles clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0133QDXUjHwX9uprkupEgB29
The copyright gate checks every file a PR touches; these two predate the gate and entered the diff via the monotonic-clock shim. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0133QDXUjHwX9uprkupEgB29
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
vm/ByteCodeTranslator/src/nativeMethods.m:2116
- Thread.sleep(long) still doesn’t reject negative durations. In Java semantics (and in this repo’s JavaAPI, see Thread.sleep(long,int) and Thread.join), negative millis must throw IllegalArgumentException. With the new deadline arithmetic, negative values can also trigger signed overflow/UB in
millis * 1000(e.g. Long.MIN_VALUE) before the loop even runs, or silently return immediately, which is inconsistent and potentially unsafe.
Please add an early millis < 0 check in the native implementation that throws IllegalArgumentException (e.g. message "timeout value is negative"), and ensure the threadActive/gcParkCaptured state is restored appropriately when throwing.
JAVA_LONG now = cn1SleepNowMicros();
JAVA_LONG maxMicros = 0x7fffffffffffffffLL;
JAVA_LONG deltaMicros = (millis > maxMicros / 1000) ? maxMicros : millis * 1000;
JAVA_LONG deadline = (deltaMicros > maxMicros - now) ? maxMicros : now + deltaMicros;
The unsynchronized static was a formal C data race across concurrent sleepers; QueryPerformanceFrequency is a cheap userspace read of a boot constant, so just call it per invocation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0133QDXUjHwX9uprkupEgB29
Two review findings: - The edge-triggered legacy volume trigger could lose its edge for good: a crossing that landed while the allocating thread was inside a native-allocation bracket (gated when conservative roots are off) was skipped, and every later allocation saw prev already above the threshold, deferring collection to the GC thread's 30s idle wake. The trigger is now LEVEL-triggered on the counter and LATCHED (cn1LegacyGcScheduled, cleared at cycle begin after the counter reset) so a suppressed crossing is retried by the next out-of-bracket legacy allocation on any thread while System.gc() is still scheduled at most once per cycle window. - Thread.sleep(long) now throws IllegalArgumentException for negative millis per the JDK contract (the (millis, nanos) overload already did). The guard lives in Java -- sleep(long) delegates to a private native sleepImpl -- so the exception class is retained by the translator's reachability pass wherever sleep is used. SleepEdge now asserts the negative case. Revalidated locally: LargeArrayLoad 6 GC cycles (x2), RESULT=1099, SLEEP_EDGE_OK (negativeThrew=true), TIMER_LATENCY_OK, MapTorture parity, GcStress x3 + MtStress x2 in both stop modes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0133QDXUjHwX9uprkupEgB29
Thread.sleep(long) became a Java wrapper delegating to the native sleepImpl(long), so the JS translator now emits calls to cn1_java_lang_Thread_sleepImpl_*; the runtime binds those (keeping the old sleep aliases for previously-translated bundles), the native registry lists the new symbol, and the opcode coverage test asserts it. Also: the Linux gate's diagnostic grep uses -E with standard alternation instead of GNU basic-regex escapes. JavascriptOpcodeCoverageTest passes (3/3); LargeArrayGcIntegrationTest re-verified green at both e2a6c74 and 0a13eca on this machine (the one local failure was CPU-starvation flake from a concurrent full module build, not a code regression). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0133QDXUjHwX9uprkupEgB29
The copyright gate checks every PR-touched file; this pre-existing test entered the diff via the sleepImpl binding assertion. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0133QDXUjHwX9uprkupEgB29
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the remaining slowdown in #5425 (final Dtest shape from the last comments: a persistent survivor set plus a pass that retains only large 10K
byte[]blocks and produces no garbage, yet took 450s on iOS vs 4s on Android, with the reporter observing "allocating the large arrays is triggering a global multiple times"). Along the way, CI validation surfaced and this PR also fixes a second long-standing VM defect:Thread.sleeptruncation by signals.Fix 1: GC trigger asymmetry for large/legacy allocations
Root cause: arrays above
CN1_BIBOP_MAX_OBJECT(512 bytes) always take the legacy allocation path. That path's only byte-based GC signal was the pre-BiBOP 1MBisHighFrequencyGCre-arm, which keeps the GC thread starting a full mark/sweep cycle every 200ms wait interval for as long as ≥1MB was allocated since the last cycle. BiBOP allocations got a modern 24MB (adaptive up to 192MB) trigger, but the legacy path was never re-tuned — it re-armed 24× more aggressively, keeping full collections running at wall-clock cadence over the whole survivor set.Change (behavioral pieces
#ifndef CN1_DISABLE_BIBOP):cn1LegacyBytesSinceGc— per-cycle byte counter (64-bit;longis 32-bit on the Windows LLP64 target) for legacy allocations, reset incn1BibopBeginGcCycle(same atomic-exchange idiom asbibopBytesSinceGc).System.gc()incodenameOneGcMalloc's legacy tail when legacy volume crossesCN1_LEGACY_GC_TRIGGER_BYTES(24MB,-Doverridable): the latch (cn1LegacyGcScheduled, cleared at cycle begin) bounds it to one schedule per cycle window, while level-triggering means a crossing suppressed by the native-bracket gate (mirrored fromcn1BibopMaybeGc, applied when conservative roots are off) is retried by the next out-of-bracket legacy allocation instead of being lost.isHighFrequencyGCre-arms atmax(1MB, bibopGcTriggerBytes)— collection scheduling is event-driven on both paths, so the 200ms loop only re-arms when volume would have crossed the trigger anyway.CN1_GC_LOG_CYCLESenv-gated cycle tracer incodenameOneGCMark(deliberately available inCN1_DISABLE_BIBOPA/B builds too; behavior-identical when unset).Measured: cycle count over the guarded workload — unfixed 15 (wall-clock cadence) vs fixed 6 (volume-driven), exactly stable across runs. On the fast-loop shape the storm showed as 10–15× inflated
large_msrounds and 4–8× higher sys CPU.Fix 2: Thread.sleep truncated by signals (found via this PR's CI)
The quieter GC exposed a deterministic Linux-suite failure (
ToastBarTopPositionmissing on both arches, 5/5 rounds). Local reproduction on the translated GTK port showedThread.sleep(3000)returning after ~20ms:usleep/nanosleepreturnEINTRon any signal and POSIX never restarts them (SA_RESTARTexcludes them), while the conservative-roots collector signal-stops sleeping threads every cycle to scan their stacks.java.util.Timerschedules throughThread.sleep, so everyTimerTaskfired almost immediately — ToastBar's 10-minute expiry dismissed the toast mid-test, which is exactly the observed re-show/poll loop timeout.Change:
Thread.sleepnow sleeps on a monotonic deadline (QueryPerformanceCounter via thecn1_win_compatshim on the MSVC target) and resumes across early wakeups, chunked <1s (POSIX allowsusleep(>=1e6)to fail withEINVALon musl — long sleeps were silent no-ops there). The deadline saturates instead of overflowing forLong.MAX_VALUE-style arguments, negative millis throwIllegalArgumentExceptionper the JDK contract (guarded in Java; the native is nowsleepImpl), and the sleeper parks withCN1_GC_PARK_CAPTUREper theCN1_YIELD_THREADcontract so its native-stack roots are scannable on Windows, which has no signal-stop fallback. Interrupt semantics are deliberately unchanged: ParparVM'sThread.interrupt()has always been flag-only (sleepnever threwInterruptedExceptionand never woke early by design), and the resume loop preserves exactly that — it neither delivers the exception nor invents a silent early wake.Measured on the translated Linux port under Xvfb:
Thread.sleep(3000)→ 3000/3000/3001/3000/3000ms (was 19–26ms); the ToastBar show/poll choreography runs to completion.Benchmark + CI regression guard
vm/benchmarks/src/com/bench/LargeArrayLoad.java— models the final Dtest (persistent small survivors; retained large arrays only; allocation-free lookups; wall-stretched phases).vm/tests/.../LargeArrayGcIntegrationTest.java(+ app resource,@Tag("benchmark")) — assertsRESULT=parity with the host JVM and gates on collection cycle count (≤10) via the tracer; load-independent, unlike wall-time assertions (verified flaky and rejected).CN1SSdiagnostics on failure, andToastBarTopPositionScreenshotTesttraces its poll state during extended waits — both were what made the sleep defect findable.Validation
CN1_GC_SIGNAL_STOP=1stop modes;RESULT=parity across host JVM / fixed / unfixed.placeObjectInHeapCollectionNULL-slot scans) are intentionally untouched — tracked by the existing TODO in the extent-snapshot section ofcn1_globals.m.🤖 Generated with Claude Code
https://claude.ai/code/session_0133QDXUjHwX9uprkupEgB29