Skip to content

bun_alloc: delete unused MimallocHeapRef, TypedArena, BumpWithFallback #31542

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.

Sign up for GitHub

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

Merged
dylan-conway merged 2 commits into from
May 29, 2026
Merged

bun_alloc: delete unused MimallocHeapRef, TypedArena, BumpWithFallback #31542

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7c5c330
bun_alloc: delete unused MimallocHeapRef, TypedArena, BumpWithFallbac…
dylan-conway May 29, 2026
3b1c8cc
review: drop leftover init_default doc-comment lines
dylan-conway May 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions src/bun_alloc/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,11 +323,9 @@
let mut __s = $crate::ArenaString::new_in($arena);
::core::fmt::Write::write_fmt(&mut __s, ::core::format_args!($($arg)*))
.expect("ArenaString::write_fmt is infallible");
__s
}};

Check warning on line 327 in src/bun_alloc/lib.rs

View check run for this annotation

Claude / Claude Code Review

typed-arena dependency now unused in bun_alloc 

Removing the `TypedArena<T>` alias drops the only reference to the `typed_arena` crate inside `bun_alloc`, but `src/bun_alloc/Cargo.toml:30` still declares `typed-arena.workspace = true`. Since this PR is a dead-code sweep, drop the now-unused crate-level dependency too (the workspace root entry stays — `ast`, `js_parser`, `css`, etc. depend on it directly).
Comment on lines 326 to 327

@claude claude Bot May 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Removing the TypedArena<T> alias drops the only reference to the typed_arena crate inside bun_alloc, but src/bun_alloc/Cargo.toml:30 still declares typed-arena.workspace = true. Since this PR is a dead-code sweep, drop the now-unused crate-level dependency too (the workspace root entry stays — ast, js_parser, css, etc. depend on it directly).

Extended reasoning...

What the issue is

The PR deletes pub type TypedArena<T> = typed_arena::Arena<T>; from src/bun_alloc/lib.rs. That line was the only reference to the typed_arena crate anywhere under src/bun_alloc/. However, the crate manifest still lists it:

# src/bun_alloc/Cargo.toml:30
typed-arena.workspace = true

After this PR, that dependency entry is dead weight in bun_alloc's Cargo.toml.

Step-by-step proof

  1. Before the PR, rg -i 'typed.arena' src/bun_alloc/ returned two hits: the Cargo.toml dependency line and the pub type TypedArena<T> = typed_arena::Arena<T>; re-export in lib.rs.
  2. This PR's diff removes the lib.rs line (the only .rs reference).
  3. After the PR, the same grep returns only src/bun_alloc/Cargo.toml:30:typed-arena.workspace = true — verified against the checked-out PR head. No use typed_arena, no path references, no extern crate, nothing in lib.rs or any submodule.
  4. Therefore typed-arena is compiled into bun_alloc's dependency graph but never used by it.

Why nothing catches it

cargo check --workspace (which the PR description cites as passing) does not warn on unused [dependencies] entries — that's the domain of cargo-udeps / cargo-machete, neither of which runs in this CI. So the leftover entry silently survives the build.

Also note: typed-arena remains a legitimate workspace dependency — ast, js_parser, css, and others list it in their own Cargo.tomls and use typed_arena::Arena directly (not via the removed bun_alloc::TypedArena alias, which the PR description confirms was "never referenced"). So only the bun_alloc crate-level entry should go; the [workspace.dependencies] root entry stays.

Impact

None at runtime. The only cost is a spurious edge in bun_alloc's build graph and a stale manifest line that contradicts the PR's stated goal ("dead-code sweep of bun_alloc. None referenced outside their own definitions"). Flagging it because incomplete cleanup in a cleanup PR is exactly the kind of thing that's easier to fix now than to rediscover later.

Fix

Delete line 30 from src/bun_alloc/Cargo.toml:

-typed-arena.workspace = true

}
/// `typed_arena::Arena<T>` — typed slab with stable addresses (AST node Store).
pub type TypedArena<T> = typed_arena::Arena<T>;

/// `bun.use_mimalloc` — false under ASAN, where the global allocator is `std::alloc::System`.
pub const USE_MIMALLOC: bool = cfg!(not(bun_asan));
Expand Down Expand Up @@ -526,7 +524,7 @@
pub use max_heap_allocator::MaxHeapAllocator;
pub use maybe_owned::MaybeOwned;
pub use nullable_allocator::NullableAllocator;
pub use stack_fallback::{ArenaPtr, BumpWithFallback, MimallocHeapRef, StackFallback};
pub use stack_fallback::{ArenaPtr, StackFallback};

#[path = "MimallocArena.rs"]
pub mod mimalloc_arena;
Expand Down
6 changes: 0 additions & 6 deletions src/bun_alloc/maybe_owned.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,15 +69,9 @@
self._parent.as_ref()
}

pub fn into_parent(self) -> Option<A> {
// Zig: `defer self.* = undefined; return self.rawParent();`
// Taking `self` by value consumes it; no explicit invalidation needed.
self._parent
}

/// Used by smart pointer types and allocator wrappers. See `crate::borrow`.
pub fn borrow(&self) -> MaybeOwnedBorrowed {
// Borrowed view carries no allocator state — just the owned/borrowed bit.

Check warning on line 74 in src/bun_alloc/maybe_owned.rs

View check run for this annotation

Claude / Claude Code Review

Stale comment references deleted into_parent() 

The trailing comment block at the bottom of this file still says "keeping one would also forbid moving `self._parent` out in `into_parent(self)`" — but `into_parent` is deleted in this PR. Drop that clause; the preceding sentence about field drop glue stands on its own. Same class of leftover as the `init_default` doc-comment fixed in 3b1c8cc3.
Comment on lines 72 to 74

@claude claude Bot May 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 The trailing comment block at the bottom of this file still says "keeping one would also forbid moving self._parent out in into_parent(self)" — but into_parent is deleted in this PR. Drop that clause; the preceding sentence about field drop glue stands on its own. Same class of leftover as the init_default doc-comment fixed in 3b1c8cc.

Extended reasoning...

What the issue is

This PR deletes MaybeOwned::into_parent(self) -> Option<A> from src/bun_alloc/maybe_owned.rs. However, the rationale comment at the bottom of the file (post-diff lines 81-84) still references it:

Per PORTING.md (Idiom map: pub fn deinit), that is exactly field drop glue on _parent: Option<A>, so no explicit Drop impl — keeping one would also forbid moving self._parent out in into_parent(self).

The bolded clause cites into_parent(self) as a second reason for omitting an explicit Drop impl. With into_parent gone, that reason no longer exists in the codebase, so the comment now points readers at a method they cannot find.

Step-by-step proof

  1. Pre-PR, maybe_owned.rs defined pub fn into_parent(self) -> Option<A> { self._parent } at lines 72-76.
  2. The diff for this PR shows that block removed (the - pub fn into_parent(self) -> Option<A> { … hunk).
  3. Post-PR, the file ends with the comment block at lines 81-84, whose final clause reads verbatim: "keeping one would also forbid moving self._parent out in into_parent(self)."
  4. grep -rn into_parent src/ after the PR returns only this comment line — there is no longer any into_parent definition or call site for it to refer to.
  5. Therefore the clause is dangling: it justifies a design choice by appeal to a constraint that has been deleted.

Why nothing catches it

This is a plain // line comment, not a doc comment, so neither cargo check nor cargo doc parses its contents. There is no intra-doc link to break. The PR description's "cargo check --workspace passes" is consistent with this being purely a prose artifact.

Impact

No runtime or compile-time effect. The cost is reader confusion: someone auditing why MaybeOwned has no Drop impl will go looking for into_parent and not find it. Since this PR is explicitly a dead-artifact sweep — and the same class of leftover (the orphaned init_default doc-comment lines in memory.rs) was already cleaned up in follow-up commit 3b1c8cc — it's worth removing for consistency.

Fix

Trim the trailing clause so the comment ends after the first reason, e.g.:

// Zig `deinit` only forwarded to `bun.memory.deinit(parent_alloc)` on the owned field.
// Per PORTING.md (Idiom map: `pub fn deinit`), that is exactly field drop glue on
// `_parent: Option<A>`, so no explicit `Drop` impl.

The field-drop-glue rationale is sufficient on its own; the into_parent clause was always a secondary "and besides" point.

MaybeOwned {
_parent: if self.is_owned() { Some(()) } else { None },
}
Expand Down
12 changes: 0 additions & 12 deletions src/bun_alloc/memory.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,5 @@
//! Basic utilities for working with memory and objects.

/// Default-initializes a value of type `T`.
///
/// Zig tried, in order: `T.initDefault()`, then `T.init()`, then `.{}`. All three
/// collapse into Rust's `Default` trait — types that had `initDefault`/`init` in Zig
/// should `impl Default` in their Rust port.
// PORT NOTE: `std.meta.hasFn` (≈ `@hasDecl`) fallback chain → single `Default` bound
// per §Comptime reflection ("optional behavior → trait with default method").
#[inline]
pub fn init_default<T: Default>() -> T {
T::default()
}

// ──────────────────────────────────────────────────────────────────────────────
// PORT NOTE: `exemptedFromDeinit`, `deinitIsVoid`, and `deinit` are intentionally
// NOT ported as functions.
Expand Down
110 changes: 0 additions & 110 deletions src/bun_alloc/stack_fallback.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,6 @@ pub struct StackFallback<const N: usize, A: Allocator = std::alloc::Global> {
buf: UnsafeCell<[MaybeUninit<u8>; N]>,
}

/// Back-compat alias for the previous name; prefer [`StackFallback`].
pub type BumpWithFallback<const N: usize, A> = StackFallback<N, A>;

impl<const N: usize, A: Allocator> StackFallback<N, A> {
/// Zig: `std.heap.stackFallback(N, fallback)`. `const` — `MaybeUninit<u8>:
/// Copy`, so `[MaybeUninit::uninit(); N]` needs no inline-const;
Expand Down Expand Up @@ -107,13 +104,6 @@ impl<const N: usize, A: Allocator> StackFallback<N, A> {
&self.fallback
}

/// Mutably borrow the fallback allocator (e.g. to rebind a heap pointer
/// after the backing `MimallocArena::reset` swapped it).
#[inline]
pub fn fallback_mut(&mut self) -> &mut A {
&mut self.fallback
}

#[inline(always)]
fn buf_base(&self) -> *mut u8 {
self.buf.get().cast::<u8>()
Expand Down Expand Up @@ -405,106 +395,6 @@ unsafe impl Allocator for ArenaPtr {
}
}

// ── MimallocHeapRef ──────────────────────────────────────────────────────────
//
// Thin `Allocator` over a raw `*mut mi_heap_t`. Unlike [`ArenaPtr`] this
// addresses the C-heap-resident `mi_heap_t` directly (stable across moves of
// the `MimallocArena` wrapper struct), at the cost of bypassing
// `MimallocArena::track_alloc`. Kept for callers that only have a heap handle.
//
// `heap == null` routes to global `mi_malloc`/`mi_free`, matching
// [`crate::ast_alloc::AstAlloc`] when no AST scope is active.

/// Borrowed `mi_heap_t*` as an [`Allocator`]. See section doc above.
#[derive(Clone, Copy)]
pub struct MimallocHeapRef {
heap: *mut mimalloc::Heap,
}

impl MimallocHeapRef {
/// Wrap a live `mi_heap_t*`. The caller guarantees `heap` outlives every
/// allocation made through this ref (i.e. the owning `MimallocArena` is not
/// `reset()`/dropped while this ref is in use).
#[inline]
pub const fn new(heap: *mut mimalloc::Heap) -> Self {
Self { heap }
}
/// Null heap → process-global `mi_malloc`/`mi_free`.
#[inline]
pub const fn global() -> Self {
Self {
heap: ptr::null_mut(),
}
}
/// The wrapped heap pointer (null when global).
#[inline]
pub fn heap(&self) -> *mut mimalloc::Heap {
self.heap
}
/// Rebind to a new heap (e.g. after `MimallocArena::reset` rebuilt it).
#[inline]
pub fn set_heap(&mut self, heap: *mut mimalloc::Heap) {
self.heap = heap;
}
}

// SAFETY: identical contract to `&MimallocArena` / `AstAlloc` —
// `mi_[heap_]malloc[_aligned]` yields ≥`size` bytes aligned to `align`;
// `mi_free` accepts any mimalloc-owned pointer regardless of origin heap;
// `mi_[heap_]realloc_aligned` preserves the `min(old,new)` prefix and frees
// the old block. `heap` must be null or a live `mi_heap_t*` for this ref's
// lifetime (caller contract — see [`MimallocHeapRef::new`]).
unsafe impl Allocator for MimallocHeapRef {
#[inline]
fn allocate(&self, layout: Layout) -> Result<NonNull<[u8]>, AllocError> {
let h = self.heap;
let p = if h.is_null() {
mimalloc::mi_malloc_auto_align(layout.size(), layout.align())
} else {
// SAFETY: `h` is live per the caller contract on `new`.
unsafe { mimalloc::mi_heap_malloc_auto_align(h, layout.size(), layout.align()) }
};
alloc_result(p, layout.size())
}

#[inline]
unsafe fn deallocate(&self, ptr: NonNull<u8>, _layout: Layout) {
// SAFETY: `ptr` came from `mi_[heap_]malloc*` per `allocate`/`grow`;
// `mi_free` is heap-agnostic and thread-safe.
unsafe { mimalloc::mi_free(ptr.as_ptr().cast()) }
}

#[inline]
unsafe fn grow(
&self,
ptr: NonNull<u8>,
_old: Layout,
new: Layout,
) -> Result<NonNull<[u8]>, AllocError> {
let h = self.heap;
// SAFETY: see `allocate`; realloc accepts cross-heap pointers.
let p = unsafe {
if h.is_null() {
mimalloc::mi_realloc_aligned(ptr.as_ptr().cast(), new.size(), new.align())
} else {
mimalloc::mi_heap_realloc_aligned(h, ptr.as_ptr().cast(), new.size(), new.align())
}
};
alloc_result(p, new.size())
}

#[inline]
unsafe fn shrink(
&self,
ptr: NonNull<u8>,
old: Layout,
new: Layout,
) -> Result<NonNull<[u8]>, AllocError> {
// SAFETY: same realloc path as `grow`.
unsafe { self.grow(ptr, old, new) }
}
}

// ported from: vendor/zig/lib/std/heap.zig (stackFallback / StackFallbackAllocator)
// ported from: vendor/zig/lib/std/heap/FixedBufferAllocator.zig (inlined)

Expand Down
Loading