Pace the GC against the process budget, not the device's RAM (issue #5537) - #5563
Open
shai-almog wants to merge 14 commits into
Open
Pace the GC against the process budget, not the device's RAM (issue #5537)#5563shai-almog wants to merge 14 commits into
shai-almog wants to merge 14 commits into
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. |
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