[SPARK-25484][SQL][TEST] Refactor ExternalAppendOnlyUnsafeRowArrayBenchmark #22617

peter-toth · 2018-10-02T20:59:56Z

What changes were proposed in this pull request?

Refactor ExternalAppendOnlyUnsafeRowArrayBenchmark to use main method.

How was this patch tested?

Manually tested and regenerated results.
Please note that spark.memory.debugFill setting has a huge impact on this benchmark. Since it is set to true by default when running the benchmark from SBT, we need to disable it:

SPARK_GENERATE_BENCHMARK_FILES=1 build/sbt ";project sql;set javaOptions in Test += \"-Dspark.memory.debugFill=false\";test:runMain org.apache.spark.sql.execution.ExternalAppendOnlyUnsafeRowArrayBenchmark"

peter-toth · 2018-10-04T09:25:39Z

cc @dongjoon-hyun @seancxmao

dongjoon-hyun · 2018-10-13T20:44:40Z

Retest this please.

SparkQA · 2018-10-13T20:55:49Z

Test build #97348 has finished for PR 22617 at commit f016e20.

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

dongjoon-hyun · 2018-10-16T05:54:13Z

Retest this please.

SparkQA · 2018-10-16T07:05:02Z

Test build #97438 has finished for PR 22617 at commit 1c30755.

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

kiszk · 2018-10-17T18:37:53Z

Retest this please

SparkQA · 2018-10-17T22:09:29Z

Test build #97504 has finished for PR 22617 at commit 1c30755.

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

peter-toth · 2018-10-28T07:59:09Z

@dongjoon-hyun , @kiszk could you please help me how take a step forward with this PR?

dongjoon-hyun · 2018-10-28T22:24:22Z

...rc/test/scala/org/apache/spark/sql/execution/ExternalAppendOnlyUnsafeRowArrayBenchmark.scala

-        val array = new ExternalAppendOnlyUnsafeRowArray(
-          ExternalAppendOnlyUnsafeRowArray.DefaultInitialSizeOfInMemoryBuffer,
-          numSpillThreshold)
+        val array = new ExternalAppendOnlyUnsafeRowArray(numSpillThreshold, numSpillThreshold)


Hi, @peter-toth . Could you explain why we need to replace ExternalAppendOnlyUnsafeRowArray.DefaultInitialSizeOfInMemoryBuffer with numSpillThreshold here? Actually, this is not an obvious refactoring.
If this is related to Fix issue in ExternalAppendOnlyUnsafeRowArray creation, please add some comments here or PR description clearly.

Thanks for the feedback @dongjoon-hyun. I added some details to the description.

Actually, following my logic in the description, I think

val array = new ExternalAppendOnlyUnsafeRowArray(numSpillThreshold, numSpillThreshold)

should be changed to

val array = new ExternalAppendOnlyUnsafeRowArray(0, numSpillThreshold)

in testAgainstRawUnsafeExternalSorter in "WITH SPILL" cases so as to compare ExternalAppendOnlyUnsafeRowArray to UnsafeExternalSorter when it behaves so.

But would be great if someone could confirm this idea.

In that case, we need two-step comparision.

Refactoring only to ensure no regression.

Change that value to check the performance value difference.

Could you rollback this line and let us finish Step 1 first?

Ok. Reverted the change.

wangyum · 2018-11-07T02:30:42Z

Retest this please

gatorsmile · 2019-01-01T06:11:14Z

retest this please

gatorsmile · 2019-01-01T06:13:51Z

Please help review it @dongjoon-hyun @kiszk @wangyum

SparkQA · 2019-01-01T06:18:04Z

Test build #100609 has finished for PR 22617 at commit 3a52abc.

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

dongjoon-hyun · 2019-01-01T06:24:51Z

Sure. @gatorsmile .
Could you rebase this and fix the build error, @peter-toth ?

peter-toth · 2019-01-02T09:15:41Z

@dongjoon-hyun, sure, I will fix it soon.

…ialization

Change-Id: I245c16a77a071c6386ce671a9c4a6d8f8fe3b78d

peter-toth · 2019-01-08T15:10:18Z

@dongjoon-hyun, I rebased my commits and fixed the build issue.

dongjoon-hyun · 2019-01-08T17:30:24Z

Thank you for updating! I'll review today.

dongjoon-hyun · 2019-01-09T07:22:10Z

...rc/test/scala/org/apache/spark/sql/execution/ExternalAppendOnlyUnsafeRowArrayBenchmark.scala

+ * {{{
+ *   1. without sbt:
+ *      bin/spark-submit --class <this class> --jars <spark core test jar> <spark sql test jar>
+ *   2. build/sbt "sql/test:runMain <this class>"


It seems that 2~3 should be the same except SPARK_GENERATE_BENCHMARK_FILES=1.
Also, we need spark.memory.debugFill configuration for 1 (spark-submit).

Actually I'm removing spark.memory.debugFill=true from the configuration of 3 to become similar to 1 (spark-submit). spark.memory.debugFill is false by default and setting it to true adds enormous overhead.
I think I can change it to += \"-Dspark.memory.debugFill=false\" if that better fits here.

dongjoon-hyun · 2019-01-09T07:29:15Z

...rc/test/scala/org/apache/spark/sql/execution/ExternalAppendOnlyUnsafeRowArrayBenchmark.scala

+      val spillThreshold = 100 * 1000
+      testAgainstRawArrayBuffer(spillThreshold, 100 * 1000, 1 << 10)
+      testAgainstRawArrayBuffer(spillThreshold, 1000, 1 << 18)
+      testAgainstRawArrayBuffer(spillThreshold, 30 * 1000, 1 << 14)


~~Let's keep the original sequence; 1000 -> 30 * 1000 -> 100 * 1000. Increasing order is more intuitive.~~
Ah, I got it. This is reordered by the calculation. Please forgot about the above comment.

>>> 1000 * (1<<18) 262144000 >>> 30 * 1000 * (1<<14) 491520000 >>> 100 * 1000 * (1<<10) 102400000

dongjoon-hyun · 2019-01-09T07:42:11Z

sql/core/benchmarks/ExternalAppendOnlyUnsafeRowArrayBenchmark-results.txt

+Spilling with 1000 rows:                 Best/Avg Time(ms)    Rate(M/s)   Per Row(ns)   Relative
+------------------------------------------------------------------------------------------------
+UnsafeExternalSorter                        15829 / 15845         16.6          60.4       1.0X
+ExternalAppendOnlyUnsafeRowArray            10158 / 10174         25.8          38.7       1.6X


It looks meaningfully different from the previous result. Let's see the server result together. I'm running this.

dongjoon-hyun · 2019-01-09T08:15:20Z

@peter-toth . What do you mean by disable? The command and comment are using spark.memory.debugFill=true.

Please note that spark.memory.debugFill setting has a huge impact on this benchmark. Since it is set to true by default when running the benchmark from SBT, we need to disable it:

It seems that we need to update the command in the PR description and in the comment; spark.memory.debugFill=false.

peter-toth · 2019-01-09T08:21:05Z

I think I should change it, it is a bit confusing now. I used Test -= ... to disable it.

dongjoon-hyun · 2019-01-09T08:23:53Z

If I changed to false, the result looks very different.

$ SPARK_GENERATE_BENCHMARK_FILES=1 build/sbt ";project sql;set javaOptions in Test -= \"-Dspark.memory.debugFill=false\";test:runMain org.apache.spark.sql.execution.ExternalAppendOnlyUnsafeRowArrayBenchmark"
...
[info] OpenJDK 64-Bit Server VM 1.8.0_191-b12 on Linux 3.10.0-862.3.2.el7.x86_64
[info] Intel(R) Xeon(R) CPU E5-2670 v2 @ 2.50GHz
[info] Array with 100000 rows:                  Best/Avg Time(ms)    Rate(M/s)   Per Row(ns)   Relative
[info] ------------------------------------------------------------------------------------------------
[info] ArrayBuffer                                   6653 / 6916         15.4          65.0       1.0X
[info] ExternalAppendOnlyUnsafeRowArray            25856 / 25968          4.0         252.5       0.3X

dongjoon-hyun · 2019-01-09T08:27:49Z

...rc/test/scala/org/apache/spark/sql/execution/ExternalAppendOnlyUnsafeRowArrayBenchmark.scala

 *   3. generate result: SPARK_GENERATE_BENCHMARK_FILES=1 build/sbt ";project sql;set javaOptions
- *        in Test -= \"-Dspark.memory.debugFill=true\";test:runMain <this class>"
+ *        in Test += \"-Dspark.memory.debugFill=false\";test:runMain <this class>"


Got it. I was confused with -=.

sorry, i did it a bit confusing way, but updated now to += ...=false in a new commit

Ur, in PR description, runMain is repeated twice; test:runMain test:runMain

dongjoon-hyun · 2019-01-09T09:14:01Z

sql/core/benchmarks/ExternalAppendOnlyUnsafeRowArrayBenchmark-results.txt

+Array with 1000 rows:                    Best/Avg Time(ms)    Rate(M/s)   Per Row(ns)   Relative
+------------------------------------------------------------------------------------------------
+ArrayBuffer                                   8839 / 8951         29.7          33.7       1.0X
+ExternalAppendOnlyUnsafeRowArray              9884 / 9888         26.5          37.7       0.9X


Could you run this once more in your side? For me, I've got the followings. The ratio difference is too big.

Mac

[info] Java HotSpot(TM) 64-Bit Server VM 1.8.0_191-b12 on Mac OS X 10.14.2 [info] Intel(R) Core(TM) i7-4770HQ CPU @ 2.20GHz [info] Array with 1000 rows: Best/Avg Time(ms) Rate(M/s) Per Row(ns) Relative [info] ------------------------------------------------------------------------------------------------ [info] ArrayBuffer 10226 / 10272 25.6 39.0 1.0X [info] ExternalAppendOnlyUnsafeRowArray 24301 / 24425 10.8 92.7 0.4X

EC2 Server

[info] OpenJDK 64-Bit Server VM 1.8.0_191-b12 on Linux 3.10.0-862.3.2.el7.x86_64 [info] Intel(R) Xeon(R) CPU E5-2670 v2 @ 2.50GHz [info] Array with 1000 rows: Best/Avg Time(ms) Rate(M/s) Per Row(ns) Relative [info] ------------------------------------------------------------------------------------------------ [info] ArrayBuffer 11988 / 12027 21.9 45.7 1.0X [info] ExternalAppendOnlyUnsafeRowArray 37480 / 37574 7.0 143.0 0.3X

The is the only difference.

I'm rerunning it soon

Got the same ratio as you have this time:

[info] Java HotSpot(TM) 64-Bit Server VM 1.8.0_162-b12 on Mac OS X 10.13.6 [info] Intel(R) Core(TM) i7-4870HQ CPU @ 2.50GHz [info] Array with 1000 rows: Best/Avg Time(ms) Rate(M/s) Per Row(ns) Relative [info] ------------------------------------------------------------------------------------------------ [info] ArrayBuffer 10028 / 10197 26.1 38.3 1.0X [info] ExternalAppendOnlyUnsafeRowArray 30053 / 30312 8.7 114.6 0.3X

Thank you for confirmation. Master branch seems to be changed.

dongjoon-hyun · 2019-01-09T09:18:36Z

I made a PR to you, @peter-toth .

EC2 result peter-toth/spark#3 (EC2 result).

And, for the record, the following is Mac result. Most stuffs are consistent with this PR except Array with 1000 rows.

================================================================================================
WITHOUT SPILL
================================================================================================

Java HotSpot(TM) 64-Bit Server VM 1.8.0_191-b12 on Mac OS X 10.14.2
Intel(R) Core(TM) i7-4770HQ CPU @ 2.20GHz
Array with 100000 rows:                  Best/Avg Time(ms)    Rate(M/s)   Per Row(ns)   Relative
------------------------------------------------------------------------------------------------
ArrayBuffer                                   5071 / 5178         20.2          49.5       1.0X
ExternalAppendOnlyUnsafeRowArray              5564 / 5583         18.4          54.3       0.9X

Java HotSpot(TM) 64-Bit Server VM 1.8.0_191-b12 on Mac OS X 10.14.2
Intel(R) Core(TM) i7-4770HQ CPU @ 2.20GHz
Array with 1000 rows:                    Best/Avg Time(ms)    Rate(M/s)   Per Row(ns)   Relative
------------------------------------------------------------------------------------------------
ArrayBuffer                                 10146 / 10169         25.8          38.7       1.0X
ExternalAppendOnlyUnsafeRowArray            24099 / 24414         10.9          91.9       0.4X

Java HotSpot(TM) 64-Bit Server VM 1.8.0_191-b12 on Mac OS X 10.14.2
Intel(R) Core(TM) i7-4770HQ CPU @ 2.20GHz
Array with 30000 rows:                   Best/Avg Time(ms)    Rate(M/s)   Per Row(ns)   Relative
------------------------------------------------------------------------------------------------
ArrayBuffer                                 22738 / 22748         21.6          46.3       1.0X
ExternalAppendOnlyUnsafeRowArray            28000 / 28096         17.6          57.0       0.8X


================================================================================================
WITH SPILL
================================================================================================

Java HotSpot(TM) 64-Bit Server VM 1.8.0_191-b12 on Mac OS X 10.14.2
Intel(R) Core(TM) i7-4770HQ CPU @ 2.20GHz
Spilling with 1000 rows:                 Best/Avg Time(ms)    Rate(M/s)   Per Row(ns)   Relative
------------------------------------------------------------------------------------------------
UnsafeExternalSorter                        18076 / 18179         14.5          69.0       1.0X
ExternalAppendOnlyUnsafeRowArray            11861 / 11872         22.1          45.2       1.5X

Java HotSpot(TM) 64-Bit Server VM 1.8.0_191-b12 on Mac OS X 10.14.2
Intel(R) Core(TM) i7-4770HQ CPU @ 2.20GHz
Spilling with 10000 rows:                Best/Avg Time(ms)    Rate(M/s)   Per Row(ns)   Relative
------------------------------------------------------------------------------------------------
UnsafeExternalSorter                             7 /    7         23.1          43.3       1.0X
ExternalAppendOnlyUnsafeRowArray                 7 /    8         22.3          44.8       1.0X

EC2 result

dongjoon-hyun · 2019-01-09T11:42:47Z

...rc/test/scala/org/apache/spark/sql/execution/ExternalAppendOnlyUnsafeRowArrayBenchmark.scala

+ *   1. without sbt:
+ *      bin/spark-submit --class <this class> --jars <spark core test jar> <spark sql test jar>
+ *   2. build/sbt build/sbt ";project sql;set javaOptions
+  *  *    in Test += \"-Dspark.memory.debugFill=false\";test:runMain <this class>"


Could you fix * *?

Change-Id: I8bcecad2863c97091d8bfb4c65386a59051938c1

dongjoon-hyun · 2019-01-09T12:15:05Z

sql/core/benchmarks/ExternalAppendOnlyUnsafeRowArrayBenchmark-results.txt

+------------------------------------------------------------------------------------------------
+UnsafeExternalSorter                            11 /   11         14.8          67.4       1.0X
+ExternalAppendOnlyUnsafeRowArray                 9 /    9         17.6          56.8       1.2X
+


I ran the original master branch and get the following. Since the trend is the same, this refactoring PR looks safe.

$ bin/spark-submit --class org.apache.spark.sql.execution.ExternalAppendOnlyUnsafeRowArrayBenchmark --jars core/target/scala-2.12/spark-core_2.12-3.0.0-SNAPSHOT-tests.jar sql/core/target/scala-2.12/spark-sql_2.12-3.0.0-SNAPSHOT-tests.jar ... Array with 1000 rows: Best/Avg Time(ms) Rate(M/s) Per Row(ns) Relative ------------------------------------------------------------------------------------------------ ArrayBuffer 9556 / 9633 27.4 36.5 1.0X ExternalAppendOnlyUnsafeRowArray 18514 / 18700 14.2 70.6 0.5X Array with 30000 rows: Best/Avg Time(ms) Rate(M/s) Per Row(ns) Relative ------------------------------------------------------------------------------------------------ ArrayBuffer 22180 / 22195 22.2 45.1 1.0X ExternalAppendOnlyUnsafeRowArray 24254 / 24331 20.3 49.3 0.9X Array with 100000 rows: Best/Avg Time(ms) Rate(M/s) Per Row(ns) Relative ------------------------------------------------------------------------------------------------ ArrayBuffer 4998 / 5052 20.5 48.8 1.0X ExternalAppendOnlyUnsafeRowArray 4778 / 4821 21.4 46.7 1.0X Spilling with 1000 rows: Best/Avg Time(ms) Rate(M/s) Per Row(ns) Relative ------------------------------------------------------------------------------------------------ UnsafeExternalSorter 17536 / 17596 14.9 66.9 1.0X ExternalAppendOnlyUnsafeRowArray 10380 / 10451 25.3 39.6 1.7X Spilling with 10000 rows: Best/Avg Time(ms) Rate(M/s) Per Row(ns) Relative ------------------------------------------------------------------------------------------------ UnsafeExternalSorter 6 / 7 25.3 39.5 1.0X ExternalAppendOnlyUnsafeRowArray 6 / 7 26.3 38.0 1.0X

dongjoon-hyun

+1, LGTM. (Pending Jenkins).

dongjoon-hyun · 2019-01-09T12:16:52Z

Thank you so much, @peter-toth .

dongjoon-hyun · 2019-01-09T12:17:13Z

Retest this please.

peter-toth · 2019-01-09T13:04:51Z

Thanks for the review @dongjoon-hyun.

SparkQA · 2019-01-09T15:58:25Z

Test build #100966 has finished for PR 22617 at commit b0d829e.

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

dongjoon-hyun · 2019-01-09T17:55:54Z

Merged to master.

…chmark ## What changes were proposed in this pull request? Refactor ExternalAppendOnlyUnsafeRowArrayBenchmark to use main method. ## How was this patch tested? Manually tested and regenerated results. Please note that `spark.memory.debugFill` setting has a huge impact on this benchmark. Since it is set to true by default when running the benchmark from SBT, we need to disable it: ``` SPARK_GENERATE_BENCHMARK_FILES=1 build/sbt ";project sql;set javaOptions in Test += \"-Dspark.memory.debugFill=false\";test:runMain org.apache.spark.sql.execution.ExternalAppendOnlyUnsafeRowArrayBenchmark" ``` Closes apache#22617 from peter-toth/SPARK-25484. Lead-authored-by: Peter Toth <peter.toth@gmail.com> Co-authored-by: Peter Toth <ptoth@hortonworks.com> Co-authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

peter-toth changed the title ~~[SPARK-25484][TEST] Refactor ExternalAppendOnlyUnsafeRowArrayBenchmark~~ [SPARK-25484][SQL][TEST] Refactor ExternalAppendOnlyUnsafeRowArrayBenchmark Oct 11, 2018

peter-toth force-pushed the SPARK-25484 branch from f016e20 to 1c30755 Compare October 14, 2018 07:26

wangyum mentioned this pull request Oct 26, 2018

[SPARK-25846][SQL][TEST] Refactor ExternalAppendOnlyUnsafeRowArrayBenchmark to use main method #22842

Closed

dongjoon-hyun reviewed Oct 28, 2018

View reviewed changes

peter-toth force-pushed the SPARK-25484 branch 2 times, most recently from 41c202e to 3a52abc Compare October 30, 2018 20:47

peter-toth and others added 5 commits January 8, 2019 15:37

[SPARK-25484][TEST] Refactor ExternalAppendOnlyUnsafeRowArrayBenchmark

5bc905a

[SPARK-25484][TEST] some code deduplication, fix slow sbt benchmark run

93892de

[SPARK-25484][SQL][TEST] rebase on master and fix

a26dbc7

[SPARK-25484][SQL][TEST] revert ExternalAppendOnlyUnsafeRowArray init…

0f676bc

…ialization

[SPARK-25484][SQL][TEST] rebase and fix build issue

0b04fa3

Change-Id: I245c16a77a071c6386ce671a9c4a6d8f8fe3b78d

peter-toth force-pushed the SPARK-25484 branch from 3a52abc to 0b04fa3 Compare January 8, 2019 15:08

dongjoon-hyun reviewed Jan 9, 2019

View reviewed changes

[SPARK-25484][SQL][TEST] fix review findings

702e61c

dongjoon-hyun reviewed Jan 9, 2019

View reviewed changes

EC2 result

87c4180

Merge pull request #3 from dongjoon-hyun/PR-22617

411174a

EC2 result

dongjoon-hyun reviewed Jan 9, 2019

View reviewed changes

[SPARK-25484][SQL][TEST] fix comment

b0d829e

Change-Id: I8bcecad2863c97091d8bfb4c65386a59051938c1

dongjoon-hyun reviewed Jan 9, 2019

View reviewed changes

dongjoon-hyun approved these changes Jan 9, 2019

View reviewed changes

asfgit closed this in 49c062b Jan 9, 2019

[SPARK-25484][SQL][TEST] Refactor ExternalAppendOnlyUnsafeRowArrayBenchmark #22617

[SPARK-25484][SQL][TEST] Refactor ExternalAppendOnlyUnsafeRowArrayBenchmark #22617

Uh oh!

Conversation

peter-toth commented Oct 2, 2018 • 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!

peter-toth commented Oct 4, 2018

Uh oh!

dongjoon-hyun commented Oct 13, 2018

Uh oh!

SparkQA commented Oct 13, 2018

Uh oh!

dongjoon-hyun commented Oct 16, 2018

Uh oh!

SparkQA commented Oct 16, 2018

Uh oh!

kiszk commented Oct 17, 2018

Uh oh!

SparkQA commented Oct 17, 2018

Uh oh!

peter-toth commented Oct 28, 2018

Uh oh!

dongjoon-hyun Oct 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

wangyum commented Nov 7, 2018

Uh oh!

gatorsmile commented Jan 1, 2019

Uh oh!

gatorsmile commented Jan 1, 2019

Uh oh!

SparkQA commented Jan 1, 2019

Uh oh!

dongjoon-hyun commented Jan 1, 2019

Uh oh!

peter-toth commented Jan 2, 2019

Uh oh!

peter-toth commented Jan 8, 2019

Uh oh!

dongjoon-hyun commented Jan 8, 2019

Uh oh!

dongjoon-hyun Jan 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Jan 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Jan 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peter-toth commented Jan 9, 2019

Uh oh!

dongjoon-hyun commented Jan 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

peter-toth commented Oct 2, 2018 •

edited

Loading

dongjoon-hyun Oct 28, 2018 •

edited

Loading

dongjoon-hyun Jan 9, 2019 •

edited

Loading

dongjoon-hyun Jan 9, 2019 •

edited

Loading

dongjoon-hyun commented Jan 9, 2019 •

edited

Loading

dongjoon-hyun commented Jan 9, 2019 •

edited

Loading

peter-toth Jan 9, 2019 •

edited

Loading

dongjoon-hyun commented Jan 9, 2019 •

edited

Loading