SI-6240 introduces GIL to Scala reflection

On a serious note, I feel really uncomfortable about having to juggle
this slew of locks. Despite that I can't immediately find a deadlock,
I'm 100% sure there is one hiding in the shadows. Hence, I'm abandoning
all runtime reflection locks in favor of a single per-universe one.

Author: xeno-by

src/reflect/scala/reflect/internal/BaseTypeSeqs.scala

@@ -38,7 +38,7 @@ trait BaseTypeSeqs {
    *  This is necessary because when run from reflection every base type sequence needs to have a
    *  SynchronizedBaseTypeSeq as mixin.
    */
-  class BaseTypeSeq protected[BaseTypeSeqs] (private[BaseTypeSeqs] val parents: List[Type], private[BaseTypeSeqs] val elems: Array[Type]) {
+  class BaseTypeSeq protected[reflect] (private[BaseTypeSeqs] val parents: List[Type], private[BaseTypeSeqs] val elems: Array[Type]) {
   self =>
     if (Statistics.canEnable) Statistics.incCounter(baseTypeSeqCount)
     if (Statistics.canEnable) Statistics.incCounter(baseTypeSeqLenTotal, elems.length)

src/reflect/scala/reflect/runtime/Gil.scala

@@ -0,0 +1,22 @@
+package scala.reflect
+package runtime
+
+private[reflect] trait Gil {
+  self: SymbolTable =>
+
+  // fixme... please...
+  // there are the following avenues of optimization we discussed with Roland:
+  // 1) replace PackageScope locks with ConcurrentHashMap, because PackageScope materializers seem to be idempotent
+  // 2) unlock unpickling completers by verifying that they are idempotent or moving non-idempotent parts
+  // 3) remove the necessity in global state for isSubType
+  lazy val gil = new java.util.concurrent.locks.ReentrantLock
+
+  @inline final def gilSynchronized[T](body: => T): T = {
+    try {
+      gil.lock()
+      body
+    } finally {
+      gil.unlock()
+    }
+  }
+}

src/reflect/scala/reflect/runtime/JavaMirrors.scala

@@ -22,7 +22,7 @@ import ReflectionUtils.{staticSingletonInstance, innerSingletonInstance, scalacS
 import scala.language.existentials
 import scala.runtime.{ScalaRunTime, BoxesRunTime}
 
-private[reflect] trait JavaMirrors extends internal.SymbolTable with api.JavaUniverse { thisUniverse: SymbolTable =>
+private[reflect] trait JavaMirrors extends internal.SymbolTable with api.JavaUniverse with TwoWayCaches { thisUniverse: SymbolTable =>
 
   private lazy val mirrors = new WeakHashMap[ClassLoader, WeakReference[JavaMirror]]()
 
@@ -44,9 +44,11 @@ private[reflect] trait JavaMirrors extends internal.SymbolTable with api.JavaUni
 
   trait JavaClassCompleter extends FlagAssigningCompleter
 
-  def runtimeMirror(cl: ClassLoader): Mirror = mirrors get cl match {
-    case Some(WeakReference(m)) => m
-    case _ => createMirror(rootMirror.RootClass, cl)
+  def runtimeMirror(cl: ClassLoader): Mirror = gilSynchronized {
+    mirrors get cl match {
+      case Some(WeakReference(m)) => m
+      case _ => createMirror(rootMirror.RootClass, cl)
+    }
   }
 
   /** The API of a mirror for a reflective universe */
@@ -684,7 +686,7 @@ private[reflect] trait JavaMirrors extends internal.SymbolTable with api.JavaUni
         completeRest()
       }
 
-      def completeRest(): Unit = thisUniverse.synchronized {
+      def completeRest(): Unit = gilSynchronized {
         val tparams = clazz.rawInfo.typeParams
 
         val parents = try {
@@ -889,7 +891,7 @@ private[reflect] trait JavaMirrors extends internal.SymbolTable with api.JavaUni
      * The Scala package with given fully qualified name. Unlike `packageNameToScala`,
      *  this one bypasses the cache.
      */
-    private[JavaMirrors] def makeScalaPackage(fullname: String): ModuleSymbol = {
+    private[JavaMirrors] def makeScalaPackage(fullname: String): ModuleSymbol = gilSynchronized {
       val split = fullname lastIndexOf '.'
       val ownerModule: ModuleSymbol =
         if (split > 0) packageNameToScala(fullname take split) else this.RootPackage

src/reflect/scala/reflect/runtime/JavaUniverseForce.scala

@@ -27,11 +27,10 @@ trait JavaUniverseForce { self: runtime.JavaUniverse  =>
 
     this.settings
     this.treeInfo
+    this.gil
     // inaccessible: this.uniqueLock
-    // inaccessible: this.subsametypeLock
-    // inaccessible: this.lubglbLock
-    // inaccessible: this.indentLock
-    // inaccessible: this.toStringLock
+    // inaccessible: this.nextIdLock
+    // inaccessible: this.freshExistentialNameLock
     // inaccessible: this.mirrors
     this.rootMirror
     this.treeBuild

src/reflect/scala/reflect/runtime/SymbolLoaders.scala

@@ -67,6 +67,25 @@ private[reflect] trait SymbolLoaders { self: SymbolTable =>
     }
   }
 
+
+  // Since runtime reflection doesn't have a luxury of enumerating all classes
+  // on the classpath, it has to materialize symbols for top-level definitions
+  // (packages, classes, objects) on demand.
+  //
+  // Someone asks us for a class named `foo.Bar`? Easy. Let's speculatively create
+  // a package named `foo` and then look up `newTypeName("bar")` in its decls.
+  // This lookup, implemented in `SymbolLoaders.PackageScope` tests the waters by
+  // trying to to `Class.forName("foo.Bar")` and then creates a ClassSymbol upon
+  // success (the whole story is a bit longer, but the rest is irrelevant here).
+  //
+  // That's all neat, but these non-deterministic mutations of the global symbol
+  // table give a lot of trouble in multi-threaded setting. One of the popular
+  // reflection crashes happens when multiple threads happen to trigger symbol
+  // materialization multiple times for the same symbol, making subsequent
+  // reflective operations stumble upon outrageous stuff like overloaded packages.
+  //
+  // Short of significantly changing SymbolLoaders I see no other way than just
+  // to slap a global lock on materialization in runtime reflection.
   class PackageScope(pkgClass: Symbol) extends Scope(initFingerPrints = -1L) // disable fingerprinting as we do not know entries beforehand
       with SynchronizedScope {
     assert(pkgClass.isType)
@@ -89,9 +108,11 @@ private[reflect] trait SymbolLoaders { self: SymbolTable =>
       else existing.sym.asInstanceOf[T]
     }
 
-    // disable fingerprinting as we do not know entries beforehand
-    private val negatives = mutable.Set[Name]() // Syncnote: Performance only, so need not be protected.
-    override def lookupEntry(name: Name): ScopeEntry = {
+    // package scopes need to synchronize on the GIL
+    // because lookupEntry might cause changes to the global symbol table
+    override def syncLockSynchronized[T](body: => T): T = gilSynchronized(body)
+    private val negatives = new mutable.HashSet[Name]
+    override def lookupEntry(name: Name): ScopeEntry = syncLockSynchronized {
       val e = super.lookupEntry(name)
       if (e != null)
         e

src/reflect/scala/reflect/runtime/SymbolTable.scala

@@ -9,7 +9,7 @@ import scala.reflect.internal.Flags._
  *  It can be used either from a reflexive universe (class scala.reflect.runtime.JavaUniverse), or else from
  *  a runtime compiler that uses reflection to get a class information (class scala.tools.reflect.ReflectGlobal)
  */
-private[scala] trait SymbolTable extends internal.SymbolTable with JavaMirrors with SymbolLoaders with SynchronizedOps {
+private[scala] trait SymbolTable extends internal.SymbolTable with JavaMirrors with SymbolLoaders with SynchronizedOps with Gil {
 
   def info(msg: => String) =
     if (settings.verbose) println("[reflect-compiler] "+msg)

src/reflect/scala/reflect/runtime/SynchronizedOps.scala

@@ -14,20 +14,25 @@ private[reflect] trait SynchronizedOps extends internal.SymbolTable
 // BaseTypeSeqs
 
   override protected def newBaseTypeSeq(parents: List[Type], elems: Array[Type]) =
-    new BaseTypeSeq(parents, elems) with SynchronizedBaseTypeSeq
+    // only need to synchronize BaseTypeSeqs if they contain refined types
+    if (elems.filter(_.isInstanceOf[RefinedType]).nonEmpty) new BaseTypeSeq(parents, elems) with SynchronizedBaseTypeSeq
+    else new BaseTypeSeq(parents, elems)
 
   trait SynchronizedBaseTypeSeq extends BaseTypeSeq {
-    override def apply(i: Int): Type = synchronized { super.apply(i) }
-    override def rawElem(i: Int) = synchronized { super.rawElem(i) }
-    override def typeSymbol(i: Int): Symbol = synchronized { super.typeSymbol(i) }
-    override def toList: List[Type] = synchronized { super.toList }
-    override def copy(head: Type, offset: Int): BaseTypeSeq = synchronized { super.copy(head, offset) }
-    override def map(f: Type => Type): BaseTypeSeq = synchronized { super.map(f) }
-    override def exists(p: Type => Boolean): Boolean = synchronized { super.exists(p) }
-    override lazy val maxDepth = synchronized { maxDepthOfElems }
-    override def toString = synchronized { super.toString }
-
-    override def lateMap(f: Type => Type): BaseTypeSeq = new MappedBaseTypeSeq(this, f) with SynchronizedBaseTypeSeq
+    override def apply(i: Int): Type = gilSynchronized { super.apply(i) }
+    override def rawElem(i: Int) = gilSynchronized { super.rawElem(i) }
+    override def typeSymbol(i: Int): Symbol = gilSynchronized { super.typeSymbol(i) }
+    override def toList: List[Type] = gilSynchronized { super.toList }
+    override def copy(head: Type, offset: Int): BaseTypeSeq = gilSynchronized { super.copy(head, offset) }
+    override def map(f: Type => Type): BaseTypeSeq = gilSynchronized { super.map(f) }
+    override def exists(p: Type => Boolean): Boolean = gilSynchronized { super.exists(p) }
+    override lazy val maxDepth = gilSynchronized { maxDepthOfElems }
+    override def toString = gilSynchronized { super.toString }
+
+    override def lateMap(f: Type => Type): BaseTypeSeq =
+      // only need to synchronize BaseTypeSeqs if they contain refined types
+      if (map(f).toList.filter(_.isInstanceOf[RefinedType]).nonEmpty) new MappedBaseTypeSeq(this, f) with SynchronizedBaseTypeSeq
+      else new MappedBaseTypeSeq(this, f)
   }
 
 // Scopes
@@ -36,15 +41,19 @@ private[reflect] trait SynchronizedOps extends internal.SymbolTable
   override def newNestedScope(outer: Scope): Scope = new Scope(outer) with SynchronizedScope
 
   trait SynchronizedScope extends Scope {
-    override def isEmpty: Boolean = synchronized { super.isEmpty }
-    override def size: Int = synchronized { super.size }
-    override def enter[T <: Symbol](sym: T): T = synchronized { super.enter(sym) }
-    override def rehash(sym: Symbol, newname: Name) = synchronized { super.rehash(sym, newname) }
-    override def unlink(e: ScopeEntry) = synchronized { super.unlink(e) }
-    override def unlink(sym: Symbol) = synchronized { super.unlink(sym) }
-    override def lookupAll(name: Name) = synchronized { super.lookupAll(name) }
-    override def lookupEntry(name: Name) = synchronized { super.lookupEntry(name) }
-    override def lookupNextEntry(entry: ScopeEntry) = synchronized { super.lookupNextEntry(entry) }
-    override def toList: List[Symbol] = synchronized { super.toList }
+    // we can keep this lock fine-grained, because methods of Scope don't do anything extraordinary, which makes deadlocks impossible
+    // fancy subclasses of internal.Scopes#Scope should do synchronization themselves (e.g. see PackageScope for an example)
+    private lazy val syncLock = new Object
+    def syncLockSynchronized[T](body: => T): T = syncLock.synchronized { body }
+    override def isEmpty: Boolean = syncLockSynchronized { super.isEmpty }
+    override def size: Int = syncLockSynchronized { super.size }
+    override def enter[T <: Symbol](sym: T): T = syncLockSynchronized { super.enter(sym) }
+    override def rehash(sym: Symbol, newname: Name) = syncLockSynchronized { super.rehash(sym, newname) }
+    override def unlink(e: ScopeEntry) = syncLockSynchronized { super.unlink(e) }
+    override def unlink(sym: Symbol) = syncLockSynchronized { super.unlink(sym) }
+    override def lookupAll(name: Name) = syncLockSynchronized { super.lookupAll(name) }
+    override def lookupEntry(name: Name) = syncLockSynchronized { super.lookupEntry(name) }
+    override def lookupNextEntry(entry: ScopeEntry) = syncLockSynchronized { super.lookupNextEntry(entry) }
+    override def toList: List[Symbol] = syncLockSynchronized { super.toList }
   }
 }