perf: BlockedVec storage for aggregations - #24815
Draft
Dandandan wants to merge 3 commits into
Draft
Conversation
Growing one allocation copies everything already in it, so a grouping with millions of groups spends much of its time reallocating its per-group state. `BlockedVec` keeps that state in one `Vec` up to a threshold and in fixed-size blocks above it, so growing appends a block instead of copying. Blocks cost a second load per group update, so the threshold keeps small and medium groupings on exactly the path they use today. Accumulators resolve the representation once per batch via `storage_mut` rather than per group, which keeps the flat path indistinguishable from a plain `Vec`. Wired into count, sum (`PrimitiveGroupsAccumulator`) and avg. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0117yCevpWYN82FH6PLrM8qu
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #24815 +/- ##
==========================================
- Coverage 81.52% 81.51% -0.02%
==========================================
Files 1123 1125 +2
Lines 406194 406628 +434
Branches 406194 406628 +434
==========================================
+ Hits 331163 331476 +313
- Misses 55664 55781 +117
- Partials 19367 19371 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The threshold was eight blocks, so the state was chopped into eight pieces when it switched, copying all of it. Setting the threshold to one block means the flat allocation *is* block zero and is handed over as it stands. A block is now 2MB for an eight byte state, a whole number of output batches, and the flat state starts at one batch rather than growing into one from nothing. `take_first` can still leave more than a block behind, so the splitting path stays for that case, covered by the existing test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0117yCevpWYN82FH6PLrM8qu
Dandandan
changed the title
BlockedVec storage for aggregations
Contributor
Author
|
run benchmark clickbench |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/aggregate-state-growth (e3fe1da) to 9c44fc0 (merge-base) diff Run configurationrun benchmark clickbenchResults will be posted here when complete File an issue against this benchmark runner |
The group keys are the other half of what a grouping stores per group, and they grew the same way: q32 keeps 12 bytes of key next to 32 bytes of aggregate state, all of it in allocations that were copied on every growth. `BlockedVec` moves to `datafusion-common` so `physical-plan` can use it, and `PrimitiveGroupValueBuilder` stores its values in it. Both comparison loops resolve the representation once and are compiled per representation, so the flat path is unchanged. The flat state no longer starts at a batch: reserving one up front costs 64KB per state vector, which `ordered_aggregate_spill.slt` showed exhausting a 600KB pool. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0117yCevpWYN82FH6PLrM8qu
|
Benchmark for this request failed before finishing (Kubernetes reason: Benchmarks requested: Kubernetes messageFile an issue against this benchmark runner |
11 tasks
Contributor
Author
|
run benchmark clickbench_partitioned |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/aggregate-state-growth (fe0a008) to 9c44fc0 (merge-base) diff Run configurationrun benchmark clickbench_partitionedResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing perf/aggregate-state-growth (fe0a008) to 9c44fc0 (merge-base) diff Run configurationrun benchmark clickbench_partitionedCPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
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
3 participants
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.
Which issue does this PR close?
Rationale for this change
Growing one allocation copies everything in it, so a grouping with millions of groups spends much of its time reallocating the state it keeps per group.
What changes are included in this PR?
BlockedVecindatafusion-common: per-group state in oneVecup toTHRESHOLD_LENgroups, fixed-size blocks above it, so growth appends a block instead of copying.storage/storage_mut) instead of per group, so the flat path stays identical to a plainVec.count,sum(PrimitiveGroupsAccumulator),avg, and byPrimitiveGroupValueBuilderfor the group keys.Are these changes tested?
take_firstleaving more than a block, and capacity accounting; the aggregate and group-values test suites and thesqllogictestaggregate files pass.Are there any user-facing changes?
size()now reports block capacity, which is the same quantity measured differently.Benchmarks
Not covered yet
NullStatebitmaps, and the other accumulators still use flat storage.