LuciferYang opened a new pull request, #12677: URL: https://github.com/apache/gluten/pull/12677
### What changes were proposed in this pull request? Three Spark memory configurations are declared with a unit but read through an accessor that assumes a different one, so the off-heap budgets Gluten derives from them are wrong. Each commit fixes one site, and the last one cleans up two problems the fixes exposed. `spark.executor.memory` is declared `bytesConf(ByteUnit.MiB)`, so a value without a suffix means MiB. The dynamic off-heap sizing branch read it with `SparkConf#getSizeAsBytes`, which reads a suffix-less value as bytes. With `spark.executor.memory=8192` it saw 8192 bytes instead of 8 GiB, and `(onHeapSize - 300MiB) * 0.6` made the off-heap budget negative. `SparkResourceUtil.getExecutorMemorySize` now reads the typed `EXECUTOR_MEMORY` entry and converts from MiB, which is also how `getMemoryOverheadSize` in the same object already read it. The typed entry carries Spark's 1g default, so the hand-rolled `conf.contains` check and the hardcoded 1GB fallback are gone. `spark.executor.minMemoryOverhead` is a size string, MiB unless suffixed. `getMemoryOverheadSize` read it with `conf.getLong`, which is `String.toLong` and fails on anything carrying a unit. `VeloxListenerApi#onDriverStart` calls the method unconditionally and `isMemoryOverheadSet` counts `minMemoryOverhead`, so setting it guarantees the raw read is reached: `spark.executor.minMemoryOverhead=512m` aborted driver startup with `NumberFormatException` on a value Spark itself accepts, and `docs/velox-spark-configuration.md` lists that config for users. `getSizeAsMb` keeps the result in MiB, which is the unit the surrounding `max` compares against, and it works on every supported Spark version, unlike the 4.0-only `EXECUTOR_MIN_MEMORY_OVERHEAD` entry. A `ResourceProfile` records executor memory amounts in MiB, while `spark.gluten.memory.offHeap.size.in.bytes` and `spark.gluten.memory.task.offHeap.size.in.bytes` are declared `bytesConf(ByteUnit.BYTE)`. `GlutenAutoAdjustStageResourceProfile#updateResourceSetting` wrote the profile amount verbatim, so with `spark.memory.offHeap.size=20g` the session off-heap budget became 20480 bytes instead of 21474836480, and the per-task budget 5120 bytes. The `MEMORY_OFFHEAP_SIZE` fallback on the same expression is already in bytes, so the two branches of one expression disagreed by 2^20. `ByteUnit#toBytes` rejects a negative input but multiplies by 2^20 with no overflow check, so a suffix-less byte count at or above 2^43 wraps. `spark.executor.memory=9000000000000` with dynamic sizing started the driver and produced `spark.gluten.memory.offHeap.size.in.bytes=-5405736044414474240`. `SparkResourceUtil` now has one `mibToBytes` helper holding a `require(>= 0)` and the overflow-raising `convertTo`, and all three MiB-to-byte boundaries go through it. A bare `toBytes` to `convertTo` swap would have regressed the other end, since `convertTo` passes negatives through and an explicit `spark.executor.memoryOverhead=-1` skips the `.max` floor, so the `require` lives inside the helper. `updateResourceSetting` also held a second implementation of `SparkResourceUtil.getTaskSlots` that disagreed with it in local mode: `getTaskSlots` resolves `local[8]` to 8 slots, while a profile reports `spark.executor.cores`, 1 by default. While the total was also 2^20 times too small the two errors cancelled, which is why nobody noticed. Correct the total and it becomes an 8x over-provision, with each of 8 concurrent tasks sized as if it owned the whole budget. Local mode now defers to the shared resolver, and the other branch gains the positivity check and one-slot floor that `getTaskSlots` already carries, so a profile with `CORES=1` and `spark.task.cpus=2` no longer throws `/ by zero` on every query. The off-heap read splits the same way: the unmodified default profile carries the conf value truncated to MiB, so `spark.memory.offHeap.size=1536k` came back as 1048576 instead of the 1572864 the plugin wrote at driver init, and the default-profile call site now says so with an ` isDefaultProfile` flag and reads the conf directly. Deriving that flag from `rp.id` does not work: `nextProfileId` starts at 0, so the first profile constructed in a JVM gets id 0 whether or not it is the default. The second commit also drops `GlutenCoreConfig.SPARK_ONHEAP_SIZE_KEY`, whose last reader the first commit removed. ### How was this patch tested? `SparkResourceUtilSuite` covers the three accessors: `spark.executor.memory` as a bare value and with a suffix, the 1g default, overflow and negative rejection, `minMemoryOverhead` with a suffix and without, the 384m floor, the factor path, and overflow and negative rejection on `memoryOverhead`. `GlutenAutoAdjustStageResourceProfileSuite` covers the rule: the MiB-to-byte conversion, the exact-bytes read for the default profile, agreement with `getTaskSlots` under `local[4]`, the one-slot floor, and the non-positive `spark.task.cpus` rejection. `GlutenDynamicOffHeapSizingSuite` boots a `SparkContext` through `spark.plugins` and asserts the budget derived from a suffix-less `spark.executor.memory`, because the helper-level tests stayed green when only the call site was reverted. Every test was checked against the unfixed code first: reverting each fix individually turns the matching tests red. `mvn -Pspark-3.5 -pl gluten-core,gluten-substrait test` gives 51 and 56 passing. `-Pspark-3.3` compiles both modules; on `-Pspark-4.1 -Pscala-2.13` `gluten-core` compiles and `gluten-substrait` has pre-existing failures unrelated to this change. ### Was this patch authored or co-authored using generative AI tooling? No Closes #12676 -- 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]
