Repository navigation
fix(compile): avoid heap-copying $bunfs module sources - #29319
sosukesuzuki wants to merge 8 commits into
Conversation
|
Updated 9:51 PM PT - Apr 22nd, 2026
❌ @sosukesuzuki, your commit 99d24d3 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 29319That installs a local version of the PR into your bun-29319 --bun |
WalkthroughAdds encoding support to module graph serialization/deserialization in the standalone module graph. JavaScript-like outputs now mark server-side chunks as Latin-1 and client-side chunks as binary. Includes a test validating encoding behavior with non-ASCII text in compiled chunks. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/bundler/compile-client-chunk-encoding.test.ts`:
- Around line 62-65: Remove the unconditional stderr check and instead only
inspect or assert stderr when the subprocess failed: keep the Promise.all call
that sets stdout, stderr, and exitCode and keep the
expect(stdout).toContain("CLIENT_OUTPUT: こんにちは"); then, right before asserting
exitCode, if (exitCode !== 0) include a check or a test failure that
logs/asserts stderr (e.g., fail with stderr content or expect(stderr).toBe("")
inside that conditional) and finally expect(exitCode).toBe(0); refer to the
variables proc, stdout, stderr, and exitCode from the snippet when making this
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c72fd49d-f2ef-44ae-8836-90387819a798
📒 Files selected for processing (2)
src/StandaloneModuleGraph.zigtest/bundler/compile-client-chunk-encoding.test.ts
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| expect(stderr).toBe(""); | ||
| expect(stdout).toContain("CLIENT_OUTPUT: こんにちは"); | ||
| expect(exitCode).toBe(0); |
There was a problem hiding this comment.
Avoid unconditional empty-stderr assertion for runtime subprocesses.
Line 63 can fail in debug ASAN runs due known warning noise on JS-executing subprocesses. Keep stdout as the primary signal, and only dump/assert stderr when exitCode !== 0 right before the exit-code assertion.
Suggested adjustment
- const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
- expect(stderr).toBe("");
- expect(stdout).toContain("CLIENT_OUTPUT: こんにちは");
- expect(exitCode).toBe(0);
+ const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+ expect(stdout).toContain("CLIENT_OUTPUT: こんにちは");
+ if (exitCode !== 0) {
+ expect(stderr).toBe("");
+ }
+ expect(exitCode).toBe(0);Based on learnings, runtime subprocess tests that execute JS may emit ASAN warning lines on stderr in debug builds, and this repo pattern avoids unconditional stderr === "" in that path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/bundler/compile-client-chunk-encoding.test.ts` around lines 62 - 65,
Remove the unconditional stderr check and instead only inspect or assert stderr
when the subprocess failed: keep the Promise.all call that sets stdout, stderr,
and exitCode and keep the expect(stdout).toContain("CLIENT_OUTPUT: こんにちは");
then, right before asserting exitCode, if (exitCode !== 0) include a check or a
test failure that logs/asserts stderr (e.g., fail with stderr content or
expect(stderr).toBe("") inside that conditional) and finally
expect(exitCode).toBe(0); refer to the variables proc, stdout, stderr, and
exitCode from the snippet when making this change.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| .encoding = switch (output_file.loader) { | ||
| .js, .jsx, .ts, .tsx => .latin1, | ||
| // Server-side chunks are printed with target=.bun → ascii_only=true, | ||
| // so their bytes are guaranteed Latin-1 safe and can back a static | ||
| // ExternalStringImpl without copying. Client-side chunks use | ||
| // target=.browser and may contain raw UTF-8. | ||
| .js, .jsx, .ts, .tsx => if ((output_file.side orelse .server) == .server) .latin1 else .binary, |
There was a problem hiding this comment.
🔴 The "guaranteed Latin-1 safe" assumption doesn't hold for --banner/--footer (and source hashbangs / // <path> comments): those are appended raw via j.pushStatic() in postProcessJSChunk.zig and never go through the printer's ascii_only escaping. Before this PR the dropped encoding field meant such bytes were decoded via cloneUTF8; now a server chunk containing e.g. --banner='// © 2024' or --banner='globalThis.msg = "héllo"' will be wrapped as a Latin-1 ExternalStringImpl and JSC will see © / héllo. Consider scanning the final chunk for bytes ≥ 0x80 (e.g. bun.strings.isAllASCII) before choosing .latin1, or routing banner/footer through the same ASCII-escaping path as the printer.
Extended reasoning...
What the bug is
This PR's optimization rests on the invariant stated in the new comment at src/StandaloneModuleGraph.zig:558-561: "Server-side chunks are printed with target=.bun → ascii_only=true, so their bytes are guaranteed Latin-1 safe." That invariant is true for everything emitted by js_printer, but it is not true for the final chunk bytes, because postProcessJSChunk.zig concatenates several raw byte slices around the printer output without escaping:
j.pushStatic(hashbang)— line 284 (sourced from the input file's first line)j.pushStatic(banner)— line 314 (sourced verbatim from--banner/Bun.build({ banner }))- the
// <pretty path>per-file comment when!minify_whitespace— around line 387+ (file paths can contain non-ASCII) j.pushStatic(ctx.c.options.footer)— line 549
StringJoiner.pushStatic just stores the slice; there is no quoteForJSON/ascii_only pass over these bytes. c.options.banner and c.options.footer come straight from args.option("--banner") / args.option("--footer") in src/cli/Arguments.zig:1088 (and from the JS config in bundle_v2.zig) with no transformation.
Why this is a regression introduced by this PR
Prior to this PR, fromBytes() did not copy module.encoding, so every File defaulted to .binary and toWTFString() always took the bun.String.cloneUTF8(this.contents) branch. That branch decodes the bytes as UTF-8, so a banner containing © (bytes c2 a9) or é (bytes c3 a9) round-tripped correctly into the WTF string handed to JSC's parser.
After this PR, server JS chunks are tagged .latin1 and fromBytes() now propagates that tag, so toWTFString() calls bun.String.createStaticExternal(this.contents, true). BunString__createStaticExternal (src/bun.js/bindings/BunString.cpp) reinterpret-casts the buffer as const LChar* and constructs an ExternalStringImpl with is8Bit() == true — i.e. each byte becomes one Latin-1 code unit with no UTF-8 detection. The resulting bun.String is used directly as .source_code in ModuleLoader.zig, so JSC parses the mojibake.
Step-by-step proof
Take bun build --compile --banner='globalThis.msg = "héllo"' entry.ts:
- The CLI stores the banner as the raw UTF-8 bytes
68 c3 a9 6c 6c 6fforhéllo. postProcessJSChunkpushes those bytes verbatim at line 314; the rest of the chunk (printer output) is pure ASCII becauseascii_only=true.toBytes()seesloader=.js,side=.server→ writesencoding = .latin1.- At runtime,
fromBytes()now copies.encoding = .latin1(line 352 of this PR). toWTFString()hits the.latin1arm →createStaticExternal(contents, true)→ WTF string whose code units are0x68, 0xC3, 0xA9, 0x6C, 0x6C, 0x6F.- JSC parses
globalThis.msg = "héllo"; at runtimeglobalThis.msg === "héllo", not"héllo".
For a comment-only banner like // © 2024 Société the corruption is cosmetic (it only affects Function.prototype.toString/stack traces), but any banner/footer that contains a non-ASCII string literal — or a non-ASCII identifier — produces incorrect runtime values or a SyntaxError. The hashbang and per-file path comment cases are rarer but follow the same mechanism (e.g. a source file under a directory named prébuild/).
Why nothing else guards against it
There is no isAllASCII check anywhere between postProcessJSChunk and StandaloneModuleGraph.toBytes, and no CLI validation rejecting non-ASCII --banner/--footer. The only thing that previously made this safe was the very bug this PR fixes (the dropped encoding field forcing cloneUTF8).
Suggested fix
Cheapest correct fix: in toBytes(), gate .latin1 on the actual bytes rather than on side alone, e.g.
.js, .jsx, .ts, .tsx => if ((output_file.side orelse .server) == .server and
bun.strings.isAllASCII(output_file.value.buffer.bytes)) .latin1 else .binary,This keeps the zero-copy win for the overwhelmingly common case (no banner, or ASCII banner) and falls back to cloneUTF8 only when the chunk genuinely contains UTF-8. Alternatively, route banner/footer/hashbang through ASCII escaping when ascii_only is set so the invariant actually holds.
StandaloneModuleGraph.fromBytes() was not copying the serialized `encoding` field into File, leaving it at the default `.binary`. This caused File.toWTFString() to always take the cloneUTF8 path, copying every embedded module source into WebKit Malloc even though the bytes already live in the mmap-backed __BUN section. Server-side JS chunks are printed with target=.bun (ascii_only=true), so their bytes are Latin-1 safe and can back a static ExternalStringImpl without copying. Client-side chunks (target=.browser) may contain raw UTF-8, so toBytes() now records them as .binary to keep the cloneUTF8 path when the server imports one.
bun build --compile auto-appends .exe on Windows, so spawning [outfile] failed with ENOENT on all three Windows CI shards (2019 x64, 2019 x64-baseline, Windows 11 aarch64).
The test fails deterministically (4 in-runner attempts) on the Windows 11 ARM64 CI shard. Other --compile-using and ARM64-bun tests also fail on the same shard (napi.test.ts, complex-workspace.test.ts), suggesting an issue specific to that runner that is unrelated to the encoding change. x64 Windows runs (2019 x64 / x64-baseline) pass with the .exe suffix fix from the previous commit.
Revert the todoIf skip so Windows ARM64 actually runs the test, and print embeddedFiles, src length, JP-presence, importPath, build/run stdout/stderr/exitCode so the failure mode is visible in CI logs.
The previous version used path.join(import.meta.dir, js.name). On
Windows the bunfs mount surfaces as B:\\~BUN\\root, so join returned
a backslash-separated absolute path that the module loader could not
resolve back to the embedded chunk (ENOENT). A relative specifier
("./" + js.name) avoids platform-specific path handling entirely and
still exercises the ModuleLoader -> File.toWTFString() path this test
is guarding.
Diagnostic console.logs are removed now that the failure is understood.
CI revealed a separate bug in bun's Windows resolver for compiled binaries: dynamic import() inside a compiled server resolves "./foo" relative to the process executable (B/~BUN/root/app.exe) rather than the importing module, so the embedded sibling chunk is not found. That is unrelated to the encoding fix in this PR and will be tracked separately. For now, skip the test on Windows so it guards the encoding behavior on POSIX, where it actually reaches File.toWTFString().
99d24d3 to
cd0cb10
Compare
| expect(stdout).toContain("CLIENT_OUTPUT: こんにちは"); | ||
| expect(exitCode).toBe(0); | ||
| }, | ||
| 60_000, |
There was a problem hiding this comment.
🟡 nit: per test/CLAUDE.md § "No timeouts" ("CRITICAL: Do not set a timeout on tests. Bun already has timeouts."), drop the trailing 60_000 argument here. Sibling compile tests like compile-argv.test.ts rely on the harness-level timeout instead of a per-test one.
Extended reasoning...
What this is
test/CLAUDE.md:118-120 documents a repo-wide test convention:
No timeouts
CRITICAL: Do not set a timeout on tests. Bun already has timeouts.
The new test passes 60_000 as the third positional argument to test():
test.skipIf(isWindows)(
"compiled client-side chunk with non-ASCII source can be imported on the server",
async () => { ... },
60_000,
);That third argument is the per-test timeout in bun:test, so this is exactly what the CLAUDE.md rule prohibits.
Why the rule exists / why nothing else mitigates it
The repo's test runner already applies its own timeout policy at the harness level. Per-test timeouts in this codebase tend to either (a) mask harness-level slow-test detection or (b) drift out of sync with CI's actual budgets. The CLAUDE.md entry is marked CRITICAL specifically so new tests don't reintroduce them. Nothing in the file needs the override — the test does a single bun build --compile plus one subprocess run, which is the same workload as compile-argv.test.ts and compile-process-execargv.test.ts, neither of which set an explicit timeout.
Step-by-step
test/CLAUDE.md:120says "Do not set a timeout on tests."test/bundler/compile-client-chunk-encoding.test.ts:67(added in this PR) passes60_000as the timeout argument totest().- → newly-introduced violation of a documented repo convention.
There is one pre-existing violator (compile-sourcemap-internal.test.ts:63 also passes 60_000), but that's pre-existing debt — the CLAUDE.md rule is unambiguous and this PR shouldn't add another instance.
Impact
Convention/style only — there's no functional bug here. The test will run identically with or without the argument in the common case; the only effect is diverging from the repo's stated test-authoring rules.
Fix
Delete the trailing 60_000, on line 67 so the call becomes:
test.skipIf(isWindows)(
"compiled client-side chunk with non-ASCII source can be imported on the server",
async () => {
...
},
);
…n contents are ASCII (#31557) `from_bytes()` was hardcoding `encoding: Encoding::Binary` for every embedded file instead of reading the per-module value serialized by `to_bytes()`, so `File::to_wtf_string()` always took the `clone_utf8` path — a fresh heap copy of the entire bundled source at module load — instead of the zero-copy `create_static_external` path that wraps the kernel-mmapped `.bun` section directly. Honoring the serialized encoding alone is unsafe: `to_bytes()` previously tagged JS as `Latin1` purely by loader type, but `--banner` / `--footer` / hashbang and client-side (`target=browser`) chunks are concatenated verbatim as UTF-8 by `postProcessJSChunk`, so non-ASCII bytes there would mojibake under a Latin-1 `ExternalStringImpl` (e.g. `--footer 'console.log("résumé")'` → `résumé`). This PR gates the `Latin1` tag in `to_bytes()` on `strings::first_non_ascii(buf_bytes).is_none()` — a build-time SIMD scan — so the runtime only takes the zero-copy path when it's actually safe, and falls back to `clone_utf8` (today's behaviour) otherwise. The printer escapes non-ASCII for server-side JS by default, so the common case (no banner/footer, no client chunks imported on the server) gets the zero-copy path. On a `bun build --compile --bytecode` binary with a 40 MB bundle, that avoids one 40 MB allocation + walk at startup and 40 MB of pinned heap that should have stayed evictable file-backed pages. Adds `compile/FooterNonAsciiUTF8` and `compile/BannerNonAsciiUTF8` tests that fail (mojibake) without the ASCII guard. (Supersedes the `.zig`-only #29319, which targets the no-longer-compiled reference implementation and gates on `side == .server` rather than verifying the bytes — that still mojibakes a non-ASCII `--footer` on a server chunk.) --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
|
Closing as stale: this PR predates the Rust rewrite. Every If the underlying change is still wanted, it will need to be redone against the current Rust/C++ tree. Apologies for the churn, and thank you for the contribution. |
2 participants
Summary
$bunfsmodule sources embedded in abun build --compilebinary live in a mmap'd__BUNsection, but on module load every chunk was being fully copied into WebKit Malloc. This PR avoids that copy and saves ~17 MB of heap in the Claude Code CLI binary (294 embedded modules).Root cause
StandaloneModuleGraph.fromBytes()was not copying theencodingfield when deserializingFileentries. The default forFile.encodingis.binary, which putsFile.toWTFString()into thecloneUTF8branch:toBytes()writes.latin1for.js/.jsx/.ts/.tsx, but the field was silently dropped on read, so every module was treated as.binaryat runtime and always went throughcloneUTF8. As a result, every embedded JS chunk existed in memory twice: once in the__BUNsection (mmap, clean) and once in WebKit Malloc (libpas, dirty).Measurement
Target: Claude Code CLI (
bun build --compileoutput, 294 embedded modules).heapsnapshot analysis
A
/heapdumpsnapshot showsModuleRecordwith a totalself_sizeof 16.78 MB — the largest type on the JSC heap.self_sizehere reflects Bun's fork ofJSModuleRecord::estimatedSize(vendor/WebKit/Source/JavaScriptCore/runtime/JSModuleRecord.cpp, underUSE(BUN_JSC_ADDITIONS)), which folds insourceCode().provider()->source().length()— i.e. the JS source bytes held on the heap.Top modules:
/$bunfs/root/chunk-1cynz86c.js/$bunfs/root/chunk-wy676xem.js/$bunfs/root/chunk-q1jpqy33.js/$bunfs/root/chunk-dxd7x54w.js/$bunfs/root/chunk-y2z8n5hx.jslldb confirmation
On a running Claude Code CLI process,
memory find -s "createSignal"across the relevant regions:__BUN(mmap, dirty=0)0x1051e0000 – 0x108a980000x116b00000 – 0x122b000000x44142000000 – 0x44152000000The same byte sequence (
createSignal,\n isEnvTruthy\n} fr) appears at0x1054e4a6ein__BUNand at0x118861775in WebKit Malloc. mimalloc has no hit, so this isn't a double copy — it's a one-way mmap → heap copy that this PR eliminates.Fix
1. Propagate
encodinginfromBytes()File{ .name = sliceToZ(raw_bytes, module.name), .loader = module.loader, + .encoding = module.encoding, .contents = sliceToZ(raw_bytes, module.contents),With this single line, server-side JS chunks (emitted by the bundler with
target=.bun, which setsascii_only=true, so the bytes are Latin-1 safe) flow through the existingcreateStaticExternalbranch, using the mmap'd__BUNsection directly as the WTF string's backing store. The__BUNmapping lives for the lifetime of the process, so a static external pointer into it is safe.2. Correct the encoding for client-side chunks
toBytes()previously marked every JS loader as.latin1unconditionally. That's unsafe for client chunks:.encoding = switch (output_file.loader) { - .js, .jsx, .ts, .tsx => .latin1, + .js, .jsx, .ts, .tsx => if ((output_file.side orelse .server) == .server) .latin1 else .binary, else => .binary, },Why:
Bun.build({ compile: { outputs } })allows client chunks (target=.browser) to be embedded alongside server chunks. The browser target usesascii_only=false, so the printer may emit raw UTF-8 bytes. Treating those as Latin-1 via a static external would corrupt any non-ASCII content. Only server chunks are marked.latin1; client chunks stay.binary(i.e. thecloneUTF8path), which is what server code sees when it doesimport('$bunfs/...')for a client chunk.Test
Adds
test/bundler/compile-client-chunk-encoding.test.ts, which builds a--compilebinary containing a client-side chunk with non-ASCII content and verifies that importing it from the server at runtime returns the correct UTF-8 bytes (no Latin-1 misinterpretation).Safety notes
ZigSourceProvider::create()usesresolvedSource.needsDerefto decide whether to callderef()on the source code.createStaticExternalreturns a static String, soderefis a no-op — this path is already exercised today for non-$bunfsmodules.JSModuleRecord::visitChildrenImplcallsreportExtraMemoryVisitedviasourceCode().memoryCost(). Using anExternalStringImplas the backing store doesn't changememoryCost(), so GC accounting is unaffected — the bytes simply no longer have a duplicate heap allocation behind them.File.toWTFString()itself is unchanged. This PR is a bug fix that lets an existing fast path fire correctly; it does not introduce a new code path.