feat(executor): use default memory pool in executor if no config provided#1637
Draft
andygrove wants to merge 1 commit intoapache:mainfrom
Draft
feat(executor): use default memory pool in executor if no config provided#1637andygrove wants to merge 1 commit intoapache:mainfrom
andygrove wants to merge 1 commit intoapache:mainfrom
Conversation
The executor's `--memory-pool-size` was optional with `None` meaning "no pool installed, use DataFusion's unbounded default". On a workload that holds large amounts of data per task in memory (notably the sort shuffle writer), an unbounded pool means operators never spill and the executor's RSS grows until the OS starts swapping. TPC-H Q3 against SF100 with `--partitions 2` peaked at ~24 GB RSS and ran in ~49s instead of ~13s. Default the pool to `concurrent_tasks * 1 GiB` when no flag is provided. Each per-task FairSpillPool gets 1 GiB of headroom — enough for typical batch materialisation while keeping the total bounded. Operators that respect the runtime memory pool (DataFusion aggregates, sorts, joins with spill, etc.) gain backpressure for free; users who want a different budget pass `--memory-pool-size` explicitly. Logging now distinguishes the explicit and default paths so it's obvious which is in effect. Verified with TPC-H Q3 SF100 `--partitions 2`, no `--memory-pool-size` flag: sort shuffle drops from ~49s to ~17s (~10 GB RSS); hash shuffle unchanged at ~13s.
andygrove
changed the title
andygrove
changed the title
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.
Which issue does this PR close?
Closes #.
Rationale for this change
--memory-pool-sizeis optional today: when not provided, the executor installs no memory pool and DataFusion falls back to its unbounded default. Operators that respect the pool (aggregates, sorts, joins, sort shuffle writer) therefore never spill and just keep growing — until the OS starts swapping. On TPC-H Q3 against SF100 with--partitions 2, this caused executor RSS to reach ~24 GB and the query to take ~49s (vs ~13s for hash shuffle).The fix in #1636 patches the sort shuffle writer specifically, but every other memory-pool-aware operator still benefits from a sensible bounded default. New users on a laptop or in a small VM should not have to read the docs to learn that a flag is effectively required to keep the executor from blowing past physical RAM.
What changes are included in this PR?
DEFAULT_MEMORY_POOL_BYTES_PER_TASK = 1 GiB.--memory-pool-sizeis not provided, default the total pool toconcurrent_tasks * DEFAULT_MEMORY_POOL_BYTES_PER_TASK. With the default 8 task slots that's an 8 GiB total budget, split into 8 × 1 GiB per-taskFairSpillPools.Memory pool (explicit)vsMemory pool (default)so it's obvious which path is in effect.ExecutorProcessConfig::memory_pool_sizedoc comment updated to describe the new default.Are there any user-facing changes?
The executor now always installs a bounded memory pool. Users who relied on the old unbounded default and have queries that run within physical RAM will see the same behaviour; users whose queries previously over-committed memory will start seeing operators spill instead. The new total budget is
concurrent_tasks * 1 GiB; pass--memory-pool-sizeto override.Verification
TPC-H Q3 against
/opt/tpch/sf100, executor started with--concurrent-tasks 8and no--memory-pool-size:--partitionsStartup log confirms the new default is taking effect:
cargo test --release -p ballista-executor --libpasses (22/22).