From 90782b1cd045c84bb82c7d5ba655540c3870cbd7 Mon Sep 17 00:00:00 2001 From: jvican Date: Wed, 19 Apr 2017 08:32:45 +0200 Subject: [PATCH 1/4] Fix implementation of `LoadBuildConfiguration` It mainly does three things: * Clean up the implementation, removing unused values like `globalPluginDefs`. * Make the implementation more readable. * And the big one: Remove the creation of a classloader that we were instantiating but whose value we were throwing away. This has an impact in performance, but I'm yet to benchmark how much it is. --- main/src/main/scala/sbt/EvaluateTask.scala | 5 ++++ main/src/main/scala/sbt/internal/Load.scala | 32 ++++++++++++--------- 2 files changed, 23 insertions(+), 14 deletions(-) diff --git a/main/src/main/scala/sbt/EvaluateTask.scala b/main/src/main/scala/sbt/EvaluateTask.scala index bed9ee8ee..3b3fc8524 100644 --- a/main/src/main/scala/sbt/EvaluateTask.scala +++ b/main/src/main/scala/sbt/EvaluateTask.scala @@ -138,6 +138,11 @@ final case class PluginData( val classpath: Seq[Attributed[File]] = definitionClasspath ++ dependencyClasspath } +object PluginData { + private[sbt] def apply(dependencyClasspath: Def.Classpath): PluginData = + PluginData(dependencyClasspath, Nil, None, None, Nil) +} + object EvaluateTask { import std.Transform import Keys.state diff --git a/main/src/main/scala/sbt/internal/Load.scala b/main/src/main/scala/sbt/internal/Load.scala index fdeae3667..453b37618 100755 --- a/main/src/main/scala/sbt/internal/Load.scala +++ b/main/src/main/scala/sbt/internal/Load.scala @@ -34,7 +34,7 @@ import Keys.{ thisProjectRef, update } -import tools.nsc.reporters.ConsoleReporter +import scala.tools.nsc.reporters.ConsoleReporter import sbt.internal.util.{ Attributed, Settings, ~> } import sbt.util.{ Eval => Ev, Show } import sbt.internal.util.Attributed.data @@ -909,13 +909,15 @@ private[sbt] object Load { def pluginDefinitionLoader(config: LoadBuildConfiguration, pluginData: PluginData): (Seq[Attributed[File]], ClassLoader) = pluginDefinitionLoader(config, pluginData.dependencyClasspath, pluginData.definitionClasspath) + def buildPluginClasspath(config: LoadBuildConfiguration, + depcp: Seq[Attributed[File]]): Def.Classpath = { + if (depcp.isEmpty) config.classpath + else (depcp ++ config.classpath).distinct + } + def pluginDefinitionLoader(config: LoadBuildConfiguration, depcp: Seq[Attributed[File]], defcp: Seq[Attributed[File]]): (Seq[Attributed[File]], ClassLoader) = { - val definitionClasspath = - if (depcp.isEmpty) - config.classpath - else - (depcp ++ config.classpath).distinct + val definitionClasspath = buildPluginClasspath(config, depcp) val pm = config.pluginManagement // only the dependencyClasspath goes in the common plugin class loader ... def addToLoader() = pm.loader add Path.toURLs(data(depcp)) @@ -1000,7 +1002,7 @@ private[sbt] object Load { final case class LoadBuildConfiguration( stagingDirectory: File, - classpath: Seq[Attributed[File]], + classpath: Def.Classpath, loader: ClassLoader, compilers: Compilers, evalPluginDef: (BuildStructure, State) => PluginData, @@ -1012,21 +1014,23 @@ final case class LoadBuildConfiguration( extraBuilds: Seq[URI], log: Logger ) { - lazy val (globalPluginClasspath, _) = Load.pluginDefinitionLoader(this, Load.globalPluginClasspath(globalPlugin)) + lazy val globalPluginClasspath: Def.Classpath = + Load.buildPluginClasspath(this, Load.globalPluginClasspath(globalPlugin)) - private[sbt] lazy val globalPluginDefs = { + lazy val detectedGlobalPlugins: DetectedPlugins = { val pluginData = globalPlugin match { - case Some(x) => PluginData(x.data.fullClasspath, x.data.internalClasspath, Some(x.data.resolvers), Some(x.data.updateReport), Nil) - case None => PluginData(globalPluginClasspath, Nil, None, None, Nil) + case Some(info) => + val data = info.data + PluginData(data.fullClasspath, data.internalClasspath, Some(data.resolvers), Some(data.updateReport), Nil) + case None => PluginData(globalPluginClasspath) } val baseDir = globalPlugin match { case Some(x) => x.base case _ => stagingDirectory } - Load.loadPluginDefinition(baseDir, this, pluginData) + val globalPlugins = Load.loadPluginDefinition(baseDir, this, pluginData) + globalPlugins.detected } - - lazy val detectedGlobalPlugins = globalPluginDefs.detected } final class IncompatiblePluginsException(msg: String, cause: Throwable) extends Exception(msg, cause) From 9175193df3c8ad0980c7dc025649b38803253a78 Mon Sep 17 00:00:00 2001 From: jvican Date: Wed, 19 Apr 2017 09:08:28 +0200 Subject: [PATCH 2/4] Remove unused `pluginDefinitionLoader` methods This commit reduces the complexity around `loadPluginDefinition` et al. `pluginDefinitionLoader` is not used anywhere in sbt, so the extra definitions are removed. Both the implementation of `loadPluginDefinition` and `pluginDefinitionLoader` are reduced to a bare minimum where the components at hand (definition classpath, dependency classpath) are properly defined. Documentation to the three methods has been added. --- main/src/main/scala/sbt/internal/Load.scala | 79 ++++++++++++++------- 1 file changed, 52 insertions(+), 27 deletions(-) diff --git a/main/src/main/scala/sbt/internal/Load.scala b/main/src/main/scala/sbt/internal/Load.scala index 453b37618..6a5f3f664 100755 --- a/main/src/main/scala/sbt/internal/Load.scala +++ b/main/src/main/scala/sbt/internal/Load.scala @@ -897,42 +897,67 @@ private[sbt] object Load { def buildPlugins(dir: File, s: State, config: LoadBuildConfiguration): LoadedPlugins = loadPluginDefinition(dir, config, buildPluginDefinition(dir, s, config)) - def loadPluginDefinition(dir: File, config: LoadBuildConfiguration, pluginData: PluginData): LoadedPlugins = - { - val (definitionClasspath, pluginLoader) = pluginDefinitionLoader(config, pluginData) - loadPlugins(dir, pluginData.copy(dependencyClasspath = definitionClasspath), pluginLoader) - } - - def pluginDefinitionLoader(config: LoadBuildConfiguration, dependencyClasspath: Seq[Attributed[File]]): (Seq[Attributed[File]], ClassLoader) = - pluginDefinitionLoader(config, dependencyClasspath, Nil) - - def pluginDefinitionLoader(config: LoadBuildConfiguration, pluginData: PluginData): (Seq[Attributed[File]], ClassLoader) = - pluginDefinitionLoader(config, pluginData.dependencyClasspath, pluginData.definitionClasspath) + /** Loads the plugins. + * + * @param dir The base directory for the build. + * @param config The configuration for the the build. + * @param pluginData The data required to load plugins. + * @return An instance of the loaded build with plugin information. + */ + def loadPluginDefinition(dir: File, + config: LoadBuildConfiguration, + pluginData: PluginData): LoadedPlugins = { + val definitionClasspath = pluginData.definitionClasspath + val dependencyClasspath = pluginData.dependencyClasspath + val pluginLoader: ClassLoader = + pluginDefinitionLoader(config, dependencyClasspath, definitionClasspath) + val fullDependencyClasspath: Def.Classpath = + buildPluginClasspath(config, pluginData.dependencyClasspath) + val newData = pluginData.copy(dependencyClasspath = fullDependencyClasspath) + loadPlugins(dir, newData, pluginLoader) + } + /** Constructs the classpath required to load plugins, the so-called + * dependency classpath, from the provided classpath and the current config. + * + * @param config The configuration that declares classpath entries. + * @param depcp The user-defined dependency classpath. + * @return A classpath aggregating both without repeated entries. + */ def buildPluginClasspath(config: LoadBuildConfiguration, depcp: Seq[Attributed[File]]): Def.Classpath = { if (depcp.isEmpty) config.classpath else (depcp ++ config.classpath).distinct } - def pluginDefinitionLoader(config: LoadBuildConfiguration, depcp: Seq[Attributed[File]], defcp: Seq[Attributed[File]]): (Seq[Attributed[File]], ClassLoader) = - { - val definitionClasspath = buildPluginClasspath(config, depcp) - val pm = config.pluginManagement - // only the dependencyClasspath goes in the common plugin class loader ... - def addToLoader() = pm.loader add Path.toURLs(data(depcp)) - - val parentLoader = if (depcp.isEmpty) pm.initialLoader else { addToLoader(); pm.loader } - val pluginLoader = - if (defcp.isEmpty) - parentLoader - else { - // ... the build definition classes get their own loader so that they don't conflict with other build definitions (#511) - ClasspathUtilities.toLoader(data(defcp), parentLoader) - } - (definitionClasspath, pluginLoader) + /** Creates a classloader with a hierarchical structure, where the parent + * classloads the dependency classpath and the return classloader classloads + * the definition classpath. + * + * @param config The configuration for the whole sbt build. + * @param dependencyClasspath The dependency classpath (sbt dependencies). + * @param definitionClasspath The definition classpath for build definitions. + * @return A classloader ready to class load plugins. + */ + def pluginDefinitionLoader(config: LoadBuildConfiguration, + dependencyClasspath: Def.Classpath, + definitionClasspath: Def.Classpath): ClassLoader = { + val manager = config.pluginManagement + val parentLoader: ClassLoader = { + if (dependencyClasspath.isEmpty) manager.initialLoader + else { + // Load only the dependency classpath for the common plugin classloader + val loader = manager.loader + loader.add(Path.toURLs(data(dependencyClasspath))) + loader + } } + // Load the definition classpath separately to avoid conflicts, see #511. + if (definitionClasspath.isEmpty) parentLoader + else ClasspathUtilities.toLoader(data(definitionClasspath), parentLoader) + } + def buildPluginDefinition(dir: File, s: State, config: LoadBuildConfiguration): PluginData = { val (eval, pluginDef) = apply(dir, s, config) From d2a51d5085392e6b0b7b88118f00247c8a788ebf Mon Sep 17 00:00:00 2001 From: jvican Date: Wed, 19 Apr 2017 10:12:03 +0200 Subject: [PATCH 3/4] Indent timing logs Sbt has a feature to show timed logs for every operation at startup. However, its output is cluttered and users cannot read how much time single methods consume nor if they call other methods. This commit improves the status quo by adding indentation. --- main/src/main/scala/sbt/internal/Load.scala | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/main/src/main/scala/sbt/internal/Load.scala b/main/src/main/scala/sbt/internal/Load.scala index 6a5f3f664..fcf5edd90 100755 --- a/main/src/main/scala/sbt/internal/Load.scala +++ b/main/src/main/scala/sbt/internal/Load.scala @@ -1015,12 +1015,18 @@ private[sbt] object Load { } } + /** Variable to control the indentation of the timing logs. */ + private var timedIndentation: Int = 0 + /** Debugging method to time how long it takes to run various compilation tasks. */ private[sbt] def timed[T](label: String, log: Logger)(t: => T): T = { + timedIndentation += 1 val start = System.nanoTime val result = t val elapsed = System.nanoTime - start - log.debug(label + " took " + (elapsed / 1e6) + " ms") + timedIndentation -= 1 + val prefix = " " * 2 * timedIndentation + log.debug(s"$prefix$label took ${elapsed / 1e6}ms") result } } From 5c48b11d3fc6ca8596d6f21e104eb23fe1881936 Mon Sep 17 00:00:00 2001 From: jvican Date: Wed, 19 Apr 2017 13:46:04 +0200 Subject: [PATCH 4/4] Simplify and document `load` This commit has two goals: * Simplify the `load` API endpoints, removing the unused ones to shorten the surface of the API. * Add documentation to the main `load` methods. --- .../main/scala/sbt/internal/BuildLoader.scala | 11 +++ main/src/main/scala/sbt/internal/Load.scala | 84 +++++++++++++------ 2 files changed, 68 insertions(+), 27 deletions(-) diff --git a/main/src/main/scala/sbt/internal/BuildLoader.scala b/main/src/main/scala/sbt/internal/BuildLoader.scala index 3f83cff7f..8150824f2 100644 --- a/main/src/main/scala/sbt/internal/BuildLoader.scala +++ b/main/src/main/scala/sbt/internal/BuildLoader.scala @@ -97,6 +97,17 @@ object BuildLoader { } } +/** Defines the responsible for loading builds. + * + * @param fail A reporter for failures. + * @param state The state. + * @param config The current configuration for any build. + * @param resolvers The structure responsible of mapping base directories. + * @param builders The structure responsible of mapping to build units. + * @param transformer An instance to modify the created build unit. + * @param full The structure responsible of mapping to loaded build units. + * @param transformAll A function specifying which builds units should be transformed. + */ final class BuildLoader( val fail: URI => Nothing, val state: State, diff --git a/main/src/main/scala/sbt/internal/Load.scala b/main/src/main/scala/sbt/internal/Load.scala index fcf5edd90..6a4d3863d 100755 --- a/main/src/main/scala/sbt/internal/Load.scala +++ b/main/src/main/scala/sbt/internal/Load.scala @@ -333,30 +333,46 @@ private[sbt] object Load { } } - def load(file: File, s: State, config: LoadBuildConfiguration): PartBuild = - load(file, builtinLoader(s, config.copy(pluginManagement = config.pluginManagement.shift, extraBuilds = Nil)), config.extraBuilds.toList) + /** Loads the unresolved build units and computes its settings. + * + * @param root The root directory. + * @param s The given state. + * @param config The configuration of the loaded build. + * @return An instance of [[PartBuild]] with all the unresolved build units. + */ + def load(root: File, s: State, config: LoadBuildConfiguration): PartBuild = { + val manager = config.pluginManagement.shift + // Forced type ascription, otherwise scalac does not compile it + val newConfig: LoadBuildConfiguration = + config.copy(pluginManagement = manager, extraBuilds = Nil) + val loader = builtinLoader(s, newConfig) + loadURI(IO.directoryURI(root), loader, config.extraBuilds.toList) + } - def builtinLoader(s: State, config: LoadBuildConfiguration): BuildLoader = - { - val fail = (uri: URI) => sys.error("Invalid build URI (no handler available): " + uri) - val resolver = (info: BuildLoader.ResolveInfo) => RetrieveUnit(info) - val build = (info: BuildLoader.BuildInfo) => Some(() => - loadUnit(info.uri, info.base, info.state, info.config)) - val components = BuildLoader.components(resolver, build, full = BuildLoader.componentLoader) - BuildLoader(components, fail, s, config) - } + /** Creates a loader for the build. + * + * @param s The given state. + * @param config The configuration of the loaded build. + * @return A [[BuildLoader]]. + */ + def builtinLoader(s: State, config: LoadBuildConfiguration): BuildLoader = { + val fail = (uri: URI) => + sys.error("Invalid build URI (no handler available): " + uri) + val resolver = (info: BuildLoader.ResolveInfo) => RetrieveUnit(info) + val build = (info: BuildLoader.BuildInfo) => Some(() => + loadUnit(info.uri, info.base, info.state, info.config)) + val loader = BuildLoader.componentLoader + val components = BuildLoader.components(resolver, build, full = loader) + BuildLoader(components, fail, s, config) + } - def load(file: File, loaders: BuildLoader, extra: List[URI]): PartBuild = - loadURI(IO.directoryURI(file), loaders, extra) - - def loadURI(uri: URI, loaders: BuildLoader, extra: List[URI]): PartBuild = - { + private def loadURI(uri: URI, loader: BuildLoader, extra: List[URI]): PartBuild = { IO.assertAbsolute(uri) - val (referenced, map, newLoaders) = loadAll(uri :: extra, Map.empty, loaders, Map.empty) + val (referenced, map, newLoaders) = loadAll(uri +: extra, Map.empty, loader, Map.empty) checkAll(referenced, map) val build = new PartBuild(uri, map) newLoaders transformAll build - } + } def addOverrides(unit: BuildUnit, loaders: BuildLoader): BuildLoader = loaders updatePluginManagement PluginManagement.extractOverrides(unit.plugins.fullClasspath) @@ -395,21 +411,35 @@ private[sbt] object Load { Project.transform(resolve, unit.definitions.builds.flatMap(_.settings)) } - @tailrec def loadAll(bases: List[URI], references: Map[URI, List[ProjectReference]], loaders: BuildLoader, builds: Map[URI, PartBuildUnit]): (Map[URI, List[ProjectReference]], Map[URI, PartBuildUnit], BuildLoader) = + @tailrec private def loadAll( + bases: List[URI], + references: Map[URI, List[ProjectReference]], + loaders: BuildLoader, + builds: Map[URI, PartBuildUnit] + ): (Map[URI, List[ProjectReference]], Map[URI, PartBuildUnit], BuildLoader) = { + def loadURI(base: URI, bases: List[URI]) = { + val (loadedBuild, refs) = loaded(loaders(base)) + val uris = refs.flatMap(Reference.uri) + val buildUnit = loadedBuild.unit + checkBuildBase(buildUnit.localBase) + val resolvers = addResolvers(buildUnit, builds.isEmpty, loaders.resetPluginDepth) + val updatedLoader = addOverrides(buildUnit, resolvers) + val updatedReferences = references.updated(base, refs) + val updatedBuilds = builds.updated(base, loadedBuild) + val sortedRemaining = uris.reverse_:::(bases).sorted + (sortedRemaining, updatedReferences, updatedLoader, updatedBuilds) + } + bases match { case b :: bs => - if (builds contains b) - loadAll(bs, references, loaders, builds) + if (builds contains b) loadAll(bs, references, loaders, builds) else { - val (loadedBuild, refs) = loaded(loaders(b)) - checkBuildBase(loadedBuild.unit.localBase) - val newLoader = addOverrides(loadedBuild.unit, addResolvers(loadedBuild.unit, builds.isEmpty, loaders.resetPluginDepth)) - // it is important to keep the load order stable, so we sort the remaining URIs - val remainingBases = (refs.flatMap(Reference.uri) reverse_::: bs).sorted - loadAll(remainingBases, references.updated(b, refs), newLoader, builds.updated(b, loadedBuild)) + val (newBases, newReferences, newLoaders, newBuilds) = loadURI(b, bs) + loadAll(newBases, newReferences, newLoaders, newBuilds) } case Nil => (references, builds, loaders) } + } def checkProjectBase(buildBase: File, projectBase: File): Unit = { checkDirectory(projectBase)