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

Delay Java compilation during pipelining #843

Merged
merged 7 commits into from
Jul 17, 2020
Merged

Conversation

eed3si9n
Copy link
Member

@eed3si9n eed3si9n commented Jul 16, 2020

This is a follow-up to #744. (This actually makes pipelining usable)

pipelining

This implements an Analysis-aware compileAllJava(...).

def compileAllJava(in: Inputs, logger: Logger): CompileResult = ...

How this is realized is more complicated than one might expect because we need all the wiring to get AnalysisCallback going to reuse the Incremental + MixedAnalyzingCompiler harness (This is so the downstream subprojects can get the incremental goodness).

Note: If incOptions pipelining is enabled, the build tool must call compileAllJava(in, logger) after both

  1. All upstream subproject Scala and Java compilation has completed, and
  2. Scala whole *.class compilation has completed && hasModified.

@typesafe-tools
Copy link

To validate this pull request against other sbt modules, please visit this link.

This implements an Analysis-aware `compileAllJava(...)`.

```scala
def compileAllJava(in: Inputs, logger: Logger): CompileResult = ...
```

How this is realized is more complicated than one might expect because we need all the wiring to get AnalysisCallback going to reuse the `Incremental` + `MixedAnalyzingCompiler` harness (This is so the *downstream* subproject can get the incremental goodness).
@eed3si9n eed3si9n changed the title compileAllJava Delay Java compilation during pipelining Jul 16, 2020
@eed3si9n eed3si9n marked this pull request as ready for review July 16, 2020 17:29
@eed3si9n eed3si9n requested a review from dwijnand July 16, 2020 17:36
Comment on lines 550 to 554
val f = for {
_ <- Future.traverse(dependsOnRef)(_.compile(i))
a <- futureAnalysis
pj <- wholeDeps
a0 <- futureScalaAnalysis
a <- futureJavaAnalysis(pj, a0)
} yield a
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the part that waits for upstream whole compilation and self Scala compilation for scripted tests.

Note this requires build tools to call `incrementalCompiler.compileAllJava(in, log)` at the right timing.
Comment on lines 184 to 188
if (config.incOptions.pipelining) {
compileScala()
if (scalaSrcs.nonEmpty) {
log.debug("done compiling Scala sources")
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Java compilation is turned off when incOptions.pipelining is turned on.

@eed3si9n
Copy link
Member Author

eed3si9n commented Jul 17, 2020

@dwijnand

But #843 with a javaOnly IncOption I think I'd be happy with.

IncrementalCompiler#compile(in, log) performs incremental compilation, and IncrementalCompiler#compileAllJava(in, log) performs non-incremental Java compilation and "analysis." Conceptually, we are splitting a subproject compilation into 2 parts (scalac and javac), and two form a logical compile with a complex timing.

Treating this as another IncrementalCompiler#compile(in, log) would not work because various pruning and invalidation logic would be different. In other words, they are not simply copy-pasted methods.

Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

I'm not a fan of incHandlerOpt.getOrElse(sys.error("incHandler was expected")), but it's a minor downside when compared to the rest of the change. Very nice work.

I just had a couple of easy questions/requests.

build.sbt Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants