Support incremental annotation processing #902

ZacSweers · 2020-01-01T02:23:31Z

Hello! I'm not actually terribly familiar with Epoxy, but figured I'd take a crack at this since it seems like one of the last major holdouts in the community for incremental annotation processing.

Notes:

It's isolating by default, but will be aggregating if KotlinModelBuilderExtension is enabled. I would recommend separating the kotlin model builder to a separate processor in a separate PR, as that would allow the others to remain isolating regardless.
- The isolating processors should maybe also be separated, as right now even one change to one isolating type will cause all of them to rerun.
Like Butter Knife, this uses a workaround for reflectively digging out the Trees API and will fall back to being a DYNAMIC processor if trees aren't available.
Updated KotlinPoet to the latest (1.4.4). Some time back we added back support for Filer after kapt added support for generating Kotlin sources via them. Note that this effectively requires Kotlin 1.3.50+.

I didn't understand this comment from the original issue

EpoxyController's @AutoModel is mostly isolated, but it is a bit of a hack because the model types it depends on aren't available until after annotation processing so it does some reflection to monitor that process

Can take a look if you can point me to the right place.

Resolves #423

This is borrowed from Butter Knife and supports kapt and gradle's incremental annotation processor

ZacSweers · 2020-01-01T14:09:47Z

We may want to shade the incap dependency since it's technically not stable.

elihart · 2020-01-03T23:55:27Z

This is great, thanks for working on it! I will be on leave for the next six weeks, but maybe @ngsilverman can work with you to get it merged in the mean time?

ColtonIdle · 2020-01-23T06:55:52Z

I'm in the middle of moving all of my epoxy models to a separate module in order to get the benefits of incremental kapt with epoxy. This PR makes me think that I should stop that effort and wait for this. Can anyone suggest whether or not this PR + modularizing (strictly putting my EpoxyModels into another module. Not modularizing anything else about my app) would save even more time for some reason? Or would this work basically negate the benefits of modularizing. See: #859

ngsilverman

Hi @ZacSweers, thanks for putting this up! Sorry it's taken me a while to get to it.

So this is my first time taking a good look at incremental annotation processing, and so I've left a few questions/comments for you.

I'm also curious as to what you mean by "The isolating processors should maybe also be separated, as right now even one change to one isolating type will cause all of them to rerun", in your PR description. I was under the impression that a change to an isolating type would cause the processor to rerun but only on that particular type. Is that incorrect?

Thanks again!

ngsilverman · 2020-01-28T00:30:46Z

epoxy-processor/src/main/java/com/airbnb/epoxy/ControllerProcessor.java

@@ -255,6 +255,8 @@ private void generateHelperClassForController(ControllerClassInfo controllerInfo
          .addMethod(buildSaveModelsForNextValidationMethod(controllerInfo));
    }

+    builder.addOriginatingElement(controllerInfo.getControllerClassElement());


In its current form ControllerProcessor relies on EpoxyProcessor passing it information about all the models (https://github.com/airbnb/epoxy/blob/master/epoxy-processor/src/main/java/com/airbnb/epoxy/ControllerProcessor.java#L91). Wouldn't this make it aggregating rather than isolating?

I don't think so because all the models are fields/properties of the same controller class right? It only cares about their declaration within the controller, not about any of their implementation details.

ngsilverman · 2020-01-28T00:34:04Z

epoxy-processor/src/main/java/com/airbnb/epoxy/KotlinModelBuilderExtensionWriter.kt

@@ -42,7 +30,7 @@ internal class KotlinModelBuilderExtensionWriter(
                )
            }
            .forEach {
-                it.writeTo(File(kaptGeneratedDirPath))
+                it.writeTo(processingEnv.filer)


ngsilverman · 2020-01-28T00:37:06Z

epoxy-processor/src/main/java/com/airbnb/epoxy/EpoxyProcessor.java

+      try {
+        // Get original ProcessingEnvironment from Gradle-wrapped one or KAPT-wrapped one.
+        for (Field field : processingEnv.getClass().getDeclaredFields()) {
+          if (field.getName().equals("delegate") || field.getName().equals("processingEnv")) {


Does delegate correspond to the Gradle wrapper and processingEnv the KAPT one? Would be nice to leave a comment.

Right, this isn't a public API but it's unlikely to change. Will add a comment

if there are any links you have to documentation or more information about these api's it'd be nice to leave them here

ngsilverman · 2020-01-28T00:39:09Z

epoxy-processor/src/main/java/com/airbnb/epoxy/EpoxyProcessor.java

+    options.add(PROCESSOR_OPTION_REQUIRE_ABSTRACT_MODELS);
+    options.add(PROCESSOR_OPTION_REQUIRE_HASHCODE);
+    options.add(PROCESSOR_OPTION_DISABLE_KOTLIN_EXTENSION_GENERATION);
+    if (trees != null) {


In the case where trees is null do we need to explicitly set the processor type as not incremental? Or is that the default (if so may still be worth a comment)?

Right, the default is DYNAMIC. That said, it may just be better to error here because I'm not sure epoxy can safely proceed without it?

ngsilverman · 2020-01-28T00:42:23Z

epoxy-processor/src/main/java/com/airbnb/epoxy/GeneratedModelWriter.kt

@@ -174,6 +174,7 @@ internal class GeneratedModelWriter(
            builderHooks?.beforeFinalBuild(this)

            addSuperinterface(modelInterfaceWriter.writeInterface(info, this.build().methodSpecs))
+            addOriginatingElement(info.superClassElement)


Shouldn't the originating element be the model class itself rather than its super?

Yes. I was thrown off by this as well, but superClassElement in this case is the original model class. The model, if I understood it correctly, is the superclass of the type this processor is generating.

ngsilverman · 2020-01-28T00:46:59Z

epoxy-processor/src/main/java/com/airbnb/epoxy/ModelBuilderInterfaceWriter.kt

@@ -46,7 +49,7 @@ internal class ModelBuilderInterfaceWriter(
                    // can generate the interface with the proper methods later
                    viewInterfacesToGenerate
                        .putOrMerge(
-                            it,
+                            ClassKey(it, modelInfo.superClassElement),


Same here, it's not clear to me why we're using the super class as the originating element. The Gradle documentation (https://docs.gradle.org/5.0/userguide/java_plugin.html#sec:incremental_annotation_processing) states:

When a source file is recompiled, Gradle will recompile all files generated from it. When a source file is deleted, the files generated from it are deleted.

And I believe in that context "source file" refers to elements marked as originating. So here we would want to recompile generated files when the model class is recompiled. It's super may or may not change in that case (but probably not).

See above comment 👍

I don't think what this ClassKey is representing is quite right. To clarify what this "view interfaces" code is doing, let's say we have ViewA and ViewB which both implement the same "Checkable" interface. The key to this map needs to be the "Checkable" interface element, and the map should store the set of methods that both ModelA and ModelB have in common (since we know they have a shared interface, this is a simplification we use).

By adding the model element to the map key we will now have separate entries for each view. I think this means that technically the interface generated by this (writeFilesForViewInterfaces) would be aggregating, since it is influenced by multiple view classes.

This complicates matters here unfortunately, and I don't see a good way to not have this be aggregating, since the generated interface will always depend on multiple models. For example, say we have an interface Header that has functions setTitle and setCaption. ViewA and ViewB both implement this interface, but only ViewA marks setTitle as a ModelProp, and only ViewB marks setCaption as a ModelProp. The resulting generated models don't actually share any methods in common from this interface, and the generated interface they share would actually be empty. If either model was changed to mark the other interface method as ModelProp then the generated interface would change to include it.

This example is contrived, but imagine an interface has many functions and views only want their models to contain a subset of those. The point of the model interface is to provide a generated interface to the user that represents the shared subset of model methods.

I don't see any good ways to avoid aggregation here, unless maybe we change it to be a processor error if multiple models with the same view interface don't have the same interface methods marked as ModelProps. It would be a breaking change, but I don't think it would affect many people. Any ideas?

ngsilverman · 2020-01-28T00:51:20Z

epoxy-processor/src/main/java/com/airbnb/epoxy/EpoxyProcessor.java

+    if (trees != null) {
+      IncrementalAnnotationProcessorType incapType = IncrementalAnnotationProcessorType.ISOLATING;
+      if (!configManager.disableKotlinExtensionGeneration()) {
+        incapType = IncrementalAnnotationProcessorType.AGGREGATING;


Why do Kotlin extensions make the processor aggregating? There's one extension file per model. Seems like an isolating use case to me.

This is because it's only generating one source file for all the aggregated extensions and originating elements sent to the filer are reported on a per-file basis. I agree isolating would be better though, but think it would be better to change that in a separate PR to limit scope. It should be possible to do that in a non-source-breaking way via just generating them into multiple files.

ZacSweers · 2020-02-02T21:59:43Z

I'm also curious as to what you mean by "The isolating processors should maybe also be separated, as right now even one change to one isolating type will cause all of them to rerun", in your PR description. I was under the impression that a change to an isolating type would cause the processor to rerun but only on that particular type. Is that incorrect?

So as far as gradle/kapt/etc are concerned, Epoxy is currently just one annotation processor that processes a number of different annotations.

This means that if just one annotated source file is dirty

They are effectively all treated dirty because all of them will be reprocessed. I don't think they'll all be recompiled, but the build mechanism will glob together everything they touch and I've found in practice that it's conservative in deciding what's relevant and what's not. In short - the processor asks for all the above, and the build will try to give it all the above.

This is against the point of incremental apt, because now it's doing redundant work over all the other files even though just one type was annotated. It may not always find them in the roundEnv's returned set of elements, but it's still looking for them.

Further more - they all will run at whatever incap type is the slowest. That means if they also use Kotlin extension generation, they'll take an extra hit in that redundant processing.

Aggregating is gnarly, because it will often result in huge source sets being recompiled as well due to the transitive and wide-casting net effect it has. Not a full recompilation, sure, but in our slack project we saw this still incurring nearly 100 files being re-compiled and processed despite no changes to anything they touch.

The ideal setup would be to confine processors to exactly what they're processing, and allow the build process to handle invoking them independently based on what's needed. While I don't think kapt does this today (and I'm not sure if gradle's java version does), in the future an optimization here would be that the build system doesn't need to even run them at all.

Now, only changed ModelView-annotated elements will actually be fed into apt and the other ones will effectively be no-ops

Some notes in that diagram:

The blue litho annotations are ones that aren't actually currently declared but used in its processing
The @GenerateExtension annotation is a theoretical one to better support kotlin extensions in a separate round. The idea would be that models that epoxy generates in the other processors can generate that annotation onto those models, then the kotlin extensions processor would pick those up in a later round.
Judging from code, it seems like databinding processing is an implicit dependency for ControllerProcessor. Ideally that would also use rounds, but if that's not possible they could just be packaged together.
Config manager may need to be a shared utility since it doesn't actually appear to generate anything.

Lastly - any processors that aren't always going to be run in this pipeline should be packaged into a separate artifact. This is because their very presence on the apt classpath will still cause them to be picked up and run (albeit with nothing to process). This is a bigger problem for aggregating processors (due to the aforementioned recompilation cost), but can even be problematic for isolating ones if their init or process() methods are non-trivial, such as Dagger's. In the above chart, it seems to me like the litho, databinding, config manager, and kotlin extension support at least

Hope that makes sense! The TL;DR is basically - it's not terrible now, but it would be cleaner to separate them and the kotlin aggregating processor at least should be moved out to avoid aggregating cost.

elihart

Sorry for the huge delay in reviewing this, just got back from leave. In general it seems great! Thanks for the well thought out diagrams and comments. I think your suggested approach to separate out the generated kotlin functions into dedicated files would work well and be easy to implement.

As far as splitting up the processors further, I don't think the impact will be too large because my suspicion is that people would generally use one approach to define models. The different processors allow for different model definition approaches 1. the original @EpoxyModelClass 2. Databinding 3. @ModelView 4. Litho (basically unsupported at this point) - I would guess that most people only use one of these types, so I don't anticipate much parallel processing of these different annotations (but I'm open to the idea, just don't think it will be as impactful as you might otherwise think)

@AutoModel is also not really supported any more as Kotlin makes it pointless. I also don't think the config manager annotations can be split out because the other processors rely on them for configuration input. So those processors shouldn't be considered for separation.

I believe the LithoSpecProcessor needs an originating element added for the litho classes it generates models from.

It seems you didn't attempt to make the databinding processor incremental. Was that intentional or an oversight? I believe these models needs to be aggregating because their contents are derived from reflectively accessing the Databinding Model generated for the xml layout. Additionally, if the EpoxyDataBindingPattern annotation is used we use reflection to look up layout files in the R class. We may need to extract this into its own annotation processor so it can be aggregating without requiring the original processor to be aggregating.

elihart · 2020-02-18T22:41:52Z

epoxy-processor/src/main/java/com/airbnb/epoxy/EpoxyProcessor.java

+      try {
+        // Get original ProcessingEnvironment from Gradle-wrapped one or KAPT-wrapped one.
+        for (Field field : processingEnv.getClass().getDeclaredFields()) {
+          if (field.getName().equals("delegate") || field.getName().equals("processingEnv")) {


if there are any links you have to documentation or more information about these api's it'd be nice to leave them here

elihart · 2020-02-18T23:19:38Z

epoxy-processor/src/main/java/com/airbnb/epoxy/ModelBuilderInterfaceWriter.kt

@@ -46,7 +49,7 @@ internal class ModelBuilderInterfaceWriter(
                    // can generate the interface with the proper methods later
                    viewInterfacesToGenerate
                        .putOrMerge(
-                            it,
+                            ClassKey(it, modelInfo.superClassElement),


I don't think what this ClassKey is representing is quite right. To clarify what this "view interfaces" code is doing, let's say we have ViewA and ViewB which both implement the same "Checkable" interface. The key to this map needs to be the "Checkable" interface element, and the map should store the set of methods that both ModelA and ModelB have in common (since we know they have a shared interface, this is a simplification we use).

By adding the model element to the map key we will now have separate entries for each view. I think this means that technically the interface generated by this (writeFilesForViewInterfaces) would be aggregating, since it is influenced by multiple view classes.

This complicates matters here unfortunately, and I don't see a good way to not have this be aggregating, since the generated interface will always depend on multiple models. For example, say we have an interface Header that has functions setTitle and setCaption. ViewA and ViewB both implement this interface, but only ViewA marks setTitle as a ModelProp, and only ViewB marks setCaption as a ModelProp. The resulting generated models don't actually share any methods in common from this interface, and the generated interface they share would actually be empty. If either model was changed to mark the other interface method as ModelProp then the generated interface would change to include it.

This example is contrived, but imagine an interface has many functions and views only want their models to contain a subset of those. The point of the model interface is to provide a generated interface to the user that represents the shared subset of model methods.

I don't see any good ways to avoid aggregation here, unless maybe we change it to be a processor error if multiple models with the same view interface don't have the same interface methods marked as ModelProps. It would be a breaking change, but I don't think it would affect many people. Any ideas?

ZacSweers · 2020-03-03T16:17:38Z

Sorry but I'm afraid I don't really have the time right now to finish this PR. If someone else wants to get it over the finish line in a separate PR please feel free.

elihart · 2020-03-04T23:31:42Z

Thanks for getting this started Zac. I'll hopefully be able to pick it up eventually, otherwise I'm happy to help anyone else that wants to carry it.

ColtonIdle · 2020-03-25T00:25:32Z

@elihart is there a chance that Zacs work can be built into a snapshot or something. It seems like incremental kapt is functional and it'd be interesting to give it a spin in the meantime until someone else can find the time to commit to it to take it over the finish line.

elihart · 2020-03-25T15:23:23Z

@ColtonIdle this code isn't quite functional yet and I don't want to release a snapshot before it is, although someone else can host it if they want.

I've noted where the remaining issues are, if somebody wants to take on the rest of the work I'll help review.

ColtonIdle · 2020-03-25T15:32:30Z

@elihart Whoops. Thought it was functional. Sorry about that.

TaxistSamael · 2020-04-03T11:28:24Z

Hey,
Are you planning to continue work on incremental annotation processing for Gradle in the near future?

elihart · 2020-04-13T23:34:21Z

Sometime in the next three months probably

ZacSweers added 8 commits December 31, 2019 20:31

Add incap deps

bc2a5f0

Wire in trees safely in incremental processors

9572877

This is borrowed from Butter Knife and supports kapt and gradle's incremental annotation processor

Update KotlinPoet and use its Filer API (requires Kotlin 1.3.50+)

996d0db

Add footnote for future

4f03029

Add originating element to GeneratedModelWriter

7633510

Make kotlin extension generation incremental

f6c08cc

Make ControllerProcessor incremental

d4f9595

Make ModelBuilderInterfaceWriter incremental

504c917

ZacSweers mentioned this pull request Jan 1, 2020

Add support for incremental annotation processing in Gradle #423

Closed

ZacSweers added 2 commits December 31, 2019 21:31

Make incap runtime implementation since it's used outside of annotations

185d0db

Nix footnote

8c2b6ff

ZacSweers requested a review from ngsilverman January 15, 2020 01:31

ngsilverman reviewed Jan 28, 2020

View reviewed changes

Add comment

ead1284

elihart reviewed Feb 18, 2020

View reviewed changes

ZacSweers closed this Mar 3, 2020

ZacSweers deleted the z/incrementalProcessing branch March 4, 2020 23:32

v-grishechko mentioned this pull request Apr 23, 2020

Incremental kapt #949

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support incremental annotation processing #902

Support incremental annotation processing #902

ZacSweers commented Jan 1, 2020 •

edited

Loading

ZacSweers commented Jan 1, 2020

elihart commented Jan 3, 2020

ColtonIdle commented Jan 23, 2020

ngsilverman left a comment

ngsilverman Jan 28, 2020

ZacSweers Feb 2, 2020

ngsilverman Jan 28, 2020

ngsilverman Jan 28, 2020

ZacSweers Feb 2, 2020

ZacSweers Feb 2, 2020

elihart Feb 18, 2020

ngsilverman Jan 28, 2020

ZacSweers Feb 2, 2020

ngsilverman Jan 28, 2020

ZacSweers Feb 2, 2020

ngsilverman Jan 28, 2020

ZacSweers Feb 2, 2020

elihart Feb 18, 2020

ngsilverman Jan 28, 2020

ZacSweers Feb 2, 2020

ZacSweers commented Feb 2, 2020

elihart left a comment

elihart Feb 18, 2020

elihart Feb 18, 2020

ZacSweers commented Mar 3, 2020

elihart commented Mar 4, 2020

ColtonIdle commented Mar 25, 2020

elihart commented Mar 25, 2020

ColtonIdle commented Mar 25, 2020

TaxistSamael commented Apr 3, 2020

elihart commented Apr 13, 2020

Support incremental annotation processing #902

Support incremental annotation processing #902

Conversation

ZacSweers commented Jan 1, 2020 • edited Loading

ZacSweers commented Jan 1, 2020

elihart commented Jan 3, 2020

ColtonIdle commented Jan 23, 2020

ngsilverman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZacSweers commented Feb 2, 2020

elihart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZacSweers commented Mar 3, 2020

elihart commented Mar 4, 2020

ColtonIdle commented Mar 25, 2020

elihart commented Mar 25, 2020

ColtonIdle commented Mar 25, 2020

TaxistSamael commented Apr 3, 2020

elihart commented Apr 13, 2020

ZacSweers commented Jan 1, 2020 •

edited

Loading