Repository navigation
Three wake bugs in the fanout pool - #5
Merged
Merged
Conversation
added 7 commits
September 11, 2026 03:11
A job displaced from a worker's LIFO slot went to a private inbox behind it. Neither is stealable, and the path that was supposed to rescue them cannot run: the inbox drains into the private deque, the deque is shared only on a heartbeat counted in `mine.pop_back` calls, and the LIFO slot sits ahead of that in the loop. A task that keeps waking itself keeps the slot occupied, so the heartbeat never ticks, so nothing this worker holds ever becomes reachable. Measured on sixteen workers with eight self-waking read-only client tasks: four runs in eight collapsed to a single active worker with the other fifteen parked in `__ulock_wait`, at exactly the one-thread rate. That looks exactly like a lost wakeup and is not one, which is why the `SeqCst` Dekker pair did not touch it. Every wake was delivered. The work became invisible. `share_displaced` sends the displaced job to the injector instead, so at most one task is ever private to a worker. It is off by default and it is a policy rather than a fix: every displacement then costs an injector push and a wake, and on work that contends on row locks there is no parallelism to win and the churn is pure cost. A 50% update workload fell from 943,606 to 651,554 while cores busy went from 1.13 to 13.16. `nudge` also woke exactly one sleeper however many jobs had just been made stealable, so a worker sharing eight jobs published seven that nobody was told about. It now wakes one per job.
The slot sits ahead of everything in the worker loop, so a task that keeps waking itself is served from it indefinitely and the worker never reaches the `mine.pop_back` that counts down to the heartbeat. Anything it is holding for somebody else therefore never becomes stealable, however long the queue behind it grows. The cap existed but was a `const` nobody could reach. Lower means the worker looks at its own queue sooner, and that costs nothing when there is nothing there: `share` returns immediately unless the private queue holds at least two jobs. Which is what makes it a cheaper answer to a pile-up than routing displaced work to the injector, where a wake is paid whether or not anybody needed one. Measured on YCSB C, sixteen workers, eight self-waking client tasks. At the old 32 the run collapsed to the one-thread rate in one attempt out of six, with the slowest of six at 2,718,656. At 4 the slowest of six was 12,912,445 and the median rose from 12,466,259 to 13,398,239. It is not free: the same change costs a 50% update workload 3.7% and a read-modify-write workload 4.1%, which is why it is a tuning and not a new constant.
`share_displaced` sent the job through `push`, which also unparks a sleeper, and that wake was most of what the policy cost. On work that contends on row locks the woken worker steals the job, runs it, blocks on the same lock and parks again, so the pool pays a park and an unpark per displacement for work that was serialised anyway. The wake is not needed for the job to be found. A worker re-checks the injector after announcing sleep and before parking, so it cannot park while this job is queued, and the worker that displaced it is by definition awake and reaches the injector itself. Anybody already asleep is asleep because there was no work, and one job does not change that. Measured against the old engine, eight client threads, six paired rounds: the policy went from 14,734 behind on a 50% update workload to 4,428 behind, and from 55,526 behind on read-modify-write to 26,835. At sixteen threads on a read-only workload it took the same policy from a median of 11,671,193 to 15,049,665.
wake read sleeping.trailing_zeros() and unparked that worker without clearing the bit. The bit is not cleared until the woken worker actually runs, so every submit in a burst read the same candidate and unparked one worker over and over while the rest stayed parked. Every wake was delivered. They all went to the same worker, which is why this never looked like a lost wakeup and why the SeqCst Dekker pair around it was beside the point. Measured through nagoya on a 16 core machine, 16 workers and 32 tasks, counting cores kept busy, which is task CPU time over wall time, so 16.0 is the whole machine and higher is better. Worst of eight runs: tuning shape before after locality self-waking 3.9 10.3 locality single-poll 4.0 11.4 spread self-waking 2.9 12.6 spread single-poll 2.9 13.9 shared_slot self-waking 4.0 10.4 shared_slot single-poll 4.0 13.8 A fresh pool always reached about 12.7 and every pool that had parked once fell to about 4.0, on repeat, for every tuning. That degradation is gone. Tasks polled exactly once collapsed identically to self-waking ones. That rules out the LIFO slot, since a task polled once never touches it, and names the unpark path instead. nudge_many already masked each candidate it took, but only inside one call, which is why the bug was invisible there. Clearing the bit is that same rule held across calls. Clearing early only claims the sleeper: the worker clears it itself on resume and sets it again if it parks again. Two deterministic tests, checked by mutation, and no wall clock in either.
submit_local puts a job in one worker's LIFO slot or inbox. Neither is stealable. It then called wake(Some(worker)), but wake treats its argument as a fallback and prefers trailing_zeros() whenever anybody is asleep. So a job handed to worker 7 while worker 0 was also asleep unparked worker 0. Worker 0 woke, found nothing it was allowed to take, and parked again. Worker 7 slept on work no other worker could reach. A wake was sent every time, which is why this survived a hunt for a lost wakeup and the SeqCst fence added directly above it. The fence was needed and fixed a different miss; this one is not an ordering problem at all. The wake went to the wrong worker. This is the shape that shows up as one busy core with tasks outstanding, because the stranded worker is stranded for as long as nothing else happens to wake it. wake and wake_specific are now distinct and the doc comments say which is which: the difference is whether the work being announced is stealable. The regression test hands a job to worker 5 while every worker is asleep and asserts worker 5 is the one woken. Under the old call it reports "a job private to worker 5 woke [0]". Worker 5 rather than worker 1 on purpose, since a two-worker test where the holder is also the lowest sleeper cannot see this.
The condition was `taken > 1`, and injector_batch defaults to 1, so the
take loop can never take more than one and the nudge never fired for any
flavor that ships. It was dead code.
A worker that woke, found sixteen jobs queued and took one told nobody. It
then looped and took another, and another, draining the injector before its
neighbours had finished waking. Under local_wakes that concentration is
permanent, because a task's waker returns it to the worker that last polled
it, so whichever worker grabbed a task at startup owns it for the run.
YCSB C, 16 client threads, 16 workers, 1.5M operations, 16 repetitions:
min p50 max cpu_x ops/core
tokio 13,026,746 14,792,857 20,115,733 12.8 1,157,605
before 8,697,719 10,216,732 21,301,082 7.5 1,361,285
after 12,021,390 15,484,396 16,737,289 12.9 1,202,396
cpu_x 7.5 to 12.9 against tokio's 12.8 is the result. Per-core rates were
always within 3% of tokio, so the entire gap was cores left idle, and the
p50 above tokio's sits inside the overlap and is parity rather than a win.
The regression test asserts distinct workers woken, not timing. Against the
old condition it reports "7 jobs were left queued and 0 workers were woken".
pathscale
force-pushed
the
perf/local-wake-hoarding
branch
from
September 10, 2026 20:11
1ad9af5 to
9495ae3
Compare
Applied from `ps-st3-pr5-source-fixes.patch` in ~/code/patch, whose author states that nothing was compiled or run. It is compiled and run now: 44 tests pass, and WorkTable's 999 pass against this tree, which is what its `[patch.crates-io]` points at. Taken as written: A zero `lifo_run_limit` is rejected at construction. It would never consume the slot, so a job put there is stranded, and the failure is silent. The wake-selection path uses `match` rather than `let`-`else`, which was over the declared 1.60 floor. `wake_specific` reads the bitmap before it writes it. An already-awake owner no longer takes a shared cache line exclusively for a bit that is not set. The publication fence and the SeqCst load keep the Dekker pair intact. The worker services the injector at the LIFO fairness boundary, so a one-worker pool, or one whose peers are all busy, makes progress on displaced work instead of waiting behind self-wakes. Not taken: the wake on displacement. The patch adds `nudge(Some(worker))` after publishing a displaced job, against a comment that had removed exactly that call. The comment is right, and the review's objection is right too, and they do not actually conflict. The objection is starvation: an owner servicing self-wakes forever leaves the job behind while peers sleep. The fairness-boundary service above already fixes that, and it ends in `take_from_injector`, which wakes one sleeper per job still queued. The sleepers are told. They are told once per fairness boundary rather than once per displacement, which is the same news for a fraction of the round trips. A wake here would be a third mechanism for one job, and the one measured to cost the most: on lock-bound work the woken worker steals the job, blocks on the same row lock and parks again, paying a park/unpark per displacement for work that was serialised anyway. Its test asserted the wake happened. It asserts the absence now, and says why, so a future patch adding the wake back fails and has to argue with the comment rather than landing quietly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three wake bugs in the fanout pool, on top of the tunings already on this branch. Each has a deterministic test that fails when the fix is reverted, and none of them measures time.
1. A burst of submits woke the same worker every time
wakereadsleeping.trailing_zeros()and unparked that worker without clearing the bit. The bit is not cleared until the woken worker actually runs, so every submit in a burst read the same candidate and unparked one worker over and over while the rest stayed parked.Every wake was delivered. They all went to the same worker, which is why this never looked like a lost wakeup and why the
SeqCstDekker pair around it was beside the point.nudge_manyalready masked each candidate it took, but only inside one call. Clearing the bit is that same rule held across calls. Clearing early only claims the sleeper: the worker clears it itself on resume and sets it again if it parks again.2. Private work woke the wrong worker
submit_localputs a job in one worker's LIFO slot or inbox. Neither is stealable. It then calledwake(Some(worker)), butwaketreats its argument as a fallback and preferstrailing_zeros()whenever anybody is asleep.So a job handed to worker 7 while worker 0 was also asleep unparked worker 0, which found nothing it was allowed to take and parked again, while worker 7 slept on work no other worker could reach. That is the shape that shows up as one busy core with tasks outstanding.
The regression test reports
a job private to worker 5 woke [0]against the old call. Worker 5 rather than worker 1 on purpose: a two-worker test where the holder is also the lowest sleeper cannot see this.3. A nudge that could never fire
take_from_injectorended withif taken > 1 { self.nudge(None) }, andinjector_batchdefaults to 1, so the take loop can never take more than one and the condition is never true. Dead code for every flavor that ships.A worker that woke, found sixteen jobs queued and took one told nobody. It then looped and took another, and another, draining the injector before its neighbours had finished waking. Under
local_wakesthat concentration is permanent, because a task's waker returns it to the worker that last polled it.Measured
Through
nagoya, 16 workers, 32 tasks, cores kept busy (task CPU over wall time, so 16.0 is the whole machine), worst of eight runs:A fresh pool always reached about 12.7 and every pool that had parked once fell to about 4.0, on repeat, for every tuning. That degradation is gone.
Tasks polled exactly once collapsed identically to self-waking ones, which is what ruled out the LIFO slot and named the unpark path instead.
YCSB C at 16 threads through WorkTable, 1.5M operations, 16 repetitions:
cpu_x7.5 to 12.9 against tokio's 12.8, with per-core rates within 3% of tokio throughout. The whole gap was cores left idle.Release note
0.6.1 is unpublished, so these fold into it.
nagoyarequiresps-st3 ^0.6.1for exactly this reason:^0.6accepts 0.6.0, and a lockfile pinning it would build a pool without any of these and say nothing.