[SPARK-21907][CORE] oom during spill #19181

eyalfa · 2017-09-10T14:45:18Z

What changes were proposed in this pull request?

a test reproducing SPARK-21907
a fix for the root cause of the issue.

org.apache.spark.util.collection.unsafe.sort.UnsafeExternalSorter.spill calls org.apache.spark.util.collection.unsafe.sort.UnsafeInMemorySorter.reset which may trigger another spill,
when this happens the array member is already de-allocated but still referenced by the code, this causes the nested spill to fail with an NPE in org.apache.spark.memory.TaskMemoryManager.getPage.
This patch introduces a reproduction in a test case and a fix, the fix simply sets the in-mem sorter's array member to an empty array before actually performing the allocation. This prevents the spilling code from 'touching' the de-allocated array.

How was this patch tested?

introduced a new test case: org.apache.spark.util.collection.unsafe.sort.UnsafeExternalSorterSuite#testOOMDuringSpill.

…e test by requiring OOM exception thrown from the reset() method.

eyalfa · 2017-09-10T14:50:41Z

@ericl , @davies ,
you guys seem to be the last ones to edit this area of the code, I'd appreciate if you could take a look.

hvanhovell · 2017-09-12T10:21:16Z

ok to test

SparkQA · 2017-09-12T13:42:26Z

Test build #81672 has finished for PR 19181 at commit ae7fbc4.

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

eyalfa · 2017-09-12T14:56:53Z

core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeInMemorySorter.java

@@ -170,6 +170,10 @@ public void free() {
  public void reset() {
    if (consumer != null) {
      consumer.freeArray(array);
+      array = LongArray.empty;


@hvanhovell ,
I'm starting to have second thoughts about the special empty instance here, I'm afraid the the nested call might trigger freeArray or something similar on it.
perhaps using null is a better option here?

…instead.

…ySorter that demonstrates an NPE when free() is called after an OOM.

…ee when called after an OOM.

hvanhovell · 2017-09-13T20:42:09Z

core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorter.java

@@ -85,7 +85,7 @@
  private final LinkedList<UnsafeSorterSpillWriter> spillWriters = new LinkedList<>();

  // These variables are reset after spilling:
-  @Nullable private volatile UnsafeInMemorySorter inMemSorter;
+  @VisibleForTesting @Nullable volatile UnsafeInMemorySorter inMemSorter;


This is slightly hair raising. Can you try to find a different - less tricky path - to test this?

@hvanhovell , is it acceptable to introduce a method indicating if the next inset is going to spill? Can make this @VisibleForTest and prefixed with something like testOnly

hvanhovell · 2017-09-13T20:43:58Z

core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeInMemorySorter.java

      array = null;
    }
  }

  public void reset() {
    if (consumer != null) {
      consumer.freeArray(array);
+      array = null;


Please document why this is needed.

hvanhovell · 2017-09-13T20:45:30Z

cc @juliuszsompolski can you take a look, or perhaps take this for a test drive?

hvanhovell · 2017-09-13T20:49:20Z

core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorterSuite.java

+    // and ended up with a failed assertion.
+    // we also expect the location of the OOM to be org.apache.spark.util.collection.unsafe.sort.UnsafeInMemorySorter.reset
+    memoryManager.markConseqOOM(2);
+    exceptions.expect(OutOfMemoryError.class);


Why not just catch the error? Seems a bit easier.

I'm used to scalatest's Matchers, things could be so much easier if this test was written in Scala...
Will rewrite...

SparkQA · 2017-09-13T23:02:40Z

Test build #81734 has finished for PR 19181 at commit 2fd8e91.

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

…isibleForTesting

…plfy exception related test.

@hvanhovell

… by @hvanhovell.

SparkQA · 2017-09-14T07:04:45Z

Test build #81765 has finished for PR 19181 at commit 57f20b7.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

eyalfa · 2017-09-14T10:18:16Z

@hvanhovell, @juliuszsompolski,
failure seem unrelated to my work, can you please request a retest?

hvanhovell · 2017-09-14T11:16:38Z

@eyalfa you should be able to retrigger tests yourself: jenkins retest this please

hvanhovell · 2017-09-14T11:16:47Z

retest this please

juliuszsompolski · 2017-09-14T11:38:24Z

LGTM.
It'd be nicer if there was an invariant that a nested spill just never can happen, i.e. there are no places that can spill on the code path that already spills.
But this fix does seem to render nested spill harmless in this particular case.

@hvanhovell In my original use case I hit this bug after hours of running on a big cluster, so reproducing it on that case is quite unpractical for me. I'll see if I'll observe it again.

eyalfa · 2017-09-14T12:10:14Z

@juliuszsompolski,
if you comment the few added lines in the reset() method, you'd see that the test fails with a stack frame very similar to the one you pasted in JIRA.

SparkQA · 2017-09-14T14:32:49Z

Test build #81782 has finished for PR 19181 at commit 57f20b7.

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

juliuszsompolski · 2017-09-15T11:26:17Z

@eyalfa Thanks. I agree that this is a good fix to this issue and lgtm.
I'm just worried that there are more lurking cases where a nested spill can trigger and cause something unexpected, and that finding and reproducing such other similar issues may be difficult.

eyalfa · 2017-09-15T11:32:35Z

I agree, but 'it is what it is'😎 We can probably come up with some mechanism that detects such scenarios and avoids invoking the spill method on an object whose already 'on the stack', I think this requires more involvement from the committers, perhaps in a separate Jira?

…

On Sep 15, 2017 14:27, "Juliusz Sompolski" ***@***.***> wrote: @eyalfa <https://github.com/eyalfa> Thanks. I agree that this is a good fix to this issue and lgtm. I'm just worried that there are more lurking cases where a nested spill can trigger and cause something unexpected, and that finding and reproducing such other similar issues may be difficult. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#19181 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABFFOSIoc8O29CLcpfn2hPWJV_5neQjqks5sil8WgaJpZM4PSVvS> .

jiangxb1987 · 2017-09-19T00:00:02Z

core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorterSuite.java

+  @Test
+  public void testOOMDuringSpill() throws Exception {
+    final UnsafeExternalSorter sorter = newSorter();
+    for (int i = 0; sorter.hasSpaceForAnotherRecord(); ++i) {


Will this make the test case takes quite a long time to finish?

according to Jenkins: 7ms, I guess we're ok than 😎

eyalfa · 2017-09-20T12:14:48Z

@hvanhovell , PTAL
please let me know if there's anything that requires fixing here.

SparkQA · 2017-09-20T15:32:40Z

Test build #81998 has finished for PR 19181 at commit 48e947d.

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

eyalfa · 2017-09-26T06:41:43Z

@hvanhovell ?

juliuszsompolski · 2017-09-29T11:03:05Z

core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorter.java

@@ -85,7 +85,7 @@
  private final LinkedList<UnsafeSorterSpillWriter> spillWriters = new LinkedList<>();

  // These variables are reset after spilling:
-  @Nullable private volatile UnsafeInMemorySorter inMemSorter;
+  private @Nullable volatile UnsafeInMemorySorter inMemSorter;


nit: unnecessary change.

juliuszsompolski · 2017-09-29T11:03:32Z

core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeInMemorySorter.java

@@ -162,14 +162,25 @@ private int getUsableCapacity() {
   */
  public void free() {
    if (consumer != null) {
-      consumer.freeArray(array);
+      if (null != array) {


nit: RHS literal (array != null)

juliuszsompolski · 2017-09-29T11:04:32Z

core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeInMemorySorter.java

      array = null;
    }
  }

  public void reset() {
    if (consumer != null) {
      consumer.freeArray(array);
+      // this is needed to prevent a 'nested' spill,


nit: it doesn't prevent a nested spill, it only renders it harmless
remove this line - the rest of the comment is true.

juliuszsompolski · 2017-09-29T11:06:00Z

core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorterSuite.java

+    }
+    // todo: this might actually not be zero if pageSize is somehow configured differently,
+    // so we actually have to compute the expected spill size according to the configured page size
+    assertEquals(0,  sorter.getSpillSize());


If this might actually not be zero, maybe don't test this assertion?

well, its a tricky one...
as far as I can track the construction and logic of the relevant classes here, and given that we're sorting Ints here, we're not supposed to spill before expandPointersArray requests additional memory, it is however very difficult and 'involved' to actually check this during test.
I can set the memoryManager to choke before the first loop, this way if my assumption breaks and the sorter attempts to allocate memory we'd get an OOM sooner than expected, effectively failing the test.

@juliuszsompolski , do you find this approach better? it'd still need a comment describing the assumption about memory usage by the sorter.

juliuszsompolski · 2017-09-29T11:06:15Z

core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorterSuite.java

+      insertNumber(sorter, 1024);
+    }
+    // we expect an OutOfMemoryError here, anything else (i.e the original NPE is a failure)
+    catch( OutOfMemoryError oom ){


nit: ws: catch (OutOfMemoryError oom) {

juliuszsompolski · 2017-09-29T11:08:09Z

core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeInMemorySorterSuite.java

+    try {
+      sorter.reset();
+    } catch( OutOfMemoryError oom ) {
+      //as expected


nit: ws: // as expected

juliuszsompolski · 2017-09-29T11:08:30Z

core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeInMemorySorterSuite.java

+    }
+    // this currently fails on NPE at org.apache.spark.memory.MemoryConsumer.freeArray(MemoryConsumer.java:108)
+    sorter.free();
+    //simulate a 'back to back' free.


nit: ws: // simulate ...

juliuszsompolski · 2017-09-29T11:09:20Z

core/src/test/scala/org/apache/spark/memory/TestMemoryManager.scala

@@ -58,11 +58,15 @@ class TestMemoryManager(conf: SparkConf)

  override def maxOffHeapStorageMemory: Long = 0L

-  private var oomOnce = false
+  private var conseqOOM = 0


nit: conseq -> consequent

juliuszsompolski · 2017-09-29T11:09:33Z

core/src/test/scala/org/apache/spark/memory/TestMemoryManager.scala

+    markConseqOOM(1)
+  }
+
+  def markConseqOOM( n : Int) : Unit = {


nit: markConsequentOOM

nit: ws: (n: Int): Unit

juliuszsompolski · 2017-09-29T11:12:46Z

core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorterSuite.java

+import org.hamcrest.Matchers;
+import org.hamcrest.TypeSafeMatcher;
+import org.junit.Rule;
+import org.junit.rules.ExpectedException;


nit: I think you don't use most of these imports anymore.

kiszk · 2017-09-29T15:07:45Z

core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeInMemorySorterSuite.java

+
+  @Test
+  public void freeAfterOOM() {
+    final TestMemoryManager testMemoryManager = new TestMemoryManager(new SparkConf().set("spark.memory.offHeap.enabled", "false"));


nit: Is it better to wrap this line since this is too long.

@kiszk

…and @kiszk.

SparkQA · 2017-10-01T15:09:38Z

Test build #82372 has finished for PR 19181 at commit e3da029.

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

juliuszsompolski · 2017-10-10T15:18:26Z

Looks good to me.
What do you think @hvanhovell ?

hvanhovell · 2017-10-10T15:46:26Z

core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorterSuite.java

+      insertNumber(sorter, 1024);
+    }
+    // we expect an OutOfMemoryError here, anything else (i.e the original NPE is a failure)
+    catch (OutOfMemoryError oom ){


Nit weird spacing: catch (OutOfMemoryError oom) {

hvanhovell · 2017-10-10T15:48:05Z

core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorterSuite.java

+      expectedOOM = oom;
+    }
+
+    assertNotNull("expected OutOfMmoryError but it seems operation surprisingly succeeded"


NIT: just move this into the catch. Just fail if you reach this point, or fail in the try statement.

hvanhovell · 2017-10-10T15:50:23Z

core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeInMemorySorterSuite.java

+    try {
+      sorter.reset();
+    } catch (OutOfMemoryError oom) {
+      // as expected


Make sure you fail on an unexpected success

hvanhovell · 2017-10-10T15:51:02Z

core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeInMemorySorterSuite.java

+  @Test
+  public void freeAfterOOM() {
+    final SparkConf sparkConf =
+            new SparkConf()


hvanhovell · 2017-10-10T15:55:54Z

core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorterSuite.java

+  @Test
+  public void testOOMDuringSpill() throws Exception {
+    final UnsafeExternalSorter sorter = newSorter();
+    // we assume that given default configuration, the size of


NIT NIT NIT NIT NIT NIT This comment is pretty good, but it is kind of hard to read because of the weird white space usage. Can you just make lines of 100 characters

hvanhovell

I have a few minor comments, other than that LGTM.

hvanhovell · 2017-10-10T17:41:30Z

I will merge this when it passes tests.

eyalfa · 2017-10-10T17:43:54Z

@hvanhovell , thanks 👍

SparkQA · 2017-10-10T20:45:30Z

Test build #82594 has finished for PR 19181 at commit 6b901ee.

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

hvanhovell · 2017-10-10T20:51:50Z

Merging to master. Thanks!

Can you create a backport for Spark-2.2?

1. a test reproducing [SPARK-21907](https://issues.apache.org/jira/browse/SPARK-21907) 2. a fix for the root cause of the issue. `org.apache.spark.util.collection.unsafe.sort.UnsafeExternalSorter.spill` calls `org.apache.spark.util.collection.unsafe.sort.UnsafeInMemorySorter.reset` which may trigger another spill, when this happens the `array` member is already de-allocated but still referenced by the code, this causes the nested spill to fail with an NPE in `org.apache.spark.memory.TaskMemoryManager.getPage`. This patch introduces a reproduction in a test case and a fix, the fix simply sets the in-mem sorter's array member to an empty array before actually performing the allocation. This prevents the spilling code from 'touching' the de-allocated array. introduced a new test case: `org.apache.spark.util.collection.unsafe.sort.UnsafeExternalSorterSuite#testOOMDuringSpill`. Author: Eyal Farago <eyal@nrgene.com> Closes apache#19181 from eyalfa/SPARK-21907__oom_during_spill.

back-port #19181 to branch-2.2. ## What changes were proposed in this pull request? 1. a test reproducing [SPARK-21907](https://issues.apache.org/jira/browse/SPARK-21907) 2. a fix for the root cause of the issue. `org.apache.spark.util.collection.unsafe.sort.UnsafeExternalSorter.spill` calls `org.apache.spark.util.collection.unsafe.sort.UnsafeInMemorySorter.reset` which may trigger another spill, when this happens the `array` member is already de-allocated but still referenced by the code, this causes the nested spill to fail with an NPE in `org.apache.spark.memory.TaskMemoryManager.getPage`. This patch introduces a reproduction in a test case and a fix, the fix simply sets the in-mem sorter's array member to an empty array before actually performing the allocation. This prevents the spilling code from 'touching' the de-allocated array. ## How was this patch tested? introduced a new test case: `org.apache.spark.util.collection.unsafe.sort.UnsafeExternalSorterSuite#testOOMDuringSpill`. Author: Eyal Farago <eyal@nrgene.com> Closes #19481 from eyalfa/SPARK-21907__oom_during_spill__BACKPORT-2.2.

back-port apache#19181 to branch-2.2. ## What changes were proposed in this pull request? 1. a test reproducing [SPARK-21907](https://issues.apache.org/jira/browse/SPARK-21907) 2. a fix for the root cause of the issue. `org.apache.spark.util.collection.unsafe.sort.UnsafeExternalSorter.spill` calls `org.apache.spark.util.collection.unsafe.sort.UnsafeInMemorySorter.reset` which may trigger another spill, when this happens the `array` member is already de-allocated but still referenced by the code, this causes the nested spill to fail with an NPE in `org.apache.spark.memory.TaskMemoryManager.getPage`. This patch introduces a reproduction in a test case and a fix, the fix simply sets the in-mem sorter's array member to an empty array before actually performing the allocation. This prevents the spilling code from 'touching' the de-allocated array. ## How was this patch tested? introduced a new test case: `org.apache.spark.util.collection.unsafe.sort.UnsafeExternalSorterSuite#testOOMDuringSpill`. Author: Eyal Farago <eyal@nrgene.com> Closes apache#19481 from eyalfa/SPARK-21907__oom_during_spill__BACKPORT-2.2.

back-port apache#19181 to branch-2.2. 1. a test reproducing [SPARK-21907](https://issues.apache.org/jira/browse/SPARK-21907) 2. a fix for the root cause of the issue. `org.apache.spark.util.collection.unsafe.sort.UnsafeExternalSorter.spill` calls `org.apache.spark.util.collection.unsafe.sort.UnsafeInMemorySorter.reset` which may trigger another spill, when this happens the `array` member is already de-allocated but still referenced by the code, this causes the nested spill to fail with an NPE in `org.apache.spark.memory.TaskMemoryManager.getPage`. This patch introduces a reproduction in a test case and a fix, the fix simply sets the in-mem sorter's array member to an empty array before actually performing the allocation. This prevents the spilling code from 'touching' the de-allocated array. introduced a new test case: `org.apache.spark.util.collection.unsafe.sort.UnsafeExternalSorterSuite#testOOMDuringSpill`. Author: Eyal Farago <eyal@nrgene.com> Closes apache#19481 from eyalfa/SPARK-21907__oom_during_spill__BACKPORT-2.2.

Eyal Farago added 2 commits September 10, 2017 13:05

SPARK-21907__oom_during_spill: introduce a reproducing test.

c9cbe1a

SPARK-21907__oom_during_spill: fix the root cause of this bug, improv…

cc8ccfd

…e test by requiring OOM exception thrown from the reset() method.

SPARK-21907__oom_during_spill: refine test code a bit.

ae7fbc4

eyalfa commented Sep 12, 2017

View reviewed changes

Eyal Farago added 3 commits September 13, 2017 22:43

SPARK-21907__oom_during_spill: rollback LongArray.empty and use null …

ea6d39b

…instead.

SPARK-21907__oom_during_spill: introduce a new test for UnsafeInMemor…

98834e8

…ySorter that demonstrates an NPE when free() is called after an OOM.

SPARK-21907__oom_during_spill: fix the NPE in UnsafeInMemorySorter.fr…

2fd8e91

…ee when called after an OOM.

hvanhovell reviewed Sep 13, 2017

View reviewed changes

Eyal Farago added 3 commits September 14, 2017 09:08

SPARK-21907__oom_during_spill: address comment by @hvanhovell about V…

df16269

…isibleForTesting

SPARK-21907__oom_during_spill: address comment by @hvanhovell and sim…

474a799

…plfy exception related test.

SPARK-21907__oom_during_spill: add documentation according to request…

57f20b7

… by @hvanhovell.

jiangxb1987 reviewed Sep 19, 2017

View reviewed changes

Merge branch 'master' into SPARK-21907__oom_during_spill

48e947d

juliuszsompolski reviewed Sep 29, 2017

View reviewed changes

kiszk reviewed Sep 29, 2017

View reviewed changes

Eyal Farago added 2 commits September 30, 2017 16:18

SPARK-21907__oom_during_spill: address comments by @juliuszsompolski …

e1f2a84

…and @kiszk.

SPARK-21907__oom_during_spill: changed test comment.

e3da029

hvanhovell reviewed Oct 10, 2017

View reviewed changes

hvanhovell approved these changes Oct 10, 2017

View reviewed changes

SPARK-21907__oom_during_spill: address NIT comments by @hvanhovell.

6b901ee

asfgit closed this in 2028e5a Oct 10, 2017

eyalfa mentioned this pull request Oct 12, 2017

[SPARK-21907][CORE][BACKPORT 2.2] oom during spill #19481

Closed

[SPARK-21907][CORE] oom during spill #19181

[SPARK-21907][CORE] oom during spill #19181

Conversation

eyalfa commented Sep 10, 2017

What changes were proposed in this pull request?

How was this patch tested?

eyalfa commented Sep 10, 2017

hvanhovell commented Sep 12, 2017

SparkQA commented Sep 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eyalfa Sep 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hvanhovell commented Sep 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Sep 13, 2017

SparkQA commented Sep 14, 2017

eyalfa commented Sep 14, 2017

hvanhovell commented Sep 14, 2017

hvanhovell commented Sep 14, 2017

juliuszsompolski commented Sep 14, 2017

eyalfa commented Sep 14, 2017

SparkQA commented Sep 14, 2017

juliuszsompolski commented Sep 15, 2017

eyalfa commented Sep 15, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eyalfa commented Sep 20, 2017 • edited Loading

SparkQA commented Sep 20, 2017

eyalfa commented Sep 26, 2017

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

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 Oct 1, 2017

juliuszsompolski commented Oct 10, 2017

hvanhovell Oct 10, 2017 • edited Loading

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

hvanhovell left a comment

Choose a reason for hiding this comment

hvanhovell commented Oct 10, 2017

eyalfa commented Oct 10, 2017

SparkQA commented Oct 10, 2017

hvanhovell commented Oct 10, 2017

eyalfa Sep 13, 2017 •

edited

Loading

eyalfa commented Sep 20, 2017 •

edited

Loading

hvanhovell Oct 10, 2017 •

edited

Loading