Repository navigation
[JSC] CodeBlock aging: refresh the execution-counter snapshot on every look, not only past the TTL - #566
Conversation
…, not only once the block is past its TTL shouldJettisonDueToOldAge() returned early while a block was inside its TTL/lease without recording the block's current execution counter. The next collection then compared against a snapshot from before the last burst of work, saw the counter had "moved", and renewed the lease of code that had not run since - and with an embedder whose idle collections come as a pair (Bun: ~10 s and ~75 s after the heap goes quiet) that renewal was never expired: the first idle collection finds every recently-run DFG/Baseline block too young, the second finds its counter moved. Check the counter first and refresh the snapshot on every look; only then apply the TTL. "Moved" now always means "since the previous collection". Actively running code is unaffected (codeblock-aging-execution-count.js). Also: an optimizing block that the old-age check let go this cycle is now jettisoned with JettisonDueToOldAge rather than JettisonDueToWeakReference (an unmarked optimizing block always looked like the latter), so jettison() takes its old-age path. Claude Code (compiled, 20-turn session, then idle), CodeBlocks alive after Bun's second idle collection: before 2,482 (Baseline 1,183 / DFG 1,087, ~7.3 MB + their metadata/JITData), after 432 (225 / 107, ~1.1 MB); anonymous RSS at idle 190-196 MB -> 179 MB. (cherry picked from commit 794bcc3ee56c78e5e3217bdf2139d34f754411c5) (cherry picked from commit 8aa8f4e)
…ited Heap reads ApproximateTime once when a collection begins (m_currentGCStartApproximateTime, next to the MonotonicTime it already takes) and shouldJettisonDueToOldAge() measures the TTL, renews leases and compares against lastActiveCollectionTime() with that value instead of calling ApproximateTime::now() for every CodeBlock the collection visits. Same results at collection granularity.
|
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 (4)
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. WalkthroughChangesCode block lifecycle
Merge Risk: ⚪ Minimal · up to Code-block aging now uses a collection-level timestamp, refreshes activity snapshots between collections, and explicitly labels aged-block cleanup. The change is ready to merge with no identified current-head risk. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description provides a detailed bug explanation, implementation summary, behavioral impact, and measurements. However, it omits required template information, including a Bugzilla URL, the reviewer line, and a 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}
| if (hasCounter && currentCount != m_previousCounter) { | ||
| m_previousCounter = currentCount; |
There was a problem hiding this comment.
🔴 Moving the counter snapshot ahead of the TTL gate means m_previousCounter = currentCount is now written during marking on every look, so updateActivity()'s later m_previousCounter < count test is always false and m_unlinkedCode->resetAge() is never called for LLInt/Baseline/DFG blocks; under useUnlinkedCodeBlockJettisoning (mini/no-JIT mode) the UnlinkedCodeBlock's age hits maxAge after ~7 GCs and dies the moment its CodeBlock ages out, forcing a full re-parse on the next call where the base branch kept the bytecode cached. Fix: have updateActivity() decide activity from the flag this pass already computed (e.g. reset age whenever shouldJettisonDueToOldAge renewed the lease) instead of re-comparing m_previousCounter.
Extended reasoning...
shouldJettisonDueToOldAge() runs from the executable's visitChildren during marking; with the reorder, an unmarked LLInt/Baseline block whose counter moved hits line 1431 and stores m_previousCounter = currentCount before the TTL early-out. The block is then strongly visited (line 1298 returns true), added to the CodeBlock set (line 1278), and at GC end reconcileWeakReferencesAtGCEnd → updateActivity() (guarded by VM::useUnlinkedCodeBlockJettisoning(), true when !useJIT() or forceMiniVMMode) reads the same counter into count and tests m_previousCounter < count at CodeBlock.cpp:1988 — now always equal, so resetAge() at 1990 is skipped. On the base branch the TTL gate returned before touching m_previousCounter, so updateActivity() still saw the previous GC's snapshot, observed movement, and reset the UnlinkedCodeBlock's age each collection (only the single renewal GC per lease skipped it). After the change nothing ever resets m_age; UnlinkedCodeBlock::visitChildren (UnlinkedCodeBlock.cpp:103) increments it every GC to maxAge=7, and once the CodeBlock finally…
Verification: normal — the reorder introduces a regression the base does not have, in the VM::useUnlinkedCodeBlockJettisoning() configuration (VM.h:766-769 → !useJIT() || forceMiniVMMode() || Options::useUnlinkedCodeBlockJettisoning()). Mechanism, verified against both branches: - ScriptExecutable::visitCodeBlockEdge (ScriptExecutable.cpp:565) calls shouldVisitStrongly() during marking, which at…
Preview Builds
|
1 participant
Follow-up to #542/#546/#560 (this was pushed to #560's branch after it had merged).
The bug
CodeBlock::shouldJettisonDueToOldAge()returned early ontimeSinceCreation() < ttlbefore reading the block's execution counter, so a collection that found a block inside its TTL/lease kept whatever counter snapshot it had from before. The next collection then compared the current counter against that stale snapshot, saw it had "moved" (it moved during the last burst of work, not since), and renewed the lease of code that hadn't run since.With an embedder whose idle collections come as a pair — Bun collects ~10 s and ~75 s after the heap goes quiet — that renewal is never expired: idle GC #1 finds every recently-run DFG/Baseline block too young (no snapshot), idle GC #2 finds every counter "moved" (renew, lease pushed 2×TTL into the future), and there is no GC #3.
Instrumented on a Claude Code session (compiled CLI, 20 turns, then idle), optimizing blocks seen by the two idle collections: before
tooYoung=1250at #1 →counterMoved=1088, agedOut=0at #2; aftertooYoung=257→agedOut=1159.The change
m_previousCounteron every look, then apply the TTL. "Moved" now always means "since the previous collection that looked".JettisonDueToOldAgerather thanJettisonDueToWeakReference(an unmarked optimizing block always matched the latter), sojettison()takes its old-age path (no exit-site tally; baseline code cache released per [JSC] Return unused MarkedBlock pages after sweeping; let idle optimized code age out #542).Actively running code is unaffected: its counter still moves between looks and renews exactly as before (
codeblock-aging-execution-count.jspasses — it asserts no jettison of running functions across 30 GCs).Effect (Claude Code, after Bun's second idle collection)