[SPARK-22097][CORE]Request an accurate memory after we unrolled the block #19316
Conversation
| reserveAdditionalMemoryIfNecessary() | ||
| serializationStream.flush() | ||
| if (bbos.size > unrollMemoryUsedByThisBlock) { | ||
| val amountToRequest = bbos.size - unrollMemoryUsedByThisBlock |
There was a problem hiding this comment.
Here, we only need request the bbos.size - unrollMemoryUsedByThisBlock. I'm sorry for that. This mistake maybe introduced by my previous patch SPARK-21923.
|
@cloud-fan Pls take a look. Thanks a lot. |
ConeyLiu
changed the title
|
Hi @cloud-fan @jiangxb1987 Do you have time to check this? Thanks a lot. |
|
ok to test |
| // Make sure that we have enough memory to store the block. By this point, it is possible that | ||
| // the block's actual memory usage has exceeded the unroll memory by a small amount, so we | ||
| // perform one final call to attempt to allocate additional memory if necessary. | ||
| if (keepUnrolling) { |
There was a problem hiding this comment.
shall we merge this if into the next if?
There was a problem hiding this comment.
We maybe need request the extra-memory (bbos.size - unrollMemoryUsedByThisBlock). So the keepUnrolling maybe change in line 393.
|
Test build #82616 has finished for PR 19316 at commit
|
| serializationStream.close() | ||
| reserveAdditionalMemoryIfNecessary() | ||
| if (bbos.size > unrollMemoryUsedByThisBlock) { | ||
| val amountToRequest = bbos.size - unrollMemoryUsedByThisBlock |
There was a problem hiding this comment.
Why not just fix that in reserveAdditionalMemoryIfNecessary ?
There was a problem hiding this comment.
Because we request val amountToRequest = (bbos.size * memoryGrowthFactor - unrollMemoryUsedByThisBlock).toLong in reserveAdditionalMemoryIfNecessary to avoid requesting for every records. But here we just need request the precise memory for the last request.
|
LGTM, merging to master! |
4 participants
What changes were proposed in this pull request?
We only need request
bbos.size - unrollMemoryUsedByThisBlockafter unrolled the block.How was this patch tested?
Existing UT.