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: [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]
