-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-8443][SQL] Split GenerateMutableProjection Codegen due to JVM Code Size Limits #7076
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
9405680
1b5aa7e
adef95a
b7a7635
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,8 @@ package org.apache.spark.sql.catalyst.expressions.codegen | |
|
|
||
| import org.apache.spark.sql.catalyst.expressions._ | ||
|
|
||
| import scala.collection.mutable.ArrayBuffer | ||
|
|
||
| // MutableProjection is not accessible in Java | ||
| abstract class BaseMutableProjection extends MutableProjection | ||
|
|
||
|
|
@@ -45,10 +47,41 @@ object GenerateMutableProjection extends CodeGenerator[Seq[Expression], () => Mu | |
| else | ||
| ${ctx.setColumn("mutableRow", e.dataType, i, evaluationCode.primitive)}; | ||
| """ | ||
| }.mkString("\n") | ||
| } | ||
| // collect projections into blocks as function has 64kb codesize limit in JVM | ||
| val projectionBlocks = new ArrayBuffer[String]() | ||
| val blockBuilder = new StringBuilder() | ||
| for (projection <- projectionCode) { | ||
| if (blockBuilder.length > 16 * 1000) { | ||
| projectionBlocks.append(blockBuilder.toString()) | ||
| blockBuilder.clear() | ||
| } | ||
| blockBuilder.append(projection) | ||
| } | ||
| projectionBlocks.append(blockBuilder.toString()) | ||
|
|
||
| val (projectionFuns, projectionCalls) = { | ||
| // inline execution if codesize limit was not broken | ||
| if (projectionBlocks.length == 1) { | ||
| ("", projectionBlocks.head) | ||
| } else { | ||
| ( | ||
| projectionBlocks.zipWithIndex.map { case (body, i) => | ||
| s""" | ||
| |private void apply$i(InternalRow i) { | ||
| | $body | ||
| |} | ||
| """.stripMargin | ||
| }.mkString, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similarly, should this be
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to above. The lines of
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SGTM. |
||
| projectionBlocks.indices.map(i => s"apply$i(i);").mkString("\n") | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| val mutableStates = ctx.mutableStates.map { case (javaType, variableName, initialValue) => | ||
| s"private $javaType $variableName = $initialValue;" | ||
| }.mkString("\n ") | ||
|
|
||
| val code = s""" | ||
| public Object generate($exprType[] expr) { | ||
| return new SpecificProjection(expr); | ||
|
|
@@ -75,9 +108,11 @@ object GenerateMutableProjection extends CodeGenerator[Seq[Expression], () => Mu | |
| return (InternalRow) mutableRow; | ||
| } | ||
|
|
||
| $projectionFuns | ||
|
|
||
| public Object apply(Object _i) { | ||
| InternalRow i = (InternalRow) _i; | ||
| $projectionCode | ||
| $projectionCalls | ||
|
|
||
| return mutableRow; | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we insert newlines so that the generated code is slightly more readable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code itself already has a new line before and after. I looked at debug results and the code look reasonably. I'm happy to add an extra newline to be safe in case that assumption changes in the future. Just let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add it later if it's a problem; this seems fine for now, but just wanted to check. Thanks for looking into this.