HyukjinKwon commented on code in PR #59331:
URL: https://github.com/apache/spark/pull/59331#discussion_r4242051158
##########
core/src/main/java/org/apache/spark/memory/MemoryConsumer.java:
##########
@@ -28,6 +28,11 @@
* A memory consumer of {@link TaskMemoryManager} that supports spilling.
*
* Note: this only supports allocation / spilling of Tungsten memory.
+ *
+ * {@link TaskMemoryManager} tracks consumers by identity, so distinct
consumer instances are
+ * always tracked, spilled and reported separately, regardless of how they
implement
Review Comment:
**Non-blocking (P2):** This guarantee holds for the registry, but not yet
end to end. TaskMemoryManager now offers every distinct instance, and then the
consumer's own `spill(size, trigger)` decides whether to release anything. Two
Scala bases still decide that by `equals`:
- `HybridQueue.spill` (sql/core/.../python/HybridQueue.scala:73): `if
(trigger == this) return 0L`
- `Spillable.spill` (core/.../util/collection/Spillable.scala:119): `if
(trigger != this && ...)`
Scala `==`/`!=` on references call `equals`. `HybridRowQueue` is a case
class, so with two queues built from the same arguments (the scenario in the PR
description), queue A's page allocation now reaches `B.spill(size, A)`, `A ==
B` is true, and B returns 0. The spill loop drops B, and its memory is still
not freed for A's request, exactly as when B was untracked. Only requests from
a non-equal consumer, such as a sorter in the same task, benefit. I checked the
semantics with a minimal Ammonite snippet of the same shape: `a eq b` is false,
`a == b` is true, and `b.spill(a)` returns 0.
The new tests don't exercise this because `ValueEqualConsumer` inherits
`TestMemoryConsumer.spill`, which ignores the trigger.
Switching those two checks to reference identity (`trigger eq this` /
`trigger ne this`) matches the Java consumers' `!=` and `SegTreeSpiller`'s
existing `trigger eq this`, and makes this sentence true for in-tree consumers.
A small `RowQueueSuite` case with two equal `HybridRowQueue`s on one
`TaskMemoryManager` would cover it.
**Verification:**
- **Regression:** Two equal HybridRowQueue instances on one
TaskMemoryManager under a tight execution-memory limit: when the second queue's
allocation forces a spill, the first queue's in-memory memory is released. With
the equals-based check the first queue would keep its memory.
- **Behavior:** A HybridQueue that is itself the trigger still returns 0
from spill and keeps writing upcoming rows to a disk queue.
- **Compatibility:** Spill behavior of the existing Spillable collections
(ExternalSorter, ExternalAppendOnlyMap) is unchanged, since they inherit
reference equality.
--
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]
