Skip to content

Commit d1219e1

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 59102b4 commit d1219e1

8 files changed

+166
-111
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ trait BaseTypeSeqs {
3737
* This is necessary because when run from reflection every base type sequence needs to have a
3838
* SynchronizedBaseTypeSeq as mixin.
3939
*/
40-
class BaseTypeSeq protected[BaseTypeSeqs] (private[BaseTypeSeqs] val parents: List[Type], private[BaseTypeSeqs] val elems: Array[Type]) {
40+
class BaseTypeSeq protected[reflect] (private[BaseTypeSeqs] val parents: List[Type], private[BaseTypeSeqs] val elems: Array[Type]) {
4141
self =>
4242
if (Statistics.canEnable) Statistics.incCounter(baseTypeSeqCount)
4343
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: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -50,9 +50,11 @@ private[reflect] trait JavaMirrors extends internal.SymbolTable with api.JavaUni
5050
definitions.init()
5151
}
5252

53-
def runtimeMirror(cl: ClassLoader): Mirror = mirrors get cl match {
54-
case Some(WeakReference(m)) => m
55-
case _ => createMirror(rootMirror.RootClass, cl)
53+
def runtimeMirror(cl: ClassLoader): Mirror = gilSynchronized {
54+
mirrors get cl match {
55+
case Some(WeakReference(m)) => m
56+
case _ => createMirror(rootMirror.RootClass, cl)
57+
}
5658
}
5759

5860
/** The API of a mirror for a reflective universe */
@@ -664,7 +666,7 @@ private[reflect] trait JavaMirrors extends internal.SymbolTable with api.JavaUni
664666
completeRest()
665667
}
666668

667-
def completeRest(): Unit = thisUniverse.synchronized {
669+
def completeRest(): Unit = gilSynchronized {
668670
val tparams = clazz.rawInfo.typeParams
669671

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

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

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

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

88-
// disable fingerprinting as we do not know entries beforehand
89-
private val negatives = mutable.Set[Name]() // Syncnote: Performance only, so need not be protected.
90-
override def lookupEntry(name: Name): ScopeEntry = {
91-
val e = super.lookupEntry(name)
92-
if (e != null)
93-
e
94-
else if (isInvalidClassName(name) || (negatives contains name))
95-
null
96-
else {
106+
// package scopes need to synchronize on the GIL
107+
// because lookupEntry might cause changes to the global symbol table
108+
override def syncLockSynchronized[T](body: => T): T = gilSynchronized(body)
109+
private val negatives = new mutable.HashSet[Name]
110+
override def lookupEntry(name: Name): ScopeEntry = syncLockSynchronized {
111+
def lookupExisting: Option[ScopeEntry] = {
112+
val e = super.lookupEntry(name)
113+
if (e != null)
114+
Some(e)
115+
else if (isInvalidClassName(name) || (negatives contains name))
116+
Some(null) // TODO: omg
117+
else
118+
None
119+
}
120+
def materialize: ScopeEntry = {
97121
val path =
98122
if (pkgClass.isEmptyPackageClass) name.toString
99123
else pkgClass.fullName + "." + name
@@ -122,6 +146,7 @@ private[reflect] trait SymbolLoaders { self: SymbolTable =>
122146
null
123147
}
124148
}
149+
lookupExisting getOrElse materialize
125150
}
126151
}
127152

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

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

1313
def info(msg: => String) =
1414
if (settings.verbose.value) 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
@@ -8,6 +8,9 @@ private[reflect] trait SynchronizedOps extends internal.SymbolTable
88

99
// Names
1010

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

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

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

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

3543
// Scopes
@@ -38,15 +46,19 @@ private[reflect] trait SynchronizedOps extends internal.SymbolTable
3846
override def newNestedScope(outer: Scope): Scope = new Scope(outer) with SynchronizedScope
3947

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

0 commit comments

Comments
 (0)