peterxcli opened a new pull request, #5493:
URL: https://github.com/apache/datafusion-comet/pull/5493
## Which issue does this PR close?
Part of #5212 (positions 6, 7, 9, and 10). It does not close the epic.
## Rationale for this change
`CometDiskBlockWriter.currentWriters` is declared `static final`, so despite
its comment ("of same shuffle task") it is an executor-global registry shared
by every shuffle task in the JVM. This causes several problems, all reproduced
with a deterministic two-task reproducer against current `main`:
- **Cross-task spilling with wrong accounting (position 6):** when task A
cannot allocate a page, `spill()` sorts the global list and force-spills the
largest writers regardless of owner. In the reproducer, task A force-flushed
task B's 3,145,728 buffered bytes (2,751 rows written to B's file from A's
thread, disk-spill metrics charged to B), counted all of it toward its own
`totalFreed`, and spilled none of its own data. With `-ea` (as CI runs), task A
then crashes with `AssertionError` in `initialCurrentPage` because its own
active page was never freed.
- **ABBA deadlock (new finding):** `insertRow` holds the writer monitor and
then takes the registry lock inside `spill()`, while a concurrently spilling
task holds the registry lock and requests the victim writer's monitor. A
benchmark with two concurrent tasks under a shared memory cap deadlocked in 3
of 3 rounds (confirmed via `ThreadMXBean.findDeadlockedThreads()`).
- **`ConcurrentModificationException` (position 7):** the spill comparator
reads other tasks' `allocatedPages` lists unsynchronized while their owners
mutate them; observed live in the same concurrent benchmark.
- **Unsynchronized cross-thread mutation (position 9):** `spilling` and
`totalWritten` are mutated from other tasks' threads without proper
synchronization.
- **Failed-task writer leak:** `stop(false)` frees memory but never removes
writers from the static list, so they are retained for the executor lifetime
(12/12 writers still strongly reachable after GC across three simulated failed
tasks). The retained writers also keep row addresses that point into freed
pages, so a later task spilling them would hand freed addresses to native code.
- **Dead code (position 10):** `spillingWriters` is never populated, so its
loop in `freeMemory()` is unreachable.
## What changes are included in this PR?
- Replace the static registry with a task-owned `LinkedList` created in
`CometBypassMergeSortShuffleWriter.write()` and passed to each partition's
`CometDiskBlockWriter`. Spilling now only ever touches the requesting task's
writers, matching Spark's own shuffle semantics, and every writer is confined
to its task's thread (which also removes the deadlock, the
`ConcurrentModificationException`, and the cross-thread mutation of positions
7/9 structurally). Writers of a failed task become unreachable together with
the task's shuffle writer, removing the leak.
- Delete the dead `spillingWriters` field and its loop in `freeMemory()`.
Single-task spill behavior is unchanged: in an A/B benchmark the spilled
byte counts were identical in every run, and wall-time differences were within
noise. The two-concurrent-task benchmark that deadlocks on `main` completes in
all repetitions with this change.
## How are these changes tested?
New `CometDiskBlockWriterSuite` builds two writers owned by two
`TaskMemoryManager`s sharing one `TestMemoryManager`, drives one task into
memory pressure, and asserts that it resolves the pressure by spilling only its
own writer while the other task's buffered rows, spill metrics, and file remain
untouched. The test fails on `main` (the victim task's buffer is force-flushed
and the requesting task trips the `initialCurrentPage` assertion) and passes
with this 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]
