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.

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