Skip to content

Bun.Transpiler: don't mi_heap_new() per instance - #32384

Closed
robobun wants to merge 4 commits into
mainfrom
farm/42074c06/transpiler-per-instance-heap
Closed

robobun wants to merge 4 commits into
mainfrom
farm/42074c06/transpiler-per-instance-heap

Conversation

@robobun

@robobun robobun commented Jun 16, 2026 •

Copy link
Copy Markdown
Collaborator

What

JSTranspiler::constructor was building a dedicated MimallocArena (mi_heap_new()) per new Bun.Transpiler() and handing it to Transpiler::init. The reference implementation passes bun.default_allocator to the inner transpiler and only uses a thin bump arena for config parsing (JSTranspiler.zig:690-695).

Switched to Arena::borrowing_default() so the inner transpiler's resting-state allocator is the process-global heap, matching the VM transpiler in jsc_hooks.rs. Every parse operation (scan / transformSync / scanImports / async transform) already swaps in a fresh per-call Arena::new() via TranspilerStateGuard, so the stored arena is only the resting-state handle plus configure_defines' bump (which the reference also routes through bun.default_allocator).

Since borrowing_default()'s Drop is a no-op, exports.replace string values can no longer rely on arena bulk-free. They are now stored as Box<[u8]> in Config.replace_exports_bufs and drop with the instance, taking the place of the per-instance bump arena the reference uses for config.fromJS. The arena parameter on Config::from_js is removed.

Repro

const { heapStats } = require("bun:jsc");
const before = heapStats({ dump: true }).mimallocDump.heaps.length;
const a = [];
for (let i = 0; i < 100; i++) a.push(new Bun.Transpiler({ loader: "ts" }));
const after = heapStats({ dump: true }).mimallocDump.heaps.length;
console.log("delta:", after - before);  // before: 100, after: 0

Numbers (release, 5000 held instances)

live mi_heaps RSS retained
before +5000 ~232 MB (46.5 KB/inst)
after +0 ~205 MB (41.0 KB/inst)

The remaining per-instance retention is the inner Transpiler/BundleOptions/Resolver/Define structs themselves and is unchanged by the allocator choice; this PR only removes the extra mi_heap_t per instance so the allocator matches the reference semantics.

Verification

  • bun bd test test/js/bun/transpiler/ (47 pass)
  • bun bd test test/bundler/transpiler/transpiler.test.js (170 pass, 21 todo)
  • bun bd test test/js/bun/jsc/heapStats-mimalloc.test.ts (pass)
  • New transpiler-arena-heap.test.ts fails on the unfixed build (64 new heaps for 64 instances) and passes with the fix (0 new heaps)

The JSTranspiler constructor was creating a dedicated MimallocArena
(mi_heap_new()) per instance and passing it to the inner Transpiler.
The reference implementation passes bun.default_allocator instead and
only uses a thin bump arena for config parsing.

Use Arena::borrowing_default() (wraps mi_heap_main()) so the inner
transpiler allocates from the process-global heap, matching the VM
transpiler in jsc_hooks.rs. Every parse operation already swaps in a
fresh per-call Arena::new() via TranspilerStateGuard, so the stored
arena is only the resting-state handle plus configure_defines' bump.

5000 new Bun.Transpiler({loader:'ts'}) instances in release:
  before: 232 MB retained, 5000 live mi_heaps
  after:  205 MB retained, 0 new mi_heaps
@robobun

robobun commented Jun 16, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 4:23 AM PT - Jun 16th, 2026

✅ @robobun, your commit f08a2d32069f504fd327c5c0f0616869b456ce8e passed in Build #62784! 🎉


🧪   To try this PR locally:

bunx bun-pr 32384

That installs a local version of the PR into your bun-32384 executable, so you can run:

bun-32384 --bun

@coderabbitai

coderabbitai Bot commented Jun 16, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 97ed92b7-b7ee-4517-b2f0-f445440a6f3f

📥 Commits

Reviewing files that changed from the base of the PR and between d22b2d0 and f08a2d3.

📒 Files selected for processing (2)
  • src/runtime/api/JSTranspiler.rs
  • test/js/bun/transpiler/transpiler-arena-heap.test.ts

Walkthrough

JSTranspiler now uses Arena::borrowing_default() instead of Arena::new() for its long-lived backing arena, preventing per-instance mimalloc heap allocation. Config takes ownership of replace_exports_bufs to store replacement-export string bytes for the lifetime of the instance. Config::from_js signature removes the arena parameter, and export_replacement_value is updated to accept the owned buffer instead. A new test validates that heap-count growth remains bounded across 64 Transpiler constructions.

Changes

JSTranspiler arena allocator fix and heap-count test

Layer / File(s) Summary
Config data ownership model for replacement exports
src/runtime/api/JSTranspiler.rs
Config::replace_exports_bufs field documentation is updated to reflect persistent storage of replacement-export string bytes. Config::default() initializes the vector. Config::from_js signature removes the arena: &Arena parameter. export_replacement_value signature changes from arena: &Arena to bufs: &mut Vec<Box<[u8]>>. String-handling implementation boxes generated bytes, pushes them into the buffer, and returns an EString referencing the stored slice.
Export parsing call sites using replace_exports_bufs
src/runtime/api/JSTranspiler.rs
Two call sites during exports.replace parsing are updated to pass &mut self.replace_exports_bufs to export_replacement_value: one for standard replacement values and one for injection-style object/array replacements.
Arena::borrowing_default() in JSTranspiler constructor
src/runtime/api/JSTranspiler.rs
bun_alloc import is cleaned up (comment removed, standalone use added). JSTranspiler::arena field documentation describes the new resting-state arena behavior and per-call arena swapping. Constructor switches from Arena::new() to Arena::borrowing_default() with inline comments explaining the allocator model and heap behavior. The config.from_js() call removes the arena argument.
Mimalloc heap-count regression test
test/js/bun/transpiler/transpiler-arena-heap.test.ts
Adds a test that warms up shared state, snapshots mimalloc heap count via heapStats({ dump: true }), constructs 64 Bun.Transpiler instances, asserts the heap-count delta is well below a per-instance threshold, and calls transformSync on the first and last instances to verify functionality and output correctness.

Suggested reviewers

  • cirospaciari
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Bun.Transpiler: don't mi_heap_new() per instance' accurately reflects the main change: eliminating per-instance heap allocations by switching from mi_heap_new() to Arena::borrowing_default().
Description check ✅ Passed The PR description comprehensively covers both required template sections with detailed explanation of the change and thorough verification steps including test results and performance metrics.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

Comment thread src/runtime/api/JSTranspiler.rs
With the resting-state arena now wrapping mi_heap_main(), its Drop is a
no-op, so arena.alloc_slice_copy() for exports.replace string values
would strand those bytes for the process lifetime. The reference
implementation frees them via the per-instance bump arena on finalize.

Store each string value as a Box<[u8]> in Config.replace_exports_bufs
instead and borrow from there; they drop with the JSTranspiler. Drops
the arena parameter from Config::from_js since nothing else used it,
and updates the stale arena field comment.
Comment thread src/runtime/api/JSTranspiler.rs
Comment thread test/js/bun/transpiler/transpiler-arena-heap.test.ts Outdated
The configure_defines bump is intentionally routed through the
borrowing_default arena to match the reference
(Transpiler.Transpiler.init(bun.default_allocator, ...)) and the VM
transpiler in jsc_hooks.rs. Auto-injected defaults hit the
const_default_define_value fast path and never touch the arena.

Drop the toEqual block whose sub-checks are tautological; the
toBeLessThan assertion carries the delta in its failure output.

@claude claude Bot left a comment

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.

Thanks — 4eaecf8 addresses both remaining comments (vacuous toEqual dropped, configure_defines trade-off now explicitly documented as reference-matching). No further issues from me, but deferring to a maintainer to sign off on the allocator-lifetime change and the deliberate define: retention trade-off.

Extended reasoning...

Overview

This PR changes JSTranspiler::constructor from Arena::new() (one mi_heap_new() per Bun.Transpiler instance) to Arena::borrowing_default() (shared process heap), matching the Zig reference's Transpiler.init(bun.default_allocator, …). To compensate for losing the per-instance bulk-free, exports.replace string backing storage moves to a new Config.replace_exports_bufs: Vec<Box<[u8]>> field that drops with the instance; the arena parameter on Config::from_js is removed. A new regression test asserts heap count doesn't grow per instance. Two follow-up commits (db1d93f, 4eaecf8) addressed all three legs of my earlier reviews: the exports.replace leak is fixed via owned boxes, the stale field comment is rewritten, the vacuous toEqual is deleted, and the configure_defines slow-path retention is now explicitly documented in-code as the reference behavior (option (b) from my suggested fix).

Security risks

None. This is an allocator/lifetime refactor with no auth, input-parsing, or boundary-crossing changes. The 'static lifetime erasure for E::EString data is pre-existing pattern; the new Box<[u8]> backing is address-stable across Vec reallocation and outlives the borrowing Exprs by field declaration order.

Level of scrutiny

Moderate-to-high. This touches allocator semantics for a public runtime API (Bun.Transpiler), with raw-pointer lifetime laundering (detach_lifetime_ref), drop-order-dependent borrows, and a deliberate acceptance of a user-controllable retention path (define: slow-path values living for process lifetime) on the grounds that the Zig reference has the same behavior. The reasoning is sound and well-documented, but it's the kind of trade-off a maintainer should explicitly OK rather than a bot.

Other factors

CI reported failures on db1d93f (the build comment hasn't updated for 4eaecf8 yet). The transpiler test suites are reported passing locally in the PR description, and the new test correctly fails on the unfixed build. All prior review feedback has been addressed; nothing outstanding from my side.

@robobun

robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

Heads up for whoever rebases this: #38541 changes export_replacement_value to allocate the E::EString node itself in the instance arena (Expr::allocate) rather than in the thread-local AST store, because the store is reset by the next main-thread parse and the node is kept for the life of the transpiler. With borrowing_default() here, the node would need an owned home alongside the bytes in replace_exports_bufs (for example a Box<E::EString> kept in Config), since Expr::init puts it in the store and a plain arena.alloc on the main heap would never be freed.

@robobun

robobun commented Sep 13, 2026

Copy link
Copy Markdown
Collaborator Author

Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-06-16 and it conflicts with main. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main.

@robobun robobun closed this Sep 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant