From a9673772c654017cb922eb3d9fb2e159b1bade81 Mon Sep 17 00:00:00 2001 From: Brice Jaglin Date: Tue, 7 Jul 2020 13:00:53 +0200 Subject: [PATCH 1/5] fix session caching for the default scalafixInterface & parser Regression introduced in 2ca68e4: to be memoized, settings must be bound to a setting key and defined on a scope. --- .../scala/scalafix/sbt/ScalafixPlugin.scala | 58 +++++++++++-------- 1 file changed, 34 insertions(+), 24 deletions(-) diff --git a/src/main/scala/scalafix/sbt/ScalafixPlugin.scala b/src/main/scala/scalafix/sbt/ScalafixPlugin.scala index 7df562be..5e1b0230 100644 --- a/src/main/scala/scalafix/sbt/ScalafixPlugin.scala +++ b/src/main/scala/scalafix/sbt/ScalafixPlugin.scala @@ -123,6 +123,20 @@ object ScalafixPlugin extends AutoPlugin { Invisible ) + private val scalafixInterfaceProvider: SettingKey[() => ScalafixInterface] = + SettingKey( + "scalafixInterfaceProvider", + "Implementation detail - do not use", + Invisible + ) + + private val scalafixParser: SettingKey[Parser[ShellArgs]] = + SettingKey( + "scalafixParser", + "Implementation detail - do not use", + Invisible + ) + override lazy val projectConfigurations: Seq[Configuration] = Seq(ScalafixConfig) @@ -147,7 +161,19 @@ object ScalafixPlugin extends AutoPlugin { ) ), scalafixDependencies := Nil, - commands += ScalafixEnable.command + commands += ScalafixEnable.command, + scalafixInterfaceProvider := ScalafixInterface.fromToolClasspath( + scalafixScalaBinaryVersion.in(ThisBuild).value, + scalafixDependencies = scalafixDependencies.in(ThisBuild).value, + scalafixCustomResolvers = scalafixResolvers.in(ThisBuild).value + ), + scalafixParser := new ScalafixCompletions( + workingDirectory = baseDirectory.in(ThisBuild).value.toPath, + // Unfortunately, local rules will not show up as completions in the parser, as that parser can only + // depend on settings, while local rules classpath must be looked up via tasks + loadedRules = () => scalafixInterfaceProvider.value().availableRules(), + terminalWidth = Some(JLineAccess.terminalWidth) + ).parser ) override def buildSettings: Seq[Def.Setting[_]] = @@ -157,25 +183,6 @@ object ScalafixPlugin extends AutoPlugin { lazy val stdoutLogger = Compat.ConsoleLogger(System.out) - private val scalafixInterface: Def.Initialize[() => ScalafixInterface] = - Def.setting { - ScalafixInterface.fromToolClasspath( - scalafixScalaBinaryVersion.in(ThisBuild).value, - scalafixDependencies = scalafixDependencies.in(ThisBuild).value, - scalafixCustomResolvers = scalafixResolvers.in(ThisBuild).value - ) - } - - private val parser: Def.Initialize[Parser[ShellArgs]] = Def.setting { - new ScalafixCompletions( - workingDirectory = baseDirectory.in(ThisBuild).value.toPath, - // Unfortunately, local rules will not show up as completions in the parser, as that parser can only - // depend on settings, while local rules classpath must be looked up via tasks - loadedRules = () => scalafixInterface.value().availableRules(), - terminalWidth = Some(JLineAccess.terminalWidth) - ).parser - } - private def scalafixArgsFromShell( shell: ShellArgs, scalafixInterface: () => ScalafixInterface, @@ -222,7 +229,7 @@ object ScalafixPlugin extends AutoPlugin { private def scalafixAllInputTask(): Def.Initialize[InputTask[Unit]] = // workaround https://github.com/sbt/sbt/issues/3572 by invoking directly what Def.inputTaskDyn would via macro InputTask - .createDyn(InputTask.initParserAsInput(parser))( + .createDyn(InputTask.initParserAsInput(scalafixParser))( Def.task(shellArgs => scalafixAllTask(shellArgs, thisProject.value)) ) @@ -246,7 +253,7 @@ object ScalafixPlugin extends AutoPlugin { ): Def.Initialize[InputTask[Unit]] = // workaround https://github.com/sbt/sbt/issues/3572 by invoking directly what Def.inputTaskDyn would via macro InputTask - .createDyn(InputTask.initParserAsInput(parser))( + .createDyn(InputTask.initParserAsInput(scalafixParser))( Def.task(shellArgs => scalafixTask(shellArgs, config)) ) @@ -276,7 +283,7 @@ object ScalafixPlugin extends AutoPlugin { val scalafixConf = scalafixConfig.in(config).value.map(_.toPath) val (shell, mainInterface0) = scalafixArgsFromShell( shellArgs, - scalafixInterface.value, + scalafixInterfaceProvider.value, projectDepsExternal, scalafixDependencies.in(ThisBuild).value, scalafixResolvers.in(ThisBuild).value, @@ -306,7 +313,10 @@ object ScalafixPlugin extends AutoPlugin { } private def scalafixHelp: Def.Initialize[Task[Unit]] = Def.task { - scalafixInterface.value().withArgs(Arg.ParsedArgs(List("--help"))).run() + scalafixInterfaceProvider + .value() + .withArgs(Arg.ParsedArgs(List("--help"))) + .run() () } From bde92e3af7b72129e971b08e050992abd9a68c13 Mon Sep 17 00:00:00 2001 From: Brice Jaglin Date: Tue, 7 Jul 2020 13:00:53 +0200 Subject: [PATCH 2/5] initialize parser before invocation time rather than at sbt init --- src/main/scala/scalafix/sbt/ScalafixPlugin.scala | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/main/scala/scalafix/sbt/ScalafixPlugin.scala b/src/main/scala/scalafix/sbt/ScalafixPlugin.scala index 5e1b0230..37c38d2f 100644 --- a/src/main/scala/scalafix/sbt/ScalafixPlugin.scala +++ b/src/main/scala/scalafix/sbt/ScalafixPlugin.scala @@ -8,7 +8,6 @@ import coursierapi.Repository import sbt.KeyRanks.Invisible import sbt.Keys._ import sbt._ -import sbt.complete._ import sbt.internal.sbtscalafix.Caching._ import sbt.internal.sbtscalafix.{Compat, JLineAccess} import sbt.plugins.JvmPlugin @@ -130,9 +129,9 @@ object ScalafixPlugin extends AutoPlugin { Invisible ) - private val scalafixParser: SettingKey[Parser[ShellArgs]] = + private val scalafixCompletions: SettingKey[ScalafixCompletions] = SettingKey( - "scalafixParser", + "scalafixCompletions", "Implementation detail - do not use", Invisible ) @@ -167,13 +166,13 @@ object ScalafixPlugin extends AutoPlugin { scalafixDependencies = scalafixDependencies.in(ThisBuild).value, scalafixCustomResolvers = scalafixResolvers.in(ThisBuild).value ), - scalafixParser := new ScalafixCompletions( + scalafixCompletions := new ScalafixCompletions( workingDirectory = baseDirectory.in(ThisBuild).value.toPath, // Unfortunately, local rules will not show up as completions in the parser, as that parser can only // depend on settings, while local rules classpath must be looked up via tasks loadedRules = () => scalafixInterfaceProvider.value().availableRules(), terminalWidth = Some(JLineAccess.terminalWidth) - ).parser + ) ) override def buildSettings: Seq[Def.Setting[_]] = @@ -229,7 +228,7 @@ object ScalafixPlugin extends AutoPlugin { private def scalafixAllInputTask(): Def.Initialize[InputTask[Unit]] = // workaround https://github.com/sbt/sbt/issues/3572 by invoking directly what Def.inputTaskDyn would via macro InputTask - .createDyn(InputTask.initParserAsInput(scalafixParser))( + .createDyn(InputTask.initParserAsInput(scalafixCompletions(_.parser)))( Def.task(shellArgs => scalafixAllTask(shellArgs, thisProject.value)) ) @@ -253,7 +252,7 @@ object ScalafixPlugin extends AutoPlugin { ): Def.Initialize[InputTask[Unit]] = // workaround https://github.com/sbt/sbt/issues/3572 by invoking directly what Def.inputTaskDyn would via macro InputTask - .createDyn(InputTask.initParserAsInput(scalafixParser))( + .createDyn(InputTask.initParserAsInput(scalafixCompletions(_.parser)))( Def.task(shellArgs => scalafixTask(shellArgs, config)) ) From b75a0fab6b13a44676bbf1634ec55c0a428e5827 Mon Sep 17 00:00:00 2001 From: Brice Jaglin Date: Tue, 7 Jul 2020 13:00:53 +0200 Subject: [PATCH 3/5] only use a custom toolclasspath if we have classes in ScalafixConfig This avoids initialization of a classloader that has the exact same class files as its parent for each invocation. --- src/main/scala/scalafix/sbt/ScalafixPlugin.scala | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/main/scala/scalafix/sbt/ScalafixPlugin.scala b/src/main/scala/scalafix/sbt/ScalafixPlugin.scala index 37c38d2f..4e44ef98 100644 --- a/src/main/scala/scalafix/sbt/ScalafixPlugin.scala +++ b/src/main/scala/scalafix/sbt/ScalafixPlugin.scala @@ -204,13 +204,19 @@ object ScalafixPlugin extends AutoPlugin { throw new ScalafixFailed(List(ScalafixError.CommandLineError)) } val rulesDepsExternal = parsed.map(_.dependency) - val refreshClasspath = - (projectDepsInternal ++ projectDepsExternal ++ rulesDepsExternal).nonEmpty + val projectDepsInternal0 = projectDepsInternal.filter { + case directory if directory.isDirectory => + directory.**(AllPassFilter).get.exists(_.isFile) + case file if file.isFile => true + case _ => false + } + val customToolClasspath = + (projectDepsInternal0 ++ projectDepsExternal ++ rulesDepsExternal).nonEmpty val interface = - if (refreshClasspath) + if (customToolClasspath) scalafixInterface().withArgs( ToolClasspath( - projectDepsInternal.map(_.toURI.toURL), + projectDepsInternal0.map(_.toURI.toURL), baseDepsExternal ++ projectDepsExternal ++ rulesDepsExternal, baseResolvers ) From 89cfd04629a6c67d3110e073c1c9948675c85a37 Mon Sep 17 00:00:00 2001 From: Brice Jaglin Date: Tue, 7 Jul 2020 13:00:53 +0200 Subject: [PATCH 4/5] share custom toolclasspath classloader across projects & configs Since d81dd72 (see commit description), when `scalafix` or `scalafixAll` is run on a build root that aggregates many projects, many identical classloaders may be created when an external rule is provided on the CLI or when projects use local rules via ScalafixConfig. This allows aggregated tasks across projects & configs to share a single, warm classloader when possible (same Scalafix classpath). Note that cache is bound on purpose to a task value, so two successive invocations of `scalafix` with the exact same arguments & environment will not share the same classloader. This is for simplicity, as local rules can be updated from one run to the other, and to my knowledge, there is no simple way to invalidate a URLClassLoader if the underlying class files change. --- .../scalafix/internal/sbt/BlockingCache.scala | 21 +++++++++++ .../scala/scalafix/sbt/ScalafixPlugin.scala | 37 ++++++++++++++----- 2 files changed, 49 insertions(+), 9 deletions(-) create mode 100644 src/main/scala/scalafix/internal/sbt/BlockingCache.scala diff --git a/src/main/scala/scalafix/internal/sbt/BlockingCache.scala b/src/main/scala/scalafix/internal/sbt/BlockingCache.scala new file mode 100644 index 00000000..91a08c4c --- /dev/null +++ b/src/main/scala/scalafix/internal/sbt/BlockingCache.scala @@ -0,0 +1,21 @@ +package scalafix.internal.sbt + +import scala.collection.mutable + +/** A basic thread-safe cache without any eviction. */ +class BlockingCache[K, V] { + private val underlying = new mutable.HashMap[K, V] + + /** + * @param value By-name parameter evaluated when the key if missing. Value computation is guaranteed + * to be called only once per key across all invocations. + */ + def getOrElseUpdate(key: K, value: => V): V = { + // ConcurrentHashMap does not guarantee that there is only one evaluation of the value, so + // we use our own (global) locking, which is OK as the number of keys is expected to be + // very small (bound by the number of projects in the sbt build). + underlying.synchronized { + underlying.getOrElseUpdate(key, value) + } + } +} diff --git a/src/main/scala/scalafix/sbt/ScalafixPlugin.scala b/src/main/scala/scalafix/sbt/ScalafixPlugin.scala index 4e44ef98..3cee04f3 100644 --- a/src/main/scala/scalafix/sbt/ScalafixPlugin.scala +++ b/src/main/scala/scalafix/sbt/ScalafixPlugin.scala @@ -136,6 +136,16 @@ object ScalafixPlugin extends AutoPlugin { Invisible ) + // Memoize ScalafixInterface instances initialized with a custom tool classpath across projects & configurations + // during task execution, to amortize the classloading cost when invoking scalafix concurrently on many targets + private val scalafixInterfaceCache + : TaskKey[BlockingCache[ToolClasspath, ScalafixInterface]] = + TaskKey( + "scalafixInterfaceCache", + "Implementation detail - do not use", + Invisible + ) + override lazy val projectConfigurations: Seq[Configuration] = Seq(ScalafixConfig) @@ -172,7 +182,11 @@ object ScalafixPlugin extends AutoPlugin { // depend on settings, while local rules classpath must be looked up via tasks loadedRules = () => scalafixInterfaceProvider.value().availableRules(), terminalWidth = Some(JLineAccess.terminalWidth) - ) + ), + scalafixInterfaceCache := new BlockingCache[ + ToolClasspath, + ScalafixInterface + ] ) override def buildSettings: Seq[Def.Setting[_]] = @@ -185,6 +199,7 @@ object ScalafixPlugin extends AutoPlugin { private def scalafixArgsFromShell( shell: ShellArgs, scalafixInterface: () => ScalafixInterface, + scalafixInterfaceCache: BlockingCache[ToolClasspath, ScalafixInterface], projectDepsExternal: Seq[ModuleID], baseDepsExternal: Seq[ModuleID], baseResolvers: Seq[Repository], @@ -213,15 +228,18 @@ object ScalafixPlugin extends AutoPlugin { val customToolClasspath = (projectDepsInternal0 ++ projectDepsExternal ++ rulesDepsExternal).nonEmpty val interface = - if (customToolClasspath) - scalafixInterface().withArgs( - ToolClasspath( - projectDepsInternal0.map(_.toURI.toURL), - baseDepsExternal ++ projectDepsExternal ++ rulesDepsExternal, - baseResolvers - ) + if (customToolClasspath) { + val toolClasspath = ToolClasspath( + projectDepsInternal0.map(_.toURI.toURL), + baseDepsExternal ++ projectDepsExternal ++ rulesDepsExternal, + baseResolvers + ) + scalafixInterfaceCache.getOrElseUpdate( + toolClasspath, + // costly: triggers artifact resolution & classloader creation + scalafixInterface().withArgs(toolClasspath) ) - else + } else // if there is nothing specific to the project or the invocation, reuse the default // interface which already has the baseDepsExternal loaded scalafixInterface() @@ -289,6 +307,7 @@ object ScalafixPlugin extends AutoPlugin { val (shell, mainInterface0) = scalafixArgsFromShell( shellArgs, scalafixInterfaceProvider.value, + scalafixInterfaceCache.value, projectDepsExternal, scalafixDependencies.in(ThisBuild).value, scalafixResolvers.in(ThisBuild).value, From 28e70a723c4ac7b8b839f548dedf302302800e89 Mon Sep 17 00:00:00 2001 From: Brice Jaglin Date: Tue, 7 Jul 2020 14:56:54 +0200 Subject: [PATCH 5/5] use java collections for cache implementation --- .../scalafix/internal/sbt/BlockingCache.scala | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/main/scala/scalafix/internal/sbt/BlockingCache.scala b/src/main/scala/scalafix/internal/sbt/BlockingCache.scala index 91a08c4c..7f9ab57e 100644 --- a/src/main/scala/scalafix/internal/sbt/BlockingCache.scala +++ b/src/main/scala/scalafix/internal/sbt/BlockingCache.scala @@ -1,21 +1,22 @@ package scalafix.internal.sbt -import scala.collection.mutable +import java.{util => ju} /** A basic thread-safe cache without any eviction. */ class BlockingCache[K, V] { - private val underlying = new mutable.HashMap[K, V] + + // Number of keys is expected to be very small so the global lock should not be a bottleneck + private val underlying = ju.Collections.synchronizedMap(new ju.HashMap[K, V]) /** * @param value By-name parameter evaluated when the key if missing. Value computation is guaranteed * to be called only once per key across all invocations. */ - def getOrElseUpdate(key: K, value: => V): V = { - // ConcurrentHashMap does not guarantee that there is only one evaluation of the value, so - // we use our own (global) locking, which is OK as the number of keys is expected to be - // very small (bound by the number of projects in the sbt build). - underlying.synchronized { - underlying.getOrElseUpdate(key, value) - } - } + def getOrElseUpdate(key: K, value: => V): V = + underlying.computeIfAbsent( + key, + new ju.function.Function[K, V] { + override def apply(k: K): V = value + } + ) }