Skip to content

feat: report JVM Arrow memory in the executor memory usage log - #6250

Open
andygrove wants to merge 1 commit into
apache:mainfrom
andygrove:jvm-arrow-memory-log
Open

andygrove wants to merge 1 commit into
apache:mainfrom
andygrove:jvm-arrow-memory-log

Conversation

@andygrove

Copy link
Copy Markdown
Member

Which issue does this PR close?

No issue. This replaces #5998, which was the first item of #5997 and took a different approach. The notes on #5997 explain why that approach was dropped. #6167, which asks for resident memory in the same log line, is related.

Rationale for this change

Comet's JVM-side Arrow buffers (CometArrowAllocator) are off-heap memory that no memory pool tracks, so they have to fit in spark.executor.memoryOverhead. The memory tuning guide sizes that overhead from the executor's periodic memory usage log. The log reported only native allocation and pool reservations, though, so the JVM Arrow part was left to the margin.

#5998 tried to charge these buffers to Spark's off-heap pool instead. That turned native spills into task failures: Spark cannot make Comet's native consumer give memory back, so the JVM allocation that feeds a spilling native operator its next batch was refused first. This PR makes the memory visible instead of bounding it.

What changes are included in this PR?

  • Each memory usage log line also reports the Arrow memory Comet holds on the JVM side: the total charged to CometArrowAllocator, and the part of it charged to CometArrowImportAllocator. These are the figures tracing already reports as jvm_arrow_allocated and jvm_arrow_imported. A line from a local run:

    Comet native memory usage: allocated 34.8 MiB, reserved 66.2 MiB (4 native plans, 4 memory pools); JVM Arrow allocated 18.1 MiB, 0.0 MiB of it imported from native
    
  • The container warning now counts the Arrow memory the JVM allocated itself, which is the total less the imported part. Imported buffers are left out because native code allocated them, so allocated already counts them.

    • The JVM figure is added before the pools' reservations are subtracted. A native operator that holds on to a JVM batch, such as a sort buffering a cached scan's output, reserves it, so those bytes are in both the JVM figure and reserved but not in allocated.
    • Subtracting first and adding the JVM figure afterwards would count them twice. In the line above, reserved is larger than allocated partly for this reason.
  • The memory tuning guide's sizing recipe uses the same definition of untracked memory, and no longer leaves JVM Arrow buffers to the margin.

  • The spark.comet.memory.logInterval description, the memory management guide and the tracing guide mention the new figures. The memory management guide also records why charging these buffers to Spark's pool was dropped.

How are these changes tested?

  • CometExecIteratorLifecycleSuite covers the change.
    • The log line includes the JVM Arrow figures.
    • JvmArrowMemory.of reads a root allocator and its import child, keeping the imported part apart.
    • The warning counts Arrow memory the JVM allocated, leaves imported memory to allocated, and counts a JVM batch that native reserves only once. Putting back the subtract-then-add formula fails that last case.
  • I checked the output end to end with a local run: 4 tasks, a sort within partitions over a Comet-cached table, and spark.comet.memory.logInterval=1s. It logged the line quoted above.

The executor's periodic memory usage log reported only what Comet's native
allocator had handed out and what its memory pools had reserved. Arrow memory
that Comet holds on the JVM side appeared in neither, so the tuning guide's
recipe for sizing the memory overhead had to leave it to the margin.

Each line now also reports the total charged to CometArrowAllocator and the
part of it charged to CometArrowImportAllocator, the figures tracing already
reports as jvm_arrow_allocated and jvm_arrow_imported.

The container warning counts the Arrow memory the JVM allocated itself. It is
added to the native allocation before the pools' reservations are subtracted,
because a native operator that holds on to a JVM batch reserves it: those
bytes are in both the JVM figure and the reservations, but not in the native
allocation. The sizing recipe uses the same definition.

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

  • Prior state and problem: The executor memory log omitted JVM Arrow allocator charges, leaving that memory out of the overhead estimate.
  • Design approach: Read the existing root and import allocator counters and incorporate their difference into the container warning.
  • Correctness / compatibility analysis: Adding JVM Arrow memory before subtracting reservations handles JVM batches buffered by native operators without counting their reservations twice. Checked relevant Spark memory APIs and container sizing against Spark 3.4.3, 3.5.9, 4.0.4, 4.1.3 and experimental 4.2.0 sources. The allocator split remains a documented estimate, not a measurement of physical memory.
  • Key design decisions: Reuses the existing executor timer and allocator accounting. The added work is two counter reads and formatting per interval, with no new per-batch instrumentation or allocation enforcement.
  • Implementation sketch: JvmArrowMemory groups the readings, memoryUsageMessage prints them, and nativeMemoryLimitWarning includes the estimated JVM contribution. Tests and tuning documentation cover the new calculation.
  • Behavioral changes worth calling out: Log lines include JVM Arrow totals, and warnings can now reflect JVM Arrow pressure. Query execution, buffer ownership and spilling remain unchanged.
  • Suggested improvements: None meeting the P1/P2 reporting threshold. No introduced P1/P2 issues found within this review.

Reviewed the full eight-file comparison from base bc4be39964cbe9cdb5f2a949740a8164e6b5755b to head 8c8ce915d92013e80196092e39bbc6c449d75fc8, including the migration/deprecation differences attributable to base-only commit #6248. The PR is open and not a draft. Existing reviews, issue comments, inline comments and review threads were empty.

Routed skills: review-comet-pr, review-comet-memory-pr, and review-comet-ffi-pr.

Exact-head CI: 8 checks passed, 9 remained in progress, and 13 were skipped. Native/Spark builds and Rust tests had no completed verdict. Spark SQL, Iceberg and macOS suites were skipped. No failures were reported at the final check.

Validation: Three changed logging/allocator/warning tests passed in an isolated harness using the exact helper source, Arrow 18.3.0, Scala 2.13.18 and JDK 21. git diff --check passed. The full lifecycle suite and end-to-end execution were not run locally: no compiled native artifacts were present, and Maven bootstrap failed resolving repo.maven.apache.org. The isolated harness does not establish full integration or cross-version build success.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants