ExplicitNonNullaryApply is slow and may causes OutOfMemoryError

[exoego]

I am using scala-rewrites 0.1.1 with Scala 2.13, sbt 1.3.13, sbt-scalafix 0.9.18.
My SBT_OPTS is -Xms512M -Xmx8G -Xss2M -Dfile.encoding=UTF-8 -XX:MaxMetaspaceSize=1024M -XX:+CMSClassUnloadingEnabled

To address new warnings on Scala 2.13.3, I am trying to usefix.scala213.ExplicitNonNullaryApply on my codebases.
And I thought ExplicitNonNullaryApply is very slow:

sbt:myProject> commonJVM/scalafixAll fix.scala213.ExplicitNonNullaryApply
[info] Compiling 59 Scala sources to /home/vagrant/IdeaProjects/myProject/common/jvm/target/scala-2.13/classes ...
[info] Running scalafix on 59 Scala sources
[info] Compiling 17 Scala sources to /home/vagrant/IdeaProjects/myProject/common/jvm/target/scala-2.13/test-classes ...
[info] Running scalafix on 17 Scala sources
[success] Total time: 349 s (05:49), completed 2020/07/06 9:15:31
sbt:myProject> commonJVM/scalafixAll fix.scala213.ExplicitNonNullaryApply
[info] Running scalafix on 59 Scala sources
[info] Running scalafix on 17 Scala sources
[success] Total time: 302 s (05:02), completed 2020/07/06 9:20:38
sbt:myProject> commonJVM/scalafixAll fix.scala213.ExplicitNonNullaryApply
[info] Running scalafix on 17 Scala sources
[info] Running scalafix on 59 Scala sources

Exception: java.lang.OutOfMemoryError thrown from the UncaughtExceptionHandler in thread "classloader-cache-cleanup-0"

1st run took about 6 minutes, then 2nd run took 6 minutes, finally 3rd run causes OutOfMemoryError.
My commonJVM project has about 12,000 lines of Scala code.
My other projects has 100,000+ lines of Scala code, so I am afraid that running ExplicitNonNullaryApply on large Scala project is very time-consuming/not stable.

I prefer using scalafix with CI to enfoce coding standard to improve code quality, so it would be great if ExplicitNonNullaryApply become fast enough for daily usage.
Taking long on 1st run is accpetable, but I expect 2nd-or-later run should be fast since no code changes...

It is fine to close this issue as wontfix if ExplicitNonNullaryApply is intended for one-time migration, and not for daily use.
Migrating huge existing code base to prepare for Scala 3 is heavylifting, so even one-time usage is highly valuable.
After migratiion using one-time ExplicitNonNullaryApply, programmers can manually fix when they forgot to add () to new code, by following scala compiler warning.

[dwijnand]

Hi @exoego. Thanks for trying out ExplicitNonNullaryApply and for the feedback. Here are my thoughts:

finally 3rd run causes OutOfMemoryError

This could be an sbt issue, as I've been experiencing (sadly) frequent crashing in sbt with OOM recently... Though not on the 3rd execution of a task.

I expect 2nd-or-later run should be fast since no code changes...

This sounds like an sbt-scalafix issue, which should use the caching mechanisms in sbt to skip when sources haven't changed.

running ExplicitNonNullaryApply on large Scala project is very time-consuming/not stable.

The underlying issue here is that method overriding information isn't stored in the SemanticDB. It's succinctly described in this "Known limitations" point: https://scalacenter.github.io/scalafix/docs/developers/symbol-information.html#lookup-method-overrides. Scalafix's built-in ExplicitResultTypes rule also suffers from this problem, and the ExplicitNonNullaryApply rewrite here uses the same, problematic, workaround: spinning up the presentation compiler (PC) at runtime to re-compile the code so the rewrite rule can query the presentation compiler. That compilation takes time, the compiler doesn't stick around across multiple rewrites so it can be re-used, and that compilation isn't cached/incremental.

It is fine to close this issue as wontfix if ExplicitNonNullaryApply is intended for one-time migration, and not for daily use.

In my mind it was always meant to be a one-time migration, but obviously, with more code being introduced all the time to codebases, it would be great if it were runnable continuously. But none of the "caching the scalafixAll task", "keeping the presentation compiler around" or "making it incremental" improvements would help the CI use case - it's the recompilation that's the issue.

Given all the above, I'm disinclined to working around the upstream issues in scala-rewrites in favour of closing this as as "wont-fix" or "upstream-issue". But I'll mull over it a bit and ask my peers, first.

If you (or anyone) have any further thoughts on this, more feedback welcome.

[exoego]

Thanks for quick and detailed consideration !!
I understand that this is an issue in sbt and sbt-scalafix.
I am fine to close this issue as wontfix or upstream issue.

[bjaglin]

This sounds like an sbt-scalafix issue, which should use the caching mechanisms in sbt to skip when sources haven't changed.

There is an opt-in scalafixCaching setting key. See also scalacenter/scalafix#1164.

[olafurpg]

@dwijnand can you elaborate on why the rule needs to re-typecheck the source file with the PC? The ExplicitResultTypes rule also uses the PC to query super methods but it runs fast.

[dwijnand]

To understand if a method is defined in Java, as those are exempt from needing parens, e.g. toString is fine, no need to rewrite to toString(), same with some POJO getFoo. In #48 I shuffled the order of the predicates, so it uses the PC as a last resort, which probably helps. I might also front the PC usage with known cases, like Any#toString. But, overall, I've found ExplicitNonNullaryApply to be fast too, can you spot any difference between the two which might explain why mine might be slower?

https://github.com/scala/scala-rewrites/blob/ec6df524b7bcf6030f3c27908f3f0deaaa899270/rewrites/src/main/scala/fix/scala213/ExplicitNonNullaryApply.scala

[olafurpg]

@dwijnand At a quick look, I suspect the overhead comes from this method call to typedTreeAt for every method call https://github.com/scala/scala-rewrites/blob/ec6df524b7bcf6030f3c27908f3f0deaaa899270/rewrites/src/main/scala/impl/Power.scala#L27

Instead, you can use the Scalafix Tree.symbol API to get the SemanticDB symbol and then convert that into a PC symbol using inverseSemanticdbSymbols from here https://github.com/scalacenter/scalafix/blob/b8667ef7e1e2fa81251765d46547d585564ebb0a/scalafix-rules/src/main/scala/scala/meta/internal/pc/ScalafixGlobal.scala#L60

[bjaglin]

Not sure if it's related, but I have identified a performance regression introduced in sbt-scalafix 0.9.18 related to classloading external rules (each project/configuration initializes a separate classloader...). Working on a fix.

[dwijnand]

Instead

Wonderful! Thanks, Olaf. #50