Opt-in caching for scalafix invocations #102
Conversation
bjaglin
force-pushed
the
caching
branch
13 times, most recently
from
2af116f to
6177b8b
Compare
| override final type Internal = Array[Byte] | ||
| override final def convert(keys: Seq[Arg.CacheKey]): Array[Byte] = { | ||
| val baos = new ByteArrayOutputStream() | ||
| output.withValue(new JavaOutput(baos)) { | ||
| keys.foreach(stamp) | ||
| } | ||
| Hash.apply(baos.toByteArray) | ||
| } |
There was a problem hiding this comment.
this effectively replicates the 1.x behavior (although with a binary representation and a different hashing algorithm)
| object StampingImpossible extends RuntimeException with NoStackTrace | ||
|
|
||
| implicit val stamper = new CacheKeysStamper { | ||
| override protected def stamp: Arg.CacheKey => Unit = { |
There was a problem hiding this comment.
The overall stamp for cacheKeyArgs is the concatenation of each arg stamp, taken in the order they were set, which depends on the flow of the scalafix InputTask implementation (so relatively stable). As we control this order (as opposed to the user, which can only affect the order of the ParsedArgs), it's not necessary to append the type/name of each arg to disambuiguate 2 args that would have the same value, as they cannot be in the same position across 2 invocations.
|
|
||
| case object NoCache extends Arg with CacheKey { | ||
| override def apply(sa: ScalafixArguments): ScalafixArguments = | ||
| sa // caching is currently implemented in sbt-scalafix itself |
There was a problem hiding this comment.
I am assuming --no-cache could become a scalafix-cli argument in the future if/when caching is implemented upstream.
bjaglin
force-pushed
the
caching
branch
3 times, most recently
from
175da1d to
7514078
Compare
|
Rebased against #103 |
bjaglin
force-pushed
the
caching
branch
2 times, most recently
from
84269dd to
d497fc2
Compare
|
Rebased against #100 |
The cache will be invalidated even though none of the rules locally defined is used, as finding the mapping between rules and class files would be very complicated.
bjaglin
changed the title
|
Merging, as discussed in https://gitter.im/scalacenter/scalafix?at=5ec3f53d8fe27138ba6e01de |
See https://gitter.im/scalacenter/scalafix?at=5ea5f83aa634f42f7a0524a8.
It took much more effort than expected, mostly due to the compat issues between sbt versions. I was about to give up and say that caching would be effective in sbt 1.x only, but I think I found a decent way to keep the codebase as common as possible.
As classpath-only changes (i.e. no source file updated) do not invalidate the cache and the implementation is probably rough on the edges anyway (false positives?), I was thinking about leaving this opt-in & advertise it as "experimental" for now.
There will most likely be upcoming Scalafix features that trigger false positive cache hits (any new flag/syntax that references a local/remote resource won't be stamped correctly), which can be a problem if the sbt plugin version evolves independently of the Scalafix version. I guess the right thing to do would be to implement caching in Scalafix itself in the long run.