Ngone51 commented on a change in pull request #23426: [SPARK-26527][CORE] Let
acquireUnrollMemory fail fast if required space exceeds memory limit
URL: https://github.com/apache/spark/pull/23426#discussion_r245238037
##########
File path:
core/src/main/scala/org/apache/spark/memory/StaticMemoryManager.scala
##########
@@ -79,16 +79,23 @@ private[spark] class StaticMemoryManager(
memoryMode: MemoryMode): Boolean = synchronized {
require(memoryMode != MemoryMode.OFF_HEAP,
"StaticMemoryManager does not support off-heap unroll memory")
- val currentUnrollMemory =
onHeapStorageMemoryPool.memoryStore.currentUnrollMemory
- val freeMemory = onHeapStorageMemoryPool.memoryFree
- // When unrolling, we will use all of the existing free memory, and, if
necessary,
- // some extra space freed from evicting cached blocks. We must place a cap
on the
- // amount of memory to be evicted by unrolling, however, otherwise
unrolling one
- // big block can blow away the entire cache.
- val maxNumBytesToFree = math.max(0, maxUnrollMemory - currentUnrollMemory
- freeMemory)
- // Keep it within the range 0 <= X <= maxNumBytesToFree
- val numBytesToFree = math.max(0, math.min(maxNumBytesToFree, numBytes -
freeMemory))
- onHeapStorageMemoryPool.acquireMemory(blockId, numBytes, numBytesToFree)
+ if (numBytes > maxOnHeapStorageMemory) {
Review comment:
I'm wondering that does this check really do a good favour to reduce "some
computation and memory evicting costs" ?
Codes hit this check may have two possible cases(which I can imagine so far):
* a big `initialMemoryThreshold`, which is even closer to
`maxOnHeapStorageMemory` (rarely)
* a normal `initialMemoryThreshold` (default 1m), which is far less than
`maxOnHeapStorageMemory`.
In this case, codes hit this check must have experienced a long way of
unroll, so, it is highly possible that many block evictions have already
happened during the proccess. So, it may be not a big difference if we do
another block eviction here, though, we'll fail to store the block in the end.
The check may be useful if we could move it to `putIterator()` in
`MemoryStore` before we do unroll for the block, but it seems difficult since
we haven't know the total size of the block at that time. Another thought,
here, is if we can find a way to measure the cost for continue unroll memory
for this block considering the side effect casued by block evictions(which
seems more complex to achieve), then we could decide to continue unroll or not
accroding to the cost.
BTW, since we're at this point, do we also need to add a similar check for
`UnifiedMemoryManager` ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
