From 329d989e73f81de337383fcd8585189cd8fb5fc7 Mon Sep 17 00:00:00 2001 From: Ethan Atkins Date: Thu, 30 Apr 2020 18:22:15 -0700 Subject: [PATCH 1/2] Use sbt source file stamps in zinc This fixes the nio/external-hooks test and also restores the performance of the benchmarks for the latest sbt version in https://github.com/eatkins/scala-build-watch-performance which had regressed when the custom ExternalHooks were disabled in 7c4b01d9f722252d3e7379f69591861bdd57152c. The main change is that it changes the ReadStamps object that is passed into the compiler options to one that uses the unmanagedFileStampCache and managedFileStampCache for source files and falls back to the default stamper otherwise. This improves the performance quite significantly since we only hash the files once. It also makes it so that the analysis file will contain the source file stamps of the files when compilation began, rather than when compilation ended. That is what nio/external-hooks was testing. In the real world what could happen was that one modified a source file during compilation but then no incremental re-compilation would occur because after the initial compilation completed, zinc wrote the stamp of the modified source file in the analysis file even though it may have actually compiled a different version of the source file. --- main/src/main/scala/sbt/Defaults.scala | 37 ++++++++++++++++++- main/src/main/scala/sbt/Keys.scala | 4 +- .../nio/external-hooks/{pending => test} | 0 3 files changed, 37 insertions(+), 4 deletions(-) rename sbt/src/sbt-test/nio/external-hooks/{pending => test} (100%) diff --git a/main/src/main/scala/sbt/Defaults.scala b/main/src/main/scala/sbt/Defaults.scala index 19dacc16f..d3b77ae5e 100644 --- a/main/src/main/scala/sbt/Defaults.scala +++ b/main/src/main/scala/sbt/Defaults.scala @@ -214,8 +214,41 @@ object Defaults extends BuildCommon { VirtualTerminal.handler, ) ++ serverHandlers.value :+ ServerHandler.fallback }, - uncachedStamper := Stamps.uncachedStamps(fileConverter.value), - reusableStamper := Stamps.timeWrapLibraryStamps(uncachedStamper.value, fileConverter.value), + timeWrappedStamper := Stamps + .timeWrapLibraryStamps(Stamps.uncachedStamps(fileConverter.value), fileConverter.value), + reusableStamper := { + val converter = fileConverter.value + val unmanagedCache = unmanagedFileStampCache.value + val managedCache = managedFileStampCache.value + val backing = timeWrappedStamper.value + new xsbti.compile.analysis.ReadStamps { + def getAllLibraryStamps() + : java.util.Map[xsbti.VirtualFileRef, xsbti.compile.analysis.Stamp] = + backing.getAllLibraryStamps() + def getAllProductStamps() + : java.util.Map[xsbti.VirtualFileRef, xsbti.compile.analysis.Stamp] = + backing.getAllProductStamps() + def getAllSourceStamps() + : java.util.Map[xsbti.VirtualFileRef, xsbti.compile.analysis.Stamp] = + new java.util.HashMap[xsbti.VirtualFileRef, xsbti.compile.analysis.Stamp] + def library(fr: xsbti.VirtualFileRef): xsbti.compile.analysis.Stamp = { + val path = converter.toPath(fr) + managedCache + .getOrElseUpdate(path, sbt.nio.FileStamper.Hash) + .map(_.stamp) + .getOrElse(backing.library(fr)) + } + def product(fr: xsbti.VirtualFileRef): xsbti.compile.analysis.Stamp = backing.product(fr) + def source(fr: xsbti.VirtualFile): xsbti.compile.analysis.Stamp = { + val path = converter.toPath(fr) + unmanagedCache + .get(path) + .orElse(managedCache.getOrElseUpdate(path, sbt.nio.FileStamper.Hash)) + .map(_.stamp) + .getOrElse(backing.source(fr)) + } + } + }, traceLevel in run :== 0, traceLevel in runMain :== 0, traceLevel in bgRun :== 0, diff --git a/main/src/main/scala/sbt/Keys.scala b/main/src/main/scala/sbt/Keys.scala index 51f4ad4eb..12a28d56c 100644 --- a/main/src/main/scala/sbt/Keys.scala +++ b/main/src/main/scala/sbt/Keys.scala @@ -227,8 +227,8 @@ object Keys { val fileConverter = settingKey[FileConverter]("The file converter used to convert between Path and VirtualFile") val allowMachinePath = settingKey[Boolean]("Allow machine-specific paths during conversion.") val rootPaths = settingKey[Map[String, NioPath]]("The root paths used to abstract machine-specific paths.") - private[sbt] val uncachedStamper = settingKey[ReadStamps]("The stamper to create timestamp or hash.") - private[sbt] val reusableStamper = settingKey[ReadStamps]("The stamper can be reused across subprojects and sessions.") + private[sbt] val timeWrappedStamper = settingKey[ReadStamps]("The stamper to create timestamp or hash.") + private[sbt] val reusableStamper = taskKey[ReadStamps]("The stamper can be reused across subprojects and sessions.") // package keys val packageBin = taskKey[File]("Produces a main artifact, such as a binary jar.").withRank(ATask) diff --git a/sbt/src/sbt-test/nio/external-hooks/pending b/sbt/src/sbt-test/nio/external-hooks/test similarity index 100% rename from sbt/src/sbt-test/nio/external-hooks/pending rename to sbt/src/sbt-test/nio/external-hooks/test From e81d459f69a65ccf5acaece2dcaf94e015d66ec5 Mon Sep 17 00:00:00 2001 From: Ethan Atkins Date: Thu, 30 Apr 2020 20:32:40 -0700 Subject: [PATCH 2/2] Remove custom ExternalHooks This is now handled by passing a custom instance of ReadStamps to CompileOptions instead. --- build.sbt | 4 + main/src/main/scala/sbt/Defaults.scala | 21 --- .../scala/sbt/internal/ExternalHooks.scala | 170 ------------------ main/src/main/scala/sbt/nio/Keys.scala | 6 - 4 files changed, 4 insertions(+), 197 deletions(-) delete mode 100644 main/src/main/scala/sbt/internal/ExternalHooks.scala diff --git a/build.sbt b/build.sbt index 1f9877f24..596dab849 100644 --- a/build.sbt +++ b/build.sbt @@ -903,6 +903,8 @@ lazy val mainProj = (project in file("main")) exclude[DirectMissingMethodProblem]("sbt.Plugins.topologicalSort"), exclude[IncompatibleMethTypeProblem]("sbt.Defaults.allTestGroupsTask"), exclude[DirectMissingMethodProblem]("sbt.StandardMain.shutdownHook"), + exclude[DirectMissingMethodProblem]("sbt.nio.Keys.compileBinaryFileInputs"), + exclude[DirectMissingMethodProblem]("sbt.nio.Keys.compileSourceFileInputs"), exclude[MissingClassProblem]("sbt.internal.ResourceLoaderImpl"), exclude[IncompatibleSignatureProblem]("sbt.internal.ConfigIndex.*"), exclude[IncompatibleSignatureProblem]("sbt.internal.Inspect.*"), @@ -927,6 +929,8 @@ lazy val mainProj = (project in file("main")) exclude[MissingClassProblem]("sbt.internal.FileManagement$"), exclude[MissingClassProblem]("sbt.internal.FileManagement$CopiedFileTreeRepository"), exclude[MissingClassProblem]("sbt.internal.server.LanguageServerReporter*"), + exclude[MissingClassProblem]("sbt.internal.ExternalHooks"), + exclude[MissingClassProblem]("sbt.internal.ExternalHooks$"), // false positives exclude[DirectMissingMethodProblem]("sbt.plugins.IvyPlugin.requires"), exclude[DirectMissingMethodProblem]("sbt.plugins.JUnitXmlReportPlugin.requires"), diff --git a/main/src/main/scala/sbt/Defaults.scala b/main/src/main/scala/sbt/Defaults.scala index d3b77ae5e..ed391b545 100644 --- a/main/src/main/scala/sbt/Defaults.scala +++ b/main/src/main/scala/sbt/Defaults.scala @@ -701,27 +701,6 @@ object Defaults extends BuildCommon { compileAnalysisFile := { compileAnalysisTargetRoot.value / compileAnalysisFilename.value }, - /* - // Comment this out because Zinc now uses farm hash to invalidate the virtual paths. - // To use watch to detect initial changes, we need to revalidate using content hash. - externalHooks := { - import sbt.nio.FileChanges - import sjsonnew.BasicJsonProtocol.mapFormat - val currentInputs = - (unmanagedSources / inputFileStamps).value ++ (managedSourcePaths / outputFileStamps).value - val sv = scalaVersion.value - val previousInputs = compileSourceFileInputs.previous.flatMap(_.get(sv)) - val inputChanges = previousInputs - .map(sbt.nio.Settings.changedFiles(_, currentInputs)) - .getOrElse(FileChanges.noPrevious(currentInputs.map(_._1))) - val currentOutputs = (dependencyClasspathFiles / outputFileStamps).value - val previousOutputs = compileBinaryFileInputs.previous.flatMap(_.get(sv)) - val outputChanges = previousOutputs - .map(sbt.nio.Settings.changedFiles(_, currentOutputs)) - .getOrElse(FileChanges.noPrevious(currentOutputs.map(_._1))) - ExternalHooks.default.value(inputChanges, outputChanges, fileTreeView.value) - }, - */ externalHooks := IncOptions.defaultExternal, incOptions := { val old = incOptions.value diff --git a/main/src/main/scala/sbt/internal/ExternalHooks.scala b/main/src/main/scala/sbt/internal/ExternalHooks.scala deleted file mode 100644 index fc7253981..000000000 --- a/main/src/main/scala/sbt/internal/ExternalHooks.scala +++ /dev/null @@ -1,170 +0,0 @@ -/* - * sbt - * Copyright 2011 - 2018, Lightbend, Inc. - * Copyright 2008 - 2010, Mark Harrah - * Licensed under Apache License 2.0 (see LICENSE) - */ - -/* -package sbt.internal - -import java.nio.file.{ Path, Paths } -import java.util.Optional - -import sbt.Def -import sbt.Keys._ -import sbt.internal.inc.ExternalLookup -import sbt.internal.inc.Stamp.equivStamp.equiv -import sbt.nio.Keys._ -import sbt.nio.file.syntax._ -import sbt.nio.file.{ FileAttributes, FileTreeView, RecursiveGlob } -import sbt.nio.{ FileChanges, FileStamp, FileStamper } -import sbt.util.InterfaceUtil.jo2o -import xsbti.{ VirtualFile, VirtualFileRef } -import xsbti.api.AnalyzedClass -import xsbti.compile._ -import xsbti.compile.analysis.Stamp - -private[sbt] object ExternalHooks { - private type Func = - (FileChanges, FileChanges, FileTreeView[(Path, FileAttributes)]) => ExternalHooks - def default: Def.Initialize[sbt.Task[Func]] = Def.task { - val unmanagedCache = unmanagedFileStampCache.value - val managedCache = managedFileStampCache.value - val cp = dependencyClasspath.value.map(_.data) - cp.foreach { file => - val path = file.toPath - managedCache.getOrElseUpdate(path, FileStamper.Hash) - } - val classGlob = classDirectory.value.toGlob / RecursiveGlob / "*.class" - val options = (compileOptions in compile).value - ((inputFileChanges, outputFileChanges, fileTreeView) => { - fileTreeView.list(classGlob).foreach { - case (path, _) => - s"updating $path" - managedCache.update(path, FileStamper.Hash) - } - apply(inputFileChanges, outputFileChanges, options, unmanagedCache, managedCache) - }): Func - } - private def apply( - inputFileChanges: FileChanges, - outputFileChanges: FileChanges, - options: CompileOptions, - unmanagedCache: FileStamp.Cache, - managedCache: FileStamp.Cache - ): DefaultExternalHooks = { - val converter = jo2o(options.converter) getOrElse { - sys.error("file converter was expected") - } - val lookup = new ExternalLookup { - override def changedSources( - previousAnalysis: CompileAnalysis - ): Option[Changes[VirtualFileRef]] = Some { - new Changes[VirtualFileRef] { - override val getAdded: java.util.Set[VirtualFileRef] = - new java.util.HashSet[VirtualFileRef] - override val getRemoved: java.util.Set[VirtualFileRef] = - new java.util.HashSet[VirtualFileRef] - override val getChanged: java.util.Set[VirtualFileRef] = - new java.util.HashSet[VirtualFileRef] - override val getUnmodified: java.util.Set[VirtualFileRef] = - new java.util.HashSet[VirtualFileRef] - - override def toString: String = - s"""Changes(added = $getAdded, removed = $getRemoved, changed = $getChanged, unmodified = ...)""" - - private def add(p: VirtualFileRef, sets: java.util.Set[VirtualFileRef]*): Unit = { - sets.foreach(add(p, _)) - } - private def add(f: VirtualFileRef, set: java.util.Set[VirtualFileRef]): Unit = { - set.add(f); () - } - val allChanges = new java.util.HashSet[VirtualFileRef] - inputFileChanges match { - case FileChanges(c, d, m, _) => - c.map(converter.toVirtualFile).foreach(add(_, getAdded, allChanges)) - d.map(converter.toVirtualFile).foreach(add(_, getRemoved, allChanges)) - m.map(converter.toVirtualFile).foreach(add(_, getChanged, allChanges)) - case _ => - } - override def isEmpty: java.lang.Boolean = - getAdded.isEmpty && getRemoved.isEmpty && getChanged.isEmpty - - private val prevSources = previousAnalysis.readStamps().getAllSourceStamps - prevSources.forEach { (file: VirtualFileRef, s: Stamp) => - if (!allChanges.contains(file)) { - val path: Path = converter.toPath(file) - unmanagedCache - .get(path) - .orElse(managedCache.getOrElseUpdate(path, FileStamper.Hash)) match { - case None => add(file, getRemoved) - case Some(stamp) => - if (equiv(stamp.stamp, s)) add(file, getUnmodified) - else add(file, getChanged) - } - } - } - options.sources.foreach(file => if (!prevSources.containsKey(file)) getAdded.add(file)) - } - } - - override def shouldDoIncrementalCompilation( - set: Set[String], - compileAnalysis: CompileAnalysis - ): Boolean = true - - // This could use the cache as well, but it would complicate the cache implementation. - override def hashClasspath(files: Array[VirtualFile]): Optional[Array[FileHash]] = - Optional.empty[Array[FileHash]] - - import scala.collection.JavaConverters._ - private val javaHome = Option(System.getProperty("java.home")).map(Paths.get(_)) - override def changedBinaries( - previousAnalysis: CompileAnalysis - ): Option[Set[VirtualFileRef]] = { - val base: Set[VirtualFileRef] = - (outputFileChanges.modified ++ outputFileChanges.created ++ outputFileChanges.deleted) - .map(converter.toVirtualFile(_)) - .toSet - Some(base ++ previousAnalysis.readStamps.getAllLibraryStamps.asScala.flatMap { - case (file, stamp) => - val path = converter.toPath(file) - val stampOpt = managedCache.getOrElseUpdate(path, FileStamper.Hash) - stampOpt match { - case Some(s) if equiv(s.stamp, stamp) => None - case _ => - javaHome match { - case Some(h) if path.startsWith(h) => None - case _ if file.name == "rt.jar" => None - case _ => - // stampOpt map { s => println(s"stamp changed for $file from ${s.stamp} to $stamp") } - Some(file) - } - } - }) - } - - override def removedProducts( - previousAnalysis: CompileAnalysis - ): Option[Set[VirtualFileRef]] = { - None - Some(previousAnalysis.readStamps.getAllProductStamps.asScala.flatMap { - case (file, stamp) => - val path = converter.toPath(file) - managedCache.get(path) match { - case Some(s) if equiv(s.stamp, stamp) => None - case Some(s) => Some(file) - case _ => - // This shouldn't be necessary - if (java.nio.file.Files.exists(path)) None - else Some(file) - } - }.toSet) - } - override def lookupAnalyzedClass(binaryClassName: String): Option[AnalyzedClass] = None - } - new DefaultExternalHooks(Optional.of(lookup), Optional.empty[ClassFileManager]) - } -} - */ diff --git a/main/src/main/scala/sbt/nio/Keys.scala b/main/src/main/scala/sbt/nio/Keys.scala index d00257d58..373f23cda 100644 --- a/main/src/main/scala/sbt/nio/Keys.scala +++ b/main/src/main/scala/sbt/nio/Keys.scala @@ -181,12 +181,6 @@ object Keys { private[sbt] val dependencyClasspathFiles = taskKey[Seq[Path]]("The dependency classpath for a task.").withRank(Invisible) private[sbt] val compileOutputs = taskKey[Seq[Path]]("Compilation outputs").withRank(Invisible) - private[sbt] val compileSourceFileInputs = - taskKey[Map[String, Seq[(Path, FileStamp)]]]("Source file stamps stored by scala version") - .withRank(Invisible) - private[sbt] val compileBinaryFileInputs = - taskKey[Map[String, Seq[(Path, FileStamp)]]]("Source file stamps stored by scala version") - .withRank(Invisible) private[this] val hasCheckedMetaBuildMsg = "Indicates whether or not we have called the checkBuildSources task. This is to avoid warning " +