Repository navigation
Add Linux mimalloc build matrix entries (amd64/arm64, release + LTO) - #283
Conversation
Adds a USE_EXTERNAL_MIMALLOC CMake option (requires USE_MIMALLOC=ON) that compiles bmalloc/WTF/JSC against the mimalloc v3 headers but leaves every mi_* symbol unresolved in libbmalloc.a/libWTF.a/libJavaScriptCore.a. Bun links its own vendored mimalloc (oven-sh/mimalloc) to satisfy them, so a second copy is not archived into the WebKit tarball. The vendored mimalloc object library is still built and linked into the jsc shell directly so bin/jsc stays runnable. Wires the option through Dockerfile and release.sh as build args, and adds four Linux glibc matrix entries (amd64/arm64 x release/LTO) plus the matching download/rename/release steps. Also drops an unused #include <mimalloc/types.h> from StructureAlignedMemoryAllocator.cpp; the include path for it was never wired into JavaScriptCore and nothing from that header is referenced.
WalkthroughChangesMimalloc configuration is added across WebKit’s CMake options, allocator runtime paths, Docker builds, release scripts, and CI matrices. Internal and external mimalloc linkage can now be selected across Linux, Windows, macOS, musl, FreeBSD, and Android builds. Mimalloc allocator configuration and runtime
Build and release propagation
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![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, but deferring to a human since this introduces a new packaging strategy (USE_EXTERNAL_MIMALLOC — shipping archives with intentionally unresolved mi_* symbols) that's worth a maintainer sign-off.
What was reviewed:
- Matrix entries: all four labels are consistently threaded through upload → download-artifact → mv → release files.
mimalloc-objtarget still exists whenUSE_EXTERNAL_MIMALLOC=ON(theadd_subdirectory(mimalloc)is gated onUSE_MIMALLOC, which the new option depends on), so the jsc-shell link works.- Removed
<mimalloc/types.h>include: confirmedmi_arena_id_t/mi_manage_os_memory_ex/mi_heap_new_in_arenaare all declared in the publicmimalloc.h. - Existing matrix entries unaffected — new build args default to
OFFat every layer (workflow expr, release.sh, Dockerfile).
Extended reasoning...
Overview
This PR adds four new Linux glibc CI matrix entries (amd64/arm64 × release/LTO) that build JSC with mimalloc instead of libpas, and introduces a USE_EXTERNAL_MIMALLOC CMake option so the shipped static archives leave mi_* symbols unresolved for Bun to satisfy at link time. Changes span the GitHub Actions workflow, Dockerfile, release.sh, three CMake files, and one dead-include removal in StructureAlignedMemoryAllocator.cpp.
Security risks
None. This is build/packaging plumbing. No auth, crypto, permissions, or user-input handling. Action SHAs remain pinned; new build args are plain CMake ON/OFF flags plumbed through Docker.
Level of scrutiny
Medium-low. The bulk of the diff is mechanical CI matrix expansion following the exact pattern of adjacent entries, and any breakage in the new configs would surface immediately in CI without affecting existing builds (everything defaults to OFF). The CMake changes are small and idiomatic (WEBKIT_OPTION_DEFINE + WEBKIT_OPTION_DEPEND, one conditional guard, one list(APPEND ... $<TARGET_OBJECTS:...>)). The one C++ change is a trivially-verifiable unused-include removal.
Other factors
I'm deferring rather than approving because USE_EXTERNAL_MIMALLOC encodes a packaging design choice — ship archives with unresolved allocator symbols and let the consumer link its own mimalloc — that a maintainer should explicitly agree to, even though the implementation is straightforward and the author verified it locally with nm and a running jsc shell. Candidate issues about the "Implies USE_MIMALLOC" wording and DEVELOPER_MODE test executables were examined and ruled out (the CI build doesn't set DEVELOPER_MODE, and WEBKIT_OPTION_DEPEND correctly force-disables the option if USE_MIMALLOC is off).
C_INCLUDE_PATH points at gcc-13's compiler-builtin header directory. clang searches it ahead of its own resource dir, so any C translation unit that includes <immintrin.h> resolves to gcc's incompatible copy and fails to compile. mimalloc's static.c hits this under -march=haswell via bits.h. clang ships its own freestanding and intrinsic headers, so the gcc directory is not needed here.
|
Preview build 29181439487 failed on Root cause: the Dockerfile sets 22b4f7c unsets |
There was a problem hiding this comment.
Beyond the inline nit, I also checked: the dropped <mimalloc/types.h> include is safe — every symbol used in the USE(MIMALLOC) branch of StructureAlignedMemoryAllocator.cpp (including mi_arena_id_t) is declared in mimalloc.h; the mimalloc-obj target still exists under USE_EXTERNAL_MIMALLOC because the WEBKIT_OPTION_DEPEND on USE_MIMALLOC keeps add_subdirectory(mimalloc) in effect; and the DEVELOPER_MODE test executables were examined for the same unresolved-mi_* link concern and ruled out.
Extended reasoning...
Only a doc-string nit surfaced. I'm not auto-approving because this introduces a cross-repo ABI contract (compile against vendored mimalloc v3.2.8 headers, link Bun's v3.3.2) and the unset C_INCLUDE_PATH in the shared Dockerfile step affects every Linux glibc build, not just the new mimalloc variants — both look correct but are worth a human glance.
Preview Builds
|
Linux glibc release builds (including CI release + release-lto) now use the -mimalloc WebKit prebuilt by default, so JSC and bun share bun's vendored mimalloc as the process-wide allocator. Debug, asan, baseline, musl and non-Linux stay on libpas (no -mimalloc prebuilts ship for those yet). webkitVersion is pinned to the oven-sh/WebKit#283 preview release when webkitMimalloc is on; drop that override in resolveConfig once the -mimalloc artifacts.
…pinned prebuilt The webkitMimalloc default now requires webkit=prebuilt with no explicit --webkit-version, so release-local builds and version overrides stay on libpas instead of forwarding mimalloc flags to a checkout that predates oven-sh/WebKit#283 or requesting -mimalloc tarballs older autobuilds don't ship. The preview pin is now the commit sha (mapped to the PR-tagged release in deps/webkit.ts), so process.versions.webkit keeps reporting a commit hash on Linux glibc release instead of the tag name.
|
8f51b889 fixes two segfaults the fork() in child (next.js dev server,
Both are fixed by dropping the Also added a |
…der fork() and Malloc=1 Two bugs in the USE(MIMALLOC) path: 1. OSAllocator::tryReserveUncommittedAligned applies MADV_DONTFORK to the structure-heap reservation on Linux. mi_manage_os_memory_ex stores mi_arena_t and per-thread theap structs inside that region and registers them in mimalloc's process-wide subproc->arenas[] and subproc->heaps lists, which _mi_process_fork_child walks in the child before exec. After fork() those pages are unmapped in the child, so the walk segfaults. Undo DONTFORK on the region after registering it with mimalloc. 2. The mimalloc branch ran unconditionally without the m_useSystemHeap guard the libpas branch has. With Malloc=1 (m_useSystemHeap=true), tryMallocStructureBlock uses the bitvector fallback while the constructor still created a mimalloc arena whose mi_arena_t sits at the 64KiB-aligned start of the passed range; the bitvector was also never seeded with set(0), so it handed out block 0 (empty-StructureID conflict) and block 4 (overwriting mi_arena_t). Guard on m_useSystemHeap and seed the bitvector in the else branch, matching libpas. Also drive mimalloc's purge from Heap::didFinishCollection after a full GC via releaseFastMallocFreeMemoryForThisThread, since mimalloc has no background scavenger and otherwise only purges from allocation slow paths.
8f51b88 to
f625cc5
Compare
| if (USE_EXTERNAL_MIMALLOC) | ||
| # The shipped static archives intentionally leave mi_* unresolved so the | ||
| # consumer links its own mimalloc. The jsc shell still needs a working | ||
| # allocator, so link the vendored mimalloc object library here only. | ||
| list(APPEND jsc_LIBRARIES $<TARGET_OBJECTS:mimalloc-obj>) | ||
| endif () |
There was a problem hiding this comment.
🟡 Minor: with -DDEVELOPER_MODE=ON -DUSE_EXTERNAL_MIMALLOC=ON, the test executables (testapi/testmasm/testb3/etc.) would fail to link with unresolved mi_* — they only inherit jsc_FRAMEWORKS, not jsc_LIBRARIES, so mimalloc-obj isn't pulled in. Not a CI concern (Docker builds --target jsc only, and local mimalloc dev would use USE_MIMALLOC=ON without the external flag), and the "here only" scoping may well be intentional — just flagging in case it wasn't.
Extended reasoning...
What
The new block appends $<TARGET_OBJECTS:mimalloc-obj> only to jsc_LIBRARIES. The DEVELOPER_MODE test executables further down (testapi, testmasm, testRegExp, testb3, testair, testdfg, testwasmdebugger) each set testX_FRAMEWORKS ${jsc_FRAMEWORKS} but do not set a testX_LIBRARIES list, so they don't inherit this addition.
Under USE_EXTERNAL_MIMALLOC=ON, Source/bmalloc/CMakeLists.txt now guards its append with AND NOT USE_EXTERNAL_MIMALLOC, so bmalloc_LIBRARIES no longer carries mimalloc-obj and the bmalloc framework target no longer transitively provides the mi_* symbols to any consumer.
Step-by-step
- Configure with
-DUSE_MIMALLOC=ON -DUSE_EXTERNAL_MIMALLOC=ON -DDEVELOPER_MODE=ON. bmalloc/CMakeLists.txt:660skipslist(APPEND bmalloc_LIBRARIES $<TARGET_OBJECTS:mimalloc-obj>), solibbmalloc.a/libWTF.a/libJavaScriptCore.acarrymi_malloc/mi_free/etc. as undefined refs (as intended).shell/CMakeLists.txtsetstestmasm_FRAMEWORKS = ${jsc_FRAMEWORKS}(JavaScriptCore/WTF/bmalloc) but notestmasm_LIBRARIES.WEBKIT_EXECUTABLE(testmasm)→_WEBKIT_TARGET_SETUPlinks${testmasm_LIBRARIES}(empty) and${testmasm_FRAMEWORKS}— neither containsmimalloc-obj.- Link fails with
undefined reference to 'mi_malloc'/'mi_free'/ etc.
The jsc target itself links fine because of the new block; only the DEVELOPER_MODE binaries are affected.
Why this is low severity
USE_EXTERNAL_MIMALLOCis a packaging knob whose stated purpose is producing archives with unresolvedmi_*for Bun's link step; combining it withDEVELOPER_MODEisn't a target use case.- No CI matrix entry exercises this combo — the Docker path builds only
--target jscand never setsDEVELOPER_MODE. - The failure mode is a loud link error, not silent breakage.
- A developer wanting to run
testmasm/testb3against mimalloc locally would set-DUSE_MIMALLOC=ONwithoutUSE_EXTERNAL_MIMALLOC, in which casebmalloc_LIBRARIESstill containsmimalloc-objand every executable links fine — so "can't debug mimalloc locally" is not a concern. - The comment "link the vendored mimalloc object library here only" suggests the scoping was deliberate.
Fix (if desired)
Also append $<TARGET_OBJECTS:mimalloc-obj> to each testX_LIBRARIES inside the DEVELOPER_MODE block (or hoist a shared _mimalloc_obj list that both jsc and the test targets append), so -DDEVELOPER_MODE=ON -DUSE_EXTERNAL_MIMALLOC=ON remains a buildable configuration.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Source/cmake/WebKitFeatures.cmake`:
- Line 311: Update the USE_EXTERNAL_MIMALLOC option setup so enabling it
automatically enables USE_MIMALLOC before WEBKIT_OPTION_DEPEND or dependency
enforcement runs. Preserve the existing option dependency and ensure
-DUSE_EXTERNAL_MIMALLOC=ON remains enabled without requiring callers to set
USE_MIMALLOC separately.
In `@Source/JavaScriptCore/heap/Heap.cpp`:
- Around line 2636-2642: Update the USE(MIMALLOC) full-GC cleanup in Heap’s
collection flow to call the process-wide FastMalloc purge,
releaseFastMallocFreeMemory(), instead of the current-thread-only
releaseFastMallocFreeMemoryForThisThread(). Keep the existing
CollectionScope::Full guard and surrounding behavior unchanged.
In `@Source/JavaScriptCore/heap/StructureAlignedMemoryAllocator.cpp`:
- Around line 142-155: Make the MADV_DOFORK restoration in the
StructureAlignedMemoryAllocator initialization path fatal on failure: capture
the result of the existing EAGAIN retry loop and apply RELEASE_ASSERT(result ==
0) after it completes, ensuring the structure heap region is restored before
returning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 61c94fe2-349d-4dc7-ae30-2633b86a1e4f
📒 Files selected for processing (8)
.github/workflows/build-reusable.ymlDockerfileSource/JavaScriptCore/heap/Heap.cppSource/JavaScriptCore/heap/StructureAlignedMemoryAllocator.cppSource/JavaScriptCore/shell/CMakeLists.txtSource/bmalloc/CMakeLists.txtSource/cmake/WebKitFeatures.cmakerelease.sh
| if (!m_useSystemHeap) [[likely]] { | ||
| void* memory = reinterpret_cast<void*>(reinterpret_cast<uintptr_t>(g_jscConfig.startOfStructureHeap) + MarkedBlock::blockSize); | ||
| size_t size = g_jscConfig.sizeOfStructureHeap - MarkedBlock::blockSize; | ||
| RELEASE_ASSERT(mi_manage_os_memory_ex(memory, size, false, false, false, -1, true, &structureArena)); | ||
| structureHeap = mi_heap_new_in_arena(structureArena); | ||
| #if OS(LINUX) && defined(MADV_DOFORK) | ||
| // Undo tryReserveUncommittedAligned's MADV_DONTFORK: mimalloc stores | ||
| // mi_arena_t and theaps inside this region and registers them in | ||
| // process-wide lists that _mi_process_fork_child walks pre-exec. | ||
| while (madvise(reinterpret_cast<void*>(g_jscConfig.startOfStructureHeap), g_jscConfig.sizeOfStructureHeap, MADV_DOFORK) == -1 && errno == EAGAIN) { } | ||
| #endif | ||
| return; | ||
| } | ||
| m_usedBlocks.set(0); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file first, then inspect the relevant region.
ast-grep outline Source/JavaScriptCore/heap/StructureAlignedMemoryAllocator.cpp --view expanded || true
echo '--- lines 1-260 ---'
sed -n '1,260p' Source/JavaScriptCore/heap/StructureAlignedMemoryAllocator.cpp | cat -n
echo '--- search for MADV_DONTFORK / MADV_DOFORK / tryReserveUncommittedAligned ---'
rg -n "MADV_DONTFORK|MADV_DOFORK|tryReserveUncommittedAligned|madvise\\(" Source/JavaScriptCore -SRepository: oven-sh/WebKit
Length of output: 14780
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- search for fork-child / process-fork references in bundled mimalloc ---'
rg -n "_mi_process_fork_child|fork_child|MADV_DONTFORK|MADV_DOFORK|madvise" Source/JavaScriptCore Source/bmalloc -S
echo '--- inspect the allocator reservation helper if present ---'
rg -n "tryReserveUncommittedAligned|StructureAllocatorPages|DONTFORK" Source -SRepository: oven-sh/WebKit
Length of output: 11007
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- OSAllocatorPOSIX DONTFORK/DOFORK sections ---'
sed -n '145,255p' Source/WTF/wtf/posix/OSAllocatorPOSIX.cpp | cat -n
echo '--- mimalloc fork-child handling and madvise retry pattern ---'
sed -n '520,550p' Source/bmalloc/mimalloc/mimalloc/src/prim/unix/prim.c | cat -n
echo '--- any existing RELEASE_ASSERT or syscall wrappers around madvise failures on Linux ---'
rg -n "RELEASE_ASSERT\\(.*madvise|PAS_ASSERT\\(!madvise|SYSCALL\\(madvise|errno == EAGAIN" Source/WTF Source/bmalloc -SRepository: oven-sh/WebKit
Length of output: 11559
Treat MADV_DOFORK failure as fatal at Source/JavaScriptCore/heap/StructureAlignedMemoryAllocator.cpp:147-151. If the advice call returns an error, the region stays MADV_DONTFORK, leaving mimalloc’s arena/heap pointers out of the forked child and risking a crash in _mi_process_fork_child. RELEASE_ASSERT(result == 0) after the retry loop.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Source/JavaScriptCore/heap/StructureAlignedMemoryAllocator.cpp` around lines
142 - 155, Make the MADV_DOFORK restoration in the
StructureAlignedMemoryAllocator initialization path fatal on failure: capture
the result of the existing EAGAIN retry loop and apply RELEASE_ASSERT(result ==
0) after it completes, ensuring the structure heap region is restored before
returning.
…n for Linux glibc release f625cc54 in oven-sh/WebKit#283 fixes the two segfaults the previous preview caused on the Linux glibc release lanes: - _mi_process_fork_child walked into the MADV_DONTFORK structure-heap region where mimalloc had stored mi_arena_t/theap structs; now undone with MADV_DOFORK after mi_manage_os_memory_ex. - With Malloc=1 the mimalloc arena was created unconditionally and the bitvector fallback overwrote it; now guarded on m_useSystemHeap. Re-enable the default (reverted in 99fd9d7 for the segfaults) with the same gating as before: Linux glibc release, prebuilt WebKit, no explicit webkitVersion, non-asan, non-baseline.
…gets Replaces the four separate -mimalloc Linux matrix entries with USE_MIMALLOC=ON + USE_EXTERNAL_MIMALLOC=ON on every non-asan, non-debug release target across linux (glibc, musl, android), macos, windows and freebsd. The -mimalloc archive name suffix is dropped; the regular release archives now ship with mi_* symbols unresolved for the consumer to link its own mimalloc.
| USE_MIMALLOC: "ON" | ||
| USE_EXTERNAL_MIMALLOC: "ON" |
There was a problem hiding this comment.
🟡 The PR title/description are stale relative to commit 8e7b610: they say four new -mimalloc-suffixed Linux entries are added and "existing matrix entries default to OFF so they are unchanged", but the final diff instead flips USE_MIMALLOC/USE_EXTERNAL_MIMALLOC to ON on the existing release labels across Linux glibc/musl, macOS, Windows, FreeBSD, and Android — no -mimalloc labels exist. Looks intentional per the commit title; would just update the title/description so the scope is clear.
Extended reasoning...
What
The PR description says:
Adds four new Linux glibc matrix entries that build JSC with mimalloc …
bun-webkit-linux-amd64-mimalloc,bun-webkit-linux-amd64-mimalloc-lto,bun-webkit-linux-arm64-mimalloc,bun-webkit-linux-arm64-mimalloc-lto
…
Wired throughDockerfile/release.shas build args; existing matrix entries default toOFFso they are unchanged.
That accurately described the initial commit (1d65722), but the final commit on the branch — 8e7b610 "Enable USE_MIMALLOC/USE_EXTERNAL_MIMALLOC on all non-asan release targets" — replaced the additive approach with an in-place switch. In the final .github/workflows/build-reusable.yml there are no -mimalloc-suffixed labels; instead USE_MIMALLOC: "ON" / USE_EXTERNAL_MIMALLOC: "ON" are added to the existing labels: bun-webkit-linux-{amd64,arm64}[-lto][-baseline], all six -musl release entries, all four macos release/LTO entries, all five windows release/LTO/baseline entries, both freebsd release entries, and both android release entries. Debug and ASAN entries are the only ones left on the default (libpas).
Why the description no longer matches
- The four
-mimalloclabels named in the description do not appear anywhere in the workflow, thereleasejob's download/rename steps, or the release-asset lists. - The sentence "existing matrix entries default to
OFFso they are unchanged" is now false — every non-debug/non-asan existing label is changed to external-mimalloc. - The description scopes the change to "Linux glibc" and
Dockerfile/release.sh, but the diff also touchesDockerfile.{musl,macos,windows,freebsd,android}and their wrapper scripts, and flips flags for those platforms in the matrix.
Step-by-step
- Read the PR title: "Add Linux mimalloc build matrix entries (amd64/arm64, release + LTO)" — implies 4 additive entries.
- Grep the final
build-reusable.ymlformimallocin alabel:— no matches. - Look at, e.g., the
bun-webkit-linux-amd64matrix entry (lines ~89-96): it now carriesUSE_MIMALLOC: "ON"andUSE_EXTERNAL_MIMALLOC: "ON". - Same for
bun-webkit-macos-arm64,bun-webkit-windows-amd64,bun-webkit-freebsd-amd64,bun-webkit-linux-arm64-android, etc. - Conclusion: the description describes an earlier revision; the actual scope is "switch every release artifact to external-mimalloc", not "add 4 new Linux variants".
Impact
This is a PR-metadata issue, not a code defect. The commit title (8e7b610) is explicit about the intent, artifacts publish under per-SHA autobuild-{sha} tags so older Bun builds pinning a specific SHA are unaffected, and the timeline shows Bun's CI has already exercised these builds. So there's no downstream breakage risk from the description being stale — the concern is purely that a reviewer approving from the title/description would underestimate the scope (an allocator/ABI change to every existing release label across 6 platforms rather than 4 additive Linux entries).
Fix
Update the PR title and description to reflect the final state, e.g. "Switch all non-debug/non-asan release artifacts to USE_MIMALLOC + USE_EXTERNAL_MIMALLOC", drop the list of -mimalloc labels, and remove/revise the "existing matrix entries default to OFF so they are unchanged" sentence. No code change needed.
The consumer's mimalloc now runs a demand-driven scavenger thread (woken from mi_arena_schedule_purge) that handles arena purge off the GC thread. The synchronous force-collect here costs ~13% on allocation- heavy workloads without further RSS benefit once the scavenger is present.
…fter full GC With the consumer's mimalloc scavenger thread handling arena madvise in the background, a force=true collect on the GC thread after every full GC is just synchronous cost (~13% on allocation-heavy workloads). force=false processes retired theap pages and schedules arena purges (waking the scavenger) without draining them inline.
Adds a webkitMimalloc config field (Linux glibc release only) that selects
the bun-webkit-linux-{amd64,arm64}-mimalloc[-lto] tarballs from
oven-sh/WebKit. Those archives route bmalloc/FastMalloc/JSC through
mimalloc instead of libpas and leave every mi_* symbol unresolved, so
bun's own oven-sh/mimalloc dep satisfies them at link time and the whole
process shares one allocator.
Wired into prebuiltSuffix()/prebuiltDestDir(), local-mode cmake args
(USE_MIMALLOC=ON, USE_EXTERNAL_MIMALLOC=ON), the CLI bool-field set, and
the config summary.
Also adds a release-mimalloc profile pinned to the oven-sh/WebKit#283
preview release (autobuild-preview-pr-283-1d65722d) until that PR merges
and WEBKIT_VERSION is bumped.
Linux glibc release builds (including CI release + release-lto) now use the -mimalloc WebKit prebuilt by default, so JSC and bun share bun's vendored mimalloc as the process-wide allocator. Debug, asan, baseline, musl and non-Linux stay on libpas (no -mimalloc prebuilts ship for those yet). webkitVersion is pinned to the oven-sh/WebKit#283 preview release when webkitMimalloc is on; drop that override in resolveConfig once the -mimalloc artifacts.
…pinned prebuilt The webkitMimalloc default now requires webkit=prebuilt with no explicit --webkit-version, so release-local builds and version overrides stay on libpas instead of forwarding mimalloc flags to a checkout that predates oven-sh/WebKit#283 or requesting -mimalloc tarballs older autobuilds don't ship. The preview pin is now the commit sha (mapped to the PR-tagged release in deps/webkit.ts), so process.versions.webkit keeps reporting a commit hash on Linux glibc release instead of the tag name.
…n for Linux glibc release f625cc54 in oven-sh/WebKit#283 fixes the two segfaults the previous preview caused on the Linux glibc release lanes: - _mi_process_fork_child walked into the MADV_DONTFORK structure-heap region where mimalloc had stored mi_arena_t/theap structs; now undone with MADV_DOFORK after mi_manage_os_memory_ex. - With Malloc=1 the mimalloc arena was created unconditionally and the bitvector fallback overwrote it; now guarded on m_useSystemHeap. Re-enable the default (reverted in 99fd9d7 for the segfaults) with the same gating as before: Linux glibc release, prebuilt WebKit, no explicit webkitVersion, non-asan, non-baseline.
…OS too tryReserveUncommittedAligned maps this region with mach_vm_map(..., VM_INHERIT_NONE) on Darwin, so the same fork-child walk into mimalloc's in-region mi_arena_t/theap structs applies there. Restore VM_INHERIT_COPY after registering the region with mimalloc, mirroring the Linux MADV_DOFORK branch. Also reword the USE_EXTERNAL_MIMALLOC help string from "Implies" to "Requires" to match WEBKIT_OPTION_DEPEND's actual semantics.
There was a problem hiding this comment.
24c63e7 addressed the macOS vm_inherit gap I flagged, and I didn't find new issues in this revision. Still deferring to a human: the scope grew from four additive Linux variants to flipping every non-debug/non-asan release label (Linux glibc/musl, macOS, Windows, FreeBSD, Android) to USE_EXTERNAL_MIMALLOC, which changes the shipped allocator and introduces an mi_* ABI contract with the consumer — that's worth explicit human sign-off.
Checked in this pass: the new Darwin vm_inherit branch mirrors the Linux MADV_DOFORK path and covers the arena metadata; the scavengeThisThread(bool force) default-arg keeps existing WTF callers source-compatible; and the || 'OFF' fallbacks in the workflow leave debug/asan entries on libpas as before.
Extended reasoning...
Overview
The PR wires a new USE_EXTERNAL_MIMALLOC CMake option through six Dockerfiles and their wrapper scripts, adds the option to WebKitFeatures.cmake with a WEBKIT_OPTION_DEPEND on USE_MIMALLOC, gates mimalloc-obj out of bmalloc_LIBRARIES (linking it into the jsc shell only), and flips USE_MIMALLOC/USE_EXTERNAL_MIMALLOC to ON for every release/LTO/baseline matrix entry across all platforms in build-reusable.yml. Runtime changes: StructureAlignedMemoryAllocator now guards the mimalloc arena path with !m_useSystemHeap and re-inherits the structure-heap region on Linux (MADV_DOFORK) and Darwin (vm_inherit(..., VM_INHERIT_COPY)); Heap::didFinishCollection calls bmalloc::api::scavengeThisThread(false) after full GCs; scavengeThisThread gains a bool force parameter defaulting to true.
Security risks
The change is squarely in memory-allocator and GC territory. It alters fork-inheritance semantics for the 4 GB structure-heap reservation, and it makes the shipped static archives depend on the consumer supplying an ABI-compatible mimalloc (11 mi_* symbols including mi_manage_os_memory_ex/mi_heap_new_in_arena). A version skew between the vendored headers compiled against here and Bun's linked mimalloc could produce subtle heap corruption rather than a clean link error. The RELEASE_ASSERT inside mi_manage_os_memory_ex still evaluates its side-effecting argument in release builds, so that path is fine. No injection/auth surface.
Level of scrutiny
High. This is not a config tweak — it swaps the process-wide allocator for every production release artifact and touches platform-specific fork-safety code that already produced two segfaults during this PR's own iteration (the _mi_process_fork_child crash and the Malloc=1 bitvector overwrite). The PR title/description are also still stale relative to the final scope (I flagged this earlier and it hasn't been updated), which itself argues for a human confirming the intended blast radius.
Other factors
Preview builds succeeded through 342334e per the bot comment, and the earlier CI failure was root-caused and fixed (the C_INCLUDE_PATH unset). My prior blocking finding (macOS fork inheritance) was addressed in 24c63e7. Two of my earlier minor notes remain open but non-blocking (DEVELOPER_MODE test executables under USE_EXTERNAL_MIMALLOC, and the stale PR description). CodeRabbit's suggestion to RELEASE_ASSERT on MADV_DOFORK failure is also unaddressed; I don't consider it blocking (the pre-existing tryReserveUncommittedAligned MADV_DONTFORK call has the same non-fatal treatment), but it's a reasonable point for a human to weigh.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Source/JavaScriptCore/heap/StructureAlignedMemoryAllocator.cpp (1)
144-161: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAssert success of the fork-inheritance restoration on both platforms.
The Linux
MADV_DOFORKretry loop discards its final result, and the new Darwinvm_inheritcall likewise ignores itskern_return_t. If either fails for a non-retryable reason, the structure heap region keeps the priorMADV_DONTFORK/VM_INHERIT_NONEinheritance, which — per this PR's own rationale — causes mimalloc's fork-child init to miss arena/theap metadata stored in this region and crash. Both branches should assert success.🔒️ Proposed fix to assert success on both platforms
`#if` OS(LINUX) && defined(MADV_DOFORK) - while (madvise(reinterpret_cast<void*>(g_jscConfig.startOfStructureHeap), g_jscConfig.sizeOfStructureHeap, MADV_DOFORK) == -1 && errno == EAGAIN) { } + int madviseResult; + while ((madviseResult = madvise(reinterpret_cast<void*>(g_jscConfig.startOfStructureHeap), g_jscConfig.sizeOfStructureHeap, MADV_DOFORK)) == -1 && errno == EAGAIN) { } + RELEASE_ASSERT(madviseResult == 0); `#elif` OS(DARWIN) - vm_inherit(mach_task_self(), static_cast<vm_address_t>(g_jscConfig.startOfStructureHeap), static_cast<vm_size_t>(g_jscConfig.sizeOfStructureHeap), VM_INHERIT_COPY); + kern_return_t inheritResult = vm_inherit(mach_task_self(), static_cast<vm_address_t>(g_jscConfig.startOfStructureHeap), static_cast<vm_size_t>(g_jscConfig.sizeOfStructureHeap), VM_INHERIT_COPY); + RELEASE_ASSERT(inheritResult == KERN_SUCCESS); `#endif`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/JavaScriptCore/heap/StructureAlignedMemoryAllocator.cpp` around lines 144 - 161, Assert successful completion of the fork-inheritance restoration in the Linux and Darwin branches within the structure-heap initialization path. Preserve the Linux EAGAIN retry loop, but retain and assert its final madvise result; likewise capture the vm_inherit return value and assert success, using the platform-appropriate success constants.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@Source/JavaScriptCore/heap/StructureAlignedMemoryAllocator.cpp`:
- Around line 144-161: Assert successful completion of the fork-inheritance
restoration in the Linux and Darwin branches within the structure-heap
initialization path. Preserve the Linux EAGAIN retry loop, but retain and assert
its final madvise result; likewise capture the vm_inherit return value and
assert success, using the platform-appropriate success constants.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 72c1d406-ba62-4775-9b60-fcc7c4db6ee9
📒 Files selected for processing (16)
.github/workflows/build-reusable.ymlDockerfile.androidDockerfile.freebsdDockerfile.macosDockerfile.muslDockerfile.windowsSource/JavaScriptCore/heap/Heap.cppSource/JavaScriptCore/heap/StructureAlignedMemoryAllocator.cppSource/bmalloc/bmalloc/bmalloc.cppSource/bmalloc/bmalloc/bmalloc.hSource/cmake/WebKitFeatures.cmakeandroid-release.shfreebsd-release.shmacos-cross-release.shmusl-release.shwindows-cross-release.sh
oven-sh/WebKit#283 makes USE_MIMALLOC the default on every non-asan release target and drops the separate -mimalloc matrix entries, so the regular archive names are the mimalloc builds and there is nothing left to select. WEBKIT_VERSION now points at the #283 preview head directly (with WEBKIT_RELEASE_TAG mapping it to the preview release tag until the PR merges and a regular autobuild exists).
Drops the WEBKIT_RELEASE_TAG indirection; prebuiltUrl's existing autobuild- prefix check handles the tag form. process.versions.webkit now shows the tag until oven-sh/WebKit#283 merges and this becomes a regular autobuild-<sha>.
fa7d098 to
779afea
Compare
779afea to
687028b
Compare
a31f622 to
b60ef78
Compare
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
Source/WTF/wtf/AutomaticThread.cpp:213— nit:didReleaseOnIdleis declared/reset unconditionally but only read inside#if !USE(SYSTEM_MALLOC), so underUSE(SYSTEM_MALLOC)(e.g. the macOS-debug lanes viaENABLE_MALLOC_HEAP_BREAKDOWN) clang emits-Wunused-but-set-variable. CI won't fail (-WerrorisDEVELOPER_MODE-gated), but wrapping the declaration and the line-220 reset in the same#if !USE(SYSTEM_MALLOC)guard (or addingUNUSED_VARIABLE(didReleaseOnIdle)) would keep it clean.Extended reasoning...
What
bool didReleaseOnIdle = false;at line 213 and its resetdidReleaseOnIdle = false;on thePollResult::Workbranch at line 220 are compiled unconditionally. The only read of the variable — and the only write-to-true— live inside the#if !USE(SYSTEM_MALLOC)block at lines 227-234. WhenUSE(SYSTEM_MALLOC)is defined, the variable is therefore initialized and reassigned but never read, which is exactly the pattern clang's-Wunused-but-set-variablediagnoses.Where this configuration is exercised
Source/cmake/OptionsJSCOnly.cmakeforcesUSE_SYSTEM_MALLOC=1wheneverENABLE_MALLOC_HEAP_BREAKDOWNis on, andmacos-cross-release.shdefaultsENABLE_MALLOC_HEAP_BREAKDOWN=ONfor Debug builds. So the CI matrix entriesbun-webkit-macos-arm64-debugandbun-webkit-macos-amd64-debugcompile this file withUSE(SYSTEM_MALLOC)true.WebKitCompilerFlags.cmake:359adds-Wall -Wextrafor GCC/Clang (non-MSVC), and-Wunused-but-set-variablehas been part of clang's-Wallsince clang-13 (LLVM 21 is used here). There is no global-Wno-unused-but-set-variablefor WTF — the codebase suppresses this warning locally withIGNORE_WARNINGS_BEGIN("unused-but-set-variable")where needed (e.g.Lexer.cpp,testapi.c), which shows it's normally addressed case-by-case rather than blanket-disabled.Why nothing prevents it
Commit 687028b guarded the body of the idle-release with
#if !USE(SYSTEM_MALLOC)but left the flag's declaration and theWork-branch reset outside the guard. Nothing else suppresses the diagnostic for this variable.Step-by-step
- Configure
bun-webkit-macos-arm64-debug:WEBKIT_RELEASE_TYPE=Debug, somacos-cross-release.shsetsENABLE_MALLOC_HEAP_BREAKDOWN=ON. OptionsJSCOnly.cmakeseesENABLE_MALLOC_HEAP_BREAKDOWNand forcesUSE_SYSTEM_MALLOC=1.AutomaticThread.cppcompiles withUSE(SYSTEM_MALLOC)true, so lines 227-234 are preprocessed out.- What remains: line 213 initializes
didReleaseOnIdle = false; line 220 assignsdidReleaseOnIdle = false; nothing reads it. - clang-21, invoked with
-Wall -Wextra(WebKitCompilerFlags.cmake:359), emitswarning: variable 'didReleaseOnIdle' set but not used [-Wunused-but-set-variable]. -Werroris only added whenDEVELOPER_MODE AND DEVELOPER_MODE_FATAL_WARNINGS(WebKitCompilerFlags.cmake:166-170); the Docker builds don't setDEVELOPER_MODE, so this stays a warning and CI passes.
Impact
No runtime effect and no CI failure — the shipped macOS-debug lanes just gain a warning. A local
-DDEVELOPER_MODE=ON -DUSE_SYSTEM_MALLOC=ON(or-DENABLE_MALLOC_HEAP_BREAKDOWN=ON) build would fail with-Werror=unused-but-set-variable, but that combination isn't exercised by CI. Hence nit.Fix
Either wrap both the declaration at line 213 and the reset at line 220 in
#if !USE(SYSTEM_MALLOC)to match the read site, or addUNUSED_VARIABLE(didReleaseOnIdle);after the declaration. - Configure
b60ef78 to
24c63e7
Compare
There was a problem hiding this comment.
Noting for the record: the mi_on_thread_idle / AutomaticThread idle-hook commit (fa7d0989) that my previous 🔴 flagged as a link-time blocker has been dropped from the branch — head is back at 24c63e7, which has a green preview build, so that finding and the associated AutomaticThread lock-hold note no longer apply. Beyond the new inline nit, nothing else surfaced this pass.
Extended reasoning...
The branch was force-pushed back to 24c63e7 after my last review; mi_on_thread_idle, bmalloc::api::onThreadIdle(), and the AutomaticThread.cpp / FastMalloc changes from fa7d0989 are no longer in the diff (verified via grep and the 20-file changed-files list). That resolves my prior 🔴 undefined-reference blocker on bmalloc.h and moots the AutomaticThread lock-hold-time nit. The remaining diff at 24c63e7 is the state the 2026-07-13 00:30 preview build already covered. This run's only new finding is the arm64 -march=armv8.1-a nit on the vendored mimalloc-obj, which affects only the tarball's bin/jsc smoke-test shell (not the shipped archives or Bun). This remains a large allocator/ABI switch across every non-debug/non-asan release label on six platforms with runtime GC and structure-heap changes, so I'm continuing to leave it for human review.
| if (USE_EXTERNAL_MIMALLOC) | ||
| # The shipped static archives intentionally leave mi_* unresolved so the | ||
| # consumer links its own mimalloc. The jsc shell still needs a working | ||
| # allocator, so link the vendored mimalloc object library here only. | ||
| list(APPEND jsc_LIBRARIES $<TARGET_OBJECTS:mimalloc-obj>) | ||
| endif () |
There was a problem hiding this comment.
🟡 The vendored mimalloc's CMakeLists forces MI_OPT_ARCH=ON on arm64, which appends -march=armv8.1-a via target_compile_options — placed after CMAKE_C_FLAGS, so it overrides the workflow's -march=armv8-a+crc baseline. Only bin/jsc is affected (the shipped .a archives don't carry mimalloc-obj under USE_EXTERNAL_MIMALLOC), so Bun itself is fine, but the tarball's smoke-test shell will SIGILL on ARMv8.0 hardware (Cortex-A53/A57/A72). One-line fix: add set(MI_NO_OPT_ARCH ON CACHE BOOL "" FORCE) to Source/bmalloc/mimalloc/CMakeLists.txt alongside the other MI_* overrides.
Extended reasoning...
What
The wrapper at Source/bmalloc/mimalloc/CMakeLists.txt sets seven MI_* cache vars (MI_OVERRIDE, MI_OSX_INTERPOSE, MI_OSX_ZONE, MI_BUILD_SHARED, MI_BUILD_STATIC, MI_BUILD_OBJECT, MI_BUILD_TESTS) but does not set MI_NO_OPT_ARCH. The vendored Source/bmalloc/mimalloc/mimalloc/CMakeLists.txt then does:
- Line 137-138:
CMAKE_SYSTEM_PROCESSORmatchingarm64|ARM64|aarch64|AARCH64→MI_ARCH=arm64 - Line 157-160:
MI_NO_OPT_ARCHis unset → forMI_ARCH==arm64, forceset(MI_OPT_ARCH "ON") - Line 484-485:
MI_OPT_ARCH+MI_ARCH==arm64→set(MI_OPT_ARCH_FLAGS "-march=armv8.1-a") - Line 505-506:
list(APPEND mi_cflags ${MI_OPT_ARCH_FLAGS}) - Line 711:
target_compile_options(mimalloc-obj PRIVATE ${mi_cflags} ...)
CMake places target_compile_options after CMAKE_C_FLAGS on the compile line, and clang uses the last -march= when several are given. So -march=armv8.1-a overrides the workflow's deliberate -march=armv8-a+crc -mtune=ampere1 (build-reusable.yml:150, release.sh, musl-release.sh, android-release.sh, freebsd-release.sh, windows-cross-release.sh all set an armv8-a baseline for arm64).
Why it's newly live
Before this PR, USE_MIMALLOC_DEFAULT was OFF on x86_64/arm64 (WebKitFeatures.cmake:98), so mimalloc-obj was never built for the arm64 release lanes. Commit 8e7b610 in this PR flips USE_MIMALLOC=ON/USE_EXTERNAL_MIMALLOC=ON on every non-debug/non-asan release lane, and shell/CMakeLists.txt:21-26 now links $<TARGET_OBJECTS:mimalloc-obj> into the jsc executable so "bin/jsc stays runnable in the tarball".
x64 is not affected: line 13 defaults MI_OPT_ARCH to OFF, and only the arm64 branch at line 159-160 forces it back on.
Why nothing catches it
CI runs on linux-arm64-gh (Ampere/Graviton, ARMv8.2+), the Docker builds never execute bin/jsc after copying it, and the PR description's manual ./bin/jsc -e ... verification was on linux-x64.
Impact — why this is a nit
The shipped static archives (libbmalloc.a/libWTF.a/libJavaScriptCore.a) are not affected: under USE_EXTERNAL_MIMALLOC=ON, Source/bmalloc/CMakeLists.txt:660 guards its append with AND NOT USE_EXTERNAL_MIMALLOC, so mimalloc-obj is linked only into bin/jsc and the archives carry mi_* as undefined refs for Bun to resolve. Bun itself is unaffected. Only the tarball's smoke-test shell contradicts the arch baseline the rest of the tarball is built for — and nobody in practice runs bin/jsc from the bun-webkit prebuilt on Raspberry Pi hardware. The failure mode is a loud SIGILL, not silent corruption.
Step-by-step proof
bun-webkit-linux-arm64matrix entry:MARCH_FLAG="-march=armv8-a+crc -mtune=ampere1",USE_MIMALLOC=ON,USE_EXTERNAL_MIMALLOC=ON.- Dockerfile passes
MARCH_FLAGintoCFLAGS→CMAKE_C_FLAGS. Source/bmalloc/CMakeLists.txt→add_subdirectory(mimalloc)→ wrapper doesn't setMI_NO_OPT_ARCH→ vendored CMakeLists detectsaarch64→MI_ARCH=arm64→ forcesMI_OPT_ARCH=ON→mi_cflagsgets-march=armv8.1-a.static.ccompile line:clang ... -march=armv8-a+crc -mtune=ampere1 ... -march=armv8.1-a -c static.c→ last-marchwins → clang emits LSE atomics (casal,ldadd,swp) for mimalloc's_Atomicfast paths.shell/CMakeLists.txt:25links$<TARGET_OBJECTS:mimalloc-obj>intojsc.Dockerfile:301/Dockerfile.muslcopy$WEBKIT_OUT_DIR/bin→/output/bin→ shipped inbun-webkit-linux-arm64.tar.gz.- On a Cortex-A53/A57/A72 (Raspberry Pi 3/4, AWS Graviton1):
./bin/jsc -e 'print(1)'→ firstmi_malloc→casal→ SIGILL.
Fix
Add one line to Source/bmalloc/mimalloc/CMakeLists.txt alongside the existing overrides:
set(MI_NO_OPT_ARCH ON CACHE BOOL "Inherit -march from the parent build" FORCE)so mimalloc inherits the parent build's -march instead of overriding it.
oven-sh/WebKit#283 has merged, so the preview release it was pinned to is gone. Points at 4895f45d instead: the merge of #283 plus one follow-up fix. That follow-up is worth knowing about — main's build broke immediately after the merge, and it was not the merge. Kitware shipped cmake 4.4.0 to their apt repo between #283's CI run and main's, and the glibc Dockerfile installs cmake unpinned. 4.4 rejects an empty STREQUAL operand that 4.3 accepted, and _WEBKIT_TARGET_LINK_FRAMEWORK feeds it one: <framework>_LINKED_INTO is only set for shared libraries, so every ENABLE_STATIC_JSC=ON configure hit it. Quoting the operands fixes it; nothing about it is mimalloc-specific. Pinning cmake in that Dockerfile is worth doing separately: an upstream apt release broke main with no code change on our side, and will again.
…in 4895f45d) [skip size check] Carries oven-sh/WebKit#283 (mimalloc build matrix) and the _LINKED_INTO cmake fix from fork main, plus #278. Preview published with 43 artifacts.
2 participants
Adds four new Linux glibc matrix entries that build JSC with mimalloc as the allocator instead of libpas:
bun-webkit-linux-amd64-mimallocbun-webkit-linux-amd64-mimalloc-ltobun-webkit-linux-arm64-mimallocbun-webkit-linux-arm64-mimalloc-ltoUSE_EXTERNAL_MIMALLOC
New CMake option (requires
USE_MIMALLOC=ON). When set, bmalloc/WTF/JSC compile against the vendored mimalloc v3 headers, butmimalloc-objis not linked intobmalloc_LIBRARIES, so the shippedlibbmalloc.a/libWTF.a/libJavaScriptCore.acarrymi_*as undefined references. Bun provides them at link time from its ownoven-sh/mimalloc(v3.3.2) build so there is a single mimalloc in the process.The vendored
mimalloc-objis still built and linked directly into thejscshell sobin/jscstays runnable in the tarball.Verified locally (Release, linux-x64)
All 11 referenced symbols are present with identical signatures in both the vendored mimalloc v3.2.8 header and Bun's v3.3.2 header.
Also drops an unused
#include <mimalloc/types.h>fromStructureAlignedMemoryAllocator.cpp; its include path was never wired into JavaScriptCore, and nothing from that header is used.Wired through
Dockerfile/release.shas build args; existing matrix entries default toOFFso they are unchanged.