Make Scala compilation 26% faster #97
Conversation
jadenPete
force-pushed
the
jpeterson-zincrunner-performance-improvements
branch
from
9b381ac to
531e308
Compare
| val classloader = ClasspathUtil.toLoader(outputDirectory +: classpath) | ||
|
|
||
| /** | ||
| * TODO: Make this more similar to the `readAPI` defined in [[sbt.internal.inc.javac.AnalyzingJavaCompiler]]. |
There was a problem hiding this comment.
Is this TODO a blocker for our use cases?
There was a problem hiding this comment.
I don't think so. If it were, we'd see problems with test discovery in mixed-compilation targets (i.e. targets with Scala and Java code), but we don't have any of those in our monorepo because all of our tests are written in Scala. I added this comment because I think it's something we should figure out before we bring our workers into the mainline ruleset.
There was a problem hiding this comment.
Do you think we could ask to see if the functions you're using could be moved to a Util class instead of private in an internal package?
There was a problem hiding this comment.
We could, but I don't think the effort would be worthwhile, especially considering that the Scala maintainers have expressed in the past that Zinc/compiler bridge stuff isn't meant to be a public interface.
If we're ever using the compiler bridge in the mainline ruleset, I think it'd be good to start up conversations with the Scala maintainers about designing a build system-agnostic, public interface to the Scala compilers. If such an interface existed, this file wouldn't need to exist.
src/main/scala/higherkindness/rules_scala/common/sbt-testing/fingerprints.scala
Show resolved
Hide resolved
src/main/scala/higherkindness/rules_scala/common/sbt-testing/fingerprints.scala
Outdated
Show resolved
Hide resolved
src/main/scala/higherkindness/rules_scala/workers/zinc/test/TestRunner.scala
Outdated
Show resolved
Hide resolved
tests/test-frameworks/mixed/BUILD
Outdated
| @@ -3,15 +3,17 @@ load("@rules_scala_annex//rules:scala.bzl", "scala_test") | |||
| scala_test( | |||
| name = "mixed_2_13", | |||
| srcs = glob(["*.scala"]), | |||
| deps_used_whitelist = [ | |||
There was a problem hiding this comment.
Could you add a comment explaining this change? Might be worth a TODO to see if we can get the deps used to not mark these things required for test discovery as unused.
There was a problem hiding this comment.
Are these not runtime deps? 🤔 If we don't depend on them but we need them it seems like they're runtime deps
There was a problem hiding this comment.
They aren't, no. These tests aren't needed at runtime; they're needed for test discovery. The reason they have to be in deps now is because test discovery now happens at compile-time, not at runtime. I haven't updated the deps checker to mark these as used because in most cases, the target containing the implementation of the SBT test interface is the same one that the target would've depended on anyway. That's why none of the other external dependencies are in this list.
I can fix this, though. One important thing to note is that this will break a property of our dependency checker that's been thus-far guaranteed: that it only allows a dependency to be in deps or not in deps. When we do test discovery, we go down a big list of test "frameworks". If we can find the test framework in the classpath, we look for tests that use that framework; otherwise, we don't. This means that, for example, if specs2 isn't on the classpath, but the target contains specs2 tests, the tests will silently fail to execute.
I think this is fine and unavoidable, but you disagree, I think we should leave this as-is.
tests/test-frameworks/mixed/BUILD
Outdated
| @@ -3,15 +3,17 @@ load("@rules_scala_annex//rules:scala.bzl", "scala_test") | |||
| scala_test( | |||
| name = "mixed_2_13", | |||
| srcs = glob(["*.scala"]), | |||
| deps_used_whitelist = [ | |||
There was a problem hiding this comment.
Are these not runtime deps? 🤔 If we don't depend on them but we need them it seems like they're runtime deps
jadenPete
force-pushed
the
jpeterson-zincrunner-performance-improvements
branch
from
a030609 to
8c13f7a
Compare
Basic benchmarking shows that we're incurring a ~49% overhead from using Zinc instead of the Scala compiler directly. That is, we wrote a basic worker that invokes the Scala 2 compiler programmatically via `scala.tools.nsc.Main`, and found that it was ~49% faster across-the-board. This doesn't necessarily mean that we can speed up `ZincRunner` by 49%. A lot of that time is spent in compilation phases [added](https://github.com/scala/scala/blob/2.13.x/src/sbt-bridge/scala/tools/xsbt/CallbackGlobal.scala#L166) by the compiler bridge (implementation of the compiler interface) itself. There are three of these phases; they're responsible for: 1. Mapping generated `.class` files to sources 2. Analyzing depedendencies between code 3. Producing the analysis information outputted by Zinc We rely on 2 and 3 for: - Dependency checking - Detecting main classes - Test discovery We only use 2 and 3, and although it's likely the phases could be slimmed down to only extract the information we need, implementing them ourselves would add a lot of complexity to `ZincRunner` and make it less portable . There's also no guarantee that we could do a better job. What can be eliminated is all the overhead related to incremental compilation. We haven't used incremental compilation for years and have no intention of re-enabling it. This commit isn't about ripping out Zinc entirely; we still use Zinc for the following things: - Classloading the compiler interface from the compiler classpath - Implementations of various `xsbti.*` traits and interfaces
jadenPete
force-pushed
the
jpeterson-zincrunner-performance-improvements
branch
from
8c13f7a to
078f10c
Compare
Before, we did test discovery at runtime, but we now do it at compile-time, for speed and to avoid having to serialize the analysis information returned by the compiler bridge. This commits marks targets used during test discovery as used, thereby subjecting them to depednency checking. In most cases, this won't make a difference as the target containing the implementation of the SBT test interface is the same one that the target being built would've depended on anyway. One important thing to note is that this will break a property of our dependency checker that's been thus-far guaranteed: that it only allows a dependency to be in `deps` or not in `deps`. When we do test discovery, we iterate through a list of test "frameworks". If we find a test framework in the classpath, we look for tests that use that framework; otherwise, we don't. This means that, for example, if specs2 isn't on the classpath, but the target contains specs2 tests, the tests will silently fail to execute. This means that now, having that a dependency in `deps` or not in `deps` are sometimes both valid states to be in. Prior to this change, that wasn't the case; there was a single, correct list of dependencies.
jadenPete
force-pushed
the
jpeterson-zincrunner-performance-improvements
branch
from
d5aa4f0 to
78331d3
Compare
This PR speeds up Scala compilation with this ruleset by ~26% across-the-board and substantially reduces the complexity of the ruleset by using the compiler bridge directly instead of Zinc. Note that all the goodies we get from Zinc and its analysis information are not exclusive to Zinc and are instead gotten by using the compiler bridge, which Zinc invokes under the hood. However, this PR does drop support for incremental compilation.
Please read the commit messages (especially the second commit description) for more information on the technical context surrounding this decision.