adriangb opened a new pull request, #24023: URL: https://github.com/apache/datafusion/pull/24023
## Which issue does this PR close? Follow-up to #23985, which added `pool_peak_bytes`. No open issue. ## Rationale for this change `pool_peak_bytes` is documented as the peak reservation for a query, but it is currently a **max across all of that query's iterations**, and every iteration's entry carries the same contaminated value. The recorder is reset in `start_new_case` and read in `write_iter`. Those look adjacent, but benchmarks run *every* iteration of a query before reporting *any* of them — `benchmark_query` returns a `Vec<QueryResult>` that is then looped over. So by the time the first `write_iter` runs, the recorder already holds the high-water mark of the whole set. The existing comment states the consequence: ```rust // The peak is not reset between iterations, so this ends up holding the // largest reservation seen across all of them. ``` A peak reservation is a max over the operators holding memory at the same instant, so it moves with how the runtime happened to interleave partitions on that pass. Reducing across iterations with `max` means one unlucky pass sets the number for the query, and the field jumps between runs of identical code. Measured on a same-commit A/B (both sides the same SHA, so every difference is noise), TPC-H SF1, 5 iterations: | | | | --- | --- | | 14 of 22 queries | agree to within 1%, several at exactly `+0.0%` | | Q11 | **+61.0%** | | Q1 | **−25.0%** (40.0 → 30.0 MiB) | | Q16 | **+22.1%** (45.0 → 55.0 MiB) | | Q21 | **+10.0%** | The distribution is bimodal rather than continuous, and the outliers land on round multiples — Q1 and Q16 both move by exactly 10 MiB. That is a discrete quantity (partitions holding a reservation concurrently) changing by whole units, amplified by taking the largest of 5 draws. It also biases the reported value upward: max-of-5 overstates what a single run reserves. ## What changes are included in this PR? - `take_iteration_peak(&Arc<dyn MemoryPool>)` reads the peak and rebases the recorder, so each iteration is measured on its own. - `QueryResult` carries `pool_peak_bytes`; each benchmark's iteration loop populates it where the iteration actually ends. - `write_iter` takes that reading and keeps the **smallest** across a query's iterations. `min` is the judgement call here. It answers "how much does this query need when nothing else is in the way", which is what a commit-to-commit comparison wants, and it matches how `compare.py` already reduces timings when not passed `--detailed`. Median is a one-line change if maintainers prefer it — happy to switch. ## Are these changes tested? Yes — unit tests in `util/run.rs`, including that the quietest iteration sets the value, that an iteration no longer inherits the previous one's mark, and that bytes held across a reading stay in it (`reset_peak` rebases on what is reserved, not on zero, so data loaded up front still counts for every iteration that runs with it). ## Are there any user-facing changes? The results JSON is **unchanged** — `pool_peak_bytes` remains one field per query, computed differently. `compare.py` needs no change. Reported values will generally be lower than before and should be reproducible run to run. Anything tracking these numbers over time will see a step at this commit. -- 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]
