Conversation
The cgroup diagram grouped everything the container counts by who accounts for it, but said nothing about which setting sizes each region. That left the two facts the section goes on to state as prose only: that spark.memory.offHeap.size is inside the pod limit rather than headroom on top of it, and that spark.executor.memoryOverhead is the container's only slack. Add the three configuration values as inputs to the diagram. The edges make the asymmetry visible: offHeap.size alone sizes three regions, including Comet's native reservations, which are neither off-heap in Spark's sense nor allocated by the JVM, while memoryOverhead sizes none of them and only widens the container enough to absorb the unaccounted group. Drop spark.executor.memory from the JVM heap node, now that an edge carries it.
sunchao
left a comment
There was a problem hiding this comment.
Correctness
The existing cgroup diagram names memory consumers and their accounting owners. This change adds configuration inputs and edges so readers can connect those consumers to executor sizing. It changes one documentation file, with 17 added lines and two removed lines.
I checked the mapping against Comet at ffbbd371 and the maintained Spark 3.5 and 4.0 sources. In the page's stated off-heap mode, Tungsten, Comet's JVM shuffle pages and Comet's declared native reservations compete for the same Spark off-heap budget. Those three edges must not be read as three independent allocations of offHeap.size. The container includes that allowance once. spark.executor.memory sets the JVM heap ceiling, while Spark's execution/storage accounting manages a subset of that heap. The overhead setting increases the container allowance without creating another Spark execution pool. Undeclared native allocations remain outside that pool's accounting.
Two new labels need correction below. The fraction does not limit greedy_unified, and the three displayed inputs do not sum to the container limit when a Python application has an explicit PySpark memory allowance. Comet's default fair_unified pool and fraction 1.0 are verified, but the diagram is not currently scoped to that default. There are no changes to expression semantics, null handling, ANSI behavior or execution fallback.
Validation and limits
A small Rust probe compiled the actual parse_memory_pool_config source with only its error type stubbed. It confirmed that fair_unified retains the scaled size and greedy_unified discards it at fractions 0.25, 0.5 and 1.0. This was a configuration-parser probe, not Spark/JNI allocation execution or an RSS measurement.
The exact Mermaid fence rendered locally using already-installed Mermaid 11.12.0 and Chromium, and Prettier 3.6.2 passed. This does not validate Mermaid CLI 11.17.0, the Ubuntu deployment runner or a full Sphinx build. The existing deployment issue from #6021 is separate from these new diagram labels.
CI has seven successful checks and fifteen skipped checks. Detect changes and Preflight checked out merge 667be1f2, whose parents are base 5705a58a and this head. Its tree equals the head tree, the changed-file list includes this page, and formatting passed. The docs deployment and product-build jobs were skipped. Maintained Spark 3.4 and 4.1 source branches were unavailable, so source comparison is limited to 3.5 and 4.0.
Performance
This is a documentation-only change with no execution-path overhead. The diagram adds three nodes and five edges to an existing render. A runtime microbenchmark is not applicable, and I have not measured documentation build time or browser rendering cost.
The sizing distinction matters operationally: changing a reservation limit is different from increasing the container limit. The diagram should preserve that distinction without suggesting that a fraction always creates usable physical headroom.
Design
Adding inputs to the existing diagram makes the configuration relationship visible while keeping every allocation inside the same container boundary. The shared off-heap relationship is supported by the JVM and native accounting paths, and the dashed overhead edge distinguishes an allowance from a consumer budget.
The two corrections can stay within this design: qualify the fraction by pool type and include or explicitly exclude the conditional PySpark allowance. No additional diagram or memory model is needed.
Abstraction & complexity
The change uses ordinary Mermaid nodes, edges and a short explanatory paragraph. It introduces no rendering abstraction or runtime component. The structure is appropriate for the page. The main maintenance requirement is keeping the labels' conditions aligned with the pool-selection and container-sizing code.
| EM --> HEAP | ||
| OH --> TUNG | ||
| OH --> SHUFP | ||
| OH -->|"scaled by spark.comet.exec.memoryPool.fraction"| NATRES |
There was a problem hiding this 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. 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.
|
|
||
| ```mermaid | ||
| flowchart TB | ||
| subgraph CFG["what you configure, summing to the pod limit"] |
There was a problem hiding this 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 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.
|
Comet native allocations are not connected to overhead mem or offheap. The allocator just takes mem from pod OS not asking Spark. So Spark have no idea someone else like Comet is consuming memory, as well as Comet doesnt’ respect Spark memory params. Currently they just 2 neighbours living together in the same house, but not knowing about each other The only thing that likely unite then is CometTaskMemoryManager when offheap enabled, when disabled Comet just takes whatever it wants. I think we need to make this clarification in diagram, my head is struggling to come up with idea |
sunchao
left a comment
There was a problem hiding this comment.
Rechecked ffbbd371 against 5705a58a for the new allocation/accounting discussion. The page already limits its scope to off-heap-enabled mode.
The native allocator obtains memory independently of Spark, but declared reservations are connected to Spark: both unified pools call CometTaskMemoryManager.acquireMemory over JNI, and that method charges TaskMemoryManager using an OFF_HEAP consumer. It returns a byte allowance, not allocated pages. Those reservations compete with Spark's off-heap consumers. Native allocations that bypass reservations are outside that budget, so reservation totals do not cap actual native RSS. spark.executor.memoryOverhead enlarges the container allowance without creating a reservation pool or an allocator-owned region. A label such as “shared reservation budget, not an allocation or RSS limit” on the native edge would make the distinction explicit.
Off-heap-disabled mode is outside this diagram and is not automatically unbounded. It derives a separate native budget from spark.comet.memoryOverhead, and selects a DataFusion pool, with unbounded as a separate option.
The head/base and authored diff are unchanged. The existing P2 fraction qualification and P2 conditional PySpark allowance still apply. No new P1/P2 findings or duplicate inlines. Source checks covered maintained Spark 3.5/4.0. CI remains seven successful and fifteen skipped checks, including skipped product builds and site deployment. No new runtime tests or rendering run.
We have two configs to control how much memory Comet can use: Spark uses off-heap memory for buffering sort/shuffle data. Increasing
I agree that the diagram is confusing. |
|
The diagram doesn't really help end users. The main observation from all of this IMO is that we should really recommend that users specify memoryOverhead to some small amount in addition to enabling off-heap. Will create PR to update the docs. |
Which issue does this PR close?
Closes #6028.
Rationale for this change
The cgroup diagram added in #6014 groups everything the container counts by who accounts for it,
but says nothing about which setting sizes each region. That leaves the two facts the section goes
on to state as prose only: that
spark.memory.offHeap.sizeis inside the pod limit rather thanheadroom on top of it, and that
spark.executor.memoryOverheadis the container's only slack.Both are properties of how the configuration maps onto the regions, and the mapping is uneven in a
way that is easy to get wrong.
spark.memory.offHeap.sizealone sizes three regions, includingComet's native reservations, which are neither off-heap in Spark's sense nor allocated by the JVM.
spark.executor.memoryOverheadsizes none of them; it buys no budget any consumer can draw on andonly widens the container enough to absorb the unaccounted group.
What changes are included in this PR?
Three configuration values are added as inputs to the existing diagram, rather than adding a third
diagram to the page:
flowchart TB subgraph CFG["what you configure, summing to the pod limit"] EM["spark.executor.memory"] MO["spark.executor.memoryOverhead"] OH["spark.memory.offHeap.size"] end subgraph CG["pod cgroup memory.max, kernel OOM kill above this"] subgraph SEEN["visible to Spark's accounting"] HEAP["JVM heap<br>execution and storage"] TUNG["Spark Tungsten off-heap<br>TaskMemoryManager"] SHUFP["Comet JVM shuffle pages<br>CometUnifiedShuffleMemoryAllocator"] NATRES["Comet native heap, reserved<br>operators that call try_grow<br>declared to Spark over JNI, never measured"] end subgraph NONE["accounted by nobody"] NATUND["Comet native heap, undeclared<br>kernels, array builders, decompression<br>Parquet metadata, object_store, tokio"] ARROWR["Comet JVM Arrow<br>CometArrowAllocator, unbounded"] NONHEAP["JVM non-heap<br>metaspace, code cache, thread stacks<br>GC structures, Netty direct buffers"] PAGEC["page cache charged to the cgroup<br>file I/O, including spill files"] 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 MO -.->|"no budget, just slack"| NONEAlso:
spark.executor.memorydropped from the JVM heap node, now that an edge carries it.How are these changes tested?
Docs only, no code paths touched.
npx prettier "docs/source/contributor-guide/memory_management.md" --checkpasses, which is whatthe Preflight job runs.
The diagram is not verified under
mmdc. Since #6021 the docs build draws these fences withmermaid-cli, so a syntax error would now fail the build rather than degrade to raw text, and I could
not install
mmdclocally. The rendered block above is GitHub drawing the same source, whichconfirms it parses but not that
mmdcaccepts it. Worth a docs build before merging.