[SPARK-21090][core]Optimize the unified memory manager code #18296
Conversation
|
Test build #78030 has finished for PR 18296 at commit
|
|
Test build #78032 has started for PR 18296 at commit |
|
retest this please |
|
@srowen I have updated the description of this PR,thanks |
|
I don't quite understand how that addresses the question, but I don't know this code. @JoshRosen is the most likely reviewer, but maybe you can elaborate the example more. |
|
Test build #78037 has finished for PR 18296 at commit
|
|
@JoshRosen Could you help to review it, thanks |
| @@ -160,7 +160,7 @@ private[spark] class UnifiedMemoryManager private[memory] ( | |||
| case MemoryMode.OFF_HEAP => ( | |||
| offHeapExecutionMemoryPool, | |||
| offHeapStorageMemoryPool, | |||
| maxOffHeapMemory) | |||
| maxOffHeapStorageMemory) | |||
There was a problem hiding this comment.
This looks like a bug in the old code. Good catch.
| @@ -171,7 +171,8 @@ private[spark] class UnifiedMemoryManager private[memory] ( | |||
| if (numBytes > storagePool.memoryFree) { | |||
| // There is not enough free memory in the storage pool, so try to borrow free memory from | |||
| // the execution pool. | |||
| val memoryBorrowedFromExecution = Math.min(executionPool.memoryFree, numBytes) | |||
| val memoryBorrowedFromExecution = Math.min(executionPool.memoryFree, | |||
| numBytes - storagePool.memoryFree) | |||
There was a problem hiding this comment.
This change makes sense: if we hit this branch then we've failed to acquire enough memory to store a block. In particular, we might need M bytes of memory but only A are available in storage. In this case, we should only request the (M - A) bytes of extra memory that we need, but the old code was incorrectly requesting all M which would cause us to over-evict. Therefore, I think this change is correct.
|
Also cc @cloud-fan @jiangxb1987 |
|
LGTM, merging to master! |
## What changes were proposed in this pull request? 1.In `acquireStorageMemory`, when the Memory Mode is OFF_HEAP ,the `maxOffHeapMemory` should be modified to `maxOffHeapStorageMemory`. after this PR,it will same as ON_HEAP Memory Mode. Because when acquire memory is between `maxOffHeapStorageMemory` and `maxOffHeapMemory`,it will fail surely, so if acquire memory is greater than `maxOffHeapStorageMemory`(not greater than `maxOffHeapMemory`),we should fail fast. 2. Borrow memory from execution, `numBytes` modified to `numBytes - storagePool.memoryFree` will be more reasonable. Because we just acquire `(numBytes - storagePool.memoryFree)`, unnecessary borrowed `numBytes` from execution ## How was this patch tested? added unit test case Author: liuxian <[email protected]> Closes #18296 from 10110346/wip-lx-0614. (cherry picked from commit 112bd9b) Signed-off-by: Wenchen Fan <[email protected]>
|
Actually this is a bug fix, and it's a small fix(without tests, only 3 lines), so backporting in to 2.2 |
What changes were proposed in this pull request?
1.In
acquireStorageMemory, when the Memory Mode is OFF_HEAP ,themaxOffHeapMemoryshould be modified tomaxOffHeapStorageMemory. after this PR,it will same as ON_HEAP Memory Mode.Because when acquire memory is between
maxOffHeapStorageMemoryandmaxOffHeapMemory,it will fail surely, so if acquire memory is greater thanmaxOffHeapStorageMemory(not greater thanmaxOffHeapMemory),we should fail fast.2. Borrow memory from execution,
numBytesmodified tonumBytes - storagePool.memoryFreewill be more reasonable.Because we just acquire
(numBytes - storagePool.memoryFree), unnecessary borrowednumBytesfrom executionHow was this patch tested?
added unit test case