JSC: add a bytecode cache entry for UnlinkedFunctionExecutable - #270
JSC: add a bytecode cache entry for UnlinkedFunctionExecutable#270robobun wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 34 minutes Limit details: You’ve used all 5 included reviews currently available under your plan. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling 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 (4)
WalkthroughThis PR adds a dedicated bytecode cache entry for ChangesFunction executable cache entry
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Source/JavaScriptCore/runtime/CachedTypes.cpp (1)
2253-2274: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winNew Bun-motivated cache format is unguarded by
USE(BUN_JSC_ADDITIONS).This entire feature (
CachedFunctionExecutableTag, thetagFromSourceCodeTypecase,FunctionExecutableCacheEntry, andencodeFunctionExecutable/decodeFunctionExecutable) exists purely so Bun can cache bytecode for builtins — it has no consumer in stock JSC. Elsewhere in this same file, comparable Bun-specific behavior (CachedStringSourceProvider's source-reuse optimization) is wrapped in#if USE(BUN_JSC_ADDITIONS). This new addition doesn't follow that convention anywhere in the diff.Consider wrapping the new tag, dispatch cases,
FunctionExecutableCacheEntry, and theencodeFunctionExecutable/decodeFunctionExecutablefree functions (also declared inCachedTypes.h) in the same guard for consistency and easier upstream rebasing.As per coding guidelines, "Guard Bun-specific features with USE(BUN_JSC_ADDITIONS) and event-loop integration with USE(BUN_EVENT_LOOP)" for
Source/JavaScriptCore/**/*.{cpp,h}.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/JavaScriptCore/runtime/CachedTypes.cpp` around lines 2253 - 2274, The new Bun-only bytecode cache support is currently unguarded, so wrap the `CachedFunctionExecutableTag` addition, the `tagFromSourceCodeType` `FunctionType` dispatch, `FunctionExecutableCacheEntry`, and the `encodeFunctionExecutable`/`decodeFunctionExecutable` declarations and definitions in `USE(BUN_JSC_ADDITIONS)` just like the existing Bun-specific `CachedStringSourceProvider` logic. Keep the guard consistent across `CachedTypes.cpp` and `CachedTypes.h` so stock JSC does not compile or expose the Bun-only cache format.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Source/JavaScriptCore/runtime/CachedTypes.cpp`:
- Around line 2253-2274: The new Bun-only bytecode cache support is currently
unguarded, so wrap the `CachedFunctionExecutableTag` addition, the
`tagFromSourceCodeType` `FunctionType` dispatch, `FunctionExecutableCacheEntry`,
and the `encodeFunctionExecutable`/`decodeFunctionExecutable` declarations and
definitions in `USE(BUN_JSC_ADDITIONS)` just like the existing Bun-specific
`CachedStringSourceProvider` logic. Keep the guard consistent across
`CachedTypes.cpp` and `CachedTypes.h` so stock JSC does not compile or expose
the Bun-only cache format.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ea70c59a-ed76-4280-939e-313c43fe7497
📒 Files selected for processing (4)
Source/JavaScriptCore/runtime/CachedTypes.cppSource/JavaScriptCore/runtime/CachedTypes.hSource/JavaScriptCore/runtime/CodeCache.cppSource/JavaScriptCore/runtime/CodeCache.h
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any issues and the implementation closely mirrors the existing CacheEntry/decodeCodeBlockImpl patterns, but this adds a new top-level serialization format to JSC's bytecode cache with no in-tree tests, so it's worth a human familiar with CachedTypes giving it a look.
Extended reasoning...
Overview
This PR adds a new top-level bytecode cache entry type for UnlinkedFunctionExecutable, so Bun can precompile and cache its JS builtins (whose top-level compilation unit is a builtin function rather than a program/module). It touches four files in Source/JavaScriptCore/runtime/: a new CachedFunctionExecutableTag enum value and FunctionExecutableCacheEntry class in CachedTypes.cpp, exported encodeFunctionExecutable/decodeFunctionExecutable entry points, a recursivelyGenerateUnlinkedCodeBlockForFunctionExecutable helper in CodeCache.cpp, and a sourceCodeKeyForSerializedFunctionExecutable key builder. Roughly 130 net new lines.
Security risks
Bytecode cache decoding is inherently security-relevant since it interprets a serialized binary blob. The new decodeFunctionExecutable follows the same defensive shape as decodeCodeBlockImpl: it checks a minimum size, goes through GenericCacheEntry::isUpToDate (version stamp), verifies the tag before bit_casting to the derived type, decodes under DeferGC, and rejects on SourceCodeKey mismatch. In Bun's intended use the blobs are self-generated at build time (trusted), and a mismatch falls back to parsing from source. I don't see a new attack surface beyond what the existing program/module cache already exposes, but this is exactly the kind of code where a second pair of eyes on layout/alignment and the bit_cast dispatch is valuable.
Level of scrutiny
Medium-high. The change is additive and pattern-matched against existing CacheEntry<T> machinery (including the static_assert(alignof(...) <= alignof(std::max_align_t))), and the three tag switches each gain a well-behaved arm. However, it introduces a new persisted binary format in core JSC, relies on std::bit_cast between base/derived cache-entry pointers, and wires SourceCodeType::FunctionType into tagFromSourceCodeType where it previously fell through to ASSERT_NOT_REACHED. That's more than a mechanical or config-level change.
Other factors
- No in-tree JSC tests exercise the new encode/decode path; verification is described as happening on the Bun side in a follow-up PR.
- No prior reviewer comments on the PR to consider.
- The bug-hunting pass found nothing.
Given the scope (new serialization format in a core subsystem) and lack of in-tree coverage, I'm deferring rather than auto-approving.
Preview Builds
|
|
This duplicates #177 by @sosukesuzuki, which I found only after opening it. Same four files, same idea, arrived at independently. I'm happy to close this in favour of that one. Before that, the one place they diverge, in case it's worth folding back into #177. #177's This PR makes the entry a real Either shape works. Whichever survives, the key check seems worth having, so that callers can't silently decode bytecode built from source they no longer have. Consumer on the Bun side: oven-sh/bun#33316 (Rust), which is the successor to oven-sh/bun#28461 (Zig, unrebasable after the Rust rewrite). |
A compiled binary already ships bytecode for the app's own modules, but every builtin it touches (node:net, node:fs, the internal/* modules they require) is still parsed from source on first use. That is the bulk of the parse work left at startup. Builtins are compiled with createBuiltinExecutable() into an UnlinkedFunctionExecutable, which the bytecode cache had no entry type for; oven-sh/WebKit#270 adds encodeFunctionExecutable()/decodeFunctionExecutable() for exactly this. WEBKIT_VERSION points at that PR's preview build for now and must be re-pointed at the merged main sha before landing. Unlike the other bytecode generation steps, this one has to walk a graph the bundler cannot see. The bundle's import records name only the builtins the app imports directly; those then require() each other through InternalModuleRegistry by numeric id, resolved at runtime. So codegen emits the adjacency of those @createInternalModuleById(N) edges, and caching "net" means caching node:net plus every internal/* module it transitively reaches. The entries are embedded in the standalone module graph and looked up by module id on first require. JSC validates each one against a cache version and a SourceCodeKey over the builtin's source, and the decoded executable keeps its m_isBuiltinFunction bit, so a rejected or missing entry just parses in builtin mode exactly as before. Cross-compiles skip generation entirely: src/js is specialized per platform at Bun's own build time, so the key would never match. Measured on a debug build running require("node:net") + node:http + node:fs: ~1.3s to ~0.77s startup, +5.8MB binary.
d87c5e8 to
321befd
Compare
There was a problem hiding this comment.
I didn't find any bugs, but this adds a new top-level bytecode-cache entry type and decode path in JSC's core serialization layer, and there's an open coordination question with #177 — worth a human look before landing.
What was reviewed:
FunctionExecutableCacheEntrylayout/methods vs. the existingCacheEntry<T>template — same field order, same key/tag/version validation.decodeFunctionExecutable()mirrorsdecodeCodeBlockImpl()(size guard,DeferGC, key equality check); tag mismatch cleanly returnsfalse/nullptr.- New
CachedFunctionExecutableTagarm added to all threeGenericCacheEntryswitches;tagFromSourceCodeType(FunctionType)was previously unreachable so no existing caller changes behavior. recursivelyGenerateUnlinkedCodeBlockForFunctionExecutable()reuses the existinggenerateUnlinkedCodeBlockForFunctions()recursion.
Extended reasoning...
Overview
This PR adds a fourth top-level bytecode-cache entry shape to CachedTypes.cpp: FunctionExecutableCacheEntry, tagged CachedFunctionExecutableTag, which serializes an UnlinkedFunctionExecutable (plus its nested code-block tree) behind a versioned GenericCacheEntry header with a CachedSourceCodeKey. It exports encodeFunctionExecutable() / decodeFunctionExecutable() and two CodeCache.cpp helpers (sourceCodeKeyForSerializedFunctionExecutable(), recursivelyGenerateUnlinkedCodeBlockForFunctionExecutable()). Roughly 130 lines across four files, all additive.
Security risks
Bytecode-cache deserialization is inherently sensitive: a mismatched or hostile blob that passes validation is treated as trusted bytecode. The mitigations here match the existing program/module paths exactly — isUpToDate() checks the JSC cache version, m_tag is checked before bit_cast, and the decoded SourceCodeKey (which hashes the source) must equal the caller's key. decodeFunctionExecutable() also has a sizeof(FunctionExecutableCacheEntry) lower-bound guard. I don't see a new attack surface beyond what decodeCodeBlockImpl() already exposes, but this is the kind of code where a second set of eyes on the bit_cast layout assumptions and the CachedPtr<CachedFunctionExecutable> decode path is warranted.
Level of scrutiny
Medium-high. The implementation is a near-verbatim copy of CacheEntry<T> and decodeCodeBlockImpl(), which lowers risk considerably, and none of the new code is reachable from existing JSC callers (only Bun's forthcoming consumer will call the new exports). But it's still ~130 lines of hand-written serialization in the JSC runtime, not a config tweak or mechanical change.
Other factors
The author flagged that this duplicates #177 and offered to close in favour of it, while noting a design difference (this PR validates the SourceCodeKey, #177 validates only the version stamp). Which shape lands — and whether the key check should be folded into #177 instead — is a coordination decision a maintainer should make, not a bot. There are no in-tree tests for the new path; the round-trip is exercised only from the downstream Bun PR.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Source/JavaScriptCore/runtime/CachedTypes.cpp (1)
1-1: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftWrap the new function-executable cache feature in
USE(BUN_JSC_ADDITIONS). This entire feature (CachedFunctionExecutableTag,FunctionExecutableCacheEntry,encodeFunctionExecutable/decodeFunctionExecutable,sourceCodeKeyForSerializedFunctionExecutable,recursivelyGenerateUnlinkedCodeBlockForFunctionExecutable) exists solely so Bun can cache builtin bytecode fornode:net/node:fs/internal/*— it is not a general WebKit/JSC requirement, yet none of it is conditionally compiled, unlike the existing Bun-specificCachedStringSourceProviderlogic in the same file (which is wrapped in#if USE(BUN_JSC_ADDITIONS)).
Source/JavaScriptCore/runtime/CachedTypes.cpp#L2654-2701: wrapFunctionExecutableCacheEntry, the new tag value/tagFromSourceCodeTypecase, the newGenericCacheEntrydecode overloads and dispatch cases, andencodeFunctionExecutable/decodeFunctionExecutablein#if USE(BUN_JSC_ADDITIONS).Source/JavaScriptCore/runtime/CachedTypes.h#L131-143: guard theencodeFunctionExecutable/decodeFunctionExecutabledeclarations with the same macro (or provide stub/no-op declarations for non-Bun builds).Source/JavaScriptCore/runtime/CodeCache.cpp#L328-349: guardsourceCodeKeyForSerializedFunctionExecutableandrecursivelyGenerateUnlinkedCodeBlockForFunctionExecutable.Source/JavaScriptCore/runtime/CodeCache.h#L269-278: guard the matching declarations.As per coding guidelines, "Guard Bun-specific features with USE(BUN_JSC_ADDITIONS) and event-loop integration with USE(BUN_EVENT_LOOP)".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/JavaScriptCore/runtime/CachedTypes.cpp` at line 1, The new function-executable cache feature is compiled unconditionally instead of only for Bun. Wrap FunctionExecutableCacheEntry, its tag and GenericCacheEntry encode/decode dispatch, and encodeFunctionExecutable/decodeFunctionExecutable in CachedTypes.cpp; guard their declarations in CachedTypes.h. Similarly guard sourceCodeKeyForSerializedFunctionExecutable and recursivelyGenerateUnlinkedCodeBlockForFunctionExecutable, plus their declarations in CodeCache.cpp and CodeCache.h, with USE(BUN_JSC_ADDITIONS), preserving non-Bun builds without these Bun-specific APIs.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Source/JavaScriptCore/runtime/CachedTypes.cpp`:
- Around line 2654-2701: Guard the Bun-only FunctionExecutableCacheEntry class
and its CachedFunctionExecutableTag usage with `#if` USE(BUN_JSC_ADDITIONS),
matching the existing CachedStringSourceProvider guard pattern. Ensure non-Bun
JSC builds do not compile or reference this cache entry or tag, while preserving
the current behavior when the feature is enabled.
In `@Source/JavaScriptCore/runtime/CachedTypes.h`:
- Around line 131-143: Wrap the Bun-specific declarations
encodeFunctionExecutable and decodeFunctionExecutable in CachedTypes.h with the
USE(BUN_JSC_ADDITIONS) preprocessor guard, matching the corresponding guard in
CachedTypes.cpp and preserving their existing signatures.
In `@Source/JavaScriptCore/runtime/CodeCache.cpp`:
- Around line 328-349: Wrap both Bun-only functions,
sourceCodeKeyForSerializedFunctionExecutable and
recursivelyGenerateUnlinkedCodeBlockForFunctionExecutable, in the
USE(BUN_JSC_ADDITIONS) conditional guard, matching the guard used for the
corresponding functionality in CachedTypes.cpp. Keep their implementations
unchanged within the guarded block.
In `@Source/JavaScriptCore/runtime/CodeCache.h`:
- Around line 269-278: Wrap the Bun-specific declarations in
CodeCache.h—recursivelyGenerateUnlinkedCodeBlockForFunctionExecutable,
serializeBytecode, and the serialized source-code key helpers—in the same `#if`
USE(BUN_JSC_ADDITIONS) guard used by CachedTypes.cpp. Keep unrelated
declarations such as writeCodeBlock outside the guard.
---
Outside diff comments:
In `@Source/JavaScriptCore/runtime/CachedTypes.cpp`:
- Line 1: The new function-executable cache feature is compiled unconditionally
instead of only for Bun. Wrap FunctionExecutableCacheEntry, its tag and
GenericCacheEntry encode/decode dispatch, and
encodeFunctionExecutable/decodeFunctionExecutable in CachedTypes.cpp; guard
their declarations in CachedTypes.h. Similarly guard
sourceCodeKeyForSerializedFunctionExecutable and
recursivelyGenerateUnlinkedCodeBlockForFunctionExecutable, plus their
declarations in CodeCache.cpp and CodeCache.h, with USE(BUN_JSC_ADDITIONS),
preserving non-Bun builds without these Bun-specific APIs.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5a0c5b02-e8db-4057-99a8-1fe0f65a1358
📒 Files selected for processing (4)
Source/JavaScriptCore/runtime/CachedTypes.cppSource/JavaScriptCore/runtime/CachedTypes.hSource/JavaScriptCore/runtime/CodeCache.cppSource/JavaScriptCore/runtime/CodeCache.h
| // Eagerly compile `executable` and every function nested beneath it, so that the whole | ||
| // tree can be serialized with encodeFunctionExecutable(). `parentSource` is the source | ||
| // the executable was created from, not its own linked sub-range. | ||
| JS_EXPORT_PRIVATE UnlinkedFunctionCodeBlock* recursivelyGenerateUnlinkedCodeBlockForFunctionExecutable(VM&, UnlinkedFunctionExecutable*, const SourceCode& parentSource, ParserError&); | ||
|
|
||
| void writeCodeBlock(const SourceCodeKey&, const SourceCodeValue&); | ||
| RefPtr<CachedBytecode> serializeBytecode(VM&, UnlinkedCodeBlock*, const SourceCode&, SourceCodeType, LexicallyScopedFeatures, JSParserScriptMode, FileSystem::FileHandle&, BytecodeCacheError&, OptionSet<CodeGenerationMode>); | ||
| SourceCodeKey sourceCodeKeyForSerializedProgram(VM&, const SourceCode&); | ||
| SourceCodeKey sourceCodeKeyForSerializedModule(VM&, const SourceCode&); | ||
| JS_EXPORT_PRIVATE SourceCodeKey sourceCodeKeyForSerializedFunctionExecutable(VM&, const SourceCode&, const String& name); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Same missing Bun-feature guard as CachedTypes.cpp.
These declarations expose a Bun-only feature but aren't wrapped in #if USE(BUN_JSC_ADDITIONS). See consolidated comment for details across the affected files.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Source/JavaScriptCore/runtime/CodeCache.h` around lines 269 - 278, Wrap the
Bun-specific declarations in
CodeCache.h—recursivelyGenerateUnlinkedCodeBlockForFunctionExecutable,
serializeBytecode, and the serialized source-code key helpers—in the same `#if`
USE(BUN_JSC_ADDITIONS) guard used by CachedTypes.cpp. Keep unrelated
declarations such as writeCodeBlock outside the guard.
Source: Coding guidelines
A compiled binary already ships bytecode for the app's own modules, but every builtin it touches (node:net, node:fs, the internal/* modules they require) is still parsed from source on first use. That is the bulk of the parse work left at startup. Builtins are compiled with createBuiltinExecutable() into an UnlinkedFunctionExecutable, which the bytecode cache had no entry type for; oven-sh/WebKit#270 adds encodeFunctionExecutable()/decodeFunctionExecutable() for exactly this. WEBKIT_VERSION points at that PR's preview build for now and must be re-pointed at the merged main sha before landing. Unlike the other bytecode generation steps, this one has to walk a graph the bundler cannot see. The bundle's import records name only the builtins the app imports directly; those then require() each other through InternalModuleRegistry by numeric id, resolved at runtime. So codegen emits the adjacency of those @createInternalModuleById(N) edges, and caching "net" means caching node:net plus every internal/* module it transitively reaches. The entries are embedded in the standalone module graph and looked up by module id on first require. JSC validates each one against a cache version and a SourceCodeKey over the builtin's source, and the decoded executable keeps its m_isBuiltinFunction bit, so a rejected or missing entry just parses in builtin mode exactly as before. Cross-compiles skip generation entirely: src/js is specialized per platform at Bun's own build time, so the key would never match. Measured on a debug build running require("node:net") + node:http + node:fs: ~1.3s to ~0.77s startup, +5.8MB binary.
321befd to
ce6f209
Compare
|
Rebased onto The conflict GitHub was reporting is with #420, which adds a declaration at the same spot in One deliberate change while I was in here: everything is now under Still a duplicate of #177 in intent; the comparison in my earlier comment stands, and I'm still happy for either to be the one that lands. |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it adds new bytecode-cache serialization/deserialization machinery in CachedTypes.cpp and overlaps in intent with #177, a human look would still be worthwhile — both to sanity-check the encode/decode shape and to decide which of the two PRs should land.
What was reviewed:
FunctionExecutableCacheEntrylayout and methods against the existingCacheEntry<T>template — field order, key handling, andstatic_assert(alignof …)all match.decodeFunctionExecutableagainstdecodeCodeBlockImpl— sameDeferGCscope, same key comparison, plus an added size guard.- The three tag-dispatch switches — each gains a guarded
CachedFunctionExecutableTagarm; the code-block decode overload correctly returnsfalseon tag mismatch rather than asserting. - All additions are under
#if USE(BUN_JSC_ADDITIONS); with the flag off,tagFromSourceCodeType()keeps upstream'sbreak;forFunctionType.
Extended reasoning...
Overview
This PR adds a fourth top-level bytecode cache entry type to JSC so that an UnlinkedFunctionExecutable (the compilation unit for a JS builtin) can be serialized and later decoded as a self-validating cache entry. It touches four files: CachedTypes.{h,cpp} (new tag, new FunctionExecutableCacheEntry class deriving from GenericCacheEntry, three new switch arms, and encodeFunctionExecutable/decodeFunctionExecutable) and CodeCache.{h,cpp} (sourceCodeKeyForSerializedFunctionExecutable and recursivelyGenerateUnlinkedCodeBlockForFunctionExecutable). Roughly ~130 net lines, all additive and all under #if USE(BUN_JSC_ADDITIONS).
Security risks
Bytecode cache deserialization is security-relevant: a malformed or mismatched blob decoded into an executable could run arbitrary code. The new decode path reuses the existing GenericCacheEntry header (cache version + boot-session UUID) via isUpToDate(), checks the tag before bit_cast-ing to the concrete entry type, and rejects on SourceCodeKey mismatch — the same layered validation the program/module entries use. decodeFunctionExecutable also adds a cachedBytecode->size() < sizeof(FunctionExecutableCacheEntry) guard before touching the buffer. The intended input is bytecode produced by Bun itself for its own builtins, not user-supplied blobs. I don't see a new attack surface beyond what the existing code-block cache entries already expose.
Level of scrutiny
Medium-to-high. The change is almost entirely a pattern-copy of CacheEntry<T> and decodeCodeBlockImpl, and with USE(BUN_JSC_ADDITIONS) off the four files are byte-for-byte upstream, so risk to non-Bun builds is nil. But it is new serialization logic in a core JSC subsystem, has no in-repo callers or tests (the round-trip is exercised from Bun's side per the description), and there is an open design question — this PR and #177 solve the same problem with slightly different validation semantics, and the author explicitly defers to maintainers on which should land.
Other factors
The earlier CodeRabbit feedback about missing USE(BUN_JSC_ADDITIONS) guards has been addressed in ce6f209; every added line in the current diff is guarded. The one CodeRabbit inline on CodeCache.h that isn't marked resolved is in fact addressed — the two new declarations there sit inside a #if USE(BUN_JSC_ADDITIONS) block. Preview builds succeeded. Given the scope, the lack of in-repo test coverage, and the #177-vs-#270 decision that a maintainer needs to make, deferring to human review rather than auto-approving.
A compiled binary already ships bytecode for the app's own modules, but every builtin it touches (node:net, node:fs, the internal/* modules they require) is still parsed from source on first use. That is the bulk of the parse work left at startup. Builtins are compiled with createBuiltinExecutable() into an UnlinkedFunctionExecutable, which the bytecode cache had no entry type for; oven-sh/WebKit#270 adds encodeFunctionExecutable()/decodeFunctionExecutable() for exactly this. WEBKIT_VERSION points at that PR's preview build for now and must be re-pointed at the merged main sha before landing. Unlike the other bytecode generation steps, this one has to walk a graph the bundler cannot see. The bundle's import records name only the builtins the app imports directly; those then require() each other through InternalModuleRegistry by numeric id, resolved at runtime. bundle-modules.ts already derives that graph from the bundled output to lay the source blob out in dependency order; it now also emits it as an adjacency table, and caching "net" means caching node:net plus every internal/* module it transitively reaches. The entries are embedded in the standalone module graph and looked up by module id on first require, behind a flag the graph sets at startup so a Bun that embeds nothing never takes the lookup. JSC validates each entry against a cache version and a SourceCodeKey over the builtin's source; the runtime and the generator obtain that source and build that SourceCode through the same two functions, so a build's entries are always keyed on what the same build parses. A rejected or missing entry parses in builtin mode exactly as before. Cross-compiles skip generation: src/js is specialized per platform at Bun's own build time, so the key would never match. The bytecode VM only registers Bun's private names (JSVMClientData:: registerBuiltinNames) rather than creating full client data: the builtin-mode lexer needs the names, and nothing else in client data applies to a VM with no Bun VirtualMachine behind it. Measured on a debug build running require("node:net") + node:http + node:fs: ~1.3s to ~0.77s startup; 69 builtins served from the cache.
The bytecode cache has top-level entries for programs, modules and eval, because those are the top-level compilation units of a script. A JS builtin's top-level unit is a builtin function: it is compiled with createBuiltinExecutable() into an UnlinkedFunctionExecutable, whose code block is generated lazily and has no cache entry of its own. encodeFunctionCodeBlock() produces a bare CachedFunctionCodeBlock with no validation header, and the only thing that can attach one back to an executable is CachedFunctionExecutable, which is private to the cache implementation. Add CachedFunctionExecutableTag and a FunctionExecutableCacheEntry holding a SourceCodeKey plus a CachedPtr<CachedFunctionExecutable>, exposed as encodeFunctionExecutable()/decodeFunctionExecutable(). SourceCodeType::FunctionType already existed but was unreachable in tagFromSourceCodeType(); it now maps to the new tag, so isCachedBytecodeStillValid() works for these entries too. Decoding restores the executable's m_isBuiltinFunction bit, so an entry that is stale, rejected, or missing a nested code block falls back to parsing in builtin mode exactly as before. recursivelyGenerateUnlinkedCodeBlockForFunctionExecutable() compiles an executable and everything nested beneath it, which is what makes the whole tree serializable, and sourceCodeKeyForSerializedFunctionExecutable() builds the matching key. Everything is under USE(BUN_JSC_ADDITIONS), like the other fork additions in these files; with it off, the files are byte-for-byte upstream. No existing cache entry changes shape.
ce6f209 to
02b1d0a
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it adds a new top-level bytecode-cache entry type with its own serialization/deserialization path in JSC, and because there's still an open call on whether this or #177 is the one that should land, a human look would be worthwhile.
What was reviewed:
- New
FunctionExecutableCacheEntrylayout vs.GenericCacheEntrydispatch — samestd::bit_cast+static_assert(alignof)pattern asCacheEntry<T>, and the tag is checked before the cast. decodeFunctionExecutable()— size check,isUpToDate()version gate, tag gate, andSourceCodeKeyequality all reject before returning an executable;DeferGCscope matchesdecodeCodeBlockImpl().- All additions are under
USE(BUN_JSC_ADDITIONS); with the flag off,tagFromSourceCodeType()and the switch statements are byte-for-byte upstream.
Extended reasoning...
Overview
This PR adds a fourth top-level bytecode-cache entry shape to JSC's CachedTypes.cpp: a FunctionExecutableCacheEntry that serializes an UnlinkedFunctionExecutable (plus its nested code-block tree) as a self-validating GenericCacheEntry, alongside the existing program/module/eval entries. It exposes four new exported symbols — encodeFunctionExecutable(), decodeFunctionExecutable(), recursivelyGenerateUnlinkedCodeBlockForFunctionExecutable(), and sourceCodeKeyForSerializedFunctionExecutable() — and threads a new CachedFunctionExecutableTag through tagFromSourceCodeType() and the three GenericCacheEntry dispatch switches. Everything is guarded by #if USE(BUN_JSC_ADDITIONS) (added in ce6f209 after CodeRabbit feedback), so non-Bun builds are unchanged.
Security risks
Bytecode-cache deserialization is security-adjacent: the decoder reads a byte buffer produced elsewhere, bit_casts to a concrete entry type based on a tag, and materializes GC objects from it. The new path follows the existing CacheEntry<T> pattern closely — isUpToDate() (bytecode version) is checked first, then the tag, then the SourceCodeKey (source hash) — and there is a minimum-size check before the first bit_cast. In Bun's intended use the buffer is baked into the binary at build time, not read from an untrusted filesystem. I did not spot a way for a mismatched or truncated entry to get past the guards, but this is exactly the sort of code where a second pair of eyes on the layout assumptions and CachedPtr<CachedFunctionExecutable> decode path is worth having.
Level of scrutiny
Medium-high. The change is additive and feature-flagged, and it very deliberately reuses the existing encoder/decoder machinery rather than inventing new serialization. But it is ~150 lines of new C++ in JSC's runtime that create GC cells from serialized bytes, and the round-trip is only exercised from Bun's side (oven-sh/bun#33316), not by any test in this repo. That combination — core-runtime serialization plus out-of-tree test coverage — is past the threshold where I would auto-approve.
Other factors
The author has twice noted that this duplicates #177 in intent and is happy for either to land, with the substantive difference being that this variant validates the full SourceCodeKey (source hash) while #177 validates only the JSC bytecode version. Which of the two lands, or whether the key check should be folded into #177 instead, is a maintainer call rather than a correctness call, and it should be made by a human. All prior reviewer feedback (the USE(BUN_JSC_ADDITIONS) guards) has been addressed and those threads are resolved.
1 participant
Needed so Bun can ship a bytecode cache for its JS builtins (
node:net,node:fs, theinternal/*modules, …). Those are the last big chunk of parse work left in abun build --compile --bytecodebinary's startup, and today they are uncacheable.Why they are uncacheable
The bytecode cache has top-level entries for programs, modules and eval, because those are the top-level compilation units of a script. A builtin's top-level unit is a function:
createBuiltinExecutable()produces anUnlinkedFunctionExecutablewhose code block is generated lazily on first call. There is no cache entry shaped like that.encodeFunctionCodeBlock()exists, but it emits a bareCachedFunctionCodeBlockwith noGenericCacheEntryheader (no version stamp, no key), and the only thing that can attach a decoded code block back onto an executable isCachedFunctionExecutable, which is private toCachedTypes.cpp.Parsing the builtin as a program instead is not a workaround:
BytecodeGenerator'sProgramNodeconstructor hardcodesm_isBuiltinFunction(false), so every nested function comes back as a normal executable, and the first one that needs a re-parse (aCodeForConstructthat was never cached, anything cleared bydeleteAllUnlinkedCodeBlocks) hitsSyntaxError: Invalid character '@'.What this adds
CachedCodeBlockTag::CachedFunctionExecutableTagandFunctionExecutableCacheEntry, aGenericCacheEntryholding aSourceCodeKeyplus aCachedPtr<CachedFunctionExecutable>.encodeFunctionExecutable()/decodeFunctionExecutable(), mirroringencodeCodeBlock()/decodeCodeBlock().SourceCodeType::FunctionTypealready existed but was unreachable intagFromSourceCodeType(); it now maps to the new tag, soisCachedBytecodeStillValid()covers these entries.recursivelyGenerateUnlinkedCodeBlockForFunctionExecutable()— compiles an executable and everything nested beneath it, which is what makes the tree serializable. It's the function-shaped sibling ofrecursivelyGenerateUnlinkedCodeBlockForProgram().sourceCodeKeyForSerializedFunctionExecutable()— builds the matching key.Safety
Everything is additive: no existing entry changes shape, no existing code path changes behavior, and the three tag switches gain one arm each.
CachedFunctionExecutable::decode()already restoresm_isBuiltinFunction, so a decoded builtin is still a builtin. An entry that is stale, version-mismatched, key-mismatched, or missing a nested code block just falls back to parsing in builtin mode, exactly as today. That's the property the Program-shaped approach can't give you.Verification
Built
jsc(Debug,JSCOnly,USE_BUN_JSC_ADDITIONS=ON) and linked it into a debug Bun. The four new symbols resolve:The round-trip (generate a builtin's cache entry, decode it into a fresh executable, run the module) is exercised from Bun's side; a Bun PR consuming this follows once this lands in a release.