Clean up CacheManager et al. #1083

andrewor14 · 2014-06-14T04:12:04Z

UPDATE

I have removed the special handling for StorageLevel.MEMORY_*_SER for now, because it introduces a potential performance regression. With the latest changes, this PR should include mainly style (code readability) fixes. The only functionality change is the update in MemoryStore#putBytes to actually return updated blocks, though this is a minor bug fix.

Now this is mainly a precursor to another PR (once again).

Old comment

The deserialized version of a partition may occupy much more space than the serialized version. Therefore, if a partition is to be cached with StorageLevel.MEMORY_*_SER, we don't need to fully unroll it into an ArrayBuffer, but instead we can unroll it into a potentially much smaller ByteBuffer. This may save us from OOMs in this case.

We only unroll the serialized form of each partition for this case, because the deserialized form may be much larger and may not fit in memory. This commit also abstracts out part of the logic of getOrCompute to make it more readable.

Previously we never returned the updated blocks in MemoryStore's putBytes. This is a simple bug with a simple fix.

AmplabJenkins · 2014-06-14T04:14:35Z

Merged build triggered.

AmplabJenkins · 2014-06-14T04:14:43Z

Merged build started.

AmplabJenkins · 2014-06-14T04:58:34Z

Merged build finished. All automated tests passed.

AmplabJenkins · 2014-06-14T04:58:34Z

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/15783/

rxin · 2014-06-14T06:44:37Z

core/src/main/scala/org/apache/spark/CacheManager.scala

Might make sense to remove this assume; in case we add a new storage level in the future, this won't hold any more and because this code is so far away from the storage level code, we will likely forget to update this location.

rxin · 2014-06-14T07:03:25Z

BTW can you construct a unit test for this in CacheManagerSuite?

Would be good also to add a unit test to test the lock (which existed earlier but had no test for it). Thanks.

AmplabJenkins · 2014-06-16T19:24:46Z

Merged build triggered.

AmplabJenkins · 2014-06-16T19:24:52Z

Merged build started.

AmplabJenkins · 2014-06-16T20:08:19Z

Merged build finished. All automated tests passed.

AmplabJenkins · 2014-06-16T20:08:19Z

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/15822/

andrewor14 · 2014-06-16T20:50:49Z

It's worth noting that the special handling of the memory serialized storage level actually introduces a regression. In particular, now we add an extra step to deserialize the bytes in the end, which could be slow for large partitions.

This special handling will most likely be superseded by a more general solution for SPARK-1777, which avoids unrolling an entire partition if there is not enough space for it, regardless of the storage level. For now, I will put this PR on hold.

This special handling sacrifices CPU cycles for memory usage by introducing an additional step to deserialize the serialized bytes put into BlockManager. This may cause a performance regression in some cases. For now, let's keep the functionality the same as before, and only include style changes in this PR. This is a precursor to another incoming PR that changes the way unroll RDD partitions.

…titions

AmplabJenkins · 2014-06-20T01:09:57Z

Merged build triggered.

AmplabJenkins · 2014-06-20T01:10:03Z

Merged build started.

andrewor14 · 2014-06-20T01:10:23Z

I have removed the special handling of the memory serialized case for the aforementioned reason. However, I would like get the style changes in master as I have a separate WIP PR that builds on top of this one.

AmplabJenkins · 2014-06-20T01:19:57Z

Merged build triggered.

AmplabJenkins · 2014-06-20T01:20:03Z

Merged build started.

AmplabJenkins · 2014-06-20T02:04:19Z

Merged build finished. All automated tests passed.

AmplabJenkins · 2014-06-20T02:04:19Z

Merged build finished. All automated tests passed.

AmplabJenkins · 2014-06-20T02:04:19Z

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/15933/

AmplabJenkins · 2014-06-20T02:04:19Z

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/15935/

…titions

AmplabJenkins · 2014-06-20T23:05:02Z

Merged build triggered.

AmplabJenkins · 2014-06-20T23:05:11Z

Merged build started.

rxin · 2014-06-20T23:05:27Z

LGTM.

AmplabJenkins · 2014-06-21T00:04:29Z

Merged build finished. All automated tests passed.

AmplabJenkins · 2014-06-21T00:04:29Z

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/15972/

rxin · 2014-06-21T00:14:25Z

Merged in master. Thanks!

**UPDATE** I have removed the special handling for `StorageLevel.MEMORY_*_SER` for now, because it introduces a potential performance regression. With the latest changes, this PR should include mainly style (code readability) fixes. The only functionality change is the update in `MemoryStore#putBytes` to actually return updated blocks, though this is a minor bug fix. Now this is mainly a precursor to another PR (once again). --------- *Old comment* The deserialized version of a partition may occupy much more space than the serialized version. Therefore, if a partition is to be cached with `StorageLevel.MEMORY_*_SER`, we don't need to fully unroll it into an `ArrayBuffer`, but instead we can unroll it into a potentially much smaller `ByteBuffer`. This may save us from OOMs in this case. Author: Andrew Or <[email protected]> Closes apache#1083 from andrewor14/unroll-them-partitions and squashes the following commits: 7048aa0 [Andrew Or] Merge branch 'master' of github.com:apache/spark into unroll-them-partitions 3d9a366 [Andrew Or] Minor change for readability d12b95f [Andrew Or] Remove unused imports (minor) a4c387b [Andrew Or] Merge branch 'master' of github.com:apache/spark into unroll-them-partitions cf5f565 [Andrew Or] Remove special handling for MEM_*_SER 0091ec0 [Andrew Or] Address review feedback 44ef282 [Andrew Or] Actually return updated blocks in putBytes 2941c89 [Andrew Or] Clean up BlockStore (minor) a8f181d [Andrew Or] Add special handling for StorageLevel.MEMORY_*_SER

…vent log files (#1083)

andrewor14 added 3 commits June 13, 2014 20:49

Add special handling for StorageLevel.MEMORY_*_SER

a8f181d

We only unroll the serialized form of each partition for this case, because the deserialized form may be much larger and may not fit in memory. This commit also abstracts out part of the logic of getOrCompute to make it more readable.

Clean up BlockStore (minor)

2941c89

Actually return updated blocks in putBytes

44ef282

Previously we never returned the updated blocks in MemoryStore's putBytes. This is a simple bug with a simple fix.

andrewor14 changed the title ~~[SPARK-1201] Do not fully materialize partitions for StorageLevel.MEMORY_*_SER~~ [SPARK-1201] Do not fully materialize partitions for MEMORY_SER Jun 14, 2014

andrewor14 changed the title ~~[SPARK-1201] Do not fully materialize partitions for MEMORY_SER~~ [SPARK-1201] Do not fully materialize partitions for MEMORY_*_SER Jun 14, 2014

rxin reviewed Jun 14, 2014
View reviewed changes

Address review feedback

0091ec0

andrewor14 added 2 commits June 19, 2014 18:03

Merge branch 'master' of github.com:apache/spark into unroll-them-par…

a4c387b

…titions

andrewor14 changed the title ~~[SPARK-1201] Do not fully materialize partitions for MEMORY_*_SER~~ [Minor] Clean up CacheManager, BlockStore, and MemoryStore Jun 20, 2014

andrewor14 changed the title ~~[Minor] Clean up CacheManager, BlockStore, and MemoryStore~~ Clean up CacheManager, BlockStore, and MemoryStore Jun 20, 2014

andrewor14 changed the title ~~Clean up CacheManager, BlockStore, and MemoryStore~~ [Minor] Clean up CacheManager, BlockStore, and MemoryStore Jun 20, 2014

andrewor14 changed the title ~~[Minor] Clean up CacheManager, BlockStore, and MemoryStore~~ [Minor] Clean up CacheManager et al. Jun 20, 2014

Remove unused imports (minor)

d12b95f

andrewor14 changed the title ~~[Minor] Clean up CacheManager et al.~~ Clean up CacheManager et al. Jun 20, 2014

andrewor14 added 2 commits June 20, 2014 16:03

Minor change for readability

3d9a366

Merge branch 'master' of github.com:apache/spark into unroll-them-par…

7048aa0

…titions

asfgit closed this in 01125a1 Jun 21, 2014

andrewor14 deleted the unroll-them-partitions branch June 21, 2014 01:18

andrewor14 mentioned this pull request Jul 22, 2014

[SPARK-1777] Prevent OOMs from single partitions #1165

Closed

wangyum pushed a commit that referenced this pull request May 26, 2023

[CARMEL-6152] [Followup3] Handle FileNotFoundException when listing e…

5ee3529

…vent log files (#1083)

Clean up CacheManager et al. #1083

Clean up CacheManager et al. #1083

Uh oh!

Conversation

andrewor14 commented Jun 14, 2014

Uh oh!

AmplabJenkins commented Jun 14, 2014

Uh oh!

AmplabJenkins commented Jun 14, 2014

Uh oh!

AmplabJenkins commented Jun 14, 2014

Uh oh!

AmplabJenkins commented Jun 14, 2014

Uh oh!

rxin Jun 14, 2014

Choose a reason for hiding this comment

Uh oh!

rxin commented Jun 14, 2014

Uh oh!

AmplabJenkins commented Jun 16, 2014

Uh oh!

AmplabJenkins commented Jun 16, 2014

Uh oh!

AmplabJenkins commented Jun 16, 2014

Uh oh!

AmplabJenkins commented Jun 16, 2014

Uh oh!

andrewor14 commented Jun 16, 2014

Uh oh!

AmplabJenkins commented Jun 20, 2014

Uh oh!

AmplabJenkins commented Jun 20, 2014

Uh oh!

andrewor14 commented Jun 20, 2014

Uh oh!

AmplabJenkins commented Jun 20, 2014

Uh oh!

AmplabJenkins commented Jun 20, 2014

Uh oh!

AmplabJenkins commented Jun 20, 2014

Uh oh!

AmplabJenkins commented Jun 20, 2014

Uh oh!

AmplabJenkins commented Jun 20, 2014

Uh oh!

AmplabJenkins commented Jun 20, 2014

Uh oh!

AmplabJenkins commented Jun 20, 2014

Uh oh!

AmplabJenkins commented Jun 20, 2014

Uh oh!

rxin commented Jun 20, 2014

Uh oh!

AmplabJenkins commented Jun 21, 2014

Uh oh!

AmplabJenkins commented Jun 21, 2014

Uh oh!

rxin commented Jun 21, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants