Incremental Java compilation on public constant change

[asodja]

Signed-off-by: Anze Sodja anze.36@gmail.com

Fixes #13493
Fixes #6482

Context

Currently on any change for class with public constant Gradle does full recompilation for Java. This makes incremental compilation with Gradle very tricky, since you have to follow some rules: like no public constant. Even worse, in some cases that is not possible (constants used on annotations).

This PR takes in to account constant changes for incremental builds so full rebuild is not necessary.
It's using Compiler api plugin, similar as class file to name mapping. Currently only constant origins are tracked.
So change to any constant will affect also others. To track all constants separately is possible, but it's expensive, so it might require some tricks. So I did not implement it in this PR.

Slack discussion:
https://gradle-community.slack.com/archives/CA7UM03V3/p1614676803095900?thread_ts=1614009425.009300&cid=CA7UM03V3

Contributor Checklist

• Review Contribution Guidelines
• Make sure that all commits are signed off to indicate that you agree to the terms of Developer Certificate of Origin.
• Make sure all contributed code can be distributed under the terms of the Apache License 2.0, e.g. the code was written by yourself or the original code is licensed under a license compatible to Apache License 2.0.
• Check "Allow edit from maintainers" option in pull request so that additional changes can be pushed by Gradle team
• Provide integration tests (under <subproject>/src/integTest) to verify changes from a user perspective
• Provide unit tests (under <subproject>/src/test) to verify logic
• Update User Guide, DSL Reference, and Javadoc for public-facing changes
• Ensure that tests pass sanity check: ./gradlew sanityCheck (some jmh task fails with --release flag not found, I don't think this is related to my change)
• Ensure that tests pass locally: ./gradlew <changed-subproject>:quickTest

Gradle Core Team Checklist

• Verify design and implementation
• Verify test coverage and CI build status
• Verify documentation
• Recognize contributor in release notes

[asodja]

I know this is a big change, so I expect few iterations to make this in to Gradle. But I believe this will benefit Gradle and it's users.

I probably missed some cases when I was writing tests, or there are parts that are not done by Gradle conventions or can be done better so I am open for any comment. Also currently this feature is automatically enabled, maybe it deserves some flag to make it enabled/disabled.

Some more info:
Algorithm:

• analyse classes with Compiler API, find all constants for visited classes
• write all constants to file (similar as does file to class mapping): this file is "constant dependent => setof(constant origins)" mapping
• when saving incremental results read constants file and merge it with previous constants results from cache and save that to incremental result (IncrementalResultStoringCompiler.storeResult)
  In cache we actually save "constant origin hash => set(constant dependents)". It's not actually a Map<Integer, Set> but there is a special structure ConstantToClassMapping that should be more memory efficent.

On next run:

• read constants mapping from cache
• for every class read "constants dependents" in ClassSetAnalysis.java and add them for recompilation
• do all steps from before again

Main changes:
ConstantsCollector.java
ConstantsTreeVisitor.java
IncrementalResultStoringCompiler.java
ClassSetAnalysis.java

Also is there a reason that source-class mapping is not in cache and it's written to file? This breaks build-cache for compileJava task.

[oehme]

I've had a quick look at this and the changes make sense to me. It would be great to test this on some real world scenarios. For example, what about Android builds where the R class contains a bunch of constants. Does this change help when R changes?

[ljacomet]

@asodja Thank you for this PR. The Gradle team is busy wrapping up the Gradle 7.0 release. This will not get attention until that release is out.

[asodja]

Thanks for letting me know. No problem

[asodja]

As warned by @oehme there are conflicts with #16536. I will try to resolve conflicts in next days

[melix]

@asodja we have merged significant changes to the incremental Java compiler from @oehme . Would you be fine with rebasing your PR on top of those changes? If not I can take a stab on it, just let me know!

[oehme]

@melix I think you missed his previous message, he's already looking into it :) I'd be happy to help too, if needed. It was my changes that caused this mess after all :)

[melix]

Indeed, after refreshing the page they showed up 🤦 Sorry for the noise D

[asodja]

So I rebased it to latest master, it wasn't that bad in the end. But it still needs some work, some Groovy tests fail and previous compilation data is read twice now since that logic was changed a bit. And I just did a quick dirty fix for that so I could run tests.

I will fix it in next days.

[asodja]

Hey @melix, this is now ready for a review.

One note: I also did an optimization that adds a bit of complexity but it's imo worth it: for every constant I find "accessible" and "private" dependents. So accessible dependents are dependents that have accessible constant calculated from constant, e.g.:

// A is an "accessible" dependent
class A {
    static final int CALCULATE_CONSTANT = CONSTANT; // or CONSTANT + 1, or CONSTANT * CONSTANT etc.
}

And "private" dependents are all other cases. When collecting classes to compile I then check accessible dependents transitively, but for private dependents transitive check is not needed.

I also did a simple optimization: constants that are calculated from method call, result in "private" dependent, e.g.:

// A is a "private" dependent
class A {
    static final int CALCULATE_CONSTANT = methodCall(CONSTANT); // But not methodCall(CONSTANT) + 1 etc. since I would have to check whole expression tree
}

Let me know if this analysis can produce an incorrect compilation result.

[melix]

Thanks, let me take a look at this!

[blindpirate]

Oh it needs to rebase master to pick up the changes to native tests to be able to run on new Ubuntu...

would it be possible to avoid this JSON?

It's part of the sanityCheck, so you can exclude that, but that makes it more confusing...

[melix]

@blindpirate please avoid rebasing branches when we are collaborating on a branch. Typically I thought I had to merge master into this branch, which I did, but then I realized I couldn't push anymore because you had force pushed this branch. No big deal, but it's easier to rebase only if you're the only collaborator, otherwise just use regular merges.

[wolfs]

The current failure indicates that we can't generate the TC DSL because of the removal of all the performance tests from .teamcity/performance-tests-ci.json. I don't know why that happens exactly. You can reproduce the problem by running mvn clean teamcity-configs:generate in the .teamcity directory.

It would probably be best of @blindpirate took a look at this failure and try to fix it. Or you publish a snapshot from this branch and rebaseline the performance tests on that snapshot before merging.

[blindpirate]

@melix sorry for that - I knew I shouldn't but I usually work on my own branch and I was so accustomed to it. Will never happen again. I'm looking into the failures - also I think there might be some corner cases for the bot to handle contributor PRs. If you think this is good to go, I can do my best to make everything pass.

[melix]

@blindpirate I'm taking care of this, I'll ping you tomorrow if I need help, get rest, thanks!

[melix]

@bot-gradle test RFN

[bot-gradle]

OK, I've already triggered ReadyForNightly build for you.

[melix]

@bot-gradle test RFM

[bot-gradle]

OK, I've already triggered ReadyForMerge build for you.

[melix]

@asodja Finally got this merged, thanks a lot for your efforts!

[asodja]

Thank you for the help and accepting PR!