MaxGekk commented on code in PR #59042:
URL: https://github.com/apache/spark/pull/59042#discussion_r4108715201
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala:
##########
@@ -1248,7 +1248,67 @@ class CodegenContext extends Logging {
makeSplitFunction,
foldFunctions)
- foldFunctions(outerClassFunctionCalls ++ innerClassFunctionCalls)
+ foldFunctions(groupFunctionCalls(
+ (outerClassFunctionCalls ++ innerClassFunctionCalls).toSeq,
+ func,
+ arguments,
+ returnType,
+ makeSplitFunction,
+ foldFunctions))
+ }
+ }
+
+ /**
+ * The calls to the functions `splitExpressions` split out, grouped into
functions of their own
Review Comment:
Right, my edit left it detached. Moved it above `groupFunctionCalls` in
71d702dccdd.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala:
##########
@@ -1248,7 +1248,67 @@ class CodegenContext extends Logging {
makeSplitFunction,
foldFunctions)
- foldFunctions(outerClassFunctionCalls ++ innerClassFunctionCalls)
+ foldFunctions(groupFunctionCalls(
+ (outerClassFunctionCalls ++ innerClassFunctionCalls).toSeq,
+ func,
+ arguments,
+ returnType,
+ makeSplitFunction,
+ foldFunctions))
+ }
+ }
+
+ /**
+ * The calls to the functions `splitExpressions` split out, grouped into
functions of their own
+ * where there are too many for the calling method to be JIT-compiled, level
by level.
+ *
+ * The calls are what is left in the calling method, one per block of code,
and a `CASE WHEN`
+ * of a thousand branches has hundreds of blocks: their calls alone take the
caller past the
+ * 8000 bytes HotSpot compiles
(`CodeGenerator.DEFAULT_JVM_HUGE_METHOD_LIMIT`), so the method
+ * every row goes through runs interpreted. A call and its fold compile to
at most about 20 bytes
+ * plus 2 for each argument - the invocation, a load per argument, and the
fold's store, test
+ * and branch - so the calls are left as they are while they fit in half of
that limit, the rest
+ * being the caller's own code, and grouped otherwise. The groups are small,
of
+ * `CodeGenerator.SPLIT_CALLS_PER_GROUP` calls, so that C2 can inline the
split functions into
+ * each group within its inlining budget; a group of hundreds of calls stays
compilable but has
+ * most of its calls left out of inlining, and runs slower. A group is
folded with
+ * `foldFunctions` and wrapped with `makeSplitFunction`, which is what
+ * `generateInnerClassesFunctionCalls` does with the functions of one inner
class, so every
+ * caller of `splitExpressions` supports it.
+ */
+ /**
+ * How many calls to split functions taking `arguments` fit in one method
with room to spare
+ * below the 8000 bytes HotSpot compiles; see `groupFunctionCalls`.
+ */
+ private def splitCallsPerMethod(arguments: Seq[(String, String)]): Int =
math.max(2,
Review Comment:
Agreed, the budget is per call. `ConcatWs` calls it three times in one
method on its vararg path, so three call sets each just under the bound could
together approach the limit. A call measured about 15 bytes in `javap` (see the
next thread), which puts one ungrouped set at about 2500 bytes at most, so
three of them leave room for the caller's own code, but not much. I've
documented this at `splitCallsPerMethod`. Sharing one budget across the calls a
method makes would need state in the context, and I'd rather keep that for a
separate change if it ever shows up in practice.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala:
##########
@@ -1740,6 +1815,19 @@ object CodeGenerator extends Logging {
// method which is going to be called by the outer class instead of the many
small ones
final val MERGE_SPLIT_METHODS_THRESHOLD = 3
+ /**
+ * The most bytecode a call to a split function and its fold take in the
calling method,
+ * besides a load per argument; see `CodegenContext.groupFunctionCalls`.
+ */
+ final val MAX_BYTES_PER_SPLIT_CALL = 20
Review Comment:
It wasn't when you asked, so I checked it now. I compiled the generated
classes of the 1000-branch `CASE WHEN` projection and of the 30000-wide
`Murmur3Hash` and disassembled them with `javap -c -p`. A `CASE WHEN` call with
its fold (invoke, loads, store, compare, branch) is about 15 bytes, measured
over the 8-call group methods: 119 bytes for 8 calls. Calls into another nested
class, with the `aload_0` and two `getfield`s for the instance, are 13 to 15
bytes: the hash's group methods are 102 to 105 bytes for 7 to 8 calls. So 20 is
an upper bound with some margin, and the half-limit budget stays on top of
that. I've added these measurements to the constant's comment.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala:
##########
@@ -1314,13 +1374,28 @@ class CodegenContext extends Logging {
val argDefinitionString = arguments.map { case (t, name) => s"$t $name"
}.mkString(", ")
val argInvocationString = arguments.map(_._2).mkString(", ")
+ val callsPerMethod = splitCallsPerMethod(arguments)
innerClassToFunctions.flatMap {
case ((innerClassName, innerClassInstance), innerClassFunctions) =>
// for performance reasons, the functions are prepended, instead of
appended,
// thus here they are in reversed order
val orderedFunctions = innerClassFunctions.reverse
- if (orderedFunctions.size > MERGE_SPLIT_METHODS_THRESHOLD) {
+ if (orderedFunctions.size > callsPerMethod) {
+ // Too many calls for one method to stay JIT-compilable: one merged
method per part,
+ // whose calls the caller groups further if there are still too many
of them.
+ orderedFunctions.grouped(callsPerMethod).zipWithIndex.map { case
(part, i) =>
Review Comment:
Good catch, that's inconsistent with the reasoning for
`SPLIT_CALLS_PER_GROUP`. The part size only needed to bound bytes, but a part
of ~170 calls has the same C2 inlining problem as a large group. The parts are
now of `SPLIT_CALLS_PER_GROUP` calls, and `groupFunctionCalls` handles the
levels above them. The cost on the side SPARK-22226 cared about is small: each
part adds one method to the outer class's constant pool, a few entries, instead
of one per inner class, so about 125 methods for an inner class of 1000
functions, against the 65535 limit. The `COALESCE` test of 10000 children goes
through this path.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala:
##########
@@ -1248,7 +1248,67 @@ class CodegenContext extends Logging {
makeSplitFunction,
foldFunctions)
- foldFunctions(outerClassFunctionCalls ++ innerClassFunctionCalls)
+ foldFunctions(groupFunctionCalls(
+ (outerClassFunctionCalls ++ innerClassFunctionCalls).toSeq,
+ func,
+ arguments,
+ returnType,
+ makeSplitFunction,
+ foldFunctions))
+ }
+ }
+
+ /**
+ * The calls to the functions `splitExpressions` split out, grouped into
functions of their own
+ * where there are too many for the calling method to be JIT-compiled, level
by level.
+ *
+ * The calls are what is left in the calling method, one per block of code,
and a `CASE WHEN`
+ * of a thousand branches has hundreds of blocks: their calls alone take the
caller past the
+ * 8000 bytes HotSpot compiles
(`CodeGenerator.DEFAULT_JVM_HUGE_METHOD_LIMIT`), so the method
+ * every row goes through runs interpreted. A call and its fold compile to
at most about 20 bytes
+ * plus 2 for each argument - the invocation, a load per argument, and the
fold's store, test
+ * and branch - so the calls are left as they are while they fit in half of
that limit, the rest
+ * being the caller's own code, and grouped otherwise. The groups are small,
of
+ * `CodeGenerator.SPLIT_CALLS_PER_GROUP` calls, so that C2 can inline the
split functions into
+ * each group within its inlining budget; a group of hundreds of calls stays
compilable but has
+ * most of its calls left out of inlining, and runs slower. A group is
folded with
+ * `foldFunctions` and wrapped with `makeSplitFunction`, which is what
+ * `generateInnerClassesFunctionCalls` does with the functions of one inner
class, so every
+ * caller of `splitExpressions` supports it.
+ */
+ /**
+ * How many calls to split functions taking `arguments` fit in one method
with room to spare
+ * below the 8000 bytes HotSpot compiles; see `groupFunctionCalls`.
+ */
+ private def splitCallsPerMethod(arguments: Seq[(String, String)]): Int =
math.max(2,
+ DEFAULT_JVM_HUGE_METHOD_LIMIT / 2 / (MAX_BYTES_PER_SPLIT_CALL + 2 *
arguments.length))
+
+ private def groupFunctionCalls(
+ calls: Seq[String],
+ funcName: String,
+ arguments: Seq[(String, String)],
+ returnType: String,
+ makeSplitFunction: String => String,
+ foldFunctions: Seq[String] => String,
+ level: Int = 0): Seq[String] = {
+ val callsPerMethod = splitCallsPerMethod(arguments)
+ if (calls.length <= callsPerMethod) {
+ calls
+ } else {
+ val argDefinitionString = arguments.map { case (t, name) => s"$t $name"
}.mkString(", ")
+ val argInvocationString = arguments.map(_._2).mkString(", ")
+ val groupCalls = calls.grouped(SPLIT_CALLS_PER_GROUP).zipWithIndex.map {
case (group, i) =>
+ val name = s"${funcName}_group${level}_$i"
+ val code =
+ s"""
+ |private $returnType $name($argDefinitionString) {
+ | ${makeSplitFunction(foldFunctions(group))}
+ |}
+ """.stripMargin
+ s"${addNewFunction(name, code)}($argInvocationString)"
Review Comment:
Yes, the 30000-wide hash test goes through exactly this path. I disassembled
its generated classes. The outer class spills into `NestedClass_0` to
`NestedClass_6`, and all five group methods land in `NestedClass_6` and call
private split functions of `NestedClass_0` to `_5` and of the outer class, for
example `computeHash_0_group0_100$` calls into `NestedClass_1` and
`NestedClass_2`. Janino compiles private methods of nested classes as static
`name$(outer, ...)` methods, which is what makes the sibling access work, and
the test checks the hash value against the interpreted one.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CodeGenerationSuite.scala:
##########
@@ -102,6 +102,91 @@ class CodeGenerationSuite extends SparkFunSuite with
ExpressionEvalHelper {
assert(actual.head == cases)
}
+ /**
+ * Runs `body` and fails if the code it generates has a method past the 8000
bytes HotSpot
+ * compiles, which `CodeGenerator` logs as it compiles the class.
+ */
+ private def assertJitCompilable(body: => Unit): Unit = {
+ val appender = new LogAppender("methods too long to be JIT compiled")
+ withLogAppender(appender,
+ loggerNames = Seq(classOf[CodeGenerator[_, _]].getName), level =
Some(Level.INFO)) {
+ body
+ }
+ val tooLong = appender.loggingEvents.map(_.getMessage.getFormattedMessage)
+ .filter(_.contains("too long to be JIT compiled"))
+ assert(tooLong.isEmpty, tooLong.mkString("\n"))
+ }
+
+ // The tests below pin SPARK-59783: an expression split into hundreds of
functions left one call
+ // per function in the method holding them, and the calls alone took that
method past the
+ // 8000 bytes HotSpot compiles, so every row ran it interpreted. Each test
covers one caller of
+ // `splitExpressions`, with as many children as it takes for that method to
cross the limit
+ // without the grouping: fewer where one child generates more code.
+
+ test("SPARK-59783: the calls to the functions CASE WHEN splits into are
JIT-compilable") {
+ val wide = 3000
+ val input = BoundReference(0, IntegerType, nullable = false)
+ val expression = CaseWhen(
+ (1 to wide).map(k => (EqualTo(input, Literal(k)), Multiply(input,
Literal(k)))), Literal(0))
+ assertJitCompilable {
+ val projection = GenerateMutableProjection.generate(Seq(expression))
+ Seq(7 -> 49, wide -> wide * wide, wide + 1 -> 0).foreach { case (in,
out) =>
+ assert(projection(new GenericInternalRow(Array[Any](in))).getInt(0) ==
out)
+ }
+ }
+ }
+
+ test("SPARK-59783: the calls to the functions COALESCE splits into are
JIT-compilable") {
+ val wide = 10000
+ val expression = Coalesce((0 until wide).map(i => BoundReference(i,
IntegerType, true)))
+ assertJitCompilable {
+ val projection = GenerateMutableProjection.generate(Seq(expression))
+ val values = new Array[Any](wide)
+ assert(projection(new GenericInternalRow(values)).isNullAt(0))
+ values(wide - 1) = 42
+ assert(projection(new GenericInternalRow(values)).getInt(0) == 42)
+ }
+ }
+
+ test("SPARK-59783: the calls to the functions IN splits into are
JIT-compilable") {
+ val wide = 10000
+ val input = BoundReference(0, IntegerType, nullable = false)
+ val expression = In(input, (1 to wide).map(k => Literal(k * 2)))
+ assertJitCompilable {
+ val projection = GenerateMutableProjection.generate(Seq(expression))
+ Seq(2 -> true, wide * 2 -> true, 3 -> false).foreach { case (in, out) =>
+ assert(projection(new
GenericInternalRow(Array[Any](in))).getBoolean(0) == out)
+ }
+ }
+ }
+
+ test("SPARK-59783: the calls to the functions a hash splits into are
JIT-compilable") {
Review Comment:
Locally the five tests take about 4 s together: `CASE WHEN` 0.4 s,
`COALESCE` 0.5 s, `IN` 0.3 s, the ordering 0.3 to 0.4 s, and the hash test 2.2
to 2.4 s for both `Murmur3Hash` and `XxHash64`. Each width is the smallest
round number at which the test fails without the change. At 10000 children the
hash test still passes without it, so it runs at 30000. I can bisect between
the two if you'd like it faster.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
