[JSC] Honor a GC request deferred by DeferGCForAWhile at the next VM entry - #526
[JSC] Honor a GC request deferred by DeferGCForAWhile at the next VM entry#526dylan-conway wants to merge 1 commit into
Conversation
…entry collectIfNecessaryOrDefer() under a DeferGCForAWhile scope only records m_didDeferGCWork; unlike ~DeferGC, ~DeferGCForAWhile does not act on it, so the request waits for the next allocation slow path or ~DeferGC. The large extra-memory reports made while linking a CodeBlock (ScriptExecutable::prepareForExecutionImpl: metadata table, baseline JIT code) are always in that situation. An embedder that repeatedly enters the VM to run freshly linked large functions can go tens of iterations without either trigger, allocating hundreds of MB of CodeBlocks with no collection, which then land in MarkedBlocks the allocator never revisits and stay unswept until a synchronous full collection. Check the pending flag when a VMEntryScope is constructed and run collectIfNecessaryOrDefer() there. This is a point where the callee is about to allocate anyway; it is skipped while any DeferGC scope is active. Gated by Options::usePollingDeferredGCWorkAtVMEntry (default on).
|
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 (4)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. WalkthroughThe change adds heap APIs for pending deferred GC work, adds a default-enabled polling option, and invokes deferred GC processing from ChangesDeferred GC VM-entry handling
Merge Risk: ⚪ Minimal · up to This localized change honors deferred garbage-collection work at VM entry to prevent temporary memory buildup, while preserving the existing budget check and providing a default-on control. No actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description gives a detailed explanation of the bug, implementation, affected paths, and measurements, but it omits required template information such as the Bugzilla URL, review status, and changed-file/function list.
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes GC scheduling behavior on a hot inline path and Source/JavaScriptCore is CODEOWNER-covered, a human look would still be worthwhile.
What was reviewed:
hasPendingDeferredGCWork()/performPendingDeferredGCWork()mirror the existingdecrementDeferralDepthAndGCIfNeededSlow()pattern (clear flag, callcollectIfNecessaryOrDefer()), and the!m_deferralDepthguard prevents firing under an activeDeferGC/DeferGCForAWhile.- Checked
VMEntryScopeconstruction sites inInterpreter.cppandDebuggerCallFrame.cpp— all already allocate immediately after the scope, so a GC at this point is not a new hazard for callers. VMEntryScopeInlines.halready pulls inHeap.handOptionstransitively viaVM.h;performPendingDeferredGCWorkisJS_EXPORT_PRIVATEand out-of-line, so the inline site stays cheap.
Extended reasoning...
Overview
This PR adds a new place where JavaScriptCore honors a deferred GC request: the VMEntryScope constructor. It introduces Heap::hasPendingDeferredGCWork() (an inline check of m_didDeferGCWork && !m_deferralDepth) and Heap::performPendingDeferredGCWork() (an out-of-line, exported function that clears the flag and calls collectIfNecessaryOrDefer()), plus a new usePollingDeferredGCWorkAtVMEntry option (default on) that gates the check in VMEntryScopeInlines.h. The motivation is that extra-memory reports made under DeferGCForAWhile in ScriptExecutable::prepareForExecutionImpl set m_didDeferGCWork but nothing acts on it until the next allocation slow path or ~DeferGC, which an embedder loop may not reach for many iterations. The new code is a near-verbatim copy of the body of decrementDeferralDepthAndGCIfNeededSlow().
Security risks
None identified. There is no untrusted input handling, no auth or permissions surface, and no data exposure. The change only affects when the collector is polled. The !m_deferralDepth guard ensures the new call site respects any active deferral scope, and collectIfNecessaryOrDefer() itself re-checks isDeferred() before doing anything, so the invariant that GC never runs while deferred is preserved. Conservative stack scanning already protects on-stack cell references at VMEntryScope construction sites, and every audited caller (Interpreter::executeProgram/executeCall/executeConstruct/executeModuleProgram, DebuggerCallFrame) already tolerates GC immediately after constructing the scope because they allocate right afterward.
Level of scrutiny
High. This touches GC scheduling in an ALWAYS_INLINE constructor on the C++→JS entry path — one of the hottest paths in the engine — and changes when collection can be triggered. GC-timing changes in JSC have historically been subtle: introducing a new safepoint requires confidence that no caller holds an unrooted cell that depends on GC not running there. The PR description addresses this convincingly and includes extensive benchmarks showing the option itself does not add collections on unaffected workloads, but this is exactly the kind of change a JSC/heap owner should sign off on.
Other factors
The changed files fall under /Source/JavaScriptCore in .github/CODEOWNERS, which routes to the JSC reviewers group; per the approval guidelines that alone means human review is required. The change is small, well-commented, option-gated for A/B, and follows the established pattern in Heap.cpp, so I have no concerns beyond the need for owner sign-off on a GC-timing change.
Preview Builds
|
1 participant
Heap::collectIfNecessaryOrDefer()called under aDeferGCForAWhilescope only setsm_didDeferGCWork.~DeferGCacts on that flag;~DeferGCForAWhiledeliberately does not, so the request waits for whatever comes next: an allocation slow path or some unrelated~DeferGC. The extra-memory reports made while linking aCodeBlockinScriptExecutable::prepareForExecutionImpl(the metadata table, and the baseline JIT code size when unlinked baseline code is installed) are always in that position.This adds one more place that honors the flag:
VMEntryScope's constructor, i.e. the next time the embedder enters the VM (Interpreter::executeProgram/executeCall/executeConstruct/executeEval/executeModuleProgram). JS→JS calls do not construct aVMEntryScopeand are unaffected. The check ism_didDeferGCWork && !m_deferralDepth, so it does nothing while anyDeferGC/DeferGCForAWhileis active, and when it fires it just runscollectIfNecessaryOrDefer(), which still only collects if the cycle's budget is exceeded. A collection at this point is nothing new for callers: the first thingexecuteProgramdoes after constructing the scope is allocate aProgramExecutable, and the callee allocates. Gated byOptions::usePollingDeferredGCWorkAtVMEntry(default on) so it can be A/B'd.What goes wrong without it
Bun's
test/cli/run/require-cache.test.ts("files transpiled and loaded don't leak the output source code › via require() with a lot of function calls") requires and un-caches the same 100 KB CommonJS module (one function, 20 000 call statements) 500 times in a synchronous loop, does a synchronous full GC, and reads RSS. Since the 8c4fd56 upgrade it fails on Windows x64 CI nearly every run (RSS +150–250 MB); the same thing happens on Linux in roughly one run in five, before and after that upgrade, and on the pre-upgrade Windows binary as soon as anything perturbs timing (logGC=1is enough). Traced with counters onCodeBlock/UnlinkedCodeBlockconstruction/destruction and on the collector:UnlinkedFunctionCodeBlockfor the module function (CodeCache), so its LLInt tier-up counter is shared and after ~150 requires it is baseline-compiled (1.9 MB of code for the 20 000 call sites). From then on every require reports ~4 MB of extra memory while linking (2 MB metadata + 1.9 MBsetJITCode), all underDeferGCForAWhile.CodeBlockis swept the next cycle. Around the tier-up a Full collection can land while a moduleCodeBlockis live, leaving a ~5 MB eden budget; the next iteration's 4 MB doesn't exceed it at its only slow paths, the allocators keep their free lists, and then nothing on the require path takes a slow path or runs a~DeferGCfor a long time. Measured: 32, 49, 27 consecutive iterations with no collection andbytesAllocatedThisCycleat 120–280 MB, every byte of it reported withisDeferred()true.CodeBlockdirectory fills MarkedBlocks 0…3 during that window. When a collection finally runs they are all dead, but destruction happens at sweep, every later cycle restarts allocation at block 0, and steady-state demand is one or twoCodeBlocks per cycle, so blocks 1…3 are never swept again. 28 deadCodeBlocks × ~2 MB of metadata stayed resident for the rest of the run, invisible to the collector's accounting, until the finalcollectNow(Sync, Full)swept them; libpas returns those pages ~100–250 ms later, after the test has read RSS. (With an idle run loop the incremental sweeper and scavenger clean this up in well under a second, so it is a synchronous-execution transient, not a leak.)With the flag honored at VM entry the deferred request from step 1 is acted on within one iteration, the per-iteration cadence never breaks, and nothing accumulates.
Measurements
Linux x64, LTO release builds of Bun against this branch's base with and without the patch; "fixoff" is the patched binary with
--usePollingDeferredGCWorkAtVMEntry=0, which separates the behavior change from binary layout differences. 10 interleaved runs per workload, pinned cores; time is median/min ms, RSS is peak MB, cpu is user+sys seconds.The failure mode: require-cache fixture, 20 runs each — vanilla 3/20 over threshold (171–203 MB), fixoff 1/20, patched 0/20 (0/30 in an earlier instrumented run, where the largest GC-less allocation window dropped from 281 MB to 11 MB). Windows x64 LTO build, with
logGC=1to push it into the bad timing: unpatched 12/12 over threshold (33–182 MB), patched 0/12.GC cycle counts with the option on vs off, 3 runs each: identical for every other workload below (
m_await7/7,m_replace3/3,m_jsonreviver6/6,m_serve_inproc8/8,m_gcchurn98/98,modload-cjs2/2,modload-esm0/0,npmload2/2,ts-transpile4/4,buntest2/2;m_immediate14–21 vs 12–20). The check fires often (786 times inbuntest) but is a no-op unless the budget is already exceeded, so it does not add collections.The
m_*set is deliberately heavy on C++→JS entries, the only path this touches: 3M drained microtasks, 2M awaits,String.prototype.replacewith a function (10M callbacks),JSON.parsereviver (2.4M callbacks), 1MdispatchEvent, 1MsetImmediate, 30k in-processBun.serverequests;m_gcchurnis pure-JS allocation churn. The rest: loading a 500-module CJS and ESM graph,require("typescript")+@babel/core+@babel/preset-env+lodash,ts.transpileModuleon 1.5 MB, a 15 000-testbun testrun, bundling three.js, requiring 300 distinct large modules. HTTP underab -k -c50 -n150000, 10 rounds: hello-world 52.9k / 52.7k / 52.9k req/s, server CPU 1.24 / 1.25 / 1.25 s, peak RSS 56 / 51 / 51 MB; React SSR 12.45k / 12.23k / 12.19k req/s, CPU 13.27 / 13.52 / 13.55 s, peak RSS 235 / 182 / 234 MB.bun -e 0startup 4.6±0.7 / 4.7±0.8 / 4.5±0.7 ms (order flips between rounds). Wherever patched and vanilla differ by more than ~2%, fixoff moves with patched, i.e. it is the two binaries rather than the option.