Optimizations in escape hatch #898
Conversation
|
|
||
| if (hasDisabledPatch) EmptyPatch | ||
| else loop(name, underlying) |
There was a problem hiding this comment.
it shouldn't be necessary to check whether the underlying patches are disabled if it was already verified that the parent (AtomicPatch) contains a disabled patch. This should prevent some expensive tree traversals
| } | ||
| private object SuppressWarningsArgs { | ||
| def unapply(mods: List[Mod]): Option[List[Term]] = | ||
| mods.collectFirst { |
There was a problem hiding this comment.
no need to traverse all elements since the annotation can only appear once
marcelocenerine
force-pushed
the
optimized_escape_hatch
branch
from
c4486ce to
4ba47b6
Compare
There was a problem hiding this comment.
Great work @marcelocenerine this is a big contribution to Scalafix that I am sure many users with large codebases will love.
I have only one blocking comment that is unrelated to the optimizations: I would prefer to hardcode SuppressWarnings over classOf[SuppressWarnings].getSimpleName. There are cases when runtime reflection is warranted but I don't think it's necessary in this case.
| def apply(tree: Tree, associatedComments: AssociatedComments): EscapeHatch = | ||
| def apply( | ||
| input: Input, | ||
| tree: => Tree, |
There was a problem hiding this comment.
Note: my experience with by-name is that it's easy to accidentally trigger the value. You can also use () => Tree or the LazyValue[T] in https://github.com/scalacenter/scalafix/blob/18650f572178ab1fa461846604bd82f9fed996ac/scalafix-core/src/main/scala/scalafix/internal/v1/LazyValue.scala which has the benefit of being a normal type and avoids accidental double computations. I prefer explicit tree.value or tree() to make it clearer when the function is called.
| @@ -180,17 +184,22 @@ object EscapeHatch { | |||
| } | |||
|
|
|||
| private object AnnotatedEscapes { | |||
| private val SuppressWarnings = "SuppressWarnings" | |||
| private val SuppressWarnings = classOf[SuppressWarnings].getSimpleName | |||
There was a problem hiding this comment.
Is this necessary? I would avoid runtime reflection when possible even if that means hardcoding a string literal, the name is not going to change in the JDK anytime soon anyways.
There was a problem hiding this comment.
fair enough. Reverted
| import scala.meta.contrib.AssociatedComments | ||
| import scala.meta.inputs.Input | ||
|
|
||
| class EscapeHatchSuite extends FunSuite { |
| test("Patch.empty") { | ||
| val empty = Patch.empty | ||
| assert(empty.isEmpty) | ||
| assert(empty.atomic.isEmpty) |
scalafix-core/src/main/scala/scalafix/internal/patch/EscapeHatch.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
LGTM 👍 Fantastic work @marcelocenerine Combined with scalameta/scalameta#1793 I am excited to try out the new speedups 😁
|
We can release scalafix v0.9.1 together with scalameta v4.1.0 |
This PR implements some optimizations in
EscapeHatch(see details in #894):Tree.EscapeHatchis short circuitedThe tables below compare the running time of what is on
mastervs. this PR + scalameta/scalameta#1793. These figures are the median of 5 consecutive runs of Scalafix against the Spark code base (3201 files; 103181 lines). The setup is the same as explained here:EscapeHatch(total):ok:on|:off@filterEscapeHatch(total):ok:on|:off@filterI believe that most projects using Scalafix don't use suppressions often (only exceptionally as it should be). For those, the cost of
EscapeHatchis now zero. Any source file containing suppressions only pay the price of the chosen suppression method.As highlighted here, other parts of Scalafix got faster due to the improved tree traverser currently under review in Scalameta. Rules will also benefit from that.