sunchao commented on code in PR #6066:
URL: https://github.com/apache/datafusion-comet/pull/6066#discussion_r4064406970
##########
spark/src/main/scala/org/apache/comet/CometExecIterator.scala:
##########
@@ -392,37 +391,26 @@ object CometExecIterator extends Logging {
}
def getMemoryConfig(conf: SparkConf): MemoryConfig = {
- val numCores = numDriverOrExecutorCores(conf)
- val coresPerTask = conf.get("spark.task.cpus", "1").toInt
// there are different paths for on-heap vs off-heap mode
val offHeapMode = CometSparkSessionExtensions.isOffHeapEnabled(conf)
if (offHeapMode) {
// in off-heap mode, Comet uses unified memory management to share
off-heap memory with Spark
val offHeapSize =
ByteUnit.MiB.toBytes(conf.getSizeAsMb("spark.memory.offHeap.size"))
val memoryFraction = CometConf.COMET_OFFHEAP_MEMORY_POOL_FRACTION.get()
val memoryLimit = (offHeapSize * memoryFraction).toLong
- val memoryLimitPerTask = (memoryLimit.toDouble * coresPerTask /
numCores).toLong
val memoryPoolType = COMET_OFFHEAP_MEMORY_POOL_TYPE.get()
logDebug(
s"memoryPoolType=$memoryPoolType, " +
s"offHeapSize=${toMB(offHeapSize)}, " +
s"memoryFraction=$memoryFraction, " +
- s"memoryLimit=${toMB(memoryLimit)}, " +
- s"memoryLimitPerTask=${toMB(memoryLimitPerTask)}")
- MemoryConfig(offHeapMode, memoryPoolType = memoryPoolType, memoryLimit,
memoryLimitPerTask)
+ s"memoryLimit=${toMB(memoryLimit)}")
+ MemoryConfig(offHeapMode, memoryPoolType, memoryLimit)
} else {
- // we'll use the built-in memory pool from DF, and initializes with
`memory_limit`
- // and `memory_fraction` below.
- val memoryLimit =
CometSparkSessionExtensions.getCometMemoryOverhead(conf)
- // example 16GB maxMemory * 16 cores with 4 cores per task results
- // in memory_limit_per_task = 16 GB * 4 / 16 = 16 GB / 4 = 4GB
- val memoryLimitPerTask = (memoryLimit.toDouble * coresPerTask /
numCores).toLong
- val memoryPoolType = COMET_ONHEAP_MEMORY_POOL_TYPE.get()
- logDebug(
- s"memoryPoolType=$memoryPoolType, " +
- s"memoryLimit=${toMB(memoryLimit)}, " +
- s"memoryLimitPerTask=${toMB(memoryLimitPerTask)}")
- MemoryConfig(offHeapMode, memoryPoolType = memoryPoolType, memoryLimit,
memoryLimitPerTask)
+ // On-heap mode exists only so that the Spark SQL tests can run against
Comet without
+ // changing Spark's memory configuration, and native memory cannot be
charged to Spark's
+ // on-heap pool, so nothing is accounted. See the memory management
contributor guide.
+ logDebug("on-heap mode: native memory is unbounded and unaccounted")
+ MemoryConfig(offHeapMode, memoryPoolType = "unbounded", memoryLimit = 0)
Review Comment:
### Correctness
**[P2] Keep a compatibility path for the released on-heap memory controls**
Could we preserve a deprecated path for the existing settings before making
this unconditional in 1.1? For example, an on-heap test invocation with
`spark.comet.exec.onHeap.enabled=true`, `spark.comet.memoryOverhead=128m` and
`spark.comet.exec.onHeap.memoryPool=greedy` previously selected a finite native
reservation pool. It now silently selects an unbounded pool, regardless of
those explicit settings. The shuffle budget settings disappear too.
`memoryOverhead`, `onHeap.memoryPool` and `shuffle.jvm.memoryFactor` all
[shipped in
1.0.0](https://github.com/apache/datafusion-comet/blob/1.0.0/spark/src/main/scala/org/apache/comet/CometConf.scala#L688).
The [versioning
policy](https://github.com/apache/datafusion-comet/blob/f1c4e61045f77856e50370c77a2b30e26f7148de/docs/source/about/versioning_policy.md#L140)
covers `spark.comet.*` configuration semantics, including these
testing-category keys, and requires deprecation before removal in a major
release. Keeping the old behavior available through the documented
compatibility mechanism, with an upgrade-guide entry, would let this
simplification land without silently removing an existing test harness's
configured bound.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
