You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by 10110346 <gi...@git.apache.org> on 2018/02/26 10:00:04 UTC
[GitHub] spark pull request #20676: [SPARK-23516][CORE] It is unnecessary to transfer...
GitHub user 10110346 opened a pull request:
https://github.com/apache/spark/pull/20676
[SPARK-23516][CORE] It is unnecessary to transfer unroll memory to storage memory
## What changes were proposed in this pull request?
In fact, unroll memory is also storage memory,so i think it is unnecessary to release unroll memory really, and then to get storage memory again.
## How was this patch tested?
Existing unit test
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/10110346/spark notreleasemem
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/20676.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #20676
----
commit 0574be52a46ff29253c432f1c71ded041d991351
Author: liuxian <li...@...>
Date: 2018-02-26T09:38:30Z
fix
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20676: [SPARK-23516][CORE] It is unnecessary to transfer unroll...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20676
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/87669/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20676: [SPARK-23516][CORE] It is unnecessary to transfer unroll...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20676
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/87704/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20676: [SPARK-23516][CORE] It is unnecessary to transfer unroll...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20676
**[Test build #87713 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/87713/testReport)** for PR 20676 at commit [`496f2fb`](https://github.com/apache/spark/commit/496f2fb4d154476557c07595cab84d9c0b2299fa).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20676: [SPARK-23516][CORE] It is unnecessary to transfer unroll...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20676
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/1091/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20676: [SPARK-23516][CORE] It is unnecessary to transfer unroll...
Posted by ConeyLiu <gi...@git.apache.org>.
Github user ConeyLiu commented on the issue:
https://github.com/apache/spark/pull/20676
This is for compatibility reasons. The memory management also support legacy memory management (`StaticMemoryManager`). In `StaticMemoryManager`, the storage memory and unroll memory is managed separately. So we can't say
>In fact, unroll memory is also storage memory
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20676: [SPARK-23516][CORE] It is unnecessary to transfer...
Posted by Ngone51 <gi...@git.apache.org>.
Github user Ngone51 commented on a diff in the pull request:
https://github.com/apache/spark/pull/20676#discussion_r170956019
--- Diff: core/src/main/scala/org/apache/spark/storage/memory/MemoryStore.scala ---
@@ -246,18 +246,18 @@ private[spark] class MemoryStore(
val amountToRequest = size - unrollMemoryUsedByThisBlock
keepUnrolling = reserveUnrollMemoryForThisTask(blockId, amountToRequest, memoryMode)
if (keepUnrolling) {
- unrollMemoryUsedByThisBlock += amountToRequest
+ unrollMemoryUsedByThisBlock = size
}
+ } else if (size < unrollMemoryUsedByThisBlock) {
+ releaseUnrollMemoryForThisTask(memoryMode, unrollMemoryUsedByThisBlock - size)
+ unrollMemoryUsedByThisBlock = size
}
if (keepUnrolling) {
val entry = entryBuilder.build()
- // Synchronize so that transfer is atomic
- memoryManager.synchronized {
- releaseUnrollMemoryForThisTask(memoryMode, unrollMemoryUsedByThisBlock)
- val success = memoryManager.acquireStorageMemory(blockId, entry.size, memoryMode)
- assert(success, "transferring unroll memory to storage memory failed")
- }
+ // In fact, unroll memory is also storage memory, it is unnecessary to
+ // release unroll memory really
+ releaseUnrollMemoryForThisTask(memoryMode, unrollMemoryUsedByThisBlock, false)
--- End diff --
Did you check other calls on this function? Are you sure all other calls' third param default to be ```true```, except here only?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20676: [SPARK-23516][CORE] It is unnecessary to transfer unroll...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20676
**[Test build #87705 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/87705/testReport)** for PR 20676 at commit [`496f2fb`](https://github.com/apache/spark/commit/496f2fb4d154476557c07595cab84d9c0b2299fa).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20676: [SPARK-23516][CORE] It is unnecessary to transfer unroll...
Posted by 10110346 <gi...@git.apache.org>.
Github user 10110346 commented on the issue:
https://github.com/apache/spark/pull/20676
retest this please
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20676: [SPARK-23516][CORE] It is unnecessary to transfer unroll...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20676
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/87713/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20676: [SPARK-23516][CORE] It is unnecessary to transfer unroll...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20676
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20676: [SPARK-23516][CORE] It is unnecessary to transfer...
Posted by Ngone51 <gi...@git.apache.org>.
Github user Ngone51 commented on a diff in the pull request:
https://github.com/apache/spark/pull/20676#discussion_r171124468
--- Diff: core/src/main/scala/org/apache/spark/storage/memory/MemoryStore.scala ---
@@ -246,18 +246,18 @@ private[spark] class MemoryStore(
val amountToRequest = size - unrollMemoryUsedByThisBlock
keepUnrolling = reserveUnrollMemoryForThisTask(blockId, amountToRequest, memoryMode)
if (keepUnrolling) {
- unrollMemoryUsedByThisBlock += amountToRequest
+ unrollMemoryUsedByThisBlock = size
}
+ } else if (size < unrollMemoryUsedByThisBlock) {
--- End diff --
Ah, I see, glad to hear that.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20676: [SPARK-23516][CORE] It is unnecessary to transfer unroll...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20676
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20676: [SPARK-23516][CORE] It is unnecessary to transfer unroll...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20676
**[Test build #87685 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/87685/testReport)** for PR 20676 at commit [`0574be5`](https://github.com/apache/spark/commit/0574be52a46ff29253c432f1c71ded041d991351).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20676: [SPARK-23516][CORE] It is unnecessary to transfer unroll...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20676
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20676: [SPARK-23516][CORE] It is unnecessary to transfer unroll...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20676
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20676: [SPARK-23516][CORE] It is unnecessary to transfer unroll...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20676
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/1073/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20676: [SPARK-23516][CORE] It is unnecessary to transfer unroll...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20676
**[Test build #87669 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/87669/testReport)** for PR 20676 at commit [`0574be5`](https://github.com/apache/spark/commit/0574be52a46ff29253c432f1c71ded041d991351).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20676: [SPARK-23516][CORE] It is unnecessary to transfer unroll...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20676
**[Test build #87704 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/87704/testReport)** for PR 20676 at commit [`6e549ee`](https://github.com/apache/spark/commit/6e549eefe6971f3f71f77879e1f6f0c8371f4cec).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20676: [SPARK-23516][CORE] It is unnecessary to transfer unroll...
Posted by 10110346 <gi...@git.apache.org>.
Github user 10110346 commented on the issue:
https://github.com/apache/spark/pull/20676
Thanks all , i will close this PR.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20676: [SPARK-23516][CORE] It is unnecessary to transfer unroll...
Posted by 10110346 <gi...@git.apache.org>.
Github user 10110346 commented on the issue:
https://github.com/apache/spark/pull/20676
retest this please.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20676: [SPARK-23516][CORE] It is unnecessary to transfer unroll...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20676
**[Test build #87705 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/87705/testReport)** for PR 20676 at commit [`496f2fb`](https://github.com/apache/spark/commit/496f2fb4d154476557c07595cab84d9c0b2299fa).
* This patch **fails due to an unknown error code, -9**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20676: [SPARK-23516][CORE] It is unnecessary to transfer unroll...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20676
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20676: [SPARK-23516][CORE] It is unnecessary to transfer unroll...
Posted by 10110346 <gi...@git.apache.org>.
Github user 10110346 commented on the issue:
https://github.com/apache/spark/pull/20676
In `StaticMemoryManager`, the storage memory and unroll memory is managed separately, but, unroll memory is also storage memory, so we do not need release unroll memory really,Just need to release the memory for `unrollMemoryMap` . @ConeyLiu
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20676: [SPARK-23516][CORE] It is unnecessary to transfer unroll...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20676
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/1098/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20676: [SPARK-23516][CORE] It is unnecessary to transfer unroll...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20676
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/87685/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20676: [SPARK-23516][CORE] It is unnecessary to transfer...
Posted by 10110346 <gi...@git.apache.org>.
Github user 10110346 commented on a diff in the pull request:
https://github.com/apache/spark/pull/20676#discussion_r171115245
--- Diff: core/src/main/scala/org/apache/spark/storage/memory/MemoryStore.scala ---
@@ -246,18 +246,18 @@ private[spark] class MemoryStore(
val amountToRequest = size - unrollMemoryUsedByThisBlock
keepUnrolling = reserveUnrollMemoryForThisTask(blockId, amountToRequest, memoryMode)
if (keepUnrolling) {
- unrollMemoryUsedByThisBlock += amountToRequest
+ unrollMemoryUsedByThisBlock = size
}
+ } else if (size < unrollMemoryUsedByThisBlock) {
--- End diff --
There is no problem before,because it releases all unroll memory in 257 line
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20676: [SPARK-23516][CORE] It is unnecessary to transfer unroll...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20676
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20676: [SPARK-23516][CORE] It is unnecessary to transfer unroll...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20676
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/1056/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20676: [SPARK-23516][CORE] It is unnecessary to transfer unroll...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20676
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20676: [SPARK-23516][CORE] It is unnecessary to transfer...
Posted by Ngone51 <gi...@git.apache.org>.
Github user Ngone51 commented on a diff in the pull request:
https://github.com/apache/spark/pull/20676#discussion_r171128021
--- Diff: core/src/main/scala/org/apache/spark/storage/memory/MemoryStore.scala ---
@@ -246,18 +246,18 @@ private[spark] class MemoryStore(
val amountToRequest = size - unrollMemoryUsedByThisBlock
keepUnrolling = reserveUnrollMemoryForThisTask(blockId, amountToRequest, memoryMode)
if (keepUnrolling) {
- unrollMemoryUsedByThisBlock += amountToRequest
+ unrollMemoryUsedByThisBlock = size
}
+ } else if (size < unrollMemoryUsedByThisBlock) {
+ releaseUnrollMemoryForThisTask(memoryMode, unrollMemoryUsedByThisBlock - size)
+ unrollMemoryUsedByThisBlock = size
}
if (keepUnrolling) {
val entry = entryBuilder.build()
- // Synchronize so that transfer is atomic
- memoryManager.synchronized {
- releaseUnrollMemoryForThisTask(memoryMode, unrollMemoryUsedByThisBlock)
- val success = memoryManager.acquireStorageMemory(blockId, entry.size, memoryMode)
- assert(success, "transferring unroll memory to storage memory failed")
- }
+ // In fact, unroll memory is also storage memory, it is unnecessary to
+ // release unroll memory really
+ releaseUnrollMemoryForThisTask(memoryMode, unrollMemoryUsedByThisBlock, false)
--- End diff --
Well, it seems a little awkward. How about change ```transferUnrollToStorage()```'s(which is removed after #19285, [see L242](https://github.com/apache/spark/blame/c22eaa94e85aaac649566495dcf763a5de3c8d06/core/src/main/scala/org/apache/spark/storage/memory/MemoryStore.scala#L242) ) behavior , rather than ```releaseUnrollMemoryForThisTask ```?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20676: [SPARK-23516][CORE] It is unnecessary to transfer unroll...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20676
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/87705/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20676: [SPARK-23516][CORE] It is unnecessary to transfer unroll...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/20676
I don't think unroll memory is storage memory from the interface. It's only true for unified memory manager. I'm -1 on this change unless you can convince the community to remove the static memory manager entirely.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20676: [SPARK-23516][CORE] It is unnecessary to transfer unroll...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20676
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20676: [SPARK-23516][CORE] It is unnecessary to transfer unroll...
Posted by Ngone51 <gi...@git.apache.org>.
Github user Ngone51 commented on the issue:
https://github.com/apache/spark/pull/20676
Hi, @ConeyLiu. I don't think it's about compatibility. Because both ```StaticMemoryManager``` and ```UnifiedMemoryManager``` call the same function for release unroll memory, which is ```releaseUnrollMemory()```, see[https://github.com/apache/spark/blob/b14993e1fcb68e1c946a671c6048605ab4afdf58/core/src/main/scala/org/apache/spark/memory/MemoryManager.scala#L165](url)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20676: [SPARK-23516][CORE] It is unnecessary to transfer...
Posted by 10110346 <gi...@git.apache.org>.
Github user 10110346 closed the pull request at:
https://github.com/apache/spark/pull/20676
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20676: [SPARK-23516][CORE] It is unnecessary to transfer unroll...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20676
**[Test build #87713 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/87713/testReport)** for PR 20676 at commit [`496f2fb`](https://github.com/apache/spark/commit/496f2fb4d154476557c07595cab84d9c0b2299fa).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20676: [SPARK-23516][CORE] It is unnecessary to transfer unroll...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20676
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/1090/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20676: [SPARK-23516][CORE] It is unnecessary to transfer...
Posted by Ngone51 <gi...@git.apache.org>.
Github user Ngone51 commented on a diff in the pull request:
https://github.com/apache/spark/pull/20676#discussion_r170961404
--- Diff: core/src/main/scala/org/apache/spark/storage/memory/MemoryStore.scala ---
@@ -246,18 +246,18 @@ private[spark] class MemoryStore(
val amountToRequest = size - unrollMemoryUsedByThisBlock
keepUnrolling = reserveUnrollMemoryForThisTask(blockId, amountToRequest, memoryMode)
if (keepUnrolling) {
- unrollMemoryUsedByThisBlock += amountToRequest
+ unrollMemoryUsedByThisBlock = size
}
+ } else if (size < unrollMemoryUsedByThisBlock) {
+ releaseUnrollMemoryForThisTask(memoryMode, unrollMemoryUsedByThisBlock - size)
+ unrollMemoryUsedByThisBlock = size
}
if (keepUnrolling) {
val entry = entryBuilder.build()
- // Synchronize so that transfer is atomic
- memoryManager.synchronized {
- releaseUnrollMemoryForThisTask(memoryMode, unrollMemoryUsedByThisBlock)
- val success = memoryManager.acquireStorageMemory(blockId, entry.size, memoryMode)
- assert(success, "transferring unroll memory to storage memory failed")
- }
--- End diff --
It's a concept difference between unroll memory and storage memory. And it seems reasonable to transfer unroll memory to storage memory from the view of their concept, though, it makes no big difference underlying storage memory. I'm not sure will this change cause some side effect, if none, I'll approve of it.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20676: [SPARK-23516][CORE] It is unnecessary to transfer unroll...
Posted by ConeyLiu <gi...@git.apache.org>.
Github user ConeyLiu commented on the issue:
https://github.com/apache/spark/pull/20676
Yeah, I see that. I'm not sure it's OK to change. But I think we should follow the interface design, not the underlying implementation.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20676: [SPARK-23516][CORE] It is unnecessary to transfer...
Posted by ConeyLiu <gi...@git.apache.org>.
Github user ConeyLiu commented on a diff in the pull request:
https://github.com/apache/spark/pull/20676#discussion_r171115071
--- Diff: core/src/main/scala/org/apache/spark/storage/memory/MemoryStore.scala ---
@@ -246,18 +246,18 @@ private[spark] class MemoryStore(
val amountToRequest = size - unrollMemoryUsedByThisBlock
keepUnrolling = reserveUnrollMemoryForThisTask(blockId, amountToRequest, memoryMode)
if (keepUnrolling) {
- unrollMemoryUsedByThisBlock += amountToRequest
+ unrollMemoryUsedByThisBlock = size
}
+ } else if (size < unrollMemoryUsedByThisBlock) {
--- End diff --
In #19285, we first release `unrollMemoryUsedByThisBlock` unroll memory, and then we request `entry.size` storage memory. So, there is no waste of resources here.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20676: [SPARK-23516][CORE] It is unnecessary to transfer unroll...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20676
**[Test build #87704 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/87704/testReport)** for PR 20676 at commit [`6e549ee`](https://github.com/apache/spark/commit/6e549eefe6971f3f71f77879e1f6f0c8371f4cec).
* This patch **fails due to an unknown error code, -9**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20676: [SPARK-23516][CORE] It is unnecessary to transfer...
Posted by Ngone51 <gi...@git.apache.org>.
Github user Ngone51 commented on a diff in the pull request:
https://github.com/apache/spark/pull/20676#discussion_r170954231
--- Diff: core/src/main/scala/org/apache/spark/storage/memory/MemoryStore.scala ---
@@ -246,18 +246,18 @@ private[spark] class MemoryStore(
val amountToRequest = size - unrollMemoryUsedByThisBlock
keepUnrolling = reserveUnrollMemoryForThisTask(blockId, amountToRequest, memoryMode)
if (keepUnrolling) {
- unrollMemoryUsedByThisBlock += amountToRequest
+ unrollMemoryUsedByThisBlock = size
}
+ } else if (size < unrollMemoryUsedByThisBlock) {
--- End diff --
#19285 miss this check before? Oops! This could waste storage memory. Thanks for adding this.
Also, cc @cloud-fan @ConeyLiu
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20676: [SPARK-23516][CORE] It is unnecessary to transfer unroll...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20676
**[Test build #87685 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/87685/testReport)** for PR 20676 at commit [`0574be5`](https://github.com/apache/spark/commit/0574be52a46ff29253c432f1c71ded041d991351).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20676: [SPARK-23516][CORE] It is unnecessary to transfer...
Posted by 10110346 <gi...@git.apache.org>.
Github user 10110346 commented on a diff in the pull request:
https://github.com/apache/spark/pull/20676#discussion_r171116974
--- Diff: core/src/main/scala/org/apache/spark/storage/memory/MemoryStore.scala ---
@@ -246,18 +246,18 @@ private[spark] class MemoryStore(
val amountToRequest = size - unrollMemoryUsedByThisBlock
keepUnrolling = reserveUnrollMemoryForThisTask(blockId, amountToRequest, memoryMode)
if (keepUnrolling) {
- unrollMemoryUsedByThisBlock += amountToRequest
+ unrollMemoryUsedByThisBlock = size
}
+ } else if (size < unrollMemoryUsedByThisBlock) {
+ releaseUnrollMemoryForThisTask(memoryMode, unrollMemoryUsedByThisBlock - size)
+ unrollMemoryUsedByThisBlock = size
}
if (keepUnrolling) {
val entry = entryBuilder.build()
- // Synchronize so that transfer is atomic
- memoryManager.synchronized {
- releaseUnrollMemoryForThisTask(memoryMode, unrollMemoryUsedByThisBlock)
- val success = memoryManager.acquireStorageMemory(blockId, entry.size, memoryMode)
- assert(success, "transferring unroll memory to storage memory failed")
- }
+ // In fact, unroll memory is also storage memory, it is unnecessary to
+ // release unroll memory really
+ releaseUnrollMemoryForThisTask(memoryMode, unrollMemoryUsedByThisBlock, false)
--- End diff --
t am sure all other calls' third param default to be true
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20676: [SPARK-23516][CORE] It is unnecessary to transfer unroll...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20676
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20676: [SPARK-23516][CORE] It is unnecessary to transfer unroll...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20676
**[Test build #87669 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/87669/testReport)** for PR 20676 at commit [`0574be5`](https://github.com/apache/spark/commit/0574be52a46ff29253c432f1c71ded041d991351).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org