Split up cache for ClassBTypes in two, according to origin

Split up the `classBTypeFromInternalName` map in two, depending if the
type was created from a symbol or classfile. This allows adding an additional
assertion: when getting a `ClassBType` for a symbol that's being compiled in
the current run, we want to assert that an existing, cached type was created
from that symbol, and not from a (matching, but old) classfile on the classpath.

Note that creating `ClassBTypes` from symbols is the ordinary case. In particular,
bytecode generation creats these types for every class being emitted. Types created
from classfiles are only used for type references not present in the soruce but
introduced by the inliner. At this later stage, all types for classes being compiled
are already created (and cached).

Author: lrytz

src/compiler/scala/tools/nsc/backend/jvm/BCodeHelpers.scala

@@ -276,12 +276,14 @@ abstract class BCodeHelpers extends BCodeIdiomatic with BytecodeWriters {
   final class CClassWriter(flags: Int) extends asm.ClassWriter(flags) {
 
     /**
-     * This method is thread-safe: it depends only on the BTypes component, which does not depend
-     * on global. TODO @lry move to a different place where no global is in scope, on bTypes.
+     * This method is used by asm when computing stack map frames. It is thread-safe: it depends
+     * only on the BTypes component, which does not depend on global.
+     * TODO @lry move to a different place where no global is in scope, on bTypes.
      */
     override def getCommonSuperClass(inameA: String, inameB: String): String = {
-      val a = classBTypeFromInternalName(inameA)
-      val b = classBTypeFromInternalName(inameB)
+      // All types that appear in a class node need to have their ClassBType cached, see [[cachedClassBType]].
+      val a = cachedClassBType(inameA).get
+      val b = cachedClassBType(inameB).get
       val lub = a.jvmWiseLUB(b).get
       val lubName = lub.internalName
       assert(lubName != "scala/Any")

src/compiler/scala/tools/nsc/backend/jvm/BTypes.scala

@@ -64,17 +64,19 @@ abstract class BTypes {
   def compilerSettings: ScalaSettings
 
   /**
-   * A map from internal names to ClassBTypes. Every ClassBType is added to this map on its
-   * construction.
+   * Every ClassBType is cached on construction and accessible through this method.
    *
-   * This map is used when computing stack map frames. The asm.ClassWriter invokes the method
+   * The cache is used when computing stack map frames. The asm.ClassWriter invokes the method
    * `getCommonSuperClass`. In this method we need to obtain the ClassBType for a given internal
-   * name. The method assumes that every class type that appears in the bytecode exists in the map.
-   *
-   * Concurrent because stack map frames are computed when in the class writer, which might run
-   * on multiple classes concurrently.
+   * name. The method assumes that every class type that appears in the bytecode exists in the map
    */
-  val classBTypeFromInternalName: concurrent.Map[InternalName, ClassBType] = recordPerRunCache(TrieMap.empty)
+  def cachedClassBType(internalName: InternalName): Option[ClassBType] =
+    classBTypeCacheFromSymbol.get(internalName).orElse(classBTypeCacheFromClassfile.get(internalName))
+
+  // Concurrent maps because stack map frames are computed when in the class writer, which
+  // might run on multiple classes concurrently.
+  val classBTypeCacheFromSymbol: concurrent.Map[InternalName, ClassBType] = recordPerRunCache(TrieMap.empty)
+  val classBTypeCacheFromClassfile: concurrent.Map[InternalName, ClassBType] = recordPerRunCache(TrieMap.empty)
 
   /**
    * Store the position of every MethodInsnNode during code generation. This allows each callsite
@@ -173,8 +175,8 @@ abstract class BTypes {
    * be found in the `byteCodeRepository`, the `info` of the resulting ClassBType is undefined.
    */
   def classBTypeFromParsedClassfile(internalName: InternalName): ClassBType = {
-    classBTypeFromInternalName.getOrElse(internalName, {
-      val res = ClassBType(internalName)
+    cachedClassBType(internalName).getOrElse({
+      val res = ClassBType(internalName)(classBTypeCacheFromClassfile)
       byteCodeRepository.classNode(internalName) match {
         case Left(msg) => res.info = Left(NoClassBTypeInfoMissingBytecode(msg)); res
         case Right(c)  => setClassInfoFromClassNode(c, res)
@@ -186,8 +188,8 @@ abstract class BTypes {
    * Construct the [[ClassBType]] for a parsed classfile.
    */
   def classBTypeFromClassNode(classNode: ClassNode): ClassBType = {
-    classBTypeFromInternalName.getOrElse(classNode.name, {
-      setClassInfoFromClassNode(classNode, ClassBType(classNode.name))
+    cachedClassBType(classNode.name).getOrElse({
+      setClassInfoFromClassNode(classNode, ClassBType(classNode.name)(classBTypeCacheFromClassfile))
     })
   }
 
@@ -847,7 +849,7 @@ abstract class BTypes {
    * a missing info. In order not to crash the compiler unnecessarily, the inliner does not force
    * infos using `get`, but it reports inliner warnings for missing infos that prevent inlining.
    */
-  final case class ClassBType(internalName: InternalName) extends RefBType {
+  final case class ClassBType(internalName: InternalName)(cache: mutable.Map[InternalName, ClassBType]) extends RefBType {
     /**
      * Write-once variable allows initializing a cyclic graph of infos. This is required for
      * nested classes. Example: for the definition `class A { class B }` we have
@@ -868,7 +870,7 @@ abstract class BTypes {
       checkInfoConsistency()
     }
 
-    classBTypeFromInternalName(internalName) = this
+    cache(internalName) = this
 
     private def checkInfoConsistency(): Unit = {
       if (info.isLeft) return

src/compiler/scala/tools/nsc/backend/jvm/BTypesFromSymbols.scala

@@ -115,17 +115,22 @@ class BTypesFromSymbols[G <: Global](val global: G) extends BTypes {
     else if (classSym == NullClass) srNullRef
     else {
       val internalName = classSym.javaBinaryNameString
-      classBTypeFromInternalName.getOrElse(internalName, {
-        // The new ClassBType is added to the map in its constructor, before we set its info. This
-        // allows initializing cyclic dependencies, see the comment on variable ClassBType._info.
-        val res = ClassBType(internalName)
-        if (completeSilentlyAndCheckErroneous(classSym)) {
-          res.info = Left(NoClassBTypeInfoClassSymbolInfoFailedSI9111(classSym.fullName))
-          res
-        } else {
-          setClassInfo(classSym, res)
-        }
-      })
+      cachedClassBType(internalName) match {
+        case Some(bType) =>
+          if (currentRun.compiles(classSym))
+            assert(classBTypeCacheFromSymbol.contains(internalName), s"ClassBType for class being compiled was already created from a classfile: ${classSym.fullName}")
+          bType
+        case None =>
+          // The new ClassBType is added to the map in its constructor, before we set its info. This
+          // allows initializing cyclic dependencies, see the comment on variable ClassBType._info.
+          val res = ClassBType(internalName)(classBTypeCacheFromSymbol)
+          if (completeSilentlyAndCheckErroneous(classSym)) {
+            res.info = Left(NoClassBTypeInfoClassSymbolInfoFailedSI9111(classSym.fullName))
+            res
+          } else {
+            setClassInfo(classSym, res)
+          }
+      }
     }
   }
 
@@ -626,8 +631,8 @@ class BTypesFromSymbols[G <: Global](val global: G) extends BTypes {
   def mirrorClassClassBType(moduleClassSym: Symbol): ClassBType = {
     assert(isTopLevelModuleClass(moduleClassSym), s"not a top-level module class: $moduleClassSym")
     val internalName = moduleClassSym.javaBinaryNameString.stripSuffix(nme.MODULE_SUFFIX_STRING)
-    classBTypeFromInternalName.getOrElse(internalName, {
-      val c = ClassBType(internalName)
+    cachedClassBType(internalName).getOrElse({
+      val c = ClassBType(internalName)(classBTypeCacheFromSymbol)
       // class info consistent with BCodeHelpers.genMirrorClass
       val nested = exitingPickler(memberClassesForInnerClassTable(moduleClassSym)) map classBTypeFromSymbol
       c.info = Right(ClassInfo(
@@ -643,8 +648,8 @@ class BTypesFromSymbols[G <: Global](val global: G) extends BTypes {
 
   def beanInfoClassClassBType(mainClass: Symbol): ClassBType = {
     val internalName = mainClass.javaBinaryNameString + "BeanInfo"
-    classBTypeFromInternalName.getOrElse(internalName, {
-      val c = ClassBType(internalName)
+    cachedClassBType(internalName).getOrElse({
+      val c = ClassBType(internalName)(classBTypeCacheFromSymbol)
       c.info = Right(ClassInfo(
         superClass = Some(sbScalaBeanInfoRef),
         interfaces = Nil,

src/compiler/scala/tools/nsc/backend/jvm/CoreBTypes.scala

@@ -22,7 +22,7 @@ import scala.tools.nsc.backend.jvm.BTypes.InternalName
  * constructor will actually go through the proxy. The lazy vals make sure the instance is assigned
  * in the proxy before the fields are initialized.
  *
- * Note: if we did not re-create the core BTypes on each compiler run, BType.classBTypeFromInternalNameMap
+ * Note: if we did not re-create the core BTypes on each compiler run, BType.classBTypeCacheFromSymbol
  * could not be a perRunCache anymore: the classes defined here need to be in that map, they are
  * added when the ClassBTypes are created. The per run cache removes them, so they would be missing
  * in the second run.

src/compiler/scala/tools/nsc/backend/jvm/analysis/BackendUtils.scala

@@ -81,7 +81,7 @@ class BackendUtils[BT <: BTypes](val btypes: BT) {
 
     // Make sure to reference the ClassBTypes of all types that are used in the code generated
     // here (e.g. java/util/Map) are initialized. Initializing a ClassBType adds it to the
-    // `classBTypeFromInternalName` map. When writing the classfile, the asm ClassWriter computes
+    // `cachedClassBType` maps. When writing the classfile, the asm ClassWriter computes
     // stack map frames and invokes the `getCommonSuperClass` method. This method expects all
     // ClassBTypes mentioned in the source code to exist in the map.
 

test/junit/scala/tools/nsc/backend/jvm/opt/BTypesFromClassfileTest.scala

@@ -27,7 +27,10 @@ class BTypesFromClassfileTest extends BytecodeTesting {
   duringBackend(global.scalaPrimitives.init()) // needed: it's only done when running the backend, and we don't actually run the compiler
   duringBackend(bTypes.initializeCoreBTypes())
 
-  def clearCache() = bTypes.classBTypeFromInternalName.clear()
+  def clearCache() = {
+    bTypes.classBTypeCacheFromSymbol.clear()
+    bTypes.classBTypeCacheFromClassfile.clear()
+  }
 
   def sameBType(fromSym: ClassBType, fromClassfile: ClassBType, checked: Set[InternalName] = Set.empty): Set[InternalName] = {
     if (checked(fromSym.internalName)) checked

test/junit/scala/tools/nsc/backend/jvm/opt/CallGraphTest.scala

@@ -22,7 +22,8 @@ class CallGraphTest extends BytecodeTesting {
   import compiler._
   import global.genBCode.bTypes
   val notPerRun: List[Clearable] = List(
-    bTypes.classBTypeFromInternalName,
+    bTypes.classBTypeCacheFromSymbol,
+    bTypes.classBTypeCacheFromClassfile,
     bTypes.byteCodeRepository.compilingClasses,
     bTypes.byteCodeRepository.parsedClasses,
     bTypes.callGraph.callsites)
@@ -145,7 +146,7 @@ class CallGraphTest extends BytecodeTesting {
     val m = getAsmMethod(c, "m")
     val List(fn) = callsInMethod(m)
     val forNameMeth = byteCodeRepository.methodNode("java/lang/Class", "forName", "(Ljava/lang/String;)Ljava/lang/Class;").get._1
-    val classTp = classBTypeFromInternalName("java/lang/Class")
+    val classTp = cachedClassBType("java/lang/Class").get
     val r = callGraph.callsites(m)(fn)
     checkCallsite(fn, m, forNameMeth, classTp, safeToInline = false, atInline = false, atNoInline = false)
   }

test/junit/scala/tools/nsc/backend/jvm/opt/InlineInfoTest.scala

@@ -21,7 +21,8 @@ class InlineInfoTest extends BytecodeTesting {
   override def compilerArgs = "-opt:l:classpath"
 
   def notPerRun: List[Clearable] = List(
-    bTypes.classBTypeFromInternalName,
+    bTypes.classBTypeCacheFromSymbol,
+    bTypes.classBTypeCacheFromClassfile,
     bTypes.byteCodeRepository.compilingClasses,
     bTypes.byteCodeRepository.parsedClasses)
   notPerRun foreach global.perRunCaches.unrecordCache
@@ -52,7 +53,7 @@ class InlineInfoTest extends BytecodeTesting {
       """.stripMargin
     val classes = compile(code)
 
-    val fromSyms = classes.map(c => global.genBCode.bTypes.classBTypeFromInternalName(c.name).info.get.inlineInfo)
+    val fromSyms = classes.map(c => global.genBCode.bTypes.cachedClassBType(c.name).get.info.get.inlineInfo)
 
     val fromAttrs = classes.map(c => {
       assert(c.attrs.asScala.exists(_.isInstanceOf[InlineInfoAttribute]), c.attrs)
@@ -71,7 +72,7 @@ class InlineInfoTest extends BytecodeTesting {
         |}
       """.stripMargin
     compileClasses("class C { new A }", javaCode = List((jCode, "A.java")))
-    val info = global.genBCode.bTypes.classBTypeFromInternalName("A").info.get.inlineInfo
+    val info = global.genBCode.bTypes.cachedClassBType("A").get.info.get.inlineInfo
     assertEquals(info.methodInfos, Map(
       "bar()I"    -> MethodInlineInfo(true,false,false),
       "<init>()V" -> MethodInlineInfo(false,false,false),

test/junit/scala/tools/nsc/backend/jvm/opt/InlinerIllegalAccessTest.scala

@@ -23,6 +23,11 @@ class InlinerIllegalAccessTest extends BytecodeTesting {
   def assertEmpty(ins: List[AbstractInsnNode]) = for (i <- ins)
     throw new AssertionError(textify(i))
 
+  def clearClassBTypeCaches(): Unit = {
+    classBTypeCacheFromSymbol.clear()
+    classBTypeCacheFromClassfile.clear()
+  }
+
   @Test
   def typeAccessible(): Unit = {
     val code =
@@ -56,7 +61,7 @@ class InlinerIllegalAccessTest extends BytecodeTesting {
     check(eClass, assertEmpty) // C is public, so accessible in E
 
     byteCodeRepository.parsedClasses.clear()
-    classBTypeFromInternalName.clear()
+    clearClassBTypeCaches()
 
     cClass.access &= ~ACC_PUBLIC // ftw
     addToRepo(allClasses)
@@ -72,6 +77,11 @@ class InlinerIllegalAccessTest extends BytecodeTesting {
         }
       // MatchError otherwise
     })
+
+    // ensure the caches are empty at the end for the next test to run (`check` caches types by
+    // calling `classBTypeFromParsedClassfile`). note that per-run caches are cleared at the end
+    // of a compilation, not the beginning.
+    clearClassBTypeCaches()
   }
 
   @Test
@@ -190,5 +200,10 @@ class InlinerIllegalAccessTest extends BytecodeTesting {
     // privated method accesses can only be inlined in the same class
     for (m <- Set(rdC, rhC)) check(m, cCl, cCl, assertEmpty)
     for (m <- Set(rdC, rhC); c <- allClasses.tail) check(m, cCl, c, cOrDOwner)
+
+    // ensure the caches are empty at the end for the next test to run (`check` caches types by
+    // calling `classBTypeFromParsedClassfile`). note that per-run caches are cleared at the end
+    // of a compilation, not the beginning.
+    clearClassBTypeCaches()
   }
 }

test/junit/scala/tools/nsc/backend/jvm/opt/InlinerTest.scala

@@ -27,7 +27,8 @@ class InlinerTest extends BytecodeTesting {
   import global.genBCode.bTypes
   // allows inspecting the caches after a compilation run
   def notPerRun: List[Clearable] = List(
-    bTypes.classBTypeFromInternalName,
+    bTypes.classBTypeCacheFromSymbol,
+    bTypes.classBTypeCacheFromClassfile,
     bTypes.byteCodeRepository.compilingClasses,
     bTypes.byteCodeRepository.parsedClasses,
     bTypes.callGraph.callsites)