[JSC] Cap the per-thread cached AssemblerBuffer size - #476
Conversation
takeBufferIfLarger() only ever grows the ThreadSpecific slot, so one huge Baseline compile pins its scratch buffer to the thread for the life of the process. Add Options::maximumCachedAssemblerBufferSize (default 1 MB, 0 = unbounded) and free anything larger instead of caching it. Same for BranchCompactionLinkBuffer on ARM64.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // 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 | ||
| } |
There was a problem hiding this comment.
🟡 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.
- Constructor (
AssemblerDataImpl()): initializesthistom_inlineBuffer/InlineCapacity(128), then callsthis->takeBufferIfLarger(*threadSpecificAssemblerData()). takeBufferIfLarger(lines 168–184): the slot is not inline and 900 KB > 128, so it moves the slot's buffer intothisand resets the slot:other.m_buffer = other.m_inlineBuffer; other.m_capacity = InlineCapacity;. The thread-specific slot is now at 128 bytes.- The compile runs and
grow()s past 1 MB — say to 12 MB. - Destructor:
cacheLimit = 1 MB,m_capacity = 12 MB > cacheLimit, so theifbody is skipped andclear()frees the 12 MB buffer. Nothing is donated back. - The slot remains at 128 bytes (from step 2).
- Next compile on this thread: the constructor calls
takeBufferIfLargeron the slot;other.isInlineBuffer()is true so it returns immediately. This compile starts from 128 bytes. Reaching 1 MB takes ~22grow()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
AssemblerDataImplcompile, so the next compile starts fromInlineCapacity. - Or have the destructor donate back a bounded buffer when over the limit — e.g.
reallocdown tocacheLimitand 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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Included review availability: 4 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. WalkthroughChangesThe change adds Assembler buffer cache limits
Merge Risk: ⚪ Minimal · up to The change bounds otherwise permanently retained assembler scratch buffers while preserving existing caching below the limit; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
2 participants
AssemblerDataImpl(assembler/AssemblerBuffer.h) writes machine code into a buffer that grows 1.5× pergrow(). When the compile finishes, the destructor hands the buffer tothreadSpecificAssemblerData(), and the nextAssemblerDataImplon that thread borrows it back. Both directions go throughtakeBufferIfLarger(), so the cached buffer only ever grows — there is no path that shrinks it. Once a thread has compiled one very large function, that function's full buffer stays resident until the thread exits.BranchCompactionLinkBuffer(LinkBuffer.cpp) has the same pattern for the ARM64 branch-compaction copy.This matters for Bun because single-file bundles routinely contain a handful of enormous functions — a bundler's module wrapper, generated tables, a top-level that runs once but contains an init loop. Such a function gets Baseline-compiled through
loop_osr(there is no size gate on the Baseline tier, unlikemaximumOptimizationCandidateBytecodeCostfor DFG), its scratch buffer grows to a few MB, and that allocation is then pinned for the life of the process even though the function never compiles again (m_unlinkedBaselineCodekeeps the result).Change
Options::maximumCachedAssemblerBufferSize, default 1 MB. In~AssemblerDataImpland~BranchCompactionLinkBuffer, a buffer above the limit is freed instead of being donated to the thread-specific slot.0restores the unbounded behaviour, so the two can be A/B'd on one binary withBUN_JSC_maximumCachedAssemblerBufferSize=0.Cost
Only a compile whose buffer exceeds the limit pays anything: on the next oversized compile the buffer has to be re-malloc'd and grown from the ~1 MB cached one (about 4–6
reallocsteps at 1.5×). That is microseconds against a multi-MB code-generation pass, and in the bundles I looked at the oversized compile happens once or twice per process.Measurement
Microbenchmark: a one-shot function with a hot loop followed by 60k straight-line statements, Baseline code 12.4 MB (
BUN_JSC_logJIT=1),BUN_JSC_useConcurrentJIT=0, macOS arm64, physical footprint (proc_pid_rusage) afterBun.gc(true):maximumCachedAssemblerBufferSizeThe 11.5 MB difference is the scratch buffer that is now released. For reference, a
@babel/standalonebundle pins a 10.6 MB buffer the same way (its wrapper function's Baseline code is 7.6 MB).No change to generated code; only whether a scratch buffer is kept.
Upstream
Nothing here is Bun-specific, so I plan to send the same change to upstream WebKit. Landing it here first so Bun picks it up without waiting on that.