From d3c8a0bc81ea45177cee9a487c2c1739dbbdafff Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Thu, 22 Aug 2013 12:57:27 +0200 Subject: SI-6240 Synchronizes Names Previously we didn't have all possible name creation facilities covered with locks, so some of them silently misbehaved and caused much grief: http://groups.google.com/group/scala-internals/browse_thread/thread/ec1d3e2c4bcb000a. This patch gets all the name factories under control. Rather than relying on subclasses to override mutating methods and add synchronization, they should instead override `synchronizeNames` to return true and leave the placement of the locks up to `reflect.internal.Names`. These locks are placed around sections that mutate `typeHashtable` and `termHashtable`. This is done in the reflection universe, and in the interactive compiler. The latter change will obviate an incomplete attempt do to the same in `ScalaPresentationCompiler` in the scala-ide project. --- .../scala/tools/nsc/interactive/Global.scala | 1 + src/reflect/scala/reflect/internal/Names.scala | 85 ++++++++++++++-------- .../scala/reflect/runtime/SynchronizedOps.scala | 5 +- 3 files changed, 58 insertions(+), 33 deletions(-) diff --git a/src/interactive/scala/tools/nsc/interactive/Global.scala b/src/interactive/scala/tools/nsc/interactive/Global.scala index 492f0f4fb4..721fbf24c7 100644 --- a/src/interactive/scala/tools/nsc/interactive/Global.scala +++ b/src/interactive/scala/tools/nsc/interactive/Global.scala @@ -140,6 +140,7 @@ class Global(settings: Settings, _reporter: Reporter, projectName: String = "") abort("originalOwner is not kept in presentation compiler runs.") override def forInteractive = true + override protected def synchronizeNames = true override def newAsSeenFromMap(pre: Type, clazz: Symbol): AsSeenFromMap = new InteractiveAsSeenFromMap(pre, clazz) diff --git a/src/reflect/scala/reflect/internal/Names.scala b/src/reflect/scala/reflect/internal/Names.scala index 0d78e24548..b5116f8b67 100644 --- a/src/reflect/scala/reflect/internal/Names.scala +++ b/src/reflect/scala/reflect/internal/Names.scala @@ -18,6 +18,18 @@ trait Names extends api.Names { final val nameDebug = false + // Ideally we would just synchronize unconditionally and let HotSpot's Biased Locking + // kick in in the compiler universe, where access to the lock is single threaded. But, + // objects created in the first 4seconds of the JVM startup aren't eligible for biased + // locking. + // + // We might also be able to accept the performance hit, but we don't have tools to + // detect performance regressions. + // + // Discussion: https://groups.google.com/forum/#!search/biased$20scala-internals/scala-internals/0cYB7SkJ-nM/47MLhsgw8jwJ + protected def synchronizeNames: Boolean = false + private val nameLock: Object = new {} + /** Memory to store all names sequentially. */ var chrs: Array[Char] = new Array[Char](NAME_SIZE) private var nc = 0 @@ -76,22 +88,26 @@ trait Names extends api.Names { * string is not at the very end.) */ protected def newTermName(cs: Array[Char], offset: Int, len: Int, cachedString: String): TermName = { - val h = hashValue(cs, offset, len) & HASH_MASK - var n = termHashtable(h) - while ((n ne null) && (n.length != len || !equals(n.start, cs, offset, len))) - n = n.next - - if (n ne null) n - else { - // The logic order here is future-proofing against the possibility - // that name.toString will become an eager val, in which case the call - // to enterChars cannot follow the construction of the TermName. - val ncStart = nc - enterChars(cs, offset, len) - if (cachedString ne null) new TermName_S(ncStart, len, h, cachedString) - else new TermName_R(ncStart, len, h) + def body = { + val h = hashValue(cs, offset, len) & HASH_MASK + var n = termHashtable(h) + while ((n ne null) && (n.length != len || !equals(n.start, cs, offset, len))) + n = n.next + + if (n ne null) n + else { + // The logic order here is future-proofing against the possibility + // that name.toString will become an eager val, in which case the call + // to enterChars cannot follow the construction of the TermName. + val ncStart = nc + enterChars(cs, offset, len) + if (cachedString ne null) new TermName_S(ncStart, len, h, cachedString) + else new TermName_R(ncStart, len, h) + } } + if (synchronizeNames) nameLock.synchronized(body) else body } + protected def newTypeName(cs: Array[Char], offset: Int, len: Int, cachedString: String): TypeName = newTermName(cs, offset, len, cachedString).toTypeName @@ -128,6 +144,8 @@ trait Names extends api.Names { * For those of METHOD sort, its descriptor is stored ie has a leading '(' * * can-multi-thread + * TODO SI-6240 !!! JZ Really? the constructors TermName and TypeName publish unconstructed `this` references + * into the hash tables; we could observe them here before the subclass constructor completes. */ final def lookupTypeName(cs: Array[Char]): TypeName = { lookupTypeNameIfExisting(cs, true) } @@ -494,23 +512,26 @@ trait Names extends api.Names { override def toString = new String(chrs, index, len) } + // SYNCNOTE: caller to constructor must synchronize if `synchronizeNames` is enabled sealed abstract class TermName(index0: Int, len0: Int, hash: Int) extends Name(index0, len0) { type ThisNameType = TermName protected[this] def thisName: TermName = this - val next: TermName = termHashtable(hash) termHashtable(hash) = this def isTermName: Boolean = true def isTypeName: Boolean = false def toTermName: TermName = this def toTypeName: TypeName = { - val h = hashValue(chrs, index, len) & HASH_MASK - var n = typeHashtable(h) - while ((n ne null) && n.start != index) - n = n.next - - if (n ne null) n - else createCompanionName(h) + def body = { + val h = hashValue(chrs, index, len) & HASH_MASK + var n = typeHashtable(h) + while ((n ne null) && n.start != index) + n = n.next + + if (n ne null) n + else createCompanionName(h) + } + if (synchronizeNames) nameLock.synchronized(body) else body } def newName(str: String): TermName = newTermName(str) def companionName: TypeName = toTypeName @@ -518,6 +539,7 @@ trait Names extends api.Names { newTermName(chrs, start + from, to - from) def nameKind = "term" + /** SYNCNOTE: caller must synchronize if `synchronizeNames` is enabled */ protected def createCompanionName(h: Int): TypeName } @@ -534,16 +556,20 @@ trait Names extends api.Names { val next: TypeName = typeHashtable(hash) typeHashtable(hash) = this + def isTermName: Boolean = false def isTypeName: Boolean = true def toTermName: TermName = { - val h = hashValue(chrs, index, len) & HASH_MASK - var n = termHashtable(h) - while ((n ne null) && n.start != index) - n = n.next - - if (n ne null) n - else createCompanionName(h) + def body = { + val h = hashValue(chrs, index, len) & HASH_MASK + var n = termHashtable(h) + while ((n ne null) && n.start != index) + n = n.next + + if (n ne null) n + else createCompanionName(h) + } + if (synchronizeNames) nameLock.synchronized(body) else body } def toTypeName: TypeName = this def newName(str: String): TypeName = newTypeName(str) @@ -553,6 +579,7 @@ trait Names extends api.Names { def nameKind = "type" override def decode = if (nameDebug) super.decode + "!" else super.decode + /** SYNCNOTE: caller must synchronize if `synchronizeNames` is enabled */ protected def createCompanionName(h: Int): TermName } diff --git a/src/reflect/scala/reflect/runtime/SynchronizedOps.scala b/src/reflect/scala/reflect/runtime/SynchronizedOps.scala index 132470b2e7..6aa47a0405 100644 --- a/src/reflect/scala/reflect/runtime/SynchronizedOps.scala +++ b/src/reflect/scala/reflect/runtime/SynchronizedOps.scala @@ -9,10 +9,7 @@ private[reflect] trait SynchronizedOps extends internal.SymbolTable // Names - private lazy val nameLock = new Object - - override def newTermName(s: String): TermName = nameLock.synchronized { super.newTermName(s) } - override def newTypeName(s: String): TypeName = nameLock.synchronized { super.newTypeName(s) } + override protected def synchronizeNames = true // BaseTypeSeqs -- cgit v1.2.3 From 865591bf56a34e0db1de7e6c7b491d2494a97ee3 Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Thu, 22 Aug 2013 12:57:27 +0200 Subject: Lock down methods in Names Marking some methods as final. Once known to be overriden in scala-ide are instead marked with @deprecatedOveriding. This is to signal the new means of synchronization to subclasses. --- src/reflect/scala/reflect/internal/Names.scala | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/src/reflect/scala/reflect/internal/Names.scala b/src/reflect/scala/reflect/internal/Names.scala index b5116f8b67..ed248d6e1e 100644 --- a/src/reflect/scala/reflect/internal/Names.scala +++ b/src/reflect/scala/reflect/internal/Names.scala @@ -28,7 +28,7 @@ trait Names extends api.Names { // // Discussion: https://groups.google.com/forum/#!search/biased$20scala-internals/scala-internals/0cYB7SkJ-nM/47MLhsgw8jwJ protected def synchronizeNames: Boolean = false - private val nameLock: Object = new {} + private val nameLock: Object = new Object /** Memory to store all names sequentially. */ var chrs: Array[Char] = new Array[Char](NAME_SIZE) @@ -76,18 +76,19 @@ trait Names extends api.Names { } /** Create a term name from the characters in cs[offset..offset+len-1]. */ - def newTermName(cs: Array[Char], offset: Int, len: Int): TermName = + final def newTermName(cs: Array[Char], offset: Int, len: Int): TermName = newTermName(cs, offset, len, cachedString = null) - def newTermName(cs: Array[Char]): TermName = newTermName(cs, 0, cs.length) - def newTypeName(cs: Array[Char]): TypeName = newTypeName(cs, 0, cs.length) + final def newTermName(cs: Array[Char]): TermName = newTermName(cs, 0, cs.length) + + final def newTypeName(cs: Array[Char]): TypeName = newTypeName(cs, 0, cs.length) /** Create a term name from the characters in cs[offset..offset+len-1]. * TODO - have a mode where name validation is performed at creation time * (e.g. if a name has the string "$class" in it, then fail if that * string is not at the very end.) */ - protected def newTermName(cs: Array[Char], offset: Int, len: Int, cachedString: String): TermName = { + final def newTermName(cs: Array[Char], offset: Int, len: Int, cachedString: String): TermName = { def body = { val h = hashValue(cs, offset, len) & HASH_MASK var n = termHashtable(h) @@ -108,33 +109,35 @@ trait Names extends api.Names { if (synchronizeNames) nameLock.synchronized(body) else body } - protected def newTypeName(cs: Array[Char], offset: Int, len: Int, cachedString: String): TypeName = + final def newTypeName(cs: Array[Char], offset: Int, len: Int, cachedString: String): TypeName = newTermName(cs, offset, len, cachedString).toTypeName /** Create a term name from string. */ + @deprecatedOverriding("To synchronize, use `override def synchronizeNames = true`", "2.11.0") // overriden in https://github.com/scala-ide/scala-ide/blob/master/org.scala-ide.sdt.core/src/scala/tools/eclipse/ScalaPresentationCompiler.scala def newTermName(s: String): TermName = newTermName(s.toCharArray(), 0, s.length(), null) /** Create a type name from string. */ + @deprecatedOverriding("To synchronize, use `override def synchronizeNames = true`", "2.11.0") // overriden in https://github.com/scala-ide/scala-ide/blob/master/org.scala-ide.sdt.core/src/scala/tools/eclipse/ScalaPresentationCompiler.scala def newTypeName(s: String): TypeName = newTermName(s).toTypeName /** Create a term name from the UTF8 encoded bytes in bs[offset..offset+len-1]. */ - def newTermName(bs: Array[Byte], offset: Int, len: Int): TermName = { + final def newTermName(bs: Array[Byte], offset: Int, len: Int): TermName = { val chars = Codec.fromUTF8(bs, offset, len) newTermName(chars, 0, chars.length) } - def newTermNameCached(s: String): TermName = + final def newTermNameCached(s: String): TermName = newTermName(s.toCharArray(), 0, s.length(), cachedString = s) - def newTypeNameCached(s: String): TypeName = + final def newTypeNameCached(s: String): TypeName = newTypeName(s.toCharArray(), 0, s.length(), cachedString = s) /** Create a type name from the characters in cs[offset..offset+len-1]. */ - def newTypeName(cs: Array[Char], offset: Int, len: Int): TypeName = + final def newTypeName(cs: Array[Char], offset: Int, len: Int): TypeName = newTermName(cs, offset, len, cachedString = null).toTypeName /** Create a type name from the UTF8 encoded bytes in bs[offset..offset+len-1]. */ - def newTypeName(bs: Array[Byte], offset: Int, len: Int): TypeName = + final def newTypeName(bs: Array[Byte], offset: Int, len: Int): TypeName = newTermName(bs, offset, len).toTypeName /** -- cgit v1.2.3