cloud-fan commented on code in PR #56293:
URL: https://github.com/apache/spark/pull/56293#discussion_r3364631673
##########
core/src/main/java/org/apache/spark/memory/TaskMemoryManager.java:
##########
@@ -114,7 +117,14 @@ public class TaskMemoryManager {
/**
* The amount of memory that is acquired but not used.
*/
- private volatile long acquiredButNotUsed = 0L;
+ @GuardedBy("this")
+ private long acquiredButNotUsed = 0L;
+
+ /**
+ * Prevent nested page allocations while spilling from recursively entering
allocator recovery.
+ */
+ private final ThreadLocal<Boolean> inPageAllocationRecovery =
Review Comment:
Non-blocking idiom note. This is a raw `ThreadLocal` managed manually with
`set(true)` ... `finally remove()` in both `recoverFromPageAllocationFailure`
and `acquireAdditionalExecutionMemoryForPageAllocation`. `remove()` resets to
the initial `false` unconditionally rather than restoring the *prior* value, so
nesting the two would clear an outer scope's flag early — the same sharp edge
@viirya raised above.
Spark's idiom for a lexically-scoped thread local is
`org.apache.spark.util.LexicalThreadLocal`, whose `runWith` does save-old / set
/ `finally` restore-old and is nesting-safe by construction. The catch is that
it's a Scala trait carrying state (`private val tl`) with no Java users, so it
can't be cleanly extended from this Java class (the trait field initializer
wouldn't run). Given that, the pragmatic in-language fix is to capture the
prior boolean at entry and restore it in `finally` rather than calling
`remove()`:
```java
final boolean prev = inPageAllocationRecovery.get();
inPageAllocationRecovery.set(true);
try {
...
} finally {
inPageAllocationRecovery.set(prev);
}
```
Current code is correct for the three built-in consumers, so this is
robustness/idiom only — not a blocker.
--
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]
