Cache Scalafix instances globally #206
Conversation
lolgab
force-pushed
the
cache-scalafix-instances
branch
from
c037bf0 to
7d57735
Compare
|
@bjaglin Do you mind reviewing it? |
| private val cache = new ConcurrentHashMap[(String, Seq[Repository]), SoftReference[Scalafix]] | ||
|
|
||
| def getOrElseCreate(scalaVersion: String, repositories: Seq[Repository]) = | ||
| cache.get((scalaVersion, repositories)) match { |
There was a problem hiding this comment.
Having a blocking method like computeIfAbsent would avoid getting several concurrent initialisations of the same instance on projects with many modules. This helped with sbt, not sure if the threading model is the same with mill.
There was a problem hiding this comment.
That's a great idea!
I used compute since I wanted to keep the SoftReference.
|
|
||
| private val cache = new ConcurrentHashMap[(String, Seq[Repository]), SoftReference[Scalafix]] | ||
|
|
||
| def getOrElseCreate(scalaVersion: String, repositories: Seq[Repository]) = |
There was a problem hiding this comment.
Since repositories is provided, should we have it in the cache key as well?
| val scalafix = Scalafix | ||
| .fetchAndClassloadInstance(scalaVersion, repositories.map(CoursierUtils.toApiRepository).asJava) | ||
| val scalafix = ScalafixCache | ||
| .getOrElseCreate(scalaVersion, repositories) |
There was a problem hiding this comment.
Potential tweak even further: as I was mentioning no later than yesterday in the docs, only a major.minor version is required, so FWIW you could strip the patch version to maximize cache hit ratio (the instance will be the same for 3.3.3 and 3.3.4).
There was a problem hiding this comment.
That assumption is true now but it might become false in future versions of scalafix. Since it's very rare to use different patch versions in the same build, I wouldn't bother trying to be clever here.
There was a problem hiding this comment.
Totally fair. FTR, that assumption is based on Scala current semver semantics ensuring backward and forward source compatibility for patch releases, but it could indeed change in the future.
There was a problem hiding this comment.
Alternatively, we could use the full scalafix classpath (as PathRefs, either sorted or as Set) as cache key.
|
@lefou Maybe if I make it a |
|
Oh but |
|
@lolgab Yeah, a |
Fixes #175