Repository navigation
Conversation
42169b5 to
e756fe9
Compare
a2cb97e to
522db22
Compare
|
run benchmark first_last env: |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/first-last-sparse-group-scoreboard (522db22) to e30aff5 (merge-base) diff Run configurationrun benchmark first_last
env:
BENCH_FILTER: "^(first_value|last_value) (update_bench|merge_bench)"Results will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing perf/first-last-sparse-group-scoreboard (522db22) to e30aff5 (merge-base) diff Run configurationrun benchmark first_last
env:
BENCH_FILTER: "^(first_value|last_value) (update_bench|merge_bench)"CPU Details (lscpu)Details
Resource Usagefirst_last — base (merge-base)
first_last — branch
File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing 522db22 (522db22) to e30aff5 diff Run configurationrun benchmark first_last
env:
BENCH_FILTER: "^last_value update_bench (list|map|struct)"
baseline:
ref: "e30aff54957c9e16912f5c1102282e2af146f90e"
changed:
ref: "522db22102ee2cb60b922a3532dadb7b38449be9"Results will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing 522db22 (522db22) to e30aff5 diff Run configurationrun benchmark first_last
env:
BENCH_FILTER: "^last_value update_bench (list|map|struct)"
baseline:
ref: "e30aff54957c9e16912f5c1102282e2af146f90e"
changed:
ref: "522db22102ee2cb60b922a3532dadb7b38449be9"CPU Details (lscpu)Details
Resource Usagefirst_last — base (merge-base)
first_last — branch
File an issue against this benchmark runner |
|
@tanmay442 Thank you for working on this. The rerun reproduced slowdowns in all eight cases: approximately 0.8–3.4%, compared with 1.1–2.5% in the first run. The largest was list[4] with no nulls: 32.6 → 33.7 ms. Could you investigate this regression? |
…d groups The grouped `first_value` / `last_value` accumulator keeps a scoreboard with one bit per group to record which group already has a winning row in the current batch. At the end of the batch it swept the whole scoreboard to find the groups that had a winner, so the work per batch grew with the number of groups in the query rather than with the number of groups the batch touched. The file already keeps the list of groups a batch touched, for the pre-ordered fast path. The tournament path now uses that list as well: it records the groups it touches while it walks the rows, collects the winners from the list, and clears exactly those bits again instead of resetting the whole scoreboard. The per-batch work is now proportional to the number of groups the batch touched. The result does not change: the same groups win, and the callers only do per-group work, so the order of the winners does not matter. With both paths restoring the all-false invariant between batches, the `extreme_buf_dirty` flag that told the pre-ordered path to reset the whole scoreboard after an interleaved `merge_batch` becomes dead and is removed. Both paths now clear their own bits before any fallible work, so the invariant is restored even when a batch fails.
522db22 to
0e0a63c
Compare
|
@kumarUjjawal I rewrote the implementation after your run, so the numbers you measured belong to the previous version. The change is a complete overhaul of the mechanism, not of the scope. The pull request still touches one file. The tournament path now collects its winners from the list of groups the batch touched. Could you trigger the benchmark runner on this pull request so i can get feedback ? Please run the whole first_last suite. The eight nested last_value update_bench rows then land in the same table as everything else. If those eight rows come back slower, could you run the same trigger on #26554 as well? There the library is the same and only the benchmark file differs, which separates the harness from the code. |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing 0e0a63c (0e0a63c) to f9bc5a3 diff Run configurationrun benchmark first_last
baseline:
ref: "f9bc5a3fc60cc7f4319cd2c156cb3d024a8c1b21"
changed:
ref: "0e0a63c2f35b1fee2122cf3df5d0e75f477f1348"Results will be posted here when complete File an issue against this benchmark runner |
|
@tanmay442 can you please not paste the whole llm response its hard to figure out what's important from the wall of text. See https://datafusion.apache.org/contributor-guide/ai-policy.html |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing 0e0a63c (0e0a63c) to f9bc5a3 diff Run configurationrun benchmark first_last
baseline:
ref: "f9bc5a3fc60cc7f4319cd2c156cb3d024a8c1b21"
changed:
ref: "0e0a63c2f35b1fee2122cf3df5d0e75f477f1348"CPU Details (lscpu)Details
Resource Usagefirst_last — base (merge-base)
first_last — branch
File an issue against this benchmark runner |
@kumarUjjawal |
Note
State of this PR. One file,
src/first_last.rs(+130/-48). It replaces theper-batch sweep of the per-group scoreboard with the list of groups the batch
touched, the idiom the pre-ordered path has used since #25050. The invariant
is that the scoreboard is all-false between batches, and both paths clear
their own bits.
No benchmark-runner measurement of this implementation exists yet. The two
benchmark reports in the thread were run on
522db22, an earlierBitIndexIteratorversion that was force-pushed away on 21 Oct. Rows with anempty baseline column are unpaired, not wins. The new cases need #26554 in
the merge base; the eight rows that came out slower do not, because both
sides of this pull request already carry the same benchmark file, and a run
of just those eight is what would settle them.
Which issue does this PR close?
Rationale for this change
This is a performance change. The rows a query returns, the error messages, the
log lines and the row order are the same.
first_value(x ORDER BY y)andlast_value(x ORDER BY y)get slower as thenumber of groups in the query grows. The grouped accumulator keeps a scoreboard
with one bit per group to record which group already has a winning row in the
current batch. At the end of the batch it swept the whole scoreboard to find
the groups that had a winner, so the work per batch grew with the number of
groups in the query rather than with the number of groups the batch touched.
The file already keeps the list of groups a batch touched, for the pre-ordered
fast path added in #25050. This change uses that list in the tournament path as
well.
What changes are included in this PR?
One file,
datafusion/functions-aggregate/src/first_last.rs.get_filtered_extreme_of_each_groupused to reset the scoreboard at the startof every batch and then sweep it to collect the winners. It now records the
groups it touches while it walks the rows, collects the winners from that list,
and clears exactly those bits again. The per-batch work is proportional to the
number of groups the batch touched.
With both paths restoring the all-false invariant between batches, the
extreme_buf_dirtyflag that told the pre-ordered path to reset the wholescoreboard after an interleaved
merge_batchis dead, and is removed. Thepre-ordered path now clears its bits before its loop, so the invariant holds
even when a batch fails.
size()now countstouched_groups_buf, which the tournament path populatesand the accumulator keeps.
The benchmark for this change is in #26554, a separate pull request, as the
contributor guide suggests. The benchmark runner pairs benchmarks by name, so
the new cases need #26554 in the merge base before they can be compared. This
pull request will be rebased once #26554 merges.
Why the behaviour does not change
scoreboard bit was set. The new code collects the same groups from the list it
built while walking the rows. The winner row of each group is the same.
than in group order. Both callers (
update_batchandmerge_batch) read andwrite state per group, so neither observes the order.
path.
resize_statessizes the scoreboard and the winner index arraytogether, so a group index is always inside the array.
Notes for the reviewer
state per group, so the per-group work and its order are the whole story.
The new test
scoreboard_is_cleared_between_batcheschecks it, and theexisting
pre_ordered_update_after_merge_interleavechecks that the twopaths can interleave on one accumulator.
of touched groups. The winners vector itself is not new.
What is the testing strategy for this PR?
cargo test -p datafusion-functions-aggregate --libpasses, 255 tests, and thefull sqllogictest suite passes, 535 of 535 test files.
cargo test -p datafusion --test core_integrationpasses, 1442 tests.cargo clippy -p datafusion-functions-aggregate --all-targets -- -D warningsisclean.
The invariant that the scoreboard is all-false between batches cannot be
reached through SQL, so it is covered by unit tests in the module:
winners_are_the_groups_the_batch_touchedchecks that the winners of a batchare exactly the groups that batch touched, for both
FIRST_VALUEandLAST_VALUE, over group counts of 1, 63, 64, 65, 127, 128, 129 and 100,000.scoreboard_is_cleared_between_batcheschecks that the scoreboard isall-false after a batch and that a group touched by the next batch is
treated as new. Removing the clearing makes both assertions fail: the
all-false check, and then the value check with the group keeping its old
value.
The existing test
pre_ordered_update_after_merge_interleavecovers theinterleaving of the two paths on one accumulator.
Measurements of the current implementation
Author-local, on an x86 machine, compared by criterion against each other. The
base is the benchmark commit
b15f964and the change is3934ca41, which isthe same benchmark file with the accumulator change applied. These are not the
project runner's numbers, and the machine is not the runner's.
The
distinct_batchescases come first, because each batch owns a disjointrange of groups, so every batch writes the state of every winner. That is the
work a query does, and it is what the change has to be judged by.
first_value update_bench_many_groups_distinct_batches groups=1000000, touched=64first_value update_bench_many_groups_distinct_batches groups=1000000, touched=8192last_value update_bench_many_groups_distinct_batches groups=1000000, touched=64last_value update_bench_many_groups_distinct_batches groups=1000000, touched=8192The
many_groupscases replay one batch, so after the first iteration theaccumulator already holds that batch's extreme and the state writes do not
happen. They isolate the per-batch bookkeeping and are not a query shape.
first_value update_bench_many_groups groups=1000000, touched=64first_value update_bench_many_groups groups=1000000, touched=8192last_value update_bench_many_groups groups=1000000, touched=64last_value update_bench_many_groups groups=1000000, touched=8192The
densecase is the other end, 100,000 groups and a 100,000 row batch, soevery batch touches every group. That is the shape where the touched list is
longest and the added cost is highest, so it is an upper bound on the price.
The change is flat there. The eight rows that came out slower are 1,024 groups
with 64 rows per group, where the touched list is proportionally far shorter.
first_value update_bench_many_groups_dense groups=100000last_value update_bench_many_groups_dense groups=100000Measurements from the replaced revision (retained for chronology)
Author-local, on
522db22, the earlierBitIndexIteratorversion. End to endthrough the datafusion CLI, four partitions:
These are a lower bound for the shape of the effect, not evidence for this
diff.
Open item
The eight nested
last_value update_benchrows measured +1.07% to +2.45% inthe full run and +0.84% to +3.37% in the eight-case rerun, both on the
replaced revision. They are not measured on the project runner for the current
head.
They are measurable now, without #26554, because both sides of this pull
request carry the same benchmark file and a run of just those eight rows would
compare the current head against its parent:
A bench-only null run is what attributes them, because the library is then
identical and the benchmark file is the only difference. Run locally on an x86
machine, the eight nested rows moved between -9.5% and -1.0% and the primitive
last_value update_benchrows between +1.5% and +4.4%, which is the oppositesign on each group to what the runner measured. The sign follows the case and
not the code. The runner's own null run, main against #26554, is the version to
quote.
Are there any user-facing changes?
No. The same groups win, with the same rows, in the same order.