sunchao commented on PR #5493:
URL:
https://github.com/apache/datafusion-comet/pull/5493#issuecomment-5529155657
**The overall design is sound, and the abstraction boundaries are
appropriate. I would keep the design and make two focused improvements to the
compatibility allocator.** I checked the unchanged head, `59b7cf2b`, with five
independent review scopes and local measurements.
1. **Avoid constructing an exception on every unsuccessful allocation
retry.**
In
[`allocateBlocking()`](https://github.com/apache/datafusion-comet/blob/59b7cf2b27765a7803e0db6526699189cff522c9/spark/src/main/java/org/apache/spark/shuffle/comet/CometBoundedShuffleMemoryAllocator.java#L177),
each wakeup attempts allocation and constructs a new `SparkOutOfMemoryError`
if capacity is still insufficient. Exception construction, formatting, and
stack unwinding happen while holding the shared allocator lock.
This is measurable. With approximately 1,000 small frees per second,
blocked requests that still cannot fit produced:
| Blocked tasks | Current code: waiter heap allocation | Capacity-check
prototype |
|---|---:|---:|
| 8 | 29.46 MiB | 0.22 MiB |
| 32 | 54.24 MiB | 0.49 MiB |
Each measurement covers **1.2 seconds**. For eight waiters, combined
waiter CPU also fell from approximately **546 ms to 367 ms**.
I would introduce one private `tryAllocateMemoryBlock()` helper that
returns a shortage indication. The immediate allocation API converts shortage
into an exception; the blocking API runs its existing waiting checks. Construct
the exception only when the operation must fail. This preserves cancellation,
timeout, notifications, and deadlock checks while removing substantial
temporary allocation.
2. **Read the timeout setting only when the call actually needs to wait.**
The [current
lookup](https://github.com/apache/datafusion-comet/blob/59b7cf2b27765a7803e0db6526699189cff522c9/spark/src/main/java/org/apache/spark/shuffle/comet/CometBoundedShuffleMemoryAllocator.java#L165)
happens even when spilling has already freed enough memory.
For a successful `allocateBlocking()` followed by `free()`, two
independent JVM runs measured approximately:
| Implementation | Time per pair | Java heap allocated |
|---|---:|---:|
| Current head | 329–336 ns | 432 bytes |
| Capacity check with deferred timeout lookup | 110–126 ns | 112 bytes |
This is a small absolute cost per page, but the change is
straightforward. Keep the setting specific to the requesting call; moving it
back into the singleton constructor would reintroduce the earlier configuration
bug.
The other performance choices look reasonable:
- **Task-owned writer lists improve contention and reduce the spill search
space.**
- **Caller-first spilling usually avoids sorting siblings entirely.** The
remaining full sort repeatedly scans writers’ page lists, but that cost
predates this PR and now occurs less often. A linear victim search could help
particular workloads; the evidence does not justify a more elaborate priority
queue.
- **Ownership accounting adds modest normal-path overhead:** roughly 45–65
ns and 72 bytes of temporary Java heap per allocation/free pair in my component
test. It occurs per page, and only in the compatibility allocator. The usual
off-heap allocator avoids this machinery.
- The row, file, and metrics synchronization is largely inherited. Its
presence should not be counted as newly introduced overhead.
**The added state is mostly necessary complexity.** Page ownership, retained
bytes, and waiting request sizes answer different questions. The separate
immediate and blocking allocation APIs also matter: callers must first get a
chance to spill their buffers. Constructor cleanup, write-failure cleanup, and
task-completion cleanup cover different lifetime boundaries.
The alternatives have clear tradeoffs:
| Alternative | Assessment |
|---|---|
| Current design with a nonthrowing allocation helper | Best fit here:
simpler retry logic without losing safeguards. |
| Timeout-only waiting | Less code, but impossible requests can occupy task
slots for five minutes. |
| Fixed per-task memory budgets | Simpler accounting, but unused capacity
becomes stranded and uneven tasks spill more. |
| Fair allocation queue | Could improve fairness under contention, but adds
scheduling and mixed-request-size handling. It still needs retained-memory
safeguards. |
| Use Spark’s memory manager for every mode | Worth considering as broader
architecture work; it changes how the compatibility mode’s separate off-heap
budget is accounted for. |
The local prototype passed **14/14 focused allocator and writer regression
tests**. These measurements establish allocator-level overhead on Spark
4.1.3/JDK 17; they do not establish an end-to-end query speedup. The PR’s
reported spill benchmarks cover a different part of the change.
--
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]
