perf: Concrete TopK array storage - #23609
Conversation
|
run benchmark topk_aggregate |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing concrete-topk-storage (8a7b678) to 95de385 (merge-base) diff using: topk_aggregate File an issue against this benchmark runner |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #23609 +/- ##
=======================================
Coverage 80.87% 80.87%
=======================================
Files 1101 1101
Lines 375765 375733 -32
Branches 375765 375733 -32
=======================================
- Hits 303915 303891 -24
+ Misses 53747 53735 -12
- Partials 18103 18107 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetopk_aggregate — base (merge-base)
topk_aggregate — branch
File an issue against this benchmark runner |
|
Benchmarks look good -- I am running them once more to see if htey are reproducable |
|
run benchmark topk_aggregate |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing concrete-topk-storage (8a7b678) to 95de385 (merge-base) diff using: topk_aggregate File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetopk_aggregate — base (merge-base)
topk_aggregate — branch
File an issue against this benchmark runner |
|
🤔 These two seem to reproducibly get worse Can you look into that @MassivePizza ? |
|
Only getting noise locally. I don't think my changes should affect "no TopK" regardless. |
avoiding from(Vec) into drop.
|
run benchmark topk_aggregate |
Retriggered the benchmark, let's see if it can be reproduced |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing concrete-topk-storage (5e07c8d) to e8a65f2 (merge-base) diff using: topk_aggregate File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetopk_aggregate — base (merge-base)
topk_aggregate — branch
File an issue against this benchmark runner |
|
Still seeing a weird 10% there that I can't repro locally, but the rest looks good 😕 |
|
run benchmark topk_aggregate |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing concrete-topk-storage (5e07c8d) to e8a65f2 (merge-base) diff Run configurationrun benchmark topk_aggregateResults will be posted here when complete File an issue against this benchmark runner |
|
This is very nice @MassivePizza and @xudong963 and @jayzhan211 -- I think it is close. |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing concrete-topk-storage (5e07c8d) to e8a65f2 (merge-base) diff Run configurationrun benchmark topk_aggregateCPU Details (lscpu)Details
Resource Usagetopk_aggregate — base (merge-base)
topk_aggregate — branch
File an issue against this benchmark runner |
|
This PR feels pretty solid on its own now.
|
|
run benchmark topk_aggregate |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing concrete-topk-storage (e5861d2) to 62650ef (merge-base) diff Run configurationrun benchmark topk_aggregateResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing concrete-topk-storage (e5861d2) to 62650ef (merge-base) diff Run configurationrun benchmark topk_aggregateCPU Details (lscpu)Details
Resource Usagetopk_aggregate — base (merge-base)
topk_aggregate — branch
File an issue against this benchmark runner |
Which issue does this PR close?
N/A
Rationale for this change
Eliminate the overhead of casting with
dyn Anyfor every item.What changes are included in this PR?
Cast once per batch and store the typed array.
PrimitiveHashTables was already generic, so this was a big and simple win.
StringHashTable was made
to use an enum for consistency/readabilitygeneric for a miniscule perf improvement over enum.Are these changes tested?
Should be covered by existing tests.
For benching try
cargo bench -p datafusion --bench topk_aggregate -- "top k=10 aggregate".Are there any user-facing changes?
None.