-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Compiled executables: alias embedded bytecode instead of copying it; smaller, page-friendly bytecode (WebKit#494) #40201
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
48 commits
Select commit
Hold shift + click to select a range
f7d2f84
Mark bytecode embedded in a compiled executable as persistent so JSC …
Jarred-Sumner 0cc77af
[autofix.ci] apply automated fixes
autofix-ci[bot] a324bf0
Standalone modules come through the builtin path: mark their bytecode…
Jarred-Sumner 648960c
Record each embedded module's source hash at build time so loading it…
Jarred-Sumner 28e2abf
Pin WebKit#494 head (per-block checksums)
Jarred-Sumner a858d77
Pin WebKit#494 head
Jarred-Sumner 6c249b8
Merge remote-tracking branch 'origin/main' into claude/bytecode-cache…
Jarred-Sumner d73aa70
[autofix.ci] apply automated fixes
autofix-ci[bot] e300c28
Pin WebKit#494 head (cpuid fix)
Jarred-Sumner fb82aa3
Pin WebKit#494 head
Jarred-Sumner 53b434b
Pin WebKit#494 head
Jarred-Sumner 4365029
Pin WebKit#494 head
Jarred-Sumner 8418a9e
ci: retrigger (WebKit preview 738c4bc9 published)
Jarred-Sumner 897a92c
bun-build-compile test: size the payload check off the source, not an…
Jarred-Sumner 6f683bb
bun-build-compile residency test: include the mapping's smaps fields …
Jarred-Sumner e61ee8f
Drop the payload-residency test: Rss of a just-written executable's m…
Jarred-Sumner 517e48b
Bump WebKit to 024831d80fa0 (bytecode cache: borrow, region layout, c…
Jarred-Sumner 2de1d82
Remove stray comment left from the dropped residency test
Jarred-Sumner 4861e28
ResolvedSource: say explicitly when bytecode_cache is never freed (ex…
Jarred-Sumner 2384d71
Mark compile-cache bytecode static on the other two paths that use it
Jarred-Sumner 909fa9d
Bump WebKit to 62f427b86ffb (bytecode cache: arrays-first records, st…
Jarred-Sumner 7aa74fd
Bump WebKit to fc1a8df1bba4 (bytecode cache: inline short strings, al…
Jarred-Sumner fa6967c
bun build --compile --bytecode: embed bytecode for the internal modul…
Jarred-Sumner a85069f
[autofix.ci] apply automated fixes
autofix-ci[bot] 6a80255
clippy: safety comment placement
Jarred-Sumner a004f52
clippy: raw borrows for FFI out-params
Jarred-Sumner 0199849
clippy: FFI accessor writing out-params is unsafe extern
Jarred-Sumner 7161059
[autofix.ci] apply automated fixes
autofix-ci[bot] 4d0a023
Bump WebKit to aff53044c546 (bytecode cache: builtin function entries…
Jarred-Sumner c815d63
Review: atomic counter, WebCore::bunVM, overflow-safe bounds check, t…
Jarred-Sumner edcdf3f
[autofix.ci] apply automated fixes
autofix-ci[bot] 0fa8e4b
bunVM is at global scope
Jarred-Sumner 0fbe69a
Review: skip internal-module bytecode when cross-compiling (the targe…
Jarred-Sumner 69d4338
[autofix.ci] apply automated fixes
autofix-ci[bot] ef7ea14
clippy: is_none_or
Jarred-Sumner 63751e8
Merge remote-tracking branch 'origin/main' into claude/bytecode-cache…
Jarred-Sumner cfbc61a
[autofix.ci] apply automated fixes
autofix-ci[bot] bf8365d
Merge origin/main into claude/bytecode-cache-borrow
Jarred-Sumner 0e1a0d0
Bump WebKit to ab29fdebb46292014bf2db3171fa49ab88d83e0e
Jarred-Sumner 708d0e8
Review: compact chunk names get a '_' prefix so they cannot collide w…
Jarred-Sumner 75f57b6
[autofix.ci] apply automated fixes
autofix-ci[bot] 5f9ba2d
bun build --compile --bytecode: one shared string table across every …
Jarred-Sumner 60b62b3
clippy: ptr cast
Jarred-Sumner 245d0ba
Review: own the EncoderStringTable with a Drop guard so an early retu…
Jarred-Sumner 511a3f6
ci: retrigger (WebKit 5803c87d release is published)
Jarred-Sumner c718748
Internal-module bytecode shares the build's EncoderStringTable; bump …
Jarred-Sumner e739e5d
Bump WebKit to c148a12dd82b (skip per-region checksums for persistent…
Jarred-Sumner 22432d8
Merge branch 'main' into claude/bytecode-cache-borrow
Jarred-Sumner File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡
EncoderStringTableHandle::Dropcalls__bun_jsc_encoder_string_table_take(table)and drops the result — but_takedoesEncoderStringTable::serialize(table)(walk the WTF hash map, copy every interned string into a freshVec<u8>, box it) beforedestroy(table), so the error path this guard exists for allocates and immediately discards the whole serialized buffer. Add a thin#[no_mangle] __bun_jsc_encoder_string_table_destroywrapper overEncoderStringTable::destroyin CachedBytecode.rs, declare it inbundle_v2::dispatch, and call that fromDropinstead. Error-path-only wasted allocation; no correctness impact.Extended reasoning...
What the issue is
The
Dropimpl added atsrc/bundler/bundle_v2.rs:1494-1500(in response to the earlier leak review) frees the C++JSC::EncoderStringTableon early return by calling the only teardown symbol declared inbundle_v2::dispatch:But
__bun_jsc_encoder_string_table_take(CachedBytecode.rs:163-167) is not a plain destructor — it serializes first:EncoderStringTable::serializecallsBun__EncoderStringTable__serialize(ZigSourceProvider.cpp), which doestable->serialize()— walking the WTF hash map and copying every interned string into aVector<uint8_t>— thenextend_from_slices that into a RustVec<u8>, then.into_boxed_slice(). On theDroppath that entireBox<[u8]>is immediately dropped.The specific code path
The handle is created in
generate_chunks_in_parallel(generateChunksInParallel.rs:622-624) whenevergenerate_bytecode_cache && compile_mode.is_executable(). On the success tail,.take()consumes the handle (nullingself.0) soDropis a no-op.Droponly does work when anErrpropagates between creation and the tail — e.g.intermediate_output.code(...)?oroutput_files.insert_for_sourcemap_or_bytecode(f)?returningErrafter some chunks have already interned strings into the table.Why existing code doesn't prevent it
bundle_v2::dispatchdeclares only twoEncoderStringTableexterns —_newand_take— so_takeis the only teardown symbol reachable from that crate.EncoderStringTable::destroyexists inbun_jsc(and the C++Bun__EncoderStringTable__destroyis already defined in ZigSourceProvider.cpp), but there is no#[no_mangle]Rust wrapper forbun_bundlerto link against, so the author reused_takeas the simplest available fix to the earlier leak review.Step-by-step proof
Bun.build({ compile: { outfile }, bytecode: true })→external_string_table = Some(EncoderStringTableHandle::new());self.0 = Some(ptr)whereptrpoints at a heap-allocatedJSC::EncoderStringTable.encodeCodeBlock(..., externalStrings)interns N strings (potentially many MB across a large bundle) into*ptr.intermediate_output.code(...)?returnsErr(e);?propagates.external_string_table: Option<EncoderStringTableHandle>is dropped →EncoderStringTableHandle::dropruns withself.0 = Some(ptr).__bun_jsc_encoder_string_table_take(ptr)→serialize(ptr)walks the hash map, allocates aVector<uint8_t>on the C++ side, copies every string, thenextend_from_slicecopies again into a RustVec, theninto_boxed_slice()→destroy(ptr).Box<[u8]>is immediatelydrop()ped — the serialization work and both allocations were pure waste.Impact
Zero correctness impact — no leak, no UAF, no observable behavior change. Error-path-only (an
Errpropagated mid-link during a--compile --bytecodebuild), happens once per failing build, and the CLI case exits immediately anyway. The only case where it accumulates is repeated failingBun.build({ compile, bytecode })calls in a long-lived process, and even there it's a transient allocation, not a leak. This is a minor efficiency point.Fix
Add one
#[no_mangle]wrapper insrc/jsc/CachedBytecode.rsnext to_take:declare it in the
unsafe extern "Rust"block inbundle_v2::dispatch, and changeDropto: