Conversation
…form() call The Rust port deep-cloned config.macro_map and config.runtime.replace_exports twice per async transform() (once in TransformTask::create, once in run()) and once per sync path, where the Zig implementation shared the hash-map storage by shallow struct copy (kept alive via the transpiler refcount). ParseOptions.macro_remappings is never read by Transpiler::parse (macro remapping goes through MacroContext.remap, a BackRef into transpiler.options.macro_remap set once at construction), so pass an empty default. For replace_exports, store the owned map on Config directly and thread a borrow through ParseOptions into parser Features via BackRef (same pattern as runtime_transpiler_cache), letting all parser read sites work unchanged via Deref. The TransformTask stores the borrow alongside its IntrusiveRc handle so the backing storage outlives the off-thread parse.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (3)
Walkthrough
ChangesReplaceableExportMap borrow-not-clone refactor
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Updated 8:08 PM PT - Jun 15th, 2026
✅ @robobun, your commit c9cfbc8bb330ac71473a78ef54e2c8447f8d3a6c passed in 🧪 To try this PR locally: bunx bun-pr 32375That installs a local version of the PR into your bun-32375 --bun |
There was a problem hiding this comment.
I didn't find correctness bugs, but this swaps owned maps for cross-thread BackRef borrows kept alive by IntrusiveRc<JSTranspiler>, and CI build-rust is currently failing on 5 platforms — worth a human look before merge.
Extended reasoning...
Overview
This PR eliminates per-call deep clones of macro_map and replace_exports in Bun.Transpiler#transform()/transformSync(). It does so by:
- Adding
ReplaceableExportMap::empty()(a&'staticLazyLock sentinel) insrc/ast/runtime.rs. - Changing
Runtime::Features.replace_exportsfrom an owned map tobun_ptr::BackRef<ReplaceableExportMap>(rawNonNull<T>wrapper) insrc/js_parser/parser.rs. - Moving owned storage to a new
Config.replace_exportsfield inJSTranspiler, and havingTransformTaskhold&'a ReplaceableExportMapborrowed from it (kept alive by the adjacentIntrusiveRc<JSTranspiler>). - Changing
ParseOptions.replace_exportsto&'b ReplaceableExportMapand updating all construction sites (transpiler.rs,RuntimeTranspilerStore.rs,jsc_hooks.rs,parse_entry.rs). - Passing
MacroMap::default()for the deadmacro_remappingsfield in theJSTranspilerpaths. - Adding a 64-way concurrent async test with a 2001-entry eliminate list.
Nine files touched across ast, bundler, js_parser, jsc, and runtime.
Security risks
None in the traditional sense (no auth/crypto/input-parsing surface). The risk here is memory safety: BackRef<T> is a lifetime-erased NonNull<T> with safe Deref, and the new TransformTask.replace_exports: &'a ReplaceableExportMap is read on a worker thread while the backing Config lives in a JsCell on the JS thread. Soundness rests on two invariants the PR documents but the type system does not enforce: (1) IntrusiveRc<JSTranspiler> keeps Config alive for the task's lifetime, and (2) Config.replace_exports is never mutated after construction. Both look correct from reading the code (the field is only written in Config::from_js during construction, and the visit pass only calls get_ptr/contains), but a UAF or data race here would be a security-relevant defect.
Level of scrutiny
High. This is a non-mechanical ownership refactor in core transpiler/parser infrastructure that introduces raw-pointer-backed cross-thread borrows. The reasoning is well-documented and the added concurrency test is good coverage, but the change shape (owned → BackRef, sync → async borrow) is exactly the kind that benefits from a second pair of eyes on the lifetime argument.
Other factors
- CI is red: robobun reports build-rust failures on 5 platforms (FreeBSD aarch64, macOS x64/aarch64, Linux x64-android, aarch64-musl) for commit 4d20adc. An autofix commit (c8bc5c9) followed; its build status isn't shown yet.
- I left one inline nit earlier about
build_with_resolve_result_eagerstill cloning into the now-deadmacro_remappingsfield — purely a consistency cleanup, not a blocker. clone_macro_mapinJSTranspiler.rsis now only used at construction time (line ~1066), no longer per-call.- The
Default for Featureschange means everyFeatures::default()now dereferences aLazyLockonce; negligible but worth noting.
…l cost ParseOptions.macro_remappings was never read by Transpiler::parse (macro remapping is resolved via MacroContext.remap, a BackRef into transpiler.options.macro_remap). Drop the field and the deep-clone at every construction site. Add a ratio test: per-call transform() time with a 4000-entry exports.eliminate list must stay within 3x of an empty-list baseline. Without the fix the ratio is 7x-9x.
… path in ratio test The flag's only readers were the macro_remappings clone-gate blocks removed in the previous commit; the remaining self-OR writes are dead bookkeeping. Rework the per-call-cost test to time transformSync() (no work-pool / event loop hops) with base/big measurement windows interleaved across 5 rounds so transient CI load affects both sides of the ratio symmetrically.
There was a problem hiding this comment.
Thanks — c9cfbc8 addresses both remaining nits (write-only has_any_macro_remappings dropped; ratio test moved to transformSync + interleaved). No further findings, but the owned→BackRef change to Features.replace_exports and the cross-thread borrow held by TransformTask's IntrusiveRc are subtle enough that a human should sign off.
Extended reasoning...
Overview
This PR eliminates per-call deep-cloning of the exports.{replace,eliminate} and macro maps in Bun.Transpiler#transform()/#transformSync(). It does so by (a) moving owned storage from Runtime::Features.replace_exports to a new Config.replace_exports field, (b) changing Features.replace_exports to BackRef<ReplaceableExportMap> and ParseOptions.replace_exports to &'b ReplaceableExportMap, (c) having TransformTask borrow the map (kept alive by its IntrusiveRc<JSTranspiler>) instead of cloning, and (d) deleting the now-dead ParseOptions.macro_remappings field and VirtualMachine.has_any_macro_remappings flag plus their five clone/write sites. A ReplaceableExportMap::empty() static is added for the default case. Two tests are added: a 64-way concurrent async correctness check and an interleaved transformSync timing-ratio regression guard.
Security risks
None identified. The change is internal memory-ownership refactoring in the transpiler hot path; no auth, crypto, network, or user-input parsing surface is touched. The BackRef is read-only and the IntrusiveRc keeps the pointee alive for the task's lifetime, so there is no new UAF surface introduced by design — but that is exactly the invariant a human should confirm.
Level of scrutiny
Medium-high. This is a correctness-sensitive ownership refactor, not a config tweak: Features.replace_exports changes type across the parser/visit pass (read at ~15 sites in visit_stmt.rs/visit/mod.rs), and TransformTask now carries a &'a borrow across a work-pool thread hop whose validity rests on the js_instance: IntrusiveRc field's drop ordering. The pattern matches the existing runtime_transpiler_cache: *mut precedent in Features and the comments document the invariant clearly, but a maintainer familiar with the BackRef/IntrusiveRc conventions should confirm.
Other factors
All three of my prior inline comments were addressed (5b0bdda dropped ParseOptions.macro_remappings; c9cfbc8 dropped has_any_macro_remappings and reworked the ratio test to transformSync + 5-round interleave). The bug-hunting system found nothing on the latest revision. The remaining timing-ratio test is much improved (no work-pool hops, symmetric noise exposure, ~7.5× vs 3× threshold separation per the author) but is still a wall-clock assertion — acceptable given the margin, but worth a maintainer's awareness. CI build #62746 is in progress.
|
Stale PR review: keep open, rework. The mechanism in this diff no longer applies to main. The part that is wanted is smaller and should wait for #40383/#40405 to land: delete the write-only |
|
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. |
What
Bun.Transpiler#transform()/#transformSync()deep-cloned the configuredmacroandexports.{replace,eliminate}hash maps on every call. The Zig implementation shared the backing storage by shallow struct copy (kept alive bytranspiler.ref()), so per-call cost was flat regardless of map size.Repro
Debug+ASAN, per-call overhead vs
ELIM=0baseline:Cause
TransformTask::createcalledclone_macro_map(&config.macro_map)andconfig.runtime.replace_exports.entries.clone(), thenrun()cloned both again intoParseOptions: four deep clones per async call, two per sync call.ParseOptions.macro_remappingsis never read byTranspiler::parse(macro remapping is resolved viaMacroContext.remap, aBackRefintotranspiler.options.macro_remappopulated once at construction), so the macro-map clones were pure waste.Fix
TransformTask.macro_map; passMacroMap::default()toParseOptionsin both paths.ReplaceableExportMapfromConfig.runtime(aRuntime::Featuresvalue) to a dedicatedConfig.replace_exportsfield.Runtime::Features.replace_exportsfrom an ownedReplaceableExportMaptobun_ptr::BackRef<ReplaceableExportMap>(same approach asruntime_transpiler_cache's raw*mut, keepingFeatureslifetime-free). All parser read sites work unchanged viaDeref.ParseOptions.replace_exportsbecomes&'b ReplaceableExportMap;TransformTaskstores the borrow next to itsIntrusiveRc<JSTranspiler>which keeps the backingConfiglive for the task's lifetime.ReplaceableExportMap::empty()returning a&'staticshared empty map for the default case.Test
Added a concurrent async
transform()test with a 2001-entryeliminatelist that cross-checks every result againsttransformSync, covering the cross-thread borrow lifetime.