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 2026/09/19 18:52:46 UTC
[PR] feat: account for Comet in spark.executor.memoryOverhead in off-heap mode [datafusion-comet]
andygrove opened a new pull request, #6053:
URL: https://github.com/apache/datafusion-comet/pull/6053
## Which issue does this PR close?
Related to #6050. Draft for discussion — this implements option 2/3 from that issue (auto-adjust), and there is an open question below about whether it can work at all on Spark 3.4, 3.5, and 4.0.
## Rationale for this change
`CometDriverPlugin` adds `spark.comet.memoryOverhead` to `spark.executor.memoryOverhead` only in on-heap mode. In off-heap mode `shouldOverrideMemoryConf` returns `false` and `getCometMemoryOverheadInMiB` returns `0`, so the configuration we recommend to everyone gets no adjustment at all.
As traced in #6050, no change ever decided this on the merits. #1379 made off-heap mode add `getCometShuffleMemorySize` instead of the full overhead; #1485 removed the test-only config that predicate depended on, which turned the off-heap branch into dead code; #1525 deleted the dead branch and #1561 wrote the resulting state up as intentional.
The premise in that comment — "we use unified memory management to share off-heap memory with Spark so do not add overhead" — only covers the memory operators actually reserve. Reservations are charged against `spark.memory.offHeap.size`, which the cluster manager already folds into the container size, so those have room. Everything Comet allocates without reserving it does not: expression kernels and Arrow array builders, decompression buffers and Parquet reader structures, object store request buffers and the tokio runtime, Comet's JVM-side Arrow buffers, and allocator overhead. Those come from the Rust global allocator and live in the native heap, and `spark.executor.memoryOverhead` is the only slack in the container for them.
## What changes are included in this PR?
- `shouldOverrideMemoryConf` no longer excludes off-heap mode, and `getCometMemoryOverheadInMiB` no longer returns `0` there, so the plugin adds `spark.comet.memoryOverhead` (default 1 GiB) to `spark.executor.memoryOverhead` in both modes.
- `COMET_ONHEAP_MEMORY_OVERHEAD` renamed to `COMET_MEMORY_OVERHEAD` (the key `spark.comet.memoryOverhead` is unchanged), moved from `CATEGORY_TESTING` to `CATEGORY_TUNING`, and documented for both modes. It remains the native pool size in on-heap mode; in off-heap mode the pool is still sized from `spark.memory.offHeap.size`, so pool sizing is unchanged in both modes.
- The plugin's `else` branch now reports the reason it did not override (Comet not executing anything) rather than the off-heap reason, which no longer applies.
- Tests: `CometPluginsUnifiedModeOverrideSuite` now asserts the overhead *is* increased in off-heap mode, with a new `CometPluginsUnifiedModeNonOverrideSuite` covering off-heap with Comet exec and shuffle both disabled. `CometSparkSessionExtensionsSuite`'s off-heap case is flipped and gains a case for the disabled path. Three stale comments referencing a 384 MB default are corrected to 1024 MB.
## Open question: this may be a no-op on Spark 3.4, 3.5, and 4.0
I could not find a way for the driver plugin's mutation to reach the executor container on three of the five supported Spark versions, and if that holds it applies to the existing on-heap bump too, not just to this change.
`ResourceProfileManager`'s constructor materializes the default `ResourceProfile` eagerly (`private val defaultProfile = ResourceProfile.getOrCreateDefaultProfile(sparkConf)`), and `getDefaultExecutorResources` snapshots `conf.get(EXECUTOR_MEMORY_OVERHEAD)` into it. The cluster managers size executors from that profile, not from a fresh conf read: `BasicExecutorFeatureStep` builds the pod from `ResourceProfile.getResourcesForClusterManager(...)`, whose `memoryOverheadMiB` comes from `getDefaultProfileExecutorResources(conf)` → the cached profile.
So the ordering in `SparkContext` decides whether the plugin can still influence it:
| Spark | `ResourceProfileManager` | `PluginContainer` | Plugin mutation reaches the container? |
| ----- | ------------------------ | ----------------- | -------------------------------------- |
| 3.4.3 | line 455 | line 565 | no |
| 3.5.8 | line 469 | line 574 | no |
| 4.0.0 | line 486 | line 588 | no |
| 4.1.3 | line 595 | line 594 | yes |
| 4.2.0 | line 595 | line 594 | yes |
`CometPluginsSuite` does not catch this because it asserts on `sc.conf`, which is updated either way, and it runs in local mode where nothing sizes a container.
If that is right, then on 3.4/3.5/4.0 the only thing that works is for the user to set `spark.executor.memoryOverhead` before the context starts — which is what #6051 now documents — and the warn-only option from #6050 becomes the more useful change rather than merely the more conservative one. Worth a second pair of eyes on the analysis before we decide; I have not reproduced it on a real cluster.
## Other things worth deciding
- **Reusing `spark.comet.memoryOverhead` for both modes.** It means one knob, but the knob means two different things: in on-heap mode it sizes the native pool (and the matching container headroom), while in off-heap mode it is purely container headroom for unreserved allocations. A separate off-heap config would be clearer at the cost of another setting.
- **The default is a behavior change.** Every off-heap Comet user's container grows by 1 GiB on Kubernetes and YARN, which changes bin-packing, quotas, and capacity planning for clusters that are fine today. If we want that to be opt-in, the cleanest form is probably a default of `0` in off-heap mode.
- **Interaction with #6051.** That PR recommends setting `spark.executor.memoryOverhead=2g`. If this lands, a user following that advice gets 2 GiB + 1 GiB. The tuning guide should be updated to say so once both are settled.
## How are these changes tested?
`CometPluginsSuite`, `CometPluginsDefaultSuite`, `CometPluginsNonOverrideSuite`, `CometPluginsUnifiedModeOverrideSuite`, `CometPluginsUnifiedModeNonOverrideSuite`, and `CometSparkSessionExtensionsSuite` pass locally against the default profile.
The version-dependent gap described above is not covered by any test, and I do not think it can be in local mode.
--
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] feat: account for Comet in spark.executor.memoryOverhead in off-heap mode [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #6053:
URL: https://github.com/apache/datafusion-comet/pull/6053#issuecomment-5744807609
Closing in favor of #6054.
Extending the adjustment to off-heap mode is the wrong move once you look at whether the adjustment lands anywhere. `ResourceProfileManager` materializes the default `ResourceProfile` in its constructor, snapshotting `spark.executor.memoryOverhead` into it, and the cluster managers size executors from that profile rather than re-reading the conf. On Spark 3.4, 3.5 and 4.0 that happens before `SparkContext` creates the plugin container, so the driver plugin's mutation has never reached the executor container on those versions — it only works on 4.1 and 4.2.
So rather than adding a second mode to a mechanism that silently works on two versions and silently does nothing on three, #6054 removes it and warns at startup instead, which is the only thing that works everywhere.
--
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] feat: account for Comet in spark.executor.memoryOverhead in off-heap mode [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove closed pull request #6053: feat: account for Comet in spark.executor.memoryOverhead in off-heap mode
URL: https://github.com/apache/datafusion-comet/pull/6053
--
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