mimalloc: fold every thread's theap into the subproc stats aggregate - #34739
Open
robobun wants to merge 3 commits into
Open
mimalloc: fold every thread's theap into the subproc stats aggregate#34739robobun wants to merge 3 commits into
robobun wants to merge 3 commits into
Conversation
mi_subproc_stats_get (behind heapStats().mimalloc and MIMALLOC_SHOW_STATS) summed subproc->stats plus each heap->stats, merging only the calling thread's theap into heap->stats first. But pages.current (and page_bins) are incremented in the allocating thread's theap while a cross-thread or scavenger free of an abandoned page decrements heap->stats directly via the NULL-theap path in _mi_arenas_page_free. Threads that never call mi_collect/mi_on_thread_idle (JSC helper threads, Worker VMs) leave their +1s sitting in an unmerged theap the aggregate never read, so the reported pages.current underreported by exactly those pages and could go negative under an import-and-bust loop (observed on macOS arm64 after JSC moved onto mimalloc). The fix is to also add every theap of each heap into the aggregate, under heap->theaps_lock. The calling thread's theap was already merge-and-zeroed by mi_heap_get_stats so including it again is a no-op. Lock order subproc->heaps_lock -> heap->theaps_lock matches mi_prof_set_all_theaps and the fork-prepare path.
Collaborator
Author
|
Updated 7:07 PM PT - Jul 19th, 2026
❌ @robobun, your commit 9ee6df2 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34739That installs a local version of the PR into your bun-34739 --bun |
Contributor
|
Warning Review limit reached
Next review available in: 15 seconds 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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The visitor no longer merge-and-zeroes the caller's theap; it reads heap->stats plus every theap under heap->theaps_lock as a side-effect-free snapshot. Reading both inside the same lock narrows the window a concurrent mi_theap_merge_stats can double-count through. Test: surface stderr/exitCode before JSON.parse so a subprocess crash reports the real diagnostic, and add an upper bound so a future double-fold regression is caught.
Collaborator
Author
|
CI on 9ee6df2 (build 75911): the new test and
Ready for review. |
Collaborator
Author
|
Re-checked this against the mimalloc pin main uses now ( What was checked
State of this branch
Two notes for whoever ports it to the fork:
Standalone check against the vendored sourceBuild (same flags clang++ -x c++ -std=gnu++23 -O2 -DNDEBUG -DMI_BUILD_RELEASE -DMI_STATIC_LIB -DMI_SKIP_COLLECT_ON_EXIT=1 \
-DMI_NO_PROCESS_DETACH=1 -DMI_CMAKE_BUILD_TYPE=release -fvisibility=hidden -ftls-model=initial-exec -fPIC \
-I include -c src/static.c -o static.o
clang -O2 -I include -c repro.c -o repro.o
clang++ static.o repro.o -lpthread -o repro
#include <mimalloc.h>
#include <mimalloc-stats.h>
#include <pthread.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#define NBLOCKS 4096
#define BLOCK 1000
static void* blocks[NBLOCKS];
static pthread_mutex_t mu = PTHREAD_MUTEX_INITIALIZER;
static pthread_cond_t cv = PTHREAD_COND_INITIALIZER;
static int ready = 0, done = 0;
static long long pages_current(void) {
mi_stats_t_decl(s);
if (!mi_stats_get(&s)) { fprintf(stderr, "mi_stats_get failed\n"); exit(2); }
return (long long)s.pages.current;
}
static long long dump_pages(void) {
char* json = mi_heap_dump_json(false, true);
if (json == NULL) { fprintf(stderr, "mi_heap_dump_json failed\n"); exit(2); }
long long n = 0;
for (const char* p = json; (p = strstr(p, "\"block_size\"")) != NULL; p += 12) n++;
mi_free(json);
return n;
}
static void* worker(void* arg) {
(void)arg;
for (int i = 0; i < NBLOCKS; i++) { blocks[i] = mi_malloc(BLOCK); memset(blocks[i], 1, BLOCK); }
pthread_mutex_lock(&mu);
ready = 1; pthread_cond_broadcast(&cv);
while (!done) pthread_cond_wait(&cv, &mu);
pthread_mutex_unlock(&mu);
for (int i = 0; i < NBLOCKS; i++) mi_free(blocks[i]);
return NULL;
}
int main(void) {
void* warm = mi_malloc(64); mi_free(warm);
const long long stat_before = pages_current();
const long long dump_before = dump_pages();
pthread_t t;
pthread_create(&t, NULL, worker, NULL);
pthread_mutex_lock(&mu);
while (!ready) pthread_cond_wait(&cv, &mu);
pthread_mutex_unlock(&mu);
const long long stat = pages_current();
const long long dump = dump_pages();
pthread_mutex_lock(&mu);
done = 1; pthread_cond_broadcast(&cv);
pthread_mutex_unlock(&mu);
pthread_join(t, NULL);
const long long stat_after_join = pages_current();
const long long dump_after_join = dump_pages();
const int ok = (stat >= dump - 5) && (stat <= dump + 10);
printf("mimalloc %d: before={stat:%lld,dump:%lld} while-thread-alive={stat:%lld,dump:%lld} after-join={stat:%lld,dump:%lld} -> %s\n",
mi_version(), stat_before, dump_before, stat, dump, stat_after_join, dump_after_join,
ok ? "OK (stat agrees with dump)" : "UNDERREPORTED (stat disagrees with dump)");
return ok ? 0 : 1;
}Output at With ( Release build of bun at the previous pin |
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.
Since #34009 moved JSC onto mimalloc,
heapStats().mimalloc.pages.current(andpage_bins[].current) can drift negative under an import-and-bust loop. First seen on macOS arm64--smol(45 → 72 → -125 → -534over 1000 iterations) and noted in #34159's side note; #34359 and #34686 work around the same drift innode-net.test.ts.Repro
The live heap walk (
mimallocDump, ground truth) counts 82 pages; the stat counter reports 46. The 36 missing pages are the Worker thread's.Cause
mi_subproc_stats_getsummedsubproc->statsplus eachheap->stats, after merging only the calling thread's theap into its heap (mi_heap_get_stats→_mi_heap_theap_peek). Thepagescounter is incremented in the allocating thread's theap (arena.c:986), while a cross-thread or scavenger free of an abandoned page decrementsheap->stats.pagesdirectly via the NULL-theap branch of_mi_arenas_page_free(arena.c:1171, reached fromfree.c:271and the abandoned-holes sweep atarena.c:1350/1356). Threads that never callmi_collect/mi_on_thread_idle(JSC helper threads, Worker VMs, the transpiler pool while busy) leave their +1s in an unmerged theap the aggregate never read, so every page they allocated is invisible while its eventual-1is visible. Under churn the visible side outruns the invisible side andpages.currentgoes negative.Fix
mi_heap_aggregate_visitornow readsheap->statsplus every theap onheap->theaps, underheap->theaps_lock, as a pure snapshot with no merge-and-zero side effect. Lock ordersubproc->heaps_lock → heap->theaps_lockmatchesmi_prof_set_all_theapsand the fork-prepare path ininit.c. Reads of another thread'stheap->statsare racy with that thread's non-atomic writes, which is fine for an advisory snapshot (alignedint64_treads on every target Bun supports); a concurrentmi_theap_merge_stats(add then memzero, withouttheaps_lock) can transiently over-count by one merging theap's pending delta, which the test's upper bound tolerates.Applied as a patch because I can't push to oven-sh/mimalloc; the same change belongs upstream there.
Why this is the right level
The alternative is to move the
pagesincrement toheap->stats(atomic) at page-alloc time so both sides always target the same counter. That adds an atomic RMW on every fresh page alloc and still leaves the other theap-local counters (page_committed,pages_abandoned,page_bins) under-reported for the same reason. Folding all theaps into the read-side aggregate fixes every counter with no hot-path change.Verification
The test brackets the counter against the live walk (
dump - 5 ≤ stat ≤ dump + 10) so it catches both the pre-fix underreport and any future double-fold. All five tests intest/js/bun/jsc/heapStats-mimalloc.test.tspass on the debug build.no test proof · iteration 0 · The fix is a vendored-dep patch (
patches/mimalloc/+scripts/build/deps/mimalloc.ts); neither is undersrc/orpackages/, so the fail-before stash is a no-op and both builds have the patch applied. Fail-before/pass-after shown above withUSE_SYSTEM_BUN=1vsbun bd.