perf(pool): size the auto pool by physical cores, not logical CPUs - #609
Closed
singaraiona wants to merge 2 commits into
Closed
singaraiona wants to merge 2 commits into
singaraiona wants to merge 2 commits into
Conversation
The pool recruited one participant per online logical CPU. Two SMT siblings
split one core's issue bandwidth, so the second thread re-runs the same
instruction stream at roughly half the IPC rather than adding throughput.
ray_physical_core_count already existed for exactly this reason — its own
comment records the full suite running ~11% slower with 32 SMT threads than
with 16 physical cores on a 5950X — and was used only to estimate a cache
size, never to size the pool.
Full ClickBench (10M rows, 43 queries x 3, three repetitions of the whole
suite per configuration, same build and same parted store on both sides):
host cycles instructions sum of hot times
Ryzen AI 7 350 (8/16) -31.6% -0.4% -1.6%
i7-10700K (8/16) -34.4% -0.3% -0.8%
Instructions being flat is the point: the saving is stall cycles, not work
removed. Cycle counts varied under 0.5% across repetitions; the hot-time
column is inside the run-to-run spread on both hosts (5.7% and 1.8% at -c 16),
so the claim is no measurable wall-time regression rather than a speedup.
Physical cores were also the more stable configuration on both.
On the reported join shape the same mechanism shows directly: 22.74 G vs
22.71 G instructions — identical — at IPC 1.51 vs 3.20, for 15.11 G vs
7.50 G cycles and no wall-clock difference.
Not free everywhere. A 4-physical/8-logical i7-6700 pays ~9.4% on hot times
for the same ~50% cycle saving, because it surrenders half its threads and has
only four cores to fall back on; measured with interleaved A/B runs, +9.6 /
+9.1 / +9.5% across three reps. Latency-bound deployments on low core-count
SMT hosts should set -c explicitly, which is the existing knob for it.
One query regresses consistently across all three machines: q20,
(count (select {from: hits where: (like URL "*google*")})), +16% to +21%.
ClickBench's URL column is 2.62M distinct values in a ~303 MB symbol heap, so
that query is millions of random dereferences into a heap far larger than
cache followed by short branchy scans — latency-bound, the one shape a sibling
thread genuinely fills. `like` itself is not at fault: its cost tracks
distinct symbols rather than rows, so the SYM optimisation is already there,
and the SMT advantage appears only once the heap stops fitting in cache. A
carve-out keeping such scans wide would rest on a real property, but it is one
query against a whole-suite effect that is inside the noise, so it is left as a
possible follow-up rather than bundled here.
RAYFORCE_CORES and -c are unaffected, and ray_physical_core_count falls back to
the logical count when the topology cannot be read, so systems whose topology
is unreadable keep the old behaviour.
Closes #606
Rayforce targeted audit passedThe required Rayforce audit gate passed on the latest run. Workflow run: https://github.com/RayforceDB/rayforce/actions/runs/35633156797 |
Two defects the audit caught in the previous commit, both introduced by it. platform.h declares ray_physical_core_count unconditionally, but platform.c defined it only in the POSIX and Windows arms. pool.c compiles on every platform, so moving the auto-size path onto it left the WASM arm referencing a symbol that does not exist — the line previously called ray_thread_count, which that arm does provide. Nothing in the repo builds WASM today, so CI could not have caught it; it is a maintained portability arm regressed by a diff that never looked at it. Defined the same way the Windows arm does, and the WASM threading comment no longer justifies its single-threaded invariant in terms of a function the pool has stopped calling. docs/docs/language/repl.md stated "By default the pool includes every online logical CPU, including SMT threads" — the exact default the previous commit inverts, and the only prose a user has for this behaviour. It now describes the physical-core default, says why, and keeps the -c guidance for a latency-bound service on a low core-count SMT host, which is where the wider pool still wins wall-clock time. docs/grouping-engine-scaling-plan.md carried the same stale assumption in a checklist item. .sys.info's `cores` is left alone: it reports _SC_NPROCESSORS_ONLN, a hardware fact rather than the pool width, and docs/docs/guides/memory.md describes it correctly. Also strengthened pool/auto_physical_cores, which the audit fairly called close to a tautology: it asserts against the same function pool.c calls, so on a non-SMT runner — physical == logical, the common CI case — it cannot distinguish the new policy from the old. It now also pins physical <= logical and total <= logical, so on an SMT host a revert to the logical count fails here instead of passing quietly. The limitation is written down next to it.
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
The pool recruited one participant per online logical CPU. Two SMT siblings
split one core's issue bandwidth, so the second thread re-runs the same
instruction stream at roughly half the IPC rather than adding throughput.
ray_physical_core_count()already existed for exactly this reason — its owncomment records the suite running ~11% slower with 32 SMT threads than with 16
physical cores on a 5950X — and was used only to estimate a cache size, never to
size the pool.
Measurements
Full ClickBench, 10M rows, 43 queries × 3, three repetitions of the whole suite
per configuration, same build and same parted store on both sides. Two of the
three hosts were measured by @vbmithr on #606.
Instructions being flat is the point — the saving is stall cycles, not work
removed. Cycle counts varied under 0.5% across repetitions. The hot-time column
is inside the run-to-run spread on both hosts (5.7% and 1.8% at
-c 16), so theclaim is no measurable wall-time regression, not a speedup. Physical cores were
also the more stable configuration on both.
On the join shape from #599 the mechanism shows directly:
Where it is not free
A 4-physical/8-logical i7-6700 pays +9.4% on hot times for the same ~50%
cycle saving — it surrenders half its threads and has only four cores to fall
back on. Measured with interleaved A/B runs to cancel machine drift: +9.6 / +9.1
/ +9.5% across three reps, baseline stable to 0.4 ms. Latency-bound deployments
on low core-count SMT hosts should set
-cexplicitly, which is the existingknob for it.
The one consistent per-query regression
q20 —
(count (select {from: hits where: (like URL "*google*")}))— regresses+16% to +21% on all three machines, and it is the only query that does.
ClickBench's
URLis 2.62M distinct values in a ~303 MB symbol heap, so thatquery is millions of random dereferences into a heap far larger than cache
followed by short branchy substring scans: latency-bound, which is the one shape
a sibling thread genuinely fills.
likeitself is not at fault — its cost tracksdistinct symbols rather than rows, so the SYM optimisation is already there — and
the SMT advantage appears only once the heap stops fitting in cache (+9.8% at
2.62M distinct, none at 100k).
A carve-out keeping such scans wide would rest on a real property, but it is one
query against a whole-suite effect that is inside the noise. Left as a possible
follow-up rather than bundled here.
Compatibility
RAYFORCE_CORESand-care unaffected.ray_physical_core_countfalls back tothe logical count when the topology cannot be read, so systems whose topology is
unreadable keep the old behaviour.
test_pool.c's policy test is updated fromauto_all_logical_cpustoauto_physical_cores.Closes #606
Checklist
dev(notmaster)feat:/fix:/perf:/docs:/ …)makebuilds cleanly (no new warnings)make testpasses; tests added/updated for behaviour changes