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/18 15:24:31 UTC
[PR] docs: explain allocator hazards and diagram where memory is allocated [datafusion-comet]
andygrove opened a new pull request, #6014:
URL: https://github.com/apache/datafusion-comet/pull/6014
## Which issue does this PR close?
No separate issue. This is a documentation follow-on to the review discussion on #5998.
**Stacked on #5998.** Its commits are in this branch's history, so the diff shown here includes them
until that PR merges. Only the final commit, which touches
`docs/source/contributor-guide/memory_management.md`, belongs to this PR.
## Rationale for this change
The memory management guide explains the *layout* of Comet's allocators thoroughly, but almost none
of the *hazards*. The constraints that keep the accounting correct lived only in
`CometArrowAllocationListener`'s scaladoc, so a reasonable-looking change elsewhere could break them
silently. Making `getUsed` synchronized, for example, reintroduces a deadlock against a native
reservation arriving on a Tokio worker, and nothing in the guide would have warned against it.
Three smaller gaps came up while reading the page:
- It uses "off-heap" and "native heap" throughout without ever distinguishing them, even though the
difference is the reason Comet's accounting is hard: both sit outside the JVM heap and both count
toward container RSS, but only one is allocated by JVM code and can be reported to Spark.
- The accounting-gap section mentions memory allocated by "C dependencies" without naming any, which
leaves the reader unable to act on it. Several of Comet's compression crates look like C bindings
and are in fact pure Rust, so the guess a reader would make is wrong.
- The page had no diagrams, and "which allocator does this call site use, and who gets charged" is
much easier to show than to describe.
## What changes are included in this PR?
All changes are to `docs/source/contributor-guide/memory_management.md`.
- **A new "Constraints on a JVM-side memory consumer" subsection**, covering the lock order that
requires `getUsed` and `spill` to stay lock-free, the rule that neither Arrow callback may throw
(`BaseAllocator.buffer` creates the buffer before calling `onAllocation`, so a throw loses it),
the two ways a Spark acquisition escapes `NonFatal` (`SparkOutOfMemoryError` is an `Error`, and
`ExecutionMemoryPool.acquireMemory` parks in `lock.wait()` so a task kill raises
`InterruptedException`), the absence of any per-consumer usage figure and why recovering a
stranded partial grant therefore needs the `TaskMemoryManager` monitor, and the fact that a
reporting-only consumer takes budget it can never hand back.
- **Two mermaid diagrams.** One maps each JVM allocation site to the root or per-task allocator and
on to whoever is charged. The other shows everything the pod cgroup counts toward RSS, grouped
into what Spark can see, what only Comet's native pool has declared, and what nobody accounts for.
- **An explanation of off-heap versus native heap**, placed directly under the allocator table where
both terms appear, including the point that `spark.memory.offHeap.size` budgets both even though
the bytes live in different places.
- **Concrete C dependencies** in the non-Rust allocation bullet: libzstd, libhdfs and the object
store TLS stack in a default build, plus the allocator itself under the `jemalloc` or `mimalloc`
feature. It also names the codecs that are pure Rust in this build (`snap`, `lz4_flex`,
`zlib-rs`, `libbz2-rs-sys`) and therefore do pass through `GlobalAlloc`.
- **Plainer wording** for `spark.comet.exec.memoryPool.fraction`, which was described twice as a
"haircut".
## How are these changes tested?
Documentation only, so there are no code tests. Verified by building the docs with the project's
Sphinx environment:
- `sphinx-build -b html docs/source` succeeds. Warning count is 61, unchanged from the same build
before these edits, and no warning names `memory_management`.
- Both mermaid fences render as diagrams rather than literal code blocks: the generated
`memory_management.html` contains two `class="mermaid"` blocks and zero `highlight-mermaid` code
blocks. This relies on the `sphinxcontrib-mermaid` support and `myst_fence_as_directive` that are
already on `main`.
- `prettier --write` reports the page unchanged, so it satisfies the `prettier --check "**/*.md"`
preflight.
--
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] docs: explain allocator hazards and diagram where memory is allocated [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #6014:
URL: https://github.com/apache/datafusion-comet/pull/6014#discussion_r4048879132
##########
docs/source/contributor-guide/memory_management.md:
##########
@@ -289,6 +388,29 @@ 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:
+
+```mermaid
+flowchart TB
+ 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<br>spark.executor.memory"]
+ TUNG["Spark Tungsten off-heap<br>TaskMemoryManager"]
+ SHUFP["Comet JVM shuffle pages<br>CometUnifiedShuffleMemoryAllocator"]
+ end
+ subgraph DECL["declared to Comet's native pool only"]
Review Comment:
You're right, and the diagram contradicted this page's own "the budget is shared even though the memory is not" paragraph a few sections up. I've moved the reservation node into the Spark-visible group and labelled it `declared to Spark over JNI, never measured`, which keeps allocation location and budget accounting distinct without needing a group of its own.
The caption underneath now says Spark's accounting covers that group in two different senses: the JVM heap, Tungsten pages and shuffle pages are reported by the JVM code that allocated them, whereas a native reservation is a number an operator declared before allocating, so the budget really is spent but the reservation is only a lower bound on the bytes behind it.
Three other places made the same claim, so I fixed those too. The visibility cell for the native allocator in the "Who allocates what" table now reads "Reservations only" rather than "No", the native-heap bullet points forward to the shared-budget paragraph instead of stopping at "no JVM metric counts it", and the overview no longer says native allocations are invisible to Spark's off-heap accounting.
Rebuilt with Sphinx: still 61 warnings, unchanged from before the edit, both fences still render as diagrams rather than code blocks, and prettier is clean.
--
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] docs: explain allocator hazards and diagram where memory is allocated [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #6014:
URL: https://github.com/apache/datafusion-comet/pull/6014#issuecomment-5735046617
@andygrove for `native heap` do you mean Spark exec memory overhead area?
--
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] docs: explain allocator hazards and diagram where memory is allocated [datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on code in PR #6014:
URL: https://github.com/apache/datafusion-comet/pull/6014#discussion_r4048627256
##########
docs/source/contributor-guide/memory_management.md:
##########
@@ -289,6 +388,29 @@ 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:
+
+```mermaid
+flowchart TB
+ 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<br>spark.executor.memory"]
+ TUNG["Spark Tungsten off-heap<br>TaskMemoryManager"]
+ SHUFP["Comet JVM shuffle pages<br>CometUnifiedShuffleMemoryAllocator"]
+ end
+ subgraph DECL["declared to Comet's native pool only"]
Review Comment:
### Correctness
[P2] Show native reservations as charged to Spark's accounting
Could we place `NATRES` under Spark-visible accounting, or label it as visible to both Spark and Comet? This page covers off-heap mode, where both `fair_unified` and `greedy_unified` forward successful `try_grow` reservations through JNI to [`CometTaskMemoryManager.acquireMemory`](https://github.com/apache/datafusion-comet/blob/0f9cbaa225b82ffb6e6e9d79c4235004cb30e0d9/spark/src/main/java/org/apache/spark/CometTaskMemoryManager.java#L55-L68). That calls Spark's `acquireExecutionMemory` and updates the native consumer's reported usage. For example, a successful 128 MiB native reservation consumes 128 MiB of Spark's off-heap execution budget, even though Rust allocates the actual buffers. The current separate “Comet's native pool only” group tells readers that these reservations are invisible to Spark, contradicting the earlier shared-budget explanation and allocator diagram. The unreserved native allocations belong outside Spark's accounting, but the declared reservations do not
. Please make the new “no JVM metric counts it” paragraph consistent with this distinction too.
--
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] docs: explain allocator hazards and diagram where memory is allocated [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #6014:
URL: https://github.com/apache/datafusion-comet/pull/6014#issuecomment-5734294216
Thanks for the review @sunchao
--
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] docs: explain allocator hazards and diagram where memory is allocated [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #6014:
URL: https://github.com/apache/datafusion-comet/pull/6014
--
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] docs: explain allocator hazards and diagram where memory is allocated [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #6014:
URL: https://github.com/apache/datafusion-comet/pull/6014#issuecomment-5732338886
diagrams:
<img width="474" height="512" alt="Screenshot 2026-09-18 at 9 35 57 AM" src="https://github.com/user-attachments/assets/32b1de75-18c7-4e77-906b-8ed7a0b35a69" />
<img width="958" height="498" alt="Screenshot 2026-09-18 at 9 35 47 AM" src="https://github.com/user-attachments/assets/f390533c-dab4-42f9-99df-6a453615341f" />
--
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