-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-29061][SQL] Prints bytecode statistics in debugCodegen #25766
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 2 commits
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 |
|---|---|---|
|
|
@@ -1211,6 +1211,16 @@ abstract class CodeGenerator[InType <: AnyRef, OutType <: AnyRef] extends Loggin | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Java bytecode statistics of a compiled class by Janino. | ||
| */ | ||
| case class ByteCodeStats( | ||
| maxClassCodeSize: Int, maxMethodCodeSize: Int, maxConstPoolSize: Int, numInnerClasses: Int) | ||
|
|
||
| object ByteCodeStats { | ||
| val UNAVAILABLE = ByteCodeStats(-1, -1, -1, -1) | ||
| } | ||
|
|
||
| object CodeGenerator extends Logging { | ||
|
|
||
| // This is the default value of HugeMethodLimit in the OpenJDK HotSpot JVM, | ||
|
|
@@ -1244,7 +1254,7 @@ object CodeGenerator extends Logging { | |
| * | ||
| * @return a pair of a generated class and the max bytecode size of generated functions. | ||
| */ | ||
| def compile(code: CodeAndComment): (GeneratedClass, Int) = try { | ||
| def compile(code: CodeAndComment): (GeneratedClass, ByteCodeStats) = try { | ||
| cache.get(code) | ||
| } catch { | ||
| // Cache.get() may wrap the original exception. See the following URL | ||
|
|
@@ -1257,7 +1267,7 @@ object CodeGenerator extends Logging { | |
| /** | ||
| * Compile the Java source code into a Java class, using Janino. | ||
| */ | ||
| private[this] def doCompile(code: CodeAndComment): (GeneratedClass, Int) = { | ||
| private[this] def doCompile(code: CodeAndComment): (GeneratedClass, ByteCodeStats) = { | ||
| val evaluator = new ClassBodyEvaluator() | ||
|
|
||
| // A special classloader used to wrap the actual parent classloader of | ||
|
|
@@ -1318,10 +1328,11 @@ object CodeGenerator extends Logging { | |
| } | ||
|
|
||
| /** | ||
| * Returns the max bytecode size of the generated functions by inspecting janino private fields. | ||
| * Also, this method updates the metrics information. | ||
| * Returns the bytecode statistics (max class bytecode size, max method bytecode size, | ||
| * max constant pool size, and # of inner classes) of generated classes | ||
| * by inspecting Janino classes. Also, this method updates the metrics information. | ||
| */ | ||
| private def updateAndGetCompilationStats(evaluator: ClassBodyEvaluator): Int = { | ||
| private def updateAndGetCompilationStats(evaluator: ClassBodyEvaluator): ByteCodeStats = { | ||
| // First retrieve the generated classes. | ||
| val classes = { | ||
| val resultField = classOf[SimpleCompiler].getDeclaredField("result") | ||
|
|
@@ -1336,11 +1347,13 @@ object CodeGenerator extends Logging { | |
| val codeAttr = Utils.classForName("org.codehaus.janino.util.ClassFile$CodeAttribute") | ||
| val codeAttrField = codeAttr.getDeclaredField("code") | ||
| codeAttrField.setAccessible(true) | ||
| val codeSizes = classes.flatMap { case (_, classBytes) => | ||
| CodegenMetrics.METRIC_GENERATED_CLASS_BYTECODE_SIZE.update(classBytes.length) | ||
| val codeStats = classes.map { case (_, classBytes) => | ||
|
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. I would like to make the code more readable, by |
||
| val classCodeSize = classBytes.length | ||
| CodegenMetrics.METRIC_GENERATED_CLASS_BYTECODE_SIZE.update(classCodeSize) | ||
| try { | ||
| val cf = new ClassFile(new ByteArrayInputStream(classBytes)) | ||
| val stats = cf.methodInfos.asScala.flatMap { method => | ||
| val constPoolSize = cf.getConstantPoolSize | ||
| val methodCodeSizes = cf.methodInfos.asScala.flatMap { method => | ||
| method.getAttributes().filter(_.getClass eq codeAttr).map { a => | ||
| val byteCodeSize = codeAttrField.get(a).asInstanceOf[Array[Byte]].length | ||
| CodegenMetrics.METRIC_GENERATED_METHOD_BYTECODE_SIZE.update(byteCodeSize) | ||
|
|
@@ -1353,19 +1366,25 @@ object CodeGenerator extends Logging { | |
| byteCodeSize | ||
| } | ||
| } | ||
| Some(stats) | ||
| (classCodeSize, methodCodeSizes.max, constPoolSize) | ||
|
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. I'm curious: now that we've got a nice new
Member
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. No strong reason... I just did because I avoided the longer statement in https://github.com/apache/spark/pull/25766/files#diff-8bcc5aea39c73d4bf38aef6f6951d42cR1382; If there are other reviewers who like that, I'll update.
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. I find named fields much more readable than _1 _2 _3. In fact even with tuples I may have written the code like: ByteCodeStats(codeStats.reduce { case ((maxClassCodeSize1, maxMethodCodeSize1, maxConstPoolSize), (maxClassCodeSize2, maxMethodCodeSize2, maxConstPoolSize2)) =>
(Math.max(maxClassCodeSize1, maxClassCodeSize2),
Math.max(maxMethodCodeSize1, maxMethodCodeSize2),
Math.max(maxConstPoolSize1, maxConstPoolSize2))
})and...I'd say the
Member
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. How about the latest code? I added a new metric (# of inner classes), so using a tuple in that part is ok? |
||
| } catch { | ||
| case NonFatal(e) => | ||
| logWarning("Error calculating stats of compiled class.", e) | ||
| None | ||
| (classCodeSize, -1, -1) | ||
| } | ||
| }.flatten | ||
| } | ||
|
|
||
| if (codeSizes.nonEmpty) { | ||
| codeSizes.max | ||
| } else { | ||
| 0 | ||
| // Computes the max values of the three metrics: class code size, method code size, | ||
| // and constant pool size. | ||
| val maxCodeStats = codeStats.reduce[(Int, Int, Int)] { case (v1, v2) => | ||
| (Math.max(v1._1, v2._1), Math.max(v1._2, v2._2), Math.max(v1._3, v2._3)) | ||
|
Member
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. I like this direction. May we see these values regarding different classes? Is it better to show class name and method name, too?
Member
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. Currently, this pr prints statistics per a whole-stage codegen entry, so the current one looks ok to me. |
||
| } | ||
| ByteCodeStats( | ||
| maxClassCodeSize = maxCodeStats._1, | ||
| maxMethodCodeSize = maxCodeStats._2, | ||
| maxConstPoolSize = maxCodeStats._3, | ||
| // Minus 2 for `GeneratedClass` and an outer-most generated class | ||
| numInnerClasses = classes.size - 2) | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -1380,8 +1399,8 @@ object CodeGenerator extends Logging { | |
| private val cache = CacheBuilder.newBuilder() | ||
| .maximumSize(SQLConf.get.codegenCacheMaxEntries) | ||
| .build( | ||
| new CacheLoader[CodeAndComment, (GeneratedClass, Int)]() { | ||
| override def load(code: CodeAndComment): (GeneratedClass, Int) = { | ||
| new CacheLoader[CodeAndComment, (GeneratedClass, ByteCodeStats)]() { | ||
| override def load(code: CodeAndComment): (GeneratedClass, ByteCodeStats) = { | ||
| val startTime = System.nanoTime() | ||
| val result = doCompile(code) | ||
| val endTime = System.nanoTime() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,14 +20,15 @@ package org.apache.spark.sql.execution | |
| import java.util.Collections | ||
|
|
||
| import scala.collection.JavaConverters._ | ||
| import scala.util.control.NonFatal | ||
|
|
||
| import org.apache.spark.broadcast.Broadcast | ||
| import org.apache.spark.internal.Logging | ||
| import org.apache.spark.rdd.RDD | ||
| import org.apache.spark.sql._ | ||
| import org.apache.spark.sql.catalyst.InternalRow | ||
| import org.apache.spark.sql.catalyst.expressions.Attribute | ||
| import org.apache.spark.sql.catalyst.expressions.codegen.{CodeFormatter, CodegenContext, ExprCode} | ||
| import org.apache.spark.sql.catalyst.expressions.codegen.{ByteCodeStats, CodeFormatter, CodegenContext, CodeGenerator, ExprCode} | ||
| import org.apache.spark.sql.catalyst.plans.physical.Partitioning | ||
| import org.apache.spark.sql.catalyst.trees.TreeNodeRef | ||
| import org.apache.spark.sql.catalyst.util.StringUtils.StringConcat | ||
|
|
@@ -81,11 +82,15 @@ package object debug { | |
| def writeCodegen(append: String => Unit, plan: SparkPlan): Unit = { | ||
| val codegenSeq = codegenStringSeq(plan) | ||
| append(s"Found ${codegenSeq.size} WholeStageCodegen subtrees.\n") | ||
| for (((subtree, code), i) <- codegenSeq.zipWithIndex) { | ||
| append(s"== Subtree ${i + 1} / ${codegenSeq.size} ==\n") | ||
| for (((subtree, code, codeStats), i) <- codegenSeq.zipWithIndex) { | ||
| val codeStatsStr = s"maxClassCodeSize:${codeStats.maxClassCodeSize}; " + | ||
|
Member
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. I added one more metric for the ratio of an used constant pool like cc: @rednaxelafx @kiszk
Member
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. I am neutral on this. I am bit worry about the wider width regarding ease of reading._
Member
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. @maropu Could you elaborate on your idea to add this? How about adding ratio of
Member
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. Yea, it might be worth adding it though, this pr already merged. We could revisit this in future if need this metric for debugging...
Member
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. How about |
||
| s"maxMethodCodeSize:${codeStats.maxMethodCodeSize}; " + | ||
| s"maxConstantPoolSize:${codeStats.maxConstPoolSize}; " + | ||
| s"numInnerClasses:${codeStats.numInnerClasses}" | ||
| append(s"== Subtree ${i + 1} / ${codegenSeq.size} ($codeStatsStr) ==\n") | ||
| append(subtree) | ||
| append("\nGenerated code:\n") | ||
| append(s"${code}\n") | ||
| append(s"$code\n") | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -95,7 +100,7 @@ package object debug { | |
| * @param plan the query plan for codegen | ||
| * @return Sequence of WholeStageCodegen subtrees and corresponding codegen | ||
| */ | ||
| def codegenStringSeq(plan: SparkPlan): Seq[(String, String)] = { | ||
| def codegenStringSeq(plan: SparkPlan): Seq[(String, String, ByteCodeStats)] = { | ||
| val codegenSubtrees = new collection.mutable.HashSet[WholeStageCodegenExec]() | ||
| plan transform { | ||
| case s: WholeStageCodegenExec => | ||
|
|
@@ -105,7 +110,13 @@ package object debug { | |
| } | ||
| codegenSubtrees.toSeq.map { subtree => | ||
| val (_, source) = subtree.doCodeGen() | ||
| (subtree.toString, CodeFormatter.format(source)) | ||
| val codeStats = try { | ||
| CodeGenerator.compile(source)._2 | ||
| } catch { | ||
| case NonFatal(_) => | ||
| ByteCodeStats.UNAVAILABLE | ||
| } | ||
| (subtree.toString, CodeFormatter.format(source), codeStats) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -130,7 +141,7 @@ package object debug { | |
| * @param query the streaming query for codegen | ||
| * @return Sequence of WholeStageCodegen subtrees and corresponding codegen | ||
| */ | ||
| def codegenStringSeq(query: StreamingQuery): Seq[(String, String)] = { | ||
| def codegenStringSeq(query: StreamingQuery): Seq[(String, String, ByteCodeStats)] = { | ||
| val w = asStreamExecution(query) | ||
| if (w.lastExecution != null) { | ||
| codegenStringSeq(w.lastExecution.executedPlan) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,7 +46,7 @@ class DebuggingSuite extends SharedSparkSession { | |
| val res = codegenStringSeq(spark.range(10).groupBy(col("id") * 2).count() | ||
| .queryExecution.executedPlan) | ||
| assert(res.length == 2) | ||
| assert(res.forall{ case (subtree, code) => | ||
| assert(res.forall{ case (subtree, code, _) => | ||
| subtree.contains("Range") && code.contains("Object[]")}) | ||
| } | ||
|
|
||
|
|
@@ -90,4 +90,30 @@ class DebuggingSuite extends SharedSparkSession { | |
| | id LongType: {} | ||
| |""".stripMargin)) | ||
| } | ||
|
|
||
| test("Prints bytecode statistics in debugCodegen") { | ||
| Seq(("SELECT sum(v) FROM VALUES(1) t(v)", (0, 0)), | ||
| // We expect HashAggregate uses an inner class for fast hash maps | ||
| // in partial aggregates with keys. | ||
|
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. I'd like to avoid end-to-end tests in this case. It's highly coupled with how we codegen these operators and is easy to break if we change the implementation in the future. Can we add some UT that calls
Member
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. ok, I'll try.
Member
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. |
||
| ("SELECT k, avg(v) FROM VALUES((1, 1)) t(k, v) GROUP BY k", (0, 1))) | ||
| .foreach { case (query, (expectedNumInnerClasses0, expectedNumInnerClasses1)) => | ||
|
|
||
| val executedPlan = sql(query).queryExecution.executedPlan | ||
| val res = codegenStringSeq(executedPlan) | ||
| assert(res.length == 2) | ||
| assert(res.forall { case (_, _, codeStats) => | ||
| codeStats.maxClassCodeSize > 0 && | ||
| codeStats.maxMethodCodeSize > 0 && | ||
| codeStats.maxConstPoolSize > 0 | ||
| }) | ||
| assert(res(0)._3.numInnerClasses == expectedNumInnerClasses0) | ||
| assert(res(1)._3.numInnerClasses == expectedNumInnerClasses1) | ||
|
|
||
| val debugCodegenStr = codegenString(executedPlan) | ||
| assert(debugCodegenStr.contains("maxClassCodeSize:")) | ||
| assert(debugCodegenStr.contains("maxMethodCodeSize:")) | ||
| assert(debugCodegenStr.contains("maxConstantPoolSize:")) | ||
| assert(debugCodegenStr.contains("numInnerClasses:")) | ||
| } | ||
| } | ||
| } | ||
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.
A ByteCodeStats matches to a compiled class? maxClassCodeSize is for max inner class code size?
Uh oh!
There was an error while loading. Please reload this page.
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 current code just collects the max size among a compiled class and inner classes. But, on second thoughs, I think now we don't need to print the class size cuz IIUC the size is not related to the JVM limits: https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.11
#25766 (comment)
WDYT? @kiszk