[SPARK-4084] Reuse sort key in Sorter #2937

mengxr · 2014-10-24T22:23:51Z

Sorter uses generic-typed key for sorting. When data is large, it creates lots of key objects, which is not efficient. We should reuse the key in Sorter for memory efficiency. This change is part of the petabyte sort implementation from @rxin .

The Sorter class was written in Java and marked package private. So it is only available to org.apache.spark.util.collection. I renamed it to TimSort and add a simple wrapper of it, still called Sorter, in Scala, which is private[spark].

The benchmark code is updated, which now resets the array before each run. Here is the result on sorting primitive Int arrays of size 25 million using Sorter:

[info] - Sorter benchmark for key-value pairs !!! IGNORED !!!
Java Arrays.sort() on non-primitive int array: Took 13237 ms
Java Arrays.sort() on non-primitive int array: Took 13320 ms
Java Arrays.sort() on non-primitive int array: Took 15718 ms
Java Arrays.sort() on non-primitive int array: Took 13283 ms
Java Arrays.sort() on non-primitive int array: Took 13267 ms
Java Arrays.sort() on non-primitive int array: Took 15122 ms
Java Arrays.sort() on non-primitive int array: Took 15495 ms
Java Arrays.sort() on non-primitive int array: Took 14877 ms
Java Arrays.sort() on non-primitive int array: Took 16429 ms
Java Arrays.sort() on non-primitive int array: Took 14250 ms
Java Arrays.sort() on non-primitive int array: (13878 ms first try, 14499 ms average)
Java Arrays.sort() on primitive int array: Took 2683 ms
Java Arrays.sort() on primitive int array: Took 2683 ms
Java Arrays.sort() on primitive int array: Took 2701 ms
Java Arrays.sort() on primitive int array: Took 2746 ms
Java Arrays.sort() on primitive int array: Took 2685 ms
Java Arrays.sort() on primitive int array: Took 2735 ms
Java Arrays.sort() on primitive int array: Took 2669 ms
Java Arrays.sort() on primitive int array: Took 2693 ms
Java Arrays.sort() on primitive int array: Took 2680 ms
Java Arrays.sort() on primitive int array: Took 2642 ms
Java Arrays.sort() on primitive int array: (2948 ms first try, 2691 ms average)
Sorter without key reuse on primitive int array: Took 10732 ms
Sorter without key reuse on primitive int array: Took 12482 ms
Sorter without key reuse on primitive int array: Took 10718 ms
Sorter without key reuse on primitive int array: Took 12650 ms
Sorter without key reuse on primitive int array: Took 10747 ms
Sorter without key reuse on primitive int array: Took 10783 ms
Sorter without key reuse on primitive int array: Took 12721 ms
Sorter without key reuse on primitive int array: Took 10604 ms
Sorter without key reuse on primitive int array: Took 10622 ms
Sorter without key reuse on primitive int array: Took 11843 ms
Sorter without key reuse on primitive int array: (11089 ms first try, 11390 ms average)
Sorter with key reuse on primitive int array: Took 5141 ms
Sorter with key reuse on primitive int array: Took 5298 ms
Sorter with key reuse on primitive int array: Took 5066 ms
Sorter with key reuse on primitive int array: Took 5164 ms
Sorter with key reuse on primitive int array: Took 5203 ms
Sorter with key reuse on primitive int array: Took 5274 ms
Sorter with key reuse on primitive int array: Took 5186 ms
Sorter with key reuse on primitive int array: Took 5159 ms
Sorter with key reuse on primitive int array: Took 5164 ms
Sorter with key reuse on primitive int array: Took 5078 ms
Sorter with key reuse on primitive int array: (5311 ms first try, 5173 ms average)

So with key reuse, it is faster and less likely to trigger GC.

SparkQA · 2014-10-24T22:29:58Z

Test build #22175 has started for PR 2937 at commit b00db4d.

This patch merges cleanly.

aarondav · 2014-10-24T23:07:44Z

core/src/main/scala/org/apache/spark/util/collection/SortDataFormat.scala

+  /**
+   * Returns the sort key for the element at the given index and reuse the input key if possible.
+   */
+  protected def getKey(data: Buffer, pos: Int, reuse: K): K = {


It's somewhat surprising that a universal trait can have a default implementation, but maybe we can convert this to an abstract class to ensure it's still compiled into simple Java bytecode.

It's also funny that Java ignores the protectedness, maybe we can upgrade all actually-public methods to public (that's everything but getKey(data: Buffer, pos: Int), which is only used internally)

SparkQA · 2014-10-24T23:20:44Z

Test build #22175 has finished for PR 2937 at commit b00db4d.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- raise TypeError("Cannot convert a Row class into dict")
- class ShimFileSinkDesc(var dir: String, var tableInfo: TableDesc, var compressed: Boolean)
- class ShimFileSinkDesc(var dir: String, var tableInfo: TableDesc, var compressed: Boolean)
- case class LogInfo(startTime: Long, endTime: Long, path: String)

AmplabJenkins · 2014-10-24T23:20:48Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22175/
Test FAILed.

mengxr · 2014-10-26T03:31:49Z

@aarondav I updated the PR based on your comment. See the description for renaming Sorter.java to TimSort.Java.

SparkQA · 2014-10-26T03:35:09Z

Test build #22230 has started for PR 2937 at commit 5f0d530.

This patch merges cleanly.

SparkQA · 2014-10-26T04:30:34Z

Test build #22230 has finished for PR 2937 at commit 5f0d530.

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

AmplabJenkins · 2014-10-26T04:30:38Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22230/
Test FAILed.

mengxr · 2014-10-26T04:31:34Z

test this please

SparkQA · 2014-10-26T04:34:49Z

Test build #22231 has started for PR 2937 at commit 5f0d530.

This patch merges cleanly.

SparkQA · 2014-10-26T05:50:06Z

Test build #22231 has finished for PR 2937 at commit 5f0d530.

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

AmplabJenkins · 2014-10-26T05:50:09Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22231/
Test PASSed.

SparkQA · 2014-10-27T21:12:27Z

Test build #22307 has started for PR 2937 at commit 7de2efd.

This patch merges cleanly.

SparkQA · 2014-10-27T22:22:57Z

Test build #22307 has finished for PR 2937 at commit 7de2efd.

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

AmplabJenkins · 2014-10-27T22:23:01Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22307/
Test PASSed.

SparkQA · 2014-10-27T23:34:44Z

Test build #22312 has started for PR 2937 at commit 78f2879.

This patch merges cleanly.

SparkQA · 2014-10-28T00:46:09Z

Test build #22312 has finished for PR 2937 at commit 78f2879.

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

SparkQA · 2014-10-28T02:34:42Z

Test build #22322 has started for PR 2937 at commit a72f53c.

This patch merges cleanly.

SparkQA · 2014-10-28T03:52:31Z

Test build #22322 has finished for PR 2937 at commit a72f53c.

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

AmplabJenkins · 2014-10-28T03:52:34Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22322/
Test FAILed.

aarondav · 2014-10-28T04:43:56Z

core/src/main/scala/org/apache/spark/util/Utils.scala

-    val start = System.currentTimeMillis
-    times(numIters)(f)
-    System.currentTimeMillis - start
+  def timeIt(numIters: Int)(f: => Unit, prepare: Option[() => Unit] = None): Long = {


Maybe add that it returns the total time across all iterations (which is not the behavior I expected).

SparkQA · 2014-10-28T04:44:46Z

Test build #22334 has started for PR 2937 at commit 0b7b682.

This patch merges cleanly.

aarondav · 2014-10-28T04:47:19Z

core/src/main/scala/org/apache/spark/util/collection/SortDataFormat.scala

@@ -27,33 +27,51 @@ import scala.reflect.ClassTag
 * Example format: an array of numbers, where each element is also the key.
 * See [[KVArraySortDataFormat]] for a more exciting format.
 *
- * This trait extends Any to ensure it is universal (and thus compiled to a Java interface).
+ * Declaring and instantiating multiple subclasses of this class would prevent JIT inlining


Maybe slight rephrasing of this to something like

Note: Declaring and instantiating multiple subclasses of this class would prevent JIT inlining overridden methods and hence may decrease the shuffle performance.

I just added "Note" since it's sort of a tangent and "may" since it may not be noticeable for non computationally-intensive workloads.

aarondav · 2014-10-28T04:57:46Z

LGTM once it passes travis!

mengxr · 2014-10-28T05:04:49Z

Well, it won't pass travis ...

SparkQA · 2014-10-28T05:25:03Z

Test build #22336 has started for PR 2937 at commit c63927f.

This patch merges cleanly.

SparkQA · 2014-10-28T05:26:11Z

Test build #22336 has finished for PR 2937 at commit c63927f.

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

AmplabJenkins · 2014-10-28T05:26:12Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22336/
Test FAILed.

SparkQA · 2014-10-28T06:04:05Z

Test build #22334 has finished for PR 2937 at commit 0b7b682.

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

AmplabJenkins · 2014-10-28T06:04:08Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22334/
Test PASSed.

mengxr · 2014-10-28T06:06:48Z

@aarondav Does Jenkins count?

SparkQA · 2014-10-28T18:39:45Z

Test build #22365 has started for PR 2937 at commit d73c3d0.

This patch merges cleanly.

SparkQA · 2014-10-28T20:03:15Z

Test build #22365 has finished for PR 2937 at commit d73c3d0.

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

AmplabJenkins · 2014-10-28T20:03:18Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22365/
Test PASSed.

aarondav · 2014-10-28T22:14:18Z

LGTM! Doing the merging.

I forgot to disable the benchmark code in #2937, which increased the Jenkins build time by couple minutes. aarondav Author: Xiangrui Meng <[email protected]> Closes #2990 from mengxr/disable-benchmark and squashes the following commits: c58f070 [Xiangrui Meng] disable benchmark code

rxin and others added 3 commits October 24, 2014 14:24

cherry-pick rxin's commit

464ddce

renaming

cf94e8a

doc and tests

b00db4d

aarondav reviewed Oct 24, 2014
View reviewed changes

mengxr added 2 commits October 25, 2014 20:05

rename Sorter to TimSort and add a Scala wrapper that is private[spark]

6ffbe66

update method modifiers of SortDataFormat

5f0d530

mengxr added 2 commits October 27, 2014 11:43

add prepare to timeIt and update testsin SorterSuite

8626356

update the Sorter benchmark code to be correct

7de2efd

update tests

78f2879

update timeIt

a72f53c

fix mima

0b7b682

aarondav reviewed Oct 28, 2014
View reviewed changes

address comments

d73c3d0

mengxr force-pushed the SPARK-4084 branch from c63927f to d73c3d0 Compare October 28, 2014 18:35

asfgit closed this in 84e5da8 Oct 28, 2014

mengxr mentioned this pull request Oct 29, 2014

[FIX] disable benchmark code #2990

Closed

[SPARK-4084] Reuse sort key in Sorter #2937

[SPARK-4084] Reuse sort key in Sorter #2937

Conversation

mengxr commented Oct 24, 2014

SparkQA commented Oct 24, 2014

aarondav Oct 24, 2014

Choose a reason for hiding this comment

aarondav Oct 24, 2014

Choose a reason for hiding this comment

SparkQA commented Oct 24, 2014

AmplabJenkins commented Oct 24, 2014

mengxr commented Oct 26, 2014

SparkQA commented Oct 26, 2014

SparkQA commented Oct 26, 2014

AmplabJenkins commented Oct 26, 2014

mengxr commented Oct 26, 2014

SparkQA commented Oct 26, 2014

SparkQA commented Oct 26, 2014

AmplabJenkins commented Oct 26, 2014

SparkQA commented Oct 27, 2014

SparkQA commented Oct 27, 2014

AmplabJenkins commented Oct 27, 2014

SparkQA commented Oct 27, 2014

SparkQA commented Oct 28, 2014

SparkQA commented Oct 28, 2014

SparkQA commented Oct 28, 2014

AmplabJenkins commented Oct 28, 2014

aarondav Oct 28, 2014

Choose a reason for hiding this comment

mengxr Oct 28, 2014

Choose a reason for hiding this comment

SparkQA commented Oct 28, 2014

aarondav Oct 28, 2014

Choose a reason for hiding this comment

aarondav commented Oct 28, 2014

mengxr commented Oct 28, 2014

SparkQA commented Oct 28, 2014

SparkQA commented Oct 28, 2014

AmplabJenkins commented Oct 28, 2014

SparkQA commented Oct 28, 2014

AmplabJenkins commented Oct 28, 2014

mengxr commented Oct 28, 2014

SparkQA commented Oct 28, 2014

SparkQA commented Oct 28, 2014

AmplabJenkins commented Oct 28, 2014

aarondav commented Oct 28, 2014