[SPARK-13850] Force the sorter to Spill when number of elements in th… #13107

sitalkedia · 2016-05-13T20:29:52Z

What changes were proposed in this pull request?

Force the sorter to Spill when number of elements in the pointer array reach a certain size. This is to workaround the issue of timSort failing on large buffer size.

How was this patch tested?

Tested by running a job which was failing without this change due to TimSort bug.

sitalkedia · 2016-05-13T20:33:43Z

cc- @srowen, @JoshRosen

HyukjinKwon · 2016-05-14T00:15:41Z

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

(It seems imports might have to be reordered, https://cwiki.apache.org/confluence/display/SPARK/Spark+Code+Style+Guide#SparkCodeStyleGuide-Imports)

yhuai · 2016-05-17T03:50:49Z

What is the root cause? Can you also add a regression test?

sitalkedia · 2016-05-17T15:52:23Z

I am not 100% sure of the root cause, but I suspect this is happening when JVM is trying to allocate very large size buffer for pointer array. The issue might be because the JVM is not able to allocate large buffer in contiguous memory location on heap and since the unsafe operations assume contiguous memory location of the objects, any unsafe operation on large buffer results in memory corruption which manifests as TimSort issue.

Unfortunately, this issue is not reproducible consistently and I am not sure of the root cause. So I am not sure how can we have a regression test for it.

Also, please note that this change itself is a no-op unless you override the default value of numElementsForSpillThreshold, which is Long.MAX_VALUE.

rxin · 2016-05-24T22:53:27Z

Should we have a default value that's not Long.MAX_VALUE for this? What values do you guys typically set?

davies · 2016-05-25T05:27:19Z

TimSort require a temporary buffer to store the shorter part, which could be half of the size of pointer array in worst case. This depends on the original order of rows, it's pretty hard to reproduce. I hit that twice and have a patch, but can't reproduce it anymore (without the patch).

The better solution should be only use 2/3 of the pointer array, left 1/3 as temporary buffer for TimSort. We had done similar things for RadixSort, that will use 1/2 for pointer array for insert, another 1/2 will be used as temporary buffer.

sitalkedia · 2016-05-25T16:55:34Z

@rxin - We have seen this issue go away when we limit the pointer array size within 1G. Updated the PR to set that as a default value.

@davies - There is another issue with the allocation of temporary buffer you mentioned. That buffer is not being managed by the MemoryManager and we often see executor OOM because of that. I have opened a JIRA (SPARK-15391) for this. Would be great to have that issue fixed.

rxin · 2016-05-26T20:53:37Z

Do we still need this pull request given the other two patches?

#13336

#13318

davies · 2016-05-26T21:02:16Z

@rxin After fixing those two, we still have some other limits (the number of elements should be less than 512 mm), especially for on-heap mode. There are:

the largest memory block is 8G in on-heap mode, so the number of elements should be less than 512M.
for both Radix sort and time sort, the underlying array could not 2G (or overflow), the number of records should be less than 1G
For sorted iterator, the underlying array could not be larger than 2G.

So we still need to check the number of elements, then do spilling, or the job could fail in unexpected way.

sitalkedia · 2016-06-06T15:36:05Z

@rxin - In addition to @davies's point. This feature also controls largest contiguous memory block allocated on heap which is very useful to avoid OOM when operating on large data set. We have been seeing this issue of executor OOM due to failure to allocate large amount of contiguous buffer in memory due to defragmentation.

davies · 2016-06-06T17:03:20Z

@sitalkedia I think it will trigger Full GC and eventually spilling in that case (see https://github.com/apache/spark/blob/v1.6.1/core/src/main/java/org/apache/spark/memory/TaskMemoryManager.java) , could you provide more information on that (stacktrace or logging)?

davies · 2016-06-06T17:05:10Z

core/src/main/java/org/apache/spark/shuffle/sort/ShuffleExternalSorter.java

That bug is fixed, could you update the comment (or changing 1G to 8G)?

sitalkedia · 2016-06-27T16:00:01Z

@davies - Can you take a look?

davies · 2016-06-27T17:35:02Z

core/src/main/java/org/apache/spark/shuffle/sort/ShuffleExternalSorter.java

This one should be 1024 * 1024 * 1024 (8G)

I see, fixed that, thanks

SparkQA · 2016-06-27T18:44:29Z

Test build #3133 has finished for PR 13107 at commit c5f5a69.

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

SparkQA · 2016-06-28T10:55:27Z

Test build #61372 has finished for PR 13107 at commit c5f5a69.

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

sitalkedia · 2016-06-28T22:31:30Z

Fixed test cases.

SparkQA · 2016-06-28T22:40:39Z

Test build #61416 has finished for PR 13107 at commit ed657a3.

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

SparkQA · 2016-06-29T00:24:00Z

Test build #61423 has finished for PR 13107 at commit cab4357.

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

…e pointer array reach a certain size. This is to workaround the issue of timSort failing on large buffer size.

SparkQA · 2016-06-29T22:52:50Z

Test build #61484 has finished for PR 13107 at commit 11dc238.

This patch fails from timeout after a configured wait of 250m.
This patch merges cleanly.
This patch adds no public classes.

sitalkedia · 2016-06-30T15:33:50Z

jenkins retest this please.

SparkQA · 2016-06-30T17:32:00Z

Test build #61549 has finished for PR 13107 at commit 11dc238.

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

sitalkedia · 2016-06-30T17:42:59Z

@davies - Addressed all comments and fixed test cases.

davies · 2016-06-30T17:51:59Z

LGTM,
Merging this into master and 2.0, thanks!

Force the sorter to Spill when number of elements in the pointer array reach a certain size. This is to workaround the issue of timSort failing on large buffer size. Tested by running a job which was failing without this change due to TimSort bug. Author: Sital Kedia <[email protected]> Closes #13107 from sitalkedia/fix_TimSort. (cherry picked from commit 07f46af) Signed-off-by: Davies Liu <[email protected]>

HyukjinKwon reviewed May 14, 2016
View reviewed changes

sitalkedia force-pushed the fix_TimSort branch from ab601e3 to 7cfe644 Compare May 25, 2016 16:48

davies reviewed Jun 6, 2016
View reviewed changes

sitalkedia force-pushed the fix_TimSort branch 2 times, most recently from e25e8af to c5f5a69 Compare June 16, 2016 17:43

davies reviewed Jun 27, 2016
View reviewed changes

sitalkedia force-pushed the fix_TimSort branch from c5f5a69 to ed657a3 Compare June 28, 2016 22:30

sitalkedia force-pushed the fix_TimSort branch from ed657a3 to cab4357 Compare June 29, 2016 00:18

[SPARK-13850] Force the sorter to Spill when number of elements in th…

11dc238

…e pointer array reach a certain size. This is to workaround the issue of timSort failing on large buffer size.

sitalkedia force-pushed the fix_TimSort branch from cab4357 to 11dc238 Compare June 29, 2016 18:40

asfgit closed this in 07f46af Jun 30, 2016

[SPARK-13850] Force the sorter to Spill when number of elements in th… #13107

[SPARK-13850] Force the sorter to Spill when number of elements in th… #13107

Uh oh!

Conversation

sitalkedia commented May 13, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

sitalkedia commented May 13, 2016

Uh oh!

HyukjinKwon May 14, 2016

Choose a reason for hiding this comment

Uh oh!

yhuai commented May 17, 2016

Uh oh!

sitalkedia commented May 17, 2016

Uh oh!

rxin commented May 24, 2016

Uh oh!

davies commented May 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sitalkedia commented May 25, 2016

Uh oh!

rxin commented May 26, 2016

Uh oh!

davies commented May 26, 2016

Uh oh!

sitalkedia commented Jun 6, 2016

Uh oh!

davies commented Jun 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davies Jun 6, 2016

Choose a reason for hiding this comment

Uh oh!

sitalkedia Jun 16, 2016

Choose a reason for hiding this comment

Uh oh!

sitalkedia commented Jun 27, 2016

Uh oh!

davies Jun 27, 2016

Choose a reason for hiding this comment

Uh oh!

sitalkedia Jun 28, 2016

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 27, 2016

Uh oh!

SparkQA commented Jun 28, 2016

Uh oh!

sitalkedia commented Jun 28, 2016

Uh oh!

SparkQA commented Jun 28, 2016

Uh oh!

SparkQA commented Jun 29, 2016

Uh oh!

SparkQA commented Jun 29, 2016

Uh oh!

sitalkedia commented Jun 30, 2016

Uh oh!

SparkQA commented Jun 30, 2016

Uh oh!

sitalkedia commented Jun 30, 2016

Uh oh!

davies commented Jun 30, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

davies commented May 25, 2016 •

edited

Loading

davies commented Jun 6, 2016 •

edited

Loading