Repository navigation
[JSC] CodeBlock aging: only an embedder-tagged idle collection may let counter-less optimized code go - #558
Conversation
…t counter-less optimized code go Follow-up to #557. FTL code (and DFG code without a tier-up counter) aged out in any full collection that found the mutator quiet for optimizedCodeAgingQuietSeconds. A program that forces full collections on a timer while running hot, non-allocating code between them (setInterval(() => Bun.gc(true), 30_000) with a pooled-buffer server, say) satisfies that and would re-jettison and re-tier its FTL code on every such collection. Add GCRequest::isIdle / Heap::isIdleCollection(): an embedder sets it on the collection it requests because the application went idle. Any full collection may still renew a block's lease when it sees allocation, but only an idle one may expire it, so allocation-paced, forced and memory-pressure collections never drop such code. An idle request is not subsumed by a pending ordinary one. The jsc shell gets idleFullGC() and the stress tests now check both that idleFullGC() ages the code out and that plain fullGC() under the same conditions does not.
… age out idle FTL code in them, and only in them The full collections issued after BUN_IDLE_GC_SECONDS of heap quiet now go through JSC__VM__collectAsyncIdle, which sets GCRequest::isIdle (oven-sh/WebKit#558). JSC lets FTL code and counter-less DFG code go only in such a collection, so a program that forces Bun.gc() on a timer while running hot code keeps its optimized code. test: with eager JIT TTLs, the idle collections take a 40-function FTL workload from ~90 FunctionCodeBlocks to a handful; BUN_IDLE_GC_SECONDS=0 plus Bun.gc(true) every 0.7 s leaves them in place.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this and didn't find any bugs. It changes when the collector may drop counter-less optimized code, so a human look at the GC-request/CodeBlock-lifetime interaction is still worthwhile.
What was reviewed:
GCRequest::isIdleplumbing — default-initialized tofalse, so every existing construction site (allocation-paced, memory-pressure,collectNow/collectSyncdefaults) stays non-idle as intended.subsumedBy— an idle request correctly refuses to be absorbed by a pending non-idle one; the reverse (non-idle absorbed by idle) is fine since idle is strictly more capable.Heap::isIdleCollection()is private but reachable fromCodeBlockvia the existingfriend class CodeBlock, matching howtotalBytesAllocated()is already accessed.- Stress tests updated consistently;
-off.jsstill hard-codes1e6rather thantestLoopCount, but that's pre-existing.
Extended reasoning...
Overview
This PR adds a bool isIdle tag to GCRequest (guarded by USE(BUN_JSC_ADDITIONS)) so the embedder can mark a full collection as one requested because the application went idle. CodeBlock::shouldJettisonDueToOldAge is tightened so that FTL / counter-less DFG blocks may only be aged out during such an idle-tagged collection — ordinary allocation-paced, program-forced, or memory-pressure collections may still renew the lease but never expire it. Supporting changes: GCRequest::subsumedBy won't let an idle request be swallowed by a pending non-idle one, GCRequest::dump prints the tag, Heap::isIdleCollection() exposes m_currentRequest.isIdle, the jsc shell gains idleFullGC(), and the two codeblock-aging-ftl-idle* stress tests are updated to exercise both the idle and non-idle paths.
Security risks
None identified. The change is entirely internal to JSC's GC/CodeBlock lifetime accounting; it adds no new external inputs, parsing, or privilege surfaces. The new jsc-shell host function idleFullGC() mirrors the existing fullGC() pattern and is only in the test shell, not the runtime API.
Level of scrutiny
Moderate-to-high. The diff is small (~60 lines of C++) and correctly #if USE(BUN_JSC_ADDITIONS)-guarded, but it alters the semantics of when optimized code is jettisoned during GC — a subtle area where mistakes surface as recompile churn or, conversely, code that never releases. Source/JavaScriptCore and JSTests are also covered by CODEOWNERS (@ WebKit/jsc-reviewers), which per policy warrants a human sign-off rather than auto-approval.
Other factors
The implementation follows the established pattern from #557: isIdleCollection() sits next to the existing private totalBytesAllocated() accessor and is reached via the pre-existing friend class CodeBlock declaration in Heap.h. GCRequest::isIdle defaults to false, so every code path that constructs a GCRequest without explicitly setting it (the collector's own scheduling, collectSync(CollectionScope::Full), memory-pressure handlers) remains non-idle — the fail-safe direction. The subsumedBy asymmetry is correct: an idle request must not be absorbed by a non-idle one (or the aging pass would silently never run), while a non-idle request being absorbed by a pending idle one is harmless since idle does strictly more. Test coverage now checks both directions (survives non-idle fullGC(), drops on idleFullGC()), and the option-off test switches to idleFullGC() so it still exercises the "never age out when option is 0" path meaningfully.
Preview Builds
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (7)
Included review availability: 1 review is currently available. Your included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour. WalkthroughChangesIdle-triggered FTL aging
Merge Risk: ⚪ Minimal · up to No concrete current-head risk was established that should block merging. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description provides a clear explanation of the problem, implementation, embedder behavior, testing changes, and expected Bun behavior. It does not include the template's Bugzilla URL, reviewer line, or explicit changed-file list, but the core required information is present. 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 |
No-Verification-Needed: dependency version bump only
… optimized code against the last active collection (#560) * [JSC] CodeBlock aging: drop the per-allocation byte counter; age idle optimized code against the last collection that saw allocation #557 added a running total in Heap::didAllocate and a per-CodeBlock MB snapshot to tell whether the mutator had been allocating between full collections. With #558's idle tag that is more machinery than the decision needs: a collection already knows how much was allocated since the previous one (totalBytesAllocatedThisCycle()). - Heap stamps m_lastActiveCollectionTime at the start of any collection that finds more than optimizedCodeAgingQuietAllocationMB allocated since the previous one. Nothing is added to didAllocate(). - CodeBlock::shouldJettisonDueToOldAge for FTL / counter-less DFG code: only an idle-tagged collection may drop it, and only once neither the block's compilation nor an active collection has happened for optimizedCodeAgingQuietSeconds. No per-block state is written; m_leaseStartAllocatedMB is gone (CodeBlock is 4 bytes smaller again). - An embedder's periodic no-op collections while idle (Bun ticks collectAsync() every 30 s) don't see allocation and so don't hold the code alive; a forced or allocation-paced collection still can never drop it. Same behaviour end to end (Bun, express app, idle +120 s: 125 MB anon before and after; idle probe 89 -> 2 CodeBlocks, forced-GC probe 89 -> 84); the stress test additionally checks that an idle collection right after an active one keeps the code. * Review: accumulate per-cycle allocation across collections for the active stamp (a steadily working mutator collected in small cycles still reads as active); guard the test's back-to-back-collections assertion on the measured gap
1 participant
Follow-up to #557.
#557 let FTL code (and DFG code with no tier-up counter) age out in any full collection that found ≤ 1 MB allocated for
optimizedCodeAgingQuietSeconds. That leaves one way to get a recompile cycle: a program that forces full collections itself on a timer (setInterval(() => Bun.gc(true), 30_000)is not unusual) while running hot code that allocates almost nothing between them — every forced collection would drop the FTL code and it would re-tier each time.What
GCRequest::isIdle+Heap::isIdleCollection(): an embedder tags the collection it requests because the application went idle. InCodeBlock::shouldJettisonDueToOldAge, any full collection may still renew a counter-less block's lease when it sees allocation, but only an idle-tagged collection may expire it — allocation-paced, program-forced and memory-pressure collections never drop such code.subsumedBya pending ordinary request (it has to actually run as idle);GCRequest::dumpprints the tag.idleFullGC()=fullGC()with the tag, for tests.GarbageCollectionControllerissues afterBUN_IDLE_GC_SECONDSof heap quiet (Let idle FTL code age out once the heap goes quiet (WebKit#557/#558) bun#41297); nothing else does.Tests
codeblock-aging-ftl-idle.jsnow checks both directions: after a quiet stretch two plainfullGC()s leave the FTL block alone andidleFullGC()drops it; allocation between idle collections keeps it;-off.jsunchanged in spirit (option 0 ⇒ never). In Bun with eager TTLs: idle collections take a 40-function FTL workload from 89 CodeBlocks to 2, whileBUN_IDLE_GC_SECONDS=0+Bun.gc(true)every 0.7 s leaves 84.