Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

centralize ScalafixInterface memoization within a setting key #409

Merged
merged 5 commits into from
Apr 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 23 additions & 12 deletions src/main/scala/scalafix/internal/sbt/BlockingCache.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,32 @@ package scalafix.internal.sbt

import java.{util => ju}

import scala.util.Try

/** 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]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ConcurrentHashMap was ruled out in the past, but I confirmed that there is a per-key lock on compute().


private case class SkipUpdate(prev: V) extends Exception

/** @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 =
underlying.computeIfAbsent(
key,
new ju.function.Function[K, V] {
override def apply(k: K): V = value
}
// Evaluations of `transform` are guaranteed to be sequential for the same key
def compute(key: K, transform: Option[V] => Option[V]): V = {
Try(
underlying.compute(
key,
new ju.function.BiFunction[K, V, V] {
override def apply(key: K, prev: V): V = {
transform(Option(prev)).getOrElse(throw SkipUpdate(prev))
}
}
)
).fold(
{
case SkipUpdate(prev) => prev
case ex => throw ex
},
identity
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,6 @@ import sbt.complete.DefaultParsers._

class ScalafixCompletions(
workingDirectory: Path,
// 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: () => Seq[ScalafixRule],
terminalWidth: Option[Int],
allowedTargetFilesPrefixes: Seq[Path], // Nil means all values are valid
Expand Down
115 changes: 82 additions & 33 deletions src/main/scala/scalafix/internal/sbt/ScalafixInterface.scala
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import java.nio.file.Path
import java.{util => jutil}
import coursierapi.Repository
import sbt._
import sbt.io.RegularFileFilter
import scalafix.interfaces.{Scalafix => ScalafixAPI, _}
import scalafix.sbt.InvalidArgument

Expand Down Expand Up @@ -31,6 +32,16 @@ object Arg {
customDependencies.map(_.asCoursierCoordinates).asJava,
repositories.asJava
)

def mutableFiles: Seq[File] =
customURIs
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

customURIs is effectively the only mutable part of ToolClasspath since customDependencies are immutable once resolved

.map(uri => java.nio.file.Paths.get(uri).toFile)
.flatMap {
case classDirectory if classDirectory.isDirectory =>
classDirectory.**(RegularFileFilter).get
Copy link
Collaborator Author

@bjaglin bjaglin Apr 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

case jar =>
Seq(jar)
}
}

case class Rules(rules: Seq[String]) extends Arg with CacheKey {
Expand Down Expand Up @@ -81,7 +92,7 @@ object Arg {

class ScalafixInterface private (
scalafixArguments: ScalafixArguments, // hide it to force usage of withArgs so we can intercept arguments
val args: Seq[Arg]
val args: Seq[Arg] = Nil
) {

def withArgs(args: Arg*): ScalafixInterface = {
Expand Down Expand Up @@ -112,31 +123,42 @@ class ScalafixInterface private (
}

object ScalafixInterface {
private class LazyValue[T](thunk: () => T) extends (() => T) {
private lazy val _value = scala.util.Try(thunk())
override def apply(): T = _value.get
}
private val fromToolClasspathMemo: BlockingCache[
(String, Seq[ModuleID], Seq[Repository]),
ScalafixInterface
] = new BlockingCache

type Cache = BlockingCache[
(
String, // scalafixScalaBinaryVersion
Option[Arg.ToolClasspath]
),
(
ScalafixInterface,
Seq[Long] // mutable files stamping
)
]

private[scalafix] val defaultLogger: Logger = ConsoleLogger(System.out)

def fromToolClasspath(
def apply(
cache: Cache,
scalafixScalaBinaryVersion: String,
scalafixDependencies: Seq[ModuleID],
scalafixCustomResolvers: Seq[Repository],
toolClasspath: Arg.ToolClasspath,
logger: Logger,
callback: ScalafixMainCallback
): () => ScalafixInterface =
new LazyValue({ () =>
fromToolClasspathMemo.getOrElseUpdate(
(
scalafixScalaBinaryVersion,
scalafixDependencies,
scalafixCustomResolvers
), {
): ScalafixInterface = {

// Build or retrieve from the cache the scalafix interface that can run
// built-in rules. This is the most costly instantiation, which should be
// shared as much as possible.
val (buildinRulesInterface, _) = cache.compute(
(
scalafixScalaBinaryVersion,
None
Comment on lines +153 to +154
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

),
{
case Some(_) =>
// cache hit, don't update
None
case None =>
// cache miss, resolve scalafix artifacts and classload them
Copy link
Collaborator Author

@bjaglin bjaglin Apr 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (scalafixScalaBinaryVersion.startsWith("3"))
logger.error(
"To use Scalafix on Scala 3 projects, you must unset `scalafixScalaBinaryVersion`. " +
Expand All @@ -151,27 +173,54 @@ object ScalafixInterface {
val scalafixArguments = ScalafixAPI
.fetchAndClassloadInstance(
scalafixScalaBinaryVersion,
scalafixCustomResolvers.asJava
toolClasspath.repositories.asJava
)
.newArguments()
.withMainCallback(callback)
val printStream =

val printStream = Arg.PrintStream(
new PrintStream(
LoggingOutputStream(
logger,
Level.Info
)
)
new ScalafixInterface(scalafixArguments, Nil)
.withArgs(
Arg.PrintStream(printStream),
Arg.ToolClasspath(
Nil,
scalafixDependencies,
scalafixCustomResolvers
)
)

Some(
(
new ScalafixInterface(scalafixArguments).withArgs(printStream),
Nil
)
}
)
})
)
}
)

// stamp the files that might change, to potentialy force invalidation
// of the corresponding cache entry
val currentStamps =
toolClasspath.mutableFiles.map(IO.getModifiedTimeOrZero)

val (toolClasspathInterface, _) = cache.compute(
(
scalafixScalaBinaryVersion,
Some(toolClasspath)
),
{
case Some((_, oldStamps)) if (currentStamps == oldStamps) =>
// cache hit, don't update
None
case _ =>
// cache miss or stale stamps, resolve custom rules artifacts and classload them
Copy link
Collaborator Author

@bjaglin bjaglin Apr 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some(
(
buildinRulesInterface.withArgs(toolClasspath),
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by replacing the former value with this, we make the former interface, together with its toolClasspath classloader, eligible for garbage collection

currentStamps
)
)
}
)

toolClasspathInterface
}
}
Loading
Loading