DO NOT MERGE: inject a memory spike to test benchmark memory reporting - #15
Open
adriangb wants to merge 2 commits into
Open
DO NOT MERGE: inject a memory spike to test benchmark memory reporting#15adriangb wants to merge 2 commits into
adriangb wants to merge 2 commits into
Conversation
Validates end to end that a large allocation inside an operator is actually reported by the benchmark harness, now that apache#23985 records per-query peak MemoryPool reservation and adriangb/datafusion-benchmarking#19 surfaces it in the PR comment. Two separately sized knobs, off unless set, injected in FilterExec::execute and held for the stream's lifetime: * DF_INJECT_POOL_BYTES grows a MemoryReservation without allocating — should move pool_peak_bytes and not RSS. * DF_INJECT_ALLOC_BYTES allocates and writes a buffer without telling the pool — should move RSS and not pool_peak_bytes. A process-wide flag keeps one injection live at a time, so the spike is a fixed size rather than a multiple of the partition count. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
run benchmark tpch |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing test/memory-capture-probe (f0f42e9) to 1ae6b87 (merge-base) diff using: tpch File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Memory Pool PeaksPeak Base:
Pool accounting vs. process RSS Max pool peak is the largest reservation any single query in the run reached; peak RSS covers the whole invocation, including data loading and allocator retention, and the two high-water marks need not coincide in time. The gap is therefore an upper bound on what the pool did not account for, not a measurement of it.
Resource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
Removed MemoryInjection references from FilterExec.
Owner
Author
|
run benchmark tpch env:
DATAFUSION_RUNTIME_MEMORY_LIMIT: 8G
changed:
env:
DF_INJECT_POOL_BYTES: 1G
DF_INJECT_ALLOC_BYTES: 1G |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing test/memory-capture-probe (9484811) to 1ae6b87 (merge-base) diff using: tpch File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Memory Pool PeaksPeak Base:
Pool accounting vs. process RSS Max pool peak is the largest reservation any single query in the run reached; peak RSS covers the whole invocation, including data loading and allocator retention, and the two high-water marks need not coincide in time. The gap is therefore an upper bound on what the pool did not account for, not a measurement of it.
Resource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
Owner
Author
|
run benchmark tpch [determinism experiment 1/3] CONTROLA/A run: both sides are
|
Owner
Author
|
run benchmark tpch [determinism experiment 2/3] purge + THP disabledSame A/A setup as 1/3, plus the two cheap mimalloc knobs:PURGE_DELAY=-1 -> never madvise(DONTNEED) memory back to the OS(default is 10ms, x10 arena_purge_mult = ~100ms after idle),which removes the re-fault storms between queries/iterations.ALLOW_THP=0 -> mimalloc v3 defaults this to 1 on Linux; khugepagedcollapse timing is independent of the workload.env: |
Owner
Author
|
run benchmark tpch [determinism experiment 3/3] purge + THP + pre-committed arenaEverything from 2/3, plus pre-reserve and eagerly commit an 8 GiB arena atstartup so page faults are paid before the first query instead of beingsprinkled through the measured region. Expect RSS to sit at ~8 GiB for bothsides; that is the intended trade (memory for variance), not a regression.env: |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing main (1ae6b87) to main diff Run configurationrun benchmark tpch
baseline:
ref: "main"
changed:
ref: "main"Results will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing main (1ae6b87) to main diff Run configurationrun benchmark tpch
env:
MIMALLOC_ALLOW_THP: "0"
MIMALLOC_PURGE_DELAY: "-1"
MIMALLOC_VERBOSE: "1"
baseline:
ref: "main"
changed:
ref: "main"Results will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing main (1ae6b87) to main diff Run configurationrun benchmark tpch
env:
MIMALLOC_ALLOW_THP: "0"
MIMALLOC_ARENA_EAGER_COMMIT: "1"
MIMALLOC_ARENA_RESERVE: "8GIB"
MIMALLOC_PURGE_DELAY: "-1"
MIMALLOC_RESERVE_OS_MEMORY: "8GIB"
MIMALLOC_VERBOSE: "1"
baseline:
ref: "main"
changed:
ref: "main"Results will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing main (1ae6b87) to main diff Run configurationrun benchmark tpch
baseline:
ref: "main"
changed:
ref: "main"CPU Details (lscpu)Details
Resource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing main (1ae6b87) to main diff Run configurationrun benchmark tpch
env:
MIMALLOC_ALLOW_THP: "0"
MIMALLOC_PURGE_DELAY: "-1"
MIMALLOC_VERBOSE: "1"
baseline:
ref: "main"
changed:
ref: "main"CPU Details (lscpu)Details
Resource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing main (1ae6b87) to main diff Run configurationrun benchmark tpch
env:
MIMALLOC_ALLOW_THP: "0"
MIMALLOC_ARENA_EAGER_COMMIT: "1"
MIMALLOC_ARENA_RESERVE: "8GIB"
MIMALLOC_PURGE_DELAY: "-1"
MIMALLOC_RESERVE_OS_MEMORY: "8GIB"
MIMALLOC_VERBOSE: "1"
baseline:
ref: "main"
changed:
ref: "main"CPU Details (lscpu)Details
Resource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
adriangb
added a commit
to adriangb/datafusion-benchmarking
that referenced
this pull request
Jul 31, 2026
`bench.sh compare_detail` reduces each query's iterations to the mean
before deciding faster/slower/no-change. Both sides run sequentially in
one pod on a spot node, so a slow iteration is usually the node
interfering rather than the query — the mean folds that in, the min
discards it.
Measured on an A/A run (identical SHAs on both sides, so every reported
difference is noise):
per-query err worst aggregate
mean 1.29% 4.31% -0.26%
min 0.55% 3.52% +0.00%
adriangb/datafusion#15 (issuecomment-5139405485).
The `compare` subcommand already reduces on min, so the verdict table is
now that one. `compare_detail` still follows it: the min/mean±stddev/max
spread is what makes an unstable query legible as unstable rather than
as a regression, and dropping it would cost more than it saves.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DO NOT MERGE. A throwaway branch in this fork whose only purpose is to check that the memory reporting we just built actually catches a large allocation in an operator.
apache/datafusion#23985 records per-query peak
MemoryPoolreservation aspool_peak_bytes; adriangb/datafusion-benchmarking#19 surfaces it in the benchmark comment next to peak RSS. Neither has ever been shown a spike it was supposed to catch. This branch supplies one.Two knobs, because there are two channels
They measure different things, and a test that moved both at once would not tell you which one did the work:
DF_INJECT_POOL_BYTESMemoryReservationagainst the task's pool, allocating nothingpool_peak_bytesonlyDF_INJECT_ALLOC_BYTESBoth unset by default, so the build behaves normally until asked for a spike.
The injection goes in
FilterExec::executeand is held for the stream's lifetime.FilterExec::executeruns once per partition and a plan can hold several filters, so a process-wide flag keeps at most one injection live: the spike is a fixed size rather than a multiple of the partition count, which on a 12-partition plan would turn a 1 GiB knob into 12 GiB and exhaust the pool.The reservation uses
grow, nottry_grow— the point is to record a peak, not to test the limit, and a failure there would fail the query rather than report anything.Local results
dfbench tpch --query 1 -i 1against SF1 parquet, release build, each knob at1G:pool_peak_bytes--memory-limit 8GDF_INJECT_POOL_BYTES=1GDF_INJECT_ALLOC_BYTES=1G--memory-limitEach channel moves under exactly its own knob and is untouched by the other, which is the result the design predicts.
The memory limit is required, for the pool number only
The last row is the answer to "do we need a memory limit?".
CommonOpt::runtime_env_builderinstalls aMemoryPool— and thereforePeakRecordingPool— only when a limit is set, so without one there is no pool to record and the query keys come back as['query', 'iterations', 'start_time', 'success']. The benchmark comment renders that asn/a, correctly.RSS needs no limit: the runner's process-subtree sampling caught the untracked gigabyte in that run regardless.
So a benchmark run exercising the pool path has to pass
DATAFUSION_RUNTIME_MEMORY_LIMIT, set well above the spike — the injected reservation is real as far as the pool is concerned and will push other consumers into spilling or failing if the headroom is not there. 8G against a 1G spike on TPC-H SF1 leaves plenty.What this does not test
The baseline side of the benchmark comparison. Baseline is the merge-base, which does not carry this file, so the injection appears only on the changed side — which is what makes the delta legible, but also means the comment's per-query table is being asked to display a difference, not to survive both sides having one.
Verification
5 unit tests on the injection itself (parsing, off-by-default, each knob in isolation, the one-at-a-time flag).
cargo fmt,cargo clippy -p datafusion-physical-plan --all-targets --all-features -- -D warningsclean.🤖 Generated with Claude Code