Fix 0.9.18 caching/performance regressions #142
Conversation
bjaglin
changed the title
bjaglin
changed the title
bjaglin
changed the title
Regression introduced in 2ca68e4: to be memoized, settings must be bound to a setting key and defined on a scope.
This avoids initialization of a classloader that has the exact same class files as its parent for each invocation.
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.
| import scala.collection.mutable | ||
|
|
||
| /** A basic thread-safe cache without any eviction. */ | ||
| class BlockingCache[K, V] { |
There was a problem hiding this comment.
FWIW, if you don't mind using Java collections you can replace this class with
import java.{util => ju}
object BlockingCache {
def empty[A, B] =
ju.Collections.synchronizedMap(new ju.HashMap[A, B])
}
val cache = BlockingCache.empty[Int, String]
cache.computeIfAbsent(10, _ => "9")
cache // {10=9}Those APIs are quite quirky however (return AnyRef in many places) so I like rolling a custom Scala solution instead.
There was a problem hiding this comment.
good idea - I kept the facade but swapped the implementation in 28e70a7
| ) | ||
| if (customToolClasspath) { | ||
| val toolClasspath = ToolClasspath( | ||
| projectDepsInternal0.map(_.toURI.toURL), |
There was a problem hiding this comment.
Unrelated to this PR, I see that we're using java.net.URL as cache keys. This is problematic because URL.equals does remote blocking calls
Two hosts are considered equivalent if both host names can be resolved into the same IP addresses; else if either host name can't be resolved, the host names must be equal without regard to case; or both host names equal to null.
https://docs.oracle.com/javase/6/docs/api/java/net/URL.html#equals(java.lang.Object)
It's probably better to use URI instead of URL
There was a problem hiding this comment.
👍 already got bitten by that in another project, but apparently not enough to care... will change that in a separate PR
scalafixinvocation (changed also in 2ca68e4), to reduce the risk of JGit stuff blocking the initialization like updating to sbt-scalafix 0.9.18 causes crash scalafix#1196. Related to prevent failure on sbt init when JGit finds a .git with no commit #141.We'll probably need some performance tests to catch those regressions in the future...
Testing on a real project
I have timed the initialization overhead of
scalafixby running a warmall scalafix test:scalafix it:scalafix(third invocation withscalafixCaching := true) on a build with 25 sub-projects which have Scalafix enabled.With
scalafixDependenciesWith
scalafixDependencies&dependsOn(customRules % ScalafixConfig)The impact of (not) JITing the rules is not exposed by this test, but rule execution should be back to pre-0.9.18 level as well.