Copilot commented on code in PR #11500:
URL:
https://github.com/apache/incubator-gluten/pull/11500#discussion_r2735883860
##########
gluten-core/src/main/java/org/apache/gluten/memory/memtarget/spark/TreeMemoryConsumers.java:
##########
@@ -21,37 +21,45 @@
import org.apache.gluten.memory.memtarget.TreeMemoryTarget;
import com.google.common.base.Preconditions;
-import org.apache.commons.collections4.map.AbstractReferenceMap;
-import org.apache.commons.collections4.map.ReferenceMap;
import org.apache.spark.memory.MemoryMode;
import org.apache.spark.memory.TaskMemoryManager;
+import org.apache.spark.task.TaskResource;
+import org.apache.spark.task.TaskResources;
import org.apache.spark.util.Utils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
import java.util.Collections;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
-public final class TreeMemoryConsumers {
- private static final ReferenceMap<TaskMemoryManager, Factory> FACTORIES =
- new ReferenceMap<>(
- AbstractReferenceMap.ReferenceStrength.WEAK,
AbstractReferenceMap.ReferenceStrength.WEAK);
+import scala.Function0;
+public final class TreeMemoryConsumers {
private TreeMemoryConsumers() {}
- public static Factory factory(TaskMemoryManager tmm, MemoryMode mode) {
- synchronized (FACTORIES) {
- final Factory factory = FACTORIES.computeIfAbsent(tmm, m -> new
Factory(m, mode));
- final MemoryMode foundMode = factory.sparkConsumer.getMode();
- Preconditions.checkState(
- foundMode == mode,
- "An existing Spark memory consumer already exists but is of the
different memory "
- + "mode: %s",
- foundMode);
- return factory;
- }
+ public static Factory factory(MemoryMode mode) {
+ final Factory factory =
+ TaskResources.addResourceIfNotRegistered(
+ Factory.class.getSimpleName(),
+ new Function0<Factory>() {
+ @Override
+ public Factory apply() {
+ return new
Factory(TaskResources.getLocalTaskContext().taskMemoryManager(), mode);
+ }
+ });
Review Comment:
`TaskResources.addResourceIfNotRegistered` is keyed by the provided string
ID. Using `Factory.class.getSimpleName()` is relatively collision-prone (e.g.,
any other TaskResource named `Factory` would share the same ID within a task).
Consider using a fully-qualified, namespaced ID (e.g.,
`Factory.class.getName()` or a package-prefixed constant) to match existing
usage patterns like `IndicatorVectorPool.class.getName()` in
`gluten-arrow/src/main/java/org/apache/gluten/columnarbatch/IndicatorVector.java:34-36`
and avoid accidental resource reuse.
##########
gluten-arrow/src/main/java/org/apache/gluten/memory/arrow/alloc/ArrowBufferAllocators.java:
##########
@@ -122,7 +120,7 @@ public void release() throws Exception {
@Override
public int priority() {
- return 0; // lowest priority
+ return 10; // lowest priority
Review Comment:
The inline comment says this is the "lowest priority", but with the current
task resource ordering (higher `priority()` released first), a value of `10` is
not necessarily the lowest (e.g., `TreeMemoryConsumers.Factory` uses `5`).
Please update the comment to describe the intended ordering relative to other
TaskResources, or adjust the priority value/comment to be consistent.
```suggestion
return 10; // low priority: released after higher-priority task
resources
```
##########
gluten-arrow/src/main/scala/org/apache/gluten/memory/NativeMemoryManager.scala:
##########
@@ -113,7 +113,7 @@ object NativeMemoryManager {
override def priority(): Int = {
// Memory managers should be released after all runtimes are released.
// So lower the priority to 0.
Review Comment:
The comment says "lower the priority to 0" but the method now returns `10`.
Please update the comment to reflect the intended ordering (e.g., "set priority
lower than runtime"), or change the returned value to match the comment.
```suggestion
// So set the priority lower than runtime resources.
```
--
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]
