Skip to content

Commit 85f3bce

Browse files
committed
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.
1 parent 279709c commit 85f3bce

File tree

9 files changed

+225
-107
lines changed

9 files changed

+225
-107
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ trait BaseTypeSeqs {
3838
* This is necessary because when run from reflection every base type sequence needs to have a
3939
* SynchronizedBaseTypeSeq as mixin.
4040
*/
41-
class BaseTypeSeq protected[BaseTypeSeqs] (private[BaseTypeSeqs] val parents: List[Type], private[BaseTypeSeqs] val elems: Array[Type]) {
41+
class BaseTypeSeq protected[reflect] (private[BaseTypeSeqs] val parents: List[Type], private[BaseTypeSeqs] val elems: Array[Type]) {
4242
self =>
4343
if (Statistics.canEnable) Statistics.incCounter(baseTypeSeqCount)
4444
if (Statistics.canEnable) Statistics.incCounter(baseTypeSeqLenTotal, elems.length)
Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
package scala.reflect
2+
package runtime
3+
4+
private[reflect] trait Gil {
5+
self: SymbolTable =>
6+
7+
// fixme... please...
8+
// there are the following avenues of optimization we discussed with Roland:
9+
// 1) replace PackageScope locks with ConcurrentHashMap, because PackageScope materializers seem to be idempotent
10+
// 2) unlock unpickling completers by verifying that they are idempotent or moving non-idempotent parts
11+
// 3) remove the necessity in global state for isSubType
12+
lazy val gil = new java.util.concurrent.locks.ReentrantLock
13+
14+
@inline final def gilSynchronized[T](body: => T): T = {
15+
try {
16+
gil.lock()
17+
body
18+
} finally {
19+
gil.unlock()
20+
}
21+
}
22+
}

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

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ import ReflectionUtils.{staticSingletonInstance, innerSingletonInstance}
2222
import scala.language.existentials
2323
import scala.runtime.{ScalaRunTime, BoxesRunTime}
2424

25-
private[reflect] trait JavaMirrors extends internal.SymbolTable with api.JavaUniverse { thisUniverse: SymbolTable =>
25+
private[reflect] trait JavaMirrors extends internal.SymbolTable with api.JavaUniverse with TwoWayCaches { thisUniverse: SymbolTable =>
2626

2727
private lazy val mirrors = new WeakHashMap[ClassLoader, WeakReference[JavaMirror]]()
2828

@@ -44,9 +44,11 @@ private[reflect] trait JavaMirrors extends internal.SymbolTable with api.JavaUni
4444

4545
trait JavaClassCompleter extends FlagAssigningCompleter
4646

47-
def runtimeMirror(cl: ClassLoader): Mirror = mirrors get cl match {
48-
case Some(WeakReference(m)) => m
49-
case _ => createMirror(rootMirror.RootClass, cl)
47+
def runtimeMirror(cl: ClassLoader): Mirror = gilSynchronized {
48+
mirrors get cl match {
49+
case Some(WeakReference(m)) => m
50+
case _ => createMirror(rootMirror.RootClass, cl)
51+
}
5052
}
5153

5254
/** The API of a mirror for a reflective universe */
@@ -685,7 +687,7 @@ private[reflect] trait JavaMirrors extends internal.SymbolTable with api.JavaUni
685687
completeRest()
686688
}
687689

688-
def completeRest(): Unit = thisUniverse.synchronized {
690+
def completeRest(): Unit = gilSynchronized {
689691
val tparams = clazz.rawInfo.typeParams
690692

691693
val parents = try {
@@ -881,7 +883,7 @@ private[reflect] trait JavaMirrors extends internal.SymbolTable with api.JavaUni
881883
* The Scala package with given fully qualified name. Unlike `packageNameToScala`,
882884
* this one bypasses the cache.
883885
*/
884-
private[JavaMirrors] def makeScalaPackage(fullname: String): ModuleSymbol = {
886+
private[JavaMirrors] def makeScalaPackage(fullname: String): ModuleSymbol = gilSynchronized {
885887
val split = fullname lastIndexOf '.'
886888
val ownerModule: ModuleSymbol =
887889
if (split > 0) packageNameToScala(fullname take split) else this.RootPackage

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

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,24 @@ private[reflect] trait SymbolLoaders { self: SymbolTable =>
7474
0 < dp && dp < (name.length - 1)
7575
}
7676

77+
// Since runtime reflection doesn't have a luxury of enumerating all classes
78+
// on the classpath, it has to materialize symbols for top-level definitions
79+
// (packages, classes, objects) on demand.
80+
//
81+
// Someone asks us for a class named `foo.Bar`? Easy. Let's speculatively create
82+
// a package named `foo` and then look up `newTypeName("bar")` in its decls.
83+
// This lookup, implemented in `SymbolLoaders.PackageScope` tests the waters by
84+
// trying to to `Class.forName("foo.Bar")` and then creates a ClassSymbol upon
85+
// success (the whole story is a bit longer, but the rest is irrelevant here).
86+
//
87+
// That's all neat, but these non-deterministic mutations of the global symbol
88+
// table give a lot of trouble in multi-threaded setting. One of the popular
89+
// reflection crashes happens when multiple threads happen to trigger symbol
90+
// materialization multiple times for the same symbol, making subsequent
91+
// reflective operations stumble upon outrageous stuff like overloaded packages.
92+
//
93+
// Short of significantly changing SymbolLoaders I see no other way than just
94+
// to slap a global lock on materialization in runtime reflection.
7795
class PackageScope(pkgClass: Symbol) extends Scope(initFingerPrints = -1L) // disable fingerprinting as we do not know entries beforehand
7896
with SynchronizedScope {
7997
assert(pkgClass.isType)
@@ -96,9 +114,11 @@ private[reflect] trait SymbolLoaders { self: SymbolTable =>
96114
else existing.sym.asInstanceOf[T]
97115
}
98116

99-
// disable fingerprinting as we do not know entries beforehand
100-
private val negatives = mutable.Set[Name]() // Syncnote: Performance only, so need not be protected.
101-
override def lookupEntry(name: Name): ScopeEntry = {
117+
// package scopes need to synchronize on the GIL
118+
// because lookupEntry might cause changes to the global symbol table
119+
override def syncLockSynchronized[T](body: => T): T = gilSynchronized(body)
120+
private val negatives = new mutable.HashSet[Name]
121+
override def lookupEntry(name: Name): ScopeEntry = syncLockSynchronized {
102122
val e = super.lookupEntry(name)
103123
if (e != null)
104124
e

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import scala.reflect.internal.Flags._
99
* It can be used either from a reflexive universe (class scala.reflect.runtime.JavaUniverse), or else from
1010
* a runtime compiler that uses reflection to get a class information (class scala.tools.reflect.ReflectGlobal)
1111
*/
12-
private[scala] trait SymbolTable extends internal.SymbolTable with JavaMirrors with SymbolLoaders with SynchronizedOps {
12+
private[scala] trait SymbolTable extends internal.SymbolTable with JavaMirrors with SymbolLoaders with SynchronizedOps with Gil {
1313

1414
def info(msg: => String) =
1515
if (settings.verbose) println("[reflect-compiler] "+msg)

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

Lines changed: 34 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,9 @@ private[reflect] trait SynchronizedOps extends internal.SymbolTable
99

1010
// Names
1111

12+
// this lock isn't subsumed by the reflection GIL
13+
// because there's no way newXXXName methods are going to call anything reflective
14+
// therefore we don't have a danger of a deadlock from having a fine-grained lock for name creation
1215
private lazy val nameLock = new Object
1316

1417
override def newTermName(s: String): TermName = nameLock.synchronized { super.newTermName(s) }
@@ -17,20 +20,25 @@ private[reflect] trait SynchronizedOps extends internal.SymbolTable
1720
// BaseTypeSeqs
1821

1922
override protected def newBaseTypeSeq(parents: List[Type], elems: Array[Type]) =
20-
new BaseTypeSeq(parents, elems) with SynchronizedBaseTypeSeq
23+
// only need to synchronize BaseTypeSeqs if they contain refined types
24+
if (elems.filter(_.isInstanceOf[RefinedType]).nonEmpty) new BaseTypeSeq(parents, elems) with SynchronizedBaseTypeSeq
25+
else new BaseTypeSeq(parents, elems)
2126

2227
trait SynchronizedBaseTypeSeq extends BaseTypeSeq {
23-
override def apply(i: Int): Type = synchronized { super.apply(i) }
24-
override def rawElem(i: Int) = synchronized { super.rawElem(i) }
25-
override def typeSymbol(i: Int): Symbol = synchronized { super.typeSymbol(i) }
26-
override def toList: List[Type] = synchronized { super.toList }
27-
override def copy(head: Type, offset: Int): BaseTypeSeq = synchronized { super.copy(head, offset) }
28-
override def map(f: Type => Type): BaseTypeSeq = synchronized { super.map(f) }
29-
override def exists(p: Type => Boolean): Boolean = synchronized { super.exists(p) }
30-
override lazy val maxDepth = synchronized { maxDepthOfElems }
31-
override def toString = synchronized { super.toString }
32-
33-
override def lateMap(f: Type => Type): BaseTypeSeq = new MappedBaseTypeSeq(this, f) with SynchronizedBaseTypeSeq
28+
override def apply(i: Int): Type = gilSynchronized { super.apply(i) }
29+
override def rawElem(i: Int) = gilSynchronized { super.rawElem(i) }
30+
override def typeSymbol(i: Int): Symbol = gilSynchronized { super.typeSymbol(i) }
31+
override def toList: List[Type] = gilSynchronized { super.toList }
32+
override def copy(head: Type, offset: Int): BaseTypeSeq = gilSynchronized { super.copy(head, offset) }
33+
override def map(f: Type => Type): BaseTypeSeq = gilSynchronized { super.map(f) }
34+
override def exists(p: Type => Boolean): Boolean = gilSynchronized { super.exists(p) }
35+
override lazy val maxDepth = gilSynchronized { maxDepthOfElems }
36+
override def toString = gilSynchronized { super.toString }
37+
38+
override def lateMap(f: Type => Type): BaseTypeSeq =
39+
// only need to synchronize BaseTypeSeqs if they contain refined types
40+
if (map(f).toList.filter(_.isInstanceOf[RefinedType]).nonEmpty) new MappedBaseTypeSeq(this, f) with SynchronizedBaseTypeSeq
41+
else new MappedBaseTypeSeq(this, f)
3442
}
3543

3644
// Scopes
@@ -39,15 +47,19 @@ private[reflect] trait SynchronizedOps extends internal.SymbolTable
3947
override def newNestedScope(outer: Scope): Scope = new Scope(outer) with SynchronizedScope
4048

4149
trait SynchronizedScope extends Scope {
42-
override def isEmpty: Boolean = synchronized { super.isEmpty }
43-
override def size: Int = synchronized { super.size }
44-
override def enter[T <: Symbol](sym: T): T = synchronized { super.enter(sym) }
45-
override def rehash(sym: Symbol, newname: Name) = synchronized { super.rehash(sym, newname) }
46-
override def unlink(e: ScopeEntry) = synchronized { super.unlink(e) }
47-
override def unlink(sym: Symbol) = synchronized { super.unlink(sym) }
48-
override def lookupAll(name: Name) = synchronized { super.lookupAll(name) }
49-
override def lookupEntry(name: Name) = synchronized { super.lookupEntry(name) }
50-
override def lookupNextEntry(entry: ScopeEntry) = synchronized { super.lookupNextEntry(entry) }
51-
override def toList: List[Symbol] = synchronized { super.toList }
50+
// we can keep this lock fine-grained, because methods of Scope don't do anything extraordinary, which makes deadlocks impossible
51+
// fancy subclasses of internal.Scopes#Scope should do synchronization themselves (e.g. see PackageScope for an example)
52+
private lazy val syncLock = new Object
53+
def syncLockSynchronized[T](body: => T): T = syncLock.synchronized { body }
54+
override def isEmpty: Boolean = syncLockSynchronized { super.isEmpty }
55+
override def size: Int = syncLockSynchronized { super.size }
56+
override def enter[T <: Symbol](sym: T): T = syncLockSynchronized { super.enter(sym) }
57+
override def rehash(sym: Symbol, newname: Name) = syncLockSynchronized { super.rehash(sym, newname) }
58+
override def unlink(e: ScopeEntry) = syncLockSynchronized { super.unlink(e) }
59+
override def unlink(sym: Symbol) = syncLockSynchronized { super.unlink(sym) }
60+
override def lookupAll(name: Name) = syncLockSynchronized { super.lookupAll(name) }
61+
override def lookupEntry(name: Name) = syncLockSynchronized { super.lookupEntry(name) }
62+
override def lookupNextEntry(entry: ScopeEntry) = syncLockSynchronized { super.lookupNextEntry(entry) }
63+
override def toList: List[Symbol] = syncLockSynchronized { super.toList }
5264
}
5365
}

0 commit comments

Comments
 (0)