feat(executor): log per-task memory pool peak usage - #2163
Closed
andygrove wants to merge 3 commits into
Closed
Conversation
Wrap each per-task FairSpillPool built by `memory_pool_policy` in a new `TrackedMemoryPool`. The wrapper records peak `reserved()` bytes and emits two `tracing` info events (target `ballista_executor::memory_pool`): - On construction: `pool_id`, `limit_bytes`. - On `Drop` (i.e. when the task's `RuntimeEnv` is released): `pool_id`, `peak_bytes`, `limit_bytes`. Correlate events by `pool_id`. This gives per-task peak-memory visibility without any API changes: with the default `ballista=info` log filter, operators immediately see how close each task got to its per-task pool ceiling, which is otherwise opaque. Motivation: benchmarking SF1000 TPC-H showed a large gap between standalone and in-suite Q8 wall time, and one hypothesis is memory-pool state drift across queries — peak-per-task logs let that hypothesis be tested from executor logs alone. The wrapper delegates all `MemoryPool` methods to the inner pool; `grow` and successful `try_grow` additionally push the peak forward via a CAS-loop on an `AtomicUsize`. Failed `try_grow` does not update the peak. Includes unit tests for the peak tracking (grow / shrink / failed try_grow) and `memory_limit` delegation. Signed-off-by: Andy Grove <agrove@apache.org>
andygrove
marked this pull request as draft
July 23, 2026 23:06
`tracing` is only enabled behind the `build-binary` feature in ballista-executor, so the initial commit broke library builds that compile with default features off (e.g. building just the executor lib). The rest of the crate uses `log`, so match that. Switches the two structured `tracing::info!` calls to `log::info!` with the fields formatted directly into the message string. Same log target (`ballista_executor::memory_pool`); log parsers keying off `pool_id=` / `peak_bytes=` / `limit_bytes=` continue to work unchanged. Signed-off-by: Andy Grove <agrove@apache.org>
Under `MemoryPool`'s Arc-based lifetime the pool can only be dropped once every `MemoryReservation` has been released, and each release calls `shrink()` -- so `inner.reserved()` should always be 0 by the time `TrackedMemoryPool::drop` runs. If it isn't, some path bypassed `shrink()` (e.g. a `mem::forget`'d reservation, or a `MemoryReservation` whose `size` was adjusted without corresponding pool bookkeeping), and that is worth surfacing. Log the residual in every drop event, and escalate to WARN when it is non-zero. In a healthy run every `residual_bytes` value is 0; any non-zero occurrence is a genuine accounting anomaly to investigate. Adds two tests: one confirming reservations dropped normally leave zero reserved bytes, and one documenting that `mem::forget`'ing a reservation does leak, matching the anomaly the log surfaces. Signed-off-by: Andy Grove <agrove@apache.org>
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
1 participant
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.
Summary
Add a small
TrackedMemoryPoolwrapper (ballista/executor/src/memory_pool.rs) around the per-taskFairSpillPoolthatmemory_pool_policy()builds. It records peakreserved()bytes and emitstracing::info!events on construction and onDrop, so a task's peak memory pool usage lands in executor logs when the task'sRuntimeEnvdrops.Log format (target
ballista_executor::memory_pool):Correlate the two events by
pool_id(monotonic within the executor process).Why
Ballista builds one
FairSpillPoolper task, but there is currently no way to see how close a task got to its per-task ceiling without adding operator-level tracing or a custom pool. When benchmarking SF1000 TPC-H, individual query wall times varied significantly between standalone and in-suite runs, and memory-pool state drift is one hypothesis — but it's hard to test without per-task peak numbers. This makes the numbers observable in-place from executor logs alone.What it does not do
grow/shrink/try_grow/reserved/memory_limitall delegate; onlygrowand successfultry_growadditionally push the peak forward via a CAS on anAtomicUsize.Test plan
cargo test -p ballista-executor(42 tests pass, including 3 new tests for peak tracking + limit delegation).cargo clippy -p ballista-executor -- -D warningsclean.cargo fmt.memory pool droppedlines with plausiblepeak_bytes.