[SPARK-13695] Don't cache MEMORY_AND_DISK blocks as bytes in memory after spills #11533
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/cc @andrewor14 @nongli for review |
|
Test build #52505 has finished for PR 11533 at commit
|
| @@ -477,7 +477,7 @@ private[spark] class BlockManager( | |||
| } | |||
| } else { | |||
| // Otherwise, we also have to store something in the memory store | |||
| if (!level.deserialized || !asBlockResult) { | |||
| if (!level.deserialized && !asBlockResult) { | |||
There was a problem hiding this comment.
Is the below comment stale now?
There was a problem hiding this comment.
Yeah, I'll fix it.
There was a problem hiding this comment.
I neglected to do this earlier because this entire block of code is being completely rewritten in another one of my PRs.
|
LGTM |
|
Test build #52592 has finished for PR 11533 at commit
|
|
Test build #2615 has finished for PR 11533 at commit
|
|
LGTM2. There are conflicts now though. |
|
Conflicts fixed. |
|
Test build #52636 has finished for PR 11533 at commit
|
|
Now that the conflicts are fixed, I'm going to merge this now and will rebase my other patch. |
roygao94
pushed a commit
to roygao94/spark
that referenced
this pull request
Mar 22, 2016
…fter spills When a cached block is spilled to disk and read back in serialized form (i.e. as bytes), the current BlockManager implementation will attempt to re-insert the serialized block into the MemoryStore even if the block's storage level requests deserialized caching. This behavior adds some complexity to the MemoryStore but I don't think it offers many performance benefits and I'd like to remove it in order to simplify a larger refactoring patch. Therefore, this patch changes the behavior so that disk store reads will only cache bytes in the memory store for blocks with serialized storage levels. There are two places where we request serialized bytes from the BlockStore: 1. getLocalBytes(), which is only called when reading local copies of TorrentBroadcast pieces. Broadcast pieces are always cached using a serialized storage level, so this won't lead to a mismatch in serialization forms if spilled bytes read from disk are cached as bytes in the memory store. 2. the non-shuffle-block branch in getBlockData(), which is only called by the NettyBlockRpcServer when responding to requests to read remote blocks. Caching the serialized bytes in memory will only benefit us if those cached bytes are read before they're evicted and the likelihood of that happening seems low since the frequency of remote reads of non-broadcast cached blocks seems very low. Caching these bytes when they have a low probability of being read is bad if it risks the eviction of blocks which are cached in their expected serialized/deserialized forms, since those blocks seem more likely to be read in local computation. Given the argument above, I think this change is unlikely to cause performance regressions. Author: Josh Rosen <joshrosen@databricks.com> Closes apache#11533 from JoshRosen/remove-memorystore-level-mismatch.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When a cached block is spilled to disk and read back in serialized form (i.e. as bytes), the current BlockManager implementation will attempt to re-insert the serialized block into the MemoryStore even if the block's storage level requests deserialized caching.
This behavior adds some complexity to the MemoryStore but I don't think it offers many performance benefits and I'd like to remove it in order to simplify a larger refactoring patch. Therefore, this patch changes the behavior so that disk store reads will only cache bytes in the memory store for blocks with serialized storage levels.
There are two places where we request serialized bytes from the BlockStore:
Given the argument above, I think this change is unlikely to cause performance regressions.