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

Make incremental compile efficient in the presence of Annotation Processors #1320

Closed
eriwen opened this issue Feb 3, 2017 · 76 comments
Closed
Assignees
Labels
a:epic a:feature A new functionality

Comments

@eriwen
Copy link
Contributor

eriwen commented Feb 3, 2017

Migrated from GRADLE-3259 which had 19 votes

Compile avoidance was introduced in Gradle 3.4 (<= Users please read this). One can improve performance somewhat by separate annotation processors from the compile classpath.

Expected Behavior

Incremental Java compilation should avoid full rebuild when annotation processors are present.

Current Behavior

Currently we fall back to a full recompile when annotation processors are used. The reason for this is that we don't know which source files they need as an input. The APT API allows processor to access other source files then the one containing the annotation being processed.

Context

We could instrument the compiler to find out which files the annotation processors are actually reading and use this information to run them incrementally as well.

In cases where an annotation processor is taking the whole source tree as an input, we could warn the user (e.g. in build scans) that this is causing slower builds. Many developers might not even be aware of the trade-off between "saving time writing code" and "saving time running the build" they have to make.

Design Spec

Since this is a bigger feature, you can download the design spec as a PDF.

@eriwen eriwen added the a:feature A new functionality label Feb 3, 2017
@stephanenicolas
Copy link

Great news !

Instead of instrumenting the compiler, it would be possible also to apply the same discovery mechanism for annotation processors as APT and ask all processors which annotations they process, and find all files containing these annotations.

It could also be interesting to let the javac maintainers know about this work when it's done as it should lead to an enhancement of the APT APIs / JSR. They could also probably help to instrument the compiler if that's the way to go.

@adammurdoch
Copy link
Member

Yes, this is what "instrumenting the compiler" means.

@stephanenicolas
Copy link

stephanenicolas commented Feb 5, 2017 via email

@dongshengbc
Copy link

The link to "introduced in Gradle 3.4" does not work?

@eriwen
Copy link
Contributor Author

eriwen commented Mar 13, 2017

@dongshengbc Fixed it. Sorry I had linked to the release candidate before.

@stephanenicolas
Copy link

@eriwen , we are really very motivated to get this solved. Can we help in anyway ?

@eriwen
Copy link
Contributor Author

eriwen commented Apr 18, 2017

@stephanenicolas Thanks for volunteering. I think there are 4 potential ways to help:

  • Technical design or implementation of specific items (delegating to @oehme on this)
  • Provide really great, generic test cases so we can more easily determine effectiveness of work
  • Try out anything currently under development, providing build/technology details (I'm not keen on the plan, but Stefan may be)
  • Consider sponsoring some of the features through Groupon, so that we can afford to offload non-critical work to others (or new engineers) so our core engineers can focus on critical work like this. Let me know if Groupon is interested in this and I'll put you in touch with the right people.

@oehme leads the team that would be responsible for addressing this. Anything technically they (Groupon team) can take on here?

@stephanenicolas
Copy link

I will come back to you on this. If you could have a more detailed list of areas with which we can help, that would be great.

@stephanenicolas
Copy link

@oehme @eriwen is there a way we can get more directly in touch with you and work together on this. We are switching to gradle 4 and the new android plugin and this issue is really our last bottle neck to get decent build times. If you wanna write an email to me, my personal email address is pretty easy to find on the web..

@meierjan
Copy link

Any progress? I agree with/can just quote @stephanenicolas from the old issue tracker:

This is probably the most fundamental issue in Android builds that impacts developers productivity in our team, and actually probably the main motivation for us to soon consider moving to another build system. That's quite a pity !
FYI : https://code.google.com/p/android/issues/detail?id=200043

I wonder why this is not having a much higher priority because I think this is an issue for many (many) developer - or am I forgetting something?

@oehme
Copy link
Contributor

oehme commented Sep 14, 2017

We are working together with the Google and Groupon teams, but the work is still at an early stage. We plan to have a first milestone for simple processors like AutoValue and Butterknife late October.

I wonder why this is not having a much higher priority

There are only so many things we can tackle at once. I understand that this is probably the biggest issue for you. But I hope you understand that there are other big issues for other users.

@jaredsburrows
Copy link

@oehme @eriwen What is the status of this? Adding support in Gradle for Android builds would greatly reduce build times since a lot of projects use libraries such as Glide and Dagger 2.

@lukyer
Copy link

lukyer commented Nov 20, 2017

If anyone interested in how to achieve full gradle power (e.g. incremental builds) with annotation processors, see my workaround solution here https://stackoverflow.com/questions/47289457/how-to-configure-gradles-incremental-build-with-annotation-processor/47392788#47392788

@oehme
Copy link
Contributor

oehme commented Nov 20, 2017

@jaredsburrows see #3434 for the current status. Dagger 2 will be complicated though, because even if just one class changed, it would still need to do some heavy analysis to make sure all injection points are still valid.

@oehme
Copy link
Contributor

oehme commented Jan 24, 2018

I've broken this epic down into a few initial stories. You can take a look at the design spec to get a better idea of what use cases we plan to support and what is out of scope (for now).

@fab1an
Copy link

fab1an commented Jan 24, 2018

A good solution. Only one question:

When a processor does not opt-in (no properties file provided), we do full recompiles as
before

Why not fail the compilation until incremental is switched off here as well?
I think most people never realise their build is slow for a reason, which destroys valuable development time all over the world.

@oehme
Copy link
Contributor

oehme commented Jan 24, 2018

For backwards compatibility reasons (many users set incremental=true globally and rely on Gradle to figure out whether it can actually be incremental or not). Also we want to remove the incremental flag altogether in the mid term, making incremental the default and only mode. We'll likely add incremental compile insights to build scans by that time, so they can warn you about this more prominently than the info log can.

@pbi-qfs
Copy link

pbi-qfs commented Jan 24, 2018

The approach sounds reasonable and well thought out. I would agree with @fab1an to fail fast instead of silent full recompile, that would be more consistent.

In addition, it would be handy if the build composer would have an option to provide the processor type information in the build file, if none is provided by the processor author, e.g. in the configurations section.

@oehme
Copy link
Contributor

oehme commented Jan 24, 2018

In addition, it would be handy if the build composer would have an option to provide the processor type information in the build file, if none is provided by the processor author, e.g. in the configurations section.

I thought about this too, but I'm not sure in how many cases it would work, since the processor also needs to use the originatingElements API correctly.

@pbi-qfs
Copy link

pbi-qfs commented Jan 24, 2018

I thought about this too, but I'm not sure in how many cases it would work, since the processor also needs to use the originatingElements API correctly.

In the most simplest case, a processor (like Lombok) only modifies the one class defined by the corresponding java, and might generate additional inner classes, which can easily be associated by the $-name pattern. Taking all $-classes into account is most probably overkill, but it is a good default if the processor doesn't know about the originatingElements API, yet - it's better than a full recompile.

@oehme
Copy link
Contributor

oehme commented Jan 24, 2018

APT does not allow processors to change classes in-place. Lombok is operating outside the APT specification and uses internal APIs. I would not consider it a "simple case". There is also no way to generated inner classes. You might use a dollar sign as a separator, but that won't be considered an inner class by the compiler (and it won't have access to the internals of the outer class).

For any generated file we will require originating elements to be provided, otherwise processing will fail. That's why I think the number of processors that use this API correctly and don't have an opt-in declaration will likely be small.

@pbi-qfs
Copy link

pbi-qfs commented Jan 24, 2018

For any generated file we will require originating elements to be provided, otherwise processing will fail. That's why I think the number of processors that use this API correctly and don't have an opt-in declaration will likely be small.

I totally agree, therefore I'm looking for a way to "approximate" a solution better than a full recompile for processors which do not know about the Gradle API, yet. Such an approximation would be the association of dollar-classes with their "main" class.

@oehme
Copy link
Contributor

oehme commented Jan 24, 2018

Most processors don't use a dollar sign (some use underscores, some just use camel case) and I really don't want to put such assumptions in, as correctness is the most important concern for Gradle. Let's first see if convincing processor authors to opt in is a problem at all. If it isn't then we've saved a bunch of time. If it is, we can start thinking about alternatives.

@oehme
Copy link
Contributor

oehme commented Apr 3, 2018

@filiphr Your understanding of isolating/aggregating is correct. The working of the incremental compiler is explained in its own chapter right above the one about incremental processing, so I didn't repeat it there. In short, any change to any dependency of a class will trigger its recompilation/reprocessing.

@Alexander-- is right about resource generation - We don't support it at the moment and Gradle will throw an error if it is attempted. However, I think we can make an easy improvement here: Instead of failing, we could do a full rebuild if a resource is generated. You said it is rarely used, so most users would benefit from incremental processing in that case.

There is also no deeper reason for forbidding resource generation, it's just another thing to implement. If your processor uses the Filer API for generating resources, it would be easy to support that. See #4702.

@filiphr
Copy link

filiphr commented Apr 3, 2018

Thanks for clearing it up @oehme. To be honest I read only the part about the annotation processor (didn't read the previous chapter). You don't have to repeat, if you just reference the part that it is explained it would be much easier for annotation processor writers 😄.

Yes in our case I would say that it is used in max 1% of the cases. As it requires multiple conditions to be fulfilled and requires actions from the user. So out of the box it can never happen. It's an opt in feature. So if you can do the improvement and not error out it would be great.

Yes we use the Filer API to generate the resource. Although, the resource name might change if the user does some changes in the annotation definition. I am going to watch that issue as well.

I just saw that we are not passing the originating elements to the Filer API, so we need to do that enhancement on our side.

@oehme oehme modified the milestones: 4.8, 4.8 RC1 Apr 5, 2018
@wrotte
Copy link

wrotte commented Apr 5, 2018

Is it possible to force gradle to avoid a full rebuild even when annotation processors are present, even if they don't/haven''t implemented this? For example, in a CI pipeline, I don't want to rebuild modules that have such annotation processors in every stage, but I'm seeing that behavior.

@oehme
Copy link
Contributor

oehme commented Apr 6, 2018

@wrotte I'm afraid I don't understand your question. This is only about incremental compilation. It doesn't affect up-to-date checking or caching. Can you elaborate on your setup a bit more?

@wrotte
Copy link

wrotte commented Apr 26, 2018

@oehme Sorry for the late reply; I've been trying to distill a succinct test case and I think I've found that the latest Android plugin solves the issue I'm seeing. If I can reproduce I'll file a new issue.

@oehme
Copy link
Contributor

oehme commented May 4, 2018

Good news everyone, the upcoming Gradle 4.8 will contain several big improvements to incremental annotation processing:

  • Processors will be able to decide dynamically if they are incremental or not. This was brought up by the AutoValue team, because they have an extension mechanism and so they first need to check all extensions for incrementality before they can say "Yes, this processor is incremental". This new mechanism will supersede the META-INF registration.
  • Compilation will no longer fail when a processor does something that we know won't work incrementally. Instead Gradle will simply do a full recompile and inform the user about the problem.
  • Unused non-incremental processors will no longer prevent incremental compilation. This means that incremental and non-incremental processors can be safely packaged together. As long as the user doesn't use the non-incremental processor in their code, everything will remain incremental.

@oehme
Copy link
Contributor

oehme commented May 5, 2018

I've compiled a list of popular processors and their current state of adoption. Please express your support by giving 👍 on the issue trackers of the ones you care about. Also feel free to suggest others that we should get in touch with.

@tprochazka
Copy link

Is there any way how to get to know which plugin causing not using incremental compilation?

I see just this when I run build with --info

Incremental Java compilation disabled in variant defaultCcaDebug as you are using an incompatible plugin

@oehme
Copy link
Contributor

oehme commented May 29, 2018

That's not coming from us, but probably from the Android plugin. @ducrohet can you comment on this? It would be great if the incremental compile decision was completely left to Gradle. Is there any adverse condition we are not detecting?

@ducrohet
Copy link
Contributor

For some reason our plugin output this when you apply the retrolambda plugin but I'm not sure why. This was done a while back (2 years) and may not be required anymore. I'll investigate.

@ducrohet
Copy link
Contributor

It looks like previously this disabled inc java on both android-apt and retrolambda. Probably a remnant of a time when the java compile task didn't automatically disable inc compilation if annotation processor was one.

@tprochazka
Copy link

tprochazka commented May 29, 2018

I'm definitely not using retrolamba or android-apt plugin. I can't guarantee that it was not used in any library used by me, but it should not affect it, right?
I tested it with android build plugin 3.1.2. and 3.2.0-alpha15 and it happens with both. I switched butterknige and Android-state to a version which should support incremental compilation.

@ducrohet
Copy link
Contributor

yes, sorry, this is triggered with a few more cases than just retrolambda.
The test is: data binding enabled OR annotation processor OR retrolambda.

This still seems a bit strange to me. I'll review this.

@tprochazka
Copy link

Thanks, so data binding is the reason. Somebody on Google I/O told this year, that one of the news in new building tools is incremental support. So would be great if also java compilation will be part of this, especially now when Gradle is able to do it also with annotation processors.

@tprochazka
Copy link

I created an issue for "Incremental Java compilation disabled in variant ... as you are using an incompatible plugin" here https://issuetracker.google.com/issues/110061530 to not spam here about the issue which is not related to Gradle itself.

@oehme
Copy link
Contributor

oehme commented Jul 30, 2018

I'm closing this issue since the work on the Gradle side is done and lots of annotation processors can now be incremental. Please have a look at the list of processors we know about and their adoption status. Any kind of support from your side will help. You could

  • tell the processor author how much full recompiles are affecting your daily work
  • point them to the documentation and help them understand the contraints of incremental processing
  • send them a PR
  • help them with exploratory testing to ensure their processor behaves as expected

I'll be happy to help too, just mention me in a conversation if clarification is needed.

@oehme oehme closed this as completed Jul 30, 2018
@stephanenicolas
Copy link

@oehme what kotlin incremental annotation processing ? Do you know if it is also supported ?
If not, is there a plan / a ticket to solve it ?

@oehme
Copy link
Contributor

oehme commented Jan 14, 2019

They're currently working on making KAPT compatible.

@stephanenicolas
Copy link

Is there any link to their effort ? I actually never had any contact with people from Jetbrain..

@oehme
Copy link
Contributor

oehme commented Jan 14, 2019

I haven't found a public issue, feel free to create one on Youtrack.

@TWiStErRob
Copy link
Contributor

@oehme this one? https://youtrack.jetbrains.com/issue/KT-23880

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:epic a:feature A new functionality
Projects
None yet
Development

No branches or pull requests