compile cache: encode the bytecode the main VM compiled instead of re-parsing every module on a second VM - #40174
Jarred-Sumner wants to merge 1 commit into
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 18 minutes Limit details: You’ve used the included review currently available. Your 62 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. You’re in a promotional period — use the checkbox below to run this review for free:
On-demand reviews are free for the next 28 days. After that, they cost $0.25 per reviewed file. How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (15)
Comment |
|
Updated 12:41 AM PT - Aug 23rd, 2026
✅ @Jarred-Sumner, your commit b09921393103251faee14b107b877b0185a81fd5 passed in 🧪 To try this PR locally: bunx bun-pr 40174That installs a local version of the PR into your bun-40174 --bun |
2389795 to
f2f0117
Compare
…-parsing every module on a second VM A cold run with module.enableCompileCache() used to spend its exit re-parsing every missed module on a dedicated "BunCompileCache" thread with its own JSC VM and eagerly compiling every function in it. The SourceProvider now gets the live top-level UnlinkedCodeBlock from JSC right after CodeCache generates it (oven-sh/WebKit#493) and keeps a Weak to it on the VM's client data. The block is encoded once with encodeCodeBlock(): either right before Bun's GC drops unlinked function code (JSC__VM__runGC → deleteAllUnlinkedCodeBlocks), or at persist time (exit, module.flushCompileCache(), a --watch reload), whichever comes first. That writes the top-level block plus the code blocks of every function that ran up to that point; functions that never ran stay lazy stubs. Nothing is rooted: a block the GC already collected is simply not cached this run. The file format is unchanged: one CachedBytecode, decoded by the same path as before. Workers encode what they compiled in their own on_exit and hand the bytes to the entry; the main thread's exit writes them. Entries carry an id so bytecode for a module that was rewritten and re-required in the same process is dropped in favor of the new version's. --watch reloads now go through the JS thread when the cache is enabled, like they already did with --watch-kill-signal listeners, so the modules get encoded before execve; the grace timer still forces the reload if the JS thread is stuck, persisting only what was already encoded. Pins WebKit to the PR preview build until #493 lands.
f2f0117 to
b099213
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/standalone_graph/StandaloneModuleGraph.rs:855-884— Commit fa2e6da ("WIP: --compile embeds text imports as pre-encoded string modules (unbuilt draft)") is riding along in this PR — it's not mentioned in the description, self-labels as an unbuilt draft, touches five source files with zero tests, and has a concrete correctness bug:encode_text_module()stores non-ASCII text as WTF Latin-1/UTF-16 bytes, but the same commit now emits the .txt as an Asset OutputFile, sofs.readFileSync/Bun.embeddedFiles/stat().sizeon the bunfs path return the transcoded bytes (e.g. 'café' → 4 bytes63 61 66 E9→readFileSync(p,'utf8')yields'caf�'). This commit should be dropped from the PR, or fixed and given--compiletests covering ASCII / Latin-1-fit / UTF-16 text plus the raw-byte surfaces.Extended reasoning...
What is riding along in this PR
This PR's title and description are entirely about the compile-cache rework (commit
23897956), which is well-tested. But the diff also contains commitfa2e6da4, whose own message is "WIP: --compile embeds text imports as pre-encoded string modules (unbuilt draft)".git show --stat fa2e6da4confirms it touches five source files —ParseTask.rs,bundle_v2.rs,StandaloneModuleGraph.rs,bun_core/string/mod.rs,jsc_hooks.rs— with zero test files, and the PR description never mentions it. It looks accidentally included.The correctness bug
Under
--compile, the newLoader::Textarm inParseTask.rsnow setsunique_key_for_additional_file(kind=Asset) andbundle_v2.rsbumpsestimated_file_loader_count, soprocess_files_to_copy(bundle_v2.rs:4268-4285) emits the .txt file as anOutputFile { output_kind: Asset, loader: Text, side: Some(Side::Client) }. InStandaloneModuleGraph::to_bytes, that OutputFile is routed throughencode_text_module()(StandaloneModuleGraph.rs:855-906), which stores the file's bytes as a WTF string body — Latin-1 when every code point ≤ U+00FF, else native-endian UTF-16 — not the original UTF-8. That's correct for the intended consumer (fetch_builtin_module→to_wtf_string→ JSString default export). But because the file'ssideis mapped toFileSide::ClientandLoader::Text.is_javascript_like()is false,appears_in_embedded_files_array()(line 489-490) returns true, andfind_ref()finds it — so every raw-byte consumer ofFile.contentsnow sees the transcoded bytes:fs.readFileSync(bunfsPath)— node_fs.rs:6970 doesfile.contents.as_bytes()and returns them verbatim (Buffer) or decodes them as the requested string encoding.fs.statSync(bunfsPath).size—File::stat()(line ~495) returnscontents.len(), the transcoded byte count.Bun.embeddedFiles[i]/Bun.file(bunfsPath)—standalone_graph_jsc.rs::file_blob()wrapsfile.contents.as_bytes()in aBytesstore;.arrayBuffer()/.text()/.sizeall reflect the transcoded encoding.
Step-by-step proof
Take
greeting.txtcontainingcafé— 5 UTF-8 bytes63 61 66 C3 A9.encode_text_module:first_non_asciifindsC3→to_utf16_allocyields code units[0x0063, 0x0061, 0x0066, 0x00E9]→ all ≤ 0xFF → Latin-1 branch stores 4 bytes63 61 66 E9.to_bytesrecordsencoding = Encoding::Latin1,side = FileSide::Client,loader = Text.- At runtime,
import text from './greeting.txt'correctly yields'café'(viato_wtf_string→create_static_external(.., is_latin1=true)). - But
Bun.embeddedFilesnow includes a Blob whose.sizeis 4 and whose.arrayBuffer()is[0x63, 0x61, 0x66, 0xE9]. fs.readFileSync(bunfsPath, 'utf8')decodes63 61 66 E9as UTF-8 →0xE9is an invalid UTF-8 lead byte → returns'caf�'(U+FFFD).- For text containing any code point > U+00FF (e.g.
'€'= U+20AC), the UTF-16 branch storesAC 20(little-endian), soreadFileSyncreturns bytes with interleaved zeros / garbage.
Before this PR,
Loader::Textunder--compileinlined the string asexport default "..."and never emitted an asset OutputFile, so .txt files were absent fromBun.embeddedFilesandreadFileSyncon a bunfs .txt path returned ENOENT. This PR newly exposes the file through those APIs, but with content that does not match the source for any non-ASCII text. All-ASCII text is unaffected (ASCII ≡ Latin-1 ≡ UTF-8), which is likely why nothing failed locally.Missing tests
REVIEW.md is explicit: "Every behavioral change ships an automated test in the same PR. 'Verified manually' … don't count, even for one-liners" and "Cover the variant matrix". This feature adds a new
Encoding::Utf16variant, an unsafe u8→u16 slice reinterpretation inFile::to_wtf_string, alignment-dependentcreate_static_external_utf16, three encoding branches inencode_text_module(ASCII fast path, Latin-1 narrowing, UTF-16 with 2-byte alignment padding), and a newfetch_builtin_moduleLoader::Textruntime arm — none of which is exercised. A grep oftest/bundler/finds no test that imports a .txt file under--compileand asserts the resulting string; the only test file in the PR diff isnode-module-module.test.js, which covers only the compile-cache commit.How to fix
The cleanest option is to drop
fa2e6da4from this PR and land it separately. If it stays, it needs:- The correctness fix: either (a) exclude text-module assets from
appears_in_embedded_files_array()/find_ref()so the raw-byte APIs don't see them (matching pre-PR behavior), or (b) store the original UTF-8 bytes inFile.contentsand only transcode on theto_wtf_string()path (or store both). - Tests in
test/bundler/(itBundledwithcompile: true) that import (a) a pure-ASCII .txt, (b) a Latin-1-representable non-ASCII .txt (e.g.café), and (c) a UTF-16-requiring .txt (e.g. emoji), run the compiled binary, and assert both the imported string andfs.readFileSync/Bun.embeddedFilesbytes match the original file.
| void SourceProvider::didGenerateUnlinkedCodeBlock(JSC::VM& vm, const JSC::SourceCodeKey& key, JSC::UnlinkedCodeBlock* codeBlock) const | ||
| { | ||
| if (!m_resolvedSource.node_compile_cache_entry_id) | ||
| return; | ||
| if (codeBlock->codeType() != JSC::GlobalCode && codeBlock->codeType() != JSC::ModuleCode) | ||
| return; | ||
| WebCore::clientData(vm)->nodeCompileCachePending.append({ m_resolvedSource.node_compile_cache_key, m_resolvedSource.node_compile_cache_entry_id, key, JSC::Strong<JSC::UnlinkedCodeBlock>(vm, codeBlock) }); | ||
| } | ||
|
|
||
| // Runs on the VM's thread (exit, module.flushCompileCache(), a --watch reload) with its API lock held. | ||
| extern "C" void Bun__NodeCompileCache__encodePending(JSC::VM* vm) | ||
| { | ||
| auto pending = std::exchange(WebCore::clientData(*vm)->nodeCompileCachePending, {}); | ||
| for (auto& entry : pending) { | ||
| RefPtr<JSC::CachedBytecode> bytecode = JSC::encodeCodeBlock(*vm, entry.key, entry.codeBlock.get()); | ||
| if (bytecode) | ||
| Bun__NodeCompileCache__deliver(entry.cacheKey, entry.entryId, bytecode->span().data(), bytecode->span().size()); | ||
| else | ||
| Bun__NodeCompileCache__deliver(entry.cacheKey, entry.entryId, nullptr, 0); | ||
| } |
There was a problem hiding this comment.
🔴 nodeCompileCachePending grows unboundedly under bun --hot + NODE_COMPILE_CACHE: every edit-reload appends a {SourceCodeKey, Strong<UnlinkedCodeBlock>} that is never drained (only the HotReload::Watch branch of VirtualMachine::reload calls persist_now), so each reload permanently roots the old bytecode tree and — via SourceCodeKey's embedded RefPtr<SourceProvider> — the old Zig::SourceProvider with its full transpiled source. Pre-PR retention was O(modules) (the Rust STATE map replaces on re-fetch); post-PR it is O(modules × reloads). Fix: call encode_pending() from the HotReload::Hot path of reload(), or dedupe by cacheKey on append.
Extended reasoning...
What the bug is
Zig::SourceProvider::didGenerateUnlinkedCodeBlock (ZigSourceProvider.cpp:166-173) appends a NodeCompileCachePending { cacheKey, entryId, SourceCodeKey, Strong<UnlinkedCodeBlock> } to JSVMClientData::nodeCompileCachePending every time JSC's CodeCache generates a top-level code block for a module whose node_compile_cache_entry_id != 0. That vector is drained in exactly one place — Bun__NodeCompileCache__encodePending (line 178, via std::exchange) — which is reached only from encode_pending(), whose callers are persist_at_exit / persist_now / flush / the worker branch of on_exit. None of these run on an in-process --hot reload.
The code path that triggers it
VirtualMachine::reload (VirtualMachine.rs:3869-3884) gates the compile-cache flush on if self.hot_reload == HotReload::Watch; that branch ends in reload_process() (execve). The HotReload::Hot case falls through to line 3885+ and re-evaluates modules in-process without ever touching encode_pending. on_exit is likewise never reached under --hot because the process stays alive.
Under bun --hot with NODE_COMPILE_CACHE set (or module.enableCompileCache()), every edit changes the transpiled bytes, so node_compile_cache::fetch() sees code_hash != stored, allocates a fresh Entry with next_entry_id(), and returns Fetch::miss with a nonzero entry_id. That id lands in ResolvedSource.node_compile_cache_entry_id, so when JSC compiles the reloaded module, didGenerateUnlinkedCodeBlock sees entry_id != 0 and appends another element. Nothing ever removes it.
Why nothing prevents it
The Rust-side STATE map is fine — state.entries.insert(key, entry) replaces the old entry on each re-fetch, so its retention stays O(modules). But the C++-side nodeCompileCachePending vector is append-only until an encode pass runs, and no encode pass runs on the Hot path. The PR description explicitly mentions handling --watch ("--watch reloads go through the JS thread when the cache is enabled") but never mentions --hot, so the case appears overlooked.
Impact
Each stale vector element retains:
- a
JSC::Strong<UnlinkedCodeBlock>rooting the old top-level unlinked code block tree, which JSC's CodeCache would otherwise age out and let the GC collect; - a
JSC::SourceCodeKey, which embeds anUnlinkedSourceCodeholdingRefPtr<SourceProvider>, so the oldZig::SourceProvider— including itsm_source(the full transpiled module text) — stays alive too.
A dev session that edits a large module N times under bun --hot with the compile cache enabled leaks ~N copies of that module's source string plus its unlinked bytecode tree until process exit. This is a regression: before this PR there was no per-VM pending vector at all (bytecode was regenerated on-demand by a worker VM at persist time), so retention was O(modules); now it is O(modules × hot-reloads).
Even if these entries were eventually encoded at exit, deliver()'s entry.id != entry_id check drops the stale bytes, so keeping them serves no purpose — it is pure waste.
Step-by-step proof
- Start
NODE_COMPILE_CACHE=/tmp/cc bun --hot app.jswhereapp.jsimportsbig.js(say, 500 KB after transpile). - Cold load:
fetch("big.js", ...)→ newEntry { id: 1 }, on-disk miss →Fetch::misswithentry_id = 1. JSC compiles →didGenerateUnlinkedCodeBlockappends element Fix ?? operator #1 (Strong<UCB₁>,SourceCodeKeyreferencingSourceProvider₁holding 500 KB source). - Edit
big.js; watcher posts aHotReloadTask;VirtualMachine::reloadruns withhot_reload == Hot→ skips lines 3870-3884, falls through — noencode_pending. - Re-evaluation re-fetches:
code_hashdiffers → newEntry { id: 2 }replaces id 1 in the Rust map →Fetch::misswithentry_id = 2. JSC compiles → appends element Fix calling #private() functions in classes #2 (Strong<UCB₂>,SourceProvider₂). - Element Fix ?? operator #1 is still in the vector.
UCB₁cannot be collected (rooted byStrong);SourceProvider₁cannot be freed (refcounted by element Fix ?? operator #1'sSourceCodeKey). - Repeat N edits → N elements, ~N × (500 KB source + bytecode tree) retained.
- At process exit,
persist_at_exitfinally encodes all N;deliver(key, 1, ...)throughdeliver(key, N-1, ...)all hitentry.id != entry_id(entry.id is now N) and discard the bytes — the stale entries were never useful.
Fix
Call crate::node_compile_cache::encode_pending(self.jsc_vm_mut()) in the HotReload::Hot path of VirtualMachine::reload (e.g. right after the Watch branch, before the module-map reset). Alternatively, dedupe on append in didGenerateUnlinkedCodeBlock: scan nodeCompileCachePending for an element with the same cacheKey and replace it in place — the vector is small (one entry per missed module), and the stale element's bytecode will be discarded by deliver() anyway.
This branch has not been deployed
2 participants
What does this PR do?
module.enableCompileCache()persisted a missed module by sending its source to a dedicated thread with a second JSC VM, re-parsing it there, and eagerly compiling every function. This removes that thread and VM.Instead, the
SourceProviderreceives the live top-levelUnlinkedCodeBlockfrom JSC right afterCodeCachegenerates it (new hook in oven-sh/WebKit#493) and keeps aWeakto it on the VM's client data. The block is encoded once withencodeCodeBlock():JSC__VM__runGC→deleteAllUnlinkedCodeBlocks), ormodule.flushCompileCache(), a--watchreload),whichever comes first. That writes the top-level block plus the code blocks of every function that ran up to that point. Functions that never ran stay lazy stubs. Nothing is rooted: a block the GC already collected is simply not cached this run.
CachedBytecode, loaded by the same decode path as before.on_exitand hand the bytes to the entry; the main thread's exit writes them.--watchreloads go through the JS thread when the cache is enabled (same path--watch-kill-signallisteners already use), so modules are encoded beforeexecve. The grace timer still forces the reload if the JS thread is stuck; it persists only what was already encoded.leafExecutables()/addFunctionUpdateuse, so this composes with Bytecode cache: drop Decoder bookkeeping that Bun never reads WebKit#490.Why encode before the GC: a CommonJS program only evaluates to its wrapper function, so the tree is empty until the wrapper body runs, and Bun's
runGC(whichrun_commandcalls right after the entrypoint's synchronous evaluation) detaches every unlinked function code block. Encoding at that point captures everything compiled during startup.Supersedes #39167 (same goal, incremental-update layout). #39405 (mmap the sidecars) can rebase on top.
WEBKIT_VERSIONpoints at the PR preview build of oven-sh/WebKit#493; swap to the main sha once it lands.How did you verify your code works?
Built with
bun run build:localagainst the WebKit branch.test/js/node/module/node-module-module.test.js(52 pass, includes new tests: a module's ran functions are in the entry and larger than a load-only entry; functions that ran beforeBun.gc(true)are kept; worker modules persist; rewrite-and-re-require persists v2)test/cli/watch/watch.test.ts(8 pass;NODE_COMPILE_CACHE persists across a --watch reload)test/internal/source-lints/(162 pass; the thread-spawn inventory entry is gone)BUN_JSC_reportBytecodeCacheDecodeTimes=1shows the warm run decoding the entries;dep.jsentry is 2.0 KB when only loaded vs 2.8 KB when one of its functions ran, with or without asetTimeout/Bun.gc()before exit.