sunchao commented on code in PR #5996: URL: https://github.com/apache/datafusion-comet/pull/5996#discussion_r4038940635
########## docs/source/contributor-guide/memory_management.md: ########## @@ -58,6 +58,51 @@ heap and to Spark's own off-heap accounting, yet they land squarely in container therefore maintains its own budget that is meant to shadow the physical one, and the accuracy of that shadow is the central problem this page is about. +## Memory layout + +Two views of the same container. The first is what Spark and Comet were configured to use, which is +what sizes the pod. The second is what the kernel actually counts, which is what the OOM killer acts +on. Both total the same limit, and the regions do not line up. + +The figures are from one measured executor: TPC-H SF100 Q9, a single executor at a 9 GiB pod limit, +cgroup v2, reading Parquet from local disk. + +```text +container limit (cgroup memory.max) = 9216 MB + = spark.executor.memory + spark.executor.memoryOverhead + spark.memory.offHeap.size + +VIEW A - what Spark configures, and what sizes the pod request ++------------------------------+------------------------------+------------------+ +| JVM heap 4096 MB | off-heap budget 4096 MB | overhead 1024 MB | +| spark.executor.memory | spark.memory.offHeap.size | memoryOverhead | +| | | | +| GC-managed | read TWICE, by two | metaspace, code | +| | allocators that never | cache, thread | +| | compare notes: | stacks, GC | +| | - Spark Tungsten (all) | | +| | - Comet pool (x fraction) | | Review Comment: ### Correctness [P2] Show that native reservations are charged to Spark Could this box and the paragraph below show the shared reservation path? Both supported off-heap pools call Spark through JNI. [`CometFairMemoryPool::try_grow`](https://github.com/apache/datafusion-comet/blob/194ac0246fc3e5461d6f7b9dafc30b8436974708/native/core/src/execution/memory_pools/fair_pool.rs#L143) acquires from Spark after its local limit check, and [`CometTaskMemoryManager.acquireMemory`](https://github.com/apache/datafusion-comet/blob/194ac0246fc3e5461d6f7b9dafc30b8436974708/spark/src/main/java/org/apache/spark/CometTaskMemoryManager.java#L62) charges the task's off-heap execution pool. `greedy_unified` uses the same bridge without the local cap. Thus a 4096 MiB off-heap setting does not give Tungsten and Comet independently spendable 4096 MiB reservation budgets. The allocators do not measure every physical allocation, but the reservations do compete and are released back to Spark. Describing these pools as never comparing notes obscures that distinction and contradict s the later unified-pool section. Please depict one shared Spark budget, the additional fair-pool cap, and the allocations that bypass reservation accounting. ########## docs/source/contributor-guide/memory_management.md: ########## @@ -304,6 +349,14 @@ Two facts follow that are easy to get wrong: 2. **`spark.executor.memoryOverhead` is the only slack in the container**, and the JVM's own non-heap usage already consumes a large part of it. Comet's overshoot beyond its declared reservations eats into the same allowance. +3. **`memory.current` is not a usable pressure signal.** The cgroup counter includes reclaimable + page cache, which grows to fill whatever the container is not otherwise using. On the executor + measured in [Memory layout](#memory-layout) it reached the limit within about twelve seconds of + startup and stayed there for the whole query, while `anon` never exceeded 55% of the limit. A + container sitting at `memory.max` is the normal steady state of any workload that reads files, + not a sign of distress, so a threshold on `memory.current` fires on healthy queries and raising + the threshold only delays that. The non-reclaimable portion, `anon` plus unevictable from + `memory.stat`, is the quantity that predicts a kill. Review Comment: ### Correctness [P2] Do not treat anon plus unevictable as an OOM predictor Could this stay scoped to the measured local-disk workload? [Kernel cgroup documentation](https://docs.kernel.org/admin-guide/cgroup-v2.html#memory-interface-files) counts tmpfs/shared memory within `file`, records kernel charges separately, and describes `unevictable` as reclaim-list state rather than a disjoint type. Spark supports tmpfs spill directories. Without swap, live [tmpfs data](https://docs.kernel.org/filesystems/tmpfs.html) cannot be discarded like clean disk cache, yet the proposed sum misses it. OOM depends on charged usage reaching the limit and reclaim failing. Please qualify View B and the repeated advice so a guard following this guide does not miss that pressure. -- 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]
