Skip to content

[JSC] Cap the per-thread cached AssemblerBuffer size #476

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

Merged
Jarred-Sumner merged 1 commit into from
Aug 21, 2026
+15 −7
Merged

[JSC] Cap the per-thread cached AssemblerBuffer size #476

Show file tree
Hide file tree
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
14 changes: 9 additions & 5 deletions Source/JavaScriptCore/assembler/AssemblerBuffer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,14 +185,18 @@

~AssemblerDataImpl()
{
if constexpr (type == AssemblerDataType::Code)
threadSpecificAssemblerData()->takeBufferIfLarger(*this);
// A one-off huge compile would otherwise pin its scratch buffer to this thread forever.
unsigned cacheLimit = Options::maximumCachedAssemblerBufferSize();
if (!cacheLimit || m_capacity <= cacheLimit) {
if constexpr (type == AssemblerDataType::Code)
threadSpecificAssemblerData()->takeBufferIfLarger(*this);
#if ENABLE(JIT_SIGN_ASSEMBLER_BUFFER)
if constexpr (type == AssemblerDataType::Hashes)
threadSpecificAssemblerHashes()->takeBufferIfLarger(*this);
if constexpr (type == AssemblerDataType::Hashes)
threadSpecificAssemblerHashes()->takeBufferIfLarger(*this);
#else
static_assert(type != AssemblerDataType::Hashes);
static_assert(type != AssemblerDataType::Hashes);
#endif
}

Check warning on line 199 in Source/JavaScriptCore/assembler/AssemblerBuffer.h

View check run for this annotation

Claude / Claude Code Review

Oversized compile drains the thread-specific cache to 128 bytes, not ~1 MB 

Minor: the constructor unconditionally moves the cached buffer *out* of `threadSpecificAssemblerData()` (resetting the slot to InlineCapacity=128), so when an oversized compile skips donation here the slot is left at 128 bytes — not "~1 MB" as the Cost section says. The very next compile on that thread starts from 128 bytes and needs ~20+ `grow()` steps, not 4–6. No correctness impact and still microseconds, but you may want to either correct the description before upstreaming or donate back
Comment on lines +188 to +199

@claude claude Bot Aug 20, 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.

🟡 Minor: the constructor unconditionally moves the cached buffer out of threadSpecificAssemblerData() (resetting the slot to InlineCapacity=128), so when an oversized compile skips donation here the slot is left at 128 bytes — not "~1 MB" as the Cost section says. The very next compile on that thread starts from 128 bytes and needs ~20+ grow() steps, not 4–6. No correctness impact and still microseconds, but you may want to either correct the description before upstreaming or donate back a buffer capped at cacheLimit.

Extended reasoning...

What this is

The PR's Cost section says: "on the next oversized compile the buffer has to be re-malloc'd and grown from the ~1 MB cached one (about 4–6 realloc steps at 1.5×)". That implies the thread-specific cache retains a ~1 MB buffer after an oversized compile. For AssemblerDataImpl it does not — the cache is drained to 128 bytes.

Step-by-step trace

Assume the thread-specific slot currently holds a ~900 KB buffer from prior compiles.

  1. Constructor (AssemblerDataImpl()): initializes this to m_inlineBuffer / InlineCapacity (128), then calls this->takeBufferIfLarger(*threadSpecificAssemblerData()).
  2. takeBufferIfLarger (lines 168–184): the slot is not inline and 900 KB > 128, so it moves the slot's buffer into this and resets the slot: other.m_buffer = other.m_inlineBuffer; other.m_capacity = InlineCapacity;. The thread-specific slot is now at 128 bytes.
  3. The compile runs and grow()s past 1 MB — say to 12 MB.
  4. Destructor: cacheLimit = 1 MB, m_capacity = 12 MB > cacheLimit, so the if body is skipped and clear() frees the 12 MB buffer. Nothing is donated back.
  5. The slot remains at 128 bytes (from step 2).
  6. Next compile on this thread: the constructor calls takeBufferIfLarger on the slot; other.isInlineBuffer() is true so it returns immediately. This compile starts from 128 bytes. Reaching 1 MB takes ~22 grow() calls at 1.5×; reaching 12 MB takes ~28 — not the 4–6 the description states.

Why the description's model doesn't hold here

The description reads as if the cap leaves the cache at ~1 MB and only the excess is dropped. In practice, because the constructor always drains the slot (it takes ownership, not a copy), skipping the destructor donation leaves the slot empty. So the behaviour is "cap = drain to InlineCapacity on any oversized compile", not "cap = clamp to 1 MB".

BranchCompactionLinkBuffer is different: its constructor only takes the cached buffer when threadSpecific->size() >= size; otherwise it mallocs fresh and leaves the cache intact. So for that class, an oversized compile does leave the ~1 MB cache in place, and the description holds. The discrepancy is only in AssemblerDataImpl.

Impact

  • Correctness: none — generated code is unchanged.
  • Memory: the stated savings are real (arguably slightly better, since even the ~1 MB isn't retained after an oversized compile).
  • Perf: the compile immediately following an oversized one pays ~20 extra reallocs instead of ~4–6. That is still microseconds against a multi-MB codegen pass. In the PR's own workload ("oversized compile happens once or twice per process"), a normal-sized compile will typically run in between and refill the cache before the next oversized one anyway.

Suggested fix

Either is fine:

  • Adjust the description before upstreaming to say the cache is emptied (not capped at ~1 MB) after an oversized AssemblerDataImpl compile, so the next compile starts from InlineCapacity.
  • Or have the destructor donate back a bounded buffer when over the limit — e.g. realloc down to cacheLimit and then donate, or only skip donation if the slot is already ≥ cacheLimit. That would make the code match the described "cap" semantics.

Not blocking — flagging mainly because the PR is headed upstream and the cost model in the description doesn't match AssemblerDataImpl's actual behaviour.

clear();
}

Expand Down
7 changes: 5 additions & 2 deletions Source/JavaScriptCore/assembler/LinkBuffer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,8 +235,11 @@ class BranchCompactionLinkBuffer {
if (m_bufferProvided)
return;

auto& threadSpecific = threadSpecificBranchCompactionLinkBuffer();
threadSpecific->takeBufferIfLarger(*this);
size_t cacheLimit = Options::maximumCachedAssemblerBufferSize();
if (!cacheLimit || m_size <= cacheLimit) {
auto& threadSpecific = threadSpecificBranchCompactionLinkBuffer();
threadSpecific->takeBufferIfLarger(*this);
}

if (m_data)
BranchCompactionLinkBufferMalloc::free(m_data);
Expand Down
1 change: 1 addition & 0 deletions Source/JavaScriptCore/runtime/OptionsList.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,6 +335,7 @@ bool hasCapacityToUseLargeGigacage();
v(Bool, breakOnThrow, false, Normal, nullptr) \
\
v(Unsigned, maximumOptimizationCandidateBytecodeCost, 100000, Normal, nullptr) \
v(Unsigned, maximumCachedAssemblerBufferSize, 1 * MB, Normal, "Assembler scratch buffers larger than this are freed after compilation instead of being cached per thread (0 = cache any size)"_s) \
\
v(Unsigned, maximumFunctionForCallInlineCandidateBytecodeCostForDFG, 80, Normal, nullptr) \
v(Unsigned, maximumFunctionForClosureCallInlineCandidateBytecodeCostForDFG, 80, Normal, nullptr) \
Expand Down
Loading