Start mimalloc's scavenger thread explicitly at startup - #34502
Conversation
mimalloc now leaves starting its background scavenger thread to the application rather than spawning it from a process-attach constructor. Bump the mimalloc pin, bind the new mi_scavenger_start(), and call it early in main so freed memory keeps being purged off-thread and the event loop's heap handoff has a scavenger to hand off to.
|
Updated 5:31 PM PT - Jul 17th, 2026
@dylan-conway, your commit 733bfac is building: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe bundled mimalloc dependency is repinned to a new Git commit, and the corresponding Changesmimalloc revision update
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 5 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any issues with the Bun-side wiring, but this bumps the process allocator pin and changes early main init ordering — worth a human look.
Checked: bun_alloc::mimalloc re-exports bun_mimalloc_sys::mimalloc, so the call path resolves; the new extern mirrors the adjacent mi_on_thread_idle binding (pub safe fn, no preconditions). Placement after the libc::signal block preserves that block's "before any other thread is spawned" SAFETY invariant, and the call is unconditional so ASAN/non-override builds (which still link and use mimalloc via MimallocArena/mi_heap_*) also get a scavenger. The process.versions.mimalloc test expectation matches the new pin.
Extended reasoning...
Overview
Four files: bumps the oven-sh/mimalloc commit pin in scripts/build/deps/mimalloc.ts, adds a pub safe fn mi_scavenger_start() extern in src/mimalloc_sys/mimalloc.rs, calls it once in src/bun_bin/lib.rs immediately after the SIGPIPE/SIGXFSZ setup, and updates the expected mimalloc hash in test/js/node/process/process.test.js.
Security risks
None identified. No user input is involved; the new FFI binding takes no arguments and is documented idempotent. The only ordering-sensitive concern is thread creation vs. signal setup, which is handled (see below).
Level of scrutiny
Higher than the diff size suggests. The Bun-side change is mechanical, but the substantive behavior change lives in the mimalloc fork commit being pinned (oven-sh/mimalloc#9), which is outside this diff. Bumping the process-wide allocator and inserting a thread-spawning call into main's init sequence is exactly the kind of change the repo's "Dependencies & vendoring" guidance flags for human review.
Other factors
- Verified the call path:
bun_alloc::mimallocispub use bun_mimalloc_sys::mimalloc(src/bun_alloc/lib.rs:23), sobun_alloc::mimalloc::mi_scavenger_start()resolves to the new extern. - The
libc::signalblock's SAFETY comment requires it to run "before any other thread is spawned"; placingmi_scavenger_start()immediately after keeps that invariant intact.bun_crash_handler::init()already precedes the signal block in existing code, so this PR doesn't change that relationship. - The call is not gated on
#[cfg(not(bun_asan))]. That looks intentional: even when the#[global_allocator]is the system allocator under ASAN, mimalloc is still linked and used directly viaMimallocArena/mi_heap_*/ the Windows uv allocator, so the scavenger is still relevant there. The binding is declaredsafewith no preconditions, matchingmi_on_thread_idlenext to it. - On Windows the call now precedes
uv_replace_allocator; the scavenger is internal to mimalloc and doesn't touch libuv, so that ordering is fine. - Deferring rather than approving because the allocator pin bump itself is the load-bearing part and warrants a maintainer's sign-off.
mimalloc keeps starting its scavenger during process init and now also exports mi_scavenger_stop(), so bun needs no startup call after all; drop the binding and the call, repin to the commit with the export, and keep the process.versions expectation in step.
| import type { Dependency, DirectBuild } from "../source.ts"; | ||
|
|
||
| const MIMALLOC_COMMIT = "24211c6e7610ae7c4ec06040758ec90bd21a1c83"; | ||
| const MIMALLOC_COMMIT = "475188181e4fc2816ec1caf3375a34fd9c0dd890"; |
There was a problem hiding this comment.
🟡 The PR title and description still say this "binds the new mi_scavenger_start(), and calls it early in main", but commit 1cb1f7a ("Reduce to a plain mimalloc bump") removed both the binding and the startup call — the net diff is now just the mimalloc commit-hash bump and the matching process.versions expectation. Since the description becomes the permanent squash-commit message (per .claude/docs/landing-prs.md, PR process: "Re-sync title/description whenever review reworks the change"), the title/description should be updated to reflect that this is now a plain mimalloc pin bump before merging.
Extended reasoning...
What the finding is
The PR title is "Start mimalloc's scavenger thread explicitly at startup" and the description says it "bumps the mimalloc pin, binds the new mi_scavenger_start(), and calls it early in main — right after the signal setup". However, the second commit on this branch — 1cb1f7a "Reduce to a plain mimalloc bump" — removed both the mi_scavenger_start() FFI binding and the startup call, so the PR title and description no longer describe what the PR actually does.
Step-by-step proof
- Commit f82b780 ("Start mimalloc's scavenger thread explicitly at startup") added:
- the
mi_scavenger_start()extern declaration insrc/mimalloc_sys/mimalloc.rs - a call to it in
src/bun_bin/lib.rsafter signal setup - the
MIMALLOC_COMMITbump inscripts/build/deps/mimalloc.ts - the
process.versions.mimallocexpectation update intest/js/node/process/process.test.js
- the
- Commit 1cb1f7a ("Reduce to a plain mimalloc bump") reverted the binding and the startup call. Its commit message says: "mimalloc keeps starting its scavenger during process init … so bun needs no startup call after all; drop the binding and the call."
- Net PR diff (
git diff a2152850..HEAD --stat):scripts/build/deps/mimalloc.ts | 2 +- test/js/node/process/process.test.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)src/bun_bin/lib.rsandsrc/mimalloc_sys/mimalloc.rsare not touched. git grep mi_scavenger_start src/returns no matches — the binding is gone from the tree.
So the delivered change is exactly: bump the pinned mimalloc commit from 24211c6… to 4751881… and update the corresponding test expectation. Nothing about scavenger startup is changed on the Bun side; per the 1cb1f7a commit message, mimalloc at the new pin still auto-starts its scavenger during process init.
Why this matters
The repo's own PR-process rules (.claude/docs/landing-prs.md, referenced from REVIEW.md) state:
The PR description is the permanent squash-commit message — keep it true. … Re-sync title/description whenever review reworks the change.
If this merges as-is, the squash commit on main will be titled "Start mimalloc's scavenger thread explicitly at startup" and its body will describe binding mi_scavenger_start() and calling it in main — none of which is present in the merged diff. Anyone later bisecting a scavenger-related regression, or reading git log to understand when/why explicit scavenger startup was added, would be misled: this commit does not add an explicit startup call. The description's second paragraph ("Without the call, purging quietly falls back to being allocation-driven…") is also now factually wrong for this PR, since there is no call and — per the 1cb1f7a message — none is needed.
What existing safeguards missed
CodeRabbit's "Title check" and "Description check" both passed, but they were run against commit f82b780 — before 1cb1f7a landed and dropped the change the title/description describe. Nothing re-validated the metadata after the rework commit.
Impact
No runtime, correctness, or build impact — the code that ships is fine. The only consequence is a misleading permanent squash-commit message in main's history. That's why this is filed as a nit, not a merge blocker.
Fix
Retitle to something like "Bump mimalloc to 4751881" (or similar) and rewrite the description to say what the net diff does: bump the pinned oven-sh/mimalloc commit and update the process.versions.mimalloc test expectation. If it's useful context, note that the new pin keeps the scavenger auto-starting during process init so no Bun-side change is needed.
2 participants
What does this PR do?
mimalloc now leaves starting its background scavenger thread to the application instead of spawning it from a process-attach constructor (oven-sh/mimalloc#9). This bumps the mimalloc pin, binds the new
mi_scavenger_start(), and calls it early inmain— right after the signal setup, so that setup still runs single-threaded.Without the call, purging quietly falls back to being allocation-driven and the event loop's heap handoff (
mi_on_thread_idle_start) has no scavenger to hand off to; the explicit start keeps freed memory being returned to the OS off-thread as before.How did you verify your code works?
The allocator side is covered by the mimalloc test suites (Debug + Release ctest, including the existing
MIMALLOC_SCAVENGER=0variant) in oven-sh/mimalloc#9. This side is a pin bump, an extern declaration mirroring the adjacent bindings, one call inmain, and the correspondingprocess.versions.mimallocexpectation — build and tests via CI.