[SPARK-22570][SQL] Avoid to create a lot of global variables by using a local variable with allocation of an object in generated code #19797

kiszk · 2017-11-22T17:07:21Z

What changes were proposed in this pull request?

This PR reduces # of global variables in generated code by replacing a global variable with a local variable with an allocation of an object every time. When a lot of global variables were generated, the generated code may meet 64K constant pool limit.
This PR reduces # of generated global variables in the following three operations:

Cast with String to primitive byte/short/int/long
RegExpReplace
CreateArray

I intentionally leave this part. This is because this variable keeps a class that is dynamically generated. In other word, it is not possible to reuse one class.

How was this patch tested?

Added test cases

kiszk · 2017-11-22T17:09:11Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeCreator.scala

To be honest, I do not know whether to stop reusing an object here is good or not.
@cloud-fan @maropu @viirya I would like to hear your opinions.

SparkQA · 2017-11-22T17:14:29Z

Test build #84113 has finished for PR 19797 at commit 8ba2f59.

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

SparkQA · 2017-11-22T18:11:11Z

Test build #84115 has finished for PR 19797 at commit 7a49c41.

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

cloud-fan · 2017-11-22T19:52:44Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala

what if we create a new wrapper every time?

It would work well, too. We may have some overhead to create a small object IntWrapper and collect it at GC.

shouldn't GC work very well in this case? My concern is, if there is no significant performance benefit, reusing global variables may be too hacky.

I see. If we do not reuse global variables, we can use only a local variable.

viirya · 2017-11-23T03:29:45Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala

Should we worry about name collision?

Do you worry about name collision among different methods? Since the lifetime of this object is very short, I intentionally use the same name for the same type (IntWrapper or LongWrapper) to reuse the object.
We could use different names among different methods.

viirya · 2017-11-23T03:30:57Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala

Add a help method to CodegenContext? Like reuseOrAddMutableState?

viirya · 2017-11-23T03:34:09Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeCreator.scala

This don't really simplify the code since you still have the else block below. It complicates the codes here actually. So I think it is better to be unchanged.

SparkQA · 2017-11-23T04:03:38Z

Test build #84124 has finished for PR 19797 at commit 4d9657a.

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

mgaido91 · 2017-11-23T19:41:58Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeCreator.scala

is there any specific reason why here we are not following the approach used for IntWrapper and LongWrapper above? ie. defining it as a local variable, instead of reusing the same global object?

It is my question. In this case, we may see larger size of an object.
Which one do you suggest?

sorry, I understand only now your comment there. Honestly I may have a partial vision of this then please correct me if I am wrong, but my preference is for local variable for two reasons:

in the case we have a lot of methods and we have inner classes containing the methods, using a variable of the outer class adds (at least) a constant pool entry, while using a local variable doesn't (as per my understanding);

since we reinitialize every time the object with new assignments, I think we are not saving any GC (we are reusing only the pointer basically, which I don't think is a big problem).

What do you think?

SparkQA · 2017-11-23T19:45:05Z

Test build #84133 has finished for PR 19797 at commit 39f1b6d.

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

cloud-fan · 2017-11-27T12:41:17Z

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

let's not do this, it's too hacky and error-prone. We should just create the object every time, or create global variable every time.

I see. I will drop this.

cloud-fan · 2017-11-27T12:43:09Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeCreator.scala

we still create an object array every time here, which doesn't help with the GC. I think we can just create a local variable here.

To make it local variable, I would like to use a new API at #19821.
After merging #19821, I will commit new version.

cloud-fan · 2017-11-27T12:44:37Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/regexpExpressions.scala

seems we can just create the string buffer every time, the overhead is small.

cloud-fan · 2017-11-27T12:49:36Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuite.scala

In the future we may fold global variables to arrays, and these tests become useless. I think a more reliable way is: check the number of global variables in CodegenContext

Could you elaborate this thought?
Do you want to this new check in test cases? Or do you mean we will remove these tests for global variables since we will have new check in another PR?

cloud-fan · 2017-11-28T02:51:59Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/complexTypesSuite.scala

something like this, we have a codegen context, and do codegen. After that, instead of compiling the code to make sure it doesn't fail, we can just check the size of ctx.mutableStates to confirm that we don't add global variables during codegen

SparkQA · 2017-11-29T20:02:59Z

Test build #84314 has finished for PR 19797 at commit 36a330d.

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

SparkQA · 2017-11-30T02:22:16Z

Test build #84324 has finished for PR 19797 at commit 3b8459d.

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

cloud-fan · 2017-11-30T02:42:08Z

...talyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/RegexpExpressionsSuite.scala

    checkEvaluation(nonNullExpr, "num-num", row1)
  }

+  test("SPARK-22570: should not create a lot of instance variables") {


nit: RegExpReplace should not create a lot of global variables

cloud-fan · 2017-11-30T02:43:35Z

...talyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/RegexpExpressionsSuite.scala

+    (1 to N).map(_ => expr.genCode(ctx).code)
+    // four global variables (lastRegex, pattern, lastReplacement, and lastReplacementInUTF8)
+    // are always required
+    assert(ctx.mutableStates.length == 4 * N)


I think we can simplify it to

val ctx = new CodegenContext RegExpReplace(Literal("100"), Literal("(\\d+)"), Literal("num")).genCode(ctx) // ... assert(ctx.mutableStates.length == 4)

cloud-fan · 2017-11-30T02:43:55Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/complexTypesSuite.scala


+  test("SPARK-22570: should not create a lot of instance variables") {
+    val ctx = new CodegenContext
+    (1 to 60000).map(i => CreateArray(Seq(Literal(s"$i"))).genCode(ctx).code)


cloud-fan · 2017-11-30T02:44:23Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/complexTypesSuite.scala

    comparePlans(Optimizer execute query, expected)
  }

+  test("SPARK-22570: should not create a lot of instance variables") {


nit: CreateArray should not create a lot of global variables

cloud-fan · 2017-11-30T02:45:26Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuite.scala

+      (1 to N).map(i => StructField(s"i$i", LongType)).toArray)
+    val input2 = Row.fromSeq((1 to N).map(i => i.toString))
+    val output2 = Row.fromSeq((1 to N).map(i => i.toLong))
+    checkEvaluation(cast(Literal.create(input2, from2), to2), output2)


can we also simplify this test like the other 2?

cloud-fan · 2017-11-30T13:18:01Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuite.scala

+  test("SPARK-22570: Cast should not create a lot of instance variables") {
+    val ctx = new CodegenContext
+    cast("1", IntegerType).genCode(ctx).code
+    cast("2", LongType).genCode(ctx).code


nit: no need to call code

cloud-fan · 2017-11-30T13:18:23Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/complexTypesSuite.scala


+  test("SPARK-22570: CreateArray should not create a lot of global variables") {
+    val ctx = new CodegenContext
+    CreateArray(Seq(Literal(1))).genCode(ctx).code


cloud-fan · 2017-11-30T13:18:51Z

LGTM

viirya · 2017-11-30T13:28:49Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuite.scala

    checkEvaluation(cast(Literal.create(inputOuter, fromOuter), toOuter), outputOuter)
  }
+
+  test("SPARK-22570: Cast should not create a lot of instance variables") {


nit: instance variables -> global variables. To match with other two added tests.

viirya · 2017-11-30T13:30:10Z

...talyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/RegexpExpressionsSuite.scala

 import org.apache.spark.SparkFunSuite
 import org.apache.spark.sql.AnalysisException
 import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.expressions.codegen.{CodeAndComment, CodegenContext, CodeGenerator}


CodeAndComment and CodeGenerator are not used.

viirya · 2017-11-30T13:30:24Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/complexTypesSuite.scala

 import org.apache.spark.sql.catalyst.dsl.expressions._
 import org.apache.spark.sql.catalyst.dsl.plans._
 import org.apache.spark.sql.catalyst.expressions._
+import org.apache.spark.sql.catalyst.expressions.codegen.{CodeAndComment, CodegenContext, CodeGenerator}


viirya · 2017-11-30T13:31:03Z

LGTM

SparkQA · 2017-11-30T15:56:28Z

Test build #84337 has finished for PR 19797 at commit a455ac3.

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

SparkQA · 2017-11-30T17:13:31Z

Test build #84341 has finished for PR 19797 at commit 747e215.

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

cloud-fan · 2017-11-30T18:28:36Z

thanks, merging to master!

kiszk commented Nov 22, 2017

View reviewed changes

cloud-fan reviewed Nov 22, 2017

View reviewed changes

viirya reviewed Nov 23, 2017

View reviewed changes

kiszk force-pushed the SPARK-22570 branch from 4d9657a to 39f1b6d Compare November 23, 2017 16:30

mgaido91 reviewed Nov 23, 2017

View reviewed changes

mgaido91 mentioned this pull request Nov 23, 2017

[SPARK-22520][SQL] Support code generation for large CaseWhen #19752

Closed

cloud-fan reviewed Nov 27, 2017

View reviewed changes

cloud-fan reviewed Nov 28, 2017

View reviewed changes

kiszk added 7 commits November 30, 2017 03:10

initial commit for cast

41a591a

initial commit for regexpreplace

de5cd38

createarray

1046e1d

fix scala style error

13e05b7

fix conflict of variable names

072cdd2

address review comments

d4e4b07

address review comments

36a330d

kiszk force-pushed the SPARK-22570 branch from 39f1b6d to 36a330d Compare November 29, 2017 19:38

kiszk changed the title ~~[SPARK-22570][SQL] Avoid to create a lot of global variables to reuse an object in generated code~~ [SPARK-22570][SQL] Avoid to create a lot of global variables by using a local variable with allocation of an object in generated code Nov 29, 2017

fix test failure of HiveThriftBinaryServerSuite.SPARK-4407

3b8459d

cloud-fan reviewed Nov 30, 2017

View reviewed changes

kiszk added 3 commits November 30, 2017 13:00

fix test failure of HiveThriftBinaryServerSuite.SPARK-4407

420b10d

clear object pointer at the end of lifetime to avoid unnecessary GC

cd4c696

address review comments

a455ac3

cloud-fan reviewed Nov 30, 2017

View reviewed changes

viirya reviewed Nov 30, 2017

View reviewed changes

address review comments

747e215

asfgit closed this in 999ec13 Nov 30, 2017

kiszk mentioned this pull request Dec 6, 2017

[SPARK-22715][SQL] Reuse the same array in CreateNamedStruct #19910

Closed

[SPARK-22570][SQL] Avoid to create a lot of global variables by using a local variable with allocation of an object in generated code #19797

[SPARK-22570][SQL] Avoid to create a lot of global variables by using a local variable with allocation of an object in generated code #19797

Uh oh!

Conversation

kiszk commented Nov 22, 2017 • 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!

kiszk Nov 22, 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!

SparkQA commented Nov 22, 2017

Uh oh!

SparkQA commented Nov 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan Nov 23, 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 Nov 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiszk Nov 23, 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!

mgaido91 Nov 23, 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!

SparkQA commented Nov 23, 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!

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 Nov 29, 2017

Uh oh!

SparkQA commented Nov 30, 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!

cloud-fan Nov 30, 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

kiszk commented Nov 22, 2017 •

edited

Loading

kiszk Nov 22, 2017 •

edited

Loading

cloud-fan Nov 23, 2017 •

edited

Loading

kiszk Nov 23, 2017 •

edited

Loading

mgaido91 Nov 23, 2017 •

edited

Loading

cloud-fan Nov 30, 2017 •

edited

Loading