standalone_graph: zero-copy source string for compiled JS modules when contents are ASCII - #31557
Conversation
|
Updated 2:46 AM PT - May 29th, 2026
❌ @autofix-ci[bot], your commit ce4bd3d has 2 failures in
🧪 To try this PR locally: bunx bun-pr 31557That installs a local version of the PR into your bun-31557 --bun |
|
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 (2)
WalkthroughDeserialization now restores each embedded File.encoding from the serialized CompiledModuleGraphFile.encoding. Serialization emits Latin1 for JS-like loaders only when the output buffer contains no non-ASCII bytes; otherwise it uses Binary. A bundler test verifies compiled output preserves UTF-8 non-ASCII bytes. ChangesEncoding preservation and emission
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Addressed in c48a2bb — gated |
There was a problem hiding this comment.
LGTM — the ASCII guard in to_bytes() addresses the mojibake concern, and the new --banner/--footer tests lock it in.
Extended reasoning...
Overview
This PR touches src/standalone_graph/StandaloneModuleGraph.rs (two small hunks) and adds two regression tests in test/bundler/bundler_compile.test.ts. The Rust change (1) makes from_bytes() read the per-module encoding from the serialized record instead of hardcoding Encoding::Binary, and (2) makes to_bytes() tag JS modules as Latin1 only when strings::first_non_ascii(buf_bytes).is_none() — i.e., the final chunk bytes are verifiably all-ASCII. Together this lets File::to_wtf_string() take the zero-copy create_static_external path over the mmapped .bun section instead of heap-cloning the source at module load.
Security risks
None identified. No untrusted input handling, auth, or crypto is involved. The Latin1 external-string path wraps bytes that are already mmapped read-only into the process; the ASCII guard runs at build time on bundler output, not on runtime user input. The fallback path (Binary → clone_utf8) is the pre-PR behavior.
Level of scrutiny
Moderate. from_bytes() runs at startup of every bun build --compile binary and to_wtf_string() feeds source code to JSC, so a wrong encoding tag would mojibake or SyntaxError user code. However, the change is ~15 lines, the new behavior is gated on a byte-level SIMD ASCII check (not a loader-type heuristic), and any non-ASCII byte falls back to the existing safe path — so the worst case is identical to today's behavior. ASCII bytes are bit-identical under Latin-1 and UTF-8, so the zero-copy wrap is sound when the guard passes.
Other factors
I flagged the original mojibake regression (--banner/--footer/hashbang concatenated verbatim as UTF-8) on the first revision; the author addressed it in c48a2bb exactly as suggested — gating on actual byte contents rather than loader type — and added compile/FooterNonAsciiUTF8 and compile/BannerNonAsciiUTF8 tests that exercise both 2-byte (résumé) and 3-byte (こんにちは) UTF-8 sequences and would print mojibake without the guard. The bug-hunting pass on the updated revision found nothing. CODEOWNERS does not cover these paths. robobun shows build-rust failures on c48a2bb, but autofix.ci pushed ce4bd3d afterward; CI gates will block merge if anything remains red.
from_bytes()was hardcodingencoding: Encoding::Binaryfor every embedded file instead of reading the per-module value serialized byto_bytes(), soFile::to_wtf_string()always took theclone_utf8path — a fresh heap copy of the entire bundled source at module load — instead of the zero-copycreate_static_externalpath that wraps the kernel-mmapped.bunsection directly.Honoring the serialized encoding alone is unsafe:
to_bytes()previously tagged JS asLatin1purely by loader type, but--banner/--footer/ hashbang and client-side (target=browser) chunks are concatenated verbatim as UTF-8 bypostProcessJSChunk, so non-ASCII bytes there would mojibake under a Latin-1ExternalStringImpl(e.g.--footer 'console.log("résumé")'→résumé). This PR gates theLatin1tag into_bytes()onstrings::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 toclone_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 --bytecodebinary 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/FooterNonAsciiUTF8andcompile/BannerNonAsciiUTF8tests that fail (mojibake) without the ASCII guard.(Supersedes the
.zig-only #29319, which targets the no-longer-compiled reference implementation and gates onside == .serverrather than verifying the bytes — that still mojibakes a non-ASCII--footeron a server chunk.)