[SPARK-21720][SQL] Fix 64KB JVM bytecode limit problem with AND or OR #18972

kiszk · 2017-08-17T17:38:59Z

What changes were proposed in this pull request?

This PR changes AND or OR code generation to place condition and then expressions' generated code into separated methods if these size could be large. When the method is newly generated, variables for isNull and value are declared as an instance variable to pass these values (e.g. isNull1409 and value1409) to the callers of the generated method.

This PR resolved two cases:

large code size of left expression
large code size of right expression

How was this patch tested?

Added a new test case into CodeGenerationSuite

SparkQA · 2017-08-17T20:24:10Z

Test build #80793 has finished for PR 18972 at commit b915d41.

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

kiszk · 2017-08-18T01:48:54Z

Jenkins, retest this please

SparkQA · 2017-08-18T04:27:23Z

Test build #80817 has finished for PR 18972 at commit b915d41.

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

viirya · 2017-08-18T04:51:15Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala

@@ -789,6 +789,36 @@ class CodegenContext {
  }

  /**
+   * Wrap the generated code of expression by a function. ev,isNull and ev.value are passed
+   * by global variables


Not arbitrary codes all can be wrapped by this. The codes must be only created from a row object. We should note this in the comment.

SparkQA · 2017-08-18T14:39:36Z

Test build #80845 has finished for PR 18972 at commit 569a8bd.

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

kiszk · 2017-08-18T18:20:37Z

retest this please

SparkQA · 2017-08-18T20:55:32Z

Test build #80853 has finished for PR 18972 at commit 569a8bd.

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

kiszk · 2017-08-23T02:08:18Z

ping @cloud-fan

kiszk · 2017-10-12T11:26:23Z

Jenkins, retest this please

SparkQA · 2017-10-12T13:10:03Z

Test build #82675 has finished for PR 18972 at commit 569a8bd.

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

SparkQA · 2017-10-12T19:54:58Z

Test build #82695 has finished for PR 18972 at commit c9bc395.

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

kiszk · 2017-10-28T09:55:52Z

Jenkins, retest this please

SparkQA · 2017-10-28T13:00:03Z

Test build #83164 has finished for PR 18972 at commit c9bc395.

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

bali0019 · 2017-10-31T16:48:37Z

Hi @kiszk , I have a question on this PR.
Is there any time estimate on when this PR is supposed to merge ?

kiszk · 2017-10-31T22:39:34Z

@bali0019 It depends on the progress of the review.

kiszk · 2017-10-31T22:40:00Z

@cloud-fan could you review this if you have time?

cloud-fan · 2017-11-10T12:21:47Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala

@@ -809,6 +809,36 @@ class CodegenContext {
  }

  /**
+   * Wrap the generated code of expression, which was created from a row object in INPUT_ROW,
+   * by a function. ev,isNull and ev.value are passed by global variables


typo: ev.isNull

cloud-fan · 2017-11-10T12:23:03Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala

+    val funcName = freshName(baseFuncName)
+    val funcBody =
+      s"""
+         |private void $funcName(InternalRow ${INPUT_ROW}) {


does it work with whole stage codegen? the input is not InternalRow but some variable.

Yes, it works only if ctx.currentVars == null.
We will follow to support the whole stage codegen as follow-up in other PRs.

cloud-fan · 2017-11-10T12:29:45Z

sql/core/src/test/scala/org/apache/spark/sql/DataFrameSuite.scala

-        df.filter(filter).count()
-      }.getMessage
-      assert(e.contains("grows beyond 64 KB"))
+      // SPARK-21720 avoids an exception due to JVM code size limit


I think we should create a config for the threshold instead of hardcoding 1024, then we can keep the test case here, by setting the threshold to Long.max.

In general, I agree with you that we should create a config.
Although I create a PR to add a config for a constant in CodeGenerator, it revealed that we need additional (large) work to fix active session management.

Can we introduce a config after fixing active session management?

cloud-fan · 2017-11-10T17:56:26Z

sql/core/src/test/scala/org/apache/spark/sql/DataFrameSuite.scala

@@ -2067,7 +2067,7 @@ class DataFrameSuite extends QueryTest with SharedSQLContext {
      .count
  }

-  testQuietly("SPARK-19372: Filter can be executed w/o generated code due to JVM code size limit") {
+  test("SPARK-19372: Filter can be executed w/o generated code due to JVM code size limit") {


I think this test case becomes invalid as we won't trigger the codegen fallback branch now. Can we just ignore this test and add a TODO to say something about the config?

I see. I will do it on Sunday.

SparkQA · 2017-11-10T20:34:42Z

Test build #83693 has finished for PR 18972 at commit 2f06555.

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

SparkQA · 2017-11-12T14:47:50Z

Test build #83740 has finished for PR 18972 at commit bf35498.

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

This PR changes `AND` or `OR` code generation to place condition and then expressions' generated code into separated methods if these size could be large. When the method is newly generated, variables for `isNull` and `value` are declared as an instance variable to pass these values (e.g. `isNull1409` and `value1409`) to the callers of the generated method. This PR resolved two cases: * large code size of left expression * large code size of right expression Added a new test case into `CodeGenerationSuite` Author: Kazuaki Ishizaki <ishizaki@jp.ibm.com> Closes #18972 from kiszk/SPARK-21720. (cherry picked from commit 9bf696d) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

cloud-fan · 2017-11-12T22:14:58Z

thanks, merging to master/2.2!

…essions ## What changes were proposed in this pull request? A frequently reported issue of Spark is the Java 64kb compile error. This is because Spark generates a very big method and it's usually caused by 3 reasons: 1. a deep expression tree, e.g. a very complex filter condition 2. many individual expressions, e.g. expressions can have many children, operators can have many expressions. 3. a deep query plan tree (with whole stage codegen) This PR focuses on 1. There are already several patches(apache#15620 apache#18972 apache#18641) trying to fix this issue and some of them are already merged. However this is an endless job as every non-leaf expression has this issue. This PR proposes to fix this issue in `Expression.genCode`, to make sure the code for a single expression won't grow too big. According to maropu 's benchmark, no regression is found with TPCDS (thanks maropu !): https://docs.google.com/spreadsheets/d/1K3_7lX05-ZgxDXi9X_GleNnDjcnJIfoSlSCDZcL4gdg/edit?usp=sharing ## How was this patch tested? existing test Author: Wenchen Fan <wenchen@databricks.com> Author: Wenchen Fan <cloud0fan@gmail.com> Closes apache#19767 from cloud-fan/codegen.

This PR changes `AND` or `OR` code generation to place condition and then expressions' generated code into separated methods if these size could be large. When the method is newly generated, variables for `isNull` and `value` are declared as an instance variable to pass these values (e.g. `isNull1409` and `value1409`) to the callers of the generated method. This PR resolved two cases: * large code size of left expression * large code size of right expression Added a new test case into `CodeGenerationSuite` Author: Kazuaki Ishizaki <ishizaki@jp.ibm.com> Closes apache#18972 from kiszk/SPARK-21720. (cherry picked from commit 9bf696d) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

viirya reviewed Aug 18, 2017

View reviewed changes

poplav added a commit to poplav/spark that referenced this pull request Aug 18, 2017

: Ported latest fix apache#18972

f52ea2c

kiszk added 2 commits October 12, 2017 23:54

Initial commit

0bcbe43

address review comment

36e0b76

kiszk force-pushed the SPARK-21720 branch from 569a8bd to 36e0b76 Compare October 12, 2017 17:04

fix test failure of DataFrameSuite.SPARK-19372

c9bc395

cloud-fan requested changes Nov 10, 2017

View reviewed changes

fix typo

2f06555

cloud-fan reviewed Nov 10, 2017

View reviewed changes

address review comment

bf35498

asfgit closed this in 9bf696d Nov 12, 2017

cloud-fan mentioned this pull request Nov 16, 2017

[SPARK-22543][SQL] fix java 64kb compile error for deeply nested expressions #19767

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-21720][SQL] Fix 64KB JVM bytecode limit problem with AND or OR #18972

[SPARK-21720][SQL] Fix 64KB JVM bytecode limit problem with AND or OR #18972

kiszk commented Aug 17, 2017 •

edited

Loading

SparkQA commented Aug 17, 2017

kiszk commented Aug 18, 2017

SparkQA commented Aug 18, 2017

viirya Aug 18, 2017

SparkQA commented Aug 18, 2017

kiszk commented Aug 18, 2017

SparkQA commented Aug 18, 2017

kiszk commented Aug 23, 2017

kiszk commented Oct 12, 2017

SparkQA commented Oct 12, 2017

SparkQA commented Oct 12, 2017

kiszk commented Oct 28, 2017

SparkQA commented Oct 28, 2017

bali0019 commented Oct 31, 2017 •

edited

Loading

kiszk commented Oct 31, 2017

kiszk commented Oct 31, 2017

cloud-fan Nov 10, 2017

cloud-fan Nov 10, 2017

kiszk Nov 10, 2017

cloud-fan Nov 10, 2017

kiszk Nov 10, 2017

cloud-fan Nov 10, 2017

kiszk Nov 10, 2017

SparkQA commented Nov 10, 2017

SparkQA commented Nov 12, 2017

cloud-fan commented Nov 12, 2017

[SPARK-21720][SQL] Fix 64KB JVM bytecode limit problem with AND or OR #18972

[SPARK-21720][SQL] Fix 64KB JVM bytecode limit problem with AND or OR #18972

Conversation

kiszk commented Aug 17, 2017 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Aug 17, 2017

kiszk commented Aug 18, 2017

SparkQA commented Aug 18, 2017

Choose a reason for hiding this comment

SparkQA commented Aug 18, 2017

kiszk commented Aug 18, 2017

SparkQA commented Aug 18, 2017

kiszk commented Aug 23, 2017

kiszk commented Oct 12, 2017

SparkQA commented Oct 12, 2017

SparkQA commented Oct 12, 2017

kiszk commented Oct 28, 2017

SparkQA commented Oct 28, 2017

bali0019 commented Oct 31, 2017 • edited Loading

kiszk commented Oct 31, 2017

kiszk commented Oct 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Nov 10, 2017

SparkQA commented Nov 12, 2017

cloud-fan commented Nov 12, 2017

kiszk commented Aug 17, 2017 •

edited

Loading

bali0019 commented Oct 31, 2017 •

edited

Loading