[SPARK-26539][CORE] Remove spark.memory.useLegacyMode and StaticMemoryManager #23457

srowen · 2019-01-04T21:25:05Z

What changes were proposed in this pull request?

Remove spark.memory.useLegacyMode and StaticMemoryManager. Update tests that used the StaticMemoryManager to equivalent use of UnifiedMemoryManager.

How was this patch tested?

Existing tests, with modifications to make them work with a different mem manager.

srowen · 2019-01-04T21:29:41Z

core/src/main/scala/org/apache/spark/memory/UnifiedMemoryManager.scala

@@ -45,7 +45,7 @@ import org.apache.spark.storage.BlockId
 *                          it if necessary. Cached blocks can be evicted only if actual
 *                          storage memory usage exceeds this region.
 */
-private[spark] class UnifiedMemoryManager private[memory] (
+private[spark] class UnifiedMemoryManager(


Tests and other internals now need to use this constructor directly

srowen · 2019-01-04T21:30:10Z

core/src/test/scala/org/apache/spark/storage/MemoryStoreSuite.scala

@@ -61,7 +60,7 @@ class MemoryStoreSuite
  }

  def makeMemoryStore(maxMem: Long): (MemoryStore, BlockInfoManager) = {
-    val memManager = new StaticMemoryManager(conf, Long.MaxValue, maxMem, numCores = 1)
+    val memManager = new UnifiedMemoryManager(conf, maxMem, (0.4 * maxMem).toInt, 1)


This mimics the behavior of the old memory manager with spark.storage.unrollFraction=0.4

srowen · 2019-01-04T21:31:52Z

core/src/test/scala/org/apache/spark/storage/MemoryStoreSuite.scala

    val result4 = putIteratorAsBytes("b4", bigIterator, ClassTag.Any)
    assert(result4.isLeft) // unroll was unsuccessful
    assert(!memoryStore.contains("b1"))
-    assert(!memoryStore.contains("b2"))
+    assert(memoryStore.contains("b2")) // not necessarily evicted


This behavior changed after the change above. However, I am not sure this assertion is correct? there are 4800 bytes available. b2 takes 400. An attempt to put 4000 may fail (? well that was the previous and current behavior, anyway), but I don't see that it necessarily requires evicting b2; the allocation failed. We've recently had a very similar discussion about a different change that also uncovered this assertion as an issue.

isn't b2 takes 4000 and b4 takes 40000 ?

I verified the test and I think the assertion is correct.

Actually, for current behaviour with UnifiedMemoryManager, there may be 12000 bytes available. When we try to put b4 into memory, it will require 24064 memory once it unrolled 16 elements (which hit the memoryCheckPeriod for first time). And 24064 bytes exceed the maxMemory of 12000, so, it would just return instread of evicting any blocks.

Oops yes b2 takes 4000 and b4 40000. OK, certainly b4 can't fit. Yes on thinking this through a second time, this isn't quite the right change. The difference between total and storage memory in the unified memory manager is irrelevant here. It's just that this test needs less total memory available -- a little more than 8000 bytes, not 12000. Let me push a fix.

srowen · 2019-01-04T21:32:19Z

core/src/test/scala/org/apache/spark/util/collection/ExternalAppendOnlyMapSuite.scala

-      .set("spark.memory.useLegacyMode", "true")
-      .set("spark.testing.memory", "100000000")
+      .set("spark.memory.storageFraction", "0.999")
+      .set("spark.testing.memory", "471859200")


This is the minimum allowed value. The new setting 0.999 mimics having almost no shuffle memory

SparkQA · 2019-01-05T01:51:41Z

Test build #100757 has finished for PR 23457 at commit 40e4209.

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

Ngone51

Pretty Cool!

Ngone51 · 2019-01-05T06:36:40Z

core/src/main/scala/org/apache/spark/memory/package.scala

@@ -61,15 +61,10 @@ package org.apache.spark
 * }}}
 *
 *
- * There are two implementations of [[org.apache.spark.memory.MemoryManager]] which vary in how
- * they handle the sizing of their memory pools:
+ * There is one implementations of [[org.apache.spark.memory.MemoryManager]]:


nit: implementations -> implementation

Ngone51 · 2019-01-05T07:36:16Z

core/src/test/scala/org/apache/spark/storage/MemoryStoreSuite.scala

    val result4 = putIteratorAsBytes("b4", bigIterator, ClassTag.Any)
    assert(result4.isLeft) // unroll was unsuccessful
    assert(!memoryStore.contains("b1"))
-    assert(!memoryStore.contains("b2"))
+    assert(memoryStore.contains("b2")) // not necessarily evicted


isn't b2 takes 4000 and b4 takes 40000 ?

I verified the test and I think the assertion is correct.

Actually, for current behaviour with UnifiedMemoryManager, there may be 12000 bytes available. When we try to put b4 into memory, it will require 24064 memory once it unrolled 16 elements (which hit the memoryCheckPeriod for first time). And 24064 bytes exceed the maxMemory of 12000, so, it would just return instread of evicting any blocks.

SparkQA · 2019-01-05T20:06:54Z

Test build #100789 has finished for PR 23457 at commit f4ee34e.

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

gatorsmile · 2019-01-05T22:22:08Z

cc @jiangxb1987

SparkQA · 2019-01-08T23:15:25Z

Test build #100940 has finished for PR 23457 at commit 69ca3a9.

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

SparkQA · 2019-01-09T05:50:03Z

Test build #4500 has finished for PR 23457 at commit 69ca3a9.

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

viirya · 2019-01-09T06:48:55Z

core/src/main/scala/org/apache/spark/memory/package.scala

@@ -61,15 +61,10 @@ package org.apache.spark
 * }}}
 *
 *
- * There are two implementations of [[org.apache.spark.memory.MemoryManager]] which vary in how
- * they handle the sizing of their memory pools:
+ * There is one implementation of [[org.apache.spark.memory.MemoryManager]]:
 *
 *  - [[org.apache.spark.memory.UnifiedMemoryManager]], the default in Spark 1.6+, enforces soft


nit: since just one implementation now, still need to mention this is default?

SparkQA · 2019-01-09T19:36:56Z

Test build #100973 has finished for PR 23457 at commit 9990796.

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

srowen · 2019-01-10T14:58:33Z

Merged to master

gatorsmile · 2019-01-25T00:23:46Z

core/src/test/java/org/apache/spark/memory/TaskMemoryManagerSuite.java

@@ -29,7 +29,7 @@
  @Test
  public void leakedPageMemoryIsDetected() {
    final TaskMemoryManager manager = new TaskMemoryManager(
-      new StaticMemoryManager(
+      new UnifiedMemoryManager(
        new SparkConf().set("spark.memory.offHeap.enabled", "false"),
        Long.MAX_VALUE,
        Long.MAX_VALUE,


What does these mean after the change?

maxHeapMemory = Long.MAX_VALUE and onHeapStorageRegionSize = Long.MAX_VALUE?

gatorsmile · 2019-01-25T00:24:39Z

core/src/test/scala/org/apache/spark/storage/MemoryStoreSuite.scala

@@ -60,7 +59,7 @@ class MemoryStoreSuite
  }

  def makeMemoryStore(maxMem: Long): (MemoryStore, BlockInfoManager) = {
-    val memManager = new StaticMemoryManager(conf, Long.MaxValue, maxMem, numCores = 1)
+    val memManager = new UnifiedMemoryManager(conf, maxMem, maxMem, 1)


The same question here

gatorsmile · 2019-01-25T00:27:46Z

Is it fine to simply replace it? Basically, the cases I pointed out above change the semantics. That means, onHeapExecutionMemory will be zero, if maxHeapMemory and onHeapStorageRegionSize are the same.

srowen · 2019-01-25T00:47:03Z

You're right, the semantics are different, but I don't think they matter in the test context here. It is probably more accurate to pass something like (maxHeapMemory, maxHeapMemory / 2). The tests pass, at least, because I believe they don't care about execution memory, which is 0.

gatorsmile · 2019-01-25T02:52:01Z

Opened a ticket https://issues.apache.org/jira/browse/SPARK-26725 I think we should not write an invalid inputs in the test cases.

gatorsmile · 2019-01-25T07:49:30Z

sql/core/src/main/scala/org/apache/spark/sql/execution/joins/HashedRelation.scala

@@ -94,7 +94,7 @@ private[execution] object HashedRelation {
      taskMemoryManager: TaskMemoryManager = null): HashedRelation = {
    val mm = Option(taskMemoryManager).getOrElse {
      new TaskMemoryManager(
-        new StaticMemoryManager(
+        new UnifiedMemoryManager(
          new SparkConf().set(MEMORY_OFFHEAP_ENABLED.key, "false"),
          Long.MaxValue,
          Long.MaxValue,


How about these?

…tructor in test suites ## What changes were proposed in this pull request? Adjust mem settings in UnifiedMemoryManager used in test suites to ha…ve execution memory > 0 Ref: #23457 (comment) ## How was this patch tested? Existing tests Closes #23645 from srowen/SPARK-26725. Authored-by: Sean Owen <sean.owen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…yManager ## What changes were proposed in this pull request? Remove spark.memory.useLegacyMode and StaticMemoryManager. Update tests that used the StaticMemoryManager to equivalent use of UnifiedMemoryManager. ## How was this patch tested? Existing tests, with modifications to make them work with a different mem manager. Closes apache#23457 from srowen/SPARK-26539. Authored-by: Sean Owen <sean.owen@databricks.com> Signed-off-by: Sean Owen <sean.owen@databricks.com>

…tructor in test suites ## What changes were proposed in this pull request? Adjust mem settings in UnifiedMemoryManager used in test suites to ha…ve execution memory > 0 Ref: apache#23457 (comment) ## How was this patch tested? Existing tests Closes apache#23645 from srowen/SPARK-26725. Authored-by: Sean Owen <sean.owen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

srowen force-pushed the SPARK-26539 branch from fdf51d8 to 40e4209 Compare January 4, 2019 21:29

srowen commented Jan 4, 2019

View reviewed changes

Ngone51 approved these changes Jan 5, 2019

View reviewed changes

Remove spark.memory.useLegacyMode and StaticMemoryManager

69ca3a9

srowen force-pushed the SPARK-26539 branch from f4ee34e to 69ca3a9 Compare January 8, 2019 19:09

viirya reviewed Jan 9, 2019

View reviewed changes

Remove reference to 'default' memory manager

9990796

cloud-fan approved these changes Jan 10, 2019

View reviewed changes

srowen closed this in 2f8a938 Jan 10, 2019

srowen deleted the SPARK-26539 branch January 21, 2019 17:57

gatorsmile reviewed Jan 25, 2019

View reviewed changes

srowen mentioned this pull request Jan 25, 2019

[SPARK-26725][TEST] Fix the input values of UnifiedMemoryManager constructor in test suites #23645

Closed

gatorsmile reviewed Jan 25, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-26539][CORE] Remove spark.memory.useLegacyMode and StaticMemoryManager #23457

[SPARK-26539][CORE] Remove spark.memory.useLegacyMode and StaticMemoryManager #23457

srowen commented Jan 4, 2019

srowen Jan 4, 2019

srowen Jan 4, 2019

srowen Jan 4, 2019

Ngone51 Jan 5, 2019

srowen Jan 5, 2019

srowen Jan 4, 2019

SparkQA commented Jan 5, 2019

Ngone51 left a comment

Ngone51 Jan 5, 2019

Ngone51 Jan 5, 2019

SparkQA commented Jan 5, 2019

gatorsmile commented Jan 5, 2019

SparkQA commented Jan 8, 2019

SparkQA commented Jan 9, 2019

viirya Jan 9, 2019

SparkQA commented Jan 9, 2019

srowen commented Jan 10, 2019

gatorsmile Jan 25, 2019

gatorsmile Jan 25, 2019

gatorsmile commented Jan 25, 2019

srowen commented Jan 25, 2019

gatorsmile commented Jan 25, 2019 •

edited

Loading

gatorsmile Jan 25, 2019

[SPARK-26539][CORE] Remove spark.memory.useLegacyMode and StaticMemoryManager #23457

[SPARK-26539][CORE] Remove spark.memory.useLegacyMode and StaticMemoryManager #23457

Conversation

srowen commented Jan 4, 2019

What changes were proposed in this pull request?

How was this patch tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jan 5, 2019

Ngone51 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jan 5, 2019

gatorsmile commented Jan 5, 2019

SparkQA commented Jan 8, 2019

SparkQA commented Jan 9, 2019

Choose a reason for hiding this comment

SparkQA commented Jan 9, 2019

srowen commented Jan 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gatorsmile commented Jan 25, 2019

srowen commented Jan 25, 2019

gatorsmile commented Jan 25, 2019 • edited Loading

Choose a reason for hiding this comment

gatorsmile commented Jan 25, 2019 •

edited

Loading