centralize ScalafixInterface memoization within a setting key #409
Conversation
bjaglin
changed the title
bjaglin
force-pushed
the
memoization
branch
3 times, most recently
from
e015cb7 to
d46ef1e
Compare
bjaglin
changed the title
bjaglin
changed the title
bjaglin
force-pushed
the
memoization
branch
2 times, most recently
from
c28d835 to
a7f9da8
Compare
bjaglin
changed the title
bjaglin
force-pushed
the
memoization
branch
5 times, most recently
from
e9d2d5e to
2870da0
Compare
| // cache hit, don't update | ||
| None | ||
| case _ => | ||
| // cache miss or stale stamps, resolve custom rules artifacts and classload them |
There was a problem hiding this comment.
| // cache hit, don't update | ||
| None | ||
| case None => | ||
| // cache miss, resolve scalafix artifacts and classload them |
There was a problem hiding this comment.
| scalafixScalaBinaryVersion, | ||
| None |
There was a problem hiding this comment.
Memoizing first an interface per Scala version (ie.e ignoring the requested tool classpath) is key for the tool classpath instances created below to share the common classloader with the entire Scala standard library.
| // cache miss or stale stamps, resolve custom rules artifacts and classload them | ||
| Some( | ||
| ( | ||
| buildinRulesInterface.withArgs(toolClasspath), |
There was a problem hiding this comment.
by replacing the former value with this, we make the former interface, together with its toolClasspath classloader, eligible for garbage collection
bjaglin
force-pushed
the
memoization
branch
2 times, most recently
from
b7ec5df to
2402659
Compare
…zation The persistent cache is now bound to a setting key so that its lifecycle is aligned with an sbt session (allowing flushing via a `reload`) The per-invocation cache is merged into the persistent one, which means that the local rules classloader will now remain warm/JITed across invocations. The cache key and the stamping strategy prevents instances from leaking when local rules are being iterated on and recompiled between invocations.
| .map(uri => java.nio.file.Paths.get(uri).toFile) | ||
| .flatMap { | ||
| case classDirectory if classDirectory.isDirectory => | ||
| classDirectory.**(RegularFileFilter).get |
There was a problem hiding this comment.
not directly related to this PR: this was changed from AllPassFilter to RegularFileFilter as the code was moved, in order to skip directories (that don't have any impact on the behavior)
| ) | ||
|
|
||
| val informationalInvocation = shellArgs.extra | ||
| .intersect(List("--help", "-h", "--version", "-v")) |
There was a problem hiding this comment.
bonus feature: this removes the need for triggering compilation when --version or -v is requested
| /** A basic thread-safe cache without any eviction. */ | ||
| class BlockingCache[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]) | ||
| private val underlying = new ju.concurrent.ConcurrentHashMap[K, V] |
There was a problem hiding this comment.
ConcurrentHashMap was ruled out in the past, but I confirmed that there is a per-key lock on compute().
| @@ -31,6 +32,16 @@ object Arg { | |||
| customDependencies.map(_.asCoursierCoordinates).asJava, | |||
| repositories.asJava | |||
| ) | |||
|
|
|||
| def mutableFiles: Seq[File] = | |||
| customURIs | |||
There was a problem hiding this comment.
customURIs is effectively the only mutable part of ToolClasspath since customDependencies are immutable once resolved
Code review showed that we have several types of memoization since #380. It turns that we have actually 3 different types of caching at the moment:
LazyValuememoizer, effectively useless since Support several scalafixScalaBinaryVersion within a build #380 given how it's useddependency:ruleThis PR is folding these 3 caches into a single setting-bound one, to (hopefully) make the code more maintainable, as well as improving the UX for developers using local rules:
scalafixinvocations (particularly benefiting users ofscalafixOnCompile) thanks to the warm classloader for local rules