sunchao commented on code in PR #6029: URL: https://github.com/apache/datafusion-comet/pull/6029#discussion_r4051437786
########## docs/source/contributor-guide/memory_management.md: ########## @@ -390,13 +390,18 @@ hard ceiling on the sum of everything in the container. That cgroup counts, amon - Comet's JVM-side Arrow buffers (`CometArrowAllocator`), - page cache charged to the cgroup by the container's file I/O, including spill files. -Everything the cgroup counts, and who accounts for each part: +Everything the cgroup counts, which configuration value sizes it, and who accounts for each part: ```mermaid flowchart TB + subgraph CFG["what you configure, summing to the pod limit"] Review Comment: ### Correctness [P2] Account for the optional PySpark allowance in the sum Could you include `spark.executor.pyspark.memory` as a conditional input, or scope this caption to applications without that allowance? Spark's `ResourceProfile.getResourcesForClusterManager` adds the configured PySpark amount for a Python application, and the executor container's request and limit use that total. [The formula immediately above this diagram](https://github.com/apache/datafusion-comet/blob/ffbbd3711c1aef74308e6f5a2a0dc4bda858dce8/docs/source/contributor-guide/memory_management.md#L373-L381) already includes it. For a Python application with 4 GiB heap, 1 GiB overhead, 8 GiB off-heap and 2 GiB PySpark memory, the container limit is 15 GiB. These three inputs sum to 13 GiB. The new unconditional caption therefore understates the configured limit for that supported case. ########## docs/source/contributor-guide/memory_management.md: ########## @@ -409,6 +414,11 @@ flowchart TB FRAG["allocator overhead<br>fragmentation, padding<br>jemalloc retained and dirty pages"] end end + EM --> HEAP + OH --> TUNG + OH --> SHUFP + OH -->|"scaled by spark.comet.exec.memoryPool.fraction"| NATRES Review Comment: ### Correctness [P2] Qualify the fraction edge for `fair_unified` Could you mark this scaling as applying to `fair_unified` only? The page also covers `greedy_unified`, but [the parser discards the scaled limit for that pool](https://github.com/apache/datafusion-comet/blob/ffbbd3711c1aef74308e6f5a2a0dc4bda858dce8/native/core/src/execution/memory_pools/config.rs#L53-L63). Its factory passes no size to `CometUnifiedMemoryPool`, whose `try_grow` delegates directly to Spark. For example, with an 8 GiB off-heap pool and fraction `0.5`, `greedy_unified` does not impose the 4 GiB limit this edge suggests. Lowering the fraction therefore provides no additional headroom in that supported mode. I confirmed the parser behavior for fractions `0.25`, `0.5` and `1.0` using the actual source. Please qualify the label or explicitly show that `greedy_unified` uses Spark's shared limit without the fraction. -- 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]
