You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "sunchao (via GitHub)" <gi...@apache.org> on 2026/06/03 05:14:04 UTC
[PR] [SPARK-57242][CORE] Avoid unbounded page allocation retries after allocator OOM [spark]
sunchao opened a new pull request, #56293:
URL: https://github.com/apache/spark/pull/56293
### Why are the changes needed?
`TaskMemoryManager.allocatePage()` first acquires execution memory and then asks the Tungsten allocator to create the physical page. If the physical allocator throws `OutOfMemoryError`, Spark currently retains the grant and recursively calls `allocatePage()` again.
Repeated physical allocation failures can therefore accumulate acquired-but-unused grants and retry indefinitely instead of recovering or failing promptly. This is the generic allocator failure mechanism discussed by SPARK-54354; SPARK-54818 improved its diagnostics but left the recursive retry behavior unchanged.
### What changes were proposed in this PR?
- Replace recursive page allocation with an iterative retry against the existing execution-memory grant.
- Spill task-managed consumers directly after allocator OOM and retry only while tracked consumer memory decreases.
- Return allocation failure when no consumer can make progress, allowing callers to raise `SparkOutOfMemoryError`.
- Prevent nested page allocations from recursively entering allocator recovery.
- Make failed-grant cleanup idempotent.
- Allocate `ShuffleInMemorySorter` replacement arrays lazily after spilling and restore/recheck the pointer array when page allocation triggers a spill.
### How was this PR tested?
- Added deterministic `TaskMemoryManagerSuite` coverage for allocator OOM without spillable memory, successful spill and retry using the same grant, nested page allocation during recovery, and off-heap allocator failure.
- Added shuffle sorter coverage for lazy pointer-array reset, cleanup after reset, and data-page allocation triggering a spill.
- `TaskMemoryManagerSuite`
- `ShuffleInMemorySorterSuite`
- `ShuffleExternalSorterSuite`
- `UnsafeExternalSorterSuite` (26 JUnit tests passed during a broader core Java test run)
- Core reactor compile and checkstyle
- Scalafmt validation
- `git diff --check`
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57242][CORE] Avoid unbounded page allocation retries after allocator OOM [spark]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on PR #56293:
URL: https://github.com/apache/spark/pull/56293#issuecomment-4743504989
@cloud-fan Good catch, you're right. I missed marking the second minimum-drop branch.
Fixed in `d08d28d`: it now sets the same marker, and `bytesToBytesMapFallsBackFromExactFitPageAfterSpill` reproduces the spill-active sequence `[1024, 4096, 4096, 36]` and verifies that `BytesToBytesMap` falls back rather than retaining the exact-fit page. `TaskMemoryManagerSuite` and both on-heap and off-heap `BytesToBytesMap` suites pass.
The external sorters are different: after accepting an exact-fit page, the next failed full-page allocation makes each sorter self-spill and free its data pages, so they cannot accumulate one page-table slot per record like `BytesToBytesMap`. The guard is therefore intentionally limited to the consumers that cannot safely retain these pages. I also updated the PR description to document the marker and consumer fallback behavior.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57242][CORE] Avoid unbounded page allocation retries after allocator OOM [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #56293:
URL: https://github.com/apache/spark/pull/56293#discussion_r3358947082
##########
core/src/main/java/org/apache/spark/memory/TaskMemoryManager.java:
##########
@@ -96,6 +97,8 @@ public class TaskMemoryManager {
private final MemoryManager memoryManager;
+ private final MemoryAllocator tungstenMemoryAllocator;
Review Comment:
At the first glance, I'm wondering if this is an inevitable change. If this is not inevitable, I'd prefer to avoid the change of constructor by keeping the existing indirect path, `memoryManager.tungstenMemoryAllocator()`
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57242][CORE] Avoid unbounded page allocation retries after allocator OOM [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #56293:
URL: https://github.com/apache/spark/pull/56293#discussion_r3358960662
##########
core/src/main/java/org/apache/spark/memory/TaskMemoryManager.java:
##########
@@ -96,6 +97,8 @@ public class TaskMemoryManager {
private final MemoryManager memoryManager;
+ private final MemoryAllocator tungstenMemoryAllocator;
Review Comment:
I checked the usage of `tungstenMemoryAllocator` variable in this PR. For me, it's not required. Let's revert this change.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57242][CORE] Avoid unbounded page allocation retries after allocator OOM [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #56293:
URL: https://github.com/apache/spark/pull/56293#issuecomment-4653360743
LGTM, also cc @jiangxb1987 @Ngone51
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57242][CORE] Avoid unbounded page allocation retries after allocator OOM [spark]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on PR #56293:
URL: https://github.com/apache/spark/pull/56293#issuecomment-4626048694
cc @cloud-fan @viirya @dongjoon-hyun @peter-toth
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57242][CORE] Avoid unbounded page allocation retries after allocator OOM [spark]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on PR #56293:
URL: https://github.com/apache/spark/pull/56293#issuecomment-4771657435
Thanks for all the reviews! Merged to `master` / `branch-4.x`
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57242][CORE] Avoid unbounded page allocation retries after allocator OOM [spark]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on code in PR #56293:
URL: https://github.com/apache/spark/pull/56293#discussion_r3360433228
##########
core/src/main/java/org/apache/spark/memory/TaskMemoryManager.java:
##########
@@ -96,6 +97,8 @@ public class TaskMemoryManager {
private final MemoryManager memoryManager;
+ private final MemoryAllocator tungstenMemoryAllocator;
Review Comment:
Thanks for checking. Yes, this overload exists only to inject a deterministic failing `MemoryAllocator` in `TaskMemoryManagerSuite`. The existing public constructor is unchanged, and the new constructor is package-private and `@VisibleForTesting`. `MemoryManager.tungstenMemoryAllocator` is final, so keeping only the indirect accessor would not let the tests exercise physical allocator OOM independently from execution-memory grant failures. I am keeping the test-only overload.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57242][CORE] Avoid unbounded page allocation retries after allocator OOM [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57242][CORE] Avoid unbounded page allocation retries after allocator OOM [spark]
Posted by "shrirangmhalgi (via GitHub)" <gi...@apache.org>.
shrirangmhalgi commented on PR #56293:
URL: https://github.com/apache/spark/pull/56293#issuecomment-4619258196
Question (non-blocking): If a consumer's `spill()` callback internally re-acquires execution memory (e.g., via `acquireExecutionMemory` for a non-page allocation, which bypasses `inPageAllocationRecovery`), then `consumerToSpill.getUsed()` after the spill could be ≥ `usedBeforeSpill`. The `Math.max(0L, ...)` clamp treats this as "no progress," and the consumer gets removed from the iteration list.
In that scenario the consumer did free physical memory (which is what we need for the allocator retry), but also re-acquired tracked memory in the same callback. The method would undercount released bytes and potentially give up before the allocator has enough headroom.
Is this intentional best-effort behavior? If so, a brief comment here noting that progress tracking is conservative (may undercount when spill callbacks re-acquire) would help future readers understand why the bounded fallback paths exist even when spillable consumers remain.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57242][CORE] Avoid unbounded page allocation retries after allocator OOM [spark]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on PR #56293:
URL: https://github.com/apache/spark/pull/56293#issuecomment-4625254187
@shrirangmhalgi Thanks for calling this out. Yes, progress here is intentionally measured conservatively as the net reduction in the consumer's tracked memory. `MemoryConsumer.spill()` callbacks must not reacquire execution memory; calling `TaskMemoryManager.acquireExecutionMemory()` directly from a spill callback would violate the same contract. If a custom consumer does violate it, this path treats the consumer as making no progress rather than over-crediting released memory.
I added a short clarification next to the calculation in `7dc2c94627d`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57242][CORE] Avoid unbounded page allocation retries after allocator OOM [spark]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on PR #56293:
URL: https://github.com/apache/spark/pull/56293#issuecomment-4736535467
> Do we need more review, @sunchao ?
Thanks again for the stamp @viirya @dongjoon-hyun @cloud-fan ! Through another testing I a correctness gap in the new allocator-OOM recovery path: certain exact-fit retry pages can cause a false OOM instead of a safe fallback. Therefore, I just applied another update on the PR, which should be narrowly scoped. Please take a look again when you have a chance 🙏 .
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57242][CORE] Avoid unbounded page allocation retries after allocator OOM [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #56293:
URL: https://github.com/apache/spark/pull/56293#issuecomment-4721941330
Do we need more review, @sunchao ?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57242][CORE] Avoid unbounded page allocation retries after allocator OOM [spark]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on code in PR #56293:
URL: https://github.com/apache/spark/pull/56293#discussion_r3364960433
##########
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:
Thanks for checking this. Both guarded helpers return immediately when the flag is already true, so any invocation that reaches `set(true)` necessarily has a prior value of false. A nested invocation cannot enter its own `set`/`finally` scope and therefore cannot clear the outer flag. I prefer keeping `remove()` here because it also removes the ThreadLocal entry from the pooled thread rather than retaining a false value. If the call structure changes to permit nesting in the future, we should introduce a truly scoped helper or depth counter then.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57242][CORE] Avoid unbounded page allocation retries after allocator OOM [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #56293:
URL: https://github.com/apache/spark/pull/56293#discussion_r3358947082
##########
core/src/main/java/org/apache/spark/memory/TaskMemoryManager.java:
##########
@@ -96,6 +97,8 @@ public class TaskMemoryManager {
private final MemoryManager memoryManager;
+ private final MemoryAllocator tungstenMemoryAllocator;
Review Comment:
At the first glance, I'm wondering if this is an inevitable change. If this is not inevitable, I'd prefer to avoid the change of constructor by keeping the existing indirect path, `memoryManager.tungstenMemoryAllocator()`
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57242][CORE] Avoid unbounded page allocation retries after allocator OOM [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #56293:
URL: https://github.com/apache/spark/pull/56293#discussion_r3358960662
##########
core/src/main/java/org/apache/spark/memory/TaskMemoryManager.java:
##########
@@ -96,6 +97,8 @@ public class TaskMemoryManager {
private final MemoryManager memoryManager;
+ private final MemoryAllocator tungstenMemoryAllocator;
Review Comment:
~I checked the usage of `tungstenMemoryAllocator` variable in this PR. For me, it's not required. Let's revert this change.~
##########
core/src/main/java/org/apache/spark/memory/TaskMemoryManager.java:
##########
@@ -96,6 +97,8 @@ public class TaskMemoryManager {
private final MemoryManager memoryManager;
+ private final MemoryAllocator tungstenMemoryAllocator;
Review Comment:
~At the first glance, I'm wondering if this is an inevitable change. If this is not inevitable, I'd prefer to avoid the change of constructor by keeping the existing indirect path, `memoryManager.tungstenMemoryAllocator()`~
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57242][CORE] Avoid unbounded page allocation retries after allocator OOM [spark]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao closed pull request #56293: [SPARK-57242][CORE] Avoid unbounded page allocation retries after allocator OOM
URL: https://github.com/apache/spark/pull/56293
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57242][CORE] Avoid unbounded page allocation retries after allocator OOM [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #56293:
URL: https://github.com/apache/spark/pull/56293#discussion_r3432991495
##########
core/src/main/java/org/apache/spark/memory/TaskMemoryManager.java:
##########
@@ -394,33 +556,107 @@ private MemoryBlock allocatePage(
allocatedPages.set(pageNumber);
}
MemoryBlock page = null;
+ boolean pageAllocated = false;
+ int retryCount = 0;
+ long allocationSize = acquired;
+ long partialAllocationSize = 0;
+ boolean tryingPartialAllocation = false;
+ boolean minimumRetryAfterPartialAllocationFailure = false;
try {
- page = memoryManager.tungstenMemoryAllocator().allocate(acquired);
- } catch (OutOfMemoryError e) {
- if (retryCount == 0) {
- logger.warn("Failed to allocate a page ({} bytes) for {} times, try again.", e,
- MDC.of(LogKeys.PAGE_SIZE, acquired),
- MDC.of(LogKeys.NUM_RETRY, retryCount));
- } else {
- logger.warn("Failed to allocate a page ({} bytes) for {} times, try again.",
- MDC.of(LogKeys.PAGE_SIZE, acquired),
- MDC.of(LogKeys.NUM_RETRY, retryCount));
+ while (true) {
+ try {
+ page = tungstenMemoryAllocator.allocate(allocationSize);
+ break;
+ } catch (OutOfMemoryError e) {
+ logPageAllocationFailure(allocationSize, retryCount, e);
+ if (tryingPartialAllocation) {
+ if (minimumSize > 0 && minimumSize < allocationSize) {
+ // Reuse the retained grant for one final attempt at the caller's usable minimum.
+ long surplus = Math.subtractExact(acquired, minimumSize);
+ releaseExecutionMemory(surplus, consumer);
+ acquired = minimumSize;
+ allocationSize = minimumSize;
+ minimumRetryAfterPartialAllocationFailure = true;
+ retryCount++;
+ continue;
+ }
+ return null;
+ }
+ long released = recoverFromPageAllocationFailure(allocationSize, consumer);
+ if (released > 0) {
+ long remaining = allocationSize - partialAllocationSize;
+ partialAllocationSize += Math.min(released, remaining);
+ } else if (partialAllocationSize > 0 && partialAllocationSize < allocationSize) {
+ // Preserve one bounded attempt to combine the memory made available by spilling with
+ // any remaining free-tail grant, then fall back to a partial page for callers that can
+ // use one. The additional grant may already include the spilled memory, so do not add
+ // it to partialAllocationSize.
+ long additionalAcquired =
+ acquireAdditionalExecutionMemoryForPageAllocation(size, consumer);
+ if (additionalAcquired > 0) {
+ long overlap =
+ additionalAcquired >= partialAllocationSize ? partialAllocationSize : 0L;
+ if (overlap > 0) {
+ releaseExecutionMemory(overlap, consumer);
+ }
+ acquired = Math.addExact(acquired, additionalAcquired - overlap);
+ }
+ allocationSize =
+ additionalAcquired > 0 ? additionalAcquired : partialAllocationSize;
+ tryingPartialAllocation = true;
+ } else if (partialAllocationSize == 0) {
+ // Preserve one bounded attempt to acquire a smaller free-tail grant. The previous
+ // recursive implementation could return a partial page this way after retaining the
+ // rejected grant, but could also retry without bound.
+ long additionalAcquired =
+ acquireAdditionalExecutionMemoryForPageAllocation(size, consumer);
+ if (additionalAcquired <= 0) {
+ return null;
+ }
+ acquired = Math.addExact(acquired, additionalAcquired);
+ allocationSize = additionalAcquired;
+ tryingPartialAllocation = true;
+ } else if (minimumSize > 0 && minimumSize < allocationSize) {
+ // The original grant is still reserved. If the caller padded a smaller allocation to
+ // the configured page size, make one bounded attempt at the minimum usable size without
Review Comment:
This minimum-drop branch returns an exact-fit page (`allocationSize = minimumSize`) just like the new branch at L573-581, but it doesn't set `minimumRetryAfterPartialAllocationFailure`, so the page is never recorded in `pagesAllocatedFromMinimumRetry` (set only at L639). It fires after spilling made progress yet the allocator still OOMs and the surplus is released — i.e. execution memory is available — so its page is pathological in exactly the same way as the branch it does mark.
Because `BytesToBytesMap.acquireNewPage` rejects only *marked* pages (L847), it will accept a page from this branch and re-enter the per-record page-table-slot exhaustion this delta is meant to prevent, reachable whenever a second spillable consumer is present (so recovery spilling makes progress). Consider setting the flag in both minimum-drop branches, e.g. via a shared helper. (No test currently drives this path for B2B — the new B2B tests inject allocator OOM but register no spillable peer, so they only exercise L573.)
##########
core/src/main/java/org/apache/spark/unsafe/map/BytesToBytesMap.java:
##########
@@ -836,11 +836,19 @@ public boolean append(Object kbase, long koff, int klen, Object vbase, long voff
* @return whether there is enough space to allocate the new page.
*/
private boolean acquireNewPage(long required) {
+ final MemoryBlock page;
try {
- currentPage = allocatePage(required);
+ page = allocatePage(required);
} catch (SparkOutOfMemoryError e) {
return false;
}
+ // Retaining exact-fit minimum-retry pages would consume one page-table slot per map entry.
+ if (required < pageSizeBytes && page.size() == required &&
+ isPageAllocationFromMinimumRetry(page)) {
Review Comment:
This rejection is gated on `isPageAllocationFromMinimumRetry`, which `TaskMemoryManager` sets only for the new branch (L573-581), not for the pre-existing minimum-drop at L619-627 — yet both yield pathological exact-fit pages. As written, an exact-fit page from the L619 path slips through and is accepted, re-introducing the page-table-slot exhaustion in spill-active scenarios. See the comment on `TaskMemoryManager.java:621` for the full trace.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57242][CORE] Avoid unbounded page allocation retries after allocator OOM [spark]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on PR #56293:
URL: https://github.com/apache/spark/pull/56293#issuecomment-4651460050
Updated the PR and addressed most of the comments except the one on `ThreadLocal` and `remove()`. Please take another look at the PR @viirya and @cloud-fan , and let me know what you think. 🙏
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57242][CORE] Avoid unbounded page allocation retries after allocator OOM [spark]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on PR #56293:
URL: https://github.com/apache/spark/pull/56293#issuecomment-4628304268
@viirya Thanks for the careful review. I addressed the two concrete maintainability points in `f72fa825533`:
- `MemoryConsumer.spill()` now documents that implementations must release memory synchronously on the calling thread and must not acquire task memory, directly or from another thread.
- The PR description now explicitly calls out the extra pointer-capacity check on shuffle insertion instead of saying successful allocations are entirely unchanged.
I kept the `ThreadLocal` guard rather than switching to a consumer-keyed set. The current guard intentionally covers any nested page allocation on the synchronous recovery call stack, including allocations by a consumer other than the one currently being spilled. A consumer-keyed set would require predicting and registering every possible nested consumer, while allowing an asynchronous spill worker to acquire task memory would conflict with the spill contract and can reintroduce blocking/deadlock behavior. The two guard helpers also cannot currently nest: each checks the guard before setting it, and the recovery loop invokes only one guarded path at a time.
This update also fixes an adversarial shuffle regression found during follow-up validation: with `spark.shuffle.sort.initialBufferSize=1`, the prior head could create one spill per record. Successful pointer growth is now retained, with a one-time minimum-array fallback if it starves the first data page. The 4,097-record test now produces 12 TimSort spills and 14 radix-sort spills instead of 4,097, and radix no longer OOMs. The focused and broader memory/shuffle suites pass (68 tests), as does Checkstyle.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57242][CORE] Avoid unbounded page allocation retries after allocator OOM [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #56293:
URL: https://github.com/apache/spark/pull/56293#discussion_r3358960662
##########
core/src/main/java/org/apache/spark/memory/TaskMemoryManager.java:
##########
@@ -96,6 +97,8 @@ public class TaskMemoryManager {
private final MemoryManager memoryManager;
+ private final MemoryAllocator tungstenMemoryAllocator;
Review Comment:
~I checked the usage of `tungstenMemoryAllocator` variable in this PR. For me, it's not required. Let's revert this change.~
Is it only introduced to inject `TestAllocator` in the test suite?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org