dongjoon-hyun commented on code in PR #59331:
URL: https://github.com/apache/spark/pull/59331#discussion_r4252554517
##########
core/src/main/java/org/apache/spark/memory/TaskMemoryManager.java:
##########
@@ -113,10 +113,12 @@ public class TaskMemoryManager {
final MemoryMode tungstenMemoryMode;
/**
- * Tracks spillable memory consumers.
+ * Tracks spillable memory consumers. Consumers are tracked by identity
rather than by
+ * {@code equals}, so that distinct consumers which happen to be equal (for
example, Scala case
Review Comment:
This example is stale after the last commit. `MemoryConsumer` now defines
identity `equals`/`hashCode`, so a Scala case class consumer is no longer equal
to another instance. The identity set is now the safety net for subclasses that
explicitly override `equals`, as the PR description says for (2) and (3). Could
we say that here? For example:
```java
/**
* Tracks spillable memory consumers. Consumers are tracked by identity
rather than by
* {@code equals}, so that distinct consumers are all offered for spilling
and reported in the
* memory usage breakdown even if a subclass overrides {@code equals}.
*/
```
##########
core/src/main/java/org/apache/spark/memory/MemoryConsumer.java:
##########
@@ -28,6 +28,13 @@
* A memory consumer of {@link TaskMemoryManager} that supports spilling.
*
* Note: this only supports allocation / spilling of Tungsten memory.
+ *
+ * Memory consumers have identity equality: {@link #equals} and {@link
#hashCode} are defined here
+ * by reference, so a subclass, including a Scala case class, does not get
value equality unless it
Review Comment:
nit. This holds for subclasses compiled against this version. scalac decides
at compile time whether a case class gets synthetic `equals`/`hashCode`, so a
case-class consumer that was compiled against an earlier Spark, for example in
a plugin jar, keeps its value equality on this version until it is rebuilt.
`TaskMemoryManager` still offers each such instance for spilling thanks to the
identity set, but the consumer's own `trigger == this` check still matches an
equal but distinct trigger.
Could we say "when compiled against this version" here, and in the PR
description's "now has identity equality instead"? That would also record why
the identity set and `ne` in `Spillable` are still needed next to this override.
##########
core/src/main/java/org/apache/spark/memory/MemoryConsumer.java:
##########
@@ -156,6 +166,21 @@ public void freeMemory(long size) {
used.getAndAdd(-size);
}
+ /**
+ * Memory consumers are compared by identity. This is not final so that
existing subclasses which
+ * override it keep compiling, but its presence stops Scala case classes
from generating value
+ * equality.
+ */
+ @Override
+ public boolean equals(Object other) {
Review Comment:
A note on the merge order with #59282, which is still open. It adds this to
`HybridQueue`:
```scala
// A memory consumer must use identity equality. TaskMemoryManager tracks
consumers in a
// HashSet, so two queues that compare equal would be tracked (and offered
for spilling) as one.
// These are final so that no subclass, including a case class, can switch
to value equality.
final override def equals(other: Any): Boolean = ...
final override def hashCode(): Int = System.identityHashCode(this)
```
Keeping `equals`/`hashCode` non-final here is what lets that code compile on
top of this PR, which is good. But once both PRs are on master, the `HashSet`
sentence is wrong and the two overrides repeat this base class. Could whichever
PR merges second reconcile that comment on master, for example by pointing it
at the `MemoryConsumer` contract? On the maintenance branches, which get only
#59282, the comment and the overrides stay accurate.
##########
core/src/main/java/org/apache/spark/memory/MemoryConsumer.java:
##########
@@ -28,6 +28,13 @@
* A memory consumer of {@link TaskMemoryManager} that supports spilling.
*
* Note: this only supports allocation / spilling of Tungsten memory.
+ *
+ * Memory consumers have identity equality: {@link #equals} and {@link
#hashCode} are defined here
+ * by reference, so a subclass, including a Scala case class, does not get
value equality unless it
+ * explicitly overrides them. {@link TaskMemoryManager} also tracks consumers
by identity, so every
+ * distinct instance is tracked, offered for spilling and reported in the
memory usage breakdown,
Review Comment:
nit. "every distinct instance is tracked, offered for spilling and reported
in the memory usage breakdown" says a bit more than `TaskMemoryManager` does:
- A consumer is added to the set only when it calls `acquireExecutionMemory`
(`TaskMemoryManager.java` L256).
- It is offered for spilling only while `getUsed() > 0`, and only to a
requester with the same memory mode (L232, L355).
- The error breakdown lists at most `oomErrorConsumerBreakdownLimit`
consumers and folds the rest into "(N more consumers)" (L544-L556).
Could we limit the sentence to what identity guarantees? For example:
"distinct instances are never merged into one entry, so each of them can be
offered for spilling and reported in the memory usage breakdown".
##########
core/src/test/scala/org/apache/spark/memory/MemoryConsumerSuite.scala:
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.memory
+
+import org.apache.spark.{SparkConf, SparkFunSuite}
+
+class MemoryConsumerSuite extends SparkFunSuite {
+
+ test("case class consumers have identity equality") {
+ val taskMemoryManager = new TaskMemoryManager(new TestMemoryManager(new
SparkConf()), 0)
+ val consumer = CaseClassConsumer(taskMemoryManager, 1)
+ val other = CaseClassConsumer(taskMemoryManager, 1)
+
+ // The case class does not generate value-based equals/hashCode because
MemoryConsumer
+ // already defines them.
+ assert(consumer != other)
+ assert(consumer == consumer)
+ assert(consumer.hashCode === System.identityHashCode(consumer))
+ // A spill triggered by an equal-looking consumer is not mistaken for a
self-triggered one.
+ assert(consumer.spill(Long.MaxValue, other) === 1)
Review Comment:
This calls `spill` directly on consumers that are not registered with
`taskMemoryManager`. It checks that `==` is identity for a case class, but not
the end-to-end case that the description says is now fixed regardless of the
merge order with #59282: two case-class consumers with equal arguments, both
registered with `TaskMemoryManager`, where one requests more memory than is
free and the other actually spills. `SpillableSuite` was changed to drive that
through `acquireExecutionMemory` for the same reason. Could we do the same here?
Separately, `CaseClassConsumer.spill` (L44-L45) uses `trigger == this` and
says that is fine because `equals` is identity. The `spill` Javadoc added in
the same commit tells implementers to compare `trigger` by reference. Since
this is the only new example of a case-class consumer, maybe the comment could
add that real consumers should still use `eq`, per that Javadoc.
##########
core/src/main/java/org/apache/spark/memory/MemoryConsumer.java:
##########
@@ -156,6 +166,21 @@ public void freeMemory(long size) {
used.getAndAdd(-size);
}
+ /**
+ * Memory consumers are compared by identity. This is not final so that
existing subclasses which
Review Comment:
nit. The PR title (and the SPARK-60112 title) still say "Track memory
consumers by identity in TaskMemoryManager". But the description now calls this
override (1) the root-cause fix, and the `TaskMemoryManager` set the safety
net. Since the title becomes the commit subject, could we update it to cover
this part too, e.g. "[SPARK-60112][CORE] Use identity equality for memory
consumers"?
--
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]
