adriangb commented on PR #24857: URL: https://github.com/apache/datafusion/pull/24857#issuecomment-5513496232
## Summary of benchmarks and measurements Consolidating the evidence for this PR in one place, including the parts that are not yet settled. ### 1. Minimum viable memory budget (strongest evidence) Two tests added in `datafusion/core/tests/memory_limit/`. The query is `select group_key, count(distinct value), avg(payload) from t group by group_key` over 4,000 keys with 2 distinct short values each, `target_partitions = 1`, spilling disabled. The minimum memory limit at which it completes, bisected on each side: | value column | before | after | reduction | | --- | --- | --- | --- | | `Utf8` | fails 34 MB, passes 36 MB | fails 1.8 MB, passes 2.0 MB | ~18x | | `Utf8View` | fails 120 MB, passes 124 MB | fails 2.4 MB, passes 2.6 MB | ~46x | The tests are pinned at 8 MB and 16 MB, at least 4x clear of both cliffs, and were verified to fail on the merge-base and pass on this branch, with five consecutive repeat runs green. The `avg(payload)` is deliberate rather than incidental: a non-distinct `count` would let `SingleDistinctToGroupBy` rewrite the aggregate away, so the per-group accumulators would never exist and the test would pass for the wrong reason. `avg` cannot be admitted by that rule under any extension, because averaging per-group averages is arithmetically wrong. ### 2. Per-accumulator footprint One per-group accumulator holding a single 24-byte value, measured on the merge-base and on this branch: | | before, actual | before, reported | after | | --- | --- | --- | --- | | `BytesDistinctCountAccumulator` | 14,648 B | 8,240 B | 180 B | | `BytesViewDistinctCountAccumulator` | 33,920 B | 28,792 B | 260 B | Two separate defects show up in that table. The pre-allocation is the large one. The reporting gap is the second: `ArrowBytesViewMap` seeded `map_size` from `capacity() * size_of::<Entry<V>>()`, which omits the control bytes and under-reports by 1.18x, while `ArrowBytesMap` seeded it with `0` despite pre-allocating, so any map staying under its pre-allocated capacity reported its hash table as free indefinitely. ### 3. Benchmarks, memory `clickbench_extended` at `DATAFUSION_RUNTIME_MEMORY_LIMIT: 4G` ([results](https://github.com/apache/datafusion/pull/24857#issuecomment-5500755168)). Q2 is `SELECT "BrowserCountry", COUNT(DISTINCT "SocialNetwork"), COUNT(DISTINCT "HitColor"), COUNT(DISTINCT "BrowserLanguage"), COUNT(DISTINCT "SocialAction") FROM hits GROUP BY 1`, which is the only query in the benchmark suites that puts a grouped `COUNT(DISTINCT)` on a non-integer column and therefore the only one that reaches this code: | query | base | changed | change | | --- | --- | --- | --- | | Q2 (grouped, 4 string distincts) | 98.6 MiB | 11.7 MiB | **-88.1%** | | Q1 (ungrouped string distincts) | 3.4 MiB | 2.4 MiB | -29.4% | | Q12 | 1.3 MiB | 1.0 MiB | -21.2% | Everything else moves by less than 5% in either direction. Worth stating plainly: this rests on a single run of a single query, because no other benchmark query exercises the path. The memory-limit tests in section 1 are the more reliable evidence. Other suites at the same limit: [`clickbench_partitioned`](https://github.com/apache/datafusion/pull/24857#issuecomment-5500500275), [`external_aggr`](https://github.com/apache/datafusion/pull/24857#issuecomment-5500609445). Neither contains a grouped non-integer distinct count, so neither shows movement, as expected. ### 4. Benchmarks, latency No effect, which is the expected result for an allocation-sizing change. `clickbench_extended` totals 37,961 ms against 37,367 ms, with 3 queries faster, 1 slower and 10 unchanged, all inside run-to-run noise. No query failed and no spilling occurred on either side. ### 5. Open: peak RSS Peak process RSS rose on all three runs: 9.3 to 9.6 GiB on `clickbench_partitioned`, 418.8 to 445.6 MiB on `external_aggr`, and 9.9 to 10.7 GiB on `clickbench_extended`. The direction is consistent and the magnitude roughly tracks how much each suite uses the changed path. It is not yet clear whether that is real. A companion accounting-only change that cannot alter allocation moved RSS by -8.1%, -7.4% and +3.6% across the same three suites, so this harness shows at least +/-8% single-run spread and these deltas sit inside it. Repeat runs, including base-against-base to establish a null distribution, are in progress and will be posted here. If the effect is real, the likely mechanism is allocator retention rather than live memory: growing a table from zero by doubling costs log2(N) reallocations per accumulator where there was previously one up-front allocation, and freed blocks return to allocator free lists rather than to the OS. The mitigation would be a small non-zero initial capacity, on the order of 8 entries, which keeps nearly all of the reduction (most per-group sets hold fewer than ten values) while eliminating the growth steps for the common case. That variant will be measured before any recommendation is made. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
