[SPARK-17465] [Spark Core] Inappropriate memory management in `org.apache.spark.storage.MemoryStore` may lead to memory leak by saturday-shi · Pull Request #15022 · apache/spark

saturday-shi · 2016-09-09T08:35:47Z

What changes were proposed in this pull request?

The expression like if (memoryMap(taskAttemptId) == 0) memoryMap.remove(taskAttemptId) in method releaseUnrollMemoryForThisTask and releasePendingUnrollMemoryForThisTask should be called after release memory operation, whatever memoryToRelease is > 0 or not.

If the memory of a task has been set to 0 when calling a releaseUnrollMemoryForThisTask or a releasePendingUnrollMemoryForThisTask method, the key in the memory map corresponding to that task will never be removed from the hash map.

See the details in SPARK-17465.

…lMemoryForThisTask and releasePendingUnrollMemoryForThisTask method.

HyukjinKwon · 2016-09-09T11:09:29Z

Please ignore the failure of AppVeyor for this. This is supposed to be ran only for R changes only in master branch but it seems it gose something wrong. This should fail for branch-1.6 as appveyor.yml and the related script are currently only merged into master.

HyukjinKwon · 2016-09-09T11:12:27Z

Cc @shivaram it seems something went wrong. I will try to figure out this as soon as I can use my laptop.

srowen · 2016-09-09T12:25:03Z

That seems correct; I'm not sure if @andrewor14 is still available to comment?

JoshRosen · 2016-09-09T15:32:33Z

This change looks good to me as well, although I wonder whether there's an assertion or unit test that we can add to detect this bug or keep things from regressing in case this is ever refactored.

I suppose that one invariant is that unrollMemoryMap.keys.size and pendingUnrollMemoryMap.keys.size should both be upper-bounded by the number of executor cores (since that's the maximum number of concurrent tasks).

I wouldn't necessarily block merging this on the addition of new tests / asserts right now but it would be great to add them if they're low-cost and it's easy to do so.

JoshRosen · 2016-09-09T15:32:36Z

Jenkins, this is ok to test.

JoshRosen · 2016-09-09T15:37:46Z

It looks like we might want to pre-emptively make a similar change in master / branch-2.0, since it looks like there's still a potential leak here in case the unroll memory for the task is 0:

spark/core/src/main/scala/org/apache/spark/storage/memory/MemoryStore.scala

Line 596 in 2ed6012

if (unrollMemoryMap(taskAttemptId) ==0) {

SparkQA · 2016-09-09T17:34:33Z

Test build #65154 has finished for PR 15022 at commit 720f2ce.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

JoshRosen · 2016-09-09T18:27:18Z

Rather, it looks like the 2.x patch is going to just be a subset of this one, so I can merge this into both branches.

JoshRosen · 2016-09-09T19:20:46Z

Actually, I think that I spot a simple way to add a test, similar to the memory-leak-detection tests that we have for BlockInfoManager in Spark 2.x. Let me test out my idea and I'll post the diff here or will submit a PR to your PR.

JoshRosen · 2016-09-09T20:09:31Z

Actually, I spot one more step to make this really robust: I think we also need to call releasePendingUnrollMemoryForThisTask at the end of Task in order to be absolutely sure this memory will be released during error cases.

Can you add a call to releasePendingUnrollMemoryForThisTask to

spark/core/src/main/scala/org/apache/spark/scheduler/Task.scala

Line 106 in 047bc3f

SparkEnv.get.blockManager.memoryStore.releaseUnrollMemoryForThisTask()

?

saturday-shi · 2016-09-10T07:48:28Z

Thank @JoshRosen for your reply!

Actually, I spot one more step to make this really robust: I think we also need to call releasePendingUnrollMemoryForThisTask at the end of Task in order to be absolutely sure this memory will be released during error cases.

That's right. I will make the change later.
And it is really helpful if you have an good idea of adding a test to avoid regressing. I just worrying about that I don't have an easy way to check if the problem relapsed or not.

…for more robustness.

SparkQA · 2016-09-10T12:12:37Z

Test build #65200 has finished for PR 15022 at commit 6b11fe8.

This patch fails from timeout after a configured wait of 250m.
This patch merges cleanly.
This patch adds no public classes.

srowen · 2016-09-13T08:45:50Z

Jenkins retest this please

SparkQA · 2016-09-13T10:44:28Z

Test build #65307 has finished for PR 15022 at commit 6b11fe8.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

JoshRosen · 2016-09-14T20:46:15Z

LGTM so I'm going to merge this into branch-1.6.

@saturday-shi, do you mind closing this pull request now that I've merged it? GitHub won't auto-closed merged PRs which aren't opened against the master branch, so you'll have to do it.

I'm going to port a subset of this change into the master branch and may add additional tests there. I don't think there's much risk of this regressing in branch-1.6 given that we're really conservative about making changes in that branch and it's unlikely any of the memory management code there will be modified again soon.

…che.spark.storage.MemoryStore` may lead to memory leak ## What changes were proposed in this pull request? The expression like `if (memoryMap(taskAttemptId) == 0) memoryMap.remove(taskAttemptId)` in method `releaseUnrollMemoryForThisTask` and `releasePendingUnrollMemoryForThisTask` should be called after release memory operation, whatever `memoryToRelease` is > 0 or not. If the memory of a task has been set to 0 when calling a `releaseUnrollMemoryForThisTask` or a `releasePendingUnrollMemoryForThisTask` method, the key in the memory map corresponding to that task will never be removed from the hash map. See the details in [SPARK-17465](https://issues.apache.org/jira/browse/SPARK-17465). Author: Xing SHI <shi-kou@indetail.co.jp> Closes#15022 from saturday-shi/SPARK-17465.

JoshRosen · 2016-09-14T20:47:36Z

 Utils.tryLogNonFatalError {
 // Release memory used by this thread for unrolling blocks
 SparkEnv.get.blockManager.memoryStore.releaseUnrollMemoryForThisTask()
+ SparkEnv.get.blockManager.memoryStore.releasePendingUnrollMemoryForThisTask()


We ended up removing the concept of "pending unroll memory" in Spark 2.x so this line will be omitted from the PR that I'm going to open against master.

…che.spark.storage.MemoryStore` may lead to memory leak The expression like `if (memoryMap(taskAttemptId) == 0) memoryMap.remove(taskAttemptId)` in method `releaseUnrollMemoryForThisTask` and `releasePendingUnrollMemoryForThisTask` should be called after release memory operation, whatever `memoryToRelease` is > 0 or not. If the memory of a task has been set to 0 when calling a `releaseUnrollMemoryForThisTask` or a `releasePendingUnrollMemoryForThisTask` method, the key in the memory map corresponding to that task will never be removed from the hash map. See the details in [SPARK-17465](https://issues.apache.org/jira/browse/SPARK-17465). Author: Xing SHI <shi-kou@indetail.co.jp> Closes#15022 from saturday-shi/SPARK-17465.

JoshRosen · 2016-09-14T21:01:48Z

Actually I managed to cherry-pick the compatible subset of the changes into master, so the PR will be auto-closed by that.

…che.spark.storage.MemoryStore` may lead to memory leak ## What changes were proposed in this pull request? The expression like `if (memoryMap(taskAttemptId) == 0) memoryMap.remove(taskAttemptId)` in method `releaseUnrollMemoryForThisTask` and `releasePendingUnrollMemoryForThisTask` should be called after release memory operation, whatever `memoryToRelease` is > 0 or not. If the memory of a task has been set to 0 when calling a `releaseUnrollMemoryForThisTask` or a `releasePendingUnrollMemoryForThisTask` method, the key in the memory map corresponding to that task will never be removed from the hash map. See the details in [SPARK-17465](https://issues.apache.org/jira/browse/SPARK-17465). Author: Xing SHI <shi-kou@indetail.co.jp> Closesapache#15022 from saturday-shi/SPARK-17465. (cherry picked from commit a447cd8)

…che.spark.storage.MemoryStore` may lead to memory leak The expression like `if (memoryMap(taskAttemptId) == 0) memoryMap.remove(taskAttemptId)` in method `releaseUnrollMemoryForThisTask` and `releasePendingUnrollMemoryForThisTask` should be called after release memory operation, whatever `memoryToRelease` is > 0 or not. If the memory of a task has been set to 0 when calling a `releaseUnrollMemoryForThisTask` or a `releasePendingUnrollMemoryForThisTask` method, the key in the memory map corresponding to that task will never be removed from the hash map. See the details in [SPARK-17465](https://issues.apache.org/jira/browse/SPARK-17465). Author: Xing SHI <shi-kou@indetail.co.jp> Closesapache#15022 from saturday-shi/SPARK-17465.

Correct the inappropriate memory management operation in releaseUnrol…
720f2ce
…lMemoryForThisTask and releasePendingUnrollMemoryForThisTask method.

HyukjinKwon mentioned this pull request Sep 9, 2016
Backport [SPARK-5847] Allow for configuring MetricsSystem's prefix #15023
Closed

Add call to releasePendingUnrollMemoryForThisTask at task completion …
6b11fe8
…for more robustness.

JoshRosen reviewed Sep 14, 2016
View reviewed changes

asfgit closed this in bb32294Sep 14, 2016

peter-toth mentioned this pull request Jun 21, 2020
[SPARK-29375][SPARK-28940][SPARK-32041][SQL] Whole plan exchange and subquery reuse #28885
Closed

Uh oh!

Conversation

saturday-shi commented Sep 9, 2016

What changes were proposed in this pull request?

Uh oh!

HyukjinKwon commented Sep 9, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HyukjinKwon commented Sep 9, 2016

Uh oh!

srowen commented Sep 9, 2016

Uh oh!

JoshRosen commented Sep 9, 2016

Uh oh!

JoshRosen commented Sep 9, 2016

Uh oh!

JoshRosen commented Sep 9, 2016

Uh oh!

SparkQA commented Sep 9, 2016

Uh oh!

JoshRosen commented Sep 9, 2016

Uh oh!

JoshRosen commented Sep 9, 2016

Uh oh!

JoshRosen commented Sep 9, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saturday-shi commented Sep 10, 2016

Uh oh!

SparkQA commented Sep 10, 2016

Uh oh!

srowen commented Sep 13, 2016

Uh oh!

SparkQA commented Sep 13, 2016

Uh oh!

JoshRosen commented Sep 14, 2016

Uh oh!

JoshRosenSep 14, 2016

Choose a reason for hiding this comment

Uh oh!

JoshRosen commented Sep 14, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

HyukjinKwon commented Sep 9, 2016•
edited
Loading

JoshRosen commented Sep 9, 2016•
edited
Loading