GC controller: a tick counts the quiet the timer was armed with - #43215
Jarred-Sumner wants to merge 1 commit into
Conversation
When the next idle collection is due sooner than the timer's own interval the timer is armed for that, but the tick that followed still counted the interval of the mode it was in: 30 s on the slow tick. The counted quiet then ran ahead of the elapsed quiet, so the 2 min and 10 min collections came up to 29 s early, and with a list whose entries are less than 30 s apart one tick crossed two of them and did one collection for both. arm() is a method now: it stores what it armed the timer with, and idle_tick counts that.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. WalkthroughThe controller now records the interval used for each timer deadline. Idle-GC accounting uses that recorded interval after a timer fires. A regression test verifies that concurrent deadlines produce two full collections. ChangesIdle-GC cadence
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The idle-GC cadence correction preserves the actual elapsed interval and includes targeted regression coverage for concurrent deadlines. No actionable merge risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding on the new test's wall-clock budget, I also checked the two things the arm() refactor could have broken: arm() is now the only site that inserts gc_repeating_timer (dispatch.rs only dispatches the fire), so the armed_interval_ms default of 0 is never read by idle_tick — the first fire always follows an arm(). The &self in arm() derives the timer pointer exactly as the old call sites did, and timer_insert/timer_remove only relink the heap node without running user code, so the provenance/free-during-callback concern from src/CLAUDE.md does not apply here.
Extended reasoning...
The Rust change is a small, correct refactor: arm() becomes a method that records the interval it armed and derives the timer pointer from addr_of!(self.gc_repeating_timer) — the same expression both old call sites used — and the fire callback now credits armed_interval_ms instead of repeat_interval(), which fixes the case where a slow-mode tick armed for min(30 s, due_in) credited 30 s of quiet. I traced every path that inserts the timer (process_gc_timer and the fire handler, both via arm(); deinit only removes) and confirmed dispatch.rs only dispatches the fire, so the recorded interval cannot drift from what the heap holds and the zero default is unobservable. The only reported finding concerns the new test's timing headroom on debug/ASAN lanes, which is a CI-robustness question for a human to weigh rather than a correctness defect in the fix.
One verified lower-impact observation (a convention, logging or cleanup point) was not posted.
| cmd: [ | ||
| bunExe(), | ||
| "-e", | ||
| `setTimeout(() => console.error("MARK"), 300); setTimeout(() => console.error("DONE"), 4200);`, |
There was a problem hiding this comment.
🟡 (optional) CI on debug and ASAN lanes can see this new test fail with Expected: >= 2, Received: 1 even though the fix is correct; the base branch has no test on a 20 ms tick. The budget is wall-clock: DONE fires 4200 ms after eval at test/js/bun/gc/gc-controller-cadence.test.ts:201, but the two fulls need ~51 ticks whose real period is 20 ms plus each tick's own work, because arm() sets next = now + ms after the tick finishes (src/jsc/GarbageCollectionController.rs:67). Fix: make the deadline follow the condition, not the clock: keep the process alive until two FullCollection lines are observed (or a bounded ceiling), or cut the tick count by using a larger BUN_GC_TIMER_INTERVAL with a smaller ratio to the 1 s floor.
Extended reasoning...
The dismissing finder argued ~2 s of slack from nominal numbers and did not account for drift. Each tick re-arms from Timespec::now after its body ran (src/jsc/GarbageCollectionController.rs:67), so the period is 20 ms + tick cost, never 20 ms. Each tick takes the JSLock, requests collect_async, logs under BUN_JSC_logGC and on ASAN the collector's stop-the-world pauses land inside the mutator's next tick. Timeline in real time on a slow lane: startup to first arm 200-500 ms; 30 ticks to reach slow mode at 20 ms + ~10-20 ms overhead each = 0.9-1.2 s; one 1000 ms arm (line 229 floors due_in to 1000); then ~20 more fast ticks at the same inflated period = 0.6-0.8 s; then the requested full collection must actually run and print before the DONE timer callback fires. That totals 2.7-3.5 s against a 4.2 s wall, before any CI load or test.concurrent siblings (the same describe spawns several bun processes at once). The sibling run('2') needs only 2 ticks at 1 s and has no such per-tick overhead multiplier, so it is not evidence. Population: every debug/ASAN CI run of this file. Remedy: await…
Verification: nit — triggered on debug/ASAN CI lanes (or a loaded runner) where the average real period of the 20 ms GC tick stretches to roughly 3x nominal. Mechanism verified in the code: the new test at test/js/bun/gc/gc-controller-cadence.test.ts:196-216 fixes its window by wall clock (setTimeout(... "DONE", 4200) at line 201, MARK at 300 ms) and asserts fulls >= 2 (line 215) with… | nit-to-normal…
|
Updated 7:22 PM PT - Sep 17th, 2026
✅ @Jarred-Sumner, your commit 94487d59684c321a4d8f457ab4b61a8a5dfbea3d passed in 🧪 To try this PR locally: bunx bun-pr 43215That installs a local version of the PR into your bun-43215 --bun |
This branch has not been deployed
2 participants
Problem
min(interval, due_in). The tick that followed still addedrepeat_interval()to the counted quiet, which is 30 s once the timer is in its slow mode.BUN_IDLE_GC_SECONDSlist whose entries are less than 30 s apart one tick crossed two of them and did a single collection for both.Fix
arm()is a method: it stores what it armed the timer with (armed_interval_ms) and derives the timer pointer itself, so both call sites are one line and the invariant lives in one place.idle_tickcounts that value.GarbageCollectionController.rsis 240 lines (248 before).Verification
BUN_IDLE_GC_SECONDS=1,1on a 20 ms tick must show two idle full collections; a build of GC controller: idle collections at 10 s, 2 min and 10 min, nothing paged out; the last drops re-decodable bytecode #43174 shows one (Expected: >= 2, Received: 1).test/js/bun/gc/gc-controller-cadence.test.ts: 17 pass, 5.9 s, three runs on a release build.