[SPARK-22143][SQL] Fix memory leak in OffHeapColumnVector #19367

hvanhovell · 2017-09-27T14:02:51Z

What changes were proposed in this pull request?

WriteableColumnVector does not close its child column vectors. This can create memory leaks for OffHeapColumnVector where we do not clean up the memory allocated by a vectors children. This can be especially bad for string columns (which uses a child byte column vector).

How was this patch tested?

I have updated the existing tests to always use both on-heap and off-heap vectors. Testing and diagnoses was done locally.

hvanhovell · 2017-09-27T14:03:48Z

cc @ala @michal-databricks @ueshin

kiszk · 2017-09-27T14:15:59Z

sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/OffHeapColumnVector.java

@@ -85,6 +85,7 @@ public long nullsNativeAddress() {

  @Override
  public void close() {
+    super.close();
    Platform.freeMemory(nulls);


This comment is not related to this fix directly. However, is it better to check whether each field is not 0 before calling Platform.freeMemory()? For example, data or lengthData/offsetData may be 0`.

Yeah, it is a bit weird. The doc in the JDK8's Unsafe. freeMemory() states the following: The address passed to this method may be null, in which case no action is taken. See: http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/sun/misc/Unsafe.java

ueshin

LGTM except for some minor comments.

ueshin · 2017-09-27T14:29:50Z

sql/core/src/test/scala/org/apache/spark/sql/execution/vectorized/ColumnVectorSuite.scala

+      dt: DataType)(
+      block: WritableColumnVector => Unit): Unit = {
+    test(name) {
+      val c1 = new OnHeapColumnVector(size, dt)


What's this c1 for?

ueshin · 2017-09-27T14:42:25Z

sql/core/src/test/scala/org/apache/spark/sql/execution/vectorized/ColumnarBatchSuite.scala

  }

  test("toArray for primitive types") {
-    // (MemoryMode.ON_HEAP :: MemoryMode.OFF_HEAP :: Nil).foreach { memMode => {
-    (MemoryMode.ON_HEAP :: Nil).foreach { memMode => {
+    (MemoryMode.ON_HEAP :: MemoryMode.OFF_HEAP :: Nil).foreach { memMode => {


We can remove a pair of brace like .foreach { memMode => ....

ueshin · 2017-09-27T14:42:34Z

sql/core/src/test/scala/org/apache/spark/sql/execution/vectorized/ColumnarBatchSuite.scala

-    // (MemoryMode.ON_HEAP :: MemoryMode.OFF_HEAP :: Nil).foreach { memMode => {
-    (MemoryMode.ON_HEAP :: Nil).foreach { memMode => {
+    (MemoryMode.ON_HEAP :: MemoryMode.OFF_HEAP :: Nil).foreach { memMode => {
+    // (MemoryMode.ON_HEAP :: Nil).foreach { memMode => {


Can we remove this line?

michal-databricks

LGTM

ala

LGTM. Just some nitpicks.

ala · 2017-09-27T14:55:51Z

sql/core/src/test/scala/org/apache/spark/sql/execution/vectorized/ColumnVectorSuite.scala

+      dt: DataType)(
+      block: WritableColumnVector => Unit): Unit = {
+    test(name) {
+      val c1 = new OnHeapColumnVector(size, dt)


What is c1 for?

ala · 2017-09-27T14:58:03Z

sql/core/src/test/scala/org/apache/spark/sql/execution/vectorized/ColumnarBatchSuite.scala

+    test(name) {
+      modes.foreach { mode =>
+        val vector = allocate(size, dt, mode)
+        try block(vector, mode) finally {


The way you use {} here is weird.

ala · 2017-09-27T15:00:51Z

sql/core/src/test/scala/org/apache/spark/sql/execution/vectorized/ColumnarBatchSuite.scala

  }

-  test("Float APIs") {
-    (MemoryMode.ON_HEAP :: MemoryMode.OFF_HEAP :: Nil).foreach { memMode => {
+  testVector("Float Apis", 1024, FloatType, MemoryMode.ON_HEAP, MemoryMode.OFF_HEAP) {


I think changing Apis to APIs would be better than the other way around.

ala · 2017-09-27T15:03:32Z

sql/core/src/test/scala/org/apache/spark/sql/execution/vectorized/ColumnarBatchSuite.scala

+  testVector(
+    "Nest Struct in Array",
+    10,
+    new ArrayType(new StructType().add("int", IntegerType).add("long", LongType), true),


Did you mean new ArrayType(structType)?

kiszk · 2017-09-27T15:32:58Z

sql/core/src/test/scala/org/apache/spark/sql/execution/vectorized/ColumnVectorSuite.scala

-  test("struct") {
-    val schema = new StructType().add("int", IntegerType).add("double", DoubleType)
-    testVector = allocate(10, schema)
+  val structType: StructType = new StructType().add("int", IntegerType).add("double", DoubleType)


nit: Do we need : StructType?

It is a class level field now, so scalastyle wants me type it...

kiszk · 2017-09-27T15:38:55Z

sql/core/src/test/scala/org/apache/spark/sql/execution/vectorized/ColumnarBatchSuite.scala

+      name: String,
+      size: Int,
+      dt: DataType,
+      modes: MemoryMode*)(


Can we do modes: Seq[MemoryMode] = Seq(MemoryMode.ON_HEAP, MemoryMode.ON_HEAP))(? Then, we can avoid repeat testVector(..., MemoryMode.ON_HEAP, MemoryMode.OFF_HEAP) at each suite.

SparkQA · 2017-09-27T16:43:59Z

Test build #82243 has finished for PR 19367 at commit 4b494c5.

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

SparkQA · 2017-09-27T17:43:47Z

Test build #82247 has finished for PR 19367 at commit 6156758.

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

SparkQA · 2017-09-27T18:20:17Z

Test build #82248 has finished for PR 19367 at commit 84caf03.

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

hvanhovell · 2017-09-27T18:23:56Z

retest this please

SparkQA · 2017-09-27T21:07:30Z

Test build #82251 has finished for PR 19367 at commit 84caf03.

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

hvanhovell · 2017-09-27T21:08:57Z

Merging to master.

## What changes were proposed in this pull request? `WriteableColumnVector` does not close its child column vectors. This can create memory leaks for `OffHeapColumnVector` where we do not clean up the memory allocated by a vectors children. This can be especially bad for string columns (which uses a child byte column vector). ## How was this patch tested? I have updated the existing tests to always use both on-heap and off-heap vectors. Testing and diagnoses was done locally. Author: Herman van Hovell <hvanhovell@databricks.com> Closes apache#19367 from hvanhovell/SPARK-22143. # Conflicts: # sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/WritableColumnVector.java # sql/core/src/test/scala/org/apache/spark/sql/execution/vectorized/ColumnVectorSuite.scala # sql/core/src/test/scala/org/apache/spark/sql/execution/vectorized/ColumnarBatchSuite.scala

Update error messages.

4b494c5

kiszk reviewed Sep 27, 2017

View reviewed changes

ueshin reviewed Sep 27, 2017

View reviewed changes

michal-databricks approved these changes Sep 27, 2017

View reviewed changes

ala approved these changes Sep 27, 2017

View reviewed changes

Code Review

6156758

kiszk reviewed Sep 27, 2017

View reviewed changes

Remove modes

84caf03

asfgit closed this in 02bb068 Sep 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-22143][SQL] Fix memory leak in OffHeapColumnVector #19367

[SPARK-22143][SQL] Fix memory leak in OffHeapColumnVector #19367

hvanhovell commented Sep 27, 2017

hvanhovell commented Sep 27, 2017

kiszk Sep 27, 2017

hvanhovell Sep 27, 2017

ueshin left a comment

ueshin Sep 27, 2017

ueshin Sep 27, 2017

ueshin Sep 27, 2017

michal-databricks left a comment

ala left a comment

ala Sep 27, 2017

ala Sep 27, 2017

ala Sep 27, 2017

ala Sep 27, 2017

kiszk Sep 27, 2017

hvanhovell Sep 27, 2017

kiszk Sep 27, 2017

SparkQA commented Sep 27, 2017

SparkQA commented Sep 27, 2017

SparkQA commented Sep 27, 2017

hvanhovell commented Sep 27, 2017

SparkQA commented Sep 27, 2017

hvanhovell commented Sep 27, 2017

[SPARK-22143][SQL] Fix memory leak in OffHeapColumnVector #19367

[SPARK-22143][SQL] Fix memory leak in OffHeapColumnVector #19367

Conversation

hvanhovell commented Sep 27, 2017

What changes were proposed in this pull request?

How was this patch tested?

hvanhovell commented Sep 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ueshin left a 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

michal-databricks left a comment

Choose a reason for hiding this comment

ala left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Sep 27, 2017

SparkQA commented Sep 27, 2017

SparkQA commented Sep 27, 2017

hvanhovell commented Sep 27, 2017

SparkQA commented Sep 27, 2017

hvanhovell commented Sep 27, 2017