mimalloc: take destroyed blocks back off the malloc_* statistics in mi_heap_destroy - #39076
mimalloc: take destroyed blocks back off the malloc_* statistics in mi_heap_destroy#39076robobun wants to merge 1 commit into
Conversation
…i_heap_destroy Blocks that are still in use when their heap is destroyed are freed with their page in mi_heap_delete_page and never go through mi_free, so the malloc_normal / malloc_bins / malloc_huge increments made when they were allocated stayed behind and were merged into the main heap's statistics. Every bun_alloc::MimallocArena that went out of scope therefore grew heapStats().mimalloc.malloc_normal.current by its live bytes (about 940 KiB per Bun.Transpiler#transformSync call on a debug build), although the memory itself was freed. Applied as a patch on the pinned oven-sh/mimalloc commit; the destroy branch now collects the page and decrements the counters for the blocks that are still in use, mirroring free.c:mi_stat_free. Only MI_STAT builds (debug) compile the new code; release builds are unchanged.
|
Warning Review limit reached
Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Status
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this and found no bugs — the stat decrements mirror mi_stat_free exactly (same bsize <= MI_LARGE_MAX_OBJ_SIZE split, same mi_page_usable_block_size/mi_page_block_size sizing, same _mi_bin), the collect-before-count handles the thread-free double-accounting case, and the whole block is under #if MI_STAT>0 so release builds are unchanged. Still, this patches the allocator's heap-destroy path and adds a _mi_page_free_collect_no_unpurge call there; a maintainer familiar with the mimalloc fork should give it a look (and decide whether it should land in oven-sh/mimalloc directly rather than as a build-time patch).
What was reviewed:
- Patch context matches the pinned
arena.cat 2714;mi_heap_stat_decrease((mi_heap_t*)heap, ...)and the const-cast are the existing pattern in the same function. - Verified
mi_heap_free_theapsruns before_mi_heap_destroy_pagesin_mi_heap_force_destroy, so writing toheap->stats(not a theap) is correct. - Test: bounds are well below the single-call unpatched deltas, precondition is asserted via
heaps.total/heaps.current, andskipIf(!tracksMallocStats)handles release builds.
Extended reasoning...
Overview
Adds a build-time patch to the vendored mimalloc (patches/mimalloc/heap-destroy-malloc-stats.patch, wired via patches: in scripts/build/deps/mimalloc.ts) that makes mi_heap_delete_page's destroy branch decrement malloc_normal / malloc_bins / malloc_huge for the blocks it destroys without going through mi_free. Also adds a regression test in test/js/bun/jsc/heapStats-mimalloc.test.ts that drives 10 transformSync calls (each creates and destroys a MimallocArena) and bounds the counter growth.
Security risks
None identified. This is debug-build-only allocator statistics accounting; the added code is entirely inside #if (MI_STAT>0), which is off in release builds. No user-controlled input reaches the new code.
Level of scrutiny
Medium-high. The effect is debug-diagnostics-only, but the mechanism is a patch inserted into mimalloc's mi_heap_destroy page-freeing path, and it adds a _mi_page_free_collect_no_unpurge(page, false) call there (to make page->used reflect only never-freed blocks). I traced that: the page is owned and about to have used=0 set and be freed via _mi_arenas_page_free, so collecting the thread-free list first should be harmless — but this is exactly the kind of allocator-internals reasoning a maintainer who owns the fork should confirm. I cross-checked the decrement logic against free.c:mi_stat_free (lines 727–741), alloc.c:mi_page_malloc_zero (63–76), and page.c:mi_huge_page_alloc (1939): the patch is the exact batched inverse.
Other factors
- The
patches:mechanism is established (libarchive, boringssl, lsquic, etc. all use it), and the PR follows the repo's convention of a small patch with an explanatory header. No upstream issue link is included in the header, though the description notes upstreamdev3has the same gap. - The test follows repo conventions (
Buffer.alloc(n, fill).toString()instead ofrepeat, warm-up call for lazy state,Bun.gc(true)before each reading,skipIffor the release-build case where the counters aren't compiled in). The bounds (512 KiB / 2 MiB / 2,000 blocks) are below what one unpatched call leaked (~940 KiB / 2.25 MiB / 2,770), so the test can't pass on an unpatched debug build. - The PR notes it will need a trivial
patches:array merge with #34739 and that the patch should be dropped once the fix lands in the fork — a maintainer may prefer to land it there directly.
|
Thanks. On the two open points:
|
|
Updated 12:14 PM PT - Aug 15th, 2026
🔄 @robobun, the build for your commit |
1 participant
Problem
require("bun:jsc").heapStats().mimalloc.malloc_normal.current(andmalloc_bins[i].current,malloc_huge.current) only ever grows in any workload that creates and destroys mimalloc heaps, even though the memory is freed. 10Bun.Transpiler#transformSynccalls:malloc_normal.current+9,620,240,malloc_huge.current+23,592,960,malloc_bins+27,710 blocks, whileheaps.currentis unchanged andheapStats({ dump: true })shows nothing left alive. Bun destroys a heap per transpile, bundle and parse (bun_alloc::MimallocArenadrop/reset ismi_heap_destroy), so the counter is useless for leak hunting: a dev server leak was measured at +2.2 MB per rebuild with it, of which about 2 MB was this.mi_heap_destroyfrees every page of the heap throughmi_heap_delete_page(vendor/mimalloc/src/arena.c:2714, theheap_target == NULLbranch), which setspage->used = 0and frees the page. The blocks that were still in use never go throughmi_free, andfree.c:mi_stat_freeis the only place the increments made at allocation (alloc.c:mi_page_malloc_zero,page.c:mi_huge_page_alloc) are taken back off. The leftovers stay inheap->stats, andmi_heap_freemerges them into the main heap's stats (heap.c:207), which is whatmi_stats_get_jsonreports from then on.dev3has the same gap; mimalloc v2's destroy path (_mi_heap_page_destroy) did adjust these counters.MI_STAT, whichMI_DEBUGturns on); release builds report 0 for them.Fix
patches/mimalloc/heap-destroy-malloc-stats.patch, applied to the pinnedoven-sh/mimalloccommit (6e891cbe, the fork's current head) through the existingpatches:mechanism. I can't push to the fork, so this takes the same route as mimalloc: fold every thread's theap into the subproc stats aggregate #34739; the patch is to be dropped when the pin moves to a fork commit that carries the fix (thepatches:comment says so, andgit applywill most likely refuse it at that point anyway). Hunk-wise it is independent of mimalloc: fold every thread's theap into the subproc stats aggregate #34739 (arena.chere,stats.cthere); only thepatches:line inmimalloc.tswill need a trivial merge._mi_page_free_collect_no_unpurge) and then decrementsmalloc_normalbyusable block size * usedandmalloc_bins[bin]byused(ormalloc_hugeby the block size for a huge page) onheap->stats.mi_stat_free, so each block's increment is now undone exactly once: bymi_freeif it was freed, by the destroy otherwise. The collect first is what makesusedmean "never freed": a block freed from another thread sits on the page's thread-free list withusedstill counting it, butmi_stat_freealready accounted for it on the freeing thread. The adjustment goes toheap->statsbecause the heap's theaps have already been freed and merged into it when the pages are visited (mi_heap_free_theapsruns first in_mi_heap_force_destroy), which is also where this path already accounts for the page counters (_mi_arenas_page_freewiththeap == NULL). The whole addition is under#if MI_STAT > 0, likemi_stat_free, so release builds compile the same code as before.malloc_requestedis deliberately left alone:mi_freedoes not decrement it either, it is cumulative by design.test/js/bun/jsc/heapStats-mimalloc.test.ts, "malloc_* counters come back down when a heap is destroyed". Runs 10transformSynccalls, checks viaheaps.total/heaps.currentthat a heap was created and destroyed per call, and bounds the growth of all three counters below what a single call used to leave behind. Skipped on release builds, where the counters are not compiled in.USE_SYSTEM_BUN=1skips the test instead of failing it):malloc_normal+9,620,240 (test fails there; themalloc_huge+23,592,960 andmalloc_bins+27,710 assertions fail independently as well), identical across 3 runstransformSyncx 50): +10.9 MB before, 0 after; a Worker that transpiles and exits: +406 KB per Worker before, flat aftertest/js/bun/jsc/with and without the patch: same results apart from the new test (the remaining failures in both are DOMJIT tests hitting their 5 s timeout on a heavily loaded box)src/, so the stash-based fail-before check has nothing to stash; the proof is the unpatched debug build above.Background
mi_heap_t) is a separate set of pages.mi_heap_destroyreleases all of its pages at once without freeing blocks individually; Bun'sbun_alloc::MimallocArenawraps one heap and calls it fromDropandreset().heap->stats, andheap->statsinto the main heap's stats, whichheapStats().mimalloc(mi_stats_get_json) aggregates.MI_STATis mimalloc's compile-time statistics level: 2 in Bun's debug builds (MI_DEBUG=3inscripts/build/deps/mimalloc.ts), 0 in release.malloc_normalandmalloc_hugeare maintained at level 1 and up,malloc_binsat level 2.page->usedcounts blocks handed out and not yet collected back. A block freed by a thread other than the page's owner is pushed onto the page's thread-free list and only leavesusedwhen the owner collects the page; its statistics are adjusted at free time regardless.Numbers behind the test bounds (debug build, 10 calls of the test's source)
Per call that is about 940 KiB of normal blocks, 2.25 MiB of huge blocks and 2,770 blocks; the test bounds are 512 KiB, 2 MiB and 2,000. A JS-only loop with
Bun.gc(true)before each reading moves none of the three counters, so the slack is only for things the process legitimately keeps across the loop.Observed along the way and left alone here:
committed.currentgoes negative (-1 MB to -27 MB) oncetranspiler.test.jshas run in the same process, with and without this patch, which makes the file's first test fail when the two files share a process (CI runs them separately). It is a subproc-level counter this patch does not touch.