adriangb commented on PR #24857:
URL: https://github.com/apache/datafusion/pull/24857#issuecomment-5528773208
## Query 0: six runs, and a correction to the mechanism I gave earlier
An earlier comment on this PR put Query 0's pool-peak increase at +4.2% from
four readings, and
explained it as a transient where an old and a new hash table are both live
during a doubling.
**That mechanism is wrong.** Both the figure and the explanation are
corrected below.
### Six runs on the current head
All six ran `clickbench_extended` at `DATAFUSION_RUNTIME_MEMORY_LIMIT: 4G`
against head
`b9e6252ece`.
| run | base | changed | change |
| --- | --- | --- | --- |
| 1 | 814.8 MiB | 877.0 MiB | +7.6% |
| 2 | 834.8 MiB | 885.5 MiB | +6.1% |
| 3 | 812.8 MiB | 871.0 MiB | +7.2% |
| 4 | 826.8 MiB | 873.0 MiB | +5.6% |
| 5 | 814.8 MiB | 846.5 MiB | +3.9% |
| 6 | 796.8 MiB | 856.5 MiB | +7.5% |
Mean +6.3%. The base readings span 796.8 to 834.8 MiB and the changed
readings span 846.5 to
885.5 MiB, so the two ranges do not overlap. This is a real effect, not
run-to-run noise.
### It is the accounting correction, not new allocation
Measured locally on a deterministic 3-file subset with `-n 1`, three builds
of the same data and
command. The middle build differs from the base **only** in the accounting
fix, because restoring
`with_capacity(128)` plus the 8 KiB buffer makes the constructors
byte-identical to base:
| build | Q0 pool peak |
| --- | --- |
| base | 48,421,820 |
| branch with the warm-up restored | 51,048,396 |
| branch head | 51,018,828 |
That decomposes the increase exactly:
- **+2,626,576 is the accounting correction.** `map_size` charged
`capacity * size_of::<Entry>()` and only on growth. `allocation_size()`
charges the real
hashbrown allocation, which is `4 * buckets + 5,384` more: the control
bytes plus the 7/8
load-factor slack. At full scale `SearchPhrase` holds about 6.02M distinct
values, so 2^23
buckets, which predicts about 32.0 MiB and matches the measured band.
- **−29,568 is the lazy constructor.** It makes Q0 slightly better, not
worse.
The transient explanation fails on two counts. The pool samples `size()`
only between batches, so
a transient inside `insert_unique` is invisible to it. And the base
performed the same doublings
above 224 entries anyway.
Query 0 is `COUNT(DISTINCT)` over three high-cardinality strings with **no**
`GROUP BY`: a handful
of maps that each grow to millions of entries. That is the opposite
population from the one this
PR targets, so the pre-allocation was never the dominant cost there and
removing it buys nothing.
### Why this is not a regression to fix
The extra 6.3% is memory the query always held and the pool could not see.
Reverting the change
restores an under-report of about 19% on this path. That under-report is the
bug this PR exists to
fix, and it is what the memory-limit improvement depends on: the minimum
viable budget for a
grouped `COUNT(DISTINCT Utf8)` goes from 36 MB to 2.0 MB, and for `Utf8View`
from 124 MB to 2.6 MB.
Restoring the warm-up was tested and makes Query 0 **worse**, so the
"capacity hint from
`GroupsAccumulatorAdapter`" follow-up listed earlier is withdrawn. It is
measurably
counterproductive.
### Query 2 across the same six runs
The changed side reported **11.7 MiB in every run**, against a base of 98.1
to 98.6 MiB. Six
consecutive readings of -88.1% with no variance on the changed side, and
nine measurements in
total including the earlier runs.
--
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]
