Permalink
Browse files

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.
  • Loading branch information...
xeno-by committed Jan 29, 2013
1 parent 981da8e commit 5b37cfb19a41fc1b775dbdaf247e765fb6d245e0
@@ -37,7 +37,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)
@@ -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()
+ }
+ }
+}
@@ -24,7 +24,7 @@ import scala.language.existentials
import scala.runtime.{ScalaRunTime, BoxesRunTime}
import scala.reflect.internal.util.Collections._
-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]]()
@@ -46,9 +46,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 */
@@ -662,7 +664,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 {
@@ -878,7 +880,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
@@ -73,6 +73,24 @@ private[reflect] trait SymbolLoaders { self: SymbolTable =>
0 < dp && dp < (name.length - 1)
}
+ // 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)
@@ -85,15 +103,21 @@ private[reflect] trait SymbolLoaders { self: SymbolTable =>
super.enter(sym)
}
- // 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 = {
- val e = super.lookupEntry(name)
- if (e != null)
- e
- else if (isInvalidClassName(name) || (negatives contains name))
- null
- else {
+ // 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 {
+ def lookupExisting: Option[ScopeEntry] = {
+ val e = super.lookupEntry(name)
+ if (e != null)
+ Some(e)
+ else if (isInvalidClassName(name) || (negatives contains name))
+ Some(null) // TODO: omg
+ else
+ None
+ }
+ def materialize: ScopeEntry = {
val path =
if (pkgClass.isEmptyPackageClass) name.toString
else pkgClass.fullName + "." + name
@@ -122,6 +146,7 @@ private[reflect] trait SymbolLoaders { self: SymbolTable =>
null
}
}
+ lookupExisting getOrElse materialize
}
}
@@ -8,7 +8,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.value) println("[reflect-compiler] "+msg)
@@ -8,6 +8,9 @@ private[reflect] trait SynchronizedOps extends internal.SymbolTable
// Names
+ // this lock isn't subsumed by the reflection GIL
+ // because there's no way newXXXName methods are going to call anything reflective
+ // therefore we don't have a danger of a deadlock from having a fine-grained lock for name creation
private lazy val nameLock = new Object
override def newTermName(s: String): TermName = nameLock.synchronized { super.newTermName(s) }
@@ -16,20 +19,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
@@ -38,15 +46,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 }
}
}
Oops, something went wrong.

1 comment on commit 5b37cfb

Job pr-rangepos-per-commit failed for 5b37cfb (results):


Took 11 s.
sad kitty
to rebuild, comment "PLS REBUILD/pr-rangepos-per-commit@5b37cfb19a41fc1b775dbdaf247e765fb6d245e0"on PR #2083

Please sign in to comment.