You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "andygrove (via GitHub)" <gi...@apache.org> on 2025/01/02 17:41:26 UTC
[PR] chore: Follow-on PR to fully enable onheap memory usage [datafusion-comet]
andygrove opened a new pull request, #1210:
URL: https://github.com/apache/datafusion-comet/pull/1210
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
This PR is based on https://github.com/apache/datafusion-comet/pull/1021 and fully enables the use of on-heap memory.
Status:
- [x] Remove limitations that disable Comet if offheap is disabled
- [x] Fix memory pool size calculation
- [ ] Update Tuning Guide to explain all memory configuration options
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## How are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Follow-on PR to fully enable onheap memory usage [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #1210:
URL: https://github.com/apache/datafusion-comet/pull/1210#discussion_r1902629149
##########
docs/source/user-guide/tuning.md:
##########
@@ -23,11 +23,52 @@ Comet provides some tuning options to help you get the best performance from you
## Memory Tuning
-Comet shares an off-heap memory pool between Spark and Comet. This requires setting `spark.memory.offHeap.enabled=true`.
-If this setting is not enabled, Comet will not accelerate queries and will fall back to Spark.
+### Unified Memory Management with Off-Heap Memory
+
+The recommended way to share memory between Spark and Comet is to set `spark.memory.offHeap.enabled=true`. This allows
+Comet to share an off-heap memory pool with Spark. The size of the pool is specified by `spark.memory.offHeap.size`.
Review Comment:
```suggestion
Comet to share an off-heap memory pool with Spark. The size of the pool is specified by `spark.memory.offHeap.size`. For more details about Spark off-heap memory mode, please refer to Spark documentation: https://spark.apache.org/docs/latest/configuration.html.
```
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Follow-on PR to fully enable onheap memory usage [datafusion-comet]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #1210:
URL: https://github.com/apache/datafusion-comet/pull/1210#discussion_r1902289845
##########
native/core/src/execution/jni_api.rs:
##########
@@ -281,14 +279,13 @@ fn parse_memory_pool_config(
memory_pool_type: String,
memory_limit: i64,
memory_limit_per_task: i64,
- memory_fraction: f64,
) -> CometResult<MemoryPoolConfig> {
let memory_pool_config = if use_unified_memory_manager {
MemoryPoolConfig::new(MemoryPoolType::Unified, 0)
} else {
// Use the memory pool from DF
- let pool_size = (memory_limit as f64 * memory_fraction) as usize;
- let pool_size_per_task = (memory_limit_per_task as f64 * memory_fraction) as usize;
Review Comment:
If memory_fraction is not used here, we can remove the configuration option `COMET_EXEC_MEMORY_FRACTION (spark.comet.exec.memoryFraction)` since it is not used in the code base.
My understanding is that `spark.comet.exec.memoryFraction` is still useful. As per the documentation, it makes DataFusion not using up all the reserved native memory, since some of the allocations are untracked, and this is also a compensation of inaccurate memory usage estimations.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Follow-on PR to fully enable onheap memory usage [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #1210:
URL: https://github.com/apache/datafusion-comet/pull/1210#discussion_r1902182027
##########
spark/src/main/scala/org/apache/comet/CometSparkSessionExtensions.scala:
##########
@@ -919,14 +919,6 @@ class CometSparkSessionExtensions
}
override def apply(plan: SparkPlan): SparkPlan = {
-
- // Comet required off-heap memory to be enabled
- if (!isOffHeapEnabled(conf) && !isTesting) {
Review Comment:
Looks like `isOffHeapEnabled` and `isTesting` are not used after PR. Should we delete those methods?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Follow-on PR to fully enable onheap memory usage [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #1210:
URL: https://github.com/apache/datafusion-comet/pull/1210#issuecomment-2569445901
@Kontinuation, could you review this, particularly the updated Tuning Guide, to make sure I understand it correctly?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Follow-on PR to fully enable onheap memory usage [datafusion-comet]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1210:
URL: https://github.com/apache/datafusion-comet/pull/1210#issuecomment-2569962633
## [Codecov](https://app.codecov.io/gh/apache/datafusion-comet/pull/1210?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `83.33333%` with `1 line` in your changes missing coverage. Please review.
> Project coverage is 34.31%. Comparing base [(`9320aed`)](https://app.codecov.io/gh/apache/datafusion-comet/commit/9320aedc8df2e8f7e5acb42ecdc44f33dff5d592?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`63b55a5`)](https://app.codecov.io/gh/apache/datafusion-comet/commit/63b55a5d2a6207321039a03c1235a9374518ccb4?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 3 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion-comet/pull/1210?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...org/apache/comet/CometSparkSessionExtensions.scala](https://app.codecov.io/gh/apache/datafusion-comet/pull/1210?src=pr&el=tree&filepath=spark%2Fsrc%2Fmain%2Fscala%2Forg%2Fapache%2Fcomet%2FCometSparkSessionExtensions.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3Bhcmsvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9jb21ldC9Db21ldFNwYXJrU2Vzc2lvbkV4dGVuc2lvbnMuc2NhbGE=) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion-comet/pull/1210?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #1210 +/- ##
============================================
- Coverage 34.77% 34.31% -0.47%
+ Complexity 963 950 -13
============================================
Files 115 116 +1
Lines 43669 43706 +37
Branches 9563 9562 -1
============================================
- Hits 15185 14996 -189
- Misses 25544 25744 +200
- Partials 2940 2966 +26
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/datafusion-comet/pull/1210?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Follow-on PR to fully enable onheap memory usage [datafusion-comet]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #1210:
URL: https://github.com/apache/datafusion-comet/pull/1210#discussion_r1902290275
##########
docs/source/user-guide/tuning.md:
##########
@@ -23,11 +23,52 @@ Comet provides some tuning options to help you get the best performance from you
## Memory Tuning
-Comet shares an off-heap memory pool between Spark and Comet. This requires setting `spark.memory.offHeap.enabled=true`.
-If this setting is not enabled, Comet will not accelerate queries and will fall back to Spark.
+### Unified Memory Management with Off-Heap Memory
+
+The recommended way to share memory between Spark and Comet is to set `spark.memory.offHeap.enabled=true`. This allows
+Comet to share an off-heap memory pool with Spark. The size of the pool is specified by `spark.memory.offHeap.size`.
+
+### Dedicated Comet Memory Pools
+
+If the `spark.memory.offHeap.enabled` setting is not enabled then Comet will use its own dedicated memory pools that
+are not shared with Spark. This requires additional configuration settings to be specified to set the size and type of
+memory pool to use.
+
+The size of the pool can be set explicitly with `spark.comet.memoryOverhead`. If this setting is not specified then
+the memory overhead will be calculated by multiplying the executor memory by `spark.comet.memory.overhead.factor`
+(defaults to `0.2`).
+
+The type of pool can be specified with `spark.comet.exec.memoryPool`. The default setting is `greedy_task_shared`.
+
+The valid pool types are:
+
+- `greedy`
+- `greedy_global`
+- `greedy_task_shared`
+- `fair_spill`
+- `fair_spill_global`
+- `fair_spill_task_shared`
+
+Pool types ending with `_global` use a single global memory pool between all tasks.
+
+Pool types ending with `_task_shared` share a single memory pool across all attempts for a single task.
+
+Other pool types create a dedicated pool per task using a fraction of the available pool size based on number of cores
+and cores per task.
Review Comment:
```suggestion
Other pool types create a dedicated pool per native query plan using a fraction of the available pool size based on number of cores
and cores per task.
```
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Follow-on PR to fully enable onheap memory usage [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #1210:
URL: https://github.com/apache/datafusion-comet/pull/1210#issuecomment-2573995471
Thanks for the reviews @viirya @kazuyukitanimura @Kontinuation
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Follow-on PR to fully enable onheap memory usage [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #1210:
URL: https://github.com/apache/datafusion-comet/pull/1210
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Follow-on PR to fully enable onheap memory usage [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #1210:
URL: https://github.com/apache/datafusion-comet/pull/1210#discussion_r1902636032
##########
docs/source/user-guide/tuning.md:
##########
@@ -23,11 +23,52 @@ Comet provides some tuning options to help you get the best performance from you
## Memory Tuning
-Comet shares an off-heap memory pool between Spark and Comet. This requires setting `spark.memory.offHeap.enabled=true`.
-If this setting is not enabled, Comet will not accelerate queries and will fall back to Spark.
+### Unified Memory Management with Off-Heap Memory
+
+The recommended way to share memory between Spark and Comet is to set `spark.memory.offHeap.enabled=true`. This allows
+Comet to share an off-heap memory pool with Spark. The size of the pool is specified by `spark.memory.offHeap.size`.
+
+### Dedicated Comet Memory Pools
+
+If the `spark.memory.offHeap.enabled` setting is not enabled then Comet will use its own dedicated memory pools that
+are not shared with Spark. This requires additional configuration settings to be specified to set the size and type of
+memory pool to use.
+
+The size of the pool can be set explicitly with `spark.comet.memoryOverhead`. If this setting is not specified then
+the memory overhead will be calculated by multiplying the executor memory by `spark.comet.memory.overhead.factor`
+(defaults to `0.2`).
+
+The type of pool can be specified with `spark.comet.exec.memoryPool`. The default setting is `greedy_task_shared`.
+
+The valid pool types are:
+
+- `greedy`
+- `greedy_global`
+- `greedy_task_shared`
+- `fair_spill`
+- `fair_spill_global`
+- `fair_spill_task_shared`
+
+Pool types ending with `_global` use a single global memory pool between all tasks.
Review Comment:
```suggestion
Pool types ending with `_global` use a single global memory pool between all tasks on same executor.
```
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Follow-on PR to fully enable onheap memory usage [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #1210:
URL: https://github.com/apache/datafusion-comet/pull/1210#discussion_r1902634368
##########
docs/source/user-guide/tuning.md:
##########
@@ -23,11 +23,52 @@ Comet provides some tuning options to help you get the best performance from you
## Memory Tuning
-Comet shares an off-heap memory pool between Spark and Comet. This requires setting `spark.memory.offHeap.enabled=true`.
-If this setting is not enabled, Comet will not accelerate queries and will fall back to Spark.
+### Unified Memory Management with Off-Heap Memory
+
+The recommended way to share memory between Spark and Comet is to set `spark.memory.offHeap.enabled=true`. This allows
+Comet to share an off-heap memory pool with Spark. The size of the pool is specified by `spark.memory.offHeap.size`.
+
+### Dedicated Comet Memory Pools
+
+If the `spark.memory.offHeap.enabled` setting is not enabled then Comet will use its own dedicated memory pools that
Review Comment:
```suggestion
Spark uses on-heap memory mode by default, i.e., the `spark.memory.offHeap.enabled` setting is not enabled. If Spark is under on-heap memory mode, Comet will use its own dedicated memory pools that
```
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Follow-on PR to fully enable onheap memory usage [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #1210:
URL: https://github.com/apache/datafusion-comet/pull/1210#discussion_r1901922136
##########
native/core/src/execution/jni_api.rs:
##########
@@ -281,14 +279,13 @@ fn parse_memory_pool_config(
memory_pool_type: String,
memory_limit: i64,
memory_limit_per_task: i64,
- memory_fraction: f64,
) -> CometResult<MemoryPoolConfig> {
let memory_pool_config = if use_unified_memory_manager {
MemoryPoolConfig::new(MemoryPoolType::Unified, 0)
} else {
// Use the memory pool from DF
- let pool_size = (memory_limit as f64 * memory_fraction) as usize;
- let pool_size_per_task = (memory_limit_per_task as f64 * memory_fraction) as usize;
Review Comment:
The `memory_fraction` is already applied in JVM side.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Follow-on PR to fully enable onheap memory usage [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #1210:
URL: https://github.com/apache/datafusion-comet/pull/1210#discussion_r1902234781
##########
spark/src/main/scala/org/apache/comet/CometSparkSessionExtensions.scala:
##########
@@ -919,14 +919,6 @@ class CometSparkSessionExtensions
}
override def apply(plan: SparkPlan): SparkPlan = {
-
- // Comet required off-heap memory to be enabled
- if (!isOffHeapEnabled(conf) && !isTesting) {
Review Comment:
Thanks. I have removed them now.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org