You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2021/05/21 19:40:11 UTC
[GitHub] [spark] ankurdave opened a new pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
ankurdave opened a new pull request #32625:
URL: https://github.com/apache/spark/pull/32625
### What changes were proposed in this pull request?
When a memory reservation triggers a self-spill, `ExecutionMemoryPool#releaseMemory()` will immediately notify waiting tasks that memory has been freed. If there are any waiting tasks with less than 1/2N of the memory pool, they may acquire the newly-freed memory before the current task has a chance to do so. This will cause the original memory reservation to fail. If the initial spill did not release all available memory, the reservation could have been satisfied by asking it to spill again.
This PR adds logic to TaskMemoryManager to detect this case and retry.
### Why are the changes needed?
This bug affects queries with a MemoryConsumer that can spill part of its memory, such as BytesToBytesMap. If the MemoryConsumer is using all available memory and there is a waiting task, then attempting to acquire more memory on the MemoryConsumer will trigger a partial self-spill. However, because the waiting task gets priority, the attempt to acquire memory will fail even if it could have been satisfied by another spill.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Added a test to MemoryManagerSuite that previously failed and now passes.
--
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.
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
[GitHub] [spark] AmplabJenkins commented on pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32625:
URL: https://github.com/apache/spark/pull/32625#issuecomment-847717093
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/138910/
--
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.
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
[GitHub] [spark] SparkQA commented on pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32625:
URL: https://github.com/apache/spark/pull/32625#issuecomment-846272333
Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43337/
--
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.
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
[GitHub] [spark] AmplabJenkins removed a comment on pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32625:
URL: https://github.com/apache/spark/pull/32625#issuecomment-846490308
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/138836/
--
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.
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
[GitHub] [spark] SparkQA commented on pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32625:
URL: https://github.com/apache/spark/pull/32625#issuecomment-846474992
**[Test build #138836 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138836/testReport)** for PR 32625 at commit [`b293e90`](https://github.com/apache/spark/commit/b293e907daa37d34120cf3ff788bcb0df1c02a30).
--
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.
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
[GitHub] [spark] Ngone51 commented on a change in pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #32625:
URL: https://github.com/apache/spark/pull/32625#discussion_r638390509
##########
File path: core/src/main/java/org/apache/spark/memory/TaskMemoryManager.java
##########
@@ -202,14 +202,22 @@ public long acquireExecutionMemory(long required, MemoryConsumer consumer) {
}
}
- // call spill() on itself
- if (got < required) {
+ // Attempt to free up memory by self-spilling.
+ //
+ // When our spill handler releases memory, `ExecutionMemoryPool#releaseMemory()` will
+ // immediately notify other tasks that memory has been freed, and they may acquire the
+ // newly-freed memory before we have a chance to do so (SPARK-35486). In that case, we will
+ // try again in the next loop iteration.
+ while (got < required) {
try {
long released = consumer.spill(required - got, consumer);
if (released > 0) {
logger.debug("Task {} released {} from itself ({})", taskAttemptId,
Utils.bytesToString(released), consumer);
got += memoryManager.acquireExecutionMemory(required - got, taskAttemptId, mode);
+ } else {
+ // Self-spilling could not free up any more memory.
+ break;
Review comment:
ok, I misread the code.
--
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.
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
[GitHub] [spark] ankurdave commented on a change in pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
ankurdave commented on a change in pull request #32625:
URL: https://github.com/apache/spark/pull/32625#discussion_r638158490
##########
File path: core/src/test/scala/org/apache/spark/memory/MemoryManagerSuite.scala
##########
@@ -240,6 +240,37 @@ private[memory] trait MemoryManagerSuite extends SparkFunSuite with BeforeAndAft
assert(ThreadUtils.awaitResult(t2Result2, 200.millis) === 0L)
}
+ test("SPARK-35486: memory freed by self-spilling is taken by another task") {
+ val memoryManager = createMemoryManager(1000L)
+ val t1MemManager = new TaskMemoryManager(memoryManager, 1)
+ val t2MemManager = new TaskMemoryManager(memoryManager, 2)
+ val c1 = new TestPartialSpillingMemoryConsumer(t1MemManager)
+ val c2 = new TestMemoryConsumer(t2MemManager)
+ val futureTimeout: Duration = 20.seconds
+
+ // t1 acquires 1000 bytes. This should succeed immediately.
+ val t1Result1 = Future { c1.acquireMemory(1000L) }
+ assert(ThreadUtils.awaitResult(t1Result1, futureTimeout) === 1000L)
+ assert(c1.getUsed() === 1000L)
+
+ // t2 attempts to acquire 500 bytes. This should block since there is no memory available.
+ val t2Result1 = Future { c2.acquireMemory(500L) }
+ Thread.sleep(300)
+ assert(!t2Result1.isCompleted)
+ assert(c2.getUsed() === 0L)
+
+ // t1 attempts to acquire 500 bytes, causing its existing reservation to spill partially. t2 is
+ // first in line for the freed memory, so t1 must try again, causing the rest of the reservation
+ // to spill.
Review comment:
That's because of the memory sharing logic in [`ExecutionMemoryPool#acquireMemory()`](https://github.com/apache/spark/blob/d4fb98354a24e6343e8be66543c76cb445ec3a2c/core/src/main/scala/org/apache/spark/memory/ExecutionMemoryPool.scala#L168). After releasing 500 bytes, t1 still has 500 bytes remaining, so it is at its fair share and cannot acquire more memory. The memory therefore goes to t2 instead.
My wording of "first in line" might be misleading since there's no ordered queue; it's all about fair shares. I'll update the comment to explain this.
In detail:
1. Initially, t1 has 1000 bytes, while t2 has 0 bytes. t2's call to `c2.acquireMemory(500)` is blocked inside [`ExecutionMemoryPool#acquireMemory()`](https://github.com/apache/spark/blob/d4fb98354a24e6343e8be66543c76cb445ec3a2c/core/src/main/scala/org/apache/spark/memory/ExecutionMemoryPool.scala#L140), waiting on the lock.
2. t1 spills and releases 500 bytes, leaving it with 500 bytes remaining. It [notifies](https://github.com/apache/spark/blob/d4fb98354a24e6343e8be66543c76cb445ec3a2c/core/src/main/scala/org/apache/spark/memory/ExecutionMemoryPool.scala#L168) t2 via the lock.
3. Both t1 and t2 attempt to acquire memory in `ExecutionMemoryPool#acquireMemory()`. t1 finds that it is already at the `maxMemoryPerTask` of 500 bytes, so it cannot acquire the released memory. t2 is able to acquire the memory instead.
--
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.
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
[GitHub] [spark] AmplabJenkins removed a comment on pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32625:
URL: https://github.com/apache/spark/pull/32625#issuecomment-846485543
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/43358/
--
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.
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
[GitHub] [spark] AmplabJenkins removed a comment on pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32625:
URL: https://github.com/apache/spark/pull/32625#issuecomment-847717093
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/138910/
--
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.
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
[GitHub] [spark] SparkQA commented on pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32625:
URL: https://github.com/apache/spark/pull/32625#issuecomment-846295216
**[Test build #138815 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138815/testReport)** for PR 32625 at commit [`f7f4847`](https://github.com/apache/spark/commit/f7f4847e690b5dd6f406c87daa8f90a05b16bdec).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
--
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.
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
[GitHub] [spark] Ngone51 commented on a change in pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #32625:
URL: https://github.com/apache/spark/pull/32625#discussion_r637812120
##########
File path: core/src/main/java/org/apache/spark/memory/TaskMemoryManager.java
##########
@@ -202,14 +202,22 @@ public long acquireExecutionMemory(long required, MemoryConsumer consumer) {
}
}
- // call spill() on itself
- if (got < required) {
+ // Attempt to free up memory by self-spilling.
+ //
+ // When our spill handler releases memory, `ExecutionMemoryPool#releaseMemory()` will
+ // immediately notify other tasks that memory has been freed, and they may acquire the
+ // newly-freed memory before we have a chance to do so (SPARK-35486). In that case, we will
+ // try again in the next loop iteration.
+ while (got < required) {
try {
long released = consumer.spill(required - got, consumer);
if (released > 0) {
logger.debug("Task {} released {} from itself ({})", taskAttemptId,
Utils.bytesToString(released), consumer);
got += memoryManager.acquireExecutionMemory(required - got, taskAttemptId, mode);
+ } else {
+ // Self-spilling could not free up any more memory.
+ break;
Review comment:
Shall we break earlier when `got >= required`?
--
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.
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
[GitHub] [spark] SparkQA removed a comment on pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #32625:
URL: https://github.com/apache/spark/pull/32625#issuecomment-846228217
**[Test build #138815 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138815/testReport)** for PR 32625 at commit [`f7f4847`](https://github.com/apache/spark/commit/f7f4847e690b5dd6f406c87daa8f90a05b16bdec).
--
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.
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
[GitHub] [spark] SparkQA commented on pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32625:
URL: https://github.com/apache/spark/pull/32625#issuecomment-847582784
**[Test build #138910 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138910/testReport)** for PR 32625 at commit [`3f85e39`](https://github.com/apache/spark/commit/3f85e39584658160951f7ffb1670e9d0779600bc).
--
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.
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
[GitHub] [spark] SparkQA commented on pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32625:
URL: https://github.com/apache/spark/pull/32625#issuecomment-846255062
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43337/
--
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.
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
[GitHub] [spark] AmplabJenkins commented on pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32625:
URL: https://github.com/apache/spark/pull/32625#issuecomment-846304210
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/138815/
--
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.
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
[GitHub] [spark] ankurdave commented on a change in pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
ankurdave commented on a change in pull request #32625:
URL: https://github.com/apache/spark/pull/32625#discussion_r637640349
##########
File path: core/src/main/java/org/apache/spark/memory/TaskMemoryManager.java
##########
@@ -202,14 +202,22 @@ public long acquireExecutionMemory(long required, MemoryConsumer consumer) {
}
}
- // call spill() on itself
- if (got < required) {
+ // Attempt to free up memory by self-spilling.
+ //
+ // When our spill handler releases memory, `ExecutionMemoryPool#releaseMemory()` will
+ // immediately notify other tasks that memory has been freed, and they may acquire the
+ // newly-freed memory before we have a chance to do so (SPARK-35486). In that case, we will
+ // try again in the next loop iteration.
+ while (got < required) {
Review comment:
Good question. The worst case occurs when waiting tasks take all the memory released by `consumer.spill()`. The specific number of iterations depends on the behavior of `consumer.spill()`:
- If `consumer.spill()` releases memory promptly when requested to, as all current implementations do, then this can iterate up to `consumer.getUsed() / (required - got) + 2` times. In all but the last two iterations, `consumer` spills `required - got` bytes; in the second-to-last iteration, `consumer` spills any remaining bytes; and in the last iteration, `consumer` spills 0 bytes.
- If `consumer.spill()` trickles out memory, for example by releasing only 1 byte per call, then there can be up to `consumer.getUsed() + 1` iterations. In all but the last iteration, `consumer` spills 1 byte, and in the last iteration, `consumer` spills 0 bytes.
This is the same worst-case behavior as the `while (!sortedConsumers.isEmpty())` loop above.
--
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.
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
[GitHub] [spark] AmplabJenkins removed a comment on pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32625:
URL: https://github.com/apache/spark/pull/32625#issuecomment-847663947
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/43432/
--
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.
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
[GitHub] [spark] SparkQA commented on pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32625:
URL: https://github.com/apache/spark/pull/32625#issuecomment-846478827
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43358/
--
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.
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
[GitHub] [spark] Ngone51 commented on a change in pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #32625:
URL: https://github.com/apache/spark/pull/32625#discussion_r638393386
##########
File path: core/src/test/scala/org/apache/spark/memory/MemoryManagerSuite.scala
##########
@@ -240,6 +240,37 @@ private[memory] trait MemoryManagerSuite extends SparkFunSuite with BeforeAndAft
assert(ThreadUtils.awaitResult(t2Result2, 200.millis) === 0L)
}
+ test("SPARK-35486: memory freed by self-spilling is taken by another task") {
+ val memoryManager = createMemoryManager(1000L)
+ val t1MemManager = new TaskMemoryManager(memoryManager, 1)
+ val t2MemManager = new TaskMemoryManager(memoryManager, 2)
+ val c1 = new TestPartialSpillingMemoryConsumer(t1MemManager)
+ val c2 = new TestMemoryConsumer(t2MemManager)
+ val futureTimeout: Duration = 20.seconds
+
+ // t1 acquires 1000 bytes. This should succeed immediately.
+ val t1Result1 = Future { c1.acquireMemory(1000L) }
+ assert(ThreadUtils.awaitResult(t1Result1, futureTimeout) === 1000L)
+ assert(c1.getUsed() === 1000L)
+
+ // t2 attempts to acquire 500 bytes. This should block since there is no memory available.
+ val t2Result1 = Future { c2.acquireMemory(500L) }
+ Thread.sleep(300)
+ assert(!t2Result1.isCompleted)
+ assert(c2.getUsed() === 0L)
+
+ // t1 attempts to acquire 500 bytes, causing its existing reservation to spill partially. t2 is
+ // first in line for the freed memory, so t1 must try again, causing the rest of the reservation
+ // to spill.
+ val t1Result2 = Future { c1.acquireMemory(500L) }
+
+ // The spill should release enough memory for both t1's and t2's reservations to be satisfied.
+ assert(ThreadUtils.awaitResult(t2Result1, futureTimeout) === 500L)
+ assert(ThreadUtils.awaitResult(t1Result2, futureTimeout) === 500L)
+ assert(c1.getUsed() === 500L)
Review comment:
Shall we check the spilled size of `c1`? So we're sure the used memory of `c1` at this time is from the spilled memory.
--
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.
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
[GitHub] [spark] AmplabJenkins commented on pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32625:
URL: https://github.com/apache/spark/pull/32625#issuecomment-846490308
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/138836/
--
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.
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
[GitHub] [spark] ankurdave commented on a change in pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
ankurdave commented on a change in pull request #32625:
URL: https://github.com/apache/spark/pull/32625#discussion_r638497473
##########
File path: core/src/test/scala/org/apache/spark/memory/MemoryManagerSuite.scala
##########
@@ -240,6 +240,37 @@ private[memory] trait MemoryManagerSuite extends SparkFunSuite with BeforeAndAft
assert(ThreadUtils.awaitResult(t2Result2, 200.millis) === 0L)
}
+ test("SPARK-35486: memory freed by self-spilling is taken by another task") {
+ val memoryManager = createMemoryManager(1000L)
+ val t1MemManager = new TaskMemoryManager(memoryManager, 1)
+ val t2MemManager = new TaskMemoryManager(memoryManager, 2)
+ val c1 = new TestPartialSpillingMemoryConsumer(t1MemManager)
+ val c2 = new TestMemoryConsumer(t2MemManager)
+ val futureTimeout: Duration = 20.seconds
+
+ // t1 acquires 1000 bytes. This should succeed immediately.
+ val t1Result1 = Future { c1.acquireMemory(1000L) }
+ assert(ThreadUtils.awaitResult(t1Result1, futureTimeout) === 1000L)
+ assert(c1.getUsed() === 1000L)
+
+ // t2 attempts to acquire 500 bytes. This should block since there is no memory available.
+ val t2Result1 = Future { c2.acquireMemory(500L) }
+ Thread.sleep(300)
+ assert(!t2Result1.isCompleted)
+ assert(c2.getUsed() === 0L)
+
+ // t1 attempts to acquire 500 bytes, causing its existing reservation to spill partially. t2 is
+ // first in line for the freed memory, so t1 must try again, causing the rest of the reservation
+ // to spill.
+ val t1Result2 = Future { c1.acquireMemory(500L) }
+
+ // The spill should release enough memory for both t1's and t2's reservations to be satisfied.
+ assert(ThreadUtils.awaitResult(t2Result1, futureTimeout) === 500L)
+ assert(ThreadUtils.awaitResult(t1Result2, futureTimeout) === 500L)
+ assert(c1.getUsed() === 500L)
Review comment:
Done. Since MemoryConsumers don't track the spilled size, I added a `spilledBytes` member to `TestPartialSpillingMemoryConsumer`.
--
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.
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
[GitHub] [spark] ankurdave commented on a change in pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
ankurdave commented on a change in pull request #32625:
URL: https://github.com/apache/spark/pull/32625#discussion_r638108430
##########
File path: core/src/main/java/org/apache/spark/memory/TaskMemoryManager.java
##########
@@ -202,14 +202,22 @@ public long acquireExecutionMemory(long required, MemoryConsumer consumer) {
}
}
- // call spill() on itself
- if (got < required) {
+ // Attempt to free up memory by self-spilling.
+ //
+ // When our spill handler releases memory, `ExecutionMemoryPool#releaseMemory()` will
+ // immediately notify other tasks that memory has been freed, and they may acquire the
+ // newly-freed memory before we have a chance to do so (SPARK-35486). In that case, we will
+ // try again in the next loop iteration.
+ while (got < required) {
try {
long released = consumer.spill(required - got, consumer);
if (released > 0) {
logger.debug("Task {} released {} from itself ({})", taskAttemptId,
Utils.bytesToString(released), consumer);
got += memoryManager.acquireExecutionMemory(required - got, taskAttemptId, mode);
+ } else {
+ // Self-spilling could not free up any more memory.
+ break;
Review comment:
What did you have in mind?
- If `got >= required` prior to calling `consumer.spill()`, the loop condition `while (got < required)` will not be met and we will not execute the loop.
- If `got >= required` after executing `got += memoryManager.acquireExecutionMemory()`, we will finish executing the current loop iteration, then exit the loop.
--
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.
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
[GitHub] [spark] SparkQA removed a comment on pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #32625:
URL: https://github.com/apache/spark/pull/32625#issuecomment-846474992
**[Test build #138836 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138836/testReport)** for PR 32625 at commit [`b293e90`](https://github.com/apache/spark/commit/b293e907daa37d34120cf3ff788bcb0df1c02a30).
--
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.
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
[GitHub] [spark] AmplabJenkins removed a comment on pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32625:
URL: https://github.com/apache/spark/pull/32625#issuecomment-846304210
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/138815/
--
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.
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
[GitHub] [spark] ankurdave commented on pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
ankurdave commented on pull request #32625:
URL: https://github.com/apache/spark/pull/32625#issuecomment-846221547
cc @Ngone51 @dongjoon-hyun @cloud-fan
--
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.
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
[GitHub] [spark] mridulm commented on a change in pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
mridulm commented on a change in pull request #32625:
URL: https://github.com/apache/spark/pull/32625#discussion_r638397154
##########
File path: core/src/test/scala/org/apache/spark/memory/MemoryManagerSuite.scala
##########
@@ -240,6 +240,37 @@ private[memory] trait MemoryManagerSuite extends SparkFunSuite with BeforeAndAft
assert(ThreadUtils.awaitResult(t2Result2, 200.millis) === 0L)
}
+ test("SPARK-35486: memory freed by self-spilling is taken by another task") {
+ val memoryManager = createMemoryManager(1000L)
+ val t1MemManager = new TaskMemoryManager(memoryManager, 1)
+ val t2MemManager = new TaskMemoryManager(memoryManager, 2)
+ val c1 = new TestPartialSpillingMemoryConsumer(t1MemManager)
+ val c2 = new TestMemoryConsumer(t2MemManager)
+ val futureTimeout: Duration = 20.seconds
+
+ // t1 acquires 1000 bytes. This should succeed immediately.
+ val t1Result1 = Future { c1.acquireMemory(1000L) }
+ assert(ThreadUtils.awaitResult(t1Result1, futureTimeout) === 1000L)
+ assert(c1.getUsed() === 1000L)
+
+ // t2 attempts to acquire 500 bytes. This should block since there is no memory available.
+ val t2Result1 = Future { c2.acquireMemory(500L) }
+ Thread.sleep(300)
+ assert(!t2Result1.isCompleted)
+ assert(c2.getUsed() === 0L)
+
+ // t1 attempts to acquire 500 bytes, causing its existing reservation to spill partially. t2 is
+ // first in line for the freed memory, so t1 must try again, causing the rest of the reservation
+ // to spill.
Review comment:
Thanks for the details response @ankurdave ! I missed out on (3) + maxMemoryPerTask - makes sense !
--
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.
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
[GitHub] [spark] Ngone51 commented on pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on pull request #32625:
URL: https://github.com/apache/spark/pull/32625#issuecomment-847742087
Thanks, merged to master!
--
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.
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
[GitHub] [spark] SparkQA commented on pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32625:
URL: https://github.com/apache/spark/pull/32625#issuecomment-846481997
Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43358/
--
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.
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
[GitHub] [spark] SparkQA commented on pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32625:
URL: https://github.com/apache/spark/pull/32625#issuecomment-846487830
**[Test build #138836 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138836/testReport)** for PR 32625 at commit [`b293e90`](https://github.com/apache/spark/commit/b293e907daa37d34120cf3ff788bcb0df1c02a30).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
--
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.
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
[GitHub] [spark] SparkQA commented on pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32625:
URL: https://github.com/apache/spark/pull/32625#issuecomment-847621058
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43432/
--
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.
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
[GitHub] [spark] mridulm commented on a change in pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
mridulm commented on a change in pull request #32625:
URL: https://github.com/apache/spark/pull/32625#discussion_r638111271
##########
File path: core/src/test/scala/org/apache/spark/memory/MemoryManagerSuite.scala
##########
@@ -240,6 +240,37 @@ private[memory] trait MemoryManagerSuite extends SparkFunSuite with BeforeAndAft
assert(ThreadUtils.awaitResult(t2Result2, 200.millis) === 0L)
}
+ test("SPARK-35486: memory freed by self-spilling is taken by another task") {
+ val memoryManager = createMemoryManager(1000L)
+ val t1MemManager = new TaskMemoryManager(memoryManager, 1)
+ val t2MemManager = new TaskMemoryManager(memoryManager, 2)
+ val c1 = new TestPartialSpillingMemoryConsumer(t1MemManager)
+ val c2 = new TestMemoryConsumer(t2MemManager)
+ val futureTimeout: Duration = 20.seconds
+
+ // t1 acquires 1000 bytes. This should succeed immediately.
+ val t1Result1 = Future { c1.acquireMemory(1000L) }
+ assert(ThreadUtils.awaitResult(t1Result1, futureTimeout) === 1000L)
+ assert(c1.getUsed() === 1000L)
+
+ // t2 attempts to acquire 500 bytes. This should block since there is no memory available.
+ val t2Result1 = Future { c2.acquireMemory(500L) }
+ Thread.sleep(300)
+ assert(!t2Result1.isCompleted)
+ assert(c2.getUsed() === 0L)
+
+ // t1 attempts to acquire 500 bytes, causing its existing reservation to spill partially. t2 is
+ // first in line for the freed memory, so t1 must try again, causing the rest of the reservation
+ // to spill.
Review comment:
I am a bit unclear on this - how does t2 get pref over t1 for this released memory ?
--
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.
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
[GitHub] [spark] AmplabJenkins commented on pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32625:
URL: https://github.com/apache/spark/pull/32625#issuecomment-846485543
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/43358/
--
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.
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
[GitHub] [spark] dongjoon-hyun commented on pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #32625:
URL: https://github.com/apache/spark/pull/32625#issuecomment-846620900
Thank you for pinging me, @ankurdave .
--
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.
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
[GitHub] [spark] Ngone51 closed pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
Ngone51 closed pull request #32625:
URL: https://github.com/apache/spark/pull/32625
--
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.
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
[GitHub] [spark] Ngone51 commented on pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on pull request #32625:
URL: https://github.com/apache/spark/pull/32625#issuecomment-846915487
cc @tgravescs @mridulm
--
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.
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
[GitHub] [spark] SparkQA commented on pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32625:
URL: https://github.com/apache/spark/pull/32625#issuecomment-846228217
**[Test build #138815 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138815/testReport)** for PR 32625 at commit [`f7f4847`](https://github.com/apache/spark/commit/f7f4847e690b5dd6f406c87daa8f90a05b16bdec).
--
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.
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
[GitHub] [spark] ankurdave commented on pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
ankurdave commented on pull request #32625:
URL: https://github.com/apache/spark/pull/32625#issuecomment-846689070
Fine with me to consider this for master only.
--
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.
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
[GitHub] [spark] ankurdave commented on a change in pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
ankurdave commented on a change in pull request #32625:
URL: https://github.com/apache/spark/pull/32625#discussion_r638108430
##########
File path: core/src/main/java/org/apache/spark/memory/TaskMemoryManager.java
##########
@@ -202,14 +202,22 @@ public long acquireExecutionMemory(long required, MemoryConsumer consumer) {
}
}
- // call spill() on itself
- if (got < required) {
+ // Attempt to free up memory by self-spilling.
+ //
+ // When our spill handler releases memory, `ExecutionMemoryPool#releaseMemory()` will
+ // immediately notify other tasks that memory has been freed, and they may acquire the
+ // newly-freed memory before we have a chance to do so (SPARK-35486). In that case, we will
+ // try again in the next loop iteration.
+ while (got < required) {
try {
long released = consumer.spill(required - got, consumer);
if (released > 0) {
logger.debug("Task {} released {} from itself ({})", taskAttemptId,
Utils.bytesToString(released), consumer);
got += memoryManager.acquireExecutionMemory(required - got, taskAttemptId, mode);
+ } else {
+ // Self-spilling could not free up any more memory.
+ break;
Review comment:
What did you have in mind?
- If `got >= required` prior to calling `consumer.spill()`, the loop condition `while (got < required)` will not be met and we will not execute the loop.
- If `got >= required` after calling `got += memoryManager.acquireExecutionMemory()`, we will finish executing the current loop iteration, then exit the loop.
--
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.
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
[GitHub] [spark] SparkQA commented on pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32625:
URL: https://github.com/apache/spark/pull/32625#issuecomment-847645554
Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43432/
--
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.
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
[GitHub] [spark] ankurdave commented on a change in pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
ankurdave commented on a change in pull request #32625:
URL: https://github.com/apache/spark/pull/32625#discussion_r637640349
##########
File path: core/src/main/java/org/apache/spark/memory/TaskMemoryManager.java
##########
@@ -202,14 +202,22 @@ public long acquireExecutionMemory(long required, MemoryConsumer consumer) {
}
}
- // call spill() on itself
- if (got < required) {
+ // Attempt to free up memory by self-spilling.
+ //
+ // When our spill handler releases memory, `ExecutionMemoryPool#releaseMemory()` will
+ // immediately notify other tasks that memory has been freed, and they may acquire the
+ // newly-freed memory before we have a chance to do so (SPARK-35486). In that case, we will
+ // try again in the next loop iteration.
+ while (got < required) {
Review comment:
Good question. The worst case occurs when waiting tasks take all the memory released by `consumer.spill()`. The specific number of iterations depends on the behavior of `consumer.spill()`:
- If `consumer.spill()` releases memory promptly when requested to, as all current implementations do, then this can iterate up to `consumer.getUsed() / (required - got) + 1` times. In all but the last iteration, `consumer` spills `required - got` bytes, and in the last iteration, `consumer` spills 0 bytes.
- If `consumer.spill()` trickles out memory, for example by releasing only 1 byte per call, then there can be up to `consumer.getUsed() + 1` iterations. In all but the last iteration, `consumer` spills 1 byte, and in the last iteration, `consumer` spills 0 bytes.
This is the same worst-case behavior as the `while (!sortedConsumers.isEmpty())` loop above.
--
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.
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
[GitHub] [spark] SparkQA commented on pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32625:
URL: https://github.com/apache/spark/pull/32625#issuecomment-847715656
**[Test build #138910 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138910/testReport)** for PR 32625 at commit [`3f85e39`](https://github.com/apache/spark/commit/3f85e39584658160951f7ffb1670e9d0779600bc).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
--
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.
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
[GitHub] [spark] AmplabJenkins removed a comment on pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32625:
URL: https://github.com/apache/spark/pull/32625#issuecomment-846272356
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/43337/
--
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.
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
[GitHub] [spark] SparkQA removed a comment on pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #32625:
URL: https://github.com/apache/spark/pull/32625#issuecomment-847582784
**[Test build #138910 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138910/testReport)** for PR 32625 at commit [`3f85e39`](https://github.com/apache/spark/commit/3f85e39584658160951f7ffb1670e9d0779600bc).
--
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.
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
[GitHub] [spark] AmplabJenkins commented on pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32625:
URL: https://github.com/apache/spark/pull/32625#issuecomment-847663947
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/43432/
--
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.
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
[GitHub] [spark] dongjoon-hyun commented on a change in pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #32625:
URL: https://github.com/apache/spark/pull/32625#discussion_r637598685
##########
File path: core/src/main/java/org/apache/spark/memory/TaskMemoryManager.java
##########
@@ -202,14 +202,22 @@ public long acquireExecutionMemory(long required, MemoryConsumer consumer) {
}
}
- // call spill() on itself
- if (got < required) {
+ // Attempt to free up memory by self-spilling.
+ //
+ // When our spill handler releases memory, `ExecutionMemoryPool#releaseMemory()` will
+ // immediately notify other tasks that memory has been freed, and they may acquire the
+ // newly-freed memory before we have a chance to do so (SPARK-35486). In that case, we will
+ // try again in the next loop iteration.
+ while (got < required) {
Review comment:
How many times does this expect to iterate in the worst case?
--
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.
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
[GitHub] [spark] AmplabJenkins commented on pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32625:
URL: https://github.com/apache/spark/pull/32625#issuecomment-846272356
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/43337/
--
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.
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
[GitHub] [spark] dongjoon-hyun commented on a change in pull request #32625: [SPARK-35486][CORE] TaskMemoryManager: retry if other task takes memory freed by partial self-spill
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #32625:
URL: https://github.com/apache/spark/pull/32625#discussion_r637647625
##########
File path: core/src/main/java/org/apache/spark/memory/TaskMemoryManager.java
##########
@@ -202,14 +202,22 @@ public long acquireExecutionMemory(long required, MemoryConsumer consumer) {
}
}
- // call spill() on itself
- if (got < required) {
+ // Attempt to free up memory by self-spilling.
+ //
+ // When our spill handler releases memory, `ExecutionMemoryPool#releaseMemory()` will
+ // immediately notify other tasks that memory has been freed, and they may acquire the
+ // newly-freed memory before we have a chance to do so (SPARK-35486). In that case, we will
+ // try again in the next loop iteration.
+ while (got < required) {
Review comment:
Thank you for the explanation.
--
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.
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