feat: Add unbounded memory pool #1386
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1386 +/- ##
=============================================
- Coverage 56.12% 39.06% -17.06%
- Complexity 976 2071 +1095
=============================================
Files 119 263 +144
Lines 11743 60746 +49003
Branches 2251 12909 +10658
=============================================
+ Hits 6591 23733 +17142
- Misses 4012 32530 +28518
- Partials 1140 4483 +3343 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| "greedy_global" => MemoryPoolConfig::new(MemoryPoolType::GreedyGlobal, pool_size), | ||
| "fair_spill" => MemoryPoolConfig::new(MemoryPoolType::FairSpill, pool_size_per_task), | ||
| "greedy" => MemoryPoolConfig::new(MemoryPoolType::Greedy, pool_size_per_task), | ||
| "unbounded" => MemoryPoolConfig::new(MemoryPoolType::Unbounded, 0), |
There was a problem hiding this comment.
TBH, I don't know if it is ever a good idea to allow an unbounded memory pool. It doesn't hurt to have the option, but under what conditions is this choice useful?
There was a problem hiding this comment.
I found the unbounded memory pool is useful for the cases we do not want to allow any spilling and rather choose to fail the job. Spilling slows down the job a lot and the unbounded memory pool is one way to measure the best case without adjusting how much memory we provide for native exec
There was a problem hiding this comment.
I guess we can add this advice in the documentation. Speeding up the job sounds like a win until the jobs start OOMing. :)
There was a problem hiding this comment.
I see the value in having the unbounded tool for development testing purposes but not for end user use.
I would like to use it to help with #1315
| "Available memory pool types are 'greedy', 'fair_spill', 'greedy_task_shared', " + | ||
| "'fair_spill_task_shared', 'greedy_global' and 'fair_spill_global'. For off-heap " + | ||
| "types are 'unified' and `fair_unified`.") | ||
| "'fair_spill_task_shared', 'greedy_global', 'fair_spill_global', and `unbounded`. " + |
There was a problem hiding this comment.
At this point there are so many options for memory pool that it is bound to confuse users. Would it be a good idea to have a bit of documentation that helps users decide what kind of memory pool to use?
There was a problem hiding this comment.
Good idea. I also do not know some of the differences like 'fair_spill_task_shared' vs 'greedy_global'
I filed an issue #1388 and will on separately
There was a problem hiding this comment.
These are explained in the Tuning Guide:
https://datafusion.apache.org/comet/user-guide/tuning.html#dedicated-comet-memory-pools
There was a problem hiding this comment.
Thanks, I can add the doc to the place
andygrove
left a comment
andygrove
left a comment
There was a problem hiding this comment.
LGTM for development/testing purposes. Thanks @kazuyukitanimura.
|
Merged, thanks @parthchandra @andygrove |
## Which issue does this PR close? ## Rationale for this change DataFusion has an unbounded memory pool. I found it useful for experimental purpose. ## What changes are included in this PR? Added an option for unbounded memory pool. ## How are these changes tested? existing tests
## Which issue does this PR close? Closes apache#1388 ## Rationale for this change Following up on apache#1369 and apache#1386 ## What changes are included in this PR? Updated the doc ## How are these changes tested?
4 participants
Which issue does this PR close?
Rationale for this change
DataFusion has an unbounded memory pool. I found it useful for experimental purpose.
What changes are included in this PR?
Added an option for unbounded memory pool.
How are these changes tested?
existing tests