Repository navigation
Upgrade mimalloc to v3.6.0 - #47287
Jarred-Sumner wants to merge 6 commits into
Conversation
|
Updated 9:05 PM PT - Oct 28th, 2026
❌ @Jarred-Sumner, your commit 3ec111a has 1 failures in
🧪 To try this PR locally: bunx bun-pr 47287That installs a local version of the PR into your bun-47287 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and found no bugs, but a human look is still worthwhile: the one load-bearing value, purge_holes = 56 in src/mimalloc_sys/mimalloc.rs:157, could not be checked against the fork's include/mimalloc.h from this checkout (vendor/mimalloc is not fetched), and the pin is a branch commit that will move again before merge.
What was reviewed: the MIMALLOC_COMMIT bump (old SHA has no other copies in the repo; constant name unchanged for the update workflow; src/static.c unity source list unaffected); the Option enum change and its single consumer mi_option_set_default(Option::purge_holes, ...) in src/aot/compile.rs:502, where a wrong discriminant would silently set a different option; the .buildkite/ci.ts:556 "upgrade to mimalloc v3" TODO, which predates this PR and is not made stale by it. Once the pin lands on the merged SHA, re-confirming 56 against that header (and that purge_delay = 15 / show_errors = 0, the other options Bun sets, are unchanged) is the remaining check.
Extended reasoning...
The change bumps the oven-sh/mimalloc fork pin in scripts/build/deps/mimalloc.ts and renumbers the hand-mirrored purge_holes discriminant in the Rust Option enum from 50 to 56; it touches no security-sensitive surface. The enum value is used by the AoT compile memory give-back path, so correctness depends entirely on matching the new upstream header, which is not available in this checkout and could not be fetched. The author states the pin is an unmerged branch commit that will move before merge and that CI cannot build until a WebKit preview release ships, so no build or test has validated the bump yet; those two facts decided defer over approve.
| import type { Dependency, DirectBuild } from "../source.ts"; | ||
|
|
||
| const MIMALLOC_COMMIT = "6da53c355e1332db3161e0749111926ed35bc986"; | ||
| const MIMALLOC_COMMIT = "eab19c23b4aa2e48223cac448f57644524f947ae"; |
There was a problem hiding this comment.
🔴 Anyone building from a clean checkout can lose the mimalloc source: the pin is a commit on an unmerged fork PR branch, not a merged SHA. scripts/build/deps/mimalloc.ts:16 sets MIMALLOC_COMMIT to eab19c23b4aa2e48223cac448f57644524f947ae, the head of oven-sh/mimalloc#64, which fetch-cli.ts:312 downloads as a GitHub archive by SHA. Once that branch is squash-merged, rebased or deleted, the archive URL 404s and every fetch (CI, release, bisect) fails. Fix: pin only a SHA reachable from the fork's main branch after oven-sh/mimalloc#64 merges, and update the pin in the same commit that lands this PR. The description calls this a to-do before merge; nothing in the repo enforces it.
Why this was flagged
A clean build (no cached tarball under cache/) runs fetchDep in scripts/build/fetch-cli.ts:269, which builds the URL https://github.com/oven-sh/mimalloc/archive/.tar.gz at fetch-cli.ts:312 from the commit in scripts/build/deps/mimalloc.ts:16. That commit, eab19c23b4aa2e48223cac448f57644524f947ae, is the tip of the unmerged fork PR branch. GitHub serves archives only for commits reachable from a ref; after a squash merge, force-push or branch deletion the request returns 404 and downloadWithRetry at fetch-cli.ts:319 fails, so bun cannot build from a fresh checkout or when bisecting to this commit later. On the base branch the pin 6da53c355e1332db3161e0749111926ed35bc986 is a merged fork commit and the archive stays fetchable. The PR text says the pin moves to the merged SHA before merge, but nothing in the build or CI checks that the SHA is on the fork's main branch, so the ephemeral pin lands if the follow-up is forgotten.
Verification: scripts/build/deps/mimalloc.ts:16 sets MIMALLOC_COMMIT = "eab19c23b4aa2e48223cac448f57644524f947ae"; scripts/build/fetch-cli.ts:312 builds https://github.com/${repo}/archive/${commit}.tar.gz from it; scripts/build/download.ts:211-216 marks a 404 as permanent, so the dependency never builds. The PR description states the pin moves to the merged SHA before this merges; as the code stands at HEAD nothing enforces the swap.
There was a problem hiding this comment.
Yes, this blocks the merge, and the thread stays open until it is settled. The pin of the base branch (6da53c355e13, oven-sh/mimalloc#63) is not merged yet either, so #47281 has the same exposure.
Both fork PRs land as merge commits, like the last sync (oven-sh/mimalloc#60), not squashed: oven-sh/mimalloc#64 has to keep upstream's history for the next sync. Both SHAs are then reachable from bun-dev3-v2. I will check that against the fork's branch after they merge and move the pins if it does not hold.
Under a limit on address space (ulimit -v), an executable compiled ahead of time kept 8 MB of address space unused, so that a malloc that may not fail could still succeed once the rest was taken. It needed ten patches to mimalloc and was still best effort. Such a malloc crashes on purpose again, as in plain Bun. Allocations that can fail still throw a RangeError. patches/mimalloc/ is gone. We maintain a fork, so what is worth keeping is a commit there: the out-of-memory hook of operator new, the fix for exclusive arenas, the guard pages of secure mode, and a generic MI_GETENV_ALLOWED in place of the patch that hardcoded which names a locked environment reads. Also deleted: the two patches that made the allocator hand out the memory freed last during an ahead-of-time build. They cost every free three instructions in every process, and a build of the TypeScript compiler on macOS peaked 10% higher with them than without.
…rruption Three sites in mimalloc report a failed allocation with EFAULT, the code for corrupted meta data, and a secure or debug build aborts on it. One of the deleted patches turned them into ENOMEM. That change is now a commit of the fork, so the caller gets NULL and reports out of memory as it does elsewhere.
mimalloc was the last dependency that computed its patches from the config.
The fork's own options come after upstream's, and upstream added six, so purge_holes moves from 50 to 56.
2e33e9f to
3f2ed3a
Compare
dd28ac2 to
3ec111a
Compare
There was a problem hiding this comment.
Code review found no new issues
No new issues were found in this update; 1 finding from earlier reviews is still open above.
Still open from earlier reviews (1):
- 🔴
scripts/build/deps/mimalloc.ts:16—Anyone building from a clean checkout can lose the mimalloc source: the pin is a commit on an unmerged fork PR branch,…
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
3f2ed3a to
72f2b91
Compare
|
Folded into #47281. What was left here is two lines (the pin and The open thread about the pin is settled there: #47281 pins |
Moves mimalloc from v3.5.3 to v3.6.0 (oven-sh/mimalloc#64). Stacked on #47281.
MIMALLOC_COMMITv3.6.0into the forkpurge_holes = 56insrc/mimalloc_sys/mimalloc.rsprofile_time_intervalEvery other option number in that enum was compared with the merged header and is unchanged.
The pin is a branch commit and moves to the merged SHA before this merges. CI cannot build until the preview release of oven-sh/WebKit#937 is published.
Not measured yet: memory and speed of real workloads against the previous pin.