SourceProvider: hand the provider the live UnlinkedCodeBlock CodeCache generated - #493
Conversation
…e generated Bun's node compile cache wants to encode a module's bytecode once at exit, after the functions that ran were compiled, instead of re-parsing it on a second VM. cacheBytecode() only gives the provider a generator that encodes immediately, so add didGenerateUnlinkedCodeBlock(VM&, const SourceCodeKey&, UnlinkedCodeBlock*) under USE(BUN_JSC_ADDITIONS): the provider can root the block and call encodeCodeBlock() with the same key later.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 52 minutes Limit details: You’ve used the included review currently available. Your 63 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: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — adds a Bun-guarded no-op virtual hook and calls it alongside cacheBytecode().
What was reviewed:
- New virtual follows the same pattern as
cacheBytecode/commitCachedBytecode/memoryCostdirectly above it; default no-op, so existing providers are unaffected. - Call site in
getUnlinkedGlobalCodeBlockis inside theif (unlinkedCodeBlock && Options::useCodeCache())block, so the pointer is non-null andkeyis live. - Forward declarations of
VM/SourceCodeKey/UnlinkedCodeBlockare sufficient since parameters are by reference/pointer.
Extended reasoning...
Overview
Two-file, ~7-line addition. SourceProvider.h gains three forward declarations and one new virtual, didGenerateUnlinkedCodeBlock(VM&, const SourceCodeKey&, UnlinkedCodeBlock*), with an empty default body, gated behind #if USE(BUN_JSC_ADDITIONS). CodeCache.cpp invokes it in getUnlinkedGlobalCodeBlock immediately after the existing cacheBytecode() call, under the same guard. The purpose (per the description) is to let Bun's provider grab and root the freshly-generated UnlinkedCodeBlock for later encoding, avoiding a second-VM re-parse in the compile cache path.
Security risks
None. This is a notification hook with a no-op default; no new data flows into parsing, allocation, or codegen. It hands out a pointer to an object the provider could already reach indirectly via the encode-generator lambda. No untrusted input, no auth/crypto/permissions surface.
Level of scrutiny
Low. The change is tiny, mechanical, and pattern-matched against the adjacent virtuals (cacheBytecode, updateCache, commitCachedBytecode, and the Bun-only memoryCost right above it — same JS_EXPORT_PRIVATE virtual ... const { } shape). Both additions are behind USE(BUN_JSC_ADDITIONS), so upstream/non-Bun builds compile identically. The unconditional forward declarations are harmless.
Other factors
The call is placed where unlinkedCodeBlock is already checked non-null and key is a live stack local, so there's no lifetime or null-deref concern at the call site. Any interesting behavior (rooting via Strong<UnlinkedCodeBlock>, deferred encoding) lives in the Bun-side override, not in this PR — here it's purely the extension point. No outstanding reviewer comments; the only timeline entry is a CodeRabbit rate-limit notice.
Preview Builds
|
…-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.
…-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.
1 participant
Adds one virtual under
USE(BUN_JSC_ADDITIONS):CodeCache::getUnlinkedGlobalCodeBlockcalls it right aftercacheBytecode(), with the block it just generated and the key it was generated under.Why
Bun's node compile cache (
module.enableCompileCache()) currently persists a missed module by re-parsing it on a second VM at exit and eagerly compiling every function. The live tree on the main VM already has the top-level block plus the code blocks of every function that ran, andencodeCodeBlock(vm, key, block)writes exactly that. The only thing missing was a way for the provider to get hold of the block:cacheBytecode()passes a generator that encodes immediately, before any function has been compiled — for a CommonJS module that is just the wrapper stub.With this hook Bun keeps a
Weakto the block and encodes it once, before its own GC drops unlinked code or at exit. No rooting, no second VM, no incrementaladdFunctionUpdatelayout, noleafExecutables()use — so this composes with #490.Bun side: oven-sh/bun#40174.
No behavior change for anything not built with
BUN_JSC_ADDITIONS; the default implementation is a no-op.