Merge pull request #3116 from dotty-staging/classpath

Port ClassPath optimisations and bug fixes from scalac

Author: smarter

compiler/src/dotty/tools/dotc/classpath/AggregateClassPath.scala

@@ -19,32 +19,28 @@ import dotty.tools.io.{ AbstractFile, ClassPath, ClassRepresentation }
  */
 case class AggregateClassPath(aggregates: Seq[ClassPath]) extends ClassPath {
   override def findClassFile(className: String): Option[AbstractFile] = {
-    @tailrec
-    def find(aggregates: Seq[ClassPath]): Option[AbstractFile] =
-      if (aggregates.nonEmpty) {
-        val classFile = aggregates.head.findClassFile(className)
-        if (classFile.isDefined) classFile
-        else find(aggregates.tail)
-      } else None
-
-    find(aggregates)
+    val (pkg, _) = PackageNameUtils.separatePkgAndClassNames(className)
+    aggregatesForPackage(pkg).iterator.map(_.findClassFile(className)).collectFirst {
+      case Some(x) => x
+    }
+  }
+  private[this] val packageIndex: collection.mutable.Map[String, Seq[ClassPath]] = collection.mutable.Map()
+  private def aggregatesForPackage(pkg: String): Seq[ClassPath] = packageIndex.synchronized {
+    packageIndex.getOrElseUpdate(pkg, aggregates.filter(_.hasPackage(pkg)))
   }
 
   override def findClass(className: String): Option[ClassRepresentation] = {
-    @tailrec
-    def findEntry(aggregates: Seq[ClassPath], isSource: Boolean): Option[ClassRepresentation] =
-      if (aggregates.nonEmpty) {
-        val entry = aggregates.head.findClass(className) match {
-          case s @ Some(_: SourceFileEntry) if isSource => s
-          case s @ Some(_: ClassFileEntry) if !isSource => s
-          case _ => None
-        }
-        if (entry.isDefined) entry
-        else findEntry(aggregates.tail, isSource)
-      } else None
-
-    val classEntry = findEntry(aggregates, isSource = false)
-    val sourceEntry = findEntry(aggregates, isSource = true)
+    val (pkg, _) = PackageNameUtils.separatePkgAndClassNames(className)
+
+    def findEntry(isSource: Boolean): Option[ClassRepresentation] = {
+      aggregatesForPackage(pkg).iterator.map(_.findClass(className)).collectFirst {
+        case Some(s: SourceFileEntry) if isSource => s
+        case Some(s: ClassFileEntry) if !isSource => s
+      }
+    }
+
+    val classEntry = findEntry(isSource = false)
+    val sourceEntry = findEntry(isSource = true)
 
     (classEntry, sourceEntry) match {
       case (Some(c: ClassFileEntry), Some(s: SourceFileEntry)) => Some(ClassAndSourceFilesEntry(c.file, s.file))
@@ -70,6 +66,7 @@ case class AggregateClassPath(aggregates: Seq[ClassPath]) extends ClassPath {
   override private[dotty] def sources(inPackage: String): Seq[SourceFileEntry] =
     getDistinctEntries(_.sources(inPackage))
 
+  override private[dotty] def hasPackage(pkg: String) = aggregates.exists(_.hasPackage(pkg))
   override private[dotty] def list(inPackage: String): ClassPathEntries = {
     val (packages, classesAndSources) = aggregates.map { cp =>
       try {

compiler/src/dotty/tools/dotc/classpath/DirectoryClassPath.scala

@@ -45,6 +45,8 @@ trait DirectoryLookup[FileEntryType <: ClassRepresentation] extends ClassPath {
     }
   }
 
+  override private[dotty] def hasPackage(pkg: String) = getDirectory(pkg).isDefined
+
   private[dotty] def packages(inPackage: String): Seq[PackageEntry] = {
     val dirForPackage = getDirectory(inPackage)
     val nestedDirs: Array[F] = dirForPackage match {
@@ -161,6 +163,9 @@ final class JrtClassPath(fs: java.nio.file.FileSystem) extends ClassPath with No
     ps.map(p => (p.toString.stripPrefix("/packages/"), lookup(p))).toMap
   }
 
+  /** Empty string represents root package */
+  override private[dotty] def hasPackage(pkg: String) = packageToModuleBases.contains(pkg)
+
   override private[dotty] def packages(inPackage: String): Seq[PackageEntry] = {
     def matches(packageDottedName: String) =
       if (packageDottedName.contains("."))

compiler/src/dotty/tools/dotc/classpath/ZipAndJarFileLookupFactory.scala

@@ -5,38 +5,31 @@ package dotty.tools.dotc.classpath
 
 import java.io.File
 import java.net.URL
+import java.nio.file.Files
+import java.nio.file.attribute.{BasicFileAttributes, FileTime}
+
 import scala.annotation.tailrec
-import dotty.tools.io.{AbstractFile, ClassPath, FileZipArchive, ManifestResources}
-import dotty.tools.dotc.config.Settings
+import dotty.tools.io.{AbstractFile, ClassPath, ClassRepresentation, FileZipArchive, ManifestResources}
 import dotty.tools.dotc.core.Contexts.Context
 import FileUtils._
 
 /**
  * A trait providing an optional cache for classpath entries obtained from zip and jar files.
- * It's possible to create such a cache assuming that entries in such files won't change (at
- * least will be the same each time we'll load classpath during the lifetime of JVM process)
- * - unlike class and source files in directories, which can be modified and recompiled.
  * It allows us to e.g. reduce significantly memory used by PresentationCompilers in Scala IDE
  * when there are a lot of projects having a lot of common dependencies.
  */
 sealed trait ZipAndJarFileLookupFactory {
-  private val cache = collection.mutable.Map.empty[AbstractFile, ClassPath]
+  private val cache = new FileBasedCache[ClassPath]
 
   def create(zipFile: AbstractFile)(implicit ctx: Context): ClassPath = {
-    if (ctx.settings.YdisableFlatCpCaching.value) createForZipFile(zipFile)
+    if (ctx.settings.YdisableFlatCpCaching.value || zipFile.file == null) createForZipFile(zipFile)
     else createUsingCache(zipFile)
   }
 
   protected def createForZipFile(zipFile: AbstractFile): ClassPath
 
-  private def createUsingCache(zipFile: AbstractFile)(implicit ctx: Context): ClassPath = cache.synchronized {
-    def newClassPathInstance = {
-      if (ctx.settings.verbose.value || ctx.settings.Ylogcp.value)
-        println(s"$zipFile is not yet in the classpath cache")
-      createForZipFile(zipFile)
-    }
-    cache.getOrElseUpdate(zipFile, newClassPathInstance)
-  }
+  private def createUsingCache(zipFile: AbstractFile): ClassPath =
+    cache.getOrCreate(zipFile.file.toPath, () => createForZipFile(zipFile))
 }
 
 /**
@@ -50,7 +43,13 @@ object ZipAndJarClassPathFactory extends ZipAndJarFileLookupFactory {
 
     override def findClassFile(className: String): Option[AbstractFile] = {
       val (pkg, simpleClassName) = PackageNameUtils.separatePkgAndClassNames(className)
-      classes(pkg).find(_.name == simpleClassName).map(_.file)
+      file(pkg, simpleClassName + ".class").map(_.file)
+    }
+
+    // This method is performance sensitive as it is used by SBT's ExtractDependencies phase.
+    override def findClass(className: String): Option[ClassRepresentation] = {
+      val (pkg, simpleClassName) = PackageNameUtils.separatePkgAndClassNames(className)
+      file(pkg, simpleClassName + ".class")
     }
 
     override private[dotty] def classes(inPackage: String): Seq[ClassFileEntry] = files(inPackage)
@@ -133,6 +132,7 @@ object ZipAndJarClassPathFactory extends ZipAndJarFileLookupFactory {
         (for (file <- pkg if file.isClass) yield ClassFileEntryImpl(file))(collection.breakOut)
     }
 
+    override private[dotty] def hasPackage(pkg: String) = cachedPackages.contains(pkg)
     override private[dotty] def list(inPackage: String): ClassPathEntries = ClassPathEntries(packages(inPackage), classes(inPackage))
   }
 
@@ -173,3 +173,29 @@ object ZipAndJarSourcePathFactory extends ZipAndJarFileLookupFactory {
 
   override protected def createForZipFile(zipFile: AbstractFile): ClassPath = ZipArchiveSourcePath(zipFile.file)
 }
+
+final class FileBasedCache[T] {
+  private case class Stamp(lastModified: FileTime, fileKey: Object)
+  private val cache = collection.mutable.Map.empty[java.nio.file.Path, (Stamp, T)]
+
+  def getOrCreate(path: java.nio.file.Path, create: () => T): T = cache.synchronized {
+    val attrs = Files.readAttributes(path, classOf[BasicFileAttributes])
+    val lastModified = attrs.lastModifiedTime()
+    // only null on some platforms, but that's okay, we just use the last modified timestamp as our stamp
+    val fileKey = attrs.fileKey()
+    val stamp = Stamp(lastModified, fileKey)
+    cache.get(path) match {
+      case Some((cachedStamp, cached)) if cachedStamp == stamp => cached
+      case _ =>
+        val value = create()
+        cache.put(path, (stamp, value))
+        value
+    }
+  }
+
+  def clear(): Unit = cache.synchronized {
+    // TODO support closing
+    // cache.valuesIterator.foreach(_.close())
+    cache.clear()
+  }
+}

compiler/src/dotty/tools/dotc/classpath/ZipArchiveFileLookup.scala

@@ -39,6 +39,14 @@ trait ZipArchiveFileLookup[FileEntryType <: ClassRepresentation] extends ClassPa
       entry <- dirEntry.iterator if isRequiredFileType(entry)
     } yield createFileEntry(entry)
 
+  protected def file(inPackage: String, name: String): Option[FileEntryType] =
+    for {
+      dirEntry <- findDirEntry(inPackage)
+      entry <- Option(dirEntry.lookupName(name, directory = false))
+      if isRequiredFileType(entry)
+    } yield createFileEntry(entry)
+
+  override private[dotty] def hasPackage(pkg: String) = findDirEntry(pkg).isDefined
   override private[dotty] def list(inPackage: String): ClassPathEntries = {
     val foundDirEntry = findDirEntry(inPackage)
 

compiler/src/dotty/tools/io/ClassPath.scala

@@ -21,17 +21,42 @@ trait ClassPath {
   import dotty.tools.dotc.classpath._
   def asURLs: Seq[URL]
 
-  /** Empty string represents root package */
+  /*
+   * These methods are mostly used in the ClassPath implementation to implement the `list` and
+   * `findX` methods below.
+   *
+   * However, there are some other uses in the compiler, to implement `invalidateClassPathEntries`,
+   * which is used by the repl's `:require` (and maybe the spark repl, https://github.com/scala/scala/pull/4051).
+   * Using these methods directly is more efficient than calling `list`.
+   *
+   * The `inPackage` string is a full package name, e.g. "" or "scala.collection".
+   */
+
+  private[dotty] def hasPackage(pkg: String): Boolean
   private[dotty] def packages(inPackage: String): Seq[PackageEntry]
   private[dotty] def classes(inPackage: String): Seq[ClassFileEntry]
   private[dotty] def sources(inPackage: String): Seq[SourceFileEntry]
 
-  /** Allows to get entries for packages and classes merged with sources possibly in one pass. */
+  /**
+    * Returns packages and classes (source or classfile) that are members of `inPackage` (not
+    * recursively). The `inPackage` string is a full package name, e.g., "scala.collection".
+    *
+    * This is the main method uses to find classes, see class `PackageLoader`. The
+    * `rootMirror.rootLoader` is created with `inPackage = ""`.
+    */
   private[dotty] def list(inPackage: String): ClassPathEntries
 
   /**
-    * It returns both classes from class file and source files (as our base ClassRepresentation).
-    * So note that it's not so strictly related to findClassFile.
+    * Returns the class file and / or source file for a given external name, e.g., "java.lang.String".
+    * If there is both a class file and source file, the compiler can decide whether to read the
+    * class file or compile the source file.
+    *
+    * Internally this seems to be used only by `ScriptRunner`, but only to call `.isDefined`. That
+    * could probably be implemented differently.
+    *
+    * Externally, it is used by sbt's compiler interface:
+    * https://github.com/sbt/sbt/blob/v0.13.15/compile/interface/src/main/scala/xsbt/CompilerInterface.scala#L249
+    * Jason has some improvements for that in the works (https://github.com/scala/bug/issues/10289#issuecomment-310022699)
     */
   def findClass(className: String): Option[ClassRepresentation] = {
     // A default implementation which should be overridden, if we can create the more efficient
@@ -43,6 +68,16 @@ trait ClassPath {
 
     foundClassFromClassFiles orElse findClassInSources
   }
+
+ /**
+   * Returns the classfile for an external name, e.g., "java.lang.String". This method does not
+   * return source files.
+   *
+   * This method is used by the classfile parser. When parsing a Java class, its own inner classes
+   * are entered with a `ClassfileLoader` that parses the classfile returned by this method.
+   * It is also used in the backend, by the inliner, to obtain the bytecode when inlining from the
+   * classpath. It's also used by scalap.
+   */
   def findClassFile(className: String): Option[AbstractFile]
 
   def asClassPathStrings: Seq[String]

compiler/src/dotty/tools/io/ZipArchive.scala

@@ -24,7 +24,7 @@ import scala.annotation.tailrec
  *  ''Note:  This library is considered experimental and should not be used unless you know what you are doing.''
  */
 object ZipArchive {
-  private[io] val closeZipFile = sys.props.get("scala.classpath.closeZip").map(_.toBoolean).getOrElse(false)
+  private[io] val closeZipFile = sys.props.get("scala.classpath.closeZip").exists(_.toBoolean)
 
   /**
    * @param   file  a File
@@ -82,8 +82,8 @@ abstract class ZipArchive(override val file: JFile) extends AbstractFile with Eq
     override def isDirectory = true
     override def iterator: Iterator[Entry] = entries.valuesIterator
     override def lookupName(name: String, directory: Boolean): Entry = {
-      if (directory) entries(name + "/")
-      else entries(name)
+      if (directory) entries.get(name + "/").orNull
+      else entries.get(name).orNull
     }
   }
 

compiler/test/dotty/tools/dotc/classpath/ZipAndJarFileLookupFactoryTest.scala

@@ -0,0 +1,70 @@
+package dotty.tools.dotc.classpath
+
+import org.junit.Test
+import java.nio.file._
+import java.nio.file.attribute.FileTime
+
+import dotty.tools.dotc.core.Contexts.{Context, ContextBase}
+import dotty.tools.io.AbstractFile
+
+
+
+class ZipAndJarFileLookupFactoryTest {
+  @Test def cacheInvalidation(): Unit = {
+    if (scala.util.Properties.isWin) return // can't overwrite an open file on windows.
+
+    val f = Files.createTempFile("test-", ".jar")
+    Files.delete(f)
+
+    implicit val ctx: Context = new ContextBase().initialCtx
+    assert(!ctx.settings.YdisableFlatCpCaching.value) // we're testing with our JAR metadata caching enabled.
+
+    def createCp = ZipAndJarClassPathFactory.create(AbstractFile.getFile(f.toFile))
+    try {
+      createZip(f, Array(), "p1/C.class")
+      createZip(f, Array(), "p2/X.class")
+      createZip(f, Array(), "p3/Y.class")
+      val cp1 = createCp
+      assert(cp1.findClass("p1.C").isDefined)
+
+      // We expect get a cache hit as the underlying zip hasn't changed
+      val cp2 = createCp
+      assert(cp2 eq cp1)
+
+      // check things work after the cache hit
+      cp1.findClassFile("p2.X").get.toByteArray
+
+      val lastMod1 = Files.getLastModifiedTime(f)
+      // Create a new zip at the same path with different contents and last modified
+      Files.delete(f)
+      createZip(f, Array(), "p1/D.class")
+      Files.setLastModifiedTime(f, FileTime.fromMillis(lastMod1.toMillis + 2000))
+
+      // Our classpath cache should create a new instance
+      val cp3 = createCp
+      assert(cp1 ne cp3, (System.identityHashCode(cp1), System.identityHashCode(cp3)))
+      // And that instance should see D, not C, in package p1.
+      assert(cp3.findClass("p1.C").isEmpty)
+      assert(cp3.findClass("p1.D").isDefined)
+    } finally Files.delete(f)
+  }
+
+  def createZip(zipLocation: Path, content: Array[Byte], internalPath: String): Unit = {
+    val env = new java.util.HashMap[String, String]()
+    env.put("create", String.valueOf(Files.notExists(zipLocation)))
+    val fileUri = zipLocation.toUri
+    val zipUri = new java.net.URI("jar:" + fileUri.getScheme, fileUri.getPath, null)
+    val zipfs = FileSystems.newFileSystem(zipUri, env)
+    try {
+      try {
+        val internalTargetPath = zipfs.getPath(internalPath)
+        Files.createDirectories(internalTargetPath.getParent)
+        Files.write(internalTargetPath, content)
+      } finally {
+        if (zipfs != null) zipfs.close()
+      }
+    } finally {
+      zipfs.close()
+    }
+  }
+}

sbt-dotty/sbt-test/source-dependencies/binary/project/P.scala

@@ -5,10 +5,6 @@ object B extends Build
 {
 	lazy val dep = Project("dep", file("dep"))
 	lazy val use = Project("use", file("use")) settings(
-		unmanagedJars in Compile <+= packageBin in (dep, Compile) map Attributed.blank,
-
-    // Disable classpath caching since it does not invalidate modified jars
-    // (https://github.com/scala/bug/issues/10295)
-    scalacOptions += "-YdisableFlatCpCaching"
+		unmanagedJars in Compile <+= packageBin in (dep, Compile) map Attributed.blank
 	)
 }