Bytecode cache: write each code block's records contiguously, bodies breadth-first, expression info last - #495
Conversation
…breadth-first, expression info last The encoder recursed into a nested function's body as soon as it wrote that function's executable record, so the N records a code block reads when it is decoded were interleaved with N whole subtrees. Decoding one block of a mapped payload touched pages across most of the file (measured: decoding only the top-level block of a 75 MB cache made 33 MB of it resident). The encoder now writes a block's own arrays and all of its children's executable records back to back, queues the children's bodies and emits them breadth-first, and emits every block's ExpressionInfo (read only when a stack trace is built) after all bodies. Offsets in the format are relative, so the decoder is unchanged and decoded objects are identical. Same 75 MB cache: top-level decode 32.7 MB resident / 143 ms cold -> 1.9 MB / 10 ms; one level deeper 50.4 MB / 280 ms -> 17.8 MB / 117 ms; full decode 785 ms -> 465 ms.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the include-ordering nit, I checked that the captured this pointers into encoder pages stay valid across allocateNewPage — each Page holds a heap-allocated MallocSpan buffer, so the buffer address is stable even when m_pages (a Vector<Page>) reallocates. Also confirmed both Encoder construction sites (encodeCodeBlock and encodeFunctionCodeBlock) now call encodeDeferred() before release(), so no deferred work is dropped.
Extended reasoning...
The only reported finding is a style nit. I additionally traced the two correctness concerns most likely to bite with deferred encoding: (1) lambda-captured this points into Page::m_buffer, which is a separate heap allocation owned via MallocSpan and does not move when the Vector<Page> grows; (2) grep for Encoder encoder( shows exactly two entry points, both patched. The captured WriteBarrier values and raw ExpressionInfo* are held across a synchronous encode with no GC safepoints, so lifetime is bounded by the caller's live UnlinkedCodeBlock/UnlinkedFunctionExecutable. Not approving because this is a non-trivial reordering of bytecode-cache serialization — a human should confirm the layout reasoning and the RELEASE_ASSERT(m_bodies.isEmpty()) invariant.
| #include <wtf/Deque.h> | ||
| #include <wtf/Function.h> |
There was a problem hiding this comment.
🟡 nit: These new <wtf/Deque.h> and <wtf/Function.h> includes are inserted immediately after the config.h/CachedTypes.h primary-header pair, breaking WebKit include ordering. They should move into the existing <wtf/...> block at lines 49-55 (alphabetically: Deque.h before FileHandle.h, Function.h between FileHandle.h and InlineMap.h). No functional impact — style/consistency only.
Extended reasoning...
What the issue is
The PR adds two new includes at lines 28-29 of CachedTypes.cpp:
#include "config.h"
#include "CachedTypes.h"
#include <wtf/Deque.h> // ← new
#include <wtf/Function.h> // ← new
#include "BaselineJITCode.h"
...This places them immediately after the config.h + primary-header pair and before the blank line that separates that pair from the rest of the includes.
Why this violates the file's convention
WebKit's include-statement style (https://webkit.org/code-style-guidelines/#include-statements) requires config.h and the file's own header to appear as an isolated pair, followed by a blank line, followed by all other includes sorted alphabetically. This file already follows that convention: lines 26-27 hold the primary pair, then a blank line, then local "..." headers (lines 31-48), then the <wtf/...> block (lines 49-55: FileHandle.h, InlineMap.h, MallocSpan.h, Packed.h, StdLibExtras.h, UUID.h, text/AtomStringImpl.h).
Step-by-step
- Line 26:
#include "config.h"— required first. - Line 27:
#include "CachedTypes.h"— the primary header, required second. - Lines 28-29 (new):
<wtf/Deque.h>,<wtf/Function.h>— these now sit inside what should be the isolated primary-header block, and precede all local"..."includes. - Line 30: blank separator.
- Lines 49-55: the existing sorted
<wtf/...>block where these two headers belong.
check-webkit-style would flag lines 28-29 for both grouping and alphabetical ordering.
Impact
None functionally — the code compiles and behaves identically. This is purely a style/consistency deviation from the surrounding file and WebKit conventions.
Fix
Move the two lines into the existing wtf block, alphabetically sorted:
#include <wtf/Deque.h>
#include <wtf/FileHandle.h>
#include <wtf/Function.h>
#include <wtf/InlineMap.h>
#include <wtf/MallocSpan.h>
...
Preview Builds
|
|
Folded into #494 (same commits, one PR). |
1 participant
Stacked on #494 (only because both touch
CachedTypes.cpp; this one stands alone logically — encoder-only, no format or decoder change).What
Decoding a code block reads, besides its own arrays, one 104-byte
CachedFunctionExecutablerecord per directly nested function (the bodies stay lazy). The encoder wrote those records depth-first: it recursed into child k's body — and that child's whole subtree — before writing child k+1's record. So the records one decode needs were spread across the entire subtree region of the file, and for a mapped payload (abun build --compile --bytecodeexecutable) decoding one block faulted in pages from most of the file.Now
CachedFunctionExecutable::encodequeues the body instead of recursing (Encoder::deferBody),CachedCodeBlock::encodequeues itsExpressionInfoonto a second queue (deferCold), andencodeDeferred()drains bodies FIFO (breadth-first — each body is again contiguous with its own children's records) and then the cold queue, so line/column tables that are only read to build a stack trace form a tail that ordinary execution never touches (and with #494 never even copies). Every pointer in the format is a relative offset, so the decoder doesn't care where things landed.Measured
12 MB minified bundle (typescript, babel, prettier, zod, …) → 75 MB cache, 48k functions. Cache file mapped cold (
posix_fadvise(DONTNEED)+mincoreafter decode), jsc shell, x64 Linux:File size is unchanged (75,227,216 → 75,159,840, the difference is alignment padding).
JSTests/stressthrough the disk cache (write run +JSC_forceDiskCacheread run, 5,259 tests, with and without #494's borrowing): identical pass/fail tomain.