[SPARK-21603][SQL]The wholestage codegen will be much slower then that is closed when the function is too long by eatoncys · Pull Request #18810 · apache/spark

eatoncys · 2017-08-02T04:56:53Z

What changes were proposed in this pull request?

Close the whole stage codegen when the function lines is longer than the maxlines which will be setted by
spark.sql.codegen.MaxFunctionLength parameter, because when the function is too long , it will not get the JIT optimizing.
A benchmark test result is 10x slower when the generated function is too long :

ignore("max function length of wholestagecodegen") {
val N = 20 << 15

val benchmark = new Benchmark("max function length of wholestagecodegen", N)
def f(): Unit = sparkSession.range(N)
  .selectExpr(
    "id",
    "(id & 1023) as k1",
    "cast(id & 1023 as double) as k2",
    "cast(id & 1023 as int) as k3",
    "case when id > 100 and id <= 200 then 1 else 0 end as v1",
    "case when id > 200 and id <= 300 then 1 else 0 end as v2",
    "case when id > 300 and id <= 400 then 1 else 0 end as v3",
    "case when id > 400 and id <= 500 then 1 else 0 end as v4",
    "case when id > 500 and id <= 600 then 1 else 0 end as v5",
    "case when id > 600 and id <= 700 then 1 else 0 end as v6",
    "case when id > 700 and id <= 800 then 1 else 0 end as v7",
    "case when id > 800 and id <= 900 then 1 else 0 end as v8",
    "case when id > 900 and id <= 1000 then 1 else 0 end as v9",
    "case when id > 1000 and id <= 1100 then 1 else 0 end as v10",
    "case when id > 1100 and id <= 1200 then 1 else 0 end as v11",
    "case when id > 1200 and id <= 1300 then 1 else 0 end as v12",
    "case when id > 1300 and id <= 1400 then 1 else 0 end as v13",
    "case when id > 1400 and id <= 1500 then 1 else 0 end as v14",
    "case when id > 1500 and id <= 1600 then 1 else 0 end as v15",
    "case when id > 1600 and id <= 1700 then 1 else 0 end as v16",
    "case when id > 1700 and id <= 1800 then 1 else 0 end as v17",
    "case when id > 1800 and id <= 1900 then 1 else 0 end as v18")
  .groupBy("k1", "k2", "k3")
  .sum()
  .collect()

benchmark.addCase(s"codegen = F") { iter =>
  sparkSession.conf.set("spark.sql.codegen.wholeStage", "false")
  f()
}

benchmark.addCase(s"codegen = T") { iter =>
  sparkSession.conf.set("spark.sql.codegen.wholeStage", "true")
  sparkSession.conf.set("spark.sql.codegen.MaxFunctionLength", "10000")
  f()
}

benchmark.run()

/*
Java HotSpot(TM) 64-Bit Server VM 1.8.0_111-b14 on Windows 7 6.1
Intel64 Family 6 Model 58 Stepping 9, GenuineIntel
max function length of wholestagecodegen: Best/Avg Time(ms)    Rate(M/s)   Per Row(ns)   Relative
------------------------------------------------------------------------------------------------
codegen = F                                    443 /  507          1.5         676.0       1.0X
codegen = T                                   3279 / 3283          0.2        5002.6       0.1X
 */

}

How was this patch tested?

Run the unit test

kiszk · 2017-08-03T05:25:12Z

+   */
+  def existTooLongFunction(): Boolean = {
+    classFunctions.exists { case (className, functions) =>
+      functions.exists{ case (name, code) =>


Could you elaborate why only the number of characters for a function is a decision factor to enable or disable the whole-stage codegen?

@kiszk Because when the JVM parameter -XX:+DontCompileHugeMethods is true, it can not get the JIT optimization when the byte code of a function is longer than 8000, I just estimate a function lines by 8000 byte code, maybe there are some other good ways.

Got it. Thank you for your explanation. It would be good to add comment for this reasoning.

We have seen the similar control at here. Can we unify these control mechanisms into one?

Does this line counts include code comments in the functions? Do we need to strip comments?

kiszk · 2017-08-03T05:26:12Z

+    .doc("The maximum number of function length that will be supported before" +
+    " deactivating whole-stage codegen.")
+    .intConf
+    .createWithDefault(1500)


Would it be possible to explain why 1500 is the good value as default?

kiszk · 2017-08-03T05:26:55Z

+
+    benchmark.addCase(s"codegen = T") { iter =>
+      sparkSession.conf.set("spark.sql.codegen.wholeStage", "true")
+      sparkSession.conf.set("spark.sql.codegen.MaxFunctionLength", "10000")


Why doesn't this benchmark use the default number 1500?

Ok, I have added a test use the default number 1500, thanks.

gatorsmile · 2017-08-07T05:06:42Z

ok to test

SparkQA · 2017-08-07T05:09:00Z

Test build #80318 has finished for PR 18810 at commit 1b0ac5e.

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

gatorsmile · 2017-08-07T05:41:20Z

+    val existLongFunction = ctx.existTooLongFunction
+    if (existLongFunction) {
+      logWarning(s"Function is too long, Whole-stage codegen disabled for this plan:\n "
+        + s"$treeString")


This could be very big. Please follow what did in #18658

@gatorsmile , thank you for review, the treeString not contains the code, it only contains the tree string of the Physical plan like below:
*HashAggregate(keys=[k1#2395L, k2#2396, k3#2397], functions=[partial_sum(id#2392L)...
+- *Project [id#2392L, (id#2392L & 1023) AS k1#2395L, cast((id#2392L & 1023) as double) AS k2#2396...
+- *Range (0, 655360, step=1, splits=1)
So, I think it will not be very big.

gatorsmile · 2017-08-07T05:44:21Z


+  /**
+   * Returns the length of codegen function  is too long or not
+   */


Adding a checking logics here, instead of returning Boolean?

Ok, I have modified it, thanks.

SparkQA · 2017-08-07T07:04:51Z

Test build #80325 has finished for PR 18810 at commit 7e84753.

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

viirya · 2017-08-07T07:05:57Z

   */
  private val placeHolderToComments = new mutable.HashMap[String, String]

+  /**


nit: Returns if the length ....

nit: Please remove extra space before is too long.

Ok, I have modified it, thanks.

viirya · 2017-08-07T07:16:29Z

+  val WHOLESTAGE_MAX_FUNCTION_LEN = buildConf("spark.sql.codegen.MaxFunctionLength")
+    .internal()
+    .doc("The maximum number of function length that will be supported before" +
+    " deactivating whole-stage codegen.")


nit: add indent here.

viirya · 2017-08-07T07:21:37Z

    val (ctx, cleanedSource) = doCodeGen()
+    val existLongFunction = ctx.existTooLongFunction
+    if (existLongFunction) {
+      logWarning(s"Function is too long, Whole-stage codegen disabled for this plan:\n "


Simply explain why Whole-stage codegen is disabled. Found too long generated codes and JIT optimization might not work. Whole-stage codegen disabled for this plan...

Btw, we can also add log message like You can change the config spark.sql.codegen.MaxFunctionLength to adjust the function length limit.

In case users wants to run whole-stage codegen intentionally.

SparkQA · 2017-08-07T09:57:00Z

Test build #80326 has finished for PR 18810 at commit c4235dc.

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

SparkQA · 2017-08-07T10:36:31Z

Test build #80329 has finished for PR 18810 at commit 52da6b2.

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

eatoncys · 2017-08-08T08:23:58Z

cc @gatorsmile

gatorsmile · 2017-08-09T07:12:43Z

      "disable logging or -1 to apply no limit.")
    .createWithDefault(1000)

+  val WHOLESTAGE_MAX_FUNCTION_LEN = buildConf("spark.sql.codegen.MaxFunctionLength")


MaxFunctionLength -> maxLinesPerFunction

Ok, I have modified it, thanks

gatorsmile · 2017-08-09T07:15:24Z

   */
  private val placeHolderToComments = new mutable.HashMap[String, String]

+  /**


length is misleading. Here, it is just number of lines.

Ok, I have modified it, thanks

gatorsmile · 2017-08-09T07:16:32Z

+   */
+  def existTooLongFunction(): Boolean = {
+    classFunctions.exists { case (className, functions) =>
+      functions.exists{ case (name, code) =>


Like what @viirya said, could you need to check whether it excludes the comments?

Ok, I have modified it to count lines without comments and extra new lines

SparkQA · 2017-08-10T03:49:10Z

Test build #80470 has finished for PR 18810 at commit d0c753a.

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

gatorsmile · 2017-08-10T04:04:58Z

+      logWarning(s"Found too long generated codes and JIT optimization might not work, " +
+        s"Whole-stage codegen disabled for this plan, " +
+        s"You can change the config spark.sql.codegen.MaxFunctionLength " +
+        s"to adjust the function length limit:\n "


Please remove the useless s

gatorsmile · 2017-08-16T16:12:04Z

Thanks! Merging to master.

maropu · 2017-08-20T23:53:32Z

(copied from jira for just-in-case) Just for your information, I checked the performance changes of TPCDS before/after the pr #18810; the pr affected Q17/Q66 only (that is, they have too long codegen'd functions). The changes are as follows (just run TPCDSQueryBenchmark);
Q17: https://github.com/apache/spark/blob/master/sql/core/src/test/resources/tpcds/q17.sql
Q66: https://github.com/apache/spark/blob/master/sql/core/src/test/resources/tpcds/q66.sql

Q17 w/o this pr, 3224.0  --> q17 w/this pr, 2627.0 (perf. improvement)
Q66 w/o this pr, 1712.0 -->  q66 w/this pr, 3032.0 (perf. regression)

It seems their queries have gen'd funcs with 2800~2900 lines, so if we set 2900 at spark.sql.codegen.maxLinesPerFunction, we could keep the previous performance w/o pr18810.

viirya · 2017-08-21T02:30:00Z

@maropu Interesting. Would you like to benchmark with #18931 too? It is my attempt to solve long code-gen functions without disabling it.

maropu · 2017-08-21T02:39:02Z

yea, I'll do

maropu · 2017-08-21T02:48:32Z

Btw, as for merged prs, I'm just monitoring TPCDS perf. in here. Also, I wrote a script before to run TPCDS on pending prs: https://github.com/maropu/spark-tpcds-datagen#helper-scripts-for-benchmarks.

gatorsmile · 2017-08-21T17:32:52Z

Thank you for tracking it! Could you adjust the conf to a higher number (e.g. 4097) and rerun the perf?

maropu · 2017-08-22T00:50:49Z

ok, I'll make a pr as follow-up.

eatoncys added 2 commits August 2, 2017 11:48

The wholestage codegen will be slower when the function is too long

ca9eff6

The wholestage codegen will be slower when the function is too long

1b0ac5e

eatoncys changed the title ~~[SPARK-21603][sql]The wholestage codegen will be much slower then wholestage codegen is closed when the function is too long~~ [SPARK-21603][sql]The wholestage codegen will be much slower then that is closed when the function is too long Aug 2, 2017

kiszk reviewed Aug 3, 2017

View reviewed changes

gatorsmile reviewed Aug 7, 2017

View reviewed changes

eatoncys added 3 commits August 7, 2017 14:49

Modify annotation of existTooLongFunction

5582f00

Modify annotation of existTooLongFunction

7c185c6

Modify annotation of existTooLongFunction

7e84753

viirya reviewed Aug 7, 2017

View reviewed changes

Modify annotation of existTooLongFunction

c4235dc

viirya reviewed Aug 7, 2017

View reviewed changes

Modify annotation of existTooLongFunction

52da6b2

gatorsmile reviewed Aug 9, 2017

View reviewed changes

eatoncys added 3 commits August 10, 2017 09:00

count lines of function without comments

d0c753a

Add benchmark with default value

d3238e9

Add benchmark with default value

d44a2f8

gatorsmile reviewed Aug 10, 2017

View reviewed changes

asfgit closed this in 1cce1a3 Aug 16, 2017

maropu mentioned this pull request Aug 22, 2017

[SPARK-21603][SQL][FOLLOW-UP] Change the default value of maxLinesPerFunction into 4000 #19021

Closed

This was referenced Aug 24, 2017

[SPARK-21603][SQL][FOLLOW-UP] Use -1 to disable maxLinesPerFunction #19031

Closed

[SPARK-21870][SQL] Split aggregation code into small functions #19082

Closed

[SPARK-21871][SQL] Check actual bytecode size when compiling generated code #19083

Closed

juliuszsompolski mentioned this pull request Sep 22, 2017

[SPARK-22103] Move HashAggregateExec parent consume to a separate function in codegen #19324

Closed

maropu mentioned this pull request Oct 7, 2017

[SPARK-22215][SQL] Add configuration to set the threshold for generated class #19447

Closed

dongjoon-hyun mentioned this pull request Jan 30, 2018

[SPARK-23267] [SQL] Increase spark.sql.codegen.hugeMethodLimit to 65535 #20434

Closed

maropu mentioned this pull request Apr 3, 2018

[SPARK-21870][SQL] Split aggregation code into small functions #20965

Closed

Uh oh!

Conversation

eatoncys commented Aug 2, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eatoncys Aug 4, 2017 • 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!

kiszk Aug 4, 2017 • 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!

viirya Aug 7, 2017 • 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!

gatorsmile commented Aug 7, 2017

Uh oh!

SparkQA commented Aug 7, 2017

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 Aug 7, 2017

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 Aug 7, 2017

Uh oh!

SparkQA commented Aug 7, 2017

Uh oh!

eatoncys commented Aug 8, 2017

Uh oh!

gatorsmile Aug 9, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented Aug 16, 2017

Uh oh!

maropu commented Aug 20, 2017

Uh oh!

eatoncys Aug 4, 2017 •

edited

Loading

kiszk Aug 4, 2017 •

edited

Loading

viirya Aug 7, 2017 •

edited

Loading

gatorsmile Aug 9, 2017 •

edited

Loading

gatorsmile commented Aug 21, 2017 •

edited

Loading