By default, only run DCE on methods with an ATHROW.

GenBCode started to run a local DCE optimization on all methods to afford simplicity in code generation of `Nothing`-typed expressions. Benchmarks show this was costing 1-2% of compile times. This commit selectively run DCE on methods with the problematic THROW, even under `-opt:l:none`. It then changes `-opt:l:default` to the empty set of optimizations. An existing test for this code gen quirk is updated to show with either explicitly enabled or selective targeted DCE, clean code is generated.
scala · retronym · Jul 26, 2017 · Jul 28, 2017 · Jul 31, 2017 · Aug 7, 2017
commit 46bfecd54c8c0dc90d6280be6e94f45b1e20a7d9
diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala b/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala
@@ -88,7 +88,10 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder {
       assert(thrownKind.isNullType || thrownKind.isNothingType || thrownKind.asClassBType.isSubtypeOf(jlThrowableRef).get)
       genLoad(expr, thrownKind)
       lineNumber(expr)
-      emit(asm.Opcodes.ATHROW) // ICode enters here into enterIgnoreMode, we'll rely instead on DCE at ClassNode level.
+
+      // ICode used to switch into enterIgnoreMode, we'll rely instead on DCE at ClassNode level.
+      genBCode.postProcessor.markMethodForDCE(mnode)
+      emit(asm.Opcodes.ATHROW)
 
       srNothingRef // always returns the same, the invoker should know :)
     }
@@ -868,8 +871,10 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder {
          * emitted instruction was an ATHROW. As explained above, it is OK to emit a second ATHROW,
          * the verifiers will be happy.
          */
-        if (lastInsn.getOpcode != asm.Opcodes.ATHROW)
+        if (lastInsn.getOpcode != asm.Opcodes.ATHROW) {
+          genBCode.postProcessor.markMethodForDCE(mnode)
           emit(asm.Opcodes.ATHROW)
+        }
       } else if (from.isNullType) {
         /* After loading an expression of type `scala.runtime.Null$`, introduce POP; ACONST_NULL.
          * This is required to pass the verifier: in Scala's type system, Null conforms to any

diff --git a/src/compiler/scala/tools/nsc/backend/jvm/PostProcessor.scala b/src/compiler/scala/tools/nsc/backend/jvm/PostProcessor.scala
@@ -5,7 +5,7 @@ import scala.collection.mutable.ListBuffer
 import scala.reflect.internal.util.NoPosition
 import scala.reflect.io.AbstractFile
 import scala.tools.asm.ClassWriter
-import scala.tools.asm.tree.ClassNode
+import scala.tools.asm.tree.{ClassNode, MethodNode}
 import scala.tools.nsc.backend.jvm.analysis.BackendUtils
 import scala.tools.nsc.backend.jvm.opt._
 
@@ -32,6 +32,8 @@ abstract class PostProcessor extends PerRunInit {
   lazy val classfileWriter: LazyVar[ClassfileWriter] = perRunLazy(this)(new ClassfileWriter(frontendAccess))
 
   lazy val generatedClasses = recordPerRunCache(new ListBuffer[GeneratedClass])
+  lazy val methodRequiringDCE = recordPerRunCache(new java.util.concurrent.ConcurrentHashMap[MethodNode, Unit])
+  final def markMethodForDCE(node: MethodNode) = methodRequiringDCE.put(node, ())
 
   override def initialize(): Unit = {
     super.initialize()

diff --git a/src/compiler/scala/tools/nsc/backend/jvm/PostProcessorFrontendAccess.scala b/src/compiler/scala/tools/nsc/backend/jvm/PostProcessorFrontendAccess.scala
@@ -4,6 +4,7 @@ package backend.jvm
 import scala.collection.generic.Clearable
 import scala.reflect.internal.util.Position
 import scala.reflect.io.AbstractFile
+import scala.tools.asm.tree.MethodNode
 import scala.tools.nsc.backend.jvm.BTypes.InternalName
 
 /**
@@ -28,7 +29,15 @@ sealed abstract class PostProcessorFrontendAccess {
 
   def javaDefinedClasses: Set[InternalName]
 
+  def methodRequiresDCE(method: MethodNode): Boolean
+
   def recordPerRunCache[T <: Clearable](cache: T): T
+  final def recordPerRunCache[T, V](cache: java.util.Map[T, V]): java.util.Map[T, V] = {
+    recordPerRunCache(new Clearable {
+      override def clear(): Unit = cache.clear()
+    })
+    cache
+  }
 }
 
 object PostProcessorFrontendAccess {
@@ -163,5 +172,7 @@ object PostProcessorFrontendAccess {
 
 
     def recordPerRunCache[T <: Clearable](cache: T): T = frontendSynch(perRunCaches.recordCache(cache))
+
+    def methodRequiresDCE(method: MethodNode): Boolean = genBCode.postProcessor.methodRequiringDCE.containsKey(method)
   }
 }
diff --git a/src/compiler/scala/tools/nsc/backend/jvm/opt/LocalOpt.scala b/src/compiler/scala/tools/nsc/backend/jvm/opt/LocalOpt.scala
@@ -295,7 +295,8 @@ abstract class LocalOpt {
       // boxUnboxElimination) require not having unreachable instructions (null frames).
       val runDCE = (compilerSettings.optUnreachableCode && (requestDCE || nullnessOptChanged)) ||
         compilerSettings.optBoxUnbox ||
-        compilerSettings.optCopyPropagation
+        compilerSettings.optCopyPropagation ||
+        frontendAccess.methodRequiresDCE(method)
       val (codeRemoved, liveLabels) = if (runDCE) removeUnreachableCodeImpl(method, ownerClassName) else (false, Set.empty[LabelNode])
       traceIfChanged("dce")
 

diff --git a/src/compiler/scala/tools/nsc/settings/ScalaSettings.scala b/src/compiler/scala/tools/nsc/settings/ScalaSettings.scala
@@ -240,7 +240,7 @@ trait ScalaSettings extends AbsScalaSettings
     val lNone = Choice("l:none",
       "Disable optimizations. Takes precedence: `-opt:l:none,+box-unbox` / `-opt:l:none -opt:box-unbox` don't enable box-unbox.")
 
-    private val defaultChoices = List(unreachableCode)
+    private val defaultChoices = List()
     val lDefault = Choice(
       "l:default",
       "Enable default optimizations: " + defaultChoices.mkString("", ",", "."),

diff --git a/test/junit/scala/tools/nsc/backend/jvm/opt/UnreachableCodeTest.scala b/test/junit/scala/tools/nsc/backend/jvm/opt/UnreachableCodeTest.scala
@@ -17,9 +17,12 @@ import scala.tools.testing.ClearAfterClass
 class UnreachableCodeTest extends ClearAfterClass {
   // jvm-1.6 enables emitting stack map frames, which impacts the code generation wrt dead basic blocks,
   // see comment in BCodeBodyBuilder
-  val methodOptCompiler     = cached("methodOptCompiler", () => newCompiler(extraArgs = "-opt:l:method"))
-  val dceCompiler           = cached("dceCompiler",       () => newCompiler(extraArgs = "-opt:unreachable-code"))
-  val noOptCompiler         = cached("noOptCompiler",     () => newCompiler(extraArgs = "-opt:l:none"))
+  val methodOptCompiler     = cached("methodOptCompiler",    () => newCompiler(extraArgs = "-opt:l:method"))
+  val dceCompiler           = cached("dceCompiler",          () => newCompiler(extraArgs = "-opt:unreachable-code"))
+  // By default, DCE is only run on methods that are emitted with a `ATHROW` which avoids some extremely ugly bytecode without
+  // incurring the cost of running DCE everywhere.
+  val selectiveDceCompiler  = cached("selectiveCceCompiler", () => newCompiler(extraArgs = "-opt:l:default")) // same as omitting `-opt`
+  val noOptCompiler         = cached("noOptCompiler",        () => newCompiler(extraArgs = "-opt:l:none"))
 
   def assertEliminateDead(code: (Instruction, Boolean)*): Unit = {
     val method = genMethod()(code.map(_._1): _*)
@@ -201,7 +204,11 @@ class UnreachableCodeTest extends ClearAfterClass {
   }
 
   @Test
-  def loadNullNothingBytecode(): Unit = {
+  def loadNullNothingBytecodeSelectiveDCE(): Unit = testLoadNullNothingBytecode(selectiveDceCompiler)
+  @Test
+  def loadNullNothingBytecodeExplicitDCE(): Unit = testLoadNullNothingBytecode(dceCompiler)
+
+  private def testLoadNullNothingBytecode(compiler: scala.tools.testing.Compiler): Unit = {
     val code =
       """class C {
         |  def nl: Null = null
@@ -241,7 +248,7 @@ class UnreachableCodeTest extends ClearAfterClass {
     assertSameSummary(getMethod(c, "t4"), List(
       ALOAD, ALOAD, "nt", ATHROW, NOP, NOP, NOP, ATHROW))
 
-    val cDCE = dceCompiler.compileClass(code)
+    val cDCE = compiler.compileClass(code)
     assertSameSummary(getMethod(cDCE, "t3"), List(ALOAD, NEW, DUP, LDC, "<init>", ATHROW))
     assertSameSummary(getMethod(cDCE, "t4"), List(ALOAD, ALOAD, "nt", ATHROW))
   }