Fix O(N^2) worker teardown with pending Atomics.waitAsync waiters - #37280
Fix O(N^2) worker teardown with pending Atomics.waitAsync waiters#37280robobun wants to merge 3 commits into
Conversation
A worker exiting with N pending Atomics.waitAsync waiters cancels each ticket through onCancelPendingWork, and dropPendingTicketLocked removed the ticket with removeIf, a full scan of the pending-ticket hash sets: O(N^2) for the whole teardown. The scans also ran while WaiterListManager::unregister held the process-global waiter-lists lock, so Atomics.notify and Atomics.waitAsync on every other thread stalled for the entire window (20k waiters = 5.9s, 40k = 25s). Use the O(1) raw-pointer HashSet remove instead; it hashes the pointer value without dereferencing it, exactly like the removeIf predicate did.
WalkthroughThe scheduler now removes pending tickets directly from both pending-ticket sets. A worker teardown regression test covers 50,000 pending ChangesWorker teardown
Possibly related PRs
Suggested reviewers: 🚥 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.
LGTM — focused O(N²)→O(1) swap of removeIf for the raw-pointer remove overload on HashSet<Ref<Ticket>>, semantics-preserving and well-measured. One inline nit on the test's failure diagnostics; not blocking.
Extended reasoning...
Overview
This PR replaces two HashSet::removeIf calls (full-set scans) in dropPendingTicketLocked with WTF HashSet's O(1) raw-pointer remove(T*) overload. The sets are UncheckedKeyHashSet<Ref<Ticket>>, which hash/compare on pointer identity, so remove(ticket) is exactly equivalent to the old predicate pendingTicket.ptr() == ticket — including the property that the pointer is hashed by value and never dereferenced (the "ticket may be an invalid pointer" comment still holds). A regression test spawns a worker with 50k pending Atomics.waitAsync waiters, terminates it, and bounds the exit time.
Security risks
None. No untrusted input, no boundary crossings, no lifetime changes — the removed Ref<Ticket> is destroyed identically by both removeIf and remove. The lock discipline and event-loop-ref balancing are unchanged.
Level of scrutiny
Low-to-medium. The C++ change is a 4-line mechanical substitution in a helper that already has three call sites in the same file, all unchanged. The pointer-keyed take() in runPendingWork in the same file already relies on the same smart-pointer HashSet lookup family, so this isn't introducing a new pattern. The added comment documenting the O(1) requirement is useful and accurate.
Other factors
- The PR description quantifies the quadratic (4× per 2×N) and links it to the process-global waiter-lists lock — root cause is proven, not correlated.
- The test's 15s bound vs. <1s fixed / 38s+ unfixed gives wide margin against debug/ASAN CI variance and clearly discriminates.
- The only finding is a nit on subprocess test diagnostics (stderr not piped, exit code checked after
JSON.parse). No deadlock risk given the ~30-byte stdout and inherited stderr; it only affects failure-message quality, not correctness or flakiness. - Ruled out: the 90s per-test timeout is justified (60s deadline race + kill needs headroom), and the fixture's lack of a worker
'error'handler doesn't cause a hang (the deadline race bounds it).
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the earlier nit (pipe stderr, assert { stderr, exitCode } before parsing) is addressed in cef6c4d, and the comment was trimmed to a two-line invariant note.
What was reviewed:
dropPendingTicketLocked:removeIf(ptr()==ticket)→remove(ticket)onUncheckedKeyHashSet<Ref<Ticket>>— WTF's raw-pointer overload hashes/compares by pointer value only, so semantics match the old predicate exactly (including the "ticket may be invalid" case, which is guarded by!isKeepingEventLoopAliveand never dereferenced regardless).- Confirmed the sets are
Ref<Ticket>-keyed (JSCTaskScheduler.h:37-38), so there is at most one match and the bool return is equivalent. - New test: deadline race + SIGKILL avoids hanging on regression; fixture writes ~30 bytes so awaiting exit before draining pipes cannot deadlock; 15s bound vs. <1s fixed / 38s+ unfixed leaves wide CI margin.
Extended reasoning...
Overview
The PR replaces HashSet::removeIf (full-set scan) with HashSet::remove(T*) (O(1) hashed lookup) in dropPendingTicketLocked at src/jsc/bindings/JSCTaskScheduler.cpp. This function is called once per pending Atomics.waitAsync ticket during worker VM teardown while JSC holds the process-global waiter-lists lock, so the old O(N) scan produced O(N²) teardown that also stalled Atomics.notify/waitAsync on every other thread. A regression test in test/js/web/atomics.test.ts terminates a worker holding 50k pending waiters and bounds exit at 15s.
Correctness of the C++ change
The two sets are declared as UncheckedKeyHashSet<Ref<JSC::DeferredWorkTimer::Ticket>> (JSCTaskScheduler.h:37-38). WTF's HashSet<Ref<T>> hashes and compares by pointer identity, and provides a raw T* remove overload via a hash translator that hashes the pointer value and compares stored.ptr() == key — exactly what the old removeIf predicate (pendingTicket.ptr() == ticket) did, without dereferencing the argument. Since a HashSet holds unique keys, at most one entry matches, so remove()'s bool return is equivalent to removeIf()'s. The existing "ticket may be an invalid pointer" comment still applies: the second remove only runs when the first found nothing, and even so it never dereferences ticket. No memory-management, locking, or ref-counting behavior changes — this is a pure algorithmic-complexity fix.
Security risks
None. No untrusted input handling, no new allocations, no lifetime changes. The change strictly reduces time spent under a process-global lock.
Level of scrutiny
Medium file (JSC bindings, concurrent teardown path), but the specific change is a 5-line mechanical substitution of one WTF container API for its semantically-equivalent O(1) counterpart. The PR includes before/after timing evidence (release: 38s+ → <1s at N=50k; debug+ASAN: 21.5s → 482ms at N=10k) and a fails-without/passes-with gate. The fix is at the right layer — the helper all three cancellation paths (onCancelPendingWork, shutdown branch of onScheduleWorkSoon, Bun__deleteDeferredWorkTask) share.
Other factors
My prior review nit (pipe stderr, assert exit state before JSON.parse) was addressed in cef6c4d and the thread is resolved. The comment-cop bot flagged the code comment twice; the author trimmed it to two lines documenting the O(1) invariant and who depends on it, then justified keeping it — reasonable, since it explains why a future refactor to removeIf would be a regression. All timeline threads are resolved. The test follows harness conventions (tempDir, bunExe, bunEnv, concurrent pipe drain, combined-object assertion) and has a fail-fast 60s deadline so a regression doesn't hang the file for 90s. The 15s bound vs. sub-second fixed / 38s+ unfixed gives ample margin for slow debug+ASAN CI runners.
|
CI status: 194 of 196 jobs passed. The one red lane is test/cli/test/parallel.test.ts on darwin 14 x64, which is failing on main as well (the >64MB result line test timing out) and is unrelated to this change; it has been reported separately. The remaining failures in the run were known-flaky tests that passed on retry. The new Atomics.waitAsync teardown test passed on every lane. |
2 participants
Problem
A Worker exiting with N pending
Atomics.waitAsyncwaiters tears down in O(N^2), and for the whole windowAtomics.notify/Atomics.waitAsyncissued by every other thread stalls, on any SharedArrayBuffer (unrelated ones included).await worker.terminate()takes seconds to minutes.Release build, before: N=10k exits in 1.5s (one
Atomics.notifycall on the main thread stalls 1475ms), 20k in 5.9s, 40k in 25.4s. A clean 4x per 2x. Node v26 does 100k in 56ms.Cause
VM teardown runs
WaiterListManager::unregister(VM*), which cancels every still-pendingAtomics.waitAsyncticket through theDeferredWorkTimeronCancelPendingWorkhook while holding JSC's process-global waiter-lists lock. The hook'sdropPendingTicketLockedremoved each ticket withHashSet::removeIf, a full scan of the pending-ticket sets. N cancellations x O(N) scan = O(N^2), all under the global lock, soAtomics.notify/Atomics.waitAsyncon other threads (which take the same lock infindList) block for the entire teardown.Atomics.load/adddo not touch that lock, matching the observed symptom.Fix
Use WTF
HashSet's O(1) raw-pointerremoveoverload. The sets are keyed on ticket identity (Ref<Ticket>with pointer hashing), and the overload hashes the pointer value without dereferencing it, exactly like theremoveIfpredicate comparedpendingTicket.ptr() == ticket. Same semantics, constant time.Verification
New test in
test/js/web/atomics.test.tsterminates a worker holding 50k pending waiters and bounds the exit at 15s:exitAfterMs42837, failsDebug+ASAN exit times before -> after: N=5k 5561ms -> 348ms, N=10k 21506ms -> 482ms; after the fix teardown is linear through N=100k (3.1s exit, max cross-thread notify stall 1.7s, both debug; release is far lower). Also ran the
atomics-waitasync-wtftimer-uaf,timer-heap-race, andworker_threadssuites (all pass).[review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 0
evidence per changed file