perf(pool): wake only the workers a dispatch can keep busy - #605
Merged
Merged
Conversation
ray_pool_dispatch and ray_pool_dispatch_n signalled the whole pool on every dispatch, however narrow the window. The main thread participates as worker 0, so at most n_tasks-1 helpers can ever claim anything; the surplus threads woke, raced to an already-drained window, and went straight back to the semaphore. On a dispatch narrower than the machine that is pure overhead — a partition-parallel step over three partitions woke every core on the box to run two tasks. Signal min(n_tasks-1, n_workers) instead. Signalling FEWER is safe: completion is governed by the `pending` spin-wait, never by the signal count, so an unsignalled worker simply stays asleep, and under-signalling cannot produce the surplus-signal problem between consecutive dispatches that the spin-wait exists to avoid. Measured (10M-row ClickBench, 43 queries x 3, splayed): sum of per-query hot times 5690.7 ms -> 5667.3 ms (-0.4%, within noise), user CPU 55.1 s -> 54.8 s. So this is waste removal rather than a speedup on workloads whose dispatches are already wider than the pool. Not a fix for #599. That report's shape — a 130k-row join probe, which splits into 16 tasks against 8 workers — is unaffected, because the clamp lands on the same worker count it already used (measured: 5.19 s -> 5.18 s of CPU, unchanged). The investigation there points at the pool defaulting to logical rather than physical cores; that is a separate change with its own benchmark trade and is filed on its own. test_pool.c gains pool/dispatch_narrow, which is the case the existing coverage missed: dispatch_n_small uses 4 tasks on a 2-worker pool, so the clamp never binds. The new test drives windows of 1..4 tasks on a 4-worker pool, then alternates narrow and wide windows 25 times over the same pool, asserting exact task and element counts throughout — the two risks the change introduces are a task nobody claims and signal accounting that drifts across dispatches and starves a later wide window.
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.
What & why
ray_pool_dispatchandray_pool_dispatch_nsignalled the whole pool on everydispatch, however narrow the window. The main thread participates as worker 0,
so at most
n_tasks - 1helpers can ever claim anything — the surplus threadswoke, raced to an already-drained window, and went straight back to the
semaphore. A partition-parallel step over three partitions woke every core on
the box to run two tasks.
This signals
min(n_tasks - 1, n_workers)instead. Signalling fewer is safe:completion is governed by the
pendingspin-wait, never by the signal count, soan unsignalled worker simply stays asleep — and under-signalling cannot produce
the surplus-signal problem between consecutive dispatches that the spin-wait
exists to avoid in the first place.
Measurements
ClickBench, 10M rows, splayed, 43 queries × 3 runs, this box (i7-6700, 4
physical / 8 logical):
So: waste removal, not a speedup. Workloads whose dispatches are already wider
than the pool see nothing, which is most analytical work. The gain is confined
to dispatches narrower than the machine.
This is not a fix for #599
Worth stating plainly, since that issue is what prompted the change. The
reported shape — a 130k-row join probe, which splits into 16 tasks against 8
workers — is unaffected, because the clamp lands on the same worker count it
already used. Measured on that reproducer: 5.19 s → 5.18 s of CPU, wall
unchanged.
The investigation behind #599 points somewhere else: the pool defaults to
logical CPUs (
pool.c→ray_thread_count()) while the tree already carriesray_physical_core_count()for exactly this reason, used only for a cache-sizeestimate. Switching that default halves CPU on the reporter's shape (5.19 s →
2.58 s, wall 0.70 s → 0.66 s) but costs +2.7% on ClickBench's published
per-query hot times on this box, so it is a separate change with its own
benchmark trade. Filed separately with the full data rather than smuggled in
here.
Tests
pool/dispatch_narrowcovers the case the existing suite missed:dispatch_n_smalluses 4 tasks on a 2-worker pool, so the clamp never binds.The new test drives windows of 1..4 tasks on a 4-worker pool, then alternates
narrow and wide windows 25 times over one pool, asserting exact task and element
counts throughout. That targets the two risks the change introduces: a task
nobody claims, and signal accounting that drifts across dispatches and starves a
later wide window.
Checklist
dev(notmaster)feat:/fix:/perf:/docs:/ …)makebuilds cleanly (no new warnings)make testpasses; tests added/updated for behaviour changes