Improve implementation of classloader caching

  - Fix logic error in reference counted closing
  - Add a new policy to cache regardless of timestamps and
    regardless of directory elements in the classpath.

Author: retronym

src/compiler/scala/tools/nsc/classpath/ZipAndJarFileLookupFactory.scala

@@ -24,6 +24,7 @@ import scala.reflect.io.{AbstractFile, FileZipArchive, ManifestResources}
 import scala.tools.nsc.util.{ClassPath, ClassRepresentation}
 import scala.tools.nsc.{CloseableRegistry, Settings}
 import FileUtils._
+import scala.tools.nsc.io.Jar
 
 /**
  * A trait providing an optional cache for classpath entries obtained from zip and jar files.
@@ -39,7 +40,7 @@ sealed trait ZipAndJarFileLookupFactory {
       closeableRegistry.registerClosable(result)
       result
     } else {
-      cache.getOrCreate(List(zipFile.file.toPath), () => createForZipFile(zipFile, settings.releaseValue), closeableRegistry)
+      cache.getOrCreate(List(zipFile.file.toPath), () => createForZipFile(zipFile, settings.releaseValue), closeableRegistry, checkStamps = true)
     }
   }
 
@@ -195,25 +196,55 @@ final class FileBasedCache[T] {
   private case class Stamp(lastModified: FileTime, fileKey: Object)
   private case class Entry(stamps: Seq[Stamp], t: T) {
     val referenceCount: AtomicInteger = new AtomicInteger(1)
-    def referenceCountDecrementer: Closeable = new Closeable {
-      var closed = false
-      override def close(): Unit = {
-        if (!closed) {
-          closed = true
-          val count = referenceCount.decrementAndGet()
-          if (count == 0) {
-            t match {
-              case cl: Closeable => FileBasedCache.deferredClose(referenceCount, cl)
-              case _ =>
-            }
+  }
+  private val cache = collection.mutable.Map.empty[Seq[Path], Entry]
+
+  private def referenceCountDecrementer(e: Entry, paths: Seq[Path]): Closeable = new Closeable {
+    var closed = false
+    override def close(): Unit = {
+      if (!closed) {
+        closed = true
+        val count = e.referenceCount.decrementAndGet()
+        if (count == 0) {
+          e.t match {
+            case cl: Closeable =>
+              FileBasedCache.timer match {
+                case Some(timer) =>
+                  val task = new TimerTask {
+                    override def run(): Unit = {
+                      cache.synchronized {
+                        if (e.referenceCount.compareAndSet(0, -1)) {
+                          cache.remove(paths)
+                          cl.close()
+                        }
+                      }
+                    }
+                  }
+                  timer.schedule(task, FileBasedCache.deferCloseMs.toLong)
+                case None =>
+                  cl.close()
+              }
+            case _ =>
           }
         }
       }
     }
   }
-  private val cache = collection.mutable.Map.empty[Seq[Path], Entry]
 
-  def getOrCreate(paths: Seq[Path], create: () => T, closeableRegistry: CloseableRegistry): T = cache.synchronized {
+  def checkCacheability(urls: Seq[URL], checkStamps: Boolean, disableCache: Boolean): Either[String, Seq[java.nio.file.Path]] = {
+    import scala.reflect.io.{AbstractFile, Path}
+    val urlsAndFiles = urls.filterNot(_.getProtocol == "jrt").map(u => u -> AbstractFile.getURL(u))
+    val nonJarZips = urlsAndFiles.filter(_._2 == null).filter(p => Jar.isJarOrZip(p._2.file))
+    def paths = urlsAndFiles.map(t => Path(t._2.file).jfile.toPath)
+    if (!checkStamps) Right(paths)
+    else if (disableCache) Left("caching is disabled due to a policy setting")
+    else {
+      if (nonJarZips.nonEmpty) Left(s"caching is disabled because of the following classpath elements: ${nonJarZips.map(_._1).mkString(", ")}.")
+      else Right(paths)
+    }
+  }
+
+  def getOrCreate(paths: Seq[Path], create: () => T, closeableRegistry: CloseableRegistry, checkStamps: Boolean): T = cache.synchronized {
     val stamps = paths.map { path =>
       val attrs = Files.readAttributes(path, classOf[BasicFileAttributes])
       val lastModified = attrs.lastModifiedTime()
@@ -223,15 +254,35 @@ final class FileBasedCache[T] {
     }
 
     cache.get(paths) match {
-      case Some(e@Entry(cachedStamps, cached)) if cachedStamps == stamps =>
-        e.referenceCount.incrementAndGet()
-        closeableRegistry.registerClosable(e.referenceCountDecrementer)
-        cached
+      case Some(e@Entry(cachedStamps, cached)) =>
+        if (!checkStamps || cachedStamps == stamps) {
+          // Cache hit
+          val count = e.referenceCount.incrementAndGet()
+          assert(count > 0, (stamps, count))
+          closeableRegistry.registerClosable(referenceCountDecrementer(e, paths))
+          cached
+        } else {
+          // Cache miss: we found an entry but the underlying files have been modified
+          cached match {
+            case c: Closeable =>
+              if (e.referenceCount.get() == 0) {
+                c.close()
+              } else {
+                // TODO: What do do here? Maybe add to a list of closeables polled by a cleanup thread?
+              }
+          }
+          val value = create()
+          val entry = Entry(stamps, value)
+          cache.put(paths, entry)
+          closeableRegistry.registerClosable(referenceCountDecrementer(entry, paths))
+          value
+        }
       case _ =>
+        // Cache miss
         val value = create()
         val entry = Entry(stamps, value)
         cache.put(paths, entry)
-        closeableRegistry.registerClosable(entry.referenceCountDecrementer)
+        closeableRegistry.registerClosable(referenceCountDecrementer(entry, paths))
         value
     }
   }
@@ -244,29 +295,10 @@ final class FileBasedCache[T] {
 }
 
 object FileBasedCache {
-  // The tension here is that too long a delay could lead to an error (on Windows) with an inability
-  // to overwrite the JAR. To short a delay and the entry could be evicted before a subsequent
-  // sub-project compilation is able to get a cache hit. A more comprehensive solution would be to
-  // involve build tools in the policy: they could close entries with refcount of zero when that
-  // entry's JAR is about to be overwritten.
   private val deferCloseMs = Integer.getInteger("scalac.filebasedcache.defer.close.ms", 1000)
   private val timer: Option[Timer] = {
     if (deferCloseMs > 0)
       Some(new java.util.Timer(true))
     else None
   }
-  private def deferredClose(referenceCount: AtomicInteger, closable: Closeable): Unit = {
-    timer match {
-      case Some(timer) =>
-        val task = new TimerTask {
-          override def run(): Unit = {
-            if (referenceCount.get == 0)
-              closable.close()
-          }
-        }
-        timer.schedule(task, FileBasedCache.deferCloseMs.toLong)
-      case None =>
-        closable.close()
-    }
-  }
 }

src/compiler/scala/tools/nsc/plugins/Plugins.scala

@@ -69,7 +69,8 @@ trait Plugins { global: Global =>
     * @return
     */
   protected def findPluginClassLoader(classpath: Seq[Path]): ClassLoader = {
-    val disableCache = settings.YcachePluginClassLoader.value == settings.CachePolicy.None.name
+    val policy = settings.YcachePluginClassLoader.value
+    val disableCache = policy == settings.CachePolicy.None.name
     def newLoader = () => {
       val compilerLoader = classOf[Plugin].getClassLoader
       val urls = classpath map (_.toURL)
@@ -84,11 +85,16 @@ trait Plugins { global: Global =>
     // mitigate the cost of dynamic classloading as it has been
     // measured in https://github.com/scala/scala-dev/issues/458.
 
-    if (disableCache || classpath.exists(!Jar.isJarOrZip(_))) {
-      val loader = newLoader()
-      closeableRegistry.registerClosable(loader)
-      loader
-    } else pluginClassLoadersCache.getOrCreate(classpath.map(_.jfile.toPath()), newLoader, closeableRegistry)
+    val cache = pluginClassLoadersCache
+    val checkStamps = policy == settings.CachePolicy.LastModified
+    cache.checkCacheability(classpath.map(_.toURL), checkStamps, disableCache) match {
+      case Left(msg) =>
+        val loader = newLoader()
+        closeableRegistry.registerClosable(loader)
+        loader
+      case Right(paths) =>
+        cache.getOrCreate(classpath.map(_.jfile.toPath()), newLoader, closeableRegistry, checkStamps)
+    }
   }
 
   protected lazy val roughPluginsList: List[Plugin] = loadRoughPluginsList()
@@ -181,29 +187,18 @@ trait Plugins { global: Global =>
       ScalaClassLoader.fromURLs(classpath, getClass.getClassLoader)
     }
 
-    val disableCache = settings.YcacheMacroClassLoader.value == settings.CachePolicy.None.name
-    if (disableCache) newLoader()
-    else {
-      import scala.tools.nsc.io.Jar
-      import scala.reflect.io.{AbstractFile, Path}
-
-      val urlsAndFiles = classpath.map(u => u -> AbstractFile.getURL(u))
-      val hasNullURL = urlsAndFiles.filter(_._2 eq null)
-      if (hasNullURL.nonEmpty) {
-        // TODO if the only null is jrt:// we can still cache
-        // TODO filter out classpath elements pointing to non-existing files before we get here, that's another source of null
-        analyzer.macroLogVerbose(s"macro classloader: caching is disabled because `AbstractFile.getURL` returned `null` for ${hasNullURL.map(_._1).mkString(", ")}.")
-        perRunCaches.recordClassloader(newLoader())
-      } else {
-        val locations = urlsAndFiles.map(t => Path(t._2.file))
-        val nonJarZips = locations.filterNot(Jar.isJarOrZip(_))
-        if (nonJarZips.nonEmpty) {
-          analyzer.macroLogVerbose(s"macro classloader: caching is disabled because the following paths are not supported: ${nonJarZips.mkString(",")}.")
-          perRunCaches.recordClassloader(newLoader())
-        } else {
-          Macros.macroClassLoadersCache.getOrCreate(locations.map(_.jfile.toPath()), newLoader, closeableRegistry)
-        }
-      }
+    val policy = settings.YcacheMacroClassLoader.value
+    val cache = Macros.macroClassLoadersCache
+    val disableCache = policy == settings.CachePolicy.None.name
+    val checkStamps = policy == settings.CachePolicy.LastModified.name
+    cache.checkCacheability(classpath, checkStamps, disableCache) match {
+      case Left(msg) =>
+        analyzer.macroLogVerbose(s"macro classloader: $msg.")
+        val loader = newLoader()
+        closeableRegistry.registerClosable(loader)
+        loader
+      case Right(paths) =>
+        cache.getOrCreate(paths, newLoader, closeableRegistry, checkStamps)
     }
   }
 }

src/compiler/scala/tools/nsc/settings/ScalaSettings.scala

@@ -277,9 +277,10 @@ trait ScalaSettings extends AbsScalaSettings
     def setting(style: String, styleLong: String) = ChoiceSetting(s"-Ycache-$style-class-loader", "policy", s"Policy for caching class loaders for $styleLong that are dynamically loaded.", values.map(_.name), None.name, values.map(_.help))
     object None extends CachePolicy("none", "Don't cache class loader")
     object LastModified extends CachePolicy("last-modified", "Cache class loader, using file last-modified time to invalidate")
+    object Always extends CachePolicy("always", "Cache class loader with no invalidation")
     // TODO Jorge to add new policy. Think about whether there is a benefit to the user on offering this as a separate policy or unifying with the previous one.
     // object ZipMetadata extends CachePolicy("zip-metadata", "Cache classloade, using file last-modified time, then ZIP file metadata to invalidate")
-    def values: List[CachePolicy] = List(None, LastModified)
+    def values: List[CachePolicy] = List(None, LastModified, Always)
   }
 
   object optChoices extends MultiChoiceEnumeration {