Pace the GC against the process budget, not the device's RAM (issue #5537) - #5563
Merged
Conversation
…5537) An iPad killed a deep game-tree search with EXC_RESOURCE (RESOURCE_TYPE_MEMORY: high watermark memory limit exceeded) at 1.42GB, in _platform_memmove on a worker thread, while the same build ran fine in the simulator, on Android and on Windows. 1.42GB is the iPadOS per-process dirty-memory ceiling, so this was a limit being crossed, not a leak -- the reporter's live set was almost nothing. Returning surplus BiBOP pages to the OS (#5540) reduced retention and did not fix it, because retention was not the problem. The GC's backpressure decides how far a mutator may run ahead of the collector, and every part of it was sized against the DEVICE's free RAM. That is unrelated to the ceiling the process is actually metered against: cn1BibopPacingCap handed a high-throughput thread half of the host-wide free+inactive+purgeable figure, which on a large-RAM iPad is gigabytes. The mutator was licensed to run further ahead of the collector than the process was allowed to exist -- exactly the failure that function's own comment warns about ("removing it unconditionally let the mutator outrun the collector and balloon RSS to ~2GB"), reintroduced by measuring the wrong quantity. It could only ever bite where a per-process ceiling exists, which is why it read as "works everywhere but the device". Three parts: * cn1ProcessHeadroom reports the bytes this process has left, via os_proc_available_memory (equivalent to task_vm_info.limit_bytes_remaining without task_info's cost). It returns 0 both when there is no limit and when the limit is already exceeded -- opposite meanings, and the second is the emergency -- so a process that has ever reported a positive figure latches "has a limit" and a later 0 is read as "budget gone". Everywhere without a ceiling it returns -1 and the host-wide reading applies exactly as before, so nothing off iOS changes. * Under a budget the cap is clamped to half the REMAINING budget. The throughput clauses above it are preferences, not a licence to exceed the ceiling, and the 72MB static floor would otherwise authorize 72MB of fresh garbage with 10MB left to live. At footprint F under budget L a thread may grow to (L+F)/2, which is below L for every F, so the footprint approaches the ceiling geometrically and pacing slack alone can never reach it. * The legacy path -- everything above CN1_BIBOP_MAX_OBJECT (512 bytes), so every array a program allocates -- gains byte-based backpressure, which it never had. Its 24MB trigger only SCHEDULES an asynchronous cycle; the only thing that blocked the thread was a COUNT of pending allocations (CN1_MAX_HEAP_SIZE, free RAM over a 128-byte average object), so a thread churning multi-kilobyte arrays could run hundreds of megabytes ahead of the collector before anything stalled it. The park is gated on the trigger crossing already computed there, so the common path costs one comparison. Each path paces its own counter, so neither gets a tighter bound than it had. ProcessBudgetPacingIntegrationTest measures it, with the CN1_SIMULATE_PROC_MEMORY_LIMIT hook supplying a synthetic ceiling so the clamp is reachable off-device -- without which this fix would be as untestable in CI as the bug was. One binary, one workload, run twice. Bounded to 256MB it peaks at 95-131MB across runs; the identical unbounded control was measured at 98MB, 424MB, 525MB, 553MB and 626MB. The control's peak is reported but not asserted (it measures the scheduler, not the code) and neither is the bounded run's park count (0, 1, 2, 8 across repetitions). What is asserted is the invariant -- bounded peak below the budget -- and, deterministically, that an undeclared budget paces nothing at all, which is what keeps this from costing throughput on every other target. Teeth were confirmed by ablation: with the legacy backpressure removed the bounded run peaks at 472MB against the 256MB budget and the guard fails. Separately, the reporter could not attach a debugger at all: a Metal build died at launch with "Library not loaded: /System/Library/Frameworks/OpenGLES.framework/OpenGLES", referenced from the app binary. The template hard-links OpenGLES and GLKit, so the app declares a load-time dependency on a deprecated framework that need not be present. Both are now weak-linked; a Metal build never calls into them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4432c1a0a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark ResultsDetailed Performance Metrics
|
Contributor
Cloudflare Preview
|
…nterval Two problems with the first cut, one found by review and one by CI. The legacy park was gated on the 24MB scheduling trigger. That threshold answers "when should a cycle be scheduled", not "how far ahead of the collector may this thread run", and reusing it fails exactly where it matters most: near the ceiling the cap can be a few MB, so a workload that dirties each block before requesting the next could spend the whole remaining budget and be killed while legacy volume was still climbing toward 24MB. Pacing is now evaluated every CN1_PACING_CHECK_INTERVAL_BYTES (1MB) of this thread's own legacy allocation, unconditionally, which consults the cap long before any scheduling threshold and bounds the overshoot between two evaluations to that interval whatever the cap turns out to be. Cost is one thread-local add and one compare per legacy allocation, replacing the previous comparison. The same near-ceiling case had a second hole: CN1_PACING_MIN_CAP exists so a genuinely-live heap keeps making progress rather than stalling, but as an unconditional floor it authorized 4MB of fresh garbage with 2MB left to live, which is just a slower way to be killed. The floor is now itself capped by what actually remains; a thread that parks instead still has the spin's own safety cap as its escape hatch. And the legacy backpressure is now applied only where a per-process ceiling actually exists. It was written for that case, but nothing restricted it, and off Apple cn1_available_memory is a flat 100MB placeholder -- so the cap there is the 72MB static floor and the new park engaged constantly on machines in no danger at all. CI caught it: the guard's own control run, which declares no budget, parked 10 times on a Linux runner. Off a budgeted platform the legacy path now keeps exactly the behaviour it had, which is what the "no-op off iOS" claim requires. The BiBOP path is unaffected either way -- it was already paced against this same cap before this change. The guard's javadoc overstated the bound as a fixed fraction of the budget. The peak in fact RATCHETS toward the ceiling (measured pace points at 176MB, 216MB, 236MB under a 256MB budget), because the cap bounds uncollected allocation volume while the footprint also carries memory freed but not yet handed back. It converges from below and cannot cross, which is the property worth asserting; the text now says that rather than implying a fixed bound. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8dbb708099
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The arm64 leg failed to link LinkHelloMain with "ld.lld: error: undefined symbol: __isoc23_fscanf". glibc 2.38 redirects fscanf to __isoc23_fscanf in <stdio.h>, and the cross-linked Linux target resolves against a sysroot that has no such symbol, so any RETAINED scanf call fails the link outright. The footprint probe added for the simulated-budget hook put such a call on the GC's pacing path, which is always live. cn1LinuxResidentBytes in nativeMethods.m has had the identical call since long before this branch and links today only because it sits behind Runtime.freeMemory()'s native, which this app never calls and the dead-code pass therefore drops -- a latent landmine that would have surfaced as this same unexplained link error for the first customer to call Runtime.freeMemory() on that target. Both now parse the line with fgets + strtoul, which has no such redirect. The parse is checked against the real statm shape, leading and repeated whitespace, a zero resident field, a field 1 at ULONG_MAX, and three malformed inputs that must yield 0 rather than a garbage page count. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 424429d9a2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ss-wide Three review findings, all real. A park waits for the CYCLE BOUNDARY that resets the volume counter, but nothing guaranteed a cycle was coming. The callers' triggers fire at CN1_LEGACY_GC_TRIGGER_BYTES / bibopGcTriggerBytes, and under a tight budget the cap drops well below those -- which is exactly the near-ceiling case this exists for. A thread with a 4MB cap and 3MB of uncollected volume would find nothing scheduled, spin out its whole 10s safety budget, and resume with no reclamation even begun, once per check. cn1PacingPark now requests a cycle before waiting, guarded on !gcCurrentlyRunning so it costs a lock and a notify only when it is actually about to wait. This turns backpressure into reclamation rather than delay, and it is what lets the new tight-budget run below finish at all. The 1MB evaluation interval was per-thread, which bounds nothing on a machine with several allocators: sixteen workers can each allocate and dirty just under it without one of them reaching a check, while the shared counter and the footprint grow by sixteen times it. Crossings are now detected on the GLOBAL counter, using the pre-add value the trigger already computes, so a crossing is attributed to exactly the one allocation that passed the boundary whichever thread made it, and the bound holds however many threads allocate. It also drops the __thread state entirely: a shift and a compare on a value already in hand. And the guard could report green without ever running the code it protects. On a runner whose collector keeps up unaided, a bounded run reaches neither the cap nor a single park -- measured, bounded runs with zero parks and peaks as low as 95MB against the 256MB limit -- so the peak assertion alone would pass with the clamp and the legacy backpressure both removed. A third run now uses a 120MB budget, barely above the ~98MB structural floor, so the cap is a few MB while the workload churns 768MB and the collector's own trigger is 24MB: pacing cannot be avoided. Measured at 58, 110 and 43 parks across runs where the 256MB run recorded 0, 9 and 39. Its peak is deliberately not asserted -- below the structural floor there is nothing for backpressure to buy -- but its COMPLETION is, which is what catches a park that waits on a collection nobody scheduled. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb48b06c64
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The !gcCurrentlyRunning guard added with the scheduling fix reintroduced the stall it was meant to remove, in the case a paced thread hits most often. A running cycle is not a cycle that helps. cn1BibopBeginGcCycle resets the volume counters at the START of a cycle, so the bytes that brought a thread to the park were charged AFTER the running cycle's reset and only the NEXT cycle can clear them. Skipping the request while one is in flight therefore leaves the park waiting on a boundary that will not come: below the 24MB trigger nothing else schedules one, isHighFrequencyGC picks the 30s wait, and the thread spins out its 10s safety budget having achieved nothing -- and a thread paces precisely when the collector is busy, so this was the common case, not the corner. Requesting during an active cycle is also exactly how a follow-up is booked: System.gc() sets forceGc, and the collector loop tests it after gcMarkSweep() returns, taking LOCK.wait(200) rather than LOCK.wait(30000). The request is now unconditional; it costs a lock and a notify on a path that is about to sleep anyway. Measured on the tight-budget run, which is the one that paces on nearly every check: parks rise from 58/110/43 to 221/175/39 as each park becomes short and productive instead of a timeout, and the 256MB bounded run's peak falls from 235MB to 97-131MB, because the backpressure now produces reclamation rather than delay. Full suite 526 tests and all 7 benchmark tests green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ddd6a67950
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The tight-budget run asserted only that a legacy park occurred, and a park does not distinguish the two sizings: this workload churns 768MB, which parks against the 72MB static cap as readily as against a budget-derived one. A regression to host-wide sizing would have kept the guard green while restoring the device bug, because iOS host-wide headroom yields a gigabyte-scale cap. The cap VALUE does distinguish them, and structurally rather than by tuning. Off the budget path every branch of cn1BibopPacingCap takes the larger of a fraction of host RAM and base = bibopGcTriggerBytes * CN1_BIBOP_GC_HARD_CAP_MULTIPLIER, and the adaptive trigger is clamped to never fall below CN1_BIBOP_GC_TRIGGER_BYTES in either direction, so base is always at least 3 x 24MB. Only the process-budget clamp can produce a smaller cap. CN1_LOG_PACING_PARKS now also reports the smallest cap any thread computed, and the guard asserts the tight run is below that floor while the control is at or above it -- so it fails both if the budget stops sizing the cap and if a clamp starts applying where no ceiling exists. Measured across runs: control 73728KB every time (the floor exactly), tight 12279-18127KB, a 4-6x separation. The min is tracked only when the tracer is on. All 7 benchmark tests green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ad290fcba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two review findings. The progress floor was capped by fm itself, so below CN1_PACING_MIN_CAP it became the whole remaining headroom: at fm = 3MB the half-headroom ceiling of 1.5MB was overridden back up to 3MB. The park predicate is strictly greater, so that authorized 3MB of fresh dirty memory before any park, plus up to one unchecked 1MB interval on top -- 4MB spent against 3MB of budget, by the code whose purpose is to prevent exactly that. The cap is the volume allowed BEFORE parking and an interval can be allocated unobserved on top of it, so both now have to fit: the ceiling reserves CN1_PACING_CHECK_INTERVAL_BYTES, and the floor is bounded by that ceiling rather than by fm. Where there is no room for the floor the cap goes to zero and the thread parks on every check, which is the correct answer. Reserving the interval only binds below 2 * CN1_PACING_CHECK_INTERVAL_BYTES -- above that half the headroom is already tighter -- so nothing else moves. And the guard's child runs were unbounded. The behaviour under test is a thread PARKING, and a broken park stalls: it exhausts its 10s spin on every check, or deadlocks. Collecting the stream on the test thread blocks until the child closes stdout, so that stall would hang the surefire fork until the CI job's global timeout instead of failing -- the guard would stop reporting the regression and start eating the build, which is the worst of both. Runs now have a bounded wait with the child killed on expiry, and a timeout is asserted as a test failure naming the park as the likely cause. The drain runs on its own thread, both so a child that fills the pipe buffer cannot deadlock against our wait, and so a killed run still yields what it printed -- the only diagnostic a stalled run leaves. All 7 benchmark tests green; the guard run three more times, tight-run minCap 14383-19951KB against the control's 73728KB. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0aa99d0d69
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two more holes in the clamp, one found in review and one in a self-audit of the same code. The two allocation paths were capped INDEPENDENTLY against the same cap, so under a budget each could run a full cap ahead and the process could hold 2 * cap of fresh dirty memory. Since the clamp sets cap near fm/2, that is the whole remaining budget: two halves each respecting the limit while jointly blowing it. Under a budget both paths are now paced against the SUM of the two counters, which is what actually spends the budget. Without a budget they keep their separate bounds, so nothing off iOS tightens. And the reservation assumed the unchecked window was one check interval. It is not: the pacing check runs after the allocation is registered but before its caller writes to it, and calloc'd pages cost nothing until written, so the thread is about to dirty the whole block it just took. An 8MB array against 6MB of headroom would sail through a check that reserved 1MB and then dirty all 8MB with no further check. The cap now reserves max(CN1_PACING_CHECK_INTERVAL_BYTES, pendingBytes); the legacy site passes the allocation size and the BiBOP site passes 0, since it dirties at most one 64KB page before its next page-acquire check. When the pending block alone exceeds the headroom the cap goes to zero and the thread waits out a full cycle before dirtying anything -- which cannot conjure memory the process does not have, but gives reclamation its best chance of fitting it. The combined-volume change is visible in the guard: the bounded run now records BiBOP parks and a minCap below the 72MB static floor, where that path previously never engaged. All 7 benchmark tests green, guard run three more times (tight minCap 18127-19967KB against the control's 73728KB). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4a2c0e1ad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two review findings, both the same shape: per-thread bookkeeping that does not compose across threads, so a "process-wide" cap was not one. cn1BibopBeginGcCycle resets the volume counter and releases every waiter at once, but a parked thread's calloc'd block is still clean -- being parked so it could dirty the block afterwards is the whole point. The reset erased that block from the only figure the other waiters compare against, so sixteen threads holding individually-fitting 1MB blocks would all see an empty counter, all resume together, and dirty 16MB into whatever headroom was left. A resuming waiter now re-charges its block to the counter. That charges it exactly once (the original add was erased by the reset), and because the counter is what every waiter tests it serializes them: the next thread released by the same reset sees those bytes and waits for a later cycle. No new counter and no new reset path -- it reuses machinery whose lifecycle is already correct, and only under a budget. And the BiBOP accumulator only flushed at page acquire, so a thread holding a current page in each of CN1_BIBOP_NUM_CLASSES size classes could allocate ~1MB before flushing anything; across several allocators megabytes of real footprint stayed invisible to the cap. It now also flushes once the accumulator reaches CN1_BIBOP_PAGE_SIZE, bounding per-thread invisibility at one page. This keeps essentially all of the de-atomization it exists for: at CN1_BIBOP_MAX_OBJECT (512 bytes) that is still one atomic per 128+ allocations rather than one per allocation. The two macros are reordered so ACCOUNT no longer references FLUSH above its definition -- valid C, since macros expand at use, but it reads as a bug. Full suite 526 tests green, all 7 benchmark tests green, guard run three more times. The tight run's park count is also visibly steadier now (36/36/36 against a 35-221 spread before), which is what closing the accounting gap should do: the cap sees a figure closer to the real footprint, so the same workload makes the same decisions instead of depending on when each thread happened to acquire a page. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf4dcc571d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
✅ ByteCodeTranslator Quality ReportTest & Coverage
Benchmark Results
Static Analysis
Generated automatically by the PR CI workflow. |
Findings 8 through 13 in review were all the same defect wearing different clothes: admission was decided against an allocation-VOLUME counter while the constraint being enforced is live FOOTPRINT, and every bug was a new way for the counter to diverge from the truth. Threads deferred bytes into per-thread accumulators the counter could not see. The start-of-marking reset erased blocks that were allocated but not yet dirtied. Simultaneous waiters all observed that reset before any of them re-charged. The two allocation paths each ran a full cap ahead of a cap derived from the same budget. Each fix was correct and each exposed the next one, because the indirection itself was the defect. phys_footprint has none of those failure modes. The kernel maintains it, every thread and every non-Java allocation is already counted in it, and it is the exact figure the process is killed against. So the budgeted path now asks it directly: a thread is admitted only when os_proc_available_memory() shows room for the block it is about to dirty plus CN1_PACING_HEADROOM_MARGIN, and otherwise waits for REAL reclamation -- headroom rises when sweep frees memory, not when a cycle merely starts. A parked thread re-requests collection every 200ms, because a parked thread allocates nothing, so isHighFrequencyGC goes false and the collector would otherwise drop to its 30s idle wait while we sat out the spin budget. This DELETES rather than fixes: the bounded clamp arithmetic, its ceiling and floor and reserve, CN1_PACING_MIN_CAP, the summed-volume mode, and the post-reset re-charge. The BiBOP accumulator flush goes too -- its only purpose was making the counter accurate enough to pace against, which nothing now does -- so cn1_globals.h is byte-identical to master again. Off a budgeted platform the code is now exactly master's: the unbounded BiBOP path keeps its host-wide volume cap and the legacy path is not paced at all. The guard asserts that deterministically rather than by argument -- the control run must record boundedChecks == 0, so a regression that infers a ceiling where none exists fails the build. The margin is a deliberate cost: the process settles at limit-minus-64MB instead of creeping toward the ceiling, which also leaves room for native allocations (an image buffer, a Metal texture, a glyph atlas) that never pass through this path but spend the same budget. Measured, three runs: control boundedChecks=0 and minHeadroom=-1 (the budgeted path never runs without a budget); 256MB budget peaks at 97-196MB; 120MB budget parks 3-22 times, finishes, and bottoms out at 64799-65135KB of headroom -- the margin, which is what sustained allocation against a real budget settles at and what a host-wide reading could never produce. Full suite 526 tests green, all 7 benchmark tests green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dba5f2c50e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark ResultsDetailed Performance Metrics
|
Two findings against the headroom design. The first is a class I claimed the redesign had eliminated and had not: it removed counter DIVERGENCE, but not concurrent threads independently passing the same check. phys_footprint cannot see a block that is allocated but not yet written -- calloc'd pages cost nothing until touched -- so N mutators all read the same headroom before any of them dirties anything and each concludes it fits. Sixteen 16MB blocks all pass against 160MB of headroom and then collectively dirty 256MB. The margin bounds what ONE thread may take on top of what is already counted; it cannot bound what N threads take at once. Admission now goes through cn1PacingTryAdmit, which subtracts the in-flight blocks other threads have been admitted to dirty and tests-and-claims in a single CAS -- separate load and add is precisely what lets every waiter observe the same pre-claim total. A thread releases its claim at its next check, by which point the caller has written the block and the kernel has counted it. The second: the wait gave up on a fixed 10s timeout, so an allocation that could never fit was admitted anyway. Waiting on a clock is the wrong rule in both directions -- it abandons a collection that is still returning memory, and it keeps waiting long after collection has stopped helping. The wait now ends when CN1_PACING_BARREN_CYCLES completed collections have freed nothing useful, tracked on bibopGcEpoch. That epoch is published at cycle START, so two advances mean a full mark-and-sweep finished in between; the previous loop could time out mid-sweep having never observed a completed collection at all. The 10s bound remains only as a backstop for a wedged collector. What this deliberately does NOT do is fail the allocation. When calloc genuinely returns NULL today, codenameOneGcMalloc forces a cycle and recurses indefinitely: this VM has never had a way to fail an allocation, and adding one from a point where the block is already allocated and registered is a new capability with its own risks, not a fix to this change. It belongs in its own PR. Also drops the budgeted wait's poll from 50us to 1ms. What it waits for is a completed collection, hundreds of milliseconds away, so the finer granularity bought nothing and cost a headroom probe 20000 times a second on a thread doing no work. Same 10s bound, same 200ms request cadence. The unbudgeted path keeps its literal 200000-spin bound, unchanged from master. Measured: tight-run parks rise from 3-22 to 285-360, which is the claim doing its job -- concurrent threads now see each other's in-flight blocks -- and observed headroom bottoms at 59583-62431KB against the 65536KB margin, because the claim is subtracted from what admission may spend. Control still records boundedChecks=0. Full suite 526 green, 7 benchmark tests green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a40f21d4c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A claim is normally returned at the thread's NEXT allocation check. Two exits never reach one. A thread that EXITS takes its __thread claim with it and nothing subtracts it from the process-wide total -- and unlike a thread that merely goes idle, there is nothing left to hand it back later. Allocator-thread churn would accumulate phantom reservations until admission could never succeed and every allocator paced its full budget on every check. Released in collectThreadResources alongside the BiBOP page retire and byte flush, which runs on the dying thread, so the __thread claim is still reachable there. The claim state moves up beside cn1MonotonicMillis so that function can see it. And a thread whose wait ends on barren cycles or the backstop dirties its block anyway, having never been admitted -- so the block was never claimed and was invisible to every other thread's admission test for the window before the kernel counts it. That is exactly the over-admission the claim exists to prevent, arising in the case where memory is tightest. The block is now claimed however the wait ended. Both are the same misconception on my part: I had treated the claim as something taken on the success path, when what it has to track is "this thread is about to dirty these bytes" on every route out, death included. Measured: the tight run is markedly steadier now that admission accounts for in-flight blocks -- legacyParks 241/240/238 and minHeadroom 61519/60655/60143 across three runs, against a 3-360 park spread before the claim existed. Full suite 526 green, 7 benchmark tests green, iOS and macOS compile clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2f48d6cb8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Releasing a thread's previous claim when it next allocates assumed the earlier block had been written by then. Java guarantees no such thing: `a = new byte[32MB]; b = new byte[32MB];` allocates both before touching either, so admitting b handed back a's reservation while a's pages were still absent from phys_footprint -- which is precisely the window the claim exists to cover. Claims now accumulate within a cycle window and expire at a collection boundary. That boundary is sound where "next check" never was: a block allocated before it has either been written, so phys_footprint counts it and holding the claim would double-charge, or it is garbage, so the sweep reclaimed it and the claim is meaningless. Accumulating over-counts a thread sitting on several untouched blocks, which is the safe direction -- it only paces harder -- and every pacing park requests a collection, so under pressure boundaries arrive continuously and the accumulation stays small. This is the third correction to the claim mechanism and all three were the same mistake: assuming the VM knows when a block becomes real memory. It does not. A block counts when it is WRITTEN, nothing here observes that, so the only sound release points are the ones where the answer has stopped mattering -- a collection boundary, and thread death. Measured: tight-run parks settle at 148/151/173 with minHeadroom 62687-64335KB, against 238-365 parks before this change, because a thread is no longer handing back reservations it still owes. Control still records boundedChecks=0. Full suite 526 green, 7 benchmark tests green, iOS and macOS compile clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aced8285f9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Collaborator
Author
|
Compared 181 screenshots: 181 matched. |
Collaborator
Author
|
Compared 149 screenshots: 149 matched. |
Collaborator
Author
|
Compared 149 screenshots: 149 matched. |
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 143 screenshots: 143 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 217 screenshots: 217 matched. |
Collaborator
Author
|
Compared 144 screenshots: 144 matched. |
Both decisions lived only in PR replies, where neither the review bot nor a future contributor will find them -- and both look like bugs from the diff alone, so the next change to this code would "fix" them back. At cn1PacingExpireThreadClaim: why a claim is not held for a live untouched block. At the pacing give-up: why the allocation proceeds instead of failing.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
shai-almog
added a commit
that referenced
this pull request
Aug 21, 2026
…sue #5537) (#5573) * Drain the GC's grace pass as it walks, ending the overflow spiral (issue #5537) A deep game-tree search that #5563 saved from an EXC_RESOURCE kill came back frozen instead: GC pauses growing longer and more frequent until they were effectively continuous, with the simulator's footprint climbing to gigabytes while the app retained nothing. Both readings are the same defect, and it sits under the one #5563 fixed rather than beside it. Every cycle the grace pass walks the BiBOP page registry and marks every object allocated since the last cycle -- a fresh object may already be linked into the live graph, so it and its subtree have to survive. How many that is depends on the mutator's ALLOCATION RATE, not on the live set, and the pass pushed all of them onto a fixed 65536-entry worklist before draining any of it. A worker churning small short-lived objects produces several times that per cycle, so the worklist overflowed as a matter of course. Overflow is survivable -- the dropped entries are already marked and the belt re-discovers their children -- but the belt is a full O(heap) rescan. It makes the cycle several times longer, the mutator leaves proportionally more fresh objects for the next one, and that one overflows for certain. The collector never returns to its fast path. Every symptom on the issue follows from that single loop: the original kill by the iOS per-process ceiling, the frozen app once #5563's pacing held the process under that ceiling and had to park the mutator on nearly every allocation instead, and the simulator's climbing footprint where no ceiling exists at all. Both grace passes -- the page registry and the legacy table -- now drain when the worklist reaches half capacity. That costs nothing the end-of-pass drain would not have cost anyway, since the same objects are scanned, only sooner; what it buys is a cursor that cannot run away. The drain runs outside the trusted window (CN1_GC_TRUSTED_SUSPEND/RESUME, added because BEGIN/END save and restore a block-scoped local and so cannot express a hole inside a walk): a drain follows child words out of arbitrary mark functions, which is precisely what the resolve guard exists for. A _Static_assert pins the remaining assumption -- that a whole page of slots fits above the drain threshold -- so raising CN1_BIBOP_PAGE_SIZE fails the build rather than quietly restoring the spiral. Measured on a repro of the reporter's shape (worker thread, tree search, live set of one path). Realistic version, no ceiling: peak footprint 6.2GB -> 231MB, cycle time 6ms->750ms -> a flat 6ms, and 30% more nodes searched. Heavier version under a 512MB simulated ceiling, which is the device case: 77 of 150 cycles overflowed and the mutator parked 72 times -> 0 of 440 and no parks, 10.2s -> 6.8s, with 174MB of headroom left instead of 64MB. GcOverflowSpiralIntegrationTest guards it, asserting zero overflow cycles under a simulated ceiling and that the pass actually reached its drain threshold (else the first assertion would pass on a run that never allocated). Ablating the drain and leaving everything else in place fails it with 77 overflows. Overflow cycles are counted through a new env-gated [GC-OVERFLOW] tracer, and the count is taken with an exchange on the existing flag so it reads once per cycle rather than once per dropped push. Not addressed here, and separate: off a per-process ceiling the pacing cap is still a fraction of the HOST's free RAM, so on a RAM-rich Mac a sufficiently extreme allocator can build gigabytes of garbage before anything stalls it. A live-set-relative cap and an absolute cap were both measured and rejected -- each cost 2-4x throughput, because a volume-cap park waits out a whole collection while the footprint-based admission used under a real ceiling is both bounded and free. Extending that admission to hosts with a footprint probe but no ceiling is the right fix and needs its own benchmarking. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Keep the grace pass's periodic drain off the heap-rescan path The first cut hung the Mac Catalyst screenshot suite: the collector never finished a cycle, the EDT stacked up in the pacing park behind it, and the DeviceRunner never reached its completion marker. The hang sample is unambiguous -- the GC thread sits in gcMarkDrain called from the interleaved drain this branch added, while the EDT sits in cn1PacingPark under cn1BibopAlloc. gcMarkDrain is not "drain the worklist". Every call to it also walks allObjectsInHeap from index 0 and re-pushes every object already marked this cycle, so that anything left marked-but-unscanned by an overflow gets its mark function run. That is the right shape for the handful of calls a cycle makes, and quadratic for a caller that drains PERIODICALLY: the grace pass drains once per half-worklist, which turned one O(heap) rescan per cycle into hundreds. Split the worklist loop out as gcMarkDrainWorklist and point the two interleaved drains at it. The passes still end with a full gcMarkDrain, which is what closes the fixpoint; nothing a periodic drain leaves behind escapes it. The local guard could not see this and now can. A translated micro-benchmark holds almost nothing in allObjectsInHeap, so an O(table) drain and a cheap one measure the same -- which is exactly why this passed here and failed on a real app. GcOverflowSpiralApp now retains a reference-carrying legacy population (Object[] blocks; the rescan skips objects with no mark function, so an earlier byte[] version of this fixture was free and proved nothing), and the VM reports graceFullDrains: full drains taken while a grace pass is running. Two per cycle is all a correct implementation makes, one to end each pass. Ablating the fix by pointing the interleaved drain back at gcMarkDrain takes that from 262 across 133 cycles to 1277, and the new assertion fails. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
shai-almog
added a commit
that referenced
this pull request
Aug 23, 2026
…) (#5585) * Make the collector's cost track the live set, not the heap (issue #5537) A deep game-tree search on an iPad kept dying after #5540, #5563 and #5573. Each of those fixed a real defect -- pages never returned to the OS, a pacing cap measured against the device's RAM rather than the process budget, a mark worklist that overflowed by sheer allocation volume -- and none of them touched the reason the collector could not keep up in the first place. WHAT THE PROFILE SAYS. Three quarters of the GC thread's wall time is inside cn1ConservativeResolve. gcMarkObject calls it on EVERY reference field the drain follows, to reject a conservatively derived pointer before dereferencing it, and it answered by binary-searching two snapshots: the BiBOP page bases and the legacy extents. On the reporter's shape that is 13 dependent cache-missing loads to find the page and 15 more to miss it and find the array, per field. Marking therefore cost O(log heap) per reference: the collector got slower as the heap grew, which is exactly the reporter's "GC pauses become more and more frequent and take longer, until they are effectively continuous". Everything else followed from that. A cycle stretched to four or five times the collection interval, so the mutator produced four or five times a trigger's worth of garbage during each cycle the collector managed to finish, and the process settled at whatever the pacing allowed: 447MB against a live set of a few hundred bytes, riding 64MB below the ceiling that kills it. On device that is the kill. In the simulator, where there is no ceiling, it is the footprint climbing to gigabytes that the reporter saw next. Both indices are now open-addressed hash tables. The page table keys on the 64KB page base and stores the geometry inline, so a hit is one cache line; its keys change only when a page is registered (the registry is grow-only), so it is rebuilt on that event and only its geometry is refreshed per cycle -- which also retires the per-registration qsort. The legacy side keeps its sorted extent array for interior pointers, which only the conservative stack scan produces, and puts an exact-base table in front of it: a Java reference is always an object base, so the caller that dominates is answered in one probe. TWO THINGS THE FASTER COLLECTOR EXPOSED, both fixed here because both undo it. The survivor-heavy bypass read a pure-churn workload as survivor-heavy. Survival is measured at sweep as slots carrying the current epoch, and the grace pass MARKS every fresh non-leaf object with it -- so what the policy read as a live set was really the allocation rate. It was under the threshold before only because the slow collector inflated the denominator. With the collector keeping up it crossed, diverted 1.8M small objects onto the legacy heap, and brought the worklist overflow back (2-3 cycles in 500, from none). Pages now count the marks a grace pass put on them and the sweep subtracts them, so survival means what the policy needs it to mean. Off a per-process ceiling the pacing cap was a fraction of the HOST's free RAM, which is a reason to let a fast thread run further ahead of the collector and not a reason to accumulate an unbounded amount of garbage. On a roomy machine it evaluated to gigabytes, and once the collector lost the race early nothing brought it back: 13.8-15.7GB of footprint against a 4MB live set, and slower for it (12.2-13.4s against 8.1-8.6s bounded -- a process thrashing fifteen gigabytes pays for them). The cap is now bounded by a multiple of the collection TRIGGER, which already tracks the heap: a survivor-heavy render keeps 8 of its own enlarged triggers, pure churn is held to 8 of the base one. The bound is GATED ON FOOTPRINT, engaging only once the process is already past 512MB, because the point is to stop unbounded growth and not to stop a thread from running ahead. #5573 measured a volume cap costing 2-4x and rejected it; an ungated one measured here at 47% on the objectAllocation microbenchmark (31.6ms -> 43.6ms), for a process that was never going to grow. Gated, that benchmark is 31.1ms -- unchanged -- and the runaway is still bounded, because a runaway is by definition on the wrong side of the gate. That whole shape depends on how much RAM the host happened to have free, which is why it reproduced on an idle machine and vanished on a busy one. CN1_SIMULATE_FREE_MEMORY pins that reading so the guard means the same thing either way. MEASURED on the reporter's shape (GcOverflowSpiralApp, same host, 14.9GB allocated either way, RESULT bit-identical): under a 512MB simulated ceiling before after collections completed 130 583 triggers allocated per collection 4.67 1.04 peak footprint 447MB 116-219MB headroom left below the ceiling 64MB 277-395MB mutator parks 49-54 0 wall time 6.7-6.8s 6.6-7.0s with no ceiling and 32GB of host RAM (the simulator), eight concurrent copies so the collector has to fight for the machine, which is what tips it: peak footprint 13.8-15.7GB 819-861MB wall time 12.2-13.4s 8.1-8.6s GATES. vm/tests: 519 tests in the default group and 8 in the benchmark group, all green, including GcHeapIntegrityIntegrationTest (the CN1_GC_VERIFY use-after-free gate) and LargeArrayGcIntegrationTest (issue 5425). The benchmark gauntlet is GREEN in both cooperative and forced-signal stop modes, every torture bit-identical to the host JVM. run-benchmark.sh geomean unchanged. cn1_globals.m compiles clean to an arm64-apple-ios object against the iOS SDK, and in the CN1_GC_VERIFY / CN1_BIBOP_VALIDATE / CN1_GRACE_AUDIT / CN1_BIBOP_NO_FASTSWEEP / CN1_DISABLE_BIBOP / CN1_RESOLVE_DIAG configurations. GcOverflowSpiralIntegrationTest gains the property underneath all of it -- triggers allocated per completed collection, which is a ratio of two speeds and so reads the same on a loaded machine where a peak does not -- and a second run of the same binary with no ceiling, which is the half of the report that was previously out of scope. That second one is a bound rather than a reproduction, and says so: the off-ceiling runaway is bistable and took eight concurrent copies of the workload on a twelve-core host to provoke, which is not something a unit test should be creating. NOT ADDRESSED. UNDER a ceiling and under deliberate collector starvation (eight concurrent copies of this workload), the process still rides to the ceiling-minus-margin that footprint admission allows. Adding a volume brake to that path as well bounds it to 345MB with 165MB of headroom instead of 38MB, but costs 2.4x -- which is the trade #5573 rejected, and it is a different path from the off-ceiling growth bound added here. The per-cycle qsort of the extent array is now the largest remaining item in the collector at roughly a third of its time, and is the next thing worth replacing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Never look a zero key up in the page index, and rebuild it all-or-nothing Two defects in the new open-addressed page index, one of them the x86-64 CI failure and one from review. ZERO IS THE EMPTY MARKER, SO IT CANNOT ALSO BE A KEY. cn1ConservativeResolve is handed arbitrary machine words off a conservative stack scan and masks each one to its 64KB page base; any word below CN1_BIBOP_PAGE_SIZE masks to 0, and a small aligned integer left in a stack slot is enough. Probing for 0 matched the first EMPTY entry and returned it as a hit -- an all-zero CN1ConsPage whose slotSize the caller then divided by. The sorted array this replaced could not be reached that way, because every element of it was a real page base; the hazard arrived with the table. It reproduces on the first collection of any workload, which is why every job that runs a translated binary on x86-64 failed at once (exit 136 = SIGFPE) -- and why every local run and the arm64 leg passed: arm64 answers integer division by zero with 0 rather than trapping, so the word quietly resolved to slot 0 of a page that does not exist. Reproduced locally by building the same app for x86_64, and confirmed as the exact instruction by -fsanitize=undefined on arm64, which reports it there too (master: zero UBSan findings on the same workload; this branch before the fix: division by zero at the resolver, from the conservative native-stack scan). Both now run clean and agree with the host JVM. THE REBUILD IS NOW ALL-OR-NOTHING (review, #5585). It used to clear the live table and insert into it, growing on demand -- so a failed calloc part way through left a PARTIAL index. That is not a slow index, it is a silently wrong one: a page missing from it makes every reference into that page fail to resolve, gcMarkObject's guard skips the object, and the sweep frees it while it is still reachable. Worse, the registry is a prepend list, so a rebuild that stopped early kept the NEWEST pages and dropped the oldest -- exactly the ones holding a long-lived live set -- and did it on allocation failure, i.e. when a collection matters most. The table is now sized once from the registration count (plus slack for pages registered during the walk), filled into a fresh allocation, and published only when complete. On any failure the previous table stays in place and cn1ConsPgIndexedCount is left alone so the next cycle retries; what that table lacks is pages registered since it was built, whose objects are mark==-1 fresh and survive on the sweep's grace rule -- the exposure a page registered mid-snapshot has always had. With no previous table to keep, marking cannot proceed at all, so that case says so and aborts rather than sweep a heap it cannot resolve; it is a few hundred KB of calloc, so reaching it means the process is already finished. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Let the page index lose a race without calling it an out-of-memory The rebuild had one failure return for two unrelated situations. Outgrowing the size it picked means a mutator registered pages while it walked -- harmless, and self-correcting on the next cycle. Failing to calloc at all is not. Collapsing them meant a lost race on the FIRST build, where there is no previous index to keep, would have taken the abort() meant for exhaustion. It cannot happen in practice (the walk only covers what was linked when the head was loaded, and the slack is 256 pages), but the two cases deserve different answers regardless: a race now re-sizes and walks again, up to three times, before giving up and leaving the previous index in place. Only exhaustion with nothing to fall back on aborts, and the comment says so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Say why the extent table cannot take the same zero key The page index just had to learn that its empty marker must never be a lookup key. The extent table beside it uses the same marker and is safe for a reason that lives twenty lines away -- cn1ConservativeResolve rejects a zero word before either table is consulted, and no extent has a zero base. Write that down where the probe is, so the next restructuring knows the early return is load-bearing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Stop asserting collector throughput on a runner that is starving it The triggers-per-cycle assertion I added went red on CI at 4.42, and the claim attached to it -- that the ratio is "a property of the two SPEEDS and not of either", so it reads the same on a loaded machine -- is simply wrong. The mutator is one hot allocation loop; a collection has to interleave a mark, a sweep and a page walk with it, so under contention the collector is the one that loses. Measured on this workload, triggers allocated per completed collection: before this branch after a core to itself 4.67 1.04 8 copies on 12 cores 4.01-4.74 2.30-2.71 16 copies on 12 cores - 3.28 CI: 4 forks, 4 vCPU - 4.42 The old collector was bound by its own cost rather than by the CPU it could get, so its number barely moves; the fixed one is bound by the CPU, so its number walks up to meet it. They converge, and no fixed threshold separates them on an oversubscribed runner. The CI figure is that convergence, not a regression: the same job's run took 77954ms against the 5804ms this workload needs alone. The no-ceiling peak has the same shape and for a concrete reason. The growth bound works by parking a mutator that has run too far ahead, and a park gives up after two barren collections so that a thread can never be stalled by a collector that is not running. Starve the collector enough and every park gives up, so the bound stops binding: sixteen-way, the copies peak between 735MB and 15.7GB, against 819-861MB eight-way where the collector still gets to run. That assertion would have gone red next. Both are now enforced only when the run had the machine, measured by the workload's own elapsed time -- it is a fixed number of rounds, so that is a direct reading of the CPU it got. Both numbers are PRINTED on every run either way, and a contended run says which one it was and why. What this class still enforces unconditionally is the part that is a property of the code: zero worklist overflows, the bound on full drains taken inside a grace pass, and staying under the ceiling. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Restore the collector fix, and never sweep on a mark the index could not support TWO THINGS, one of them entirely my fault. THE PREVIOUS COMMIT REVERTED THE FIX. While measuring master as a baseline I ran `git checkout origin/master -- cn1_globals.m cn1_globals.h`, which does not just write the worktree -- it STAGES what it writes. I restored the worktree afterwards, saw the resulting `MM` in git status, and committed a test-only change on top; the staged master copies went with it. 460 lines of cn1_globals.m disappeared in a commit whose message is about a test assertion. That is why CI then reported the old tracer format and why the review found CN1_SIMULATE_FREE_MEMORY, CN1_BIBOP_GC_MAX_CAP_MULTIPLIER and the allocatedKb / triggerKb fields "absent from this commit's target tree" -- they were absent, exactly as reported. Both files are restored to their d5c018f content and the index was diffed against the worktree before committing this time. A STALE PAGE INDEX MUST STOP THE SWEEP, NOT JUST THE REBUILD (review, #5585). Keeping the previous index when a rebuild fails is safe for ONE cycle: the pages it is missing were registered after the last successful rebuild, so their objects are mark == -1 and the sweep's grace rule keeps them. It is not safe for two. On the next failed rebuild those objects are no longer fresh, they still do not resolve -- so gcMarkObject's guard skips them however reachable they are -- and they age into the m < V - 1 reclamation with live fields still pointing at them. The fallback traded a hard failure for silent corruption in the low-memory case that motivated it. A failed rebuild now marks the cycle's mark as unsound and codenameOneGCSweep reclaims nothing on it. Skipping a collection costs the memory that cycle would have returned; sweeping on an incomplete mark costs the heap. It is self- correcting -- the rebuild is retried every cycle and the first success marks the whole live set before anything is freed again -- and it subsumes the empty-index case, so the abort() added for that is gone: nothing is swept, so nothing is lost. The blocked-thread release still runs on both paths, or a thread parked on the collector would hang instead. Exercised rather than assumed: with two of every three rebuilds forced to fail, the skip path runs, the throttled report fires, and RESULT stays bit-identical to the host JVM. The same fault injection under CN1_GC_VERIFY -- which walks every survivor's fields after every sweep and aborts on a reference into reclaimed memory -- is running as this goes up and is clean so far; it is slow enough that it outlasts the push, and the result follows on the PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Probe the footprint when the growth bound would bind, and stop gating the test on it TWO REVIEW FINDINGS (#5585), the first of which corrects my own diagnosis. THE GROWTH BOUND WAS READING A STALE FOOTPRINT. It keys off cn1CachedProcFootprint, which cn1RefreshFreeMemCache samples once, at mark start. A cycle that begins just under the 512MB floor therefore keeps a below-floor reading for its whole duration, so cn1BibopPacingCap goes on granting the host-derived cap -- gigabytes on a roomy machine. A LONG CYCLE IS EXACTLY THE RUNAWAY THIS BOUND EXISTS TO STOP, so the clamp sat disarmed through the one interval that mattered. The footprint is now re-probed at the point of use, after asking whether the bound would bind at all so the syscall is paid for only on the path that needs it, and rate-limited to one probe per 25ms across all threads. That caps the overshoot at a refresh interval's worth of allocation instead of a collection's. I had attributed the same measurement to the wrong cause. The earlier note said the bound stopped binding under starvation because a pacing park gives up after two barren collections. That is true and still a limit, but it was not what produced the number: with the probe fixed, the same sixteen concurrent copies that peaked between 735MB and 15.7GB now peak between 871MB and 994MB, and twenty-four copies -- whose slowest run takes 118s, against the 78s of the CI job that motivated all this -- peak between 880MB and 1009MB. RESULT stays bit-identical throughout. A GATE THE REGRESSION CAN TRIP IS NOT A GATE. The no-ceiling peak assertion was gated on the run's own elapsed time, and the regression it guards makes the run slow: the test's own numbers put the broken behaviour at 12.2-13.4s against a 12s gate, so the failure could satisfy the skip condition and take the benchmark green. That gate is gone. The bound now holds under contention beyond anything CI applies, so the peak is asserted unconditionally and there is nothing left to disable. Triggers-per-cycle keeps no assertion at all -- it is a ratio of two speeds that converges on the broken collector's as the runner is oversubscribed, so no threshold separates them there and a gated version would have exactly the defect above. It is printed every run as a diagnostic, with the numbers and the reason in the javadoc. The sweep guard from the previous commit is exercised rather than assumed: with two of every three index rebuilds forced to fail, GcHeapIntegrityIntegrationTest -- the CN1_GC_VERIFY gate that walks every survivor's fields after every sweep and aborts on a reference into reclaimed memory -- passes, and the spiral workload's RESULT stays bit-identical with the skip path firing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
shai-almog
added a commit
that referenced
this pull request
Aug 25, 2026
… (issue #5537) (#5599) * Stop the SATB barrier logging fresh references (issue #5537) Four merged fixes (#5540, #5563, #5573, #5585) each named a mechanism and the reporter's build still climbed 500MB to 5GB in five minutes on the iOS Simulator with a live set of a few hundred objects, GC pauses lengthening until they were continuous. The reason none of them settled it is structural: every GC workload in vm/tests measures a PEAK under load, and a heap that grows forever at a modest rate passes "peak < 2GB over 50 rounds" without difficulty. Nothing measured whether the VM ever gives the memory back. The instrument comes first, and it is what found this. -DCN1_GC_CONFORM adds a probe that PARTITIONS the footprint -- resident pages, legacy blocks, the legacy table, the allocator's side tables -- and prints the residual the four do not account for, plus a per-phase breakdown of the mark. It deliberately is not CN1_GC_VERIFY: that flag forces cn1BibopReleaseOffset() to 0, which compiles out the page-release path, the major sweep and every madvise call, so the paths a footprint investigation is about cannot be measured in a verifier build. It changes no allocator behaviour, and the emitters are gated at RUNTIME on CN1_GC_PROBE so probe-on and probe-off are the same binary. On the reported shape -- a deep game-tree search on four workers, tiny short-lived reference-carrying objects, a constant live set -- it named the cost immediately: of a 327ms mark, 282ms was SATB termination, draining 2,718,448 logged references in one cycle. Of those, 2,718,413 were references to FRESH objects. A mark == -1 object was allocated after the cycle's snapshot was taken, so it is not in the snapshot the barrier exists to preserve, and both sweeps keep it anyway -- the grace rule promotes a fresh slot to the current epoch instead of freeing it. Its own outgoing references to non-fresh objects are still logged by the same barrier as they are stored, so nothing reachable only through a fresh object is lost, which is the hazard the insertion half was added for. Without that filter the log is a feedback loop rather than a cost: its size is mutation rate times cycle duration, draining it is part of the cycle, so a longer cycle logs more and logging more lengthens the cycle. Both reported symptoms fall out of the one loop -- the footprint climbs because the collector never catches up, and the pauses climb because the log it has to drain keeps growing. Measured, three repetitions each, interleaved in one session: footprint drift before 306,684 / 241,493 / 224,237 KB/min after -31,430 / 36,866 / 18,993 KB/min (noise around zero) page count before 3,947 -> 5,995 over 40s and still climbing after flat at 11,687 for 40s mark time before 38ms -> 180ms; after 9-68ms, no trend under a simulated 1.4GB per-process ceiling: 3.5x the search throughput (237.8M nodes vs 67.6M), peak 1271MB, no kill Throughput, interleaved A/B, checksums bit-identical: geomean 0.944 -- 5.6% faster overall, objectAllocation 1.73x (56.3ms -> 32.5ms). The barrier was that expensive. -DCN1_SATB_LOG_FRESH restores the old behaviour for A/B. GcSteadyStateIntegrationTest is the gate. It asserts the SATB log stays sized by the live set rather than by the allocation rate, and that the page heap stops growing in the second half of the run; then it rebuilds with -DCN1_SATB_LOG_FRESH and requires both to fail, so it cannot go inert. Two pre-existing defects found on the way and fixed here: * -DCN1_DISABLE_CONSERVATIVE_GC_ROOTS, the revert path cn1_globals.h documents, did not compile at all: the grace passes use CN1_GC_TRUSTED_BEGIN/END/SUSPEND/ RESUME unconditionally and those are only defined with conservative roots on. No-op definitions restore it, which is what makes it usable as an A/B arm. * [GC-INSTR] allocs= is not an allocation count -- CN1_FAST_NEW's inlined bump path never reaches that counter, so on a small-object workload it understates allocation by orders of magnitude. Renamed to outOfLineAllocs= with a note. Verified: 520 vm/tests non-benchmark tests green; all six GC benchmark tests green; run-gc-verify.sh green including both fault self-tests; run-gauntlet.sh green with every checksum matching; grace audit reports doomedChildren=0 with and without the filter; and the probe compiles across nine ablation flag combinations. Not addressed, and pre-existing: under a per-process ceiling the process still rides to ceiling-minus-64MB, which #5585 flagged as open. That is now a bounded plateau rather than unbounded growth, but the margin is thin on a device where the renderer shares the same budget. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Defend a headroom reserve under a per-process ceiling (issue #5537) The previous commit stopped the heap growing without bound. This one stops the process parking itself on the kill line, which #5585 flagged as open and which is what turns a native spike into a jetsam kill. Budget headroom is not a footprint bound. Admission against os_proc_available_memory answers only "is there budget left", so it keeps saying yes until the budget is gone. Measured on the issue-5537 game-tree shape under a simulated 1.4GB ceiling, seven times: 1,271MB resident and 63MB of headroom left, every time, against a live set of a few hundred objects. That repeatability is the tell -- it is not an accident of the workload, it is the policy converging on ceiling minus CN1_PACING_HEADROOM_MARGIN by construction. The ceiling is not special either: give the same workload an 8GB budget and it rides to 7.5GB. There is no footprint TARGET anywhere in the design. 63MB is the whole margin, and the renderer spends out of the same budget -- #5598 measured one screen texture at 30MB. So the collector now also bounds how far the mutator may run ahead of it, but only once headroom drops inside a reserve of a quarter of the budget (CN1_PACING_RESERVE_SHIFT). Inside the reserve the mutator is clamped to the static cap, the collector gets ahead, and the footprint falls back out. Gating on HEADROOM rather than on footprint is what makes this affordable: it is a control loop that engages only inside the reserve, not a tax on every allocation, and volumeParks in the [PACING] report is 0 for a run that never enters it. Both allocation paths are charged against ONE figure. Bounding them separately is a defect this code has had before -- each running a full cap ahead of a cap derived from the same budget -- and the reserve is derived from the BUDGET, never from the device's free RAM, which is the defect #5563 fixed. cn1BibopPacingCap is deliberately not reused for that reason. Measured, builds interleaved within one session (-DCN1_PACING_NO_RESERVE is the same binary with the bound compiled out), simulated 1.4GB ceiling, four workers: peak footprint smallest headroom seen no reserve 1271MB, x7 63MB, x7 reserve limit>>2 1027-1036MB 298-304MB 4.8x the margin. Throughput across seven interleaved pairs came out at 0.90 to 0.99 of the unbounded build, median 0.94; the spread is session drift, not the bound, and the sign never changed. A single repetition each of the tighter reserves put >> 3 at 1183MB/150MB and >> 4 at 1207MB/127MB, both slower -- a smaller reserve engages later and thrashes closer to the edge -- so a quarter is the knee rather than a compromise. Roughly 6% for that margin is a different trade from the volume brakes #5573 and #5585 measured at 2-4x and rejected. It cannot touch a platform with no per-process budget, because the whole branch is unreachable there: vm/benchmarks measures geomean 0.9398 against master, i.e. still 6% FASTER from the previous commit's SATB fix, with no benchmark regressing and every checksum identical. cn1PacingPastGrowthFloor's rate-limited footprint probe is factored out as cn1PacingFootprintNow so both bounds read through it. Behaviour-preserving: each of its three early returns previously answered FALSE, and the fast path above already established that the cached value is under the floor. GcSteadyStateIntegrationTest gains a third scenario asserting the process defends its reserve under a simulated ceiling, and a fourth that rebuilds with -DCN1_PACING_NO_RESERVE and requires the third to fail -- otherwise a gate that never engages would report green forever. Verified: 520 vm/tests non-benchmark tests green; all seven GC benchmark tests green (ProcessBudgetPacingIntegrationTest included, which exercises the same budgeted path); run-gc-verify.sh green with both fault self-tests; run-gauntlet.sh green with every checksum matching; nine ablation flag combinations compile. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Give the benchmark driver its GPL header, and scope two helpers to their use check-copyright-headers rejects a new source file without the complete Codename One GPLv2 + Classpath Exception header, and vm/benchmarks/src is in scope. cn1PacingUncollectedBytes and cn1PacingReserveBytes are used only from the reserve bound, so they are guarded on the same condition it is -- otherwise compiling the bound out with -DCN1_PACING_NO_RESERVE leaves them as unused statics. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Count benchmark nodes per worker, not through a shared racy counter The driver incremented one static long from four workers with an unsynchronised read-modify-write, and the sampler read it concurrently. That is not merely imprecise: the rate at which increments are lost depends on CONTENTION, and contention is exactly what differs between the builds this benchmark compares -- a build whose threads park more loses fewer increments and so reports a throughput advantage it has not got. The per-round `nodes = localNodes` writeback also overwrote the shared total instead of combining the workers' counts. Each worker now counts into its own slot, and NODES= is summed after join(), which gives it a happens-before edge to every worker's last write. The SAMPLE series sums the same slots while they are still being written, so it is renamed nodes~= and documented as a progress indicator rather than a measurement. The CI fixture (GcSteadyStateApp) never had a node counter -- its assertions come from the [GCPROBE] series -- so nothing the gate asserts is affected. Reported by chatgpt-codex-connector on #5599. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Keep every probe row a single-cycle row, publish node counts live Two review findings on #5599, both real. cn1GcProbeCycle returned early on a skipped cycle without clearing the phase accumulators, so with CN1_GC_PROBE>1 snapMs/graceMs/satbMs and friends carried a whole interval while markMs and sweepMs described only the cycle that just ran -- two time bases in one row, which would attribute an interval's worth of a phase to a single cycle's pause. The resets move into cn1GcProbeResetPhases and run on every cycle, printed or not. The cumulative counters (matured, consWords, staleSkips) are deliberately left alone: those are running totals the reader diffs. The benchmark driver published each worker's node count only after the run stopped, so every SAMPLE line reported zero. It now republishes once per round; a worker that stalls stops publishing and its slot going flat is the signal. Neither affected any measurement reported so far -- every run used CN1_GC_PROBE=1, where the skip path is unreachable, and the throughput figures come from NODES=, which is summed after join(). Also corrects the reserve's throughput figures, which came from the racy counter the previous commit replaced. Re-measured with the exact one, four interleaved pairs: 0.97-1.05 of the unbounded build, median 0.99, two of four faster with the bound on. The previous "median 0.94" overstated the cost. Peak footprint and headroom are unchanged (1271MB/63MB against 1015-1027MB/306-308MB) -- those come from the probe and Runtime, not the counter. The claim that a smaller reserve is "slower" is withdrawn; >>3 and >>4 buy less on peak and headroom, which is the argument that survives. Reported by chatgpt-codex-connector on #5599. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Load the mark word atomically in the barrier, and harden the ceiling scenario Three findings, two from review and one the review's tighter test surfaced. The SATB filter read __codenameOneGcMark with a plain load while the marker reaches the same field through __atomic_*. That is a mixed atomic/non-atomic access to one object -- undefined in C, and the same bug class #5598 fixed in the constant pool. The concrete hazard is not tearing but the compiler caching a -1 across several inlined barriers in one loop, which would keep suppressing entries after the object had aged into a genuine snapshot object. Now __ATOMIC_RELAXED, and the comment says why relaxed and not acquire: nothing is published through this read, both stale answers are safe, and what relaxed buys is that the load happens at all. An acquire fence on every object store buys nothing over that and is not free on arm64. The two CN1_GC_CONFORM census reads of the same field move with it. The fault-injected runs' measurements were accepted without checking exit status or the completion marker, so a build that crashed after emitting enough probe rows would have satisfied the assertions and turned a memory-safety regression into a green gate. Both now go through assertHealthy first. The ceiling scenario used a 1400MB budget, which needs the mutator to actually outrun the collector by 1.3GB -- and how far it outruns depends on how many cores it has to itself, so a two-core runner might never get there and the fourth scenario would go quietly inert. It now uses 768MB, which admission converges on by construction rather than by winning a race. The threshold between the two regimes becomes ABSOLUTE, twice CN1_PACING_HEADROOM_MARGIN, because the margin does not scale with the budget: a proportional threshold silently stops separating them as the budget shrinks, which is exactly what happened at 400MB (reserve 100MB, margin still 63MB, half the reserve below it). Reported by chatgpt-codex-connector on #5599. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Do not size an adopted BiBOP slot as if it were a malloc block The probe sized every non-null allObjectsInHeap entry with malloc_size / malloc_usable_size. A MATURED object is in that table but its storage is a slot inside a posix_memalign'd BiBOP arena, so the pointer is interior: glibc's malloc_usable_size reads the chunk header immediately below it and returns a garbage figure, and CI runs this gate on Linux. Its bytes are also already counted in residentPgBytes, so anything it did return double-counted into the residual that is this probe's whole point. Only an object the table INDEXES (__heapPosition >= 0) owns an individual block. The rest are counted as legAdopted instead -- the same population as matured - maturedDied but measured from the table rather than from the counters, so the two disagreeing is itself a finding. Not a small corner: on the game-tree workload legAdopted is 32,907 of a legUsed of 33,164, so 99% of the table was being sized this way. It was harmless on macOS only because malloc_size answers 0 for an interior pointer, which is also why legBlockKb read flat through the original investigation and correctly never carried the drift. Verified after the change: run-gc-verify.sh green with both fault self-tests, and vm/benchmarks geomean 0.9422 against master (0.9398 before the previous commit's atomic load, i.e. that load costs nothing), no benchmark regressing, checksums identical. Reported by chatgpt-codex-connector on #5599. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Close the wait timer at the wait, read the cycle counter atomically Three findings from review; two fixed, one measured and answered in the code. waitMs was opened before the safepoint wait and closed only after the allocation migration and both stack scans, so it double-counted work already attributed to migrateMs and stackMs -- a phase breakdown that overlaps reads a long root scan as mutator wait time, which is the opposite of what it exists to say. It now opens and closes around the wait alone, inside the lightweightThread branch, so a native thread (which is never waited for) contributes 0 instead of everything up to markStatics. The 1Hz emitter read currentGcMarkValue with a plain load while the collector increments that ordinary int -- a data race, in the one emitter documented as "atomics only" and built to keep reporting exactly when the collector is stalled. Now an atomic relaxed load, as is the mutator-side comparison in the SATB census. Not taken: requiring the -DCN1_SATB_LOG_FRESH build to also blow the second-half page-growth bound. Measured across two runs of that build, its second-half growth is 0.446 and then 0.033 -- a runaway's page pool sometimes saturates before the midpoint and the ratio then reads flat while the heap is enormous. That assertion would fail about half the time, and a coin-flip gate is worse than the inertness it guards against. The reasoning, the numbers and what does have teeth (the SATB metric, five orders of magnitude, every time) are recorded on the constant. Both series are now printed on every run so the ratio stays auditable rather than merely asserted. Verified after these changes: phases sum to markMs with no overlap (16.0 of 16.3); 520 vm/tests non-benchmark tests green; all seven GC benchmark tests green. Reported by chatgpt-codex-connector on #5599. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Read the collector's atomic epoch mirror, and claim a matured page with one edge Two follow-ups from review, both correct. The previous commit made the 1Hz emitter's read of currentGcMarkValue atomic while codenameOneGCMark still increments it with a plain ++. That is half a fix: an atomic read of a plainly-written object is still a mixed access and still undefined. Both sides now go through bibopGcEpoch, the collector's own _Atomic mirror of the same value, published at cycle start -- which is what the reviewer offered as the alternative and what should have been used first. The mutator-side comparison in the SATB census moves with it. Where there is no page heap there is no mirror, so the emitter reports cyc=-1 rather than a figure read through a data race. cn1MaturedPages tested gcHasAdopted and then let the existing plain store set it. The CAS above guarantees one thread matures a given OBJECT, but two markers can mature two different objects on the SAME page, so both could observe FALSE and both count it -- and the plain store is itself a data race the moment gcMarkResolveThreadCount stops returning 1. Now one __atomic_exchange_n: exactly one thread sees the FALSE->TRUE edge, and it does the counting. That the ratio is read chiefly in the CN1_GC_MARK_THREADS>1 arm is the point -- it would have been wrong exactly where it is used. Verified in that arm: maturedPages=2121 of pgTotal=11214, a plausible ratio rather than an inflated one. run-gc-verify.sh green with both fault self-tests; the steady state, heap integrity and process budget gates green; seven ablation combinations compile. Reported by chatgpt-codex-connector on #5599. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Make every collector-side write to the mark word atomic The barrier's read was made atomic two commits ago while gcMarkObject still stamped the same field with a plain store, so the pair was still a mixed access. The field already had an atomic convention here -- gcMarkObject's own read is __ATOMIC_ACQUIRE, the BiBOP publish is __ATOMIC_RELEASE -- and the plain writes were the inconsistency, not the new read. Every write that can run concurrently with a mutator is now a relaxed atomic store: gcMarkObject's stamp, both sweeps' grace promotion, both free-mark stores, the nursery promotion and the CN1_GC_VERIFY poison. Relaxed compiles to the same instruction on every target we build; what it buys is that the write is a write the reader is allowed to observe. Header INITIALISATION deliberately stays plain, in codenameOneGcMalloc and in cn1FusedInstallPrimArray. Those are not concurrent with anything: the barrier only ever reads the mark of an object the mutator holds a reference to, so one already published, and the publishing store orders the initialisation against any reader. That distinction is not free-floating -- making those two atomic as well cost 1.2 points of benchmark geomean (0.9550 against 0.9432, with arraySequential, quicksort and valueEscape all moving and returning), because they sit on the allocation fast path. The reasoning is recorded at the site so the next person does not reintroduce it for symmetry. Verified: vm/benchmarks geomean 0.9432 against master, six rounds interleaved, no benchmark regressing and checksums identical; run-gc-verify.sh green with both fault self-tests; all seven GC gates green; five ablation combinations compile including -DCN1_NURSERY. Reported by chatgpt-codex-connector on #5599. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Emit the generated mark chain's root store atomically too The previous commit converted every hand-written collector-side write of the mark word and missed the one that matters most, because it is not in the C sources at all: ByteCodeClass emits the root of every generated mark chain, and that store was still plain. It runs on the GC thread for every object marked while the SATB barrier atomically loads the same field from mutators, so the pair stayed a mixed atomic/non-atomic access -- the exact defect the previous commit was for, in the one place a grep of cn1_globals.m could not see. Costs nothing, as the hand-written conversions did not: vm/benchmarks geomean 0.9387 against master over six interleaved rounds (0.9432 before this change, so inside the noise), no benchmark regressing, checksums identical. A codegen change touches every translated class rather than one runtime path, so it is verified against the shapes rather than the sites: run-gc-verify.sh green with both fault self-tests, and run-gauntlet.sh green with every checksum matching. Reported by chatgpt-codex-connector on #5599. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Publish the sampler's counters, and keep the page partition valid under a race Two findings, one taken as offered and one taken but answered differently. The benchmark driver's per-worker slots were published with a plain long[] write against a concurrent reader: no visibility guarantee, and Java 8 permits a 64-bit element to be observed torn, so the live series could sit stale or jump nonsensically exactly when a stalled worker is what it is meant to show. Publication and sumNodes() now share SUM_LOCK. Once per round is about once a second per worker, so it costs nothing, and NODES= after join() remains the authoritative figure regardless. The probe's page walk is a different case. It reads plain page counters while mutators run, which is a race, but it is the same deliberate sample cn1HeapAccounting takes beside it -- "a diagnostic wants the shape, not the last digit" -- and both offered remedies cost more than the unsoundness. Stopping the page owners would perturb collector/mutator timing, which is the quantity this probe reports, and would cost CN1_GC_CONFORM the behaviour-neutrality that is the only reason it is a separate flag from CN1_GC_VERIFY. Making the page fields _Atomic would put atomic accesses on the inlined bump path in cn1_globals.h, the hottest code in the VM, to improve a diagnostic. What is worth fixing is the harm actually named: an internally inconsistent partition. Only an owned page can move under the walk -- at most one per size class per thread out of many thousands -- so freeCount is clamped into [0, bumpIndex] and a stale pair can no longer make live and dead slots sum past the page. Verified: 517295 + 25326 KB against a 776448 KB reservation. The reasoning is recorded at the walk so the next reader does not have to rediscover which of the three options was chosen and why. Verified: run-gc-verify.sh green with both fault self-tests; steady-state, heap integrity and process budget gates green; the sampler now tracks progress live (nodes~=28,697,812 mid-run against a final NODES=34,360,526); and the 520-test non-benchmark suite is green on the regenerated code from the previous commit. Reported by chatgpt-codex-connector on #5599. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Survive a stall: publish inside the traversal, bound the run Both findings are the same blind spot from two directions -- a stalled collector is one of the things this gate exists to CATCH, and neither the progress series nor the runner survived one. Publishing between rounds was not enough. One depth-14 traversal is millions of nodes, so if the collector stalls badly enough that no round completes inside the window, nothing is ever published and the series reads zero -- silent in exactly the case it is for. It now also publishes every 1<<20 nodes: a power of two so the test is an AND, coarse enough (about a fifth of a second of work) that the lock traffic is negligible against the sampler's 4Hz. Verified live: 0 -> 4,194,304 at 1s -> 33,554,432 at 9.8s, against a final NODES=35,255,230. The runner read the child's output to EOF on the test thread and only then called waitFor(), so a hung workload would block until the CI job's global timeout -- the guard would stop reporting a regression and start eating the build. It now drains on a background thread and waits with a bound, killing the child on expiry and failing with whatever it printed, which is the only diagnostic a stalled run leaves. That is not a new invention: GcOverflowSpiralIntegrationTest and ProcessBudgetPacingIntegrationTest both already do exactly this, and the naive pattern came from copying GcHeapIntegrityIntegrationTest, which is the one that does not. Reported by chatgpt-codex-connector on #5599. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Do not filter fresh SATB entries where there is no insertion barrier The filter's soundness argument ends "its non-fresh children are still logged by this same barrier as they are stored". That step has a precondition I did not state and did not check: the INSERTION half has to exist. Under CN1_NURSERY it does not. CN1_WRITE_BARRIER is the nursery remembered-set update there and enqueues nothing at all (cn1_globals.h:1020-1039), so a fresh container that takes an older child after the grace pass has that child recorded nowhere -- and dropping the deletion entry for the container then lets the sweep reclaim a child the grace-surviving container still references. That is a use-after-free, in the class of defect #5425 and #5442 were about. The filter is an optimisation and not a correctness requirement, so a build without the insertion half simply does not get it: the condition is now !defined(CN1_SATB_LOG_FRESH) && !defined(CN1_NURSERY). Adding SATB insertion to the nursery barrier was the other option offered and is the riskier one -- it changes barrier behaviour in a configuration nothing exercises, and would have to be justified by measurements no one can take. Latent rather than live: CN1_NURSERY is not defined anywhere in-tree, so no shipping or CI build takes that path. It is a documented, reachable flag, and the comment now records the dependency so the next person to enable it is not relying on an argument that quietly stopped holding. Verified: five ablation combinations compile including -DCN1_NURSERY; run-gc-verify.sh green with both fault self-tests; steady-state and heap-integrity gates green. Reported by chatgpt-codex-connector on #5599. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Let CN1_WL_LEGACY=0 actually ablate the legacy population Setting the documented knob to 0 built a zero-length legacyLiveSet and then indexed [-1] on the last line, so the driver threw AFTER the entire timed run had been paid for -- losing RESULT and GC_STEADY_STATE_DONE, which is everything the run was for. Running without the retained legacy population is a legitimate ablation, so it now works rather than crashing: the fold is skipped when there is nothing to fold. Two neighbouring values that would produce a wasted or silently empty run are clamped at the same time. A negative CN1_WL_LEGACY reached new Object[n][]; a CN1_WL_THREADS below one started no workers at all and reported that only by printing zero nodes, which is the exact failure mode -- a measurement that looks like a result -- this whole change has been about. WLCONFIG prints the clamped values, so the log says what actually ran. Verified: CN1_WL_LEGACY=0, CN1_WL_LEGACY=-5 with CN1_WL_THREADS=0, and the defaults all reach RESULT and GC_STEADY_STATE_DONE. Reported by chatgpt-codex-connector on #5599. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Check the answer, not just the telemetry, in every scenario Three of the four runs checked exit status and the completion marker but never that the workload still computed the right thing. That gap matters most exactly where it was left: the ceiling scenarios exercise the budgeted pacing path -- the code this change touches most -- under an environment the clean run never sees, so a worker could die early or compute a wrong sum while the process still exited cleanly and emitted plenty of [PACING] telemetry for the policy assertions to pass. None of the variants changes what the program computes: the faults injected are a barrier filter and a pacing bound, and the workload is deterministic by construction (fixed rounds, fixed seeds, an order-independent checksum). So RESULT must equal the host JVM's in all of them, and assertHealthy now requires it -- which also picks up the -DCN1_SATB_LOG_FRESH run, which had the same gap. Reported by chatgpt-codex-connector on #5599. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Normalise every workload knob, not just the one that was reported CN1_WL_MOVES=0 left the move chain null and the next-seed derivation dereferenced it, so the leaf-only ablation died with an NPE on the first node. That is the second knob found this way, so this fixes the class rather than the instance: all eight are normalised in one place before the timed run, and WLCONFIG prints the normalised values so the log says what actually ran rather than what was asked for. Auditing the rest turned up one more that was worse than the reported one. A negative CN1_WL_DEPTH never matches the d == 0 base case, so it recursed until the stack gave out. CN1_WL_SECONDS and CN1_WL_BRANCH below their floors produced runs that measured nothing and said so only by reporting zero -- the failure mode this entire change is about. Zero stays meaningful where it means something, and both cases are real ablations: no retained legacy population, and no reference-carrying Move per node. The second is worth having, because only a non-leaf object reaches the grace pass's worklist or maturation, so leaf-only allocation is a genuinely different workload for the parts of the collector under test. Verified: CN1_WL_MOVES=0, CN1_WL_MOVES=-3, CN1_WL_DEPTH=-1, CN1_WL_BRANCH=0, CN1_WL_SECONDS=0 and CN1_WL_LEGACY=0 all reach RESULT and GC_STEADY_STATE_DONE, and the default configuration is unchanged. Reported by chatgpt-codex-connector on #5599. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Flag the probe row when the collection cycle threw gcMarkSweep wraps mark and sweep in a catch-all so a throwing finalizer cannot wedge the collector. On that path control jumps past the timing assignments, so the probe emitted a row carrying the PREVIOUS cycle's markMs and sweepMs beside the partial current cycle's phase counters -- two cycles in one row, and it concealed the exceptional cycle, which is the one a reader most wants to see. This is the same defect as the CN1_GC_PROBE>1 skip path fixed earlier, on a different route out. The timings are now cleared BEFORE the protected region, so a throw cannot inherit them, and the row carries threw=1 rather than being suppressed: hiding it would defeat the reason this probe has a wall-clock emitter at all. The three carriers are file scope, so the setjmp/longjmp indeterminate-local rule does not apply to them. Verified: five ablation combinations compile; run-gc-verify.sh green with both fault self-tests; steady-state and heap-integrity gates green; probe rows carry threw=0 on a healthy run. Reported by chatgpt-codex-connector on #5599. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Re-evaluate the reserve throughout the wait, and stop the driver perturbing itself Two review findings, and a third defect the first one's verification exposed. The wait loop's copy of the volume bound was guarded on the thread having already been refused, so it could only transition refused->allowed. A thread that parked on BUDGET while outside the reserve then held a stale "allowed" for its whole wait and could be admitted on headroom alone after other mutators had pushed the uncollected total past the cap and the process into the reserve. There is now ONE definition, cn1PacingVolumeOk, called from both sites and recomputed every iteration -- the two copies drifted precisely because they were two. The gate parsed only the per-cycle [GCPROBE] rows, so a collector that completes its early cycles and then never finishes another was invisible to it: the rows stop, the generated main returns as soon as the workers do, and the process exits cleanly with the marker while the heap is still growing. [GCPROBE-T] was added for exactly that state and then not asserted on. The outcome check now covers the wall-clock series too, with its own anti-vacuous row count. And the driver had started perturbing its own experiment. The periodic publication added two commits ago took SUM_LOCK inside the search, and monitorEnter is a GC SAFEPOINT in this VM -- so the workers were being stopped far more often than the workload otherwise permits and the runaway stopped reproducing: peak footprint fell from 1271MB to 126MB with the reserve compiled out, in BOTH builds, which is what gave it away. Publication is now a volatile long per worker: not a safepoint, not a lock, and JLS 17.7 makes volatile long access atomic, so it also answers the visibility and tearing that the plain long[] had. With the runaway restored, the reserve's throughput cost is re-measured across four interleaved pairs at 0.875-1.035, median 0.90 -- about a tenth, not the ~1% the previous figure claimed. Peak and headroom are unchanged (1271/63 against 1022-1064/272-304). This is the third throughput figure this comment has carried and the first two were both apparatus rather than signal, so the comment now says which were which. Verified: four ablation combinations compile; run-gc-verify.sh green with both fault self-tests; the steady-state gate green with all five checks. Reported by chatgpt-codex-connector on #5599. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Attach evidence to the ceiling assertions The first vm-tests run that ever completed on this branch failed scenario 3 -- "the smallest headroom seen was 62MB" under a 768MB budget -- and reported nothing else. Every other assertion in this gate appends the run's output; this one, the only one that has actually failed, did not. The probe rows that would explain it were captured and then discarded. Both ceiling assertions now carry the [PACING] counters, the last [GCPROBE] footprint partition and the wall-clock summary. That partition is the whole point of the probe: it says whether a footprint the reserve did not defend is even in the Java heap. No behaviour change, and the gate still passes locally on macOS -- which is itself the open question, since the failure is on the Linux runner and the two measure different quantities (phys_footprint against RSS). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Assert the reserve's mechanism, report its outcome The first vm-tests run that completed on this branch failed scenario 3 on the Linux runner: 62MB of headroom under a 768MB budget. With the evidence attached, the diagnosis is not what I guessed. I expected allocator retention -- glibc arenas holding freed legacy blocks, which RSS counts and phys_footprint would not. Wrong: residKb was 7MB of a 518MB footprint, so the footprint was the Java heap almost exactly. That is the residual bucket earning its place; it killed the hypothesis in one line. What the runner actually shows is a collector that cannot keep up, with the bound working: volumeParks=879, so it engaged and parked repeatedly, while mark ran 407-545ms per cycle -- 235ms of conservative stack scan, 122-252ms waiting for mutators to reach a safepoint -- against ~170MB of allocation per cycle. With the grace rule holding a cycle's allocation two more cycles, the smallest working set that machine can hold is already above the reserve line at that budget. satbMs was 0 throughout, so the earlier fix is holding and the stack scan is simply the next cost. So an absolute headroom assertion was testing the runner rather than the collector. Scenario 3 now asserts the contract, which is true on any machine: either the process never entered its reserve, or the bound engaged when it did. The headroom achieved is printed either way, so the outcome stays visible without being asserted. A regression that stops the bound engaging fails here; a machine that is merely slow does not. Scenario 4 gains a second half for the same reason -- with the reserve compiled out the process must land on the bare admission margin, or the ceiling is not pressuring the workload and scenario 3's "never entered" branch would pass for the wrong reason -- plus volumeParks == 0, since the bound is not in that build at all. Locally: headroom 161MB inside a 192MB reserve with volumeParks=350, against 63MB and volumeParks=0 with the reserve compiled out. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #5537.
What was happening
An iPad killed a deep game-tree search with
EXC_RESOURCE (RESOURCE_TYPE_MEMORY: high watermark memory limit exceeded)at 1.42GB, inside_platform_memmoveon a worker thread, while the same build ran fine in the simulator, on Android and on Windows. 1.42GB is the iPadOS per-process dirty-memory ceiling, so this is a limit being crossed rather than a leak — the reporter's live set was almost nothing. #5540 (return surplus BiBOP pages to the OS) reduced retention and did not fix it, because retention was not the problem.Root cause
The GC's backpressure decides how far a mutator may run ahead of the collector, and all of it was sized against the device's free RAM. That has nothing to do with the ceiling the process is metered against.
cn1BibopPacingCaphanded a high-throughput thread half of the host-widefree + inactive + purgeablefigure — gigabytes on a large-RAM iPad. The mutator was licensed to run further ahead of the collector than the process was allowed to exist.That is precisely the failure the function's own comment warns about — "removing it unconditionally let the mutator outrun the collector and balloon RSS to ~2GB" — reintroduced by measuring the wrong quantity. It can only bite where a per-process ceiling exists, which is why it read as "works everywhere but the device".
The fix
cn1ProcessHeadroomreports the bytes this process has left, viaos_proc_available_memory()(equivalent totask_vm_info.limit_bytes_remaining, withouttask_info's cost). It returns 0 both when there is no limit and when the limit is already exceeded — opposite meanings, and the second is the emergency — so a process that has ever reported a positive figure latches "has a limit", and a later 0 is read as "budget gone". Everywhere without a ceiling it returns -1 and the host-wide reading applies exactly as before.Funder budgetLa thread may grow to(L+F)/2, belowLfor everyF— so the footprint approaches the ceiling geometrically and pacing slack alone can never reach it.CN1_BIBOP_MAX_OBJECT(512 bytes) — i.e. every array a program allocates — took a path whose 24MB trigger only schedules an async cycle. The only thing that blocked the thread was a count of pending allocations (CN1_MAX_HEAP_SIZE, free RAM over a 128-byte average object), so a thread churning multi-kilobyte arrays could run hundreds of MB ahead of the collector before anything stalled it. Gated on the trigger crossing already computed there, so the common path costs one comparison.Test
ProcessBudgetPacingIntegrationTest, with aCN1_SIMULATE_PROC_MEMORY_LIMIThook so the clamp is reachable off-device — without which this fix would be as untestable in CI as the bug was. One binary, one workload, run twice:The control's peak is reported but not asserted (it measures the scheduler, not the code), and neither is the bounded run's park count (0, 1, 2, 8 across repetitions of an identical run). What is asserted is the invariant — bounded peak below the budget — and, deterministically, that an undeclared budget paces nothing at all, which is what keeps this from costing throughput on every other target.
Teeth confirmed by ablation rather than assumed: with the legacy backpressure removed and everything else in place, the bounded run peaks at 472MB against the 256MB budget and the guard fails.
Also: the reporter could not attach a debugger
Raised twice in the issue and unanswered. A Metal build died at launch with
Library not loaded: /System/Library/Frameworks/OpenGLES.framework/OpenGLES, referenced from the app binary. The template hard-links OpenGLES and GLKit, so the app declares a load-time dependency on a deprecated framework that need not be present. Both are now weak-linked; a Metal build never calls into them.Verification
BibopPageFloorIntegrationTest,GcHeapIntegrityIntegrationTest,LowMemoryThrottleIntegrationTest: greenByteCodeTranslator: cleancn1_globals.msyntax-checked forarm64-apple-ios13.0andarm64-apple-macos13; confirmed theos_proc_available_memorypath is compiled in on iOS and out on macOS (it isAPI_UNAVAILABLE(macos), which covers Catalyst)🤖 Generated with Claude Code