LuciferYang commented on code in PR #13094:
URL: https://github.com/apache/gluten/pull/13094#discussion_r4266249436
##########
gluten-core/src/main/scala/org/apache/spark/memory/SparkMemoryUtil.scala:
##########
@@ -51,8 +53,21 @@ object SparkMemoryUtil {
// We assume storage memory can be fully transferred to execution memory so
far
def getCurrentAvailableOffHeapMemory: Long = {
val mm = SparkEnv.get.memoryManager
- val smp = smpField.get(mm).asInstanceOf[StorageMemoryPool]
- val emp = empField.get(mm).asInstanceOf[ExecutionMemoryPool]
+ // With dynamic off-heap sizing enabled, Gluten's global reservations are
+ // charged to the ON-heap pools (see GlobalOffHeapMemoryTarget), so the
+ // available figure must be read from the same pools. Read the flag the
same
+ // way GlobalOffHeapMemoryTarget.mode does, so the two cannot pick
different
+ // pool families.
+ val (smp, emp) =
+ if (GlutenCoreConfig.get.dynamicOffHeapSizingEnabled) {
Review Comment:
`mode` hasn't read `SparkEnv.get.conf` since #12764, which is already in
this PR's base. On main it calls
`GlutenCoreConfig.get.dynamicOffHeapSizingEnabled`, same as here, so the
comment is accurate. The `SparkEnv` read was from #11448.
On an executor task thread there's no active session, so
`GlutenCoreConfig.get` falls through to `SQLConf.get`, which returns a
`ReadOnlySQLConf` over the task's local properties.
`SQLExecution.withSQLConfPropagated` copies every `spark.*` entry of the
session conf into those properties, and the session conf starts from a full
clone of the `SparkConf`, static keys included (`BaseSessionStateBuilder.conf`
→ `SQLConf.mergeSparkConf`). So the flag is there on the shuffle-writer thread.
A task submitted outside `withSQLConfPropagated` would get the default here,
but `mode` would get the same default on that thread.
Moving only this method to `SparkEnv.get.conf` would bring back the split
from your first comment. If we want a read that doesn't depend on the calling
thread, `mode` and this method should move together, and I'd prefer to do that
in a separate PR. What do you think?
--
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]
