Measure pool_peak_bytes per iteration and report the smallest

[adriangb]

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:

// 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.

Measured effect

Two A/A runs on the benchmark harness at TPC-H SF1, 5 iterations, DATAFUSION_RUNTIME_MEMORY_LIMIT=4G. Each pins both sides to one commit, so every reported difference is noise: before on a589b4bb, after on this branch.

	median |err|	mean |err|	worst	queries >=20% off
before (max-of-5)	2.55%	9.12%	39.9%	5 of 22
after (min-of-5)	2.30%	6.28%	50.0%	2 of 22

This reduces the noise; it does not remove it. Four of the worst queries collapse — Q13 39.9% -> 0.0%, Q22 23.3% -> 2.8%, Q11 22.7% -> 3.3%, Q21 18.5% -> 6.2% — but Q1 got worse (27.5% -> 50.0%) and the worst case across the set rose.

Q1 is instructive: 20.0 MiB against 30.0 MiB, a difference of exactly one 10 MiB unit. The underlying quantity is discrete and genuinely bimodal — the query reserves either two units or three depending on how partitions interleave — so with only 5 draws, min still differs by a whole unit whenever one side never draws the low mode. No reduction over 5 samples fixes that; it needs either more iterations or a deterministic partition count.

What the change does deliver unambiguously is that each iteration's reading is its own, and the de-inflation that follows: summed across the 22 queries the reported peak drops from 1381 MiB to 1174 MiB (-15%). That part is mechanical, not sampling — max-of-5 was overstating what a single run reserves.

Caveat: one run per configuration, so the per-query deltas above carry their own noise. The aggregate figures and the counts are the trustworthy part; individual movements like Q1's are not resolvable from n=1.

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 are generally lower than before (-15% in aggregate, see above) and noticeably more stable, though not fully reproducible. Anything tracking these numbers over time will see a step at this commit.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 31.05263% with 131 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.83%. Comparing base (a589b4b) to head (59bb7b2).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
benchmarks/src/hj.rs	0.00%	13 Missing ⚠️
benchmarks/src/nlj.rs	0.00%	13 Missing ⚠️
benchmarks/src/smj.rs	0.00%	13 Missing ⚠️
benchmarks/src/bin/external_aggr.rs	0.00%	11 Missing ⚠️
benchmarks/src/clickbench.rs	0.00%	11 Missing ⚠️
benchmarks/src/dict.rs	0.00%	11 Missing ⚠️
benchmarks/src/imdb/run.rs	0.00%	11 Missing ⚠️
benchmarks/src/sort_pushdown.rs	0.00%	11 Missing ⚠️
benchmarks/src/sort_tpch.rs	0.00%	11 Missing ⚠️
benchmarks/src/tpcds/run.rs	0.00%	11 Missing ⚠️
... and 4 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #24023      +/-   ##
==========================================
- Coverage   80.86%   80.83%   -0.03%     
==========================================
  Files        1099     1099              
  Lines      374327   374480     +153     
  Branches   374327   374480     +153     
==========================================
+ Hits       302685   302729      +44     
- Misses      53596    53707     +111     
+ Partials    18046    18044       -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.