[SPARK-15391] [SQL] manage the temporary memory of timsort #13318

davies · 2016-05-26T06:36:37Z

What changes were proposed in this pull request?

Currently, the memory for temporary buffer used by TimSort is always allocated as on-heap without bookkeeping, it could cause OOM both in on-heap and off-heap mode.

This PR will try to manage that by preallocate it together with the pointer array, same with RadixSort. It both works for on-heap and off-heap mode.

This PR also change the loadFactor of BytesToBytesMap to 0.5 (it was 0.70), it enables use to radix sort also makes sure that we have enough memory for timsort.

How was this patch tested?

Existing tests.

davies · 2016-05-26T06:37:02Z

cc @ericl

rxin · 2016-05-26T06:49:16Z

Can we add a unit test for this behavior?

rxin · 2016-05-26T06:52:14Z

FYI "This PR also change the loadFactor of BytesToBytesMap to 0.5 (it was 0.75)" this is a pretty low load factor.

davies · 2016-05-26T07:02:58Z

It was 0.70 (corrected), it's 30% lower after this patch. For the simplest aggregate (one integer key and one integer value), the key-value pair need 40 bytes, the pointer array need 22+ bytes, becomes 32 bytes after the patch, it will need 15% more memory. Before this patch, TimSort still allocate some memory from heap (could OOM), so the difference will be even smaller.

ericl · 2016-05-26T07:29:54Z

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

      newArray.getBaseObject(),
      newArray.getBaseOffset(),
-      array.size() * (8 / memoryAllocationFactor)
+      pos * 8


I think we should keep the memoryAllocationFactor variable, to make it more clear we are overallocating on purpose.

+1 on that.
Is there any particular reason for removing the memoryAllocationFactor? Without this we might highly underutilize memory for TimSort.

In theory, memoryAllocationFactor should be 2 for radix sort and 1.5 for tim sort. For simplicity, I'd like to trade a little bit memory efficiency for simplicity. memoryAllocationFactor is only used in hasSpaceForAnotherRecord(), if you feel strong on this tradeoff, I could update hasSpaceForAnotherRecord() to use 1.5 for timsort.

For here, we don't need memoryAllocationFactor actually.

Should we also use 1/1.5 in BytesToBytesMap as load factor when radix sort could not be used? I feel that's too complicated.

IMO, it will be better to use the memoryAllocationFactor. Because otherwise, we might see unnecessary spilling due to memory under utilization. If we change the memory allocation factor from 2 to 1.5 for TimSort, that is a significant amount of memory considering the size of the pointer array can be in the order of GBs for large workload.

SparkQA · 2016-05-26T08:30:02Z

Test build #59350 has finished for PR 13318 at commit 6d074f6.

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

sitalkedia · 2016-05-26T20:33:37Z

Thank you for making this fix 👍

SparkQA · 2016-05-26T21:00:28Z

Test build #59405 has finished for PR 13318 at commit f651956.

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

SparkQA · 2016-05-26T21:21:59Z

Test build #59418 has finished for PR 13318 at commit c72cdea.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-05-26T23:33:38Z

Test build #3021 has finished for PR 13318 at commit c72cdea.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-05-26T23:50:13Z

Test build #3022 has finished for PR 13318 at commit c72cdea.

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

SparkQA · 2016-05-27T00:15:27Z

Test build #59432 has finished for PR 13318 at commit 2ad469d.

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

ericl · 2016-05-27T00:22:10Z

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

+   * How many records could be inserted, because part of the array should be left for sorting.
   */
-  private int pos = 0;
+  private int capacity = 0;


nit: usableCapacity?

SparkQA · 2016-05-27T06:13:31Z

Test build #59465 has finished for PR 13318 at commit 8b4e033.

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

SparkQA · 2016-05-31T20:55:19Z

Test build #59670 has finished for PR 13318 at commit 2ad8eb8.

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

davies · 2016-05-31T22:12:54Z

@ericl Comments addressed, could you take another look?

sameeragarwal · 2016-06-02T22:12:20Z

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

+
+  private int calcCapacity() {
+    // Radix sort requires same amount of used memory as buffer, Tim sort requires
+    // half of the used memory as buffer.


can we also update the comments above while instantiating array to better indicate the contents of the array (and talk about the additional buffer that's needed for sorting)

by the way, out of curiosity, what's the worst case scenario in TimSort that requires 0.5x more buffer space?

@sameeragarwal I think you already had a test case for the worst case (there are two ordered part in the array, the shortest part will be copied into a temporary buffer, os it need 0.5 buffer space), it's doced in TimSort.

sameeragarwal · 2016-06-02T22:24:05Z

Looks great overall, just few questions and a couple of minor comments.

sameeragarwal · 2016-06-03T18:46:46Z

LGTM

SparkQA · 2016-06-03T21:00:39Z

Test build #59949 has finished for PR 13318 at commit a929a06.

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

## What changes were proposed in this pull request? Currently, the memory for temporary buffer used by TimSort is always allocated as on-heap without bookkeeping, it could cause OOM both in on-heap and off-heap mode. This PR will try to manage that by preallocate it together with the pointer array, same with RadixSort. It both works for on-heap and off-heap mode. This PR also change the loadFactor of BytesToBytesMap to 0.5 (it was 0.70), it enables use to radix sort also makes sure that we have enough memory for timsort. ## How was this patch tested? Existing tests. Author: Davies Liu <[email protected]> Closes #13318 from davies/fix_timsort. (cherry picked from commit 3074f57) Signed-off-by: Davies Liu <[email protected]>

manage the temporary memory of timsort

6d074f6

ericl reviewed May 26, 2016
View reviewed changes

address comments

f651956

rxin mentioned this pull request May 26, 2016

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

Closed

fix tests

c72cdea

avoid overflow

2ad469d

ericl reviewed May 27, 2016
View reviewed changes

fix overflow

8b4e033

Davies Liu added 2 commits May 31, 2016 11:57

Merge branch 'master' of github.com:apache/spark into fix_timsort

891c007

fix new test

2ad8eb8

sameeragarwal reviewed Jun 2, 2016
View reviewed changes

address comments

a929a06

asfgit closed this in 3074f57 Jun 3, 2016

[SPARK-15391] [SQL] manage the temporary memory of timsort #13318

[SPARK-15391] [SQL] manage the temporary memory of timsort #13318

Uh oh!

Conversation

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

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

davies commented May 26, 2016

Uh oh!

rxin commented May 26, 2016

Uh oh!

rxin commented May 26, 2016

Uh oh!

davies commented May 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 26, 2016

Uh oh!

sitalkedia commented May 26, 2016

Uh oh!

SparkQA commented May 26, 2016

Uh oh!

SparkQA commented May 26, 2016

Uh oh!

SparkQA commented May 26, 2016

Uh oh!

SparkQA commented May 26, 2016

Uh oh!

SparkQA commented May 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 27, 2016

Uh oh!

SparkQA commented May 31, 2016

Uh oh!

davies commented May 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sameeragarwal commented Jun 2, 2016

Uh oh!

sameeragarwal commented Jun 3, 2016

Uh oh!

SparkQA commented Jun 3, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

davies commented May 26, 2016 •

edited

Loading