[SPARK-23516][CORE] It is unnecessary to transfer unroll memory to storage memory #20676
Conversation
|
Test build #87669 has finished for PR 20676 at commit
|
|
Test build #87685 has finished for PR 20676 at commit
|
10110346
force-pushed
the
notreleasemem
branch
from
0574be5 to
6e549ee
Compare
10110346
force-pushed
the
notreleasemem
branch
from
6e549ee to
496f2fb
Compare
|
Test build #87704 has finished for PR 20676 at commit
|
|
Test build #87705 has finished for PR 20676 at commit
|
|
retest this please. |
|
Test build #87713 has finished for PR 20676 at commit
|
| unrollMemoryUsedByThisBlock += amountToRequest | ||
| unrollMemoryUsedByThisBlock = size | ||
| } | ||
| } else if (size < unrollMemoryUsedByThisBlock) { |
There was a problem hiding this comment.
#19285 miss this check before? Oops! This could waste storage memory. Thanks for adding this.
Also, cc @cloud-fan @ConeyLiu
There was a problem hiding this comment.
In #19285, we first release unrollMemoryUsedByThisBlock unroll memory, and then we request entry.size storage memory. So, there is no waste of resources here.
There was a problem hiding this comment.
There is no problem before,because it releases all unroll memory in 257 line
| } | ||
| // In fact, unroll memory is also storage memory, it is unnecessary to | ||
| // release unroll memory really | ||
| releaseUnrollMemoryForThisTask(memoryMode, unrollMemoryUsedByThisBlock, false) |
There was a problem hiding this comment.
Did you check other calls on this function? Are you sure all other calls' third param default to be true, except here only?
There was a problem hiding this comment.
I am sure all other calls' third param default to be true
| releaseUnrollMemoryForThisTask(memoryMode, unrollMemoryUsedByThisBlock) | ||
| val success = memoryManager.acquireStorageMemory(blockId, entry.size, memoryMode) | ||
| assert(success, "transferring unroll memory to storage memory failed") | ||
| } |
There was a problem hiding this comment.
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.
|
This is for compatibility reasons. The memory management also support legacy memory management (
|
|
In |
|
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. |
|
Hi, @ConeyLiu. I don't think it's about compatibility. Because both |
|
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, or slightly update the memory manger interface to clarify the semantic of unroll memory. |
|
Thanks all , i will close this PR. |
5 participants
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