Skip to content

Bytecode cache: write each code block's records contiguously, bodies breadth-first, expression info last #495

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.

Sign up for GitHub

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

Closed
Jarred-Sumner wants to merge 1 commit into from
Closed

Bytecode cache: write each code block's records contiguously, bodies breadth-first, expression info last #495

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 30 additions & 4 deletions Source/JavaScriptCore/runtime/CachedTypes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@

#include "config.h"
#include "CachedTypes.h"
#include <wtf/Deque.h>
#include <wtf/Function.h>

Check warning on line 29 in Source/JavaScriptCore/runtime/CachedTypes.cpp

View check run for this annotation

Claude / Claude Code Review

wtf includes placed out of order 

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.
Comment on lines +28 to +29

@claude claude Bot Aug 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

  1. Line 26: #include "config.h" — required first.
  2. Line 27: #include "CachedTypes.h" — the primary header, required second.
  3. 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.
  4. Line 30: blank separator.
  5. 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>
...


#include "BaselineJITCode.h"
#include "BuiltinNames.h"
Expand Down Expand Up @@ -164,6 +166,22 @@
m_leafExecutables.add(executable, offset);
}

// Layout: a code block's own arrays and its children's executable records are written contiguously; the children's
// bodies follow breadth-first, and data that is only read on rare paths (expression info) goes after every body.
// Decoding one block then reads one contiguous run of the payload rather than records scattered through every
// descendant's subtree, so a mapped payload pages in only what is decoded.
void deferBody(Function<void()>&& encodeBody) { m_bodies.append(WTF::move(encodeBody)); }
void deferCold(Function<void()>&& encodeCold) { m_cold.append(WTF::move(encodeCold)); }
void encodeDeferred()
{
while (!m_bodies.isEmpty())
m_bodies.takeFirst()();
while (!m_cold.isEmpty()) {
m_cold.takeFirst()();
RELEASE_ASSERT(m_bodies.isEmpty());
}
}

RefPtr<CachedBytecode> release(BytecodeCacheError& error)
{
if (!m_currentPage)
Expand Down Expand Up @@ -291,6 +309,8 @@
Vector<Page> m_pages;
UncheckedKeyHashMap<const void*, ptrdiff_t> m_ptrToOffsetMap;
LeafExecutableMap m_leafExecutables;
Deque<Function<void()>> m_bodies;
Deque<Function<void()>> m_cold;
};

Decoder::Decoder(VM& vm, Ref<CachedBytecode> cachedBytecode, RefPtr<SourceProvider> provider)
Expand Down Expand Up @@ -2530,11 +2550,13 @@
m_ecmaName.encode(encoder, executable.ecmaName());
m_parentScopeTDZVariables.encode(encoder, executable.m_parentScopeTDZVariables);

m_unlinkedCodeBlockForCall.encode(encoder, executable.m_unlinkedCodeBlockForCall);
m_unlinkedCodeBlockForConstruct.encode(encoder, executable.m_unlinkedCodeBlockForConstruct);

if (!executable.m_unlinkedCodeBlockForCall || !executable.m_unlinkedCodeBlockForConstruct)
encoder.addLeafExecutable(&executable, encoder.offsetOf(this));

encoder.deferBody([this, &encoder, forCall = executable.m_unlinkedCodeBlockForCall, forConstruct = executable.m_unlinkedCodeBlockForConstruct] {
m_unlinkedCodeBlockForCall.encode(encoder, forCall);
m_unlinkedCodeBlockForConstruct.encode(encoder, forConstruct);
});
}

ALWAYS_INLINE UnlinkedFunctionExecutable* CachedFunctionExecutable::decode(Decoder& decoder) const
Expand Down Expand Up @@ -2656,7 +2678,9 @@
m_instructions.encode(encoder, codeBlock.m_instructions.get());
m_constantRegisters.encode(encoder, codeBlock.m_constantRegisters);
m_constantsSourceCodeRepresentation.encode(encoder, codeBlock.m_constantsSourceCodeRepresentation);
m_expressionInfo.encode(encoder, codeBlock.m_expressionInfo.get());
encoder.deferCold([this, &encoder, expressionInfo = codeBlock.m_expressionInfo.get()] {
m_expressionInfo.encode(encoder, expressionInfo);
});
m_jumpTargets.encode(encoder, codeBlock.m_jumpTargets);
m_outOfLineJumpTargets.encode(encoder, codeBlock.m_outOfLineJumpTargets);

Expand Down Expand Up @@ -2844,6 +2868,7 @@
encodeCodeBlock<UnlinkedModuleProgramCodeBlock>(encoder, key, codeBlock);
else
ASSERT(classInfo == UnlinkedEvalCodeBlock::info());
encoder.encodeDeferred();

return encoder.release(error);
}
Expand All @@ -2860,6 +2885,7 @@
FileSystem::FileHandle invalidFileHandle;
Encoder encoder(vm, invalidFileHandle);
encoder.malloc<CachedFunctionCodeBlock>()->encode(encoder, *codeBlock);
encoder.encodeDeferred();
return encoder.release(error);
}

Expand Down
Loading