comphead commented on PR #6121: URL: https://github.com/apache/datafusion-comet/pull/6121#issuecomment-5783763935
Checked the redraw against the sources. Separating region from authority is the right axis and worth landing. Four things I would change first. **1. The reservation-ceiling label is false for one pool type and imprecise for the other.** `parse_memory_pool_config` gives `greedy_unified` a `pool_size` of `0` (`native/core/src/execution/memory_pools/config.rs:57-62`) and `CometUnifiedMemoryPool` never reads a limit, so `spark.comet.exec.memoryPool.fraction` has no effect there at all. Under the default `fair_unified` the check is `pool_size / num_consumers` against the pool's *total* used, not the requester's own (`fair_pool.rs:149-163`), so the ceiling is a fraction of a fraction. Neither is usually the binding constraint. `ExecutionMemoryPool.acquireMemory` caps a task at `maxPoolSize / numActiveTasks` and parks it below `poolSize / (2 * numActiveTasks)`. With 8 concurrent tasks a Comet plan gets roughly 1/8 of the off-heap pool, not all of it. **2. `COBJ --> ONPOOL` is a new inaccuracy.** The unified memory manager accounts only for what consumers explicitly reserve plus cached block sizes. Comet's plans, `CometVector`s and iterators are ordinary heap objects in user memory, the `1 - spark.memory.fraction` slice. The old undifferentiated `HEAP` box was vaguer but less wrong. **3. The single `OFFPOOL` box hides the execution/storage split.** `spark.memory.offHeap.size` is divided by `spark.memory.storageFraction` into off-heap execution and off-heap storage pools (`UnifiedMemoryManager.scala:64,98-102`). Comet only touches the execution half, and reaches the rest only by evicting `OFF_HEAP` cached blocks. **4. Scope is missing.** The pool, the reservation and the fair share are all per task attempt (`task_shared.rs`), but the diagram is drawn per container, so a reader cannot see that `spark.executor.cores / spark.task.cpus` of these run at once. Two things that predate this PR but sit next to the diagram: - `memory_management.md` still says to compare `jemalloc_allocated` against "the summed `thread_NNN_comet_memory_reserved` values". Since #5934 there is an `alloc-accounting` feature emitting `native_allocated` (`native/core/src/alloc_accounting.rs`), and `jni_api.rs:252-255` warns that the per-thread counters must *not* be summed, because a shared pool reports its full reservation on every thread that references it. `comet_memory_reserved_total` is the figure to use. `tracing.md` has this right, and the "No signal for real native usage" open problem is stale for the same reason. - Each `createPlan` builds its own `RuntimeEnv` (`jni_api.rs:618,793`) and therefore its own `FileMetadataCache`, capped at DataFusion's 50 MiB default. A shuffle runs two plans per task, so eight concurrent tasks can hold up to roughly 800 MiB of Parquet metadata that no pool sees. Worth a node. An alternative diagram covering all of the above is in the next comment. -- 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]
