GC & Deduplicate String View on Spill#23565
Conversation
|
run benchmarks |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing spill-dedup-view-arrays (25cc81b) to f755cb4 (merge-base) diff using: clickbench_partitioned File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing spill-dedup-view-arrays (25cc81b) to f755cb4 (merge-base) diff using: tpcds File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing spill-dedup-view-arrays (25cc81b) to f755cb4 (merge-base) diff using: tpch File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
25cc81b to
d6079c7
Compare
|
Those benchmarks don't actually stress this path at all. In fact I don't think there are any standard benchmarks that will sweat this particular change. There is a |
Which issue does this PR close?
Rationale for this change
Spill files inflate string views via GC. I.e, a highly deduped string view can inflate considerably causing more memory and disk pressure.
What changes are included in this PR?
There is a new method added in the spill path to GC & Dedup string views
Are these changes tested?
Yes they are covered by existing tests.
However: I would like to make sure we run benchmarks with external sorting to see if this is a considerable regression. My instinct will tell me that, counterintuitively, this might be a performance increase when externally sorting since there is less bytes being moved around.
Are there any user-facing changes?
No this is an internal method change