Skip to content

Conversation

@JoshRosen
Copy link
Contributor

This patch refactors the MemoryManager class structure. After #9000, Spark had the following classes:

  • MemoryManager
  • StaticMemoryManager
  • ExecutorMemoryManager
  • TaskMemoryManager
  • ShuffleMemoryManager

This is fairly confusing. To simplify things, this patch consolidates several of these classes:

  • ShuffleMemoryManager and ExecutorMemoryManager were merged into MemoryManager.
  • TaskMemoryManager is moved into Spark Core.

Key changes and tasks:

  • Merge ExecutorMemoryManager into MemoryManager.
    • Move pooling logic into Allocator.
  • Move TaskMemoryManager from spark-unsafe to spark-core.
  • Refactor the existing Tungsten TaskMemoryManager interactions so Tungsten code use only this and not both this and ShuffleMemoryManager.
  • Refactor non-Tungsten code to use the TaskMemoryManager instead of ShuffleMemoryManager.
  • Merge ShuffleMemoryManager into MemoryManager.
    • Move code
    • Simplify 1/n calculation. Will defer to followup, since this needs more work.
  • Port ShuffleMemoryManagerSuite tests.
  • Move classes from unsafe package to memory package.
  • Figure out how to handle the hacky use of the memory managers in HashedRelation's broadcast variable construction.
  • Test porting and cleanup: several tests relied on mock functionality (such as TestShuffleMemoryManager.markAsOutOfMemory) which has been changed or broken during the memory manager consolidation
    • AbstractBytesToBytesMapSuite
    • UnsafeExternalSorterSuite
    • UnsafeFixedWidthAggregationMapSuite
    • UnsafeKVExternalSorterSuite

Compatiblity notes:

  • This patch introduces breaking changes in ExternalAppendOnlyMap, which is marked as @DevloperAPI (likely for legacy reasons): this class now cannot be used outside of a task.

@SparkQA
Copy link

SparkQA commented Oct 15, 2015

Test build #43753 has finished for PR 9127 at commit 25ba4b5.

  • This patch fails MiMa tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@JoshRosen
Copy link
Contributor Author

Jenkins, retest this please.

@SparkQA
Copy link

SparkQA commented Oct 15, 2015

Test build #43789 has finished for PR 9127 at commit 25ba4b5.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Oct 16, 2015

Test build #43858 has finished for PR 9127 at commit 8f93e94.

  • This patch fails MiMa tests.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Oct 16, 2015

Test build #43862 has finished for PR 9127 at commit 3bbc54d.

  • This patch fails MiMa tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Oct 16, 2015

Test build #43866 has finished for PR 9127 at commit ec48ff9.

  • This patch fails MiMa tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@JoshRosen JoshRosen force-pushed the SPARK-10984 branch 2 times, most recently from b1aa89d to e52f030 Compare October 17, 2015 02:00
@SparkQA
Copy link

SparkQA commented Oct 17, 2015

Test build #43875 has finished for PR 9127 at commit 7d6a37f.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@JoshRosen
Copy link
Contributor Author

Alright, the first part of this change should be ready for review. At this point, there are a number of remaining small tasks for updating mocks which are used in unit tests, but the basic "client" APIs for interacting with the memory managers have been sketched out. Please take a look at those and provide feedback on whether the new, combined interfaces make sense. Once we agree on those interfaces, we can loop back and do a second review pass looking at the test refactoring code.

@SparkQA
Copy link

SparkQA commented Oct 22, 2015

Test build #44170 has finished for PR 9127 at commit f21b767.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This use of the memory managers in HashedRelation was messy before and is slightly messier now; I'd like to find a clean way to deal with this before merging this refactoring.

@SparkQA
Copy link

SparkQA commented Oct 23, 2015

Test build #44253 has finished for PR 9127 at commit 04ec429.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Oct 23, 2015

Test build #44255 has finished for PR 9127 at commit e56d039.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Oct 23, 2015

Test build #44261 has finished for PR 9127 at commit aa14113.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@JoshRosen JoshRosen changed the title [SPARK-10984] [WIP] Simplify *MemoryManager class structure [SPARK-10984] Simplify *MemoryManager class structure Oct 23, 2015
@SparkQA
Copy link

SparkQA commented Oct 24, 2015

Test build #44269 has finished for PR 9127 at commit 5af0b17.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Oct 24, 2015

Test build #44274 has finished for PR 9127 at commit a264703.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@JoshRosen
Copy link
Contributor Author

This PR is logically blocked on #9260.

@SparkQA
Copy link

SparkQA commented Oct 24, 2015

Test build #44290 has finished for PR 9127 at commit 0b5c72f.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Oct 26, 2015

Test build #44326 has finished for PR 9127 at commit f68fdb1.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@JoshRosen
Copy link
Contributor Author

Woohoo, this passes tests!

There are still a few minor follow-up tasks that I'd like to do for this, but I'm going to defer them to separate patches: this patch is fairly large and has conflicts with several other memory-management-related patches that are in-flight or which will be opened soon. @andrewor14, if you have any post-hoc review comments, I'll handle them in a followup. @davies, this should unblock your open patch.

Merging to master.

@JoshRosen
Copy link
Contributor Author

I filed https://issues.apache.org/jira/browse/SPARK-11309 as one followup.

@davies
Copy link
Contributor

davies commented Oct 26, 2015

@JoshRosen Great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants