-
Notifications
You must be signed in to change notification settings - Fork 4.3k
deps: bump mimalloc to dev3 (bun-dev3-v2) #29251
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
eb1bae3
f91d3cf
4ff1319
cc6c7e7
a0d69d0
7eb3fb5
d811dec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,7 @@ | |
|
|
||
| import type { Dependency, NestedCmakeBuild, Provides } from "../source.ts"; | ||
|
|
||
| const MIMALLOC_COMMIT = "1beadf9651a7bfdec6b5367c380ecc3fe1c40d1a"; | ||
| const MIMALLOC_COMMIT = "76101991f43176330c0bf97f807a52c2f373bd10"; | ||
|
|
||
| export const mimalloc: Dependency = { | ||
| name: "mimalloc", | ||
|
|
@@ -68,8 +68,13 @@ export const mimalloc: Dependency = { | |
| // so UBSan doesn't false-positive on mimalloc's type punning. | ||
| args.MI_DEBUG_UBSAN = "ON"; | ||
| } else if (cfg.darwin) { | ||
| args.MI_OVERRIDE = "OFF"; | ||
| args.MI_OSX_ZONE = "OFF"; | ||
| // Register the mimalloc malloc_zone so leaks/heap/vmmap/Instruments can | ||
| // enumerate our allocations and the fork-prepare/child hooks fire. | ||
| // Interpose is OFF because we link the .o statically; the __interpose | ||
| // section only works for inserted dylibs. The zone-swap path in | ||
| // alloc-override-zone.c handles making mimalloc the default zone. | ||
| args.MI_OVERRIDE = "ON"; | ||
| args.MI_OSX_ZONE = "ON"; | ||
| args.MI_OSX_INTERPOSE = "OFF"; | ||
| } else if (cfg.linux) { | ||
| args.MI_OVERRIDE = "ON"; | ||
|
Comment on lines
68
to
80
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The 'Override behavior' decision matrix comment in Extended reasoning...What the bug is The decision matrix comment block (lines ~53–58) in However, this PR changed the darwin branch of the build configuration to: args.MI_OVERRIDE = "ON";
args.MI_OSX_ZONE = "ON";The decision matrix was not updated when the behavior changed. The specific code path The decision matrix at the top of the override behavior section is intended as the authoritative, at-a-glance summary of per-platform override decisions. The darwin block below it does include a new inline comment explaining the new rationale (zone registration for Why existing code doesn't prevent it This is a documentation-only inconsistency with no runtime impact. There is no automated check that validates the decision matrix comment against the actual CMake argument values. The inline comment in the darwin block partially addresses this, but the authoritative matrix at the top of the section remains wrong. Impact A developer reading the decision matrix to understand platform behavior would conclude macOS overriding is OFF and rely on that understanding when reasoning about allocator behavior, debugging macOS-specific issues, or reviewing future changes. The actual runtime behavior is the opposite of what the matrix says. This discrepancy is particularly misleading because the old rationale ("causes issues with system frameworks") was the reason for the previous OFF setting — a future developer might even revert the change citing that comment as justification. How to fix it Update the macOS line in the decision matrix to reflect the new ON behavior and the new rationale, e.g.: Step-by-step proof
|
||
|
|
@@ -91,10 +96,10 @@ export const mimalloc: Dependency = { | |
| args.MI_TRACK_VALGRIND = "ON"; | ||
| } | ||
|
|
||
| // If mimalloc gets bumped to a version with MI_OPT_ARCH: pass | ||
| // MI_NO_OPT_ARCH=ON to stop it setting -march=armv8.1-a on arm64 | ||
| // (SIGILLs on ARMv8.0 CPUs). Current pin has no arch-detection logic | ||
| // so our global -march=armv8-a+crc (via CMAKE_CXX_FLAGS) is sufficient. | ||
| // dev3 grew MI_OPT_ARCH which sets -march=armv8.1-a on arm64 — that | ||
| // SIGILLs on ARMv8.0 CPUs. Explicitly disable it; our global | ||
| // -march=armv8-a+crc (via CMAKE_CXX_FLAGS) is sufficient. | ||
| args.MI_NO_OPT_ARCH = "ON"; | ||
|
|
||
| // ─── Windows: silence the vendored-C-as-C++ warning flood ─── | ||
| // MI_USE_CXX=ON means .c files compile as C++. clang-cl then complains | ||
|
|
@@ -121,7 +126,7 @@ export const mimalloc: Dependency = { | |
| // to override this, so we have to mirror its naming logic. | ||
| let libname: string; | ||
| if (cfg.windows) { | ||
| libname = cfg.debug ? "mimalloc-static-debug" : "mimalloc-static"; | ||
| libname = cfg.debug ? "mimalloc-debug" : "mimalloc"; | ||
|
Comment on lines
128
to
+129
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Description: Check mimalloc CMakeLists.txt for static library naming logic
# Search for library naming patterns in mimalloc's CMake configuration
rg -n -C5 'mimalloc-static|OUTPUT_NAME|set_target_properties.*mimalloc' --type cmakeRepository: oven-sh/bun Length of output: 37 🏁 Script executed: cat -n scripts/build/deps/mimalloc.ts | head -50Repository: oven-sh/bun Length of output: 2108 🌐 Web query:
💡 Result: For mimalloc v3 on Windows using CMake with MI_BUILD_STATIC enabled, the static library target is named "mimalloc-static". The output library file is mimalloc-static.lib (on Windows) or libmimalloc-static.a (on Unix-like systems). The CMake option MI_BUILD_STATIC (default ON) controls whether the static library is built: option(MI_BUILD_STATIC "Build static library" ON). To link, use target_link_libraries(myapp PUBLIC mimalloc-static) after find_package(mimalloc CONFIG REQUIRED). This applies to v3.x releases (e.g., v3.2.8 as of 2026), consistent with v2 behavior. Note that Visual Studio .sln builds mimalloc-lib.lib directly, but CMake uses the -static suffix to avoid naming conflicts with DLL import libs. Citations:
🏁 Script executed: sed -n '115,130p' scripts/build/deps/mimalloc.tsRepository: oven-sh/bun Length of output: 565 🏁 Script executed: sed -n '115,145p' scripts/build/deps/mimalloc.tsRepository: oven-sh/bun Length of output: 1453 🏁 Script executed: git log --oneline -20 --all -- scripts/build/deps/mimalloc.tsRepository: oven-sh/bun Length of output: 213 🏁 Script executed: git show HEAD:scripts/build/deps/mimalloc.ts | sed -n '115,130p'Repository: oven-sh/bun Length of output: 565 🏁 Script executed: git show f91d3cf9:scripts/build/deps/mimalloc.ts | sed -n '115,130p'Repository: oven-sh/bun Length of output: 565 🏁 Script executed: git diff a8ee0d38^..a8ee0d38 -- scripts/build/deps/mimalloc.ts | head -60Repository: oven-sh/bun Length of output: 2100 🏁 Script executed: git show a8ee0d38 | grep -A 5 -B 5 "mimalloc-debug\|mimalloc-static" | head -50Repository: oven-sh/bun Length of output: 950 🏁 Script executed: # Check if there are any build/cmake files that reference the old naming
rg -n "mimalloc-static-debug|mimalloc-static" scripts/build/ || echo "No matches found"Repository: oven-sh/bun Length of output: 141 🏁 Script executed: # Check the current HEAD to see both the targets and the libname usage
sed -n '100,130p' scripts/build/deps/mimalloc.tsRepository: oven-sh/bun Length of output: 1146 🏁 Script executed: # Check what the actual library output files should be named
# by looking at how libname is used in the provides function
sed -n '115,160p' scripts/build/deps/mimalloc.tsRepository: oven-sh/bun Length of output: 1708 🏁 Script executed: # Check the commit that changed the Windows library naming
git log --all --oneline --grep="mimalloc" -- scripts/build/deps/mimalloc.ts | head -5Repository: oven-sh/bun Length of output: 158 🏁 Script executed: # Look for the actual commit that changed from mimalloc-static to mimalloc
git log -p --all -- scripts/build/deps/mimalloc.ts | grep -A 10 -B 10 "mimalloc-debug.*mimalloc" | head -60Repository: oven-sh/bun Length of output: 37 🏁 Script executed: git show f91d3cf9 --stat | head -20Repository: oven-sh/bun Length of output: 1133 🏁 Script executed: git show f91d3cf9 -- scripts/build/deps/mimalloc.ts | head -100Repository: oven-sh/bun Length of output: 3941 🏁 Script executed: # Check if there was a previous version of this file in CMake form
git log --all --oneline -- cmake/targets/BuildBun.cmake | head -5Repository: oven-sh/bun Length of output: 332 🏁 Script executed: # Look for any PRs or discussions about the mimalloc library naming change
git log -p --all -- scripts/build/deps/mimalloc.ts | grep -B 20 "mimalloc-debug\|mimalloc-static" | grep -A 5 -B 5 "Windows" | head -40Repository: oven-sh/bun Length of output: 37 🏁 Script executed: # Find commits that changed the Windows libname from mimalloc-static to mimalloc
git log -p --all -- scripts/build/deps/mimalloc.ts | grep -B 5 -A 5 'libname = cfg.debug ? "mimalloc' | head -50Repository: oven-sh/bun Length of output: 37 🏁 Script executed: # Check the commit just before f91d3cf9
git log --oneline -10 f91d3cf9^..HEAD -- scripts/build/deps/mimalloc.tsRepository: oven-sh/bun Length of output: 94 🏁 Script executed: # Check the CMake deletion commit to see what it was before
git show ba05a729 -- cmake/targets/BuildBun.cmake | grep -i mimalloc | head -20Repository: oven-sh/bun Length of output: 254 🏁 Script executed: # Look at the commit before ba05a729
git show ba05a729^:cmake/targets/BuildBun.cmake | grep -i mimalloc -A 2 -B 2 | head -40Repository: oven-sh/bun Length of output: 560 🏁 Script executed: # Find all commits between a8ee0d38 and f91d3cf9 that modified mimalloc.ts
git log --oneline a8ee0d38..f91d3cf9 -- scripts/build/deps/mimalloc.tsRepository: oven-sh/bun Length of output: 158 🏁 Script executed: # Look at the current HEAD commit message to understand the context
git show --quiet f91d3cf9Repository: oven-sh/bun Length of output: 691 🏁 Script executed: # Check if oven-sh/mimalloc fork has its own CMakeLists that differs from upstream
# by looking at recent changes to the mimalloc fork
cd /tmp && git clone --depth 1 https://github.com/oven-sh/mimalloc.git 2>&1 | grep -i "fatal\|clone" || echo "Clone check"Repository: oven-sh/bun Length of output: 66 🏁 Script executed: # Get commit info for the pinned mimalloc commit
git log --oneline --all | grep "09fed41b" || echo "Not in bun repo"Repository: oven-sh/bun Length of output: 70 🏁 Script executed: # Check the actual cmake/targets/BuildBun.cmake from before deletion to see how mimalloc libs were linked
git show ba05a729^:cmake/targets/BuildBun.cmake | grep -i "mimalloc" -B 3 -A 3Repository: oven-sh/bun Length of output: 782 🏁 Script executed: # Check how libname is used - look for references to it in the provides object
sed -n '115,154p' scripts/build/deps/mimalloc.tsRepository: oven-sh/bun Length of output: 1708 🏁 Script executed: # Look at the git log for the original creation to see if there's context about the naming
git show a8ee0d38:scripts/build/deps/mimalloc.ts | sed -n '100,140p'Repository: oven-sh/bun Length of output: 1734 Fix Windows library name mismatch in CMake targets. The CMake targets still build 🤖 Prompt for AI Agents |
||
| } else if (cfg.debug) { | ||
| libname = cfg.asan ? "mimalloc-asan-debug" : "mimalloc-debug"; | ||
| } else { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -94,6 +94,7 @@ __aarch64_ldclr1_acq_rel [LSE] | |
| __aarch64_ldclr2_acq_rel [LSE] | ||
| __aarch64_ldclr4_acq_rel [LSE] | ||
| __aarch64_ldclr8_acq_rel [LSE] | ||
| __aarch64_ldclr8_relax [LSE] | ||
|
Comment on lines
94
to
+97
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The section header comment says Extended reasoning...What the bug is The compiler-rt outline atomics section header in The specific code path that triggers it The diff adds these four entries to the LSE section:
Counting the actual Why existing code doesn't prevent it The comment is a human-maintained annotation — there is no automated check that verifies the count in the comment matches the actual number of entries in the section. The allowlist itself works on symbol names, not on the count in the comment. What the impact would be Zero functional impact — the allowlist matcher uses symbol names, not the count. However, the comment exists to help maintainers quickly verify the allowlist is complete. A stale count of 41 when there are actually 45 entries could mislead a reviewer into thinking symbols are missing or that the section is incomplete, creating unnecessary confusion during audits. How to fix it Change the comment from Step-by-step proof
|
||
| __aarch64_ldeor1_acq_rel [LSE] | ||
| __aarch64_ldeor2_acq_rel [LSE] | ||
| __aarch64_ldeor4_acq_rel [LSE] | ||
|
|
@@ -102,6 +103,7 @@ __aarch64_ldset1_acq_rel [LSE] | |
| __aarch64_ldset2_acq_rel [LSE] | ||
| __aarch64_ldset4_acq_rel [LSE] | ||
| __aarch64_ldset8_acq_rel [LSE] | ||
| __aarch64_ldset8_relax [LSE] | ||
| __aarch64_swp1_acq [LSE] | ||
| __aarch64_swp1_acq_rel [LSE] | ||
| __aarch64_swp1_relax [LSE] | ||
|
|
@@ -110,6 +112,8 @@ __aarch64_swp4_acq [LSE] | |
| __aarch64_swp4_acq_rel [LSE] | ||
| __aarch64_swp4_rel [LSE] | ||
| __aarch64_swp8_acq_rel [LSE] | ||
| __aarch64_swp8_rel [LSE] | ||
| __aarch64_swp8_relax [LSE] | ||
|
|
||
|
|
||
| # ---------------------------------------------------------------------------- | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -8,9 +8,8 @@ const Self = @This(); | |||||||||||||||||||||
| /// | ||||||||||||||||||||||
| /// This type is a `GenericAllocator`; see `src/allocators.zig`. | ||||||||||||||||||||||
| pub const Default = struct { | ||||||||||||||||||||||
| pub fn allocator(self: Default) std.mem.Allocator { | ||||||||||||||||||||||
| _ = self; | ||||||||||||||||||||||
| return Borrowed.getDefault().allocator(); | ||||||||||||||||||||||
| pub fn allocator(_: Default) std.mem.Allocator { | ||||||||||||||||||||||
| return .{ .ptr = undefined, .vtable = &global_mimalloc_vtable }; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| }; | ||||||||||||||||||||||
|
Comment on lines
8
to
14
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The doc comment on Extended reasoning...What the bug is and how it manifests The doc comment on the This comment was accurate before this PR, when The specific code path that triggers it
Why existing code doesn't prevent it The stale comment is purely a documentation issue — there is no runtime check or assertion that catches incorrect comments. The implementation is correct; only the doc comment describing it is wrong. What the impact would be A developer reading the comment on How to fix it Update line 7 to reflect the new behavior: -/// Uses the default thread-local heap. This type is zero-sized.
+/// Routes allocations through mi_malloc/mi_free directly; no per-thread heap pointer is cached.
+/// This type is zero-sized.Step-by-step proof
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
@@ -23,11 +22,20 @@ pub const Borrowed = struct { | |||||||||||||||||||||
| #heap: BorrowedHeap, | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| pub fn allocator(self: Borrowed) std.mem.Allocator { | ||||||||||||||||||||||
| return .{ .ptr = self.#heap, .vtable = &c_allocator_vtable }; | ||||||||||||||||||||||
| return .{ .ptr = self.#heap, .vtable = &heap_allocator_vtable }; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /// Prefer `Default.allocator()` / `getThreadLocalDefault()` for a thread-safe | ||||||||||||||||||||||
| /// global allocator. This returns the process-wide main heap so that | ||||||||||||||||||||||
| /// `gc()` / `ownsPtr()` on the result remain meaningful. | ||||||||||||||||||||||
| pub fn getDefault() Borrowed { | ||||||||||||||||||||||
| return .{ .#heap = getThreadHeap() }; | ||||||||||||||||||||||
| const heap = mimalloc.mi_heap_main(); | ||||||||||||||||||||||
| if (comptime !safety_checks) return .{ .#heap = heap }; | ||||||||||||||||||||||
| const S = struct { | ||||||||||||||||||||||
| threadlocal var dbg: ?DebugHeap = null; | ||||||||||||||||||||||
| }; | ||||||||||||||||||||||
| if (S.dbg == null) S.dbg = .{ .inner = heap, .thread_lock = .initLocked() }; | ||||||||||||||||||||||
| return .{ .#heap = &S.dbg.? }; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| pub fn gc(self: Borrowed) void { | ||||||||||||||||||||||
|
|
@@ -42,7 +50,7 @@ pub const Borrowed = struct { | |||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| pub fn ownsPtr(self: Borrowed, ptr: *const anyopaque) bool { | ||||||||||||||||||||||
| return mimalloc.mi_heap_check_owned(self.getMimallocHeap(), ptr); | ||||||||||||||||||||||
| return mimalloc.mi_heap_contains(self.getMimallocHeap(), ptr); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| fn fromOpaque(ptr: *anyopaque) Borrowed { | ||||||||||||||||||||||
|
|
@@ -81,8 +89,8 @@ pub const Borrowed = struct { | |||||||||||||||||||||
|
|
||||||||||||||||||||||
| pub fn downcast(std_alloc: std.mem.Allocator) Borrowed { | ||||||||||||||||||||||
| bun.assertf( | ||||||||||||||||||||||
| isInstance(std_alloc), | ||||||||||||||||||||||
| "not a MimallocArena (vtable is {*})", | ||||||||||||||||||||||
| std_alloc.vtable == &heap_allocator_vtable, | ||||||||||||||||||||||
| "not an owned MimallocArena heap (vtable is {*})", | ||||||||||||||||||||||
| .{std_alloc.vtable}, | ||||||||||||||||||||||
|
Comment on lines
+92
to
94
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== MimallocArena.isInstance call sites ==="
rg -n -C3 --glob '*.zig' 'MimallocArena\.isInstance\s*\('
echo
echo "=== Borrowed.downcast call sites ==="
rg -n -C3 --glob '*.zig' 'MimallocArena\.Borrowed\.downcast\s*\(|Borrowed\.downcast\s*\('Repository: oven-sh/bun Length of output: 1677 🏁 Script executed: # Check context around line 165 in src/safety/alloc.zig to see if downcast follows
cat -n src/safety/alloc.zig | sed -n '162,180p'Repository: oven-sh/bun Length of output: 838 🏁 Script executed: # Verify the actual vtable checks in MimallocArena.zig at the mentioned lines
echo "=== Lines 92-94 (Borrowed.downcast assertion) ==="
cat -n src/allocators/MimallocArena.zig | sed -n '90,96p'
echo ""
echo "=== Lines 264-265 (isInstance function) ==="
cat -n src/allocators/MimallocArena.zig | sed -n '263,267p'Repository: oven-sh/bun Length of output: 670 🏁 Script executed: # Check what vtables Default.allocator() and getThreadLocalDefault() produce
echo "=== Search for Default.allocator and getThreadLocalDefault ==="
rg -n -A5 'pub fn allocator\(\)' src/allocators/MimallocArena.zig
rg -n -A5 'pub fn getThreadLocalDefault' src/allocators/MimallocArena.zigRepository: oven-sh/bun Length of output: 384 🏁 Script executed: # Check if there are any other Borrowed usages beyond downcast that might have implicit safety assumptions
echo "=== Borrowed struct and all its methods ==="
rg -n -B2 -A8 'pub.*Borrowed' src/allocators/MimallocArena.zig | head -60Repository: oven-sh/bun Length of output: 2186 🏁 Script executed: # Look for Default.allocator() - the review mentions it produces global_mimalloc_vtable
echo "=== Default.allocator implementation ==="
rg -n -B2 -A5 'pub.*fn.*allocator.*\(\s*\)' src/allocators/MimallocArena.zig | grep -A5 "Default"
echo ""
echo "=== Check all public Default methods ==="
rg -n -B1 -A6 'pub const Default' src/allocators/MimallocArena.zigRepository: oven-sh/bun Length of output: 467 🏁 Script executed: # Double-check for any Borrowed.downcast calls that might exist
echo "=== Any downcast calls anywhere in codebase ==="
rg -n 'downcast\s*\(' --glob '*.zig' | grep -i mimalloc
echo ""
echo "=== Check if any code path could trigger downcast on global_mimalloc_vtable ==="
# Search for patterns that might combine isInstance check with downcast
rg -n -B5 -A5 'downcast' src/allocators/MimallocArena.zigRepository: oven-sh/bun Length of output: 747 🏁 Script executed: # Check the context around the downcast call in LinkerGraph.zig
echo "=== Context around downcast call in LinkerGraph.zig:420 ==="
cat -n src/bundler/LinkerGraph.zig | sed -n '410,430p'
echo ""
echo "=== Look at the class/struct definition for LinkerGraph to understand this.allocator ==="
rg -n -B5 -A15 'pub const LinkerGraph = struct' src/bundler/LinkerGraph.zig | head -40Repository: oven-sh/bun Length of output: 1070 🏁 Script executed: # Search for LinkerGraph's allocator field definition
echo "=== LinkerGraph allocator field ==="
rg -n 'allocator.*:' src/bundler/LinkerGraph.zig | head -20
echo ""
echo "=== Find where LinkerGraph is instantiated/created to see what allocator is passed ==="
rg -n 'LinkerGraph.*=' src/bundler/LinkerGraph.zig | head -10Repository: oven-sh/bun Length of output: 325 🏁 Script executed: # Search for LinkerGraph creation/init calls
echo "=== LinkerGraph creation patterns ==="
rg -n 'LinkerGraph\.init\(' --glob '*.zig' -B2 -A2
echo ""
echo "=== Alternative: search for LinkerGraph{...} construction ==="
rg -n 'LinkerGraph\s*\{' --glob '*.zig' -B2 -A2 | head -40Repository: oven-sh/bun Length of output: 580 Fix API contract between
Either narrow pub fn isInstance(alloc: std.mem.Allocator) bool {
- return alloc.vtable == &heap_allocator_vtable or alloc.vtable == &global_mimalloc_vtable;
+ return alloc.vtable == &heap_allocator_vtable;
}
+
+pub fn isMimallocBacked(alloc: std.mem.Allocator) bool {
+ return alloc.vtable == &heap_allocator_vtable or alloc.vtable == &global_mimalloc_vtable;
+}Alternatively, add a guard before the downcast in 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||
| ); | ||||||||||||||||||||||
| return .fromOpaque(std_alloc.ptr); | ||||||||||||||||||||||
|
|
@@ -98,19 +106,6 @@ const DebugHeap = struct { | |||||||||||||||||||||
| pub const deinit = void; | ||||||||||||||||||||||
| }; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| threadlocal var thread_heap: if (safety_checks) ?DebugHeap else void = if (safety_checks) null; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| fn getThreadHeap() BorrowedHeap { | ||||||||||||||||||||||
| if (comptime !safety_checks) return mimalloc.mi_heap_get_default(); | ||||||||||||||||||||||
| if (thread_heap == null) { | ||||||||||||||||||||||
| thread_heap = .{ | ||||||||||||||||||||||
| .inner = mimalloc.mi_heap_get_default(), | ||||||||||||||||||||||
| .thread_lock = .initLocked(), | ||||||||||||||||||||||
| }; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| return &thread_heap.?; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const log = bun.Output.scoped(.mimalloc, .hidden); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| pub fn allocator(self: Self) std.mem.Allocator { | ||||||||||||||||||||||
|
|
@@ -121,13 +116,11 @@ pub fn borrow(self: Self) Borrowed { | |||||||||||||||||||||
| return .{ .#heap = if (comptime safety_checks) self.#heap.get() else self.#heap }; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /// Internally, mimalloc calls mi_heap_get_default() | ||||||||||||||||||||||
| /// to get the default heap. | ||||||||||||||||||||||
| /// It uses pthread_getspecific to do that. | ||||||||||||||||||||||
| /// We can save those extra calls if we just do it once in here | ||||||||||||||||||||||
| /// In v3, `mi_malloc`/`mi_free` are already thread-local-fast — there is no | ||||||||||||||||||||||
| /// per-thread default heap to cache. Route through the global vtable. | ||||||||||||||||||||||
| pub fn getThreadLocalDefault() std.mem.Allocator { | ||||||||||||||||||||||
| if (bun.Environment.enable_asan) return bun.default_allocator; | ||||||||||||||||||||||
| return Borrowed.getDefault().allocator(); | ||||||||||||||||||||||
| return .{ .ptr = undefined, .vtable = &global_mimalloc_vtable }; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| pub fn backingAllocator(_: Self) std.mem.Allocator { | ||||||||||||||||||||||
|
|
@@ -251,17 +244,43 @@ fn vtable_remap(ptr: *anyopaque, buf: []u8, alignment: Alignment, new_len: usize | |||||||||||||||||||||
| return @ptrCast(value); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| fn global_vtable_alloc(_: *anyopaque, len: usize, alignment: Alignment, _: usize) ?[*]u8 { | ||||||||||||||||||||||
| log("Malloc: {d}\n", .{len}); | ||||||||||||||||||||||
| const ptr: ?*anyopaque = if (mimalloc.mustUseAlignedAlloc(alignment)) | ||||||||||||||||||||||
| mimalloc.mi_malloc_aligned(len, alignment.toByteUnits()) | ||||||||||||||||||||||
| else | ||||||||||||||||||||||
| mimalloc.mi_malloc(len); | ||||||||||||||||||||||
| return if (ptr) |p| @ptrCast(p) else null; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| fn global_vtable_resize(_: *anyopaque, buf: []u8, _: Alignment, new_len: usize, _: usize) bool { | ||||||||||||||||||||||
|
Comment on lines
244
to
+256
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The new Extended reasoning...What the bug is:
if (comptime bun.Environment.isDebug) {
const usable = mimalloc.mi_malloc_usable_size(ptr);
if (usable < len) {
std.debug.panic("mimalloc: allocated size is too small: {d} < {d}", .{ usable, len });
}
}The new The specific code path:
Why existing code doesn't prevent it: The check in Impact: In debug builds, any allocation going through How to fix: Add the same debug assertion in fn global_vtable_alloc(_: *anyopaque, len: usize, alignment: Alignment, _: usize) ?[*]u8 {
log("Malloc: {d}\n", .{len});
const ptr: ?*anyopaque = if (mimalloc.mustUseAlignedAlloc(alignment))
mimalloc.mi_malloc_aligned(len, alignment.toByteUnits())
else
mimalloc.mi_malloc(len);
if (comptime bun.Environment.isDebug) {
const usable = mimalloc.mi_malloc_usable_size(ptr);
if (usable < len) {
std.debug.panic("mimalloc: allocated size is too small: {d} < {d}", .{ usable, len });
}
}
return if (ptr) |p| @ptrCast(p) else null;
}Step-by-step proof:
|
||||||||||||||||||||||
| return mimalloc.mi_expand(buf.ptr, new_len) != null; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| fn global_vtable_remap(_: *anyopaque, buf: []u8, alignment: Alignment, new_len: usize, _: usize) ?[*]u8 { | ||||||||||||||||||||||
| return @ptrCast(mimalloc.mi_realloc_aligned(buf.ptr, new_len, alignment.toByteUnits())); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| pub fn isInstance(alloc: std.mem.Allocator) bool { | ||||||||||||||||||||||
| return alloc.vtable == &c_allocator_vtable; | ||||||||||||||||||||||
| return alloc.vtable == &heap_allocator_vtable or alloc.vtable == &global_mimalloc_vtable; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const c_allocator_vtable = std.mem.Allocator.VTable{ | ||||||||||||||||||||||
| /// VTable for owned heaps created with `mi_heap_new`. | ||||||||||||||||||||||
| const heap_allocator_vtable = std.mem.Allocator.VTable{ | ||||||||||||||||||||||
| .alloc = vtable_alloc, | ||||||||||||||||||||||
| .resize = vtable_resize, | ||||||||||||||||||||||
| .remap = vtable_remap, | ||||||||||||||||||||||
| .free = vtable_free, | ||||||||||||||||||||||
| }; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /// VTable for the process-wide default allocator (`mi_malloc`/`mi_free`). | ||||||||||||||||||||||
| const global_mimalloc_vtable = std.mem.Allocator.VTable{ | ||||||||||||||||||||||
| .alloc = global_vtable_alloc, | ||||||||||||||||||||||
| .resize = global_vtable_resize, | ||||||||||||||||||||||
| .remap = global_vtable_remap, | ||||||||||||||||||||||
| .free = vtable_free, | ||||||||||||||||||||||
| }; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
Comment on lines
+268
to
+283
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The PR splits the single mimalloc vtable into two (heap_allocator_vtable for owned heaps, global_mimalloc_vtable for the global allocator), but isInstance() returns true for both while Borrowed.downcast() only accepts heap_allocator_vtable — any code following the natural isInstance-then-downcast idiom will panic at the assertf for global-vtable allocators. No existing callers use this pattern today, so there is no immediate breakage, but the API contract is misleading since isInstance() returning true no longer implies downcast() will succeed. Extended reasoning...What the bug is and how it manifests This PR intentionally splits the old single The specific code path that triggers it
Why existing code does not prevent it Before this PR, What the impact would be The conventional semantic of How to fix it Two reasonable options: (1) Rename Step-by-step proof
|
||||||||||||||||||||||
| const std = @import("std"); | ||||||||||||||||||||||
| const Alignment = std.mem.Alignment; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -46,6 +46,7 @@ | |||||||||||||||||||||||||||||||
| #include "JSDOMConvertBase.h" | ||||||||||||||||||||||||||||||||
| #include "ZigSourceProvider.h" | ||||||||||||||||||||||||||||||||
| #include "mimalloc.h" | ||||||||||||||||||||||||||||||||
| extern "C" char* mi_stats_get_json(size_t, char*); | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| #include <JavaScriptCore/ControlFlowProfiler.h> | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
|
|
@@ -356,6 +357,14 @@ JSC_DEFINE_HOST_FUNCTION(functionMemoryUsageStatistics, | |||||||||||||||||||||||||||||||
| #endif | ||||||||||||||||||||||||||||||||
| #endif | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| mi_collect(false); | ||||||||||||||||||||||||||||||||
| if (char* json = mi_stats_get_json(0, nullptr)) { | ||||||||||||||||||||||||||||||||
| JSValue parsed = JSONParse(globalObject, String::fromUTF8(json)); | ||||||||||||||||||||||||||||||||
| mi_free(json); | ||||||||||||||||||||||||||||||||
| object->putDirect(vm, Identifier::fromString(vm, "mimalloc"_s), | ||||||||||||||||||||||||||||||||
| parsed.isEmpty() ? jsNull() : parsed); | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
Comment on lines
+361
to
+367
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Always emit the When 🧩 Proposed fix- mi_collect(false);
- if (char* json = mi_stats_get_json(0, nullptr)) {
- JSValue parsed = JSONParse(globalObject, String::fromUTF8(json));
- mi_free(json);
- object->putDirect(vm, Identifier::fromString(vm, "mimalloc"_s),
- parsed.isEmpty() ? jsNull() : parsed);
- }
+ mi_collect(false);
+ JSValue mimallocStats = jsNull();
+ if (char* json = mi_stats_get_json(0, nullptr)) {
+ JSValue parsed = JSONParse(globalObject, String::fromUTF8(json));
+ mi_free(json);
+ mimallocStats = parsed.isEmpty() ? jsNull() : parsed;
+ }
+ object->putDirect(vm, Identifier::fromString(vm, "mimalloc"_s), mimallocStats);📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| return JSValue::encode(object); | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update the earlier macOS decision-matrix comment to match the new behavior.
This block now enables
MI_OVERRIDE/MI_OSX_ZONEon Darwin, but the matrix comment above still says macOS override is OFF. That contradiction is easy to misread during future changes.✏️ Proposed comment-only fix
🤖 Prompt for AI Agents