Github user mallman commented on a diff in the pull request:
https://github.com/apache/spark/pull/16499#discussion_r101592331
--- Diff: core/src/main/scala/org/apache/spark/storage/BlockManager.scala
---
@@ -1018,7 +1025,9 @@ private[spark] class BlockManager(
try {
replicate(blockId, bytesToReplicate, level, remoteClassTag)
} finally {
- bytesToReplicate.dispose()
+ if (!level.useOffHeap) {
--- End diff --
I think the name of the `ChunkedByteBuffer.dispose()` method is confusing.
It actually only attempts to dispose a so-called "memory mapped" buffer.
On-heap buffers are not memory mapped, therefore this is a no-op for them.
On the other hand, when the storage level uses off-heap memory in this
context, `bytesToReplicate` is a reference to the actual off-heap memory
buffer. Disposing of this buffer will erase it from the local memory store.
Obviously, this is not the desired behavior. So we add the guard for off-heap
memory buffers here.
As far as I can tell, there is no storage level for which
`bytesToReplicate.dispose()` would actually do anything. However, technically
if `bytesToReplicate` where memory-mapped but not direct, this would dispose of
that memory. Would we even want that behavior? Overall, this `finally` clause
is attempting to destroy the data we get from `doGetLocalBytes()`. This does
not seem to be safe or correct, because we do not want to destroy the data
stored locally. Therefore, we should consider getting rid of this finally
clause entirely. What do you think?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
