Skip to content

Commit ba3fc21

Browse files
committed
introducing 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 18ffcf2 commit ba3fc21

8 files changed

+151
-126
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: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -50,8 +50,7 @@ private[reflect] trait JavaMirrors extends internal.SymbolTable with api.JavaUni
5050
definitions.init()
5151
}
5252

53-
private lazy val mirrorLock = new Object
54-
def runtimeMirror(cl: ClassLoader): Mirror = mirrorLock.synchronized {
53+
def runtimeMirror(cl: ClassLoader): Mirror = gilSynchronized {
5554
mirrors get cl match {
5655
case Some(WeakReference(m)) => m
5756
case _ => createMirror(rootMirror.RootClass, cl)
@@ -677,7 +676,7 @@ private[reflect] trait JavaMirrors extends internal.SymbolTable with api.JavaUni
677676
completeRest()
678677
}
679678

680-
def completeRest(): Unit = thisMirror.synchronized {
679+
def completeRest(): Unit = gilSynchronized {
681680
val tparams = clazz.rawInfo.typeParams
682681

683682
val parents = try {
@@ -893,7 +892,7 @@ private[reflect] trait JavaMirrors extends internal.SymbolTable with api.JavaUni
893892
* The Scala package with given fully qualified name. Unlike `packageNameToScala`,
894893
* this one bypasses the cache.
895894
*/
896-
private[JavaMirrors] def makeScalaPackage(fullname: String): ModuleSymbol = thisMirror.synchronized {
895+
private[JavaMirrors] def makeScalaPackage(fullname: String): ModuleSymbol = gilSynchronized {
897896
val split = fullname lastIndexOf '.'
898897
val ownerModule: ModuleSymbol =
899898
if (split > 0) packageNameToScala(fullname take split) else this.RootPackage

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

Lines changed: 23 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ private[reflect] trait SymbolLoaders { self: SymbolTable =>
7676
}
7777

7878
class PackageScope(pkgClass: Symbol) extends Scope(initFingerPrints = -1L) // disable fingerprinting as we do not know entries beforehand
79-
with SynchronizedScope {
79+
with SynchronizedScope {
8080
assert(pkgClass.isType)
8181

8282
// materializing multiple copies of the same symbol in PackageScope is a very popular bug
@@ -87,9 +87,11 @@ private[reflect] trait SymbolLoaders { self: SymbolTable =>
8787
super.enter(sym)
8888
}
8989

90-
// disable fingerprinting as we do not know entries beforehand
91-
private val negatives = new mutable.HashSet[Name] with mutable.SynchronizedSet[Name] // Syncnote: Performance only, so need not be protected.
92-
override def lookupEntry(name: Name): ScopeEntry = {
90+
// package scopes need to synchronize on the GIL
91+
// because lookupEntry might cause changes to the global symbol table
92+
override def syncLockSynchronized[T](body: => T): T = gilSynchronized(body)
93+
private val negatives = new mutable.HashSet[Name]
94+
override def lookupEntry(name: Name): ScopeEntry = syncLockSynchronized {
9395
def lookupExisting: Option[ScopeEntry] = {
9496
val e = super.lookupEntry(name)
9597
if (e != null)
@@ -99,41 +101,37 @@ private[reflect] trait SymbolLoaders { self: SymbolTable =>
99101
else
100102
None
101103
}
102-
103104
def materialize: ScopeEntry = {
104105
val path =
105106
if (pkgClass.isEmptyPackageClass) name.toString
106107
else pkgClass.fullName + "." + name
107108
val currentMirror = mirrorThatLoaded(pkgClass)
108109
currentMirror.tryJavaClass(path) match {
109110
case Some(cls) =>
110-
currentMirror.synchronized {
111-
lookupExisting getOrElse {
112-
val loadingMirror = currentMirror.mirrorDefining(cls)
113-
val (clazz, module) =
114-
if (loadingMirror eq currentMirror) {
115-
initAndEnterClassAndModule(pkgClass, name.toTypeName, new TopClassCompleter(_, _))
116-
} else {
117-
val origOwner = loadingMirror.packageNameToScala(pkgClass.fullName)
118-
val clazz = origOwner.info decl name.toTypeName
119-
val module = origOwner.info decl name.toTermName
120-
assert(clazz != NoSymbol)
121-
assert(module != NoSymbol)
122-
pkgClass.info.decls enter clazz
123-
pkgClass.info.decls enter module
124-
(clazz, module)
125-
}
126-
debugInfo(s"created $module/${module.moduleClass} in $pkgClass")
127-
lookupEntry(name)
128-
}
111+
lookupExisting getOrElse {
112+
val loadingMirror = currentMirror.mirrorDefining(cls)
113+
val (clazz, module) =
114+
if (loadingMirror eq currentMirror) {
115+
initAndEnterClassAndModule(pkgClass, name.toTypeName, new TopClassCompleter(_, _))
116+
} else {
117+
val origOwner = loadingMirror.packageNameToScala(pkgClass.fullName)
118+
val clazz = origOwner.info decl name.toTypeName
119+
val module = origOwner.info decl name.toTermName
120+
assert(clazz != NoSymbol)
121+
assert(module != NoSymbol)
122+
pkgClass.info.decls enter clazz
123+
pkgClass.info.decls enter module
124+
(clazz, module)
125+
}
126+
debugInfo(s"created $module/${module.moduleClass} in $pkgClass")
127+
lookupEntry(name)
129128
}
130129
case none =>
131130
debugInfo("*** not found : "+path)
132131
negatives += name
133132
null
134133
}
135134
}
136-
137135
lookupExisting getOrElse materialize
138136
}
139137
}

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)