Skip to content

perf(exec): keep 4 x threads parts ahead of a sink that merges states - #218

Merged
Hor911 merged 4 commits into
mainfrom
perf/merge-window
Oct 22, 2026
Merged

Hor911 merged 4 commits into
mainfrom
perf/merge-window

Conversation

@Hor911

@Hor911 Hor911 commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

Summary

A sink that merges each part into partial states -- the global, grouped and two-level aggregates and the top-N --
now lets 4 × threads parts run or wait ahead of its consumer instead of 2 × threads (MergeWindow); a GROUP BY's
lanes take the same value as their bound. A part union, a join's build and the probe a right join schedules itself
keep 2 × threads (PartWindow). Every part pipeline below a merging sink, a join's probe included, runs in the wider
window.

Why (ADR 0013 load balance, step 1's measurement, on a 2-socket, 64-core, 128-hardware-thread host): at 16 and 32
threads antb1's threads idle where DuckDB's do not, and the idle is the window. A part takes as long as the bytes it
reads; on the full ClickBench data one row group of the most uneven column holds about 6 times the mean, and while
it runs the parts after it finish and fill the window, which only taking that part frees. Traced part by part, Q20
at 16 threads kept its workers 68% busy with 28% of the threads' time lost to the full window; DuckDB, handing out
the same row groups without a window, kept 95% busy. With 4 × threads the same trace is 94% busy.

ADR 0013 is amended with the measurement (Decision, Memory and Load balance), including what the change does not
fix: from 64 threads the idle is the last parts, started in part order (step 2), and the merge's 32 lanes; on the
queries derived from TPC-H the idle is the consumer's own work (a right join's serial gather of its emitted rows,
ADR 0026's C2b).

Type of change

  • perf: performance improvement

Verification

Measured on the 128-thread host with two frozen builds (pixi run release of main 20b5c53 and of this change on
it, cmp-different), one process per query, best of 3 tries, two rounds with the arm order alternating per query
and per round; the host is shared, so every run records the CPU other processes used and disturbed points were
re-run. Full data (100 files). The branch was then rebased onto main b9337b9 without conflicts: #215-#217 change only
src/plan/ and other ADRs (none of the ClickBench queries has a join, so their plans are unchanged).

ClickBench, all 43 queries, total of the best times, main → this change:

threads main this change highest peak memory of a query
16 34.269 s 30.745 s 18.8 → 19.4 GB
32 22.190 s 21.011 s 20.3 → 20.7 GB
64 16.532 s 15.841 s 23.7 → 26.8 GB
128 16.370 s 16.380 s 29.7 → 31.4 GB

Largest moves: Q20 1.103 → 0.825 s at 16 threads and 0.527 → 0.368 s at 64; Q27 1.290 → 0.966 s and 0.649 →
0.511 s; Q28 4.303 → 3.602 s at 16. Slower beyond the noise: only Q23 at 64 threads (0.398 → 0.535 s; it gains at 16
and 32). Lane-heavy GROUP BYs (Q4, Q17, Q18) lean slower at 64 threads inside main's spread. At 128 threads the idle
is the last parts and the merge's 32 lanes, which this change does not touch.

The 22 queries derived from TPC-H (sf100), as a control, page cache verified resident: 16 threads 116.535 →
116.321 s, 64 threads 58.367 → 58.781 s; traced, Q1 and Q4 keep the same schedule (97% and 75% busy) with either
window. This workload is derived from the TPC-H Benchmark and is not comparable to published TPC-H Benchmark
results, as this implementation does not comply with all requirements of the TPC-H Benchmark.

Tests: MergingSinksKeepAWiderWindow checks both windows; EachSinkOpensItsWindowAheadOfAFailedFirstPart checks,
for every sink kind, that exactly its window's parts open ahead of a failed first part. Mutation-checked: with
MergeWindow at 2 × threads both fail; with only the GROUP BY's window_ at PartWindow the second fails.

$ pixi run check               (this branch, rebased on b9337b9)
lint: PASS
100% tests passed out of 2825

$ pixi run check-full          (the same change on 20b5c53, on the 128-thread Linux host)
clang-format / gersemi / ruff / tombi / markdownlint-cli2 / typos / shellcheck / actionlint / zizmor /
check_repo.py ... Passed
lint: PASS
ci (clang Debug -Werror): 100% tests passed out of 2815
asan (ASan+UBSan -Werror): 100% tests passed out of 2815
tidy: clean (a first run rejected the new test's initializers; fixed in its own commit)
coverage: 100% tests passed out of 2815; Coverage gate: PASS
fuzz-smoke: 100% tests passed out of 2
ci-gcc (GCC 15 Debug -Werror): 100% tests passed out of 2815
check-full exit=0

Checklist

  • pixi run check passes locally (lint + clang Debug -Werror + hermetic tests)
  • Tests cover the change
  • Docs updated: ADR 0013 (amended), docs/architecture.md, comments
  • No ClickBench-derived data is committed
  • Nothing derived from TPC-H is committed
  • Changes to governance paths were agreed with a maintainer: docs/adr/0013-parallel-execution.md is under
    CODEOWNERS and needs a maintainer's review

AI assistance

  • AI-assisted. Claude Code (Claude Opus 5.5) found the cause by measurement on the 128-thread host, wrote the
    change, the tests and the ADR amendment; a read-only reviewer agent reviewed the diff (5 findings, all fixed).
  • Accountable human (has read and understands the whole diff): @

🤖 Generated with Claude Code

Hor911 and others added 4 commits October 22, 2026 10:06
A sink that merges each part into partial states (global, grouped and
two-level aggregates, the top-N) now lets 4 x threads parts run or wait
ahead of its consumer instead of 2 x threads. Parts take as long as the
bytes they read, and one part several times the mean fills a window of
2 x threads with finished parts while it runs, so the other workers idle
until it is taken. A part union, whose parts hold their rows, and the
join builds and probes keep 2 x threads.

Tests: MergingSinksKeepAWiderWindow checks both windows;
EachSinkOpensItsWindowAheadOfAFailedFirstPart checks, for every sink kind,
that exactly its window's parts open ahead of a failed first part. Both
fail with the window back at 2 x threads.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review fixes. The GROUP BY sink now passes the window it gives its lanes
(window_) to its PartScheduler too, so EachSinkOpensItsWindowAheadOfAFailed
FirstPart covers the lanes' bound: setting window_ back to PartWindow fails
it (checked). The comments say which schedulers keep PartWindow (a part
union, a join's build, a right join's own probe) and that a probe under a
merging sink runs in MergeWindow, and no longer claim that a finished part
holds no rows (a routed GROUP BY part holds its rows split by partition, a
top-N part at most limit + offset). The test's expected scan set is
min(window, parts).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Amends ADR 0013 (Decision, Memory, Load balance) with step 1's
measurement on a 2-socket, 128-hardware-thread host: at 16 and 32 threads
the threads idle on the window, because a part's cost follows the bytes it
reads and one heavy part fills a window of 2 x threads. Records the paired
A/B of MergeWindow against main on all 43 ClickBench queries and the 22
queries derived from TPC-H as a control, and what the change does not fix
(the tail, the merge's 32 lanes, a right join's serial gather).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
clang-tidy (pixi run tidy) rejected the test's shape table: positional
aggregate initializers (modernize-use-designated-initializers), windows
computed in int and widened (bugprone-implicit-widening-of-multiplication-
result) and std::iota (modernize-use-ranges). No change in what it checks.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Hor911
Hor911 requested a review from lll-phill-lll as a code owner October 22, 2026 07:10
@claude

claude Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

Claude finished @Hor911's task in 23s —— View job


Claude review: no problems found

  • I found no P0 or P1 problems in the diff.
  • I read the window change in src/exec/part_operators.cc and its uses of window_ in the GROUP BY, two-level and top-N operators. I also read the new tests in parallel_test.cc and the ADR 0013 and docs/architecture.md amendments.
  • Window change: the scheduler windows and the GROUP BY lanes' bound all take MergeWindow. A part union, a join's build and the right-join probe keep PartWindow (hash_join.cc:256,432). Without an executor both windows are 1.
  • Tests: MergingSinksKeepAWiderWindow checks both window functions. EachSinkOpensItsWindowAheadOfAFailedFirstPart checks, for each sink kind, which parts open ahead of a failed first part.
  • I did not run the tests or the benchmarks. I could not check the performance figures in the PR description.
  • P2 note: peak memory rises with the wider window (the PR reports 23.7 → 26.8 GB at 64 threads). Q23 at 64 threads is slower, as the PR says.
  • The PR's TPC-H-derived timings carry the required disclaimer. They are not presented as TPC-H Benchmark results.
  • "Ask a human first" paths changed: none. docs/adr/0013-parallel-execution.md needs a maintainer's review because ADRs are under CODEOWNERS, and the PR checklist already flags this. The ADR's status is unchanged.

@github-actions

Copy link
Copy Markdown

Coverage

Module Lines Floor Branches Floor Status
common 98.07% (203/207) 92.9% 92.44% (110/119) 90.8% ok
sql 99.08% (3244/3274) 97.3% 97.64% (2190/2243) 95.6% ok
plan 95.53% (8989/9410) 93.5% 90.04% (4835/5370) 89.5% ok
io 94.30% (1241/1316) 93.2% 90.36% (722/799) 86.6% ok
exec 97.05% (11194/11534) 96.1% 88.64% (6315/7124) 85.1% ok
engine 95.98% (478/498) 93.0% 88.52% (239/270) 85.1% ok
cli 94.59% (647/684) 93.4% 87.08% (283/325) 86.4% ok
total 96.56% (25996/26923) 90.42% (14694/16250)

Coverage gate: PASS (floors: tools/ci/coverage_thresholds.json; they only go up).

Clang source-based coverage of the hermetic tests over src/<module>/ (module unit tests excluded); N/A: nothing to cover. Reproduce with pixi run coverage (details: build/coverage/report.txt).

@Hor911
Hor911 merged commit d9cda57 into main Oct 22, 2026
17 checks passed
@Hor911
Hor911 deleted the perf/merge-window branch October 22, 2026 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant