Android applications built using Eclipse automatically invoke ProGuard to shrink, optimize, and obfuscate Java .class files before dx'ing them. Fennec's Makfiles do not use ProGuard. http://developer.android.com/guide/developing/tools/proguard.html http://proguard.sourceforge.net/index.html#/manual/examples.html#androidapplication In my preliminary test, Fennec's classes.zip shrank from 359 KB to 166 KB with optimizations applied: $ java -jar ~/Code/google/android-sdk-macosx/tools/proguard/lib/proguard.jar -verbose @Code/google/android-sdk-macosx/tools/lib/proguard.cfg -libraryjars Code/google/android-sdk-macosx/platforms/android-13/:Code/google/android-sdk-macosx/extras/android/support/v13/ -injars Code/mozilla/central/obj-android/mobile/android/base/classes -outjars classes-proguarded Ignoring unused library classes... Original number of library classes: 6021 Final number of library classes: 639 Shrinking... Removing unused program classes and class elements... Original number of program classes: 240 Final number of program classes: 168 Optimizing... Number of finalized classes: 116 Number of vertically merged classes: 0 (disabled) Number of horizontally merged classes: 0 (disabled) Number of removed write-only fields: 0 (disabled) Number of privatized fields: 160 (disabled) Number of inlined constant fields: 483 (disabled) Number of privatized methods: 58 Number of staticized methods: 36 Number of finalized methods: 382 Number of removed method parameters: 19 Number of inlined constant parameters: 21 Number of inlined constant return values: 14 Number of inlined short method calls: 40 Number of inlined unique method calls: 120 Number of inlined tail recursion calls: 0 Number of merged code blocks: 2 Number of variable peephole optimizations: 410 Number of arithmetic peephole optimizations: 0 (disabled) Number of cast peephole optimizations: 3 Number of field peephole optimizations: 4 Number of branch peephole optimizations: 117 Number of simplified instructions: 39 Number of removed instructions: 237 Number of removed local variables: 72 Number of removed exception blocks: 13 Number of optimized local variable frames: 206

Add some missing public/private access modifiers to GeckoAppShell.java. Without these access modifiers, methods default to package visibility. This confuses ProGuard and it inadvertently optimizes away some package methods that are needed by C++ JNI code.

Run ProGuard on our Java .class files before linking them into a classes.dex file. For now, we use ProGuard version 4.4 because it is conveniently bundled in Google's Android SDK. Mozilla developers do not need to install any additional build tools. However, Google's ProGuard version is about two years behind the current version 4.6 (and 4.7 beta), so we may want to consider upgrading someday.

Comment on attachment 583407 [details] [diff] [review] 709230-part-2-optimize-java-classes.patch Review of attachment 583407 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/config/proguard.cfg @@ +4,5 @@ > + > +# Preserve classes and methods accessed from C++ JNI. > +-keep class org.mozilla.gecko.GeckoAppShell { public <methods>; } > +-keep class org.mozilla.gecko.GeckoEvent { public <fields>; } > +-keep class org.mozilla.gecko.gfx.GeckoSoftwareLayerClient { public <methods>; } This is somewhat brittle. Please send a note to dev.platform. Basically we need to tell people, if they write new JNI methods, they need to be aware of this new requirement.

> This is somewhat brittle. Please send a note to dev.platform. Basically we need to tell > people, if they write new JNI methods, they need to be aware of this new requirement. Good point. To address this concern, I will update the proguard.cfg to a safer default that will always preserve public methods.

Comment on attachment 583407 [details] [diff] [review] 709230-part-2-optimize-java-classes.patch Review of attachment 583407 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/config/proguard.cfg @@ +4,5 @@ > + > +# Preserve classes and methods accessed from C++ JNI. > +-keep class org.mozilla.gecko.GeckoAppShell { public <methods>; } > +-keep class org.mozilla.gecko.GeckoEvent { public <fields>; } > +-keep class org.mozilla.gecko.gfx.GeckoSoftwareLayerClient { public <methods>; } This should be sufficient, we tend to want all JNI interaction to go through GeckoAppShell anyway

Change proguard.cfg to preserve all public classes and methods to address dougt's concern about brittle list of individual class names.

Comment on attachment 583912 [details] [diff] [review] 709230-part-2-optimize-java-classes-v2.patch dougt, can you please review v2 of my proguard.cfg patch (that preserves all public classes and methods).

Cancelled review until I fix some config problems related to the try server's Android SDK directory layout.

Rebased patch part 1. r=dougt

Rebased patch part 1. r=dougt

This change fixes the mysterious build failures that only affected try builders from the "try-linux-slave" pool. Build errors "went away" when I changed ProGuard's -libraryjars classpath from "$(ANDROID_SDK)" to "$(ANDROID_SDK)/android.jar". ProGuard's documentation says both directory and .jar -libraryjar classpaths should work, but for some reason specifying only the directory causes try-linux-slave builders to barf. This change is potentially destabilizing, so it should not be merged to m-a yet.

Comment on attachment 591652 [details] [diff] [review] bug-709230-part-2-optimize-java-classes-v3.patch Review of attachment 591652 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/config/proguard.cfg @@ +64,5 @@ > +-keepclassmembers enum * { > + public static **[] values(); > + public static ** valueOf(java.lang.String); > +} > + maybe we should put the mozilla specific rules below the defaults?

Moved Mozilla settings after Google's default settings. r=dougt

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d4d44ecd75c https://hg.mozilla.org/integration/mozilla-inbound/rev/823cd2815483

Sorry, I ended up backing this out on inbound because native Android builds were intermittently crashing on startup, even (or especially?) clobber builds. It's not 100% clear that this patch was at fault, but it seemed somewhat probable. Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/365277d89057

The startup crashes and GeckoLinker errors showed up on the Fx-Team branch *before* the Proguard patches had landed there, so perhaps this bug was not at fault after all: https://tbpl.mozilla.org/?tree=Fx-Team&rev=de1de9a14af6

I'll wait a couple days for the tree to quiesce, then retest my ProGuard patches.

Comment on attachment 591648 [details] [diff] [review] bug-709230-part-1-add-access-modifiers-v2.patch [Triage Comment]

Comment on attachment 591861 [details] [diff] [review] bug-709230-part-2-optimize-java-classes-v4.patch [Triage Comment]

https://hg.mozilla.org/releases/mozilla-beta/rev/da61d0b35be3 https://hg.mozilla.org/releases/mozilla-beta/rev/1595af7124c3

backed out https://hg.mozilla.org/releases/mozilla-beta/rev/1c0aba74d116

Chris, are you still working on this?

@mw22, I still have the patches. Because of the destablization risk, I was waiting until post-fennec-1.0. Should we tag this bug for the firefox14 milestone?

Comment on attachment 591648 [details] [diff] [review] bug-709230-part-1-add-access-modifiers-v2.patch clearing beta approval per akeybl on IRC, No Fennec Native Shipping for Gecko 11

Chris, want to revive this?

Sure. This has been a low priority because ProGuard may cause inscrutable build errors when developers change JNI code. Plus, the purported performance benefits are systemic and not easily measurable.

Rebasing my old ProGuard patches.

Rebasing my old ProGuard patches.

Mostly green try build: https://tbpl.mozilla.org/?tree=Try&rev=8312580d5316

Part 1: Add Android JNI error logging. ProGuard may inline or remove "unused" methods. In particular, ProGuard has little insight into which methods are accessed from C++ via JNI. This patch adds extra logging to developers debug JNI problems where ProGuard may have removed Java methods. If FindClass(), GetFieldID(), or GetMethodID() fail, Dalvik is going to abort us later anyways, so MOZ_Crash()'ing when we first detect the error is not an excessive response.

Add missing access modifiers for Java methods accessed from JNI. Mark classes, methods, and fields accessed from C++ JNI as public so ProGuard does not optimize away "unused" code.

Part 3: Optimize Java .class files with ProGuard. Add proguard.cfg configuration file and proguard .class files from Makefile. I wouldn't land this change until Firefox 17 (later this week).

Part 4: Mark classes final and reduce class visibility public->package->private to aid ProGuard optimizations.

Part 5: Remove unused imports.

Comment on attachment 642414 [details] [diff] [review] part-2-add-access-modifiers-v6.patch Review of attachment 642414 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/GeckoAppShell.java @@ +2545,5 @@ > }); > } > } > + > +} ws

Comment on attachment 642683 [details] [diff] [review] part-5-remove-unused-imports.patch Review of attachment 642683 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/GeckoEvent.java @@ +21,2 @@ > import android.os.SystemClock; > + ws

I have landed the changes to make our code ProGuard-friendly, but I have NOT enabled ProGuard yet! https://hg.mozilla.org/integration/mozilla-inbound/rev/b1ed016041d1 https://hg.mozilla.org/integration/mozilla-inbound/rev/554503c436a3 https://hg.mozilla.org/integration/mozilla-inbound/rev/844444ca8feb https://hg.mozilla.org/integration/mozilla-inbound/rev/fb153a7c666c

Drive-by before heading to bed: This broke native android, please can it be backed out. (53 failures since this landed, all startup crashes, and no green at all: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&jobname=Android%20Tegra%20250)

oops! Thanks. I backed out part 3, which caused this bustage: https://hg.mozilla.org/integration/mozilla-inbound/rev/a0e708ee1952

Thank you. I spotted you cancelled the builds on the backout: process killed by signal 9 program finished with exit code -1 elapsedTime=2474.866740 The web-page 'stop build' button was pressed by 'cpeterson@mozilla.com': Cancelled via self-serve Please don't do this unless absolutely necessary - killing builds requires a clobber (TBPL warns about this), or did you use the buildapi page directly? (Just wondering if I need to adjust the TBPL warning, or try to expedite bug 664858).

Meant to also say: In this instance, seeing the builds complete would have been useful, since it would have confirmed the backout was successful :-)

https://hg.mozilla.org/mozilla-central/rev/b1ed016041d1 https://hg.mozilla.org/mozilla-central/rev/fb153a7c666c

(In reply to Ed Morley [:edmorley] from comment #40) > > Please don't do this unless absolutely necessary - killing builds requires a > clobber (TBPL warns about this), or did you use the buildapi page directly? > (Just wondering if I need to adjust the TBPL warning, or try to expedite bug > 664858). sorry. That was my mistake. I got my many tryserver tabs mixed up. :\

Ah, no problem :-)

(And thank you for trying to conserve Try resources :-D)

Can this bug be closed?

This bug is incomplete because I had to backout the change that actually flipped the switch on ProGuard because it caused some bugs. I'd like to keep this bug open, but we do not need to track this bug for any particular release. Someone may want to try to land this change again (someday). Third time's the charm.

Someday...

What would it take to get even a super-safe (if non-optimal) proguard configuration landed (even if it's "do nothing, but at least run proguard")? This seems like a relatively big return on investment, and I'd like to get to a point where we can at least iterate on it (e.g., in the context of Bug 856791).

(In reply to Richard Newman [:rnewman] from comment #49) > What would it take to get even a super-safe (if non-optimal) proguard > configuration landed (even if it's "do nothing, but at least run proguard")? Adding proguard to the build steps should be pretty easy. The hard part is preventing proguard from renaming, inlining, or eliding classes and methods that are dynamically loaded by our JNI and Robocop code. In the patches above, I added proguard rules to leave public classes and methods alone, but when someone adds new JNI or Robocop code, the failures show up as frustrating runtime errors that do not hint at proguard problems. > This seems like a relatively big return on investment, and I'd like to get > to a point where we can at least iterate on it (e.g., in the context of Bug > 856791). Unfortunately, the wins were small (~200 KB) because less than 1 MB of our 26 MB .apk is Java code. I also hit (and successfully worked around) a few proguard bugs that generated bad code. I eventually gave up because the benefit:frustration ratio was way too low.

(In reply to Chris Peterson (:cpeterson) from comment #50) > Unfortunately, the wins were small (~200 KB) because less than 1 MB of our > 26 MB .apk is Java code. I also hit (and successfully worked around) a few > proguard bugs that generated bad code. I eventually gave up because the > benefit:frustration ratio was way too low. ProGuard is about more than just making the apk smaller - the performance improvements produced from, for example, inlining trivial getter/setter functions, are not insignificant: http://developer.android.com/training/articles/perf-tips.html#GettersSetters It seems that ProGuard does all the things you'd assume a sane compiler would do at compile-time that the java compiler does not. I'd be interested to measure how much of an improvement this actually makes (We really do use the trivial-getters pattern a lot in our code, even in performance-sensitive places) If nobody else wants to work on this I would be interested in carrying on with it...

(In reply to Chris Peterson (:cpeterson) from comment #50) > Adding proguard to the build steps should be pretty easy. The hard part is > preventing proguard from renaming, inlining, or eliding classes and methods > that are dynamically loaded by our JNI and Robocop code. In the patches > above, I added proguard rules to leave public classes and methods alone, but > when someone adds new JNI or Robocop code, the failures show up as > frustrating runtime errors that do not hint at proguard problems. You could fix that by using a Java annotation to tag classes that are special in this way and then configure ProGuard to leave them alone. All you need to do then is ensure people remember to add the annotation to such special classes.. Alternatively, do the opposite - an opt-in annotation for ProGuard-ification. Stuff would never break, but you'd have to remember the tag for classes you wanted to optimise.

Awesome! But tread carefully.. as you can see, this bug has a long history. <:) Patch parts 1, 2, 4, and 5 have all landed. part-3-optimize-java-classes-v7.patch is the patch that actually enables ProGuard, so you should start there. Watch out for ProGuard removing "unused" methods that are actually called from C++ JNI or Robocop's class introspection. In my proguard.cfg, I used the blunt approach of preventing public classes and methods from being elided. I assumed annotating classes and methods would be too much hassle to maintain.

(In reply to Chris Kitching [:ckitching] from comment #52) > > You could fix that by using a Java annotation to tag classes that are > special in this way and then configure ProGuard to leave them alone. All you > need to do then is ensure people remember to add the annotation to such > special classes.. If you're going to try this, you may want to tackle bug 794981 first as it covers some of this.

After fiddling with this for a while this seems to be quite annoying, but I have made some progress - I see why you decided to give up on it before. As Chris mentions above, a big problem is ProGuard optimising out stuff we access from the JNI. (In fact, it seems to be prettymuch the only remaining problem not solved by the patches already landed) Unfortunately, the approach of "Telling ProGuard not to optimise public classes and methods" is no good. The JNI and Reflection can, and in our codebase does, access private members of Java classes. This may well be while the original part 3 patch here had problems when it landed. The other problem with this approach that it is so blunt it really harms ProGuard's ability to optimise our code - we have a lot of public things. We could start trying to make less stuff public to improve this, but this seems like a very large amount of (Possibly impossible to do sufficiently well) work which is likely to be accidentially undone at some point in the future by someone who doesn't realise that things not being public is suddenly more important than usual. So we need a new plan. I return to my suggestion for Annotations. These do allow us to optimise well and have ProGuard not delete things it shouldn't delete, but require us to manually write @JNITarget above every field, method and class we don't want ProGuard to delete. Currently, this leads to about 230 annotations scattered throughout the codebase. This is a mess, but has allowed me to get ProGuard to work - so I can see if the performance benefits it brings are worth going to the extra trouble of finding a neat way to do this. The conclusion is yes. Very, very much yes. When profiled on a Nexus 4 with the Dalvik instrumenting profiler, it seemed to reduce the CPU time involved in loading a page on nytimes.com by about 11%. (And for some of the other page loads profiled the improvements seen were as high as 18%!) For those who fancy viewing the profiles themselves (Using `monitor` from the adk): Profiles collected with ProGuard: https://www.dropbox.com/sh/od0twgl83mqv1t3/DAtDMAAssE Profiles collected without ProGuard: https://www.dropbox.com/sh/sokclu1frstgi4i/cmB9dZXRwg Both also had the patches from Bug 897123 and Bug 896822 applied. (Won't affect relative speed of ProGuard vs. non-ProGuard, but does reduce the size of the profiles by about 20% by vastly reducing the amount of calls) Another thing of possible interest is the ProGuard output, to compare to the one from Comment 1: 3:28.15 Ignoring unused library classes... 3:28.15 Original number of library classes: 3588 3:28.15 Final number of library classes: 1152 3:28.15 Printing kept classes, fields, and methods... 3:28.33 Shrinking... 3:28.67 Removing unused program classes and class elements... 3:28.67 Original number of program classes: 2040 3:28.67 Final number of program classes: 1797 3:28.67 Optimizing... 3:37.35 Number of finalized classes: 1074 3:37.35 Number of vertically merged classes: 0 (disabled) 3:37.35 Number of horizontally merged classes: 0 (disabled) 3:37.35 Number of removed write-only fields: 271 3:37.36 Number of privatized fields: 1327 3:37.36 Number of inlined constant fields: 32 3:37.36 Number of privatized methods: 721 3:37.36 Number of staticized methods: 248 3:37.36 Number of finalized methods: 4202 3:37.36 Number of removed method parameters: 159 3:37.36 Number of inlined constant parameters: 136 3:37.36 Number of inlined constant return values: 38 3:37.36 Number of inlined short method calls: 1212 3:37.36 Number of inlined unique method calls: 1214 3:37.36 Number of inlined tail recursion calls: 3 3:37.36 Number of merged code blocks: 37 3:37.36 Number of variable peephole optimizations: 5308 3:37.36 Number of arithmetic peephole optimizations: 0 (disabled) 3:37.36 Number of cast peephole optimizations: 11 3:37.36 Number of field peephole optimizations: 21 3:37.36 Number of branch peephole optimizations: 1551 3:37.36 Number of string peephole optimizations: 2292 3:37.36 Number of simplified instructions: 542 3:37.36 Number of removed instructions: 3436 3:37.36 Number of removed local variables: 355 3:37.36 Number of removed exception blocks: 390 3:37.36 Number of optimized local variable frames: 2367 3:37.36 Shrinking... 3:37.51 Removing unused program classes and class elements... 3:37.51 Original number of program classes: 1797 3:37.51 Final number of program classes: 1771 Shiny! Since this looks so promising, I'm going to push ahead with it. For now, there remain a number of issues with unit tests failing because the code they invoke has been inlined or deleted. Once I've tracked down and annotated everything that's needed to make it all turn shiny and green my approximate plan is as follows: Refactor JNI use such that all JNI entry points in our code are in the same class (Or a small number of classes) expressly designed for this purpose, instead of calling into arbitrary methods all over our codebase. The benefit here is that we no longer need a large number of annotations sprinkled about the codebase, we can just have one annotation for "Don't rename or inline things in this class". This seems neat enough that it might actually pass a review... Refactor the unit tests which use Reflection on our Java classes such that they no longer are calling into methods all over the code which are used for other purposes, but instead call into some designated entry point class which wraps the methods and properties the tests want to use. This way, we both reduce the number of annotations we need and improve the performance, since we no longer need to forbid ProGuard from optimising the methods which the tests use (Proguard is free to rename or inline them - it'll just inline them into the test rig's helper methods, too) This has to some extent already been done with the RobocopAPI class - I mean to expand this to be the gateway used by Robocop for accessing the code. (My latest attempt to make it stop being quite as badly orange is at https://tbpl.mozilla.org/?tree=Try&rev=60fb306f0fd6 .). I'm open to suggestions on alternative ways of getting this to work neatly. I think this "Designate a wrapper class as an entry point" pattern gives us the best of both neatness and performance gain, at the expense of being quite a lot of work to initially set up (That's what interns are for, right? :P ) It certainly seems that getting ProGuard working is something we need to do. Sort of nowish.

Nice! I for one like your plan. Note that in some cases though it might get tricky to have a JNI "entry point" class because the functions might need to be called on specific instances of the classes, and the "entry point" class would then need to hold on those instances, which might break encapsulation. That being said there's certainly room for cleanup from where we are now, so the best would probably be some middle ground where most but not all of the JNI entry points are in one class but the rest are annotated.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #56) > Nice! I for one like your plan. Note that in some cases though it might get > tricky to have a JNI "entry point" class because the functions might need to > be called on specific instances of the classes, and the "entry point" class > would then need to hold on those instances, which might break encapsulation. > That being said there's certainly room for cleanup from where we are now, so > the best would probably be some middle ground where most but not all of the > JNI entry points are in one class but the rest are annotated. I agree with Kats. I'd like to see what the JNI entry point class looks like and how it "feels" to use it. It seems like a nice way to move this bug forward.

Chris, these results are very promising! I recommend profiling with ProGuard "-optimizationpasses 99". The current proguard.cfg only runs one optimization pass. Google's proguard.cfg defaults to five passes. I reduced the number because people on the team were annoyed by the 5-10 second build delays. ProGuard won't actually run 99 passes; it will stop when a pass can perform no more optimizations (usually about 5-9 passes). If you can land ProGuard, another step worth investigation is reducing the visibility of many org.mozilla.gecko classes and methods from `public` to "package-private" (i.e. no `public`, `protected`, or `private` specifier). ProGuard can probably do a better job of shrinking and optimizing the code.

(In reply to Chris Peterson (:cpeterson) from comment #58) > Chris, these results are very promising! > > I recommend profiling with ProGuard "-optimizationpasses 99". The current > proguard.cfg only runs one optimization pass. Google's proguard.cfg defaults > to five passes. I reduced the number because people on the team were annoyed > by the 5-10 second build delays. ProGuard won't actually run 99 passes; it > will stop when a pass can perform no more optimizations (usually about 5-9 > passes). Ah! I forgot about that switch. I collected some profiles with 10 passes set (Setting 100 passes lead to ProGuard entering an apparently endless loop removing a single write-only field each time... Perhaps I was just not sufficiently patient with it.) Curiously, this appears to be not be substantially faster than using a single pass, and actually slower on some measurements... https://www.dropbox.com/sh/cbbvyigexti7pi7/L723lUGQJy This leads me to question the accuracy of my testing method. Do we have any existing performance benchmarking tools? I feel it would be good to verify the pointfulness of this work using something other than a robocop profiling task I put together in three hours. (Which is looking increasingly like an extremely poor PRNG instead of a meaningful test) On the bright side, it now passes unit testing: https://tbpl.mozilla.org/?tree=Try&rev=58bdc7692bee I do not know if those intermittent oranges are my fault or not. So all that remains is to make the patch not suck, and we're in business. (Although since "make the patch not suck" requires quite extensive refactoring of a lot of things this may turn into a reasonably large series of patches. I remain thankful that much of this can be automated.) > If you can land ProGuard, another step worth investigation is reducing the > visibility of many org.mozilla.gecko classes and methods from `public` to > "package-private" (i.e. no `public`, `protected`, or `private` specifier). > ProGuard can probably do a better job of shrinking and optimizing the code. Certainly. It seems likely that this may reduce the number of passes ProGuard needs in order to complete the task, as well.

> Do we have any existing performance benchmarking tools? I feel it would be > good to verify the pointfulness of this work using something other than a > robocop profiling task I put together in three hours. (Which is looking > increasingly like an extremely poor PRNG instead of a meaningful test) Some reading suggests the Talos suite is what I'm after. I've already run this suite on Try (see link in previous comment). Am I correct in assuming that the number shown when you click on the test in the tbpl UI is the test score, and that lower is better? If so, then comparing my run with some runs from stuff on Mozilla-Central, the results are looking extremely excellent. Do we have any other performance benchmarks we use to track how slow we are? It'd be helpful if someone could link me to such (Not just for this bug, but to assist in my ongoing war on slow)

(In reply to Chris Kitching [:ckitching] from comment #60) > Some reading suggests the Talos suite is what I'm after. I've already run > this suite on Try (see link in previous comment). Am I correct in assuming > that the number shown when you click on the test in the tbpl UI is the test > score, and that lower is better? Lower is better for most of the tests but not all. For details on specific tests, see this page: https://wiki.mozilla.org/Buildbot/Talos/Tests The compare-talos tool is useful for examining differences in Talos scores between pushes: http://perf.snarkfest.net/compare-talos/index.html?oldRevs=604c8518cc50,5a86438c4093,fd10ead17ace&newRev=58bdc7692bee&submit=true I retriggered the Talos runs in your Try push so that the stats will include more than one datapoint for each of the "after" numbers (once the new runs have finished). > Do we have any other performance benchmarks we use to track how slow we are? > It'd be helpful if someone could link me to such (Not just for this bug, but > to assist in my ongoing war on slow) Aside from Talos there is also the Eideticker test which measures graphic performance: https://wiki.mozilla.org/Project_Eideticker

(In reply to Matt Brubeck (:mbrubeck) from comment #61) > (In reply to Chris Kitching [:ckitching] from comment #60) > The compare-talos tool is useful for examining differences in Talos scores > between pushes: > http://perf.snarkfest.net/compare-talos/index.html?oldRevs=604c8518cc50, > 5a86438c4093,fd10ead17ace&newRev=58bdc7692bee&submit=true Those numbers don't look great.. to what did you compare my try build? I ran my own comparism against another recent try run of mine (Which was doing some unrelated thing to the preferences system) and got: http://perf.snarkfest.net/compare-talos/index.html?oldRevs=327ab519bf32&newRev=58bdc7692bee&submit=true This seems to suggest my patch makes a number of things worse, and very few actually better, and then not by a great deal.. That doesn't make sense. In my suspicion, I decided to do a control - to compare the talos runs of 655498beff2c (Adding a new context menu item) and 327ab519bf32 (Adding a new way to change default search providers). Two patches I wrote recently that ought to have identical performance test results, aside from the jitter on the tests: http://perf.snarkfest.net/compare-talos/index.html?oldRevs=655498beff2c&newRev=327ab519bf32&submit=true Crikey, those tests are jittery! In fact, for some of the tests the two runs on the ProGuard patch differ by as much as 8%! > > Do we have any other performance benchmarks we use to track how slow we are? > > It'd be helpful if someone could link me to such (Not just for this bug, but > > to assist in my ongoing war on slow) > > Aside from Talos there is also the Eideticker test which measures graphic > performance: > https://wiki.mozilla.org/Project_Eideticker I shall have a play with this...

(In reply to Chris Kitching [:ckitching] from comment #62) > Those numbers don't look great.. to what did you compare my try build? I compared it to its parent changeset from mozilla-central (604c8518cc50), plus the two mozilla-central pushes immediately before that one. > This seems to suggest my patch makes a number of things worse, and very few > actually better, and then not by a great deal.. That doesn't make sense. > In my suspicion, I decided to do a control - to compare the talos runs of > 655498beff2c (Adding a new context menu item) and 327ab519bf32 (Adding a new > way to change default search providers) Good; those Try pushes had the same parent (604c8518cc50) so they have no differences other than your patches. > Crikey, those tests are jittery! Yup. :( Our regression monitoring averages 12 runs before and 12 runs after each change in order to get meaningful comparisons.

(In reply to Matt Brubeck (:mbrubeck) from comment #63) > Yup. :( Our regression monitoring averages 12 runs before and 12 runs > after each change in order to get meaningful comparisons. Ew. I'm not sure even twelve is enough when the jitter is as high as this (50%-ish in some cases..) So - what to do? Rerun the tests many more times to get a closer approximation to a meaningful result? I'm not sure we can draw any conclusions from what we currently have...

Also, looking at the try run for that parent revision.. https://tbpl.mozilla.org/?tree=Try&rev=604c8518cc50 There are no Talos runs here?

(In reply to Chris Kitching [:ckitching] from comment #64) > So - what to do? Rerun the tests many more times to get a closer > approximation to a meaningful result? I'm not sure we can draw any > conclusions from what we currently have... Yes, more runs is really the only solution, either by re-triggering on Try or by landing the change and watching the graph server / monitoring emails. Aside from roboprovider, and possibly robocheck/robopan, I think most of our performance benchmarks are dominated by Gecko C++ code and so they might not be good for showing the impact of this patch. If the patch is basically neutral for our current benchmarks, and we think it's likely to buy us wins elsewhere at a reasonable cost, then I would just land it. (And of course if you want to work on making the benchmarks more stable or on adding any new ones, that would be very welcome!) (In reply to Chris Kitching [:ckitching] from comment #65) > Also, looking at the try run for that parent revision.. > https://tbpl.mozilla.org/?tree=Try&rev=604c8518cc50 > > There are no Talos runs here? The parent push was from the mozilla-central repository, not Try: https://tbpl.mozilla.org/?rev=604c8518cc50

(In reply to Matt Brubeck (:mbrubeck) from comment #66) > Aside from roboprovider, and possibly robocheck/robopan, I think most of our > performance benchmarks are dominated by Gecko C++ code and so they might not > be good for showing the impact of this patch. Ah. Yes, this patch is entirely unhelpful there. Also, we only spend about half our CPU time in the C++ code, so this seems a somewhat significant omission. > If the patch is basically > neutral for our current benchmarks, and we think it's likely to buy us wins > elsewhere at a reasonable cost, then I would just land it. I see. Fair enough - my own Java profiling measurements seem to give some quite nice numbers - I had just hoped to get these results confirmed by some tests I didn't write myself, for sort of obvious reasons. The reason I've been going on about this so much instead of just landing it and seeing what happens is that the current version of the patch is a hugely hacky mess which gets the optimiser working approximately as well as it can be, but does so at the expense of neatness. Some considerable refactoring work is needed before it is reviewable - I had hoped to have confirmation from the tests that this is worth the effort before going through with it. That said, it's ProGurd - we're sure it'll make stuff better, just not sure by how much. I'll just get on with it and see what happens. > (And of course if you want to work on making the benchmarks more stable or > on adding any new ones, that would be very welcome!) I'll add it to my disturbingly extensive to-do list :P.

Upon the completion of Bug 917512 and Bug 913985, it will be possible to make Proguard work by preventing it from deleting or renaming the elements shown in this patch. These are the elements used by Robocop's unit tests. Since work is ongoing to make Robocop stop doing this, these annotations will, eventually, cease to be necessary. If we choose this course, the annotations on BrowserContract can be combined into a single "Don't touch any elements of this class" annotation. I suggest that we should do this - while the 70 annotations necessary will be ugly, they will be removed as the work to migrate to JUnit testing continues, and will get us the largeish performance improvements of Proguard much sooner (As work on tests seems like something that will take a considerable time to arrive. Tests are not something you want to get wrong.) Additionally, since the problem of making Robocop stop doing Reflection on our code is known about and being worked on, it should never become necessary to do any maintainence on these annotations aside from deleting them as the corresponding tests are upgraded. Thus we unmaintainbility problem of solving the Proguard problem exclusively with annotations through merit of never actually having to maintain the annotations. Thoughts?

So with the impending conclusion of the code generation bugs, I've been revisiting this to create a Proguard patch that actually works. It will require 34 annotations to keep Robocop happy (Which will be reduced as the ongoing work to replace Robocop tests with JUnit tests proceeds) - Bug 916507 and friends. This number should only go down, not up. If you're adding to it you're writing tests wrong. 38 Annotations will be required to keep WebRTC happy - Bug 917512. Plausibly the generator can be expanded to cover these. Two annotations will be required to cover the entry points from JS into Java via the JNI. It's probably not worth doing anything clever to tidy these up. One annotation is needed to keep GeckoEvent happy - this single annotation is liable to have a larger effect on Proguard performance that any of the others here - Bug 919138. 135 annotations that were necessary in my prototype patches are no longer required due to the generator. (Or rather, they already exist and are pointful. How convenient!) I have also determined why my earlier attempts to measure the performance gain using Talos were unsuccessful (Despite the promising 11-18% improvement I saw locally). Essentially - I forgot how to makefile. Under certain scheduling conditions, dex would run concurrently with proguard and dex the unproguarded bytecodes. This was happening on try, but not locally. So the reason my talos runs failed to measure a difference between proguarded and non-proguarded code was that the ostensibly proguarded code wasn't. Whoops. I suggest we land the patches I'll be providing shortly with the above mentioned annotations and solve the linked bugs as and when. The generator covers the regions of code most liable to be changed regularly which, in my opinion, eliminates the vast majority of the maintainence nightmare that is the naive "Annotate stuff" solution. Victory!

Okay, the patch series that makes Proguard work, after the landing of Bug 913985. Maybe worth reading above two messages for context. Try push: https://tbpl.mozilla.org/?tree=Try&rev=744fe1a2d2a7 Some wonkyness with XPCShell to iron out. Might be 913985's fault. First part - annotate the two JNI entry points from JS into Java via the JNI and special-case GeckoEvent

This patch rearranges things needed by Robocop in a way that minimises how much stuff we have to annotate to keep it happy. Ultimately, no annotations should be needed for Robocop at all, but in the meantime, the smallish number in the next patch seems a worthy price to pay for the gains of Proguard.

The horrid stopgap measure to keep Robocop happy. Hopefully landing this and insisting on never increasing the number of annotations will encourage people to stop writing tests that should be JUnit using Robocop. Ultimately, these should all be removed.

And also don't delete WebRTC... There's a bug for getting these covered by the generator, but they seem quite well tangled up - perhaps a job for someone more familiar. Again, stopgap measure, performance gain, blah de blah.

And finally, turn on the shiny new toy.

Comment on attachment 810252 [details] [diff] [review] Part 1 - Annotate by hand the JNI entry points not covered by the code generator (All three of them). Review of attachment 810252 [details] [diff] [review]: ----------------------------------------------------------------- My approach to these reviews is somewhat "looks fine" -- I have no good way to verify that you're hitting the right set of entry points or that things work afterwards, so consider reviewing as a counterpart to really thorough testing! ::: mobile/android/base/mozglue/JNITarget.java @@ +6,5 @@ > + > +import java.lang.annotation.Retention; > +import java.lang.annotation.RetentionPolicy; > + > +@Retention(RetentionPolicy.RUNTIME) Why does this annotation need to be kept around at runtime? Isn't RetentionPolicy.CLASS enough?

Comment on attachment 810254 [details] [diff] [review] Part 3 - Annotate the remaining Robocop entry points. Review of attachment 810254 [details] [diff] [review]: ----------------------------------------------------------------- I'd be very happy to have AppConstants and BrowserContract both be entirely unmolested, rather than peppered with annos. The whole point of BrowserContract is that it's an exposed contract, and a bucket of constants is the kind of thing we might want to grab from a JNI test add-on. ::: mobile/android/base/gfx/GeckoLayerClient.java @@ +61,1 @@ > private DrawListener mDrawListener; If the methods are annotated, why do we also need to annotate the field? ::: mobile/android/base/home/SuggestClient.java @@ +43,1 @@ > private boolean mCheckNetwork; Continued ugh. This is the very opposite of designing for testability. ::: mobile/android/base/mozglue/RobocopJNITarget.java @@ +6,5 @@ > + > +import java.lang.annotation.Retention; > +import java.lang.annotation.RetentionPolicy; > + > +@Retention(RetentionPolicy.RUNTIME) Similar comment about CLASS.

Comment on attachment 810256 [details] [diff] [review] Part 5 - Activate Proguard. Review of attachment 810256 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/Makefile.in @@ +1245,5 @@ > + $(DX) --dex --output=classes.dex jars-proguarded $(ANDROID_COMPAT_LIB) > + > +proguard: $(ALL_JARS) > + @echo "Proguard" > + java -jar $(ANDROID_SDK)/../../tools/proguard/lib/proguard.jar @$(topsrcdir)/mobile/android/config/proguard.cfg -injars jars -outjars jars-proguarded -libraryjars $(ANDROID_SDK)/android.jar:$(ANDROID_COMPAT_LIB) Is this the right path for all NDKs? Prefer $(ANDROID_SDK_ROOT)/tools/ instead of the relative path from the SDK. (Assuming it's defined here.) gbrown might have more to say about this. ::: mobile/android/config/proguard.cfg @@ +14,5 @@ > +-keep public class * extends android.app.Service > +-keep public class * extends android.app.backup.BackupAgentHelper > +-keep public class * extends android.content.BroadcastReceiver > +-keep public class * extends android.content.ContentProvider > +-keep public class * extends android.preference.Preference And SyncAdapter. @@ +28,5 @@ > + public <init>(android.content.Context, android.util.AttributeSet); > +} > + > +# Preserve all classes that have special context constructors, and the > +# constructors themselves. Unnecessary dupe comment. @@ +65,5 @@ > + > +# Preserve all fundamental application classes. > + > +-keep public class * extends android.app.Activity > +-keep public class * extends android.app.Application Duplicate block… @@ +76,5 @@ > + > +-keep public class * extends android.view.View { > + public <init>(android.content.Context); > + public <init>(android.content.Context, android.util.AttributeSet); > + public <init>(android.content.Context, android.util.AttributeSet, int); Also dupe…? @@ +130,5 @@ > +# classes. > + > +-keepclassmembers class * extends java.lang.Enum { > + public static **[] values(); > + public static ** valueOf(java.lang.String); … dupe? @@ +239,5 @@ > +} > + > +# Google uses 5 passes, but each pass increases our build time by 5 seconds! > +# The optimizer's returns diminish after ~2 passes and stop at ~9 passes. > +-optimizationpasses 4 This sounds like something we want to parameterize from mozconfig, run more on buildbots than locally, or similar. @@ +245,5 @@ > +# Obfuscation because it makes exception stack traces more difficult to read. > +-dontobfuscate > + > +# Suppress warnings about missing descriptor classes. > +-dontnote **,!ch.boye.**,!org.mozilla.gecko.sync.** What descriptor classes?

(In reply to Richard Newman [:rnewman] from comment #75) > My approach to these reviews is somewhat "looks fine" -- I have no good way > to verify that you're hitting the right set of entry points or that things > work afterwards, so consider reviewing as a counterpart to really thorough > testing! Very much so. This patch set should probably sit on nightly for several weeks before it gets released anywhere - certainly not one to land and instantly ship. (Unless you want to be uplifting fixes for a very long time). The possibility of subtley breaking something is sort of high. It's also quite likely someone is going to land a new awful robocop test or such before this clears review (I think Wes mentioned he was working on a new test, for example) - so there'll likely be a need to deal with such additional breakages as may happen between now and then. Hopefully, once landed, now JUnit is becoming a thing, people won't write tests that need more annotations in Robocop-land. > ::: mobile/android/base/mozglue/JNITarget.java > @@ +6,5 @@ > > + > > +import java.lang.annotation.Retention; > > +import java.lang.annotation.RetentionPolicy; > > + > > +@Retention(RetentionPolicy.RUNTIME) > > Why does this annotation need to be kept around at runtime? Isn't > RetentionPolicy.CLASS enough? Quite right. This'll save us a few bytes of memory at runtime.

(In reply to Richard Newman [:rnewman] from comment #76) > Comment on attachment 810254 [details] [diff] [review] > Part 3 - Annotate the remaining Robocop entry points. > > Review of attachment 810254 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'd be very happy to have AppConstants and BrowserContract both be entirely > unmolested, rather than peppered with annos. The whole point of > BrowserContract is that it's an exposed contract, and a bucket of constants > is the kind of thing we might want to grab from a JNI test add-on. > > ::: mobile/android/base/gfx/GeckoLayerClient.java > @@ +61,1 @@ > > private DrawListener mDrawListener; > > If the methods are annotated, why do we also need to annotate the field? Oh, that's true, we don't need this one. Also... Notice how we have extra methods in GeckoLayerClient that exist entirely for testing (The two that are annotated). This is so very many different kinds of dreadful. > ::: mobile/android/base/home/SuggestClient.java > @@ +43,1 @@ > > private boolean mCheckNetwork; > > Continued ugh. This is the very opposite of designing for testability. Aye, this sucks. Having looked into it in more detail, it appears the test that's using this needs to be taken out and shot. The field is only ever being used to, after constructing a SuggestClient, set the field to false. Boolean members of objects in Java are implicitly initialised to false. The value is set to true in the constructor, however. But the annotated constructor is used only by Robocop - so we can just set the value to false there and drop the annotation on this field. This approach sucks slightly less. In some ways, the high coefficient of ugh that making Robocop tests written incorrectly now requires is a good thing - it'll encourage the removal and prevent the addition of such tests.

(In reply to Richard Newman [:rnewman] from comment #76) > I'd be very happy to have AppConstants and BrowserContract both be entirely > unmolested, rather than peppered with annos. The whole point of > BrowserContract is that it's an exposed contract, and a bucket of constants > is the kind of thing we might want to grab from a JNI test add-on. Probably for the best in terms of neatness - but it costs us a bytecode instruction per reference to these fields - that's a fair few extra bytes in our file. Not massively significant, but perhaps worth a followup bug to make AppConstants available to Robocop a different way (eg. Give Proguard its own copy of this class, updated at compile-time to match the one in m/a/b.). Would also speed up the tests a tad - doing reflection to get constants is kinda stupid anyway.

> (In reply to Richard Newman [:rnewman] from comment #76) > > I'd be very happy to have AppConstants and BrowserContract both be entirely > > unmolested, rather than peppered with annos. The whole point of > > BrowserContract is that it's an exposed contract, and a bucket of constants > > is the kind of thing we might want to grab from a JNI test add-on. And w.r.t BrowserContract - limitations of Proguard's config make it impossible (Or at least not obvious) how to say "When I annotate this class keep it, all its members, and all the members of inner classes thereof". While the "Keep all members" part is easy, it still optimises inner classes - hence the need to sprinkle annotations on each of BrowserContract's inner classes, to keep it off of these, too. Ultimately, I think the "Real" solution here is use JUnit. Database testing seems quite far removed from what Robocop is designed to test - JUnit can probably do it faster. In my last comment, I said: > AppConstants available to Robocop a different way (eg. Give Proguard its own Clearly, s/Proguard/Robocop/.

(In reply to Chris Kitching [:ckitching] from comment #81) > Ultimately, I think the "Real" solution here is use JUnit. Database testing > seems quite far removed from what Robocop is designed to test - JUnit can > probably do it faster. Oh, I quite agree -- 'cept for two things: * I'm not only concerned with Robocop * We will certainly want to have tests that poke the UI and then verify the contents of the DB. > > AppConstants available to Robocop a different way (eg. Give Proguard its own > > Clearly, s/Proguard/Robocop/. This is not about Robocop access to constants -- it's that getting rid of that class will bite us later when we want to, e.g., grab one of those constants in an add-on using JNI. I'm very happy to spend the additional bytes to just keep *Constants unmolested.

(In reply to Richard Newman [:rnewman] from comment #82) > (In reply to Chris Kitching [:ckitching] from comment #81) > > > Ultimately, I think the "Real" solution here is use JUnit. Database testing > > seems quite far removed from what Robocop is designed to test - JUnit can > > probably do it faster. > > Oh, I quite agree -- 'cept for two things: > > * I'm not only concerned with Robocop > * We will certainly want to have tests that poke the UI and then verify the > contents of the DB. So perhaps the solution I suggested for the other problem would make more sense for such tests - in that hypothetical Robocop-like test's build script you have it yoink the BrowserContract class for its own purposes. I mean, all BrowserContract really is is a nice OOP abstraction wrapping a bunch of column names. It's crying out for being inlined at compile time - why not just have the test compile an identical class into itself to get at the same field names? In any case, not a vast cost, just a slightly annoying maintainence overhead if we end up adding more tables. > This is not about Robocop access to constants -- it's that getting rid of > that class will bite us later when we want to, e.g., grab one of those > constants in an add-on using JNI. I'm very happy to spend the additional > bytes to just keep *Constants unmolested. This makes sense. So addons can even do cool stuff that modify the constants using Reflection to do unspeakably awful things to the app. Delightful! (In reply to Richard Newman [:rnewman] from comment #77) > ::: mobile/android/base/Makefile.in > @@ +1245,5 @@ > > + $(DX) --dex --output=classes.dex jars-proguarded $(ANDROID_COMPAT_LIB) > > + > > +proguard: $(ALL_JARS) > > + @echo "Proguard" > > + java -jar $(ANDROID_SDK)/../../tools/proguard/lib/proguard.jar @$(topsrcdir)/mobile/android/config/proguard.cfg -injars jars -outjars jars-proguarded -libraryjars $(ANDROID_SDK)/android.jar:$(ANDROID_COMPAT_LIB) > > Is this the right path for all NDKs? > > Prefer $(ANDROID_SDK_ROOT)/tools/ instead of the relative path from the SDK. > (Assuming it's defined here.) > > gbrown might have more to say about this. Much tidier, but this environment variable doesn't seem to exist... Will await more info from gbrown. > @@ +65,5 @@ > > + > > +# Preserve all fundamental application classes. > > + > > +-keep public class * extends android.app.Activity > > +-keep public class * extends android.app.Application > > Duplicate block… Ah. Should've checked the earlier progaurd.cfg more carefully. > @@ +76,5 @@ > > + > > +-keep public class * extends android.view.View { > > + public <init>(android.content.Context); > > + public <init>(android.content.Context, android.util.AttributeSet); > > + public <init>(android.content.Context, android.util.AttributeSet, int); > > Also dupe…? Really? Not seeing a duplicate of this anywhere? > @@ +239,5 @@ > > +} > > + > > +# Google uses 5 passes, but each pass increases our build time by 5 seconds! > > +# The optimizer's returns diminish after ~2 passes and stop at ~9 passes. > > +-optimizationpasses 4 > > This sounds like something we want to parameterize from mozconfig, run more > on buildbots than locally, or similar. It does. Debug builds could run just one pass, for example. How on earth do we do this? That said.. There's an argument against this, too. Fewer passes delete less stuff. Deleting less stuff means a thing that creates a bug when it gets deleted might not get deleted on your low-number-of-passes debug build but may get deleted by the buildbot, creating an unreproducible bug. > @@ +245,5 @@ > > +# Obfuscation because it makes exception stack traces more difficult to read. > > +-dontobfuscate > > + > > +# Suppress warnings about missing descriptor classes. > > +-dontnote **,!ch.boye.**,!org.mozilla.gecko.sync.** > > What descriptor classes? This line was provided by cpeterson's original patch. So far as I can tell, it's to stop Proguard from creating the problem described here about descriptor classes: http://proguard.sourceforge.net/index.html#manual/troubleshooting.html Do we actually use Java's serialisation system? (eg. in Sync?) If not, we can safely cull serialisation fields (I notice we define serialVersionUID in a few places. Wasn't sure why this was being done.) A further thing we may want to consider is using Proguard to cull logging in release builds. This can probably most neatly be done with -assumenosideeffects on the appropriate logging functions (eg. We can assumenosideeffects on Log.e and it'll then, most probably, cull all such calls). Isn't logging rather expensive to run?

Comment on attachment 810255 [details] [diff] [review] Part 4 - Annotate members referenced by WebRTC's JNI code. Review of attachment 810255 [details] [diff] [review]: ----------------------------------------------------------------- How did you figure out which methods needed this annotation?

(In reply to Gian-Carlo Pascutto (:gcp) from comment #84) > How did you figure out which methods needed this annotation? Liberal use of grep, and testing WebRTC apps to play "spot the segfaults".

Comment on attachment 810255 [details] [diff] [review] Part 4 - Annotate members referenced by WebRTC's JNI code. I can live with that.

(In reply to Chris Kitching [:ckitching] from comment #83) > > > + > > > +-keep public class * extends android.view.View { > > > + public <init>(android.content.Context); > > > + public <init>(android.content.Context, android.util.AttributeSet); > > > + public <init>(android.content.Context, android.util.AttributeSet, int); > > > > Also dupe…? > > Really? Not seeing a duplicate of this anywhere? +# Preserve all classes that have special context constructors, and the +# constructors themselves. + +-keepclasseswithmembers class * { + public <init>(android.content.Context, android.util.AttributeSet, int); ? > It does. Debug builds could run just one pass, for example. How on earth do > we do this? Run different Proguard commands for debug builds, fiddling in Makefile.in. ifdef MOZ_DEBUG ... > That said.. There's an argument against this, too. Fewer passes delete less > stuff. Deleting less stuff means a thing that creates a bug when it gets > deleted might not get deleted on your low-number-of-passes debug build but > may get deleted by the buildbot, creating an unreproducible bug. Oh, I get it. But I'd say: if we're still getting positive returns at 5, 6, or so passes... that should be what we run with for release builds, rather than because it takes an extra five seconds. And debug builds... none? fewer? > > What descriptor classes? > > This line was provided by cpeterson's original patch. So far as I can tell, > it's to stop Proguard from creating the problem described here about > descriptor classes: > http://proguard.sourceforge.net/index.html#manual/troubleshooting.html That sounds like a real problem, rather than something we should muffle, no? Or does this really not affect us? > Do we actually use Java's serialisation system? (eg. in Sync?) No that I know of. (Even our dependencies don't seem to.) The tree has lots of classes that implement Serializable, but that doesn't mean we use it. > If not, we can safely cull serialisation fields (I notice we define > serialVersionUID in a few places. Wasn't sure why this was being done.) Probably because Eclipse does it automatically. > Isn't logging rather expensive to run? It can be, if overused, or if callers check for log levels frequently. Background Service's Logger class caches those lookups and drops log calls that won't be printed, but of course string concatenation still happens. We absolutely don't want to strip all logging in release builds. We might want to strip *some* logging in release builds. If you can prove a speed benefit from doing so, then I'll be more open to persuasion!

(In reply to Richard Newman [:rnewman] from comment #87) > (In reply to Chris Kitching [:ckitching] from comment #83) > > > > > + > > > > +-keep public class * extends android.view.View { > > > > + public <init>(android.content.Context); > > > > + public <init>(android.content.Context, android.util.AttributeSet); > > > > + public <init>(android.content.Context, android.util.AttributeSet, int); > > > > > > Also dupe…? > > > > Really? Not seeing a duplicate of this anywhere? > > +# Preserve all classes that have special context constructors, and the > +# constructors themselves. > + > +-keepclasseswithmembers class * { > + public <init>(android.content.Context, android.util.AttributeSet, int); > > ? Ah, this one is duplicated - but not with the one you pointed to in the first comment... > Oh, I get it. But I'd say: if we're still getting positive returns at 5, 6, > or so passes... that should be what we run with for release builds, rather > than because it takes an extra five seconds. And debug builds... none? fewer? This makes sense. With this version of the patch, the value is set in makefile based on if MOZ_DEBUG is set. I did attempt to add support for overriding this with a property in mozconfig, but it exhibited strange caching behaviour - changes to the config didn't always result in changes to the number of runs. This approach seems sufficient, in any case. People can still muck about with the number of runs by tweaking the makefile. > > > What descriptor classes? > > > > This line was provided by cpeterson's original patch. So far as I can tell, > > it's to stop Proguard from creating the problem described here about > > descriptor classes: > > http://proguard.sourceforge.net/index.html#manual/troubleshooting.html > > That sounds like a real problem, rather than something we should muffle, no? > > Or does this really not affect us? Ah! Yes, this was originally hiding spurious errors that were appearing because Proguard couldn't find the deinitions of some library classes (It'd still work, but since it couldn't find those classes it just blindly assumed they existed) But this is no longer correct - all the libraries needed are provided to Proguard, so such errors are no longer spurious. In fact, removing this line from the config flags up two extra annotations that probably ought to be added to earlier patches in this series. > > Do we actually use Java's serialisation system? (eg. in Sync?) > > No that I know of. (Even our dependencies don't seem to.) > > The tree has lots of classes that implement Serializable, but that doesn't > mean we use it. > > > If not, we can safely cull serialisation fields (I notice we define > > serialVersionUID in a few places. Wasn't sure why this was being done.) > > Probably because Eclipse does it automatically. How odd. I'll remove the configuration code to make it keep these, then. Should get us a teensy bit of extra space. > It can be, if overused, or if callers check for log levels frequently. > Background Service's Logger class caches those lookups and drops log calls > that won't be printed, but of course string concatenation still happens. > > We absolutely don't want to strip all logging in release builds. We might > want to strip *some* logging in release builds. If you can prove a speed > benefit from doing so, then I'll be more open to persuasion! Probably not worth the faff for now. Perhaps worthy of a followup - saving those concatenations might prove worthwhile, depending on how liberal people have been.

Adding an additional annotation for something that conveniently wasn't being optimied out, but might be, and was being hidden by the warning suppression line Newman spotted.

As above.

Comment on attachment 810253 [details] [diff] [review] Part 2 - Shunt, as appropriate, methods into RobocopAPI, where possible. Review of attachment 810253 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.

What happens if someone forget to annotate methods properly? Do we get an error at build time? Or does it introduce some subtle runtime bug or crash? Are these issues easy to spot or debug? The manual nature of this annotation-based approach looks a bit too error-prone to me. Am I missing something?

As far as I understand, forgetting to annotate a method causes it not to be found at runtime. This may cause either crashes or hopefully-not-so-silent errors. If the problem is in a codepath that's not exercised by tests, it will be caught late (but then again, you have other problems in that case). We're annotating manually because the automated tools fail in light of JNI calls and reflection. What's the final gain we get by doing this? APK size, startup and page load performance?

Further to gcp's answer: (In reply to Lucas Rocha (:lucasr) from comment #92) > What happens if someone forget to annotate methods properly? Do we get an > error at build time? Or does it introduce some subtle runtime bug or crash? > Are these issues easy to spot or debug? The only reasons we need to annotate methods at all are: * Robocop tests * JNI calls * Other uses of reflection (e.g., JS add-ons calling through js-ctypes into JNI into Java). The first is not a concern: you'll get obvious test failures. The second is mostly addressed by automatically generating wrappers. For situations it which it's not, there's a fairly small set. And anything we missed should fail -- typically with a segfault -- as soon as that call path is exercised... which should be obvious, because we test our code, right? If we don't, it should be an obvious crash report. The third we can help as best we can (e.g., by annotating whole constant classes, rather than just the attributes that are currently in use), but again, the failures will be immediately obvious at dev time due to an inability to reflect on removed attributes or classes. > The manual nature of this annotation-based approach looks a bit too > error-prone to me. Am I missing something? Without having a really awesome tracing system (better than PGO), manual annotation is the only thing we can do, given that we support JNI access to our Java codebase. On the plus side, we aren't in the habit of rapidly growing our JNI-based code, so this should be quite stable. (In reply to Gian-Carlo Pascutto (:gcp) from comment #93) > What's the final gain we get by doing this? APK size, startup and page load > performance? We'll save ~500KB of APK size, and non-trivial CPU time (apparently 10+%) everywhere else, including in page load, which will translate to visible snappiness improvements. Our memory footprint and cache impacts will probably decrease, too, but I haven't seen any measurements of that. We've basically been shipping debug code until now, because our compilers don't do the kinds of inlining and code-eliminating improvements that we expect from modern compilers -- for Android, ProGuard *is* that modern optimizer.

(In reply to Lucas Rocha (:lucasr) from comment #92) > What happens if someone forget to annotate methods properly? Do we get an > error at build time? Or does it introduce some subtle runtime bug or crash? > Are these issues easy to spot or debug? > > The manual nature of this annotation-based approach looks a bit too > error-prone to me. Am I missing something? Richard's done a pretty good job of answering this, but just going to add a few things... In a limited number of cases, Proguard will print a "Note:" line at compile time complaining about a missing class (This happens in the case that an entry point is kept, but its dependancies are not). The symptom of your code being deleted when you don't want it to be is a segfault with address 0xdeadd00d as soon as you run the codepath - if you test your code even slightly this will be extremely obvious. It's for this reason that I suggest we run Proguard for at least one iteration, even on debug builds. (To avoid the annoying scenario where both coder and reviewer are using debug builds and don't see the problem until it lands and segfaults - although a single try run would also be enough to enitrely obviously spot the problem, assuming an appropriate test exists..) It's not something you're going to be able to miss unless you're doing essentially no testing your code - in which case missing an annotation is probably the least of your concerns. I'd like to see more work towards reducing the amount of manual annotations we need - the generator covers as many cases as I was able to work out this summer - it'd be great to see someone familiar with the WebRTC code expanding the scope of the generator so it can render Part 4 of this patch obsolete. This is something that seems possible, but the structure of the WebRTC codebase isn't something I've yet sat down and tried to work out. There is ongoing work to replace the Robocop tests that need Part 3 with JUnit tests (Which are better in essentially every way) - so in a few months that should also be obsolete. Robocop's not well-suited to this kind of testing anyway. The remainder is, mostly, GeckoEvent and entry points from JS into Java using the JNI. The latter could be generated, but I didn't bother since there's currently only two of them - everything else uses the message-passing system. On a related note - depending on what horror has been done to make the JNI work in JS, it might actually turn out that using it instead of the message-passing system is the solution we're looking for in Bug 903553. If we do that, it'll become worthwhile writing a generator with comparable features to the one already in place that can generate JS code. (Or expanding the existing one). As for GeckoEvent - it could be covered by the generator, but the refactoring needed to make it work seemed like it would actually make the C++ code more messy. In general, the switch to generated code cleaned up the C++ code - I decided the cost of doing all the faffing about required wasn't worth it. It seems unlikely that we'll ever be wanting to remove GeckoEvent's annotation, anyway. If someone wants to go do the crazy refactoring needed to make it work, go for it. Finally - it is, in principle, possible and of noninsane difficulty to make a compile-time checker that will determine if something has been deleted when it needs to not be. Assuming: All getMethodID/getFieldID etc. calls in code using JNI to call Java code accept only constants as arguments (ie. No procedurally determining the name of the target you want to call) Then such a checker would operate as follows: Grep (Or such) the C++ code for calls to getMethodId/getFieldId/getClass etc. Parse the resulting line and extract the classname and canonical signature of the thing that's being targeted. Use the iterator in the generator's classloader class to iterate over the post-Proguard jars looking for the stuff that's targeted. Whinge if something you wanted isn't there. Actually, this sounds quite straightforward. I shall do my utmost to find time before Sunday (When I return to Cambridge and will become somewhat more high-latency) to implement this improvement.

Comment on attachment 811915 [details] [diff] [review] Part 1 - V3 - Annotate by hand the JNI entry points not covered by the code generator. Review of attachment 811915 [details] [diff] [review]: ----------------------------------------------------------------- As far as I can see, the most recent try push noted here is from Comment 70: > Try push: https://tbpl.mozilla.org/?tree=Try&rev=744fe1a2d2a7 It has a consistent failure in Robocop testJarReader that seems related. (But keep in mind it's not the most recent patches.) I don't see anything wrong with the patch, but would like to see a solid try run.

Comment on attachment 810253 [details] [diff] [review] Part 2 - Shunt, as appropriate, methods into RobocopAPI, where possible. Review of attachment 810253 [details] [diff] [review]: ----------------------------------------------------------------- Be aware that testPasswordEncrypt is currently disabled -- those changes will not be tested until someone tries to re-enable that test. r- simply for the comment re-wording. ::: mobile/android/base/RobocopAPI.java @@ +5,5 @@ > package org.mozilla.gecko; > > +import android.content.Context; > +import android.os.SystemClock; > +import android.util.Log; It looks to me like these imports are not necessary. @@ +33,5 @@ > + * > + * Robocop is for blackbox UI testing. Unfortunately, we have a number of unit tests written using it. > + * There is ongoing work to port these to JUnit. This class is simply provided to ease the transition > + * until that work is completed. Don't make things worse. > + */ I hear your frustration, but it's not necessary to express it here. I would like you to give thought to saying what you need to here (like encouraging people to consider other frameworks for non-UI tests, or suggesting that use of reflection be minimized) in a more positive manner. Keep in mind that these tests have been written over time, to meet various needs with the tools available at the time, and generally with very good intentions. I am also confused by the content of the comment, and fear others may be confused also. I agree that some Robocop tests would be better suited to a unit test framework, reducing the need for reflection in Robocop. However reflection has also been used successfully for other Robocop features, and various mobile team members have provided compelling reasons to use reflection in Robocop. Consider: - using checkLaunchState to guard against startup races - using loadUrl instead of keystrokes to improve speed and reliability of tests that are not explicitly testing key events or url entry - using the PanningPerfAPI to efficiently retrieve and report metrics for Talos tests. I am concerned that too much faith is being put in JUnit before its applicability to these issues has been proven. @@ +93,5 @@ > + return NSSBridge.decrypt(context, profilePath, aValue); > + } > + > + // Tabs. > + public static Tabs getInstance() { nit -- Maybe this should be named getTabsInstance(), or something like that.

Comment on attachment 810254 [details] [diff] [review] Part 3 - Annotate the remaining Robocop entry points. Review of attachment 810254 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/gfx/LayerRenderer.java @@ -298,5 @@ > mFramesRendered = 0; > mCompleteFramesRendered = 0; > } > > - /** Used by robocop for testing purposes. Not for production use! */ Why remove these comments? In this case (and the other one in this file), I think the author wanted to discourage use of this function for anything but testing.

Comment on attachment 811914 [details] [diff] [review] Part 4 - V2 - Annotate members referenced by WebRTC's JNI code. Review of attachment 811914 [details] [diff] [review]: ----------------------------------------------------------------- LGTM (but I'm not familiar with this code).

Comment on attachment 811913 [details] [diff] [review] Part 5 - V2 - Activate Proguard. Review of attachment 811913 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Chris Kitching [:ckitching] from comment #83) >(In reply to Richard Newman [:rnewman] from comment #77) >> ::: mobile/android/base/Makefile.in >> @@ +1245,5 @@ >> > + $(DX) --dex --output=classes.dex jars-proguarded $(ANDROID_COMPAT_LIB) >> > + >> > +proguard: $(ALL_JARS) >> > + @echo "Proguard" >> > + java -jar $(ANDROID_SDK)/../../tools/proguard/lib/proguard.jar @$(topsrcdir)/mobile/android/config/proguard.cfg -injars jars -outjars jars-proguarded -libraryjars $(ANDROID_SDK)/android.jar:$(ANDROID_COMPAT_LIB) >> >> Is this the right path for all NDKs? >> >> Prefer $(ANDROID_SDK_ROOT)/tools/ instead of the relative path from the SDK. >> (Assuming it's defined here.) >> >> gbrown might have more to say about this. > > Much tidier, but this environment variable doesn't seem to exist... Will await more info from gbrown. Sorry, I don't know the "right" thing to do here. :gps -- can you help us out?

(In reply to Geoff Brown [:gbrown] from comment #100) > > Much tidier, but this environment variable doesn't seem to exist... Will await more info from gbrown. WebRTC and some toolkit runtime stuff uses this from the env. E.g., media/webrtc/trunk/build/android/emulator.py 142: android_sdk_root = os.environ['ANDROID_SDK_ROOT'] toolkit/crashreporter/google-breakpad/android/common-functions.sh 225: if [ -z "$_ADB" -a "$ANDROID_SDK_ROOT" ]; then Depending on how gps feels, this might be something we should add alongside ANDROID_SDK, or compute once from ANDROID_SDK if the structure is guaranteed. If it's a pain in the ass, though, I'm not bothered if we plow ahead without.

Comment on attachment 811913 [details] [diff] [review] Part 5 - V2 - Activate Proguard. Review of attachment 811913 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/Makefile.in @@ +1245,5 @@ > # Sync dependencies are provided in a single jar. Sync classes themselves are delivered as source, > # because Android resource classes must be compiled together in order to avoid overlapping resource > # indices. > + > +classes.dex: $(ALL_JARS) proguard Isn't it enough here to have this be classes.dex: proguard ? (Perhaps renaming that to `proguard-jars`.) @@ +1257,5 @@ > +endif > + > +proguard: $(ALL_JARS) > + @echo "Proguard" > + @echo "$(PROGUARD_PASSES)" @echo "Running $(PROGUARD_PASSES) ProGuard passes." @@ +1258,5 @@ > + > +proguard: $(ALL_JARS) > + @echo "Proguard" > + @echo "$(PROGUARD_PASSES)" > + @echo java -jar $(ANDROID_SDK)/../../tools/proguard/lib/proguard.jar @$(topsrcdir)/mobile/android/config/proguard.cfg -optimizationpasses $(PROGUARD_PASSES) -injars jars -outjars jars-proguarded -libraryjars $(ANDROID_SDK)/android.jar:$(ANDROID_COMPAT_LIB) I have a vague feeling here that -injars jars should agree with $(ALL_JARS). What would happen if ALL_JARS suddenly grew a jar that *wasn't* in /jars/? Or if /jars/ contains jars that aren't in ALL_JARS? @@ +1259,5 @@ > +proguard: $(ALL_JARS) > + @echo "Proguard" > + @echo "$(PROGUARD_PASSES)" > + @echo java -jar $(ANDROID_SDK)/../../tools/proguard/lib/proguard.jar @$(topsrcdir)/mobile/android/config/proguard.cfg -optimizationpasses $(PROGUARD_PASSES) -injars jars -outjars jars-proguarded -libraryjars $(ANDROID_SDK)/android.jar:$(ANDROID_COMPAT_LIB) > + java -jar $(ANDROID_SDK)/../../tools/proguard/lib/proguard.jar @$(topsrcdir)/mobile/android/config/proguard.cfg -optimizationpasses $(PROGUARD_PASSES) -injars jars -outjars jars-proguarded -libraryjars $(ANDROID_SDK)/android.jar:$(ANDROID_COMPAT_LIB) Can we do this (print then run) without including the same command string twice? ::: mobile/android/config/proguard.cfg @@ +1,5 @@ > +# Dalvik renders preverification unuseful (Would just slightly bloat the file). > +-dontpreverify > + > +# Uncomment to have Proguard list dead code detected during the run - useful for cleaning up the codebase. > +# -printusage How verbose is this? If "not much", this is nice stuff to have in the build logs. @@ +130,5 @@ > + @org.mozilla.gecko.mozglue.JNITarget *; > +} > + > +# Keep classes which contain at least one annotated element. Split over two directives > +# becuase, according to the developer of ProGuard, "the option -keepclasseswithmembers because

Flying visit - will be addressing the concerns raised in full when time permits (Next two days should provide a chance to turn these patches round). (In reply to Geoff Brown [:gbrown] from comment #97) > Be aware that testPasswordEncrypt is currently disabled -- those changes > will not be tested until someone tries to re-enable that test. How should we address this? Just land this anyway and be aware that we made this change if/when testPasswordEncrypt subsequently fails? > ::: mobile/android/base/RobocopAPI.java > @@ +5,5 @@ > > package org.mozilla.gecko; > > > > +import android.content.Context; > > +import android.os.SystemClock; > > +import android.util.Log; > > It looks to me like these imports are not necessary. Well spotted. > @@ +33,5 @@ > > + * > > + * Robocop is for blackbox UI testing. Unfortunately, we have a number of unit tests written using it. > > + * There is ongoing work to port these to JUnit. This class is simply provided to ease the transition > > + * until that work is completed. Don't make things worse. > > + */ > > I hear your frustration, but it's not necessary to express it here. I would > like you to give thought to saying what you need to here (like encouraging > people to consider other frameworks for non-UI tests, or suggesting that use > of reflection be minimized) in a more positive manner. Keep in mind that > these tests have been written over time, to meet various needs with the > tools available at the time, and generally with very good intentions. I am very sorry if this gave the impression of questioning the intentions of prevoius work. Of course it's not at all uncommon for code that's a good idea now to become a terrible idea in the future when requirements change or new techniques appear. Code becoming obsolete is usually a good thing - given suitable improvements in the Java compiler this whole bug just vanishes, for example. That'd e awesome. In my opinion, sugar-coating observations of this sort seems an inefficient thing to do. My intention when writing this comment was to try and get the point across that additional use of Reflection needs to be carefully considered. While sometimes appropriate, the associated cost is extra annoying annotations to maintain and a possible performance reduction (Since Proguard gets to optimise less stuff) - things we want to keep to a minimum. It seemed important to make it very obvious to people that "Just tacking on a new robocop test" may well not be the best, or even the easiest, approach. I'll reword the comment in a more "How to get what you want easily" instead of "What not to do" way. > I am also confused by the content of the comment, and fear others may be > confused also. I agree that some Robocop tests would be better suited to a > unit test framework, reducing the need for reflection in Robocop. However > reflection has also been used successfully for other Robocop features, and > various mobile team members have provided compelling reasons to use > reflection in Robocop. Consider: > - using checkLaunchState to guard against startup races > - using loadUrl instead of keystrokes to improve speed and reliability of > tests that are not explicitly testing key events or url entry > - using the PanningPerfAPI to efficiently retrieve and report metrics for > Talos tests. > > I am concerned that too much faith is being put in JUnit before its > applicability to these issues has been proven. I'd not seen these reasons - they are indeed compelling. It's because I'd not been able to, with absolute certainty, determine that Reflection is bad in all cases (And now it seems to be quite useful in a limited number of cases), that I opted to annotate the things Robocop needs instead of attempting to eliminate its use of Reflection entirely. It now seems that we'll always need some things annotated, but that we might be able to productively reduce the number of things needing to be annotated with future work - certainly all of the above seem quite sensible uses for Reflection. Perhaps our final destination, then, would be a RobocopAPI class which wraps all of the things we need to handle the cases you mentioned (And other similarly interesting ones), with no other annotations present. As for the strengths/weakness of JUnit for our particular unit testing needs, that's something I think RNewman has much more experience with than I. > @@ +93,5 @@ > > + return NSSBridge.decrypt(context, profilePath, aValue); > > + } > > + > > + // Tabs. > > + public static Tabs getInstance() { > > nit -- Maybe this should be named getTabsInstance(), or something like that. Of course - multiple singletons exist in the world. The old way would get confusing fast. (In reply to Richard Newman [:rnewman] from comment #102) > ::: mobile/android/base/Makefile.in > @@ +1245,5 @@ > > # Sync dependencies are provided in a single jar. Sync classes themselves are delivered as source, > > # because Android resource classes must be compiled together in order to avoid overlapping resource > > # indices. > > + > > +classes.dex: $(ALL_JARS) proguard > > Isn't it enough here to have this be > > classes.dex: proguard > > ? (Perhaps renaming that to `proguard-jars`.) Ah yes. Since proguard has ALL_JARS as a dependancy this ought to work. > @@ +1258,5 @@ > > + > > +proguard: $(ALL_JARS) > > + @echo "Proguard" > > + @echo "$(PROGUARD_PASSES)" > > + @echo java -jar $(ANDROID_SDK)/../../tools/proguard/lib/proguard.jar @$(topsrcdir)/mobile/android/config/proguard.cfg -optimizationpasses $(PROGUARD_PASSES) -injars jars -outjars jars-proguarded -libraryjars $(ANDROID_SDK)/android.jar:$(ANDROID_COMPAT_LIB) > > I have a vague feeling here that > > -injars jars > > should agree with $(ALL_JARS). What would happen if ALL_JARS suddenly grew a > jar that *wasn't* in /jars/? Or if /jars/ contains jars that aren't in > ALL_JARS? Unclear. ALL_JARS will probably work, but presumably if either of the cases you mentioned actually happen then we're doing something rather strange. > @@ +1259,5 @@ > > +proguard: $(ALL_JARS) > > + @echo "Proguard" > > + @echo "$(PROGUARD_PASSES)" > > + @echo java -jar $(ANDROID_SDK)/../../tools/proguard/lib/proguard.jar @$(topsrcdir)/mobile/android/config/proguard.cfg -optimizationpasses $(PROGUARD_PASSES) -injars jars -outjars jars-proguarded -libraryjars $(ANDROID_SDK)/android.jar:$(ANDROID_COMPAT_LIB) > > + java -jar $(ANDROID_SDK)/../../tools/proguard/lib/proguard.jar @$(topsrcdir)/mobile/android/config/proguard.cfg -optimizationpasses $(PROGUARD_PASSES) -injars jars -outjars jars-proguarded -libraryjars $(ANDROID_SDK)/android.jar:$(ANDROID_COMPAT_LIB) > > Can we do this (print then run) without including the same command string > twice? Ah! This was supposed to be removed prior to uploading the patch. This is why patches submitted from airports are unwise. If you like we can print it anyway cleaned up as you propose. It might be better to just print "Running X Proguard passes" or such, or nothing at all (Proguard does a fair bit of printing itself, although it doesn't print the total number of passes it is to attempt.) > ::: mobile/android/config/proguard.cfg > @@ +1,5 @@ > > +# Dalvik renders preverification unuseful (Would just slightly bloat the file). > > +-dontpreverify > > + > > +# Uncomment to have Proguard list dead code detected during the run - useful for cleaning up the codebase. > > +# -printusage > > How verbose is this? If "not much", this is nice stuff to have in the build > logs. Extremely, very, a lot. It prints out the name of every Java member that Proguard decided *not* to delete, if memory serves. It's useful for analysing why/if it's keeping something you're not expecting it to, but for general use it's almost certainly way too verbose.

(In reply to Richard Newman [:rnewman] from comment #102) > @@ +1258,5 @@ > > + > > +proguard: $(ALL_JARS) > > + @echo "Proguard" > > + @echo "$(PROGUARD_PASSES)" > > + @echo java -jar $(ANDROID_SDK)/../../tools/proguard/lib/proguard.jar @$(topsrcdir)/mobile/android/config/proguard.cfg -optimizationpasses $(PROGUARD_PASSES) -injars jars -outjars jars-proguarded -libraryjars $(ANDROID_SDK)/android.jar:$(ANDROID_COMPAT_LIB) > > I have a vague feeling here that > > -injars jars > > should agree with $(ALL_JARS). What would happen if ALL_JARS suddenly grew a > jar that *wasn't* in /jars/? Or if /jars/ contains jars that aren't in > ALL_JARS? Turns out that Proguard requires a colon-seperated list of paths here, be they directories containing jars or jars themselves. Since ALL_JARS is \n seperated, we'd need to do some makefile magic to s/\n/:/g in ALL_JARS to use it here. Since the scenario in which we have spurious jars as you mention is currently impossible, and would only become possible if we restructured our build system in a way that seems quite strange at the moment, do we care enough to solve this problem? (In reply to Richard Newman [:rnewman] from comment #101) > (In reply to Geoff Brown [:gbrown] from comment #100) > > > > Much tidier, but this environment variable doesn't seem to exist... Will await more info from gbrown. > > WebRTC and some toolkit runtime stuff uses this from the env. E.g., > > media/webrtc/trunk/build/android/emulator.py > 142: android_sdk_root = os.environ['ANDROID_SDK_ROOT'] > > toolkit/crashreporter/google-breakpad/android/common-functions.sh > 225: if [ -z "$_ADB" -a "$ANDROID_SDK_ROOT" ]; then > > Depending on how gps feels, this might be something we should add alongside > ANDROID_SDK, or compute once from ANDROID_SDK if the structure is > guaranteed. If it's a pain in the ass, though, I'm not bothered if we plow > ahead without. Nevertheless, adding a call to `env` in Makefile.in reveals that ANDROID_SDK_ROOT is not in scope there. This revised patch makes changes to build/autoconf/android.m4 and friends that make this work - I *think* this is correct, but my familiarity with Makefile is low. For reference, ANDROID_SDK_ROOT=$ANDROID_SDK+../../ should always hold. (And is already implicitly assumed by the build system). Everything else from the applicable reviews has, I think, been applied as suggested.

Cleanup and sugar coating.

(In reply to Geoff Brown [:gbrown] from comment #98) > Why remove these comments? In this case (and the other one in this file), I > think the author wanted to discourage use of this function for anything but > testing. Whoops - let's put that back in. Got a bit carried away - seemed sensible to remove the "This is referenced by robocop via reflection" comments in cases where an annotation is being added which has the same meaning. This one was caught in the crossfire.

Comment on attachment 814958 [details] [diff] [review] Part 2 - V2 - Shunt, as appropriate, methods into RobocopAPI, where possible. Review of attachment 814958 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Chris Kitching [:ckitching] from comment #103) > (In reply to Geoff Brown [:gbrown] from comment #97) > > Be aware that testPasswordEncrypt is currently disabled -- those changes > > will not be tested until someone tries to re-enable that test. > > How should we address this? Just land this anyway and be aware that we made > this change if/when testPasswordEncrypt subsequently fails? Under the circumstances, that's probably all we can do. Thanks for the comment change -- I think it's better now. ::: mobile/android/base/RobocopAPI.java @@ +96,5 @@ > + return NSSBridge.decrypt(context, profilePath, aValue); > + } > + > + // Tabs. > + public static Tabs getInstance() { Did you decide not to change this?

Comment on attachment 814957 [details] [diff] [review] Part 5 - V3 - Activate Proguard. Review of attachment 814957 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/RobocopAPI.java @@ +96,5 @@ > return NSSBridge.decrypt(context, profilePath, aValue); > } > > // Tabs. > + public static Tabs getTabsInstance() { Oh, I see it now!

Comment on attachment 814957 [details] [diff] [review] Part 5 - V3 - Activate Proguard. Review of attachment 814957 [details] [diff] [review]: ----------------------------------------------------------------- Let's add a comment above the definition of ALL_JARS to say that it must agree with jars/*.jar, explaining why. In fact, perhaps we should glob rather than listing directly? ::: js/src/build/autoconf/android.m4 @@ +304,5 @@ > if test -z "$android_build_tools" ; then > android_build_tools="$android_platform_tools" # SDK Tools < r22 > fi > ANDROID_SDK="${android_sdk}" > + ANDROID_SDK_ROOT="${android_sdk}"/../../ You probably ought to do this up around line 259: --- MOZ_ARG_WITH_STRING(android-sdk, [ --with-android-sdk=DIR location where the Android SDK can be found (base directory, e.g. .../android/platforms/android-6)], android_sdk=$withval) --- making this line: ANDROID_SDK_ROOT="${android_sdk_root}" and using the lowercase version in the test below. This lets you tidy up (and get the benefit of validation from) lines like: android_tools="$android_sdk"/../../tools android_platform_tools="$android_sdk"/../../platform-tools if test ! -d "$android_platform_tools" ; then android_platform_tools="$android_sdk"/tools # SDK Tools < r8 fi

Flagging myself to return to this and add the compile-time checking patch after the stuff over in Bug 913985 is all sorted out. Not much point continuing work here until then, as the bitrot will just create more work.

Unbitrotting the patch stack as I pass them to get to the top to do my new work...

Looks like *.java.in has gone away. Excellent.

Looks like someone rearranged most of WebRTC while I wasn't looking...

Merging getTabsInstance method name change into the correct patch..

(In reply to Richard Newman [:rnewman] from comment #109) > Comment on attachment 814957 [details] [diff] [review] > Part 5 - V3 - Activate Proguard. > > Review of attachment 814957 [details] [diff] [review]: > ----------------------------------------------------------------- > > Let's add a comment above the definition of ALL_JARS to say that it must > agree with jars/*.jar, explaining why. In fact, perhaps we should glob > rather than listing directly? > > ::: js/src/build/autoconf/android.m4 > @@ +304,5 @@ > > if test -z "$android_build_tools" ; then > > android_build_tools="$android_platform_tools" # SDK Tools < r22 > > fi > > ANDROID_SDK="${android_sdk}" > > + ANDROID_SDK_ROOT="${android_sdk}"/../../ > > You probably ought to do this up around line 259: > > --- > MOZ_ARG_WITH_STRING(android-sdk, > [ --with-android-sdk=DIR > location where the Android SDK can be found (base > directory, e.g. .../android/platforms/android-6)], > android_sdk=$withval) > --- > > making this line: > > ANDROID_SDK_ROOT="${android_sdk_root}" > > and using the lowercase version in the test below. > > This lets you tidy up (and get the benefit of validation from) lines like: > > android_tools="$android_sdk"/../../tools > android_platform_tools="$android_sdk"/../../platform-tools > if test ! -d "$android_platform_tools" ; then > android_platform_tools="$android_sdk"/tools # SDK Tools < r8 > fi It's been perhaps a little too long since I last looked through this stuff. Makefile is *weird*. Still, I think this is what you were after...

Proguard.cfg somehow managed to vanish from V4...

Out of time for tonight - just going to leave this here and see if anyone thinks it's a remotely sane idea. Hopefully, something like this should enable the detection of most missing annotations at build time. Detecting missing things at runtime, particularly given our patchy test coverage, is liable to be problematic.

Ack, missed a qrefresh! Also, new problem: the build system has changed where the jars go. Previously, the jars were all placed in a directory called jars. All proguard did was use that directory as input and a new jars-proguarded directory as output. The jars directory no longer exists - all the jars are just dumped into the directory that was the parent of jars when it existed. Proguard does not allow overwriting the input with the output, and requires multiple jars to be colon-delimited on the command line, so we can't directly use the ALL_JARS value to refer to them. This now becomes sort of fun. Proguard interprets its input similarly to how Java interprets a classpath option - as a colon-delimited list of jars, directories, or classfiles to process. In particular, the substring :: is interpreted as "Include also the current directory" - which we want to avoid, because this directory also includes directories containing the class files (In subdirectories inconsistent with their packages when viewed from this angle, because they're grouped by originating jar not by package location). A simple substitution of spaces to colons creates such a double colon token because of the way the webrtc jar is tacked onto the existing ALL_JARS value with an extra space. Making this work by pruning the trailing space from ALL_JARS seems unwise, so a substitution of ::->: is applied. Because makefile is craaaazy, doing a substitution for spaces to colons required a spectacular hack (Advocated by the manual, astonishingly). So here you go... Finally, since gbrown asked for it a while ago and I never got to it, and since it seems really likely something somewhere will have changed in past two months that'll break my patch: https://tbpl.mozilla.org/?tree=Try&rev=088f71d60146 Let's see where that thing is. Brian did a try run of all the stuff from Bug 913985 a couple of days ago, so those patches can be assumed to be okay (They're just yet to land). Sorry for the spam, Newman.

Got this working at last. Detects missing annotations a reasonably large amount of the time - should make maintaining this less painful. Also, the try run has gone orange for reasons that are unclear. The thing that testJarReader is claiming it can't find *does* exist when you decompile the jar, and is annotated. And hasn't changed for weeks. Hmm.

It *really* does work better if you qrefresh the config file in... (Although this is apparently not the cause of the try sadness.)

Yay! Bug 916507 appears to have both removed most of the reflection from Robocop tests and presented a simple approach for doing so elsewhere. This kills of much of the problematic code, makes Part 2 and Part 3 of this patch series mostly redundant, and is generally awesome. Excellent. Let's land that one first, when the trees ever open again. Thanks, Comella! Unfortunately this introduces some subtleties. If you compile Fennec with Proguard, then compile the new robocop (Which just sticks the Fennec classes in the bootclasspath) it'll fail to find a bunch of things because they'll have been eliminated. It is now necessary to have Proguard analyse the Robocop code at the same time it's processing Fennec if you want a build that can be Robocop'd (Proguard will be able to work out exactly what needs keeping based on what's referenced) There are two obvious approaches: 1) Compile both Fennec and Robocop at the same time, have Proguard optimise them both. Because all the references between the two will be understood by the optimiser it'll be able to not break anything. But - this means the test suite is being optimised - maybe this isn't a good idea? 2) Compile Fennec with an already-compiled Robocop jar file in the library path. This should caused Proguard to keep everything in Fennec referenced by Robocop, without the need for annotations. The problem here is how do you obtain this compiled Robocop jar file? You have to compile Fennec without Proguard, then compile Robocop, then compile Fennec again with Proguard using the robocop jar you produced earlier. Ew. It's not clear how best to proceed. Option 1 seems preferable to me. (And for release builds you need simply not include the Robocop jars in the optimisation step and Proguard will strip out all the Robocop stuff that's not needed any more). I suspect I need to get some approval from others before I start trying this sort of radical build system change.

Removing the annotations that are no longer strictly necessary due to Robocop changes.

So, the try failures, then: rpr: Problematic Reflection removed by Bug 916507. R4: Intermittent. (Probably. I don't have clearance to retrigger, but the other two worked and the error is nonsensical, so it looks like it). M6: Changes to the use of Reflection in WebRTC land added a new thing that needed annotating. (This patch) RC2: Problematic Reflection removed by Bug 916507. RC3: Problematic Reflection removed by Bug 916507. RC4: Problematic Reflection removed by Bug 916507. RC: Seems to be the same problem as M6. The remaining problem is that of making the Robocop and Fennec compilation steps work together correctly so that both are actually able to build and run. It occurs that Robocop could use the unproguarded jars built for Fennec, then Fennec can use Robocop to make the proguarded jars, then both can dex whenever is convenient.

(In reply to Chris Kitching [:ckitching] from comment #123) > 1) Compile both Fennec and Robocop at the same time, have Proguard optimise > them both. Because all the references between the two will be understood by > the optimiser it'll be able to not break anything. > But - this means the test suite is being optimised - maybe this isn't a good > idea? Presumably we'd be able to provide appropriate Proguard configuration for the test code to have no optimizations performed. > 2) Compile Fennec with an already-compiled Robocop jar file in the library > path. This should caused Proguard to keep everything in Fennec referenced by > Robocop, without the need for annotations. > The problem here is how do you obtain this compiled Robocop jar file? You > have to compile Fennec without Proguard, then compile Robocop, then compile > Fennec again with Proguard using the robocop jar you produced earlier. > Ew. Isn't this pretty much identical to the first option but with a clearer separation between what's compiled and what's Proguarded? Compile Fennec, compile Robocop, Proguard Fennec + robocop.jar, rather than compile Fennec + Robocop, Proguard Fennec + Robocop? I don't think it's necessary to _compile_ Fennec again just to Proguard it. Proguard works on class files. The main downside to this in my mind is that -- just as with the annotation approach -- we're shipping code that is less optimized than it could be, but other than that I think this interleaving approach is fine. > I suspect I need to get some approval from others before I start trying this > sort of radical build system change. Well, if we can avoid annotations, that's a win. And I don't think this dramatically adds complexity -- it just more clearly delineates the various processing steps, right?

(In reply to Richard Newman [:rnewman] from comment #126) > Isn't this pretty much identical to the first option but with a clearer > separation between what's compiled and what's Proguarded? Compile Fennec, > compile Robocop, Proguard Fennec + robocop.jar, rather than compile Fennec + > Robocop, Proguard Fennec + Robocop? > > I don't think it's necessary to _compile_ Fennec again just to Proguard it. > Proguard works on class files. Er, yes. You're right. So the question is do we want to optimise robocop itself, or do we want to just use the robocop jar to ensure we don't delete things from Fennec that we shouldn't do? In the latter case, it'd be sufficient to compile robocop and pass it as a library jar to proguard. I think. > The main downside to this in my mind is that -- just as with the annotation > approach -- we're shipping code that is less optimized than it could be, but > other than that I think this interleaving approach is fine. Okay. This seems consistent with the existing choice to not ship different code to what we test. > Well, if we can avoid annotations, that's a win. And I don't think this > dramatically adds complexity -- it just more clearly delineates the various > processing steps, right? Maybe? I'm actually not sure how to go about implementing what I've described. How can the makefile in build/mobile/robocop be combined with the one from mobile/android/base to acheive this? Are inter-makefile dependencies a thing? Certainly, this is most likely preferable to the annotation approach - I just don't know how to do it yet.

(In reply to Chris Kitching [:ckitching] from comment #127) > In the latter case, it'd be sufficient to compile robocop and pass it as a > library jar to proguard. I think. That works for me. I don't see a pressing need to optimize it right now -- unlikely to offer wins, and likely to make tests very confused. > Maybe? I'm actually not sure how to go about implementing what I've > described. How can the makefile in build/mobile/robocop be combined with the > one from mobile/android/base to acheive this? Are inter-makefile > dependencies a thing? Answering these questions is why we employ people who understand Make :D

(In reply to Richard Newman [:rnewman] from comment #128) > (In reply to Chris Kitching [:ckitching] from comment #127) > > > In the latter case, it'd be sufficient to compile robocop and pass it as a > > library jar to proguard. I think. > > That works for me. I don't see a pressing need to optimize it right now -- > unlikely to offer wins, and likely to make tests very confused. This makes sense. Unfortunately my ideas about what Proguard does with a jar passed as a "Library jar" were slightly incorrect. It appears Proguard does not consider calls going from library code back into the program - perhaps sane given the nomenclature, but irritating. The way to get calls between different classes preserved are, it appears, either explicitly via a keep directive, or to pass both caller and callee as input to the optimiser. So, to make this work without needing to bring back the bucketload of annotations Comella almost-but-not-quite enabled us get rid of for free we do have to optimise Robocop. At least, insofar as we have to pass the Robocop jars to Proguard at the same time we pass the Fennec jars. So, after some jiggery-pokery I have this approach. Some kinks exist to be ironed out (Only just got this result, but working theory is that for some strange reason some of Robocop harness *isn't* in o.m.g.tests but is instead in o.m.g, which just seems bizzaire (And exposes it to being eaten.)): https://tbpl.mozilla.org/?tree=Try&rev=4d54617fa94b The idea is to feed the tests to the optimiser, but instruct the optimiser not to delete, inline, or rename anything in the org.mozilla.gecko.tests package. This prevents Proguard from mutilating our tests (And in any case, Proguard's mutilations preserve line number tables, I believe, so any intra-procedural optimisations it does apply should be invisible to a user attempting to debug test failures in the usual way, assuming no Proguard bugs.), but makes Proguard aware of the references to program code that the tests make. As a result, nothing the tests need is optimised out (Or if it is, it's inlined into the tests as well). I *think* this is sane? It'd be very nice if it can be made to work - no need to manually maintain annotations for Robocop at all! (But a few for now until the work to eliminate Reflection from the tests is totally completed) > > Maybe? I'm actually not sure how to go about implementing what I've > > described. How can the makefile in build/mobile/robocop be combined with the > > one from mobile/android/base to acheive this? Are inter-makefile > > dependencies a thing? > > Answering these questions is why we employ people who understand Make :D Quite. I entirely expect my makefile changes to ultimately be replaced by someone who actually understands why and how it's sane to have a build system that consists of a bash script that generates a python script that generates another python script that generates some Makefiles (And, indeed, note with some interest how chunks of the makefile I remember writing a few months back appear to have vanished and yet still magically do what they're supposed to, despite apparently not existing anywhere in m-c o.0)... On the plus side, it *is* documented. :P

(In reply to Chris Kitching [:ckitching] from comment #129) > So, after some jiggery-pokery I have this approach. Some kinks exist to be > ironed out (Only just got this result, but working theory is that for some > strange reason some of Robocop harness *isn't* in o.m.g.tests but is instead > in o.m.g, which just seems bizzaire (And exposes it to being eaten.)): > https://tbpl.mozilla.org/?tree=Try&rev=4d54617fa94b Sounds like this depends on moving those tests: Bug 938659. > I *think* this is sane? It'd be very nice if it can be made to work - no > need to manually maintain annotations for Robocop at all! (But a few for now > until the work to eliminate Reflection from the tests is totally completed) Makes sense to me.

Chris: I'm holding off on reviewing these until you say they're ready.

(In reply to Richard Newman [:rnewman] from comment #131) > Chris: I'm holding off on reviewing these until you say they're ready. Good plan: https://tbpl.mozilla.org/?tree=Try&rev=27d065e08406 ETA is a few minutes. I've abandoned the earlier attempt to make Robocop work without annotations. Comella's recent work makes this in principle possible, but it requires a level of build-system manipulation prowess I currently lack. We can handle that in a followup. Remember, Bug 913985 needs to land first, and I just unbitrotted and got a green try run from that lot. So if you're looking for something to do...

Comment on attachment 833289 [details] [diff] [review] Part 7 - Mutilate the build system. Binning this for now. Handle it in a followup - maybe the content of this patch will be of use. Doubtful.

Just a rebase.

After what Comella just did the idea behind this patch now largely stops working (Or, at least, isn't going to be used very much.) The thinking was that by wrapping as many Fennec program methods as possible in methods in RobocopAPI we ensure a preserved entry point without needing to tag the program methods. This approach would lead to suboptimal optimisation, as well as being problematic to maintain, and since direct access to program classes is now possible this is unlikely to be used for future tests, which is mostly a good thing. The downside is that now it's easy to access Fennec program classes from Robocop tests without the hassle of either Reflection of RobocopAPI people are liable to start doing so very often, leading to an explosion in the number of entry points needing to be preserved. This effect is liable to make Part 3 of this patch series *extremely* susceptible to rot. This patch, then, applies its original idea to all applicable areas in Robocop tests that still use Reflection, with the expectation that at some point Comella's work will be fully completed and make these uses of Reflection go away, too.

Counter-intuitively, this patch has somewhat *expanded* as a result of Comella's work in the absence of the ability to resolve these entry points without the need for annotations. This is largely due to things that were originally moved to RobocopAPI no longer being so now needing to be annotated. The growth is not huge, and since we expect all of these to go away in due course is probably fine. This is the patch which *might* require a further revision before this series is fit to land, as this is the culprit of the failures seen on try: https://tbpl.mozilla.org/?tree=Try&rev=27d065e08406 The revised version has been pushed to run only those tests that failed: https://tbpl.mozilla.org/?tree=Try&rev=d519c5589cbb And is expected to turn green. If not, it'll just be a missing annotation in this patch.

Some WebRTC dudes moved a few methods around, so this needed tweaking. It is most likely possible to bring these entry points under the umbrella of the code generator once Bug 913985 lands (Which should be sort of nowish, as people appear to have run out of theories for how it caused the failure it didn't cause ;) .) This notion is tracked as Bug 917512, may or may not be possible, and is not an issue I am qualified to deal with at this time, due to my complete lack of familiarity with WebRTC. Still, for now, Annotate!

The RobocopJNITarget has been renamed to RobocopTarget. The Proguard invocation line in Makefile.in now uses $(ALL_JARS) directly, in conjunction with the horror that is string substitution in Make.

This one is cool. As part of its normal operation, the annotation processor iterates every member of every class in Fennec, pre-proguarding, to generate code. This patch causes the annotation processor to, while it's at it, make a record of every element it finds that has an annotation that protects it from Proguard (Some such annotations cause it to need to generate code, a few don't). It also now launches a second thread on startup. The second thread scans all of the C++ code found in the objdir for JNI calls that are getting references to things in Java code, and makes a note of all the things which C++ is looking for in Java. When both tasks are complete, any element in the second list but not in the first is reported and the build is halted. Such elements are requested from the JNI but are not protected from Proguard - this implies an annotation is missing. Some effort has been made to avoid false positives. The scanner will only detect entry points referred to using literals. No dataflow analysis or constant folding is performed to attempt to determine the target of a particular JNI reference call. Uses of the JS JNI wrappers are not subjected to analysis. Uses of Reflection in Robocop tests (Or, these days, uses of *classes* in Robocop) So while it remains quite possible to write code with entry points which are not detected by the entry point scanner, this will detect many of the more common sorts of screwup. Warning: This patch contains an indecipherable sed script: private static final String COMMAND = "find . -type f -name \\*.cpp | " + "xargs sed -rn 's/.*(getClassGlobalRef|FindClass)\\(\\w*\"([^\"]*)\"\\w*\\).*/\\2/;ta;\n" + "s/.*(getStaticMethod|GetStaticMethodID|getMethod|GetMethodID)\\(\\w*\"([^\"]*)\"[^\"]*\"([^\"]*)\"\\w*\\).*/\\2 \\3/;tb;\n" + "s/.*(getField|GetFieldID|getStaticField|GetStaticFieldID)\\(\\w*\"([^\"]*)\"[^\"]*\"([^\"]*)\"\\w*\\).*/\\2 \\3/;tc;d\n" + ":a;s/\\//\\./g;i\\\n" + "C\n" + "p;d\n" + ":b;i\\\n" + "M\n" + "p;d\n" + ":c;i\\\n" + "F\n" + "p'\n"; Please don't kill me.

No longer causing rc4 oranges on try. This patch series is now green: https://tbpl.mozilla.org/?tree=Try&rev=c9d3036fa779

Second half of the deorangifying change. testSearchSuggestions is *weird*.

Rebasing. That should do it.

Adding the four new annotations needed by Bug 941846.

I accidentially the patch.

Try push (Including what just hit fx-team): https://tbpl.mozilla.org/?tree=Try&rev=3848d1fe9d08 Appears to be a success.

Comment on attachment 8336407 [details] [diff] [review] Part 1 - V8 - Annotate by hand the JNI entry points not covered by the code generator. Review of attachment 8336407 [details] [diff] [review]: ----------------------------------------------------------------- I'm gonna call this a rubberstamp.

Comment on attachment 8334349 [details] [diff] [review] Part 2 - V7 - Replace remaining uses of Reflection in Robocop with references to RobocopAPI, where possible. Review of attachment 8334349 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/RobocopAPI.java @@ +32,5 @@ > + * instead of Robocop. > + * > + * Alternatively, you might be able to get what you want by reflecting on a method annotated for the > + * benefit of the C++ wrapper generator - these methods are sure to not disappear at compile-time. > + * Trailing whitespace.

Comment on attachment 8334351 [details] [diff] [review] Part 3 - V6 - Annotate the remaining Robocop entry points. Review of attachment 8334351 [details] [diff] [review]: ----------------------------------------------------------------- I confess that I hate this whole solution -- argh shotgun annotations -- but on the other hand it's better than all the others. ::: mobile/android/base/AppConstants.java.in @@ +5,5 @@ > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > package org.mozilla.gecko; > > +import org.mozilla.gecko.mozglue.RobocopTarget; Newline after. ::: mobile/android/base/NSSBridge.java @@ +11,2 @@ > > public class NSSBridge { Let's just keep this whole class, rather than annotating individual methods. ::: mobile/android/base/db/BrowserDB.java @@ +42,3 @@ > public Cursor filter(ContentResolver cr, CharSequence constraint, int limit); > > // This should onlyl return frecent sites, BrowserDB.getTopSites will do the s/onlyl/only

Comment on attachment 8334257 [details] [diff] [review] Part 4 - V5 - Annotate members referenced by WebRTC's JNI code. I'm assuming that (a) this general approach has already been WebRTC-folks-reviewed, and (b) it's probably bitrotted to hell.

Comment on attachment 8334352 [details] [diff] [review] Part 6 - V3 - Detect missing annotations at compile-time. Review of attachment 8334352 [details] [diff] [review]: ----------------------------------------------------------------- Split this out into a new bug.

I think this is at the point where we want to start the landing process. We've reviewed this code enough times, and most of it is "looks fine, now let's make sure it works". If you want to reset fig (or get it to this state), and then fix whatever small merge conflict might arise, great. Otherwise, we can do this with Try builds on top of fx-team. Here's one such Try build; I rebased a few of the patches. https://tbpl.mozilla.org/?tree=Try&rev=a47281ac2eb9 Let's grab the builds for ARMv7 and ARMv6, ask the QA folks to bang on them some, and make sure there are no visible performance regressions in use. If there are no alarm bells, let's get this in Nightly. Next merge is December 9th, so I think December 2nd is a good cut-off date to give us a week to spot regressions and back out without having to do it in Aurora. If we miss that, let's slip to Fx29.

Make that https://tbpl.mozilla.org/?tree=Try&rev=43ba5e1ee811 .

(In reply to Richard Newman [:rnewman] from comment #150) > Comment on attachment 8334352 [details] [diff] [review] > Part 6 - V3 - Detect missing annotations at compile-time. > > Review of attachment 8334352 [details] [diff] [review]: > ----------------------------------------------------------------- > > Split this out into a new bug. Okay, but we probably should land it pretty promptly after landing the rest of it - the additional safety it gives mitigates somewhat the horror of the whole "Annotate stuff" solution (and makes developing under the new annotation-requiring system considerably less problematic). (In reply to Richard Newman [:rnewman] from comment #148) > Comment on attachment 8334351 [details] [diff] [review] > Part 3 - V6 - Annotate the remaining Robocop entry points. > > Review of attachment 8334351 [details] [diff] [review]: > ----------------------------------------------------------------- > > I confess that I hate this whole solution -- argh shotgun annotations -- but > on the other hand it's better than all the others. Once we figure out the build system magic needed to properly exploit Comella's changes to Robocop we get to kill off all of these. For now, we're sorta stuck with them. As for this solution in general, it seems that the "Standard" solution to "Proguard eats my entry points" is just to annotate them all by hand and maintain that (at tremendous expense). The code generator, and hopefully soonish the Robocop compile-time magic enable us to move away from this (indeed, the code generator eliminates the need for (or at least, gives us a nonterrible excuse for having) about 108 annotaions). This solution still kinda sucks. > ::: mobile/android/base/NSSBridge.java > @@ +11,2 @@ > > > > public class NSSBridge { > > Let's just keep this whole class, rather than annotating individual methods. Makes sense. Although this will enlarge the class file size by around 12 bytes, as the LOGTAG field will no longer be deleted (Whoopdediddleydoo). (In reply to Richard Newman [:rnewman] from comment #149) > Comment on attachment 8334257 [details] [diff] [review] > Part 4 - V5 - Annotate members referenced by WebRTC's JNI code. > > I'm assuming that (a) this general approach has already been > WebRTC-folks-reviewed, and (b) it's probably bitrotted to hell. It was fine when I uploaded it, and between my first writing it and my uploading this latest version only one annotated method changed in WebRTC land, so it's *probably* still fine. (In reply to Richard Newman [:rnewman] from comment #151) > If you want to reset fig (or get it to this state), and then fix whatever > small merge conflict might arise, great. Otherwise, we can do this with Try > builds on top of fx-team. It's not clear that either is clearly better. Since you've rebased and done a try push already we might as well continue with that. Should I proceed to land the patches if your try push goes green, or do you want your name on the commit that breaks *everything*? :P > Let's grab the builds for ARMv7 and ARMv6, ask the QA folks to bang on them > some, and make sure there are no visible performance regressions in use. Very much this. We have a bunch of things our test coverage misses, so this is the only way to detect crashes in possibly obscure and unexercised features. > Next merge is December > 9th, so I think December 2nd is a good cut-off date to give us a week to > spot regressions and back out without having to do it in Aurora. If we miss > that, let's slip to Fx29. Exciting. Charrge!

Woo, totally green: https://tbpl.mozilla.org/?tree=Try&rev=43ba5e1ee811 (In reply to Chris Kitching [:ckitching] from comment #153) > It's not clear that either is clearly better. Since you've rebased and done > a try push already we might as well continue with that. Should I proceed to > land the patches if your try push goes green, or do you want your name on > the commit that breaks *everything*? :P We should exercise the builds before we land them. I don't mind who actually lands the patches. Though to avoid further syntactic bitrotting, I'm landing Parts 1-4 now, because without turning on Proguard, it doesn't actually do anything. We should still land soon to avoid annotation rot. (I also spent the ten minutes adding all of the reviewer flags to the patches :P)

Parts 1 to 4: https://hg.mozilla.org/integration/fx-team/rev/17739416c67e https://hg.mozilla.org/integration/fx-team/rev/83e497d2b2be https://hg.mozilla.org/integration/fx-team/rev/efa2554b6b50 https://hg.mozilla.org/integration/fx-team/rev/5a6373ab5fe6

(In reply to Richard Newman [:rnewman] from comment #154) > We should exercise the builds before we land them. Alright.. Having never done this before, I'm unsure how to proceed. Land on Fig and summon QA people? Or does the QA fairy spontaneously appear? > Though to avoid further syntactic bitrotting, I'm landing Parts 1-4 now, > because without turning on Proguard, it doesn't actually do anything. We > should still land soon to avoid annotation rot. Ta. There's also at least some chance that people might notice the annotations and follow the pattern avoiding annotation rot if they're adding new entry points. *snrk*. > (I also spent the ten minutes adding all of the reviewer flags to the > patches :P) Yeah... I feel bad pushing things to try with r=someone when they're still highly experimental. The implicit endorsement of my latest hairbrianed scheme seems generally misplaces. There wasn't a particularly clear point at which this lot transitioned from "highly experimental" to "might not catch fire". Sorry about that. Also, ten minutes? Did you never learn to sed? :P

(In reply to Chris Kitching [:ckitching] from comment #156) > Alright.. Having never done this before, I'm unsure how to proceed. Land on > Fig and summon QA people? Or does the QA fairy spontaneously appear? Click on the 'B' on the Try builds, and grab the APKs. > Also, ten minutes? Did you never learn to sed? :P I had to read through the 150-comment bug to find out each of the different reviewers for each part! Incidentally, my local build with Proguard enabled seems to be working fine…

(In reply to Richard Newman [:rnewman] from comment #157) > Incidentally, my local build with Proguard enabled seems to be working fine… Ah, so you literally mean "Push buttons and see if it breaks locally". Gotcha. Very high tech. No success in attempting to break it was encountered testing the last version of this I had running back at Mozilla. There'll be a slight delay in testing at this end for now as the phone you sent me (Thanks! That was awesome!) was dead on arrival. Seems a replacement battery should do the trick. To eBay! In the meantime, one thing I suggest you try is stuff that uses WebRTC features. That isn't (I think?) exercised by the automated tests and I'm not entirely confident I didn't miss anything when tagging those.