LuciferYang opened a new issue, #12716:
URL: https://github.com/apache/gluten/issues/12716
### Backend
VL (Velox). The code is in `gluten-core` and is shared by every append site.
### Bug description
`Spillers.AppendableSpillerList` holds a plain `ArrayList` that `append`
mutates and `spill` iterates, with no lock on either side. The two run on
different threads.
`NativeMemoryManager` hands the list to `ReservationListeners`, which
registers it as a node of the task's memory tree, before anything is appended
to it. The shuffle writers then append their own spiller on the first non-empty
batch, which is well after `records.next()` started driving the upstream
pipeline. On the other side `TreeMemoryTargets#spillTree` walks every consumer
of the task by design, per its own comment in `MemoryTargets`: "Spill from root
node so other consumers also get spilled". So a spill triggered by any consumer
reaches every other consumer's list, across runtime boundaries. The triggering
thread need not be the task thread either: an allocation on a Velox io thread
goes through that runtime's `ReservationListener`, so an async split prefetch
can fail to reserve and start a root-level walk while the task thread is back
in Java appending. The walk stays inside the loop across a JNI shrink or
reclaim call, so the window is milliseconds rather than one instructi
on.
Two symptoms follow, with different likelihoods.
The common one is a silent skip. `ArrayList`'s `size` and `modCount` are not
volatile and `add` is unsynchronized, so with no happens-before edge the
walking thread may observe neither write, end the walk early, and
under-reclaim. `ThrowOnOomMemoryTarget.borrow` retries the reservation and each
retry re-walks the tree, so this defers one round of reclaim rather than losing
it.
The louder one is a `ConcurrentModificationException`. Once the append is
observed the CME is certain rather than a narrow race: the append raises
`size`, so `hasNext` stays true, so `next()` runs, and its first act is the
`modCount` check. It surfaces through the JNI boundary as `Error during calling
Java code from native code: java.util.ConcurrentModificationException`.
GLUTEN-11509 was this same race one field over, on
`TreeMemoryConsumer#children`, with a production stack trace from the Delta
stats writer thread. Its fix (#11553) switched that map to `ConcurrentHashMap`
and left this list alone. That issue noted the main branch had no asynchronous
use of the memory tree yet; the per-runtime hooked executor for Velox io
threads (#11882, #12302) and the Delta native statistics writer (#11419) both
supply one now.
Five call sites append to such a list: `NativeMemoryManager.scala:59` (in
the constructor), `NativePlanEvaluator.java:94`, and the three shuffle writers
(`ColumnarShuffleWriter.scala:194`,
`VeloxCelebornColumnarShuffleWriter.scala:184`,
`VeloxUniffleColumnarShuffleWriter.java:195`). The first two append while their
`NativeMemoryManager` is still being constructed, so no other thread of the
task is allocating yet. The shuffle writers are the reachable ones, because
their list is registered when the writer is built and appended to only on the
first non-empty batch.
### Gluten version
main (1.8.0-SNAPSHOT)
### Spark version
Version-agnostic (applies to spark-3.3 / 3.4 / 3.5 / 4.0 / 4.1).
### Spark configurations
All defaults. A multi-core executor so `taskSlots > 1` keeps the
`RetryOnOomMemoryTarget` root-level walk in play,
`spark.gluten.memory.isolation=false` (default),
`spark.memory.offHeap.enabled=true`, and Velox io threads default to
`numTaskSlotsPerExecutor`. The query needs a columnar shuffle over a file
source so the io threads are prefetching while the writer appends, plus enough
memory pressure to trigger a spill.
### System information
Not applicable. The affected code is
`gluten-core/src/main/java/org/apache/gluten/memory/memtarget/Spillers.java`,
and does not depend on OS or hardware.
### Relevant logs
From GLUTEN-11509, the same walk on the sibling field, for the shape of the
failure:
```text
org.apache.gluten.exception.GlutenException: Error during calling Java code
from native code: java.util.ConcurrentModificationException
at java.base/java.util.HashMap$HashIterator.nextNode(HashMap.java:1597)
at
org.apache.gluten.memory.memtarget.TreeMemoryTargets.spillTree(TreeMemoryTargets.java:63)
at
org.apache.gluten.memory.memtarget.spark.TreeMemoryConsumer.spill(TreeMemoryConsumer.java:116)
at
org.apache.spark.memory.TaskMemoryManager.trySpillAndAcquire(TaskMemoryManager.java:228)
at
org.apache.spark.memory.TaskMemoryManager.acquireExecutionMemory(TaskMemoryManager.java:191)
```
### Fix direction
Use `CopyOnWriteArrayList`. The two sides cannot be brought under one lock
cheaply, iteration is held open across a JNI spill, and appends are two per
list per task while walks are on the reclaim path, so the copy is cheap.
Snapshot iteration also stays correct if a spiller ever appends during its own
spill, which locking `append` would not cover. Declaring the field as the
concrete type makes a revert to `ArrayList` a compile error, and `@ThreadSafe`
records the guarantee since `MemoryTarget` documents the opposite default.
Note what this does not change: a copy-on-write iterator is a snapshot, so
an append landing mid-walk still misses that round. An index walk would pick it
up and be equally thread-safe, but would let a spiller that appends during its
own spill extend a single walk without bound. Snapshot iteration fixes the work
per round and leans on the existing retry.
--
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]
