Open Bug 1067217 Opened 5 years ago Updated 5 months ago

Refactor the annotation processor as an annotation processor

Categories

(Firefox Build System :: Android Studio and Gradle Integration, defect)

ARM
Android
defect
Not set

Tracking

(Not tracked)

ASSIGNED

People

(Reporter: ckitching, Assigned: ckitching)

References

Details

Attachments

(2 files, 2 obsolete files)

The "annotation processor" is not, in fact, an annotation processor.

javac provides the "annotation processing API", a mechanism by which annotation processors can be invoked by the compiler during the build. (See section 2 of http://scg.unibe.ch/archive/projects/Erni08b.pdf for a nonterrible explanation of how the bits fit together. The documentation around annotation processing is generally poor).

What we have is an independent Java application that runs after the build, loads our compiled jars, and uses the Reflection API to scan them for annotations which it then does stuff with.

This limits us in a bunch of different ways, the set of which has become annoying enough that I'm going to start whinging about it now.


* The old approach requires all our annotations to be present in the binary so the processor can find them. An annotation processor would allow us to use RetentionPolicy.SOURCE and strip the annotations, saving a small amount of binary size.

* We could fix Bug 1048651 almost trivially. The annotation processor can be made to spit out an explicit Proguard configuration file (one that has a series of straightforward "keep program element X" directives, instead of "Keep everything annotated with Y" (which is implemented wrongly, see the other bug).

* Given a generated Proguard config, we can then strip absolutely all the annotations from the binary, saving further space (but still not very much).

* After fixing Bug 1048651, the only thing we have to do to resolve Bug 1041852 and save 350kb of binary size is to integrate Proguard's stacktrace descrambler with our crash reporting system (well, we don't technically *have* to do that, but not doing so would be a stupid idea). This seems like it'd not prove particularly challenging for someone experienced in that area of the code? (It boils down to "Run all stack traces through this program before showing them to a human").

* It would become possible to break the dependency cycle that leads to the infuriating "The generated code has changed, please run this copy command" message.
That exists because compiling the jars requires the generated C++ code to already exist, and we needed the compiled jars to generate the C++ code.
Under the new scheme, we could run annotation processing before compiling the C++. Javac has a -proc:only option that causes the compiler to abort immediately after annotation processing. This would allow a much earlier build phase to run annotation processing and generate the C++ code before we need to compile it, before later running javac normally (possibly with -proc:none) to make the jars.


Build performance would improve (about 2.5s on my machine, so not terribly exciting).
* Scanning the jars with reflection as opposed to using the annotation processing API turns out to be about 15x slower (and less tidy. You can just tell javac "Give me every program element with annotation of type X, please", which is rather more pleasant than "Let's write a loop to run through everything in the program).

* The time cost of annotation processing does not scale linearly with class count in the same way that the time cost of jar-scanning does. Javac builds a map of what's annotated with what during its early compilation steps, so it can answer requests for annotated elements very cheaply.

* Incremental processing would occur, since we would only run the processor on jars which are being recompiled. Our existing build-system logic for incremental recompilation would apply to the annotation processor, saving more time.
Don't panic.
Attachment #8489203 - Flags: review?(nalexander)
Comment on attachment 8489203 [details] [diff] [review]
Make the annotation processor... into an annotation processor

Review of attachment 8489203 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, this is a ridiculously-large patch.

Very nearly all of it is mechanical translation of the code generation logic from using the Reflection API to using the Annotation Processing API. So Method -> ExecutableElement, Field -> VariableElement, etc.

Since it turns out if I review my own patch I can tag handy helpful comments that show up inline when you open it in Splinter, I'll do that now so you have some chance of reviewing this without losing your mind.

::: build/autoconf/android.m4
@@ +361,5 @@
>          AC_MSG_ERROR([not found. Please check your SDK for the subdirectory of build-tools. With the current configuration, it should be in $android_sdk_root/build_tools])
>      fi
>  
> +    JAVA_HOME="${JAVA_HOME}"
> +    AC_SUBST(JAVA_HOME)

Necessary so we can access rt.jar (which contains the annotation processing API) in m/a/b/annotations/processors/moz.build

::: mobile/android/annotations/processors/AnnotationProcessor.java
@@ +1,3 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Nearly all the changes in this file are a new input/output mechanism. We've thrown away all the stuff that was doing JAR iterations (IterableJarLoadingURLClassLoader and friends), and replaced it with annotation processing API calls that are equivalent.

@@ +144,3 @@
>  
> +        // The elements directly annotated for keeping.
> +        targetElements.addAll(roundEnvironment.getElementsAnnotatedWith(WrapElementForJNI.class));

getElementsAnnotatedWith is much nicer than "Run through the jar screaming".

@@ +209,3 @@
>  
> +        // Write the cpp file.
> +        try (LockedIndexedRandomAccessFile outfile = new LockedIndexedRandomAccessFile(OUTFILE, IMPLEMENTATION_FILE_HEADER, IMPLEMENTATION_FILE_FOOTER)) {

Try-with-resources. Oh how I wish we could use this on Android.

In case unfamiliar: this is just like an ordinary try block, but the compiler bolts a finally block on the end and closes everything we open between the () here.

@@ +214,5 @@
> +            if (initStubs != null) {
> +                initStubs = mergeInitStubs(initStubs, stubInitialiser);
> +            } else {
> +                initStubs = createNewInitStubs(stubInitialiser);
> +            }

The special-casing of initStubs is worthy of comment.
InitStubs is a top-level function in GeneratedJNIWrappers.cpp which calls the InitStubs methods on each of the generated classes. These methods do the initialisation stuff that's necessary before the JNI will talk to us.

Of course, now we're having to safely handle multiple executions, we give InitStubs its own record (See LockedIndexedRandomAccessFile to get your head round that: the comment at the top explains it fairly well, hopefully). The bulk of the crap in mergeInitStubs is related to making sure that concurrency doesn't randomly reorder our calls (and trigger a spurious "the generated code has changed" message).

::: mobile/android/annotations/processors/ClassToProcess.java
@@ +21,5 @@
> +
> +    public void sort() {
> +        Collections.sort(elementsToWrap, new ElementsToProcessComparator());
> +    }
> +}

Just represents a class with some number of annotated elements in it. It holds a pointer to the class and to a list of the elements that need to have code generated.

This abstraction is particularly useful because I've been asked to merge mmcdonough's modified generator (which allows for generation of things that we don't have source for via a config file):
https://github.com/FlyingJester/gecko-dev/tree/master/build/jarClassProcessors

That problem is reduced to "Have an earlier step which reads the config file and spits out a bunch of extra ClassToProcesses".

I'll add a comment to the source to this effect.

::: mobile/android/annotations/processors/ClassToProcessComparator.java
@@ +7,5 @@
> +    public int compare(ClassToProcess lhs, ClassToProcess rhs) {
> +        // Since the names have to be unique...
> +        return lhs.wrappedClass.getQualifiedName().toString().compareTo(rhs.wrappedClass.getQualifiedName().toString());
> +    }
> +}

Comparator so we can ensure an ordering on the processed classes. We don't want to randomly reorder the classes between builds!

::: mobile/android/annotations/processors/CodeGenerator.java
@@ +1,3 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Absolutely every change in this class is just mechanical transformation from Reflection API to Annotation Processing API. It's boring as hell, but means touching a lot of code.

Oh well, it's not like there's blame that wasn't me to trample.

::: mobile/android/annotations/processors/classloader/ClassWithOptions.java
@@ -11,5 @@
> -    public ClassWithOptions(Class<?> someClass, String name) {
> -        wrappedClass = someClass;
> -        generatedName = name;
> -    }
> -}

Since we killed GeneratorOptions in Bug 1067056, this madness can die. (Provided that gets r+'d)

::: mobile/android/annotations/processors/utils/io/GuideTree.java
@@ +4,5 @@
> +import java.io.RandomAccessFile;
> +import java.util.Map;
> +import java.util.TreeMap;
> +
> +public class GuideTree {

Provides an index over a LockedIndexedRandomAccessFile. (Also going to add a documentation comment now).

The index is stored in a comment at the top of the file that looks like this:

/*
+InitStubs:688
org.mozilla.gecko.GeckoAppShell:89208
org.mozilla.gecko.mozglue.NativeZip:989
org.mozilla.gecko.util.Clipboard:2520
*/

Each row represents a record. The left portion is the key, the right the associated payload length.

Keys are loaded into a TreeMap. To find the offset of a given key, you simply iterate over the TreeMap summing lengths until you find the key you care about.
To find the offset for a new key, insert it into the treemap first, then look it up.

This is the basis of how we can write from many invocations into one file without destroying the world. Each written payload has a unique key, we have a total order over keys, and we have a mechanism that makes sure we always insert the payloads into the output file in the same order.

::: mobile/android/annotations/processors/utils/tree/AlphabeticElementComparator.java
@@ +15,5 @@
> +import java.util.Comparator;
> +import java.util.Iterator;
> +import java.util.List;
> +
> +public class AlphabeticElementComparator implements Comparator<Element> {

Another missing doc comment.

This is the most ridiculous and tedious class in the entire patch.
We need to make sure all our fields and methods come out in the same order (so repeatedly building with no changes to annotations doesn't produce different output), but they get given to us by javac in an undefined order. So, we need to sort them somehow.
This is a comparator over program elements that sorts them. *yawn*

::: mobile/android/annotations/processors/utils/tree/ElementToProcess.java
@@ +45,5 @@
> +        if (ele instanceof ExecutableElement) {
> +            defaultName = elementName.substring(0, 1).toUpperCase() + elementName.substring(1);
> +        } else {
> +            defaultName = elementName;
> +        }

This is slightly interesting because it's annoying and crops up in part two.

The naming of getters was inconsistently applied in the old code, so the new generated code renames a few things. (hence the very large and entirely uninteresting patch to follow that contains the new generated code and changes all the call sites of renamed things).

::: mobile/android/annotations/processors/utils/tree/ElementsToProcessComparator.java
@@ +1,5 @@
> +package org.mozilla.annotations.processors.utils.tree;
> +
> +import java.util.Comparator;
> +
> +public class ElementsToProcessComparator implements Comparator<ElementToProcess> {

Another painfully tedious comparator :/

::: mobile/android/annotations/processors/utils/tree/Utils.java
@@ +1,3 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Another class entirely full of mechanical changes.
This was just a bunch of utility functions used by the code generator, but now it has to use the APAPI classes instead of Reflection. *yawn*

::: mobile/android/base/moz.build
@@ +34,5 @@
>  mgjar.generated_sources += [
>      'org/mozilla/gecko/AppConstants.java',
>      'org/mozilla/gecko/mozglue/GeckoLoader.java',
>  ]
> +mgjar.javac_flags += ['-Xlint:all,-processing', '-processor org.mozilla.annotations.processors.AnnotationProcessor']

-processing is necessary, because "processing" is a kinda dumb warning.
"processing" warns you if you have a least one annotation processor, and you have annotations which are not being processed by an annotations.
In principle we could revisit this and see who's been using annotations spuriously, but for now this patch is already big enough (and there seem to be quite a few instances of people using annotations in strange ways)
Huge, boring patch: The new generated code. Some stuff got moved due to a slightly different sorting system, some stuff got renamed as discussed (I decided to enforce a good naming convention everywhere, instead of write extra logic to keep the bug that the old generator had)
Attachment #8489206 - Flags: review?(nalexander)
Attached patch Fixup call sitesSplinter Review
Fixing up those call sites. At this point it builds fine, let's do a try run and see if I broke anything...
Attachment #8489207 - Flags: review?(nalexander)
Thanks for all the helpful comments.  I'll try to look at this in the next day or two, but I'm quite busy.  Feel free to keep updating the ticket, I read my bug mail :)
Status update: still busy.  It's in my queue, I'm trying to get to it.  Sorry for the long delay.
Not to worry.

One interesting point: the try run went red because JAVA_HOME isn't set on the build machines. Seems extra work is needed to correctly fish out the location of the JDK from the build system (JAVA_HOME wouldn't be right anyway, it'd ignore mozconfig's --with-java-bin-path).

That should be a pretty minor change, though.
Comment on attachment 8489206 [details] [diff] [review]
New generated code

Review of attachment 8489206 [details] [diff] [review]:
-----------------------------------------------------------------

OK, finally trying to dig into this further.  This is a rubber stamp; after everything is reviewed and green, you can regenerate and land without my involvement.
Attachment #8489206 - Flags: review?(nalexander) → review+
Comment on attachment 8489207 [details] [diff] [review]
Fixup call sites

Review of attachment 8489207 [details] [diff] [review]:
-----------------------------------------------------------------

After we're green and reviewed, feel free to update this, get a green try, and land it without my further involvement.
Comment on attachment 8489203 [details] [diff] [review]
Make the annotation processor... into an annotation processor

Review of attachment 8489203 [details] [diff] [review]:
-----------------------------------------------------------------

I have some nits and some questions, but they're minor and I mostly just want to digest and see a try build.  I also want to understand how using the AP stuff works with Gradle.

But this looks pretty fantastic.

::: build/autoconf/android.m4
@@ +361,5 @@
>          AC_MSG_ERROR([not found. Please check your SDK for the subdirectory of build-tools. With the current configuration, it should be in $android_sdk_root/build_tools])
>      fi
>  
> +    JAVA_HOME="${JAVA_HOME}"
> +    AC_SUBST(JAVA_HOME)

I expect you'll want AC_DEFINE(JAVA_HOME) too.  Are you against exporting the location to rt.jar directly?  (We've done that in the past, for reasons I don't recall.  I have no particular preference.)

::: mobile/android/annotations/processors/AnnotationProcessor.java
@@ +190,4 @@
>                          break;
> +                    default:
> +                        System.err.println("Unable to process element " + targetElement.wrappedElement + " of kind " + targetElement.wrappedElement.getKind());
> +                        return true;

Seems like this should return false.

@@ +330,3 @@
>              }
> +        } else {
> +            System.out.println("Ignoring element: " + element + " of kind " + kind);

Seems like this will be noisy :(

::: mobile/android/annotations/processors/utils/io/FileWithLock.java
@@ +12,5 @@
> +import java.util.concurrent.atomic.AtomicBoolean;
> +
> +/**
> + * A RandomAccessFile that performs file locking. Only one instance of this class may exist in any
> + * concurrently executing JVM that refers to a particular file. This provides mutual-exclusion when

What enforces this?  A quick scan of RandomAccessFile doesn't suggest that you get this property.  It's possible FileChannel gives this, but I see some pretty weak wording about "programs" that doesn't agree with how I expect this to be used.  Is this a guarantee, or the contract that the consumer must agree to?

::: mobile/android/annotations/processors/utils/io/GuideTree.java
@@ +4,5 @@
> +import java.io.RandomAccessFile;
> +import java.util.Map;
> +import java.util.TreeMap;
> +
> +public class GuideTree {

What's the +InitStubs mean?  What do anonymous classes/nested classes look like?  Does this interact with native methods in any way?  Do we ever expect humans to read this, so that it's odd to support /* */ but not //?  Is the code easier if we always assume that // lines are part of this comment?  Could we simplify this by making all the indices absolute?  Could we simplify this special stuff by just reading a JSON blob out of the file with keys and values?

::: mobile/android/annotations/processors/utils/tree/AlphabeticElementComparator.java
@@ +15,5 @@
> +import java.util.Comparator;
> +import java.util.Iterator;
> +import java.util.List;
> +
> +public class AlphabeticElementComparator implements Comparator<Element> {

Double yawn, but this is the kind of thing that wants tests...

::: mobile/android/annotations/processors/utils/tree/ElementToProcess.java
@@ +45,5 @@
> +        if (ele instanceof ExecutableElement) {
> +            defaultName = elementName.substring(0, 1).toUpperCase() + elementName.substring(1);
> +        } else {
> +            defaultName = elementName;
> +        }

Could we unwind this and address the renaming before moving on this annotation processor stuff?

@@ +55,5 @@
> +            ExecutableElement cast = (ExecutableElement) ele;
> +
> +            List<? extends VariableElement> args = cast.getParameters();
> +
> +            // Translate annotations on parameters that declare them optional to the booleans.

What?

::: mobile/android/base/Makefile.in
@@ +175,5 @@
>  
>  ANNOTATION_PROCESSOR_JAR_FILES := $(DEPTH)/mobile/android/annotations/processors/annotation-processors.jar
>  
>  GeneratedJNIWrappers.cpp: $(ANNOTATION_PROCESSOR_JAR_FILES)
>  GeneratedJNIWrappers.cpp: $(ALL_JARS)

Targets without rules are probably not what we want.  This gets written (avoiding the write if possible) every compilation, correct?  It seems like this should be an output of the javac compilation process, then.  Which is hard to account for in Make :(

::: mobile/android/base/moz.build
@@ +34,5 @@
>  mgjar.generated_sources += [
>      'org/mozilla/gecko/AppConstants.java',
>      'org/mozilla/gecko/mozglue/GeckoLoader.java',
>  ]
> +mgjar.javac_flags += ['-Xlint:all,-processing', '-processor org.mozilla.annotations.processors.AnnotationProcessor']

OK, thanks for the explanation.  There's a little space to stream-line this, because we can define local functions in the moz.build file.

def allow_annotations(jar):
    jar.javac_flags += ...
    jar.extra_jars += [...]

And then allow_annotaations(mgjar), etc as necessary.  It might be irritating because I'm not sure it's OK to have two -Xlint arguments.
Attachment #8489203 - Flags: review?(nalexander) → feedback+
It appears I've made a slight screwup and split a chunk of this patch onto the other ticket.

Drat.

From your comments, it looks like you worked it all out in the end, so I'll spare you some of the explanation if it looks like you figured it out once you read that one.

(In reply to Nick Alexander :nalexander from comment #11)
> ::: build/autoconf/android.m4
> @@ +361,5 @@
> >          AC_MSG_ERROR([not found. Please check your SDK for the subdirectory of build-tools. With the current configuration, it should be in $android_sdk_root/build_tools])
> >      fi
> >  
> > +    JAVA_HOME="${JAVA_HOME}"
> > +    AC_SUBST(JAVA_HOME)
> 
> I expect you'll want AC_DEFINE(JAVA_HOME) too.  Are you against exporting
> the location to rt.jar directly?  (We've done that in the past, for reasons
> I don't recall.  I have no particular preference.)

I don't especially care.

Somethin here needs quite a bit of changing, certainly. At the moment, this fails on try due to JAVA_HOME not being set. I also need to do something to make it work as expected with the mozconfig option for setting the JDK.

More magic needed.

> ::: mobile/android/annotations/processors/AnnotationProcessor.java
> @@ +190,4 @@
> >                          break;
> > +                    default:
> > +                        System.err.println("Unable to process element " + targetElement.wrappedElement + " of kind " + targetElement.wrappedElement.getKind());
> > +                        return true;
> 
> Seems like this should return false.

The meaning of the return value is "Do you want to count the annotations that you were passed as consumed, or should we pass them to you (and other annotation processors that care about this annotation type) again in the next annotation processing round?"

As a result, you gotta return true when you're "done with" some annotations, otherwise you'll just endlessly get passed them over and over again because the compiler isn't convinced you're finished with them yet :/

... No, you're not the only one who thinks these semantics are strange.

What I can do, however, is use the Diagnostics object to spit out a compiler error. That seems like it'd make most sense. (The annotation processing API lets you do that. It's very cool).

> @@ +330,3 @@
> >              }
> > +        } else {
> > +            System.out.println("Ignoring element: " + element + " of kind " + kind);
> 
> Seems like this will be noisy :(

I can't find this line in my local version of the patch, so apparently an earlier version of me agreed with you. :P

> ::: mobile/android/annotations/processors/utils/io/FileWithLock.java
> @@ +12,5 @@
> > +import java.util.concurrent.atomic.AtomicBoolean;
> > +
> > +/**
> > + * A RandomAccessFile that performs file locking. Only one instance of this class may exist in any
> > + * concurrently executing JVM that refers to a particular file. This provides mutual-exclusion when
> 
> What enforces this?  A quick scan of RandomAccessFile doesn't suggest that
> you get this property.  It's possible FileChannel gives this, but I see some
> pretty weak wording about "programs" that doesn't agree with how I expect
> this to be used.  Is this a guarantee, or the contract that the consumer
> must agree to?

This is enforced by the semantics of FileChannel.lock(), which will block until the lock is released by whatever other instance of this class (in some other JVM, perhaps) holds the lock.
Since we call lock() in the constructor, you get the semantics I described. Be very careful not to freeze your JVM for eternity.

> ::: mobile/android/annotations/processors/utils/tree/ElementToProcess.java
> @@ +45,5 @@
> > +        if (ele instanceof ExecutableElement) {
> > +            defaultName = elementName.substring(0, 1).toUpperCase() + elementName.substring(1);
> > +        } else {
> > +            defaultName = elementName;
> > +        }
> 
> Could we unwind this and address the renaming before moving on this
> annotation processor stuff?

Possibly this is because it's 0500, but I don't understand what you're asking for here.

> ::: mobile/android/base/Makefile.in
> @@ +175,5 @@
> >  
> >  ANNOTATION_PROCESSOR_JAR_FILES := $(DEPTH)/mobile/android/annotations/processors/annotation-processors.jar
> >  
> >  GeneratedJNIWrappers.cpp: $(ANNOTATION_PROCESSOR_JAR_FILES)
> >  GeneratedJNIWrappers.cpp: $(ALL_JARS)
> 
> Targets without rules are probably not what we want.  This gets written
> (avoiding the write if possible) every compilation, correct?  It seems like
> this should be an output of the javac compilation process, then.  Which is
> hard to account for in Make :(

Hmph.

> OK, thanks for the explanation.  There's a little space to stream-line this,
> because we can define local functions in the moz.build file.
> 
> def allow_annotations(jar):
>     jar.javac_flags += ...
>     jar.extra_jars += [...]
> 
> And then allow_annotaations(mgjar), etc as necessary.  It might be
> irritating because I'm not sure it's OK to have two -Xlint arguments.

It is okay to have two -Xlint arguments, so I've done this. (-Xlint:<list of stuff> is just shorthand for a sequence of -Xlint options)
Attachment #8489203 - Attachment is obsolete: true
Attachment #8510046 - Attachment is obsolete: true
Attachment #8510046 - Flags: review?(nalexander)
Product: Firefox for Android → Firefox Build System
You need to log in before you can comment on or make changes to this bug.