adriangb opened a new pull request, #23985: URL: https://github.com/apache/datafusion/pull/23985
## Which issue does this PR close? Part of #22758. Doesn't close an issue. ## Rationale for this change DataFusion's `MemoryPool` deliberately accounts for only the "large" allocations that scale with input size; intermediate batches flowing between operators are assumed to be small and left untracked. The `MemoryPool` docs therefore advise reserving "some overhead (e.g. 10%)" on top of the configured limit. Nothing measures what that overhead actually is. `MemoryPool::reserved()` is a live value that has usually fallen back to zero by the time a query finishes, so the peak is never observed, and benchmarks report peak RSS with nothing to compare it against. This adds the missing number so the two can be compared. It is measurement only — nothing enforces a relationship between the pool's accounting and actual allocation, and no threshold or CI check is added. This is complementary to the accounting work discussed in #22898. That issue proposes changing *what* gets accounted (Arrow `claim()` / builder `with_pool()`); because `ArrowMemoryPool` funnels Arrow-level reservations back into DataFusion's `MemoryPool`, `reserved()` is where both models converge. This PR just makes the peak of that value observable per query, so the effect of any such change — or of a regression — is visible as a number rather than inferred. ## What changes are included in this PR? - `PeakRecordingPool`, a delegating `MemoryPool` wrapper recording the high-water mark of `reserved()`. Every method delegates; wrapping does not change how memory is granted, limited, or reported. - `CommonOpt::runtime_env_builder` installs it around the pool it already builds, so every benchmark run with a memory limit reports the peak without further changes. - `BenchQuery` gains `pool_peak_bytes`, reset per case by `start_new_case`, so each query gets its own reading rather than inheriting the high-water mark of the queries before it. - `print_memory_stats` prints the run-wide peak after the existing mimalloc line, leaving `mem_profile`'s output parser untouched. Everything is confined to the `benchmarks` crate. No trait changes. ## Are these changes tested? Yes — 4 unit tests plus a doctest, and verified end-to-end with `dfbench nlj --query 1 -i 2 --memory-limit 512M`, which emits `"pool_peak_bytes": 262528` per query. The wrapper is installed only when a memory limit is configured, since without one there is no pool to wrap. `pool_peak_bytes` is omitted from the results JSON in that case, leaving output byte-identical to before for runs without a limit — confirmed `compare.py` parses both old and new files. ## Are there any user-facing changes? No. Confined to the benchmarks crate; adds one optional field to the benchmark results JSON. -- 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]
