Bytecode cache: store the metadata table as entry counts so another ABI can read it - #389
Closed
dylan-conway wants to merge 39 commits into
Closed
Bytecode cache: store the metadata table as entry counts so another ABI can read it#389dylan-conway wants to merge 39 commits into
dylan-conway wants to merge 39 commits into
Conversation
…form The cache is decoded by Bun executables cross-compiled for a different OS/CPU than the one that produced it, but two things in it depended on the encoder's C++ ABI: - CachedMetadataTable serialized the finalized metadata offset table, whose entries are byte offsets computed from sizeof(Op::Metadata). Those sizes differ between ABIs (everything embedding CallLinkInfo is 16 bytes larger under MSVC), so a table encoded on Linux/macOS and linked on Windows had LLInt indexing past each entry. Serialize per-opcode entry counts instead and lay the table out again on decode via the normal finalize() path. - CachedStringSourceProvider's Bun-only m_sourceLength sat in the base class's tail padding under Itanium but not under MSVC. It didn't validate anything SourceCodeKey doesn't already (the key compares the source hash), so drop it and hand back the provider being decoded against. Also make the encoder's output a pure function of its input so it can be compared across platforms: fixed allocation alignment and page size instead of alignof(max_align_t)/pageSize(), zeroed pages so struct padding is not heap garbage, string switch tables encoded in index order rather than StringImpl-pointer hash order, and UnlinkedHandlerInfo encoded field-wise rather than copied with 30 bits of bit-field slack. The two allocation entry points now static_assert what the compiler can check about a type's portability (alignment, no destructor/array cookie, and for verbatim-copied types: no padding or bit-field slack, no long/wchar_t/long double).
The verbatim-copy check rejected `long` by name to catch the LP64/LLP64 size difference, but int64_t (EncodedJSValue) is `long` under glibc. Only reject `long` where it is the 32-bit outlier.
Preview Builds
|
…agree with Bun embeds this cache in executables it cross-compiles, so the bytes are decoded by a build whose OS, CPU and C++ ABI differ from the encoder's. Two things in the format depended on the encoder: - The metadata table was stored as offsets (lately as offset deltas), which are sums of this build's sizeof(Op::Metadata); anything embedding CallLinkInfo is 16 bytes larger under MSVC, so a table written on Linux/macOS and linked on Windows had LLInt index past every entry. The cache now stores per-opcode entry counts and UnlinkedMetadataTable::layOut() -- the one function that turns counts into offsets, shared with finalize() -- runs on the decoding side (at link() for a persistent payload, as before). - Records are placed in the payload as C++ objects, so any byte the ABI is free to place (tail padding Itanium reuses and MSVC does not, bit-field units MSVC restarts on a type change, holes after a bool/uint8_t/uint16_t member) can land differently. Every type now enters the Encoder through malloc<T> / mallocArray<T> / mallocCopy<T>, which static_assert isPortableRecord<T>(): trivially destructible, alignment <= 8, no long/wchar_t/pointers, and has_unique_object_representations -- i.e. no padding and no bit-field slack, so the layout is fully determined by the declared field sizes. The dozen records that had holes get their fields reordered/widened or an explicit zeroed m_padding; UnlinkedHandlerInfo, VariableEnvironmentEntry, PrivateNameEntry and SymbolTableEntry are encoded through Cached* records instead of copied; CachedStringSourceProvider's Bun-only m_sourceLength is gone (SourceCodeKey already compares hash and length). And the output is made a pure function of the source so that platforms can be compared byte for byte: allocation alignment and encoder page size are constants instead of max_align_t/pageSize(), and hash maps/sets and TDZ environments are written in key order instead of table order (which follows SymbolImpl's per-process hash counter, or a robin-hood table's own address).
dylan-conway
changed the title
…displacement, as the table's last entry does totalSize() of a table backed by persistent entry counts used it as-is and under-allocated the linked buffer by s_offset32TableSize for functions large enough to need 32-bit offsets (ASAN: heap-buffer-overflow writing OpPutToScope::Metadata in CodeBlock::finishCreation).
…tanding in the runtime provider Now a plain uint32_t after a base without tail padding, so both ABIs place it at the same offset (the reason the earlier revision dropped it).
…ings of one source are identical
…a payload cannot wrap it; EncodingOrder: order equal-content keys by symbol kind
… operand order, not argument-evaluation order op_iterator_open, op_iterator_next, op_async_iterator_open and op_instanceof took their two or three value-profile indices as nextValueProfileIndex() calls in one emit() argument list, so which operand got which index was up to the compiler's argument evaluation order: left-to-right under clang for SysV targets, right-to-left under the MSVC ABI. The bytecode was equivalent either way, but not byte-identical across platforms, which the bytecode cache now promises.
Two encodings of the same source could differ: - hash maps/sets and TDZ environments were written in table order, which for SymbolImpl keys follows a per-process hash counter and for robin-hood tables (string switch jump tables) the table's own address; they are now written in key order (EncodingOrder); - UnlinkedHandlerInfo was copied with the 30 unspecified bits next to its 2-bit HandlerType; it is encoded field by field (CachedHandlerInfo); - allocation alignment was alignof(std::max_align_t) and encoder pages were pageSize(), both of which vary by platform and show up as padding; they are constants; - op_iterator_open/op_iterator_next/op_async_iterator_open/op_instanceof took their value-profile indices as several nextValueProfileIndex() calls in one argument list, so which operand got which index followed the compiler's argument evaluation order (reversed under the MSVC ABI); they are sequenced. jsc shell: bytecodeCacheFor(source, kind) returns the payload as a Uint8Array; JSTests/stress/bytecode-cache-deterministic.js encodes a corpus twice and compares.
…SourceProviderCache transparent to what gets encoded Records: everything enters the Encoder through malloc<T> / mallocArray<T> / mallocCopy<T>, which static_assert isPortableRecord<T>() -- fixed-width scalars, no padding, no bit-field slack (has_unique_object_representations) -- so every byte in the payload is a named, initialized field and a record's layout follows from its declaration alone. The records that had holes get fields reordered/widened or explicit zeroed m_padding; VariableEnvironmentEntry, PrivateNameEntry and SymbolTableEntry go through Cached* records. Parser (from #433, plus one more case): whether a nested function's body was skipped through the SourceProviderCache changed what the parser recorded for it and for what follows it, and a full collection part-way through generation could drop that cache, so the payload depended on GC timing: - an expression-bodied arrow function's end line/column came from the token after the body when parsed but from the body's last token when skipped; both now use the last token; - NoEvalCacheFeature for a tagged template inside a skipped body was lost; the cache item records and replays it; - after skipping a body whose last token spans lines (a template literal) the lexer resumed on the line that token started on, so every later function's line numbers were short by the lines it spanned (visible in stack traces, not only in the cache); the cache item records where the token ended; - recursivelyGenerateUnlinkedCodeBlock() and the builtin-executable path hold the provider's cache for their duration, and VM::clearSourceProviderCaches() keeps caches that are in use. The stress test also runs under --collectContinuously=1 and --useSourceProviderCache=0.
…le-bytecode-cache
…(verifyLayout() on ARM64 release)
…le-bytecode-cache
…, registry-symbol hashes, atom width; and ** folding through libm
- Parser: sloppy-mode block function hoisting candidates were a HashMap keyed
by FunctionMetadataNode* and iterated to declare the hoisted vars, so their
registers / scope offsets followed arena addresses (ASLR): two runs of
'{ function a(){} function b(){} }' inside any sloppy function encoded
differently. Kept in declaration order now.
- SymbolImpl: registered and private symbols hashed by a process-global
counter, so a VariableEnvironment holding computed-class-field keys (or
builtin @NAMEs) past its inline capacity iterated -- and the generator
assigned scope offsets -- in an order set by how many symbols the process had
made. They hash by contents (as StaticSymbolImpl already does); plain
Symbol() keeps the counter.
- Encoder: a Latin-1 string is written 8-bit even when this thread's atom for
it is 16-bit because an equal 16-bit string was atomized first; and
CachedImmutableButterfly stores indexingMode(), not the cell-lock bits that
share its byte.
- ASTBuilder folds 'a ** b' only on operationMathPow's exact integer-exponent
path; the other cases call the C library's pow(), which rounds differently
across platforms, and left unfolded they get the pow() of where the code runs.
- Parser::restoreLexerState restores the last token's end line too.
Stress-test corpus gains block functions, >9 computed class fields and pow
folds; on main it encodes differently on every run.
…le-bytecode-cache
…verifyLayout() on ARM64 release)
…le-bytecode-cache
…e:31 like the rest of the struct
…le-bytecode-cache
…der whose CPU reserves a different number treats the payload as foreign BytecodeGenerator numbers a code block's locals after the LLInt/baseline callee-save area, whose size comes from the encoding CPU's register set. It is 4 registers on x86-64 and arm64 alike, which is what makes payloads interchangeable between them; this makes that dependency explicit instead of coincidental.
…le-bytecode-cache
Cross-decoding needs exactly two things, and this branch is now only those: - records placed in the payload have one layout under every ABI: everything enters the Encoder through malloc<T>/mallocArray<T>/mallocCopy<T>, which static_assert isPortableRecord<T>() (fixed-width scalars, no padding, no bit-field slack), and the records that had holes are corrected; - the metadata table is stored as per-opcode entry counts, and UnlinkedMetadataTable::layOut() computes offsets on the decoding side, because sizeof(Op::Metadata) belongs to the decoder. Plus the cache entry recording the encoder's reserved callee-save locals. The determinism work (hash-table order, encoder constants, parser / SourceProviderCache, symbol hashing, Annex B order, ** folding, the jsc test) moves out to #505; none of it is required for a payload to decode correctly on another platform.
dylan-conway
changed the title
…arries an entry count Keep UnlinkedMetadataTable and CachedMetadataSteps exactly as on main except: - a step is (opcode << 24 | entry count) instead of (index << 24 | offset delta); - expandSteps() lays counts out with this build's metadataSize()/metadataAlignment(), the same loop finalize() runs, and returns the end offset (so totalSize() and the 16/32-bit decision come from it instead of from the encoder); - CachedMetadataSteps::compute() is finalize()'s inverse, checked against the original table in debug builds. finalize() itself is untouched.
CachedTypes.cpp starts again from main and only adds: isPortableRecord<T>() and a static_assert of it in each of main's existing typed entry points into the payload (Encoder::malloc<T>/mallocFor<T>, VariableLengthObject::allocate<T>/ allocateFor<T>, the trivially-copyable CachedArray/CachedVector paths, encodeArrayForTail<T>); the padding/widening those asserts demand in the records that fail them; CachedMetadataSteps as (opcode, count); the callee-save check in GenericCacheEntry. Encoder's allocation API is main's, unchanged.
…and the VariableEnvironment.h friend) go away CachedPair<First, Second> gets explicit, zero-cost-when-empty padding members sized from First/Second, which makes every pair padding-free regardless of the value type; VariableEnvironmentEntry, PrivateNameEntry and SymbolTableEntry are stored as on main again.
… through the checked malloc<T>/mallocFor<T>/mallocArray<T> That check then covers the program/module code block records (allocated raw before), whose CachedVariableEnvironment had two padding bytes after its bools.
…y<CachedJSValue> for the constant pool (kind bytes as head), mallocCopy for the packed ExpressionInfo bytes
…into this process)
Drop the layout rule (isPortableRecord, the typed allocators, the struct padding changes): the records lay out the same on every 64-bit target we build for today, and Bun's cross-platform golden-hash test is what catches a future divergence. What remains is the one thing that is actually different across ABIs -- the metadata table's offsets, computed from the encoder's sizeof(Op::Metadata) -- now stored as per-opcode entry counts and laid out by the decoder, plus the callee-save-locals check in the entry.
dylan-conway
changed the title
… word in release builds too
Member
Author
|
Folded into #505, which now carries the metadata-table and callee-save changes together with the determinism work and a build-time check that the cache's record layouts agree across platforms. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Makes a bytecode cache payload readable by a build for a different OS/ABI than the one that wrote it, which is what
bun build --compile --bytecode --target=<other platform>produces (oven-sh/bun#18416: such executables crash on launch today).The one thing in the format that depends on the encoder's build is the metadata table: it was stored as a table of byte offsets, i.e. running sums of the encoder's
sizeof(Op::Metadata)/alignof(Op::Metadata), and several of those structs are larger under the MSVC ABI. It is now stored as per-opcode entry counts (opcode << 24 | count, only for opcodes that have entries), and the decoder lays the table out with its own sizes —UnlinkedMetadataTable::expandSteps(), which isfinalize()'s loop factored out so the two cannot drift.createFromPersistentSteps()(payload used in place) computes the offsets the same way. TheLayoutMetadataIs32Bitflag goes away since 16- vs 32-bit offsets is now the decoder's decision.Also records
CodeBlock::llintBaselineCalleeSaveSpaceAsVirtualRegisters()in the cache entry and rejects a payload where it differs: it is the one property of the encoding CPU that generated bytecode depends on (equal on x64 and arm64, so this never fires today).The
Cached*record structs are not touched: they lay out identically on every 64-bit target we build (nolong, no mixed-type bit-fields, no members placed in a base's tail padding). Rather than guard against hypothetical future divergence here, Bun's cross-platform golden-hash test (oven-sh/bun#40270, with #505 for determinism) compares the bytes every CI platform produces for a corpus covering every record type and opcode; a layout difference on any platform shows up there.3 files, +79/−42. JSTests/stress under the bytecode-cache helper (and with
--diskCachePayloadIsPersistentForTesting): same results asmain.