Return surplus BiBOP pages to the OS (issue #5537) - #5540
Open
shai-almog wants to merge 14 commits into
Open
Conversation
A swept-empty BiBOP page went to bibopFreePool and stayed resident for the life of the process -- there was no munmap, madvise or free anywhere in the path. Because pages are also size-class segregated, that memory was not merely idle: it was unusable by anything except a future block of CN1_BIBOP_MAX_OBJECT bytes or less, so a past small-object peak permanently crowded out later large or native allocations (an image buffer, a Metal texture, a glyph atlas). On iOS that is subtracted from a jetsam ceiling of roughly 1.4GB, and it ratchets to the high-water mark of small-object demand, which for a deepening game-tree search rises over a session. The new BibopPageFloorIntegrationTest measures the effect directly, with a control: hold 192MB of 256-byte objects, drop them, force six collection cycles, then allocate a 192MB large-buffer set, then drop and allocate the identical set again. Same size, same type, same access pattern, same allocator -- the only variable is which allocator freed the memory underneath. Over BiBOP-freed memory it cost full price (196,992KB); over legacy-freed memory it cost 144KB. Three parts: * cn1BibopTrimFreePool madvises the slot region of empty pages beyond a 64-page warm cache, at the end of each sweep. On Apple it uses MADV_FREE_REUSABLE rather than plain MADV_FREE -- only that variant decrements phys_footprint, which is the figure the kernel meters an app against. Pages are unlinked under bibopMutex before any syscall runs, so an allocator can never acquire one mid-release, then published to a separate bibopReleasedPool that the acquire path reaches only after warm pages are exhausted. The page header stays resident, so pool links and the bump cursor survive; reads of a released region cannot fault, and the conservative resolver rejects a zeroed slot on __heapPosition. * A major sweep. The ordinary sweep only sees RETIRED pages, so a page swept while it still held live objects goes to bibopPartialPool and is never looked at again. When a big live set dies during a quiet period its pages keep every dead slot, never become empty, and never reach the free pool -- measured, the pool was literally empty on the workload this was meant to fix. The major sweep splices the partial pools onto the sweep list when the app has gone quiet, when the OS reports memory pressure, or on a 16-cycle backstop, never on an allocation-driven cycle, so the O(retired pages) fast path is untouched during churn. * Runtime.freeMemory() reports phys_footprint on both Apple branches (the plain-C branch was a hardcoded 1GB stub). Without this an app calling it to decide whether it can afford a cache would never see the memory it just got back, since MADV_FREE_REUSABLE leaves resident_size unchanged until the system is under pressure. Two behaviours are deliberate and were established by measurement: * Spliced pages are excluded from cn1BibopAdaptAfterSweep's statistics. Feeding mostly-dead pages into the survival ratio halved bibopGcTriggerBytes and took the issue-5425 workload from 5 collection cycles to 8, eating most of that guard's headroom. With the exclusion it is back to exactly 5. * Page release is disabled under CN1_GC_VERIFY. The verifier works by inspecting poisoned freed slots; released pages fault back as zeroes, which made GcHeapIntegrityIntegrationTest's re-injected grace defect undetectable and the gate inert. Shipping builds do not define it. Validation: full vm/tests suite 435 passed / 0 failures; benchmark A/B against -DCN1_BIBOP_NO_PAGE_RELEASE geomean 1.0031 with identical checksums (inside a noise floor of ~5%, measured on the allocation-free benchmarks); issue-5425 cycle count unchanged at 5; 67% of footprint returned (269,616 -> 90,720KB) and the texture peak after a small-object burst down from 466,224KB to 287,824KB. The residual 33% is a page-header tax rather than slack: only whole system pages can be released, the CN1BibopPage header sits at the base of its 64KB page, and arm64 has a 16KB system page. A 4KB-page target returns about 94%. Closing that gap means moving the header out of the page, which both the address-to-page mask in cn1ConservativeResolve and the nextAll registry depend on -- a redesign, kept out of this change. This does not address the legacy allocation path, which has a GC trigger but no throttle; LegacyArrayPacingIntegrationTest is added as a harness that reports that shape (1.8GB of growth at 2048MB/s against a 4MB live set) for separate work. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3 tasks
![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: 48bd2c8120
ℹ️ 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. |
Contributor
Cloudflare Preview
|
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark ResultsDetailed Performance Metrics
|
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
|
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 149 screenshots: 149 matched. |
…ing test markers Review feedback on #5540: gcPageReleased and gcMajorSpliced were read before anything wrote them. cn1BibopRawPage hands back indeterminate memory -- an arena carved from posix_memalign, which malloc may have recycled from its own free list -- and cn1BibopFormatPage, the only initializer, did not set either field. A stale nonzero gcPageReleased makes cn1BibopTrimFreePool skip the madvise, mark the page released anyway and file it under bibopReleasedPool, so the release silently does nothing for that page; a stale gcMajorSpliced drops an ordinary page out of the adaptive-trigger statistics. Both are now set in cn1BibopFormatPage, and cn1BibopNewPage zeroes the header once per genuinely new page so a field added later cannot be silently read before its first assignment. The pool-hit path never reaches that memset. Measured effect: page release improves from 67% to 68% of footprint returned (178,896KB to 181,968KB of 269,520KB), which is the fresh pages that were being skipped when their garbage flag happened to be nonzero. Separately, both new harnesses stopped merging the child's stderr into its stdout. The VM's env-gated tracers write to stderr and a merged write can land mid-line in a marker, which surfaced as a phase silently missing from the table because its ARM_PEAK line had a [GC-CYCLE] spliced through it. Neither test parses stderr, so it now goes to the surefire log instead. Full vm/tests suite: 435 passed, 0 failures. 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: 42e7bb5042
ℹ️ 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. |
Collaborator
Author
|
Compared 181 screenshots: 181 matched. |
Review feedback on #5540 (P1), and it was right. The quiet-cycle test that selects a major sweep read only bibopCycleAllocatedBytes, but legacy allocations -- anything above CN1_BIBOP_MAX_OBJECT, so every large array -- feed cn1LegacyBytesSinceGc instead and never reach bibopBytesSinceGc. cn1BibopBeginGcCycle then discarded that counter with a (void) cast. A cycle driven entirely by legacy volume therefore looked perfectly quiet however hard the app was allocating, and spliced every partial BiBOP page into every sweep -- the O(all pages) cost issue 5425 removed, reintroduced for exactly the workload shape that reported it. cn1BibopBeginGcCycle now keeps the exchanged legacy count in legacyCycleAllocatedBytes, and the quiet test sums both paths. Measured on a new bench workload built for this, com.bench.MajorSweepMix: a 120MB BiBOP survivor set (pages that sit in bibopPartialPool, which is the population a major sweep walks) under 12 seconds of heavy 64KB legacy churn (which is what actually drives the cycles). Counting [MAJOR-SWEEP] against [GC-CYCLE]: quiet test on BiBOP bytes only 36 major sweeps / 38 cycles (95%) quiet test on both paths 2 major sweeps / 39 cycles ( 5%) The existing benchmarks could not have caught this. LargeArrayLoad allocates only 0.4-1.8MB of legacy bytes per cycle, well under the 6MB quiet threshold, so it splices 3 times in 5 cycles either way; and its phases are stretched to fixed wall durations, so neither its wall nor its CPU time moves at all. Also adds a [MAJOR-SWEEP] line to the existing CN1_LOG_PAGE_RELEASE tracer, reporting the spliced page count and both byte counters. That is what made the misclassification visible, and it is the only way to tell a legitimately quiet cycle from a misclassified one from outside the VM. Validation: full vm/tests suite 435 passed / 0 failures; issue-5425 cycle count still 5; benchmark A/B against -DCN1_BIBOP_NO_PAGE_RELEASE geomean 0.9907 with identical checksums. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The bench workload added in the previous commit went in without the Codename One GPLv2 + Classpath Exception header, which failed check-copyright-headers. Verified over the full branch range rather than just the new file, since the gate runs against the merge base: scripts/check-copyright-headers.sh reports 8 of 8 passing, and every file the branch touches is ASCII-clean (the two non-ASCII bytes in cn1_globals.m predate this branch on master). 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: 99c77af9aa
ℹ️ 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 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 144 screenshots: 144 matched. |
Review feedback on #5540 (P2). cn1BibopReleasePageMemory returned void, so the caller marked every surplus page released and moved it to bibopReleasedPool whether or not madvise had actually accepted the range. A transient EAGAIN from Linux MADV_DONTNEED, or a range Darwin refuses, therefore produced a page that was recorded as released but whose memory was still resident -- and because the flag was set, no later sweep would ever try again. The footprint would stay up with nothing to indicate anything had gone wrong. cn1BibopReleasePageMemory now returns whether the advice took, and cn1BibopTrimFreePool partitions the detached run on that result: accepted pages go to bibopReleasedPool as before, rejected ones go back to bibopFreePool. Rejected pages are still perfectly good empty pages, so returning them there both keeps them allocatable and lets the next sweep retry the release. The Apple fallback keeps its previous shape: MADV_FREE_REUSABLE first because it is the only variant that moves phys_footprint, then MADV_FREE, which still lets the kernel take the pages under pressure. Either counts as accepted. Pairing MADV_FREE_REUSE with a range that only got MADV_FREE is harmless -- it is rejected and there is no accounting to restore -- so a single released flag covers both cases. The CN1_LOG_PAGE_RELEASE tracer now reports the rejected count alongside the released one, so a platform where the advice is being refused is visible instead of silently doing nothing. Validation: full vm/tests suite 435 passed / 0 failures; the page-floor probe still returns its footprint (269,552KB to 87,584KB) with rejected=0 on every sweep. 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: 19dacecb5b
ℹ️ 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 217 screenshots: 217 matched. |
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
|
Review feedback on #5540 (P1). On Darwin a page released with MADV_FREE_REUSABLE is still classified by the kernel as reusable storage, and MADV_FREE_REUSE is what takes it back out of that state. That call's result was ignored: the acquire path formatted the page and exposed it for allocation regardless, so a rejected restore would leave the kernel free to treat storage about to hold live objects as discardable, with the footprint accounting still wrong. The previous commit made this worse rather than better by claiming a rejected MADV_FREE_REUSE is harmless. That is true only for a page released with an advice that has no pairing -- MADV_FREE, or Linux MADV_DONTNEED -- and with both kinds sharing one released flag there was no way to tell an expected rejection from a real failure. Each page now records which advice actually took (gcPageReusableAdvice), so the two cases are distinguishable. cn1BibopReusePageMemory returns whether the page is safe to allocate into: nothing to restore for an unpaired advice, and for a reusable page only after MADV_FREE_REUSE succeeds. On failure the page goes back to bibopReleasedPool still marked released, and the acquire path falls through to a fresh page -- one failed syscall, no spin, and self-healing if the cause is transient. The tracer reports the restore errno separately from the release errno so a platform rejecting one or the other is visible. Validation: full vm/tests suite 435 passed / 0 failures; the page-floor probe still returns its footprint (269,536KB to 87,568KB) with rejected=0 and reuseFailErrno=0 on every sweep. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…om CI Two problems, both mine, both found by looking at what the tests actually did on CI rather than at whether they were green. BibopPageFloorIntegrationTest was SKIPPING on every CI run. It reads memory through Runtime, and java_lang_Runtime_totalMemoryImpl / freeMemoryImpl were still hardcoded 1GB stubs on Linux, so every phase reported 0 and the test's "cannot measure here" assumption fired. The consequence is worse than a wasted 64 seconds: the Linux MADV_DONTNEED release path added by this PR had never been exercised by anything. Only macOS was ever validated. Both natives are now implemented for Linux: total from sysconf(_SC_PHYS_PAGES), used from /proc/self/statm. RSS is the right metric there -- unlike Darwin's MADV_FREE_REUSABLE, MADV_DONTNEED drops the pages immediately rather than deferring to memory pressure, so a release shows up in RSS as it happens. That makes the probe measure on CI, which is the only place the Linux path runs. LegacyArrayPacingIntegrationTest is removed. It was a diagnostic harness for the LEGACY allocation path, which this PR explicitly does not fix, so it gated nothing here; it cost 259 seconds of every CI run; and it was skipping for the same stub reason. Making it measure would have been worse, not better: its only real assertion is that an unbounded arm outruns the collector, which depends on runner load by construction and would be flaky on shared runners. It belongs with the change that fixes the legacy path, where it would guard something. The workload and its measurements remain in this branch's history and in the PR description. Validation: full vm/tests suite 434 passed / 0 failures; the page-floor probe returns 68% of its footprint (269,504KB to 87,536KB) and now runs rather than skips. 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: cea63aca34
ℹ️ 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".
…lock The probe passed on macOS and failed on Linux, and neither result was about the fix: the measurement was wrong. Verified against a real Linux target this time -- the translated clean-target C built and run under a container -- rather than by pushing and reading CI. Three separate causes, each found by measuring: A fixed settle measures the runner, not the collector. Reclamation is asynchronous and takes a platform-dependent number of cycles: a BiBOP object needs three sweeps to die, the major sweep that refills the free pool runs on a cadence, and cycles are paced at 200ms. The drop lands about 2s after the ring is dropped on macOS and about 7s in a Linux container, so six fixed rounds read the Linux run as having released nothing while the memory was still on its way back. The release settle now waits for the drop with a bounded budget, which fails honestly if it never comes instead of failing on whichever machine ran it. Stability alone is not a usable exit condition either, because early rounds look stable for the wrong reason -- nothing has started coming back yet. Where no drop is expected (the texture phases, whose released figure is reported rather than asserted) a plain bounded settle is used, which also avoids waiting out the full budget for an event that is not coming: 15s per phase, measured. And the stack has to be scrubbed before settling. ParparVM scans thread stacks CONSERVATIVELY, so a dead slot still holding the address of the dropped ring keeps everything it referenced reachable -- the collector cannot tell a stale word from a live reference. On Linux the warm-up's 192MB stayed fully resident through its settle and only came back once the NEXT phase's frames had overwritten those words. Linux, measured in a container, now matches macOS: the warm-up releases from 262,720KB to 114,800KB against a gate of 144,496KB, and the texture set still costs full price (196,428KB) over BiBOP-freed memory. That is the first time the MADV_DONTNEED path has actually been exercised. A standalone C probe of the same pattern confirms the primitive independently: 198,116KB to 13,992KB over 3072 madvise calls with no failures. Validation: full vm/tests suite 434 passed / 0 failures; probe 38s on macOS, down from 58s. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t 64-bit Two changes. Review feedback on #5540 (P2): legacyCycleAllocatedBytes was declared long, but cn1LegacyBytesSinceGc is long long precisely because long is 32 bits on the Windows LLP64 target. A collector falling more than 2GB of legacy allocation behind would truncate the cycle count to a negative value and the quiet-cycle test would read a furiously allocating app as idle -- reintroducing the O(all pages) major sweep for the exact case the previous commit fixed. The variable and the comparison are now 64-bit. The probe's release assertion no longer reads the footprint at a chosen instant. Reclamation is asynchronous and its LATENCY is load-dependent: the drop lands about 2s after the ring is dropped on an idle macOS host, about 7s in an idle Linux container, and had still not landed 15s in on a CI runner, where surefire runs this probe alongside a dozen other forks and starves the collector. Two successive attempts to fix that by waiting longer were both really measuring the runner's load. The claim under test is that the pages come back, not that they come back within some number of seconds, so the app now tracks the LOWEST footprint seen at any point after the warm-up's live set died and the assertion reads that. It cannot be satisfied by a release that never happens, and it is immune to when the release lands. That this is the right reading is settled by the failing CI run's own numbers rather than by argument: its table shows texture-after-small at base 32184KB, i.e. the footprint DID fall to 32184KB against a 145981KB budget, while the instant the old assertion sampled read 266784KB. The pages were always coming back; the measurement was taken too early. Also: the release settle no longer needs a long budget now that nothing depends on it, so its cap drops from 60 rounds to 20. The probe runs in 34s on macOS, down from 74s on CI. Validation: full vm/tests suite 434 passed / 0 failures. 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: 0c3dd9fb79
ℹ️ 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".
Review feedback on #5540 (P2), and it contradicts what the commit that introduced this path claimed. That commit said a failed MADV_FREE_REUSE cost "one failed syscall, no spin, and self-healing if the cause is transient". It put the failed page back at the HEAD of bibopReleasedPool, so every later acquisition would pop the same page, fail again, and allocate a fresh arena page. One unrestorable page would stand in front of an entire stocked pool and the heap would grow without bound while the pool sat unused. Failures now go on a separate bibopReuseFailedPool, consulted only once the good pool is empty and then at most one page per acquisition, so a permanently unrestorable page costs a single syscall and never starves the fresh-page fallback. An acquisition tries up to CN1_BIBOP_REUSE_ATTEMPTS released pages before giving up, which steps past a bad page rather than stopping at it. The path is now reachable in a test. CN1_BIBOP_FAIL_REUSE forces every MADV_FREE_REUSE to report failure -- without it the code that copes with an unrestorable page never executes, because the call does not fail in practice, which is how the defect above survived review of its own commit message. It is deliberately not part of the CN1_GC_FAULT family: those live under CN1_GC_VERIFY, and page release is disabled in verifier builds, so a fault declared there could never fire. Verified with the injection on: the probe completes, returns a bit-identical RESULT, and neither spins nor stalls. Note what that does and does not cover -- with EVERY reuse failing, each acquisition allocates fresh, so it exercises the loop's bounds and its correctness but cannot distinguish the single-bad-page case; that rests on the pool structure rather than on a measurement. Validation: full vm/tests suite 434 passed / 0 failures; the probe still returns 68% of its footprint. 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: 63ed42aa73
ℹ️ 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".
Review feedback on #5540 (P2), and it is the previous commit's own defect reproduced one level down. That commit moved failed pages off bibopReleasedPool precisely because reinserting at the HEAD meant the next acquisition popped the same page, failed, and reinserted it -- one bad page hiding a stocked pool. The new bibopReuseFailedPool was then given exactly the same LIFO reinsertion: with only one previously-failed page attempted per acquisition, a permanently unrestorable head page is retried forever and every other parked page behind it is never reached again, including pages that may have become restorable. The retry pool is now a FIFO with an explicit tail. A candidate is taken from the head and, on failure, parked at the tail, so successive acquisitions rotate through the parked pages rather than hammering one. Worth naming the pattern rather than just the fix: this is the second time the same head-reinsertion mistake went in, and both times the code read as correct because the failure path never executes in practice. CN1_BIBOP_FAIL_REUSE, added in the previous commit for exactly that reason, is what makes it runnable -- with it on, the probe completes and returns a bit-identical RESULT with the rotation exercised on every acquisition. Validation: full vm/tests suite 434 passed / 0 failures; probe unchanged in both the normal and injected-failure configurations. 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: bac4b75c30
ℹ️ 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".
Review feedback on #5540 (P2). When MADV_FREE_REUSABLE fails and the MADV_FREE fallback succeeds, the page was filed in bibopReleasedPool with gcPageReleased set and treated exactly like a properly released one. But MADV_FREE does not reduce phys_footprint -- it only lets the kernel take the pages under pressure -- so the page stayed charged to the process, and because every later trim skips pages already flagged released it was never offered the reusable advice again. The only path back was the workload exhausting the warm pool and reacquiring it, which a post-burst app may never do. A transient rejection was therefore permanent in effect, against the one figure this feature exists to reduce. cn1BibopUpgradeFallbackPages walks bibopReleasedPool on each trim and retries MADV_FREE_REUSABLE on pages whose gcPageReusableAdvice is clear -- which, within that pool, means exactly "released through the fallback". No new state: the flag that tells the acquire path whether a restore is needed already distinguishes the two cases. The budget matches CN1_BIBOP_RELEASE_PER_SWEEP because an upgrade costs the same single madvise a release does; budgeting it lower only makes a burst of rejections take proportionally longer to stop being charged. Made reachable before being trusted. CN1_BIBOP_FAIL_REUSABLE=<n> forces the first n reusable calls to fail, a count rather than a switch because the behaviour under test is what happens AFTER the transient clears. Measured with 2000 injected rejections: the trims report upgraded=1024 and the probe ends at 87,584KB against 87,536KB uninjected, i.e. the backlog fully recovers. At the initial 64-per-trim budget the same run ended at 179,776KB with the backlog still draining, which is how the budget was chosen rather than guessed. Validation: full vm/tests suite 493 passed / 0 failures. 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: b933983bdf
ℹ️ 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".
Review feedback on #5540 (P2). The fallback-upgrade scanner counted every page it looked at against CN1_BIBOP_UPGRADE_PER_SWEEP, including ones it skipped because they were already upgraded, and it always restarted at the pool head. Once the leading budget-sized window had been upgraded, every later pass spent its whole budget re-walking that window and never reached the fallback pages behind it, so with a backlog deeper than one budget the remainder stayed charged to phys_footprint indefinitely. The budget now counts ATTEMPTS. Walking past an already-upgraded page is a pointer dereference; only the madvise is worth bounding. A separate counter, bibopFallbackPageCount, tracks how many pages are actually waiting, so the pass skips its walk entirely when there is nothing to do -- which is every run in ordinary operation, since the fallback is only taken when MADV_FREE_REUSABLE is rejected and that does not happen. Worth being clear that the previous commit's measurement did NOT refute this. It injected 2000 rejections against a 1024 budget and recovered fully, which only means that run did not produce the ordering that stalls. Rebuilt with the budget lowered to 8 so a 200-page backlog is 25 windows deep, the fix reports upgraded=8 on 25 successive trims and recovers all 200; the previous code would have stopped after the first 8 and never advanced. Validation: full vm/tests suite 493 passed / 0 failures. 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.
Reclaimed BiBOP pages were never returned to the OS, and because pages are also size-class segregated that memory was not merely idle — it was unusable by anything except a future block of
CN1_BIBOP_MAX_OBJECT(512) bytes or less. A past small-object peak therefore permanently crowded out later large or native allocations: an image buffer, a Metal texture, a glyph atlas. On iOS that is subtracted from a jetsam ceiling of roughly 1.4GB for the lifetime of the process, and it only ratchets upward toward the high-water mark of small-object demand — which for a deepening game-tree search rises over a session.Relates to #5537.
The measurement
BibopPageFloorIntegrationTest(new) is a controlled comparison rather than an argument from the source. Hold 192MB of 256-byte objects, drop them, force six collection cycles, then allocate a 192MB large-buffer "texture" set, then drop those and allocate the identical set again. The treatment and the control allocate the same bytes, of the same type, with the same access pattern, through the same allocator; the only variable is which allocator freed the memory underneath.Before: the warm-up returned exactly nothing, and the texture set cost full price (196,992KB) over BiBOP-freed memory while costing 144KB over legacy-freed memory. After: 67% of the footprint comes back and the peak drops from 466,224KB to 287,824KB.
The probe never races the collector — every phase allocates, holds, drops, then forces collection and waits — so anything still resident is held by design rather than by a pacing accident. That is also why the numbers barely move between an idle host and a loaded one.
What changed
cn1BibopTrimFreePoolmadvises the slot region of empty pages beyond a 64-page (4MB) warm cache, at the end of each sweep. On Apple it usesMADV_FREE_REUSABLErather than plainMADV_FREE— only that variant decrementsphys_footprint, which is the figure the kernel meters an app against. Pages are unlinked underbibopMutexbefore any syscall runs, so an allocator can never acquire one mid-release, and they are then published to a separatebibopReleasedPoolthat the acquire path reaches only after warm pages are exhausted. The page header stays resident so pool links and the bump cursor survive; reads of a released region cannot fault, and the conservative resolver rejects a zeroed slot on__heapPosition.A major sweep. This was the half that made the first version release nothing. The ordinary sweep only sees retired pages, so a page swept while it still held live objects goes to
bibopPartialPooland is never looked at again. When a big live set dies during a quiet period its pages keep every dead slot, never become empty, and never reach the free pool — the pool measured literally empty on the very workload this was meant to fix. The major sweep splices the partial pools onto the sweep list when the app has gone quiet, when the OS reports pressure, or on a 16-cycle backstop. Never on an allocation-driven cycle, so the O(retired pages) fast path is untouched during churn.Runtime.freeMemory()reportsphys_footprinton both Apple branches; the plain-C branch was a hardcoded 1GB stub. Without this an app calling it to decide whether it can afford a cache would never see the memory it just got back, becauseMADV_FREE_REUSABLEleavesresident_sizeunchanged until the system is under pressure.Two behaviours are deliberate, and both were established by measurement rather than chosen up front:
cn1BibopAdaptAfterSweep's statistics. Feeding mostly-dead pages into the survival ratio halvedbibopGcTriggerBytesand took the issue-5425 workload from 5 collection cycles to 8, eating most of that guard's headroom. With the exclusion it is back to exactly 5.CN1_GC_VERIFY. The verifier works by inspecting poisoned freed slots; released pages fault back as zeroes, which madeGcHeapIntegrityIntegrationTest's deliberately re-injected grace defect undetectable and the gate inert. Shipping builds do not define it.Validation
vm/testssuite-DCN1_BIBOP_NO_PAGE_RELEASEGcHeapIntegrityIntegrationTestThe 0.3% geomean sits inside the noise floor:
intArithmetic, which allocates nothing and cannot be affected by this change, swung 1.3–7% across runs.Limits
The residual 33% is a page-header tax, not slack. Only whole system pages can be released, the
CN1BibopPageheader sits at the base of its 64KB page, and arm64 — device and Apple-silicon simulator alike — has a 16KB system page, so 16KB of every 64KB has to stay. A 4KB-page target returns about 94%. Closing that gap means moving the header out of the page, which both the address-to-page mask incn1ConservativeResolveand thenextAllregistry depend on. That is a redesign and is deliberately not attempted here.This does not address the legacy allocation path. It has a byte-denominated GC trigger (
CN1_LEGACY_GC_TRIGGER_BYTES) that only schedules an asynchronousSystem.gc(), and its sole backpressure is a count of outstanding slots (CN1_MAX_HEAP_SIZE) that a workload of large arrays never approaches.LegacyArrayPacingIntegrationTestis added as a harness that reports that shape — 1.8GB of growth at a 2048MB/s allocation rate against a 4MB live set — for separate work.Notes for reviewers
Both new tests are
@Tag("benchmark"). They report rather than gate the load-sensitive rows: the knee between "the collector keeps up" and "it does not" is set by collector speed relative to the mutator, so it moves under parallel-suite contention, and asserting on it would only make them flaky. They skip cleanly on a target that cannot reportphys_footprintthroughRuntime.Two measurement traps worth knowing if you re-run these locally.
ps rsswill never show this fix, sinceMADV_FREE_REUSABLEleaves pages resident until there is pressure — measurephys_footprint. And under Rosetta (this repo's JDK 8 is an x64 build, socmake/clangspawned from Maven default to x86_64) the call returnserrno 0but the footprint never moves; the test passes-DCMAKE_OSX_ARCHITECTURES=arm64on Apple silicon for that reason.🤖 Generated with Claude Code