From c44fc87971c2b313df69ac2815bae1824e7ddb1b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Hiram=20Soltren?= Date: Fri, 10 Mar 2017 13:57:26 -0600 Subject: [PATCH 1/3] [SPARK-7200] Add cleanup for memory leak tests to after() in TaskMemoryManagerSuite Cleans up all memory used by tests explicitly in after(). This new function has the JUnit @After tag and thus is guaranteed to run, even if an @Test method throws an exception. Testing: Ran tests locally. Hacked an existing test and made sure that a failure was reported in the original test, and not in after(). Verified test logging in test-reports/org.apache.spark.memory.TaskMemoryManagerSuite.xml. --- .../spark/memory/TaskMemoryManagerSuite.java | 29 ++++++++++++------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/core/src/test/java/org/apache/spark/memory/TaskMemoryManagerSuite.java b/core/src/test/java/org/apache/spark/memory/TaskMemoryManagerSuite.java index f53bc0b02bbfa..51292268d0a71 100644 --- a/core/src/test/java/org/apache/spark/memory/TaskMemoryManagerSuite.java +++ b/core/src/test/java/org/apache/spark/memory/TaskMemoryManagerSuite.java @@ -17,6 +17,7 @@ package org.apache.spark.memory; +import org.junit.After; import org.junit.Assert; import org.junit.Test; @@ -25,9 +26,18 @@ public class TaskMemoryManagerSuite { + static TaskMemoryManager manager; + + @After + public void after() { + Assert.assertEquals(0, manager.getMemoryConsumptionForThisTask()); + Assert.assertEquals(0, manager.cleanUpAllAllocatedMemory()); + manager = null; + } + @Test public void leakedPageMemoryIsDetected() { - final TaskMemoryManager manager = new TaskMemoryManager( + manager = new TaskMemoryManager( new StaticMemoryManager( new SparkConf().set("spark.memory.offHeap.enabled", "false"), Long.MAX_VALUE, @@ -35,9 +45,10 @@ public void leakedPageMemoryIsDetected() { 1), 0); final MemoryConsumer c = new TestMemoryConsumer(manager); - manager.allocatePage(4096, c); // leak memory + final MemoryBlock block = manager.allocatePage(4096, c); // leak memory Assert.assertEquals(4096, manager.getMemoryConsumptionForThisTask()); Assert.assertEquals(4096, manager.cleanUpAllAllocatedMemory()); + manager.freePage(block, c); } @Test @@ -45,7 +56,7 @@ public void encodePageNumberAndOffsetOffHeap() { final SparkConf conf = new SparkConf() .set("spark.memory.offHeap.enabled", "true") .set("spark.memory.offHeap.size", "1000"); - final TaskMemoryManager manager = new TaskMemoryManager(new TestMemoryManager(conf), 0); + manager = new TaskMemoryManager(new TestMemoryManager(conf), 0); final MemoryConsumer c = new TestMemoryConsumer(manager, MemoryMode.OFF_HEAP); final MemoryBlock dataPage = manager.allocatePage(256, c); // In off-heap mode, an offset is an absolute address that may require more than 51 bits to @@ -58,7 +69,7 @@ public void encodePageNumberAndOffsetOffHeap() { @Test public void encodePageNumberAndOffsetOnHeap() { - final TaskMemoryManager manager = new TaskMemoryManager( + manager = new TaskMemoryManager( new TestMemoryManager(new SparkConf().set("spark.memory.offHeap.enabled", "false")), 0); final MemoryConsumer c = new TestMemoryConsumer(manager, MemoryMode.ON_HEAP); final MemoryBlock dataPage = manager.allocatePage(256, c); @@ -71,7 +82,7 @@ public void encodePageNumberAndOffsetOnHeap() { public void cooperativeSpilling() { final TestMemoryManager memoryManager = new TestMemoryManager(new SparkConf()); memoryManager.limit(100); - final TaskMemoryManager manager = new TaskMemoryManager(memoryManager, 0); + manager = new TaskMemoryManager(memoryManager, 0); TestMemoryConsumer c1 = new TestMemoryConsumer(manager); TestMemoryConsumer c2 = new TestMemoryConsumer(manager); @@ -106,14 +117,13 @@ public void cooperativeSpilling() { c1.free(0); c2.free(100); - Assert.assertEquals(0, manager.cleanUpAllAllocatedMemory()); } @Test public void cooperativeSpilling2() { final TestMemoryManager memoryManager = new TestMemoryManager(new SparkConf()); memoryManager.limit(100); - final TaskMemoryManager manager = new TaskMemoryManager(memoryManager, 0); + manager = new TaskMemoryManager(memoryManager, 0); TestMemoryConsumer c1 = new TestMemoryConsumer(manager); TestMemoryConsumer c2 = new TestMemoryConsumer(manager); @@ -141,14 +151,13 @@ public void cooperativeSpilling2() { c1.free(0); c2.free(80); c3.free(10); - Assert.assertEquals(0, manager.cleanUpAllAllocatedMemory()); } @Test public void shouldNotForceSpillingInDifferentModes() { final TestMemoryManager memoryManager = new TestMemoryManager(new SparkConf()); memoryManager.limit(100); - final TaskMemoryManager manager = new TaskMemoryManager(memoryManager, 0); + manager = new TaskMemoryManager(memoryManager, 0); TestMemoryConsumer c1 = new TestMemoryConsumer(manager, MemoryMode.ON_HEAP); TestMemoryConsumer c2 = new TestMemoryConsumer(manager, MemoryMode.OFF_HEAP); @@ -170,7 +179,7 @@ public void offHeapConfigurationBackwardsCompatibility() { final SparkConf conf = new SparkConf() .set("spark.unsafe.offHeap", "true") .set("spark.memory.offHeap.size", "1000"); - final TaskMemoryManager manager = new TaskMemoryManager(new TestMemoryManager(conf), 0); + manager = new TaskMemoryManager(new TestMemoryManager(conf), 0); Assert.assertSame(MemoryMode.OFF_HEAP, manager.tungstenMemoryMode); } From 8677a990449213f6deca3266cdcfedb12a17e275 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Hiram=20Soltren?= Date: Tue, 16 May 2017 16:28:56 -0700 Subject: [PATCH 2/3] changes in response to code review from vanzin on 2017-03-28 --- .../java/org/apache/spark/memory/TaskMemoryManagerSuite.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/core/src/test/java/org/apache/spark/memory/TaskMemoryManagerSuite.java b/core/src/test/java/org/apache/spark/memory/TaskMemoryManagerSuite.java index 51292268d0a71..24fa4c2bfc5fb 100644 --- a/core/src/test/java/org/apache/spark/memory/TaskMemoryManagerSuite.java +++ b/core/src/test/java/org/apache/spark/memory/TaskMemoryManagerSuite.java @@ -26,10 +26,11 @@ public class TaskMemoryManagerSuite { - static TaskMemoryManager manager; + private TaskMemoryManager manager; @After public void after() { + Assert.assertNotNull(manager); Assert.assertEquals(0, manager.getMemoryConsumptionForThisTask()); Assert.assertEquals(0, manager.cleanUpAllAllocatedMemory()); manager = null; @@ -48,7 +49,7 @@ public void leakedPageMemoryIsDetected() { final MemoryBlock block = manager.allocatePage(4096, c); // leak memory Assert.assertEquals(4096, manager.getMemoryConsumptionForThisTask()); Assert.assertEquals(4096, manager.cleanUpAllAllocatedMemory()); - manager.freePage(block, c); + Assert.assertEquals(0, manager.cleanUpAllAllocatedMemory()); } @Test From e6ca027cc3d76fcd3eea1673cf8af6a4117cf12a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Hiram=20Soltren?= Date: Tue, 16 May 2017 16:44:28 -0700 Subject: [PATCH 3/3] minor cleanup to get rid of unused temp variable --- .../java/org/apache/spark/memory/TaskMemoryManagerSuite.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/test/java/org/apache/spark/memory/TaskMemoryManagerSuite.java b/core/src/test/java/org/apache/spark/memory/TaskMemoryManagerSuite.java index 24fa4c2bfc5fb..3251c14604d3f 100644 --- a/core/src/test/java/org/apache/spark/memory/TaskMemoryManagerSuite.java +++ b/core/src/test/java/org/apache/spark/memory/TaskMemoryManagerSuite.java @@ -46,7 +46,7 @@ public void leakedPageMemoryIsDetected() { 1), 0); final MemoryConsumer c = new TestMemoryConsumer(manager); - final MemoryBlock block = manager.allocatePage(4096, c); // leak memory + manager.allocatePage(4096, c); // leak memory Assert.assertEquals(4096, manager.getMemoryConsumptionForThisTask()); Assert.assertEquals(4096, manager.cleanUpAllAllocatedMemory()); Assert.assertEquals(0, manager.cleanUpAllAllocatedMemory());