-
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 6 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 = "9a5e1f52cdf4662f9590b69de104a4469140796f"; | ||
|
|
||
| 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,8 @@ | |||||||||||||||||||||||||||||||
| #include "JSDOMConvertBase.h" | ||||||||||||||||||||||||||||||||
| #include "ZigSourceProvider.h" | ||||||||||||||||||||||||||||||||
| #include "mimalloc.h" | ||||||||||||||||||||||||||||||||
| extern "C" char* mi_stats_get_json(size_t, char*); | ||||||||||||||||||||||||||||||||
| extern "C" char* mi_heap_dump_json(bool include_blocks, bool hash_addresses); | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| #include <JavaScriptCore/ControlFlowProfiler.h> | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
|
|
@@ -219,7 +221,7 @@ | |||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| JSC_DECLARE_HOST_FUNCTION(functionMemoryUsageStatistics); | ||||||||||||||||||||||||||||||||
| JSC_DEFINE_HOST_FUNCTION(functionMemoryUsageStatistics, | ||||||||||||||||||||||||||||||||
| (JSGlobalObject * globalObject, CallFrame*)) | ||||||||||||||||||||||||||||||||
| (JSGlobalObject * globalObject, CallFrame* callFrame)) | ||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| auto& vm = JSC::getVM(globalObject); | ||||||||||||||||||||||||||||||||
|
|
@@ -353,9 +355,37 @@ | |||||||||||||||||||||||||||||||
| object->putDirect(vm, Identifier::fromString(vm, "zones"_s), | ||||||||||||||||||||||||||||||||
| zoneSizesObject); | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| #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 |
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| // heapStats({ dump: true | "blocks" }) -> per-heap/per-page (and per-block) live snapshot. | ||||||||||||||||||||||||||||||||
| JSValue arg0 = callFrame->argument(0); | ||||||||||||||||||||||||||||||||
| if (arg0.isObject()) { | ||||||||||||||||||||||||||||||||
| JSValue dump = arg0.getObject()->get(globalObject, Identifier::fromString(vm, "dump"_s)); | ||||||||||||||||||||||||||||||||
| if (dump.toBoolean(globalObject)) { | ||||||||||||||||||||||||||||||||
| const bool includeBlocks = dump.isString() && dump.toWTFString(globalObject) == "blocks"_s; | ||||||||||||||||||||||||||||||||
| #if BUN_DEBUG | ||||||||||||||||||||||||||||||||
| const bool hashAddresses = false; | ||||||||||||||||||||||||||||||||
| #else | ||||||||||||||||||||||||||||||||
| const bool hashAddresses = true; | ||||||||||||||||||||||||||||||||
| #endif | ||||||||||||||||||||||||||||||||
| if (char* json = mi_heap_dump_json(includeBlocks, hashAddresses)) { | ||||||||||||||||||||||||||||||||
| JSValue parsed = JSONParse(globalObject, String::fromUTF8(json)); | ||||||||||||||||||||||||||||||||
| mi_free(json); | ||||||||||||||||||||||||||||||||
| object->putDirect(vm, Identifier::fromString(vm, "mimallocDump"_s), | ||||||||||||||||||||||||||||||||
| parsed.isEmpty() ? jsNull() : parsed); | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
Check warning on line 387 in src/bun.js/modules/BunJSCModule.h
|
||||||||||||||||||||||||||||||||
|
Comment on lines
+369
to
+387
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
# Verify that functionMemoryUsageStatistics performs JS-visible operations
# without a local throw scope / exception checks.
rg -n -C3 'functionMemoryUsageStatistics|DECLARE_THROW_SCOPE|JSONParse\(|->get\(globalObject, Identifier::fromString' src/bun.js/modules/BunJSCModule.hRepository: oven-sh/bun Length of output: 8839 🏁 Script executed: sed -n '223,395p' src/bun.js/modules/BunJSCModule.h | cat -nRepository: oven-sh/bun Length of output: 8469 Add throw scope and exception checking to This function performs multiple JS-visible operations without exception handling: Proposed fix JSC_DEFINE_HOST_FUNCTION(functionMemoryUsageStatistics,
(JSGlobalObject * globalObject, CallFrame* callFrame))
{
auto& vm = JSC::getVM(globalObject);
+ auto scope = DECLARE_THROW_SCOPE(vm);
if (vm.heap.size() == 0) {
vm.heap.collectNow(Sync, CollectionScope::Full);
}
@@
mi_collect(false);
if (char* json = mi_stats_get_json(0, nullptr)) {
JSValue parsed = JSONParse(globalObject, String::fromUTF8(json));
mi_free(json);
+ RETURN_IF_EXCEPTION(scope, {});
object->putDirect(vm, Identifier::fromString(vm, "mimalloc"_s),
parsed.isEmpty() ? jsNull() : parsed);
}
// heapStats({ dump: true | "blocks" }) -> per-heap/per-page (and per-block) live snapshot.
JSValue arg0 = callFrame->argument(0);
if (arg0.isObject()) {
JSValue dump = arg0.getObject()->get(globalObject, Identifier::fromString(vm, "dump"_s));
+ RETURN_IF_EXCEPTION(scope, {});
if (dump.toBoolean(globalObject)) {
+ RETURN_IF_EXCEPTION(scope, {});
const bool includeBlocks = dump.isString() && dump.toWTFString(globalObject) == "blocks"_s;
+ RETURN_IF_EXCEPTION(scope, {});
`#if` BUN_DEBUG
const bool hashAddresses = false;
`#else`
const bool hashAddresses = true;
`#endif`
if (char* json = mi_heap_dump_json(includeBlocks, hashAddresses)) {
JSValue parsed = JSONParse(globalObject, String::fromUTF8(json));
mi_free(json);
+ RETURN_IF_EXCEPTION(scope, {});
object->putDirect(vm, Identifier::fromString(vm, "mimallocDump"_s),
parsed.isEmpty() ? jsNull() : parsed);
}
}
}
- return JSValue::encode(object);
+ RELEASE_AND_RETURN(scope, JSValue::encode(object));
}
Comment on lines
358
to
+387
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 heapStats dump section in Extended reasoning...Bug 1 — Missing null fallback for mimalloc property The mimalloc property is only set on the heapStats() result when mi_stats_get_json(0, nullptr) returns non-null. If it returns null (OOM allocating the internal JSON buffer), the key is simply absent from the returned object. Callers relying on a stable shape — including the new test at heapStats-mimalloc.test.ts:7 which accesses s.mimalloc.mimalloc_version — would receive a TypeError when s.mimalloc is undefined. The fix is trivial: hoist a JSValue mimallocStats = jsNull() before the if-block and move the putDirect call outside it, unconditionally emitting the property. Bug 2 — Missing exception propagation in the dump section Before this PR, functionMemoryUsageStatistics accepted an unnamed CallFrame* argument and performed zero JS operations on user input. This PR adds a new code path that reads callFrame->argument(0): it calls arg0.getObject()->get(globalObject, ...) and then dump.toBoolean(globalObject). Both operations take a JSGlobalObject* and can throw JS exceptions — get() via a Proxy trap or throwing getter, toBoolean() via Symbol.toPrimitive or valueOf(). The function has no DECLARE_THROW_SCOPE anywhere. Every other JS-calling host function in BunJSCModule.h (lines 79, 130, 446, 481, etc.) declares one. Neither call is followed by RETURN_IF_EXCEPTION. If an exception is raised by get(), execution falls through to dump.toBoolean() then into the native mi_heap_dump_json C function (which runs while a JS exception is pending), then JSONParse, then mi_free, then putDirect, and finally returns JSValue::encode(object) as if nothing went wrong. The pending exception is silently swallowed or overwritten. Why existing code does not prevent this JSC contract: any host function passing a JSGlobalObject* to a throwing operation must own a throw scope and check for exceptions before proceeding. Without a throw scope there is no compile-time enforcement. All other functions in the file follow this pattern; the new code does not. Proof for Bug 2
Impact Bug 1: Consumers see undefined for mimalloc instead of null when the stats buffer allocation fails. Extremely unlikely in practice but the API shape is unstable. The new test would also fail with a confusing TypeError rather than a clear test assertion failure. Bug 2: Passing a Proxy with a throwing dump getter to heapStats() silently swallows the exception and returns a result object. This is an internal debug API so real-world impact is low, but the JSC invariant violation is real. Fix For Bug 1: move putDirect for mimalloc outside the null check, always emitting the property (jsNull() when mi_stats_get_json fails). |
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| return JSValue::encode(object); | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| import { heapStats } from "bun:jsc"; | ||
| import { describe, expect, test } from "bun:test"; | ||
|
|
||
| describe("heapStats() mimalloc integration", () => { | ||
| test("mimalloc aggregate stats are present", () => { | ||
| const s = heapStats(); | ||
| expect(s.mimalloc).toBeDefined(); | ||
| expect(s.mimalloc.mimalloc_version).toBeGreaterThan(3000); | ||
| expect(s.mimalloc.pages.current).toBeGreaterThan(0); | ||
| expect(s.mimalloc.committed.current).toBeGreaterThan(0); | ||
| expect(Array.isArray(s.mimalloc.malloc_bins)).toBe(true); | ||
| }); | ||
|
|
||
| test("heapStats({dump: true}) returns per-heap pages", () => { | ||
| const s = heapStats({ dump: true }); | ||
| expect(s.mimallocDump).toBeDefined(); | ||
| expect(Array.isArray(s.mimallocDump.heaps)).toBe(true); | ||
| expect(s.mimallocDump.heaps.length).toBeGreaterThan(0); | ||
| const main = s.mimallocDump.heaps.find((h: any) => h.seq === 0); | ||
| expect(main).toBeDefined(); | ||
| expect(Array.isArray(main.pages)).toBe(true); | ||
| expect(main.pages.length).toBeGreaterThan(0); | ||
| const page = main.pages[0]; | ||
| expect(typeof page.id).toBe("number"); | ||
| expect(page.block_size).toBeGreaterThan(0); | ||
| expect(page.used).toBeGreaterThanOrEqual(0); | ||
| expect(page.reserved).toBeGreaterThan(0); | ||
| expect(typeof page.thread_id).toBe("number"); | ||
| // pages-only mode: no blocks | ||
| expect(main.blocks).toBeUndefined(); | ||
| }); | ||
|
|
||
| test("heapStats({dump: 'blocks'}) includes per-block ids", () => { | ||
| const s = heapStats({ dump: "blocks" }); | ||
| const main = s.mimallocDump.heaps.find((h: any) => h.seq === 0); | ||
| expect(Array.isArray(main.blocks)).toBe(true); | ||
| expect(main.blocks.length).toBeGreaterThan(0); | ||
| const [id, size] = main.blocks[0]; | ||
| expect(typeof id).toBe("number"); | ||
| expect(size).toBeGreaterThan(0); | ||
| // every block size should match some page's block_size | ||
| const pageSizes = new Set(main.pages.map((p: any) => p.block_size)); | ||
| for (const [, sz] of main.blocks.slice(0, 50)) { | ||
| expect(pageSizes.has(sz)).toBe(true); | ||
|
Check warning on line 44 in test/js/bun/jsc/heapStats-mimalloc.test.ts
|
||
|
Comment on lines
+39
to
+44
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 'heapStats({dump: "blocks"})' test accesses Extended reasoning...What the bug is and how it manifests In const main = s.mimallocDump.heaps.find((h: any) => h.seq === 0);
expect(main).toBeDefined(); // guard present
expect(Array.isArray(main.pages)).toBe(true);The 'dump: "blocks"' test (lines 35–36) skips the guard entirely: const main = s.mimallocDump.heaps.find((h: any) => h.seq === 0);
// no expect(main).toBeDefined() here
expect(Array.isArray(main.blocks)).toBe(true); // will throw TypeError if main is undefinedThe specific code path that triggers it
Why existing code doesn't prevent it The 'dump: true' test deliberately adds What the impact would be In practice, the process-wide main heap ( How to fix it Add the missing guard at line 37 (between the const main = s.mimallocDump.heaps.find((h: any) => h.seq === 0);
expect(main).toBeDefined(); // add this line
expect(Array.isArray(main.blocks)).toBe(true);Step-by-step proof
|
||
| } | ||
| }); | ||
|
|
||
| test("dump reflects new heaps and allocations", () => { | ||
| const before = heapStats({ dump: true }).mimallocDump.heaps.length; | ||
| // MimallocArena is internal; trigger via something that creates a heap. | ||
| // Transpiler creates a per-call arena. | ||
| const t = new Bun.Transpiler(); | ||
| const out = t.transformSync("export const x = 1"); | ||
| expect(out.length).toBeGreaterThan(0); | ||
| const after = heapStats({ dump: true }).mimallocDump.heaps; | ||
| // Either a new heap was created (and may already be destroyed), or main grew. | ||
| // We assert the dump is still well-formed and >= before. | ||
| expect(after.length).toBeGreaterThanOrEqual(1); | ||
| for (const h of after) { | ||
| expect(typeof h.seq).toBe("number"); | ||
| expect(Array.isArray(h.pages)).toBe(true); | ||
| } | ||
| void before; | ||
| }); | ||
|
Comment on lines
+48
to
+64
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. This test never checks the before/after signal it describes.
🤖 Prompt for AI Agents
Comment on lines
+48
to
+64
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 test "dump reflects new heaps and allocations" records a baseline heap count in Extended reasoning...What the bug is and how it manifests The test "dump reflects new heaps and allocations" (lines 48–64) captures The specific code path that triggers it Line 49 sets Why existing code does not prevent it There is no mechanism that enforces a captured baseline must appear in an assertion. The test compiles and runs, and will pass as long as the dump returns any heap at all — even if allocations somehow caused the heap count to drop from 50 to 1. What the impact would be If a mimalloc regression caused heaps to stop being tracked after allocation (e.g., a refcount bug that destroys heaps prematurely), the heap count in the dump could shrink. The test would still pass as long as one heap remained. The invariant the test claims to enforce — that allocations produce a dump at least as large as the pre-allocation snapshot — is never actually checked. How to fix it Change the assertion on line 58 from Step-by-step proof
|
||
| }); | ||
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