1009732 - Reduce number of Proguard passes for developer builds

Status: RESOLVED FIXED

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

Description

[Nick Alexander :nalexander [he/him]]

Similar to Bug 7225554.  We're currently switching on MOZ_DEBUG to minimize Proguard passes (in developer builds) [1].  I think we should switch on MOZILLA_OFFICIAL as well.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Makefile.in#94

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: Do only one Proguard pass when build is not MOZILLA_OFFICIAL. r=ckitching DONTBUILD

This continues the convention that a developer build is when
MOZILLA_OFFICIAL is not defined.

Comment 2

[Chris Kitching [:ckitching]]

Comment on attachment 8421965 [details] [diff] [review]
Do only one Proguard pass when build is not MOZILLA_OFFICIAL. r=ckitching DONTBUILD

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

If that's a convention then this seems generally sensible.

However, there is a small risk here: If something your code depends on is optimised away in a later pass of Proguard you won't know about it in your debug build. 

That sort of breakage can occur in two scenarios:

1) You've added a new entry point to Java (using Reflection or the JNI) and not annotated it correctly to protect it from Proguard.

2) There already existed an unannotated entry point, but the optimiser didn't want to optimise it away, so it worked anyway. Your new code changes something which makes Proguard now decide it's a good idea to inline that thing (that may be entirely unrelated to your patch), and something apparently completely random breaks.

Bug 944553 mitigates this pain significantly by detecting missing annoatations at compile-time (but doing so, in general, is decidedly halting-problem-esque, so even this will not be perfect).

So: Is MOZILLA_OFFICIAL set on try? If not then I don't believe this change is a good idea. That would mean these (rare but plausible) breakages would not be seen until too late, giving someone a late night backout party.
You could perhaps handle breakage case 1 by having people reviewing such patches make sure a test has been done with a build with the full number of passes, but the ever-present possibility of case 2 makes me think that the testing infrastruture really should do the whole shebang, just to be safe.

Thoughts? (ie. *is* that flag set on try?)

Comment 3

[Nick Alexander :nalexander [he/him]]

> Review of attachment 8421965 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If that's a convention then this seems generally sensible.

Great questions!  Yes, this is the convention; but it's a recent convention, and one I'm trying to expand.
 
> However, there is a small risk here: If something your code depends on is
> optimised away in a later pass of Proguard you won't know about it in your
> debug build. 

Very true.

> That sort of breakage can occur in two scenarios:
> 
> 1) You've added a new entry point to Java (using Reflection or the JNI) and
> not annotated it correctly to protect it from Proguard.
> 
> 2) There already existed an unannotated entry point, but the optimiser
> didn't want to optimise it away, so it worked anyway. Your new code changes
> something which makes Proguard now decide it's a good idea to inline that
> thing (that may be entirely unrelated to your patch), and something
> apparently completely random breaks.
> 
> Bug 944553 mitigates this pain significantly by detecting missing
> annoatations at compile-time (but doing so, in general, is decidedly
> halting-problem-esque, so even this will not be perfect).
> 
> So: Is MOZILLA_OFFICIAL set on try? If not then I don't believe this change
> is a good idea. That would mean these (rare but plausible) breakages would
> not be seen until too late, giving someone a late night backout party.
> You could perhaps handle breakage case 1 by having people reviewing such
> patches make sure a test has been done with a build with the full number of
> passes, but the ever-present possibility of case 2 makes me think that the
> testing infrastruture really should do the whole shebang, just to be safe.
> 
> Thoughts? (ie. *is* that flag set on try?)

Every build on infra includes MOZILLA_OFFICIAL, and try essentially builds Nightly.  You can see the mozconfigs at http://mxr.mozilla.org/mozilla-central/source/mobile/android/config/mozconfigs/

All you say is correct and relevant, but I would weaken the claims a touch: because I don't think PG is expected to be deterministic in its inputs, and because our inputs are definitely rapidly changing, such issues with code getting PGed out are also possible with any finite number of passes.  But I take your point, and agree that this only is sensible given that MOZILLA_OFFICIAL is set on all infra builds.

Thanks for the lighting-quick review!

Comment 4

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/a2a09fc0c39d

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a2a09fc0c39d