Closed
Bug 1106593
(proguard-libraries)
Opened 9 years ago
Closed 9 years ago
Ensure that Google Play Services dependency is Proguarded during build
Categories
(Firefox Build System :: Android Studio and Gradle Integration, defect)
Tracking
(firefox35+ fixed, firefox36 fixed, firefox37 fixed, fennec35+)
RESOLVED
FIXED
mozilla37
People
(Reporter: rnewman, Assigned: nalexander)
References
Details
Attachments
(2 files, 2 obsolete files)
10.94 KB,
patch
|
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
39 bytes,
text/x-review-board-request
|
nalexander
:
review+
|
Details |
Nick notes: --- I was just looking at the Makefile.in code for running Proguard, and it sure looks like we're not stripping Play Services at all. That is, we write PG'd Fennec JARs into jars-proguarded, but we don't do anything to the Play Services JAR. I think we should determine if other projects strip their external JARs (I'd expect the answer is yes) and we should adjust accordingly. --- Resolving this would fix Bug 1100361, and would also make everything just a little more shiny. Assigning to Nick 'cos he's doing experiments on this.
Comment 2•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=323473d98734
Comment 3•9 years ago
|
||
Hey, it got 3MB smaller on try. Excellent. This ended up less trivial than expected. Simply passing the bundled library jars through Proguard in the obvious way results in dex failure. Proguard is buggy and likes to corrupt debug info in class files. Dex fails if classes contain invalid debug information (even though it's not needed for execution). Proguard can strip debug information and avoid this problem, but then you lose line numbers in stack traces: not really okay (and doing this for the whole app saves a pathetic 19kb, so it's not helpful). Luckily, Proguard doesn't corrupt any debug information in Mozilla classes (hence how we never saw this bug before), but does so for both the Play Services and compatibility library. Losing line number tables for library code we include as a binary seems like something we don't care too much about: you'll lose line numbers for stack frames in stack traces that are internal to the library. This is only very occasionally actually useful, methinks... Since Proguard can't be told to either strip or not strip debug data on a per-jar basis, the practical upshot of the above is that we have to run Proguard twice: once on just the libraries to strip their debug data (and absolutely nothing else), and again in the usual way to optimise the app. It turns out Proguard is quite smart about doing no work when its outputs are up to date (it just prints "Outputs appear up-to-date" and terminates), so this extra Proguard call is nearly free after the first build until you change the library jars. I'm experimenting with applying some optimisations at this step, too, as this pre-application of library-internal optimisations may allow the main execution to terminate an iteration or two earlier, saving quite a bit of time. To make matters even more exciting, Proguard has another bug which causes corruption of the bytecode if you apply >3 optimisation passes with this new approach. More investigation is needed in this direction, but the amount of size optimisation we lose by running fewer passes is vastly inferior to what we gain from optimising this new code. It is not clear (and probably impractical to measure) the performance difference of switching from 6 passes to 3. Additionally, since the time required for each pass has gone up substantially now we're optimising all this new code, running fewer passes is nice from a not-melting-automation point of view. More investigation into the exact nature of this new Proguard bug is required. I guess I've got nothing better to do while flying across the Atlantic.
Updated•9 years ago
|
status-firefox35:
--- → affected
status-firefox36:
--- → affected
status-firefox37:
--- → affected
tracking-firefox35:
--- → +
Comment 4•9 years ago
|
||
I believe this is the least risky patch needed to get Fx35 to allow installing on Gingerbread devices. The split APK work, while also a fix, involves more moving parts. Trying to summarize Chris' details in comment 3, the current approach: 1. Reduces the size of the APK by ~ 3MB 2. Proguard will corrupt debug info so we need to: A. Run Proguard on the bundled libraries, stripping the debug info B. Run Proguard on the main code, NOT stripping the debug info 3. We will lose line numbers in stack traces because of Step 2A 4. Proguard will cause corruption if doing Step #2 with more than 3 optimization runs. We use 6 now. Proposal: Create a patch that does Step 2A and Step 2B, while dropping the number of optimization runs to 3. Land on Nightly ASAP. Uplift to Aurora and Beta ASAP after verified on Nightly.
Updated•9 years ago
|
Flags: needinfo?(chriskitching)
Comment 5•9 years ago
|
||
Chris - Remember to update your Try build syntax for your next Try push.
Comment 6•9 years ago
|
||
(In reply to Mark Finkle (:mfinkle) from comment #5) > Chris - Remember to update your Try build syntax for your next Try push. Ah yes, I spotted that after reading Richard's email. Definitely we should get a proper run done before landing this. (In reply to Mark Finkle (:mfinkle) from comment #4) > Trying to summarize Chris' details in comment 3 It does rather scream "written in an airport by ckitching in a hurry". > B. Run Proguard on the main code, NOT stripping the debug info We apply Proguard to the whole program here (Mozilla code + libraries). This step is what deletes tons of unused library code, inlines some of it into Mozilla classes, etc., and gets us the size win. > 3. We will lose line numbers in stack traces because of Step 2A Almost: this only applies to stack frames inside the library. You'll get a weird stacktrace where you have line numbers for all the Mozilla code, but none for the library code. I think this is far less troubling than the prospect of losing all line numbers. > 4. Proguard will cause corruption if doing Step #2 with more than 3 > optimization runs. We use 6 now. > > Proposal: > > Create a patch that does Step 2A and Step 2B, while dropping the number of > optimization runs to 3. Land on Nightly ASAP. Uplift to Aurora and Beta ASAP > after verified on Nightly. Prettymuch. Thanks for the copyediting! I'll continue prodding this Proguard bug, as it's annoying me. Should you start getting a dex error that looks like the one discussed here: http://stackoverflow.com/questions/6159390/android-build-with-proguard-dx-simexception-local-0000-invalid you need to reduce the number of optimisation passes until it goes away. Note that Proguard has separate "optimise" and "shrink" steps, so reducing the number of passes to zero doesn't entirely remove Proguard's ability to help stripping out unused classes.
Flags: needinfo?(chriskitching)
Assignee | ||
Updated•9 years ago
|
Summary: Ensure that Google Play Services dependency is stripped during build → Ensure that Google Play Services dependency is Proguarded during build
Comment 7•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=54855573854f
Comment 8•9 years ago
|
||
Reviewboard doesn't seem to be my friend for now, so hopefully you can survive looking at it here.
Attachment #8533364 -
Flags: review?(nalexander)
Updated•9 years ago
|
Assignee: nalexander → chriskitching
Comment 9•9 years ago
|
||
So, I did a sort of clever thing: I figured out which of Proguard's optimisations was causing the corrupt bytecode after many optimisation passes, and turned that one off. I then determined experimentally if it was better to: a) turn that one optimisation off and run six passes b) Keep all the optimisations turned on and run just two passes. (My comments above saying "three" were all typos: it explodes after just two). a) won. Here's a patch which does that (the different w.r.t the patch pushed to try earlier is just the extra line in Proguard config to turn off the stupid optimisation). So, to summarize everything for those trying to follow the multiple U-turns, this patch: 1) Adds an extra Proguard pass to strip debug information from libraries, as above. 2) Adds the libraries to the main optimising Proguard execution. 2) Disables a Proguard optimisation which causes corrupt bytecode for an unrelated, stupider reason. 3) No longer does anything to the Proguard pass count. (and now should save slightly more size than the earlier version, as a handy bonus) Sorry for not using reviewboard: I'm currently having a minor argument with Mercurial about the existence of my key. :P
Comment 10•9 years ago
|
||
Attachment #8533370 -
Flags: review?(nalexander)
Comment 11•9 years ago
|
||
/r/1325 - Bug 1106593: Apply Proguard to bundled Fennec libraries. Pull down this commit: hg pull review -r 395c2de544ba91c0163e22b03d6b43414e3fb7e9
Comment 12•9 years ago
|
||
Comment on attachment 8533364 [details] [diff] [review] Apply Proguard to bundled Fennec libraries Reviewboard works a whole lot better if you *actually identify yourself*.
Attachment #8533364 -
Attachment is obsolete: true
Attachment #8533364 -
Flags: review?(nalexander)
Assignee | ||
Comment 13•9 years ago
|
||
https://reviewboard.mozilla.org/r/1323/#review703 This is basically r+, but I'd like to see the trivial nits before it ships. Thanks for hammering this out, ckitching! ::: mobile/android/base/Makefile.in (Diff revision 1) > +# redefine classes in java.lang (and is necessary for inclusion of libraries which nit: full sentence. And this is true, but doesn't really say anything about why these particular libraries are classified this way. ::: mobile/android/base/Makefile.in (Diff revision 1) > JAVA_CLASSPATH := $(subst $(NULL) ,:,$(strip $(JAVA_CLASSPATH))) nit: do this right after we define the CP above. ::: mobile/android/base/Makefile.in (Diff revision 1) > + $(ANDROID_COMPAT_LIB) \ Why is support-v4 both bundled and on the BCP? Is there some relationship required between CP, BCP, and bundled libs? ::: mobile/android/base/Makefile.in (Diff revision 1) > +JAVA_BUNDLED_LIBS = \ nit: make this lower case. The convention gps (and I) prefer is that upper case means "globally accessible" and lower case means "this file only". ::: mobile/android/base/Makefile.in (Diff revision 1) > + -injars $(subst ::,:,$(subst $(NULL) ,:,$(strip $(JAVA_BUNDLED_LIBS))))\ nit: extract this subst out to earlier, so that JBL gets the same treatment as BCP and CP. ::: mobile/android/base/Makefile.in (Diff revision 1) > -.proguard.deps: .geckoview.deps $(ALL_JARS) $(topsrcdir)/mobile/android/config/proguard.cfg > +.proguard.deps: .geckoview.deps .preprogurd.deps $(ALL_JARS) $(topsrcdir)/mobile/android/config/proguard.cfg nit: spelling again. ::: mobile/android/base/Makefile.in (Diff revision 1) > - -injars $(subst ::,:,$(subst $(NULL) ,:,$(strip $(ALL_JARS)))) \ > + -injars $(subst ::,:,$(subst $(NULL) ,:,$(strip $(ALL_JARS)))):jars-preproguarded \ nit: ditto with the subst. I wish this was easier in Make :/ ::: mobile/android/config/proguard.cfg (Diff revision 1) > +# Rules to prevent Google Play Services from exploding This is straight from that page. Can we extract it to it's own file and -include it? Let's make it easy to see what's what. ::: mobile/android/config/preproguard.cfg (Diff revision 1) > +-keepattributes Exceptions,InnerClasses,Signature,Deprecated,*Annotation*,EnclosingMethod Is -keepattributes * not a thing? ::: mobile/android/base/Makefile.in (Diff revision 1) > + -libraryjars $(library_jars):gecko-R.jar Why is gecko-R.jar special here? It's can't be referred to by third party libraries, and later, it should be PGed like everything else. Right? ::: mobile/android/base/Makefile.in (Diff revision 1) > +.preprogurd.deps: gecko-R.jar $(topsrcdir)/mobile/android/config/preproguard.cfg nit: mis-spelled progurd. I want to really make this bundled/everything else divide clear, so let's call this: .bundled.proguard.deps and put those outputs in bundled-jars-proguarded. Parallel to .proguard.deps and jars-proguarded. A comment explaining ALL_JARS is us and BUNDLED_JARS is external libs would go a long way. ::: mobile/android/base/Makefile.in (Diff revision 1) > + # First, we delete debugging information from libraries. Having line-number information for libraries nit: In general, put these comments at top-level in the Makefile (so before the rule). Worth noting that this doesn't impact the Gecko libraries. Also, we have source for every library save Google Play Services. Expanding this comment to explain the multi-part flow would be nice. This is some complicated Make.
Comment 14•9 years ago
|
||
Some replies: I'll do a new patch in the morning. (In reply to Nick Alexander :nalexander from comment #13) > ::: mobile/android/base/Makefile.in > (Diff revision 1) > > + $(ANDROID_COMPAT_LIB) \ > > Why is support-v4 both bundled and on the BCP? Is there some relationship > required between CP, BCP, and bundled libs? Not a sane one, anyway. "Bundled" = "a library we include pre-compiled without building it from source" Bootclasspath is just like classpath, but it allows you to overwrite packages in the "forbidden packages" such as java.lang. Classpath is set of places java/javac look for classes. Support-v4 is both bundled and needs (unlike all the other libraries) to be on the BCP to work, hence the slight strangeness. > ::: mobile/android/config/proguard.cfg > (Diff revision 1) > > +# Rules to prevent Google Play Services from exploding > > This is straight from that page. Can we extract it to it's own file and > -include it? Let's make it easy to see what's what. Good plan: this is one of the very few things Proguard config provides a nice way to do. > ::: mobile/android/config/preproguard.cfg > (Diff revision 1) > > +-keepattributes Exceptions,InnerClasses,Signature,Deprecated,*Annotation*,EnclosingMethod > > Is -keepattributes * not a thing? I think it is, but there's no -dontkeepattributes, so we've got no way to tell it "Keep everything except LocalVariableTable and LineNumberTable", aside from this ridiculousness. > ::: mobile/android/base/Makefile.in > (Diff revision 1) > > + -libraryjars $(library_jars):gecko-R.jar > > Why is gecko-R.jar special here? It's can't be referred to by third party > libraries, and later, it should be PGed like everything else. Right? Nope: The aapt step builds the R.javas for the libraries and sticks them in there. Gotta have gecko-R.jar on the classpath before the references in the library code to resources will resolve. I'll leave it to you to decide if this is a bug that needs filing: it doesn't really make much difference to the app. It does force the preproguarding to wait for the resource generation for *everything* before it can proceed, but that's probably not a huge issue. Yes, it does get PGed like everything else (just like the libraries do) in the second Proguard pass (the one that really optimises, as opposed to the silly "remove the debug data that breaks proguard" preprocessing step for libraries only). > ::: mobile/android/base/Makefile.in > (Diff revision 1) > > +.preprogurd.deps: gecko-R.jar $(topsrcdir)/mobile/android/config/preproguard.cfg > > nit: mis-spelled progurd. Eek. Needs more tooling. :P > I want to really make this bundled/everything else divide clear, so let's > call this: > > .bundled.proguard.deps and put those outputs in bundled-jars-proguarded. > Parallel to .proguard.deps and jars-proguarded. A comment explaining > ALL_JARS is us and BUNDLED_JARS is external libs would go a long way. My only worry here is the libraries aren't really optimised here, they've just had their debug data stripped. Definitely needs a better name, though.
Comment 15•9 years ago
|
||
(In reply to Nick Alexander :nalexander from comment #13) > I want to really make this bundled/everything else divide clear, so let's > call this: > > .bundled.proguard.deps and put those outputs in bundled-jars-proguarded. Perhaps bundled-jars-nodebug is a better name, as it expresses what's actually been done to the jars? We don't want people to think these jars have actually been optimised. They're unchanged aside from having had the problematic debug data deleted. > Parallel to .proguard.deps and jars-proguarded. A comment explaining > ALL_JARS is us and BUNDLED_JARS is external libs would go a long way. > > Also, we have source for every library save Google Play Services. True, but we're not *modifying* it, so it's somewhat rare that we're going to want to look at it. Not impossible: library bugs happen, after all, but I wanted to make the point that this is much less scary than losing all debug information everywhere. I also moved our rapidly-growing collection of Proguard configuration files into their own directory in mobile/android/config.
Comment 16•9 years ago
|
||
/r/1325 - Bug 1106593: Apply Proguard to bundled Fennec libraries. r=nalexander Pull down this commit: hg pull review -r 877254882ad87984a27021929e6213a0937aa1c9
Comment 17•9 years ago
|
||
(In reply to Chris Kitching [:ckitching] from comment #16) > /r/1325 - Bug 1106593: Apply Proguard to bundled Fennec libraries. > r=nalexander > > Pull down this commit: > > hg pull review -r 877254882ad87984a27021929e6213a0937aa1c9 ^- I think this one was in error, and that the revised commit is really on the other reviewboard instance (1323) (which is now properly showing revision 2). ... Still sorting out the shell scripts. :P
Assignee | ||
Comment 18•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/712e8ba1ec6f
Assignee | ||
Comment 19•9 years ago
|
||
AaronMT: I've tested this locally (play, pause, etc) but there's always the potential for regressions when doing systemic changes like this one. Please be on the look-out for missing symbol errors, run-time linking problems, DEX errors, and the like, and have them block this ticket. An additional run-through of the code would be great if you can manage it.
Flags: needinfo?(aaron.train)
Comment 20•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/712e8ba1ec6f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Assignee | ||
Comment 21•9 years ago
|
||
Comment on attachment 8533370 [details]
MozReview Request: bz://1106593/ckitching
Important to note that this is similar, but not identical, to the patch that landed. The substantive change was re-introducing conditional library includes dependent on MOZ_NATIVE_DEVICES; ckitching's past dropped that condition.
Attachment #8533370 -
Flags: review?(nalexander) → review+
Updated•9 years ago
|
Alias: proguard-libraries
Flags: needinfo?(aaron.train)
Assignee | ||
Comment 22•9 years ago
|
||
This applies Proguard to third-party libraries such as the Android support-v4 library and the Google Play Services libraries. Previously, these were not Proguarded, bloating the Fennec APK. Technically, this required a few work-arounds, including: * stripping library debug information with a early Proguard invocation; * altering the optimizations tried; and * reducing the number of Proguard passes.
Assignee | ||
Updated•9 years ago
|
Assignee: chriskitching → nalexander
Assignee | ||
Comment 23•9 years ago
|
||
Comment on attachment 8534460 [details] [diff] [review] [beta] Proguard third-party libraries that ship with Fennec This needs a branch specific patch for landing on beta. This builds and runs locally.
Attachment #8534460 -
Attachment description: Proguard third-party libraries that ship with Fennec → [beta] Proguard third-party libraries that ship with Fennec
Assignee | ||
Comment 24•9 years ago
|
||
Comment on attachment 8533370 [details]
MozReview Request: bz://1106593/ckitching
Approval Request Comment
[Feature/regressing bug #]: general growth of code has pushed our APK size.
[User impact if declined]: our APKs cannot be installed on certain devices.
[Describe test coverage new/current, TBPL]: nothing special. This changes only the amount of third-party library that we ship. It's possible we trim too much, but our tests won't catch that. We'd only see that, I think, when actively using the Chromecast features.
[Risks and why]: we could hard crash if a symbol has been Proguarded out. As I said above, I anticipate this would only happen when actively using the Chromecast features.
[String/UUID change made/needed]: none.
To uplifter (RyanVM!): the central commit applied cleanly to aurora with |hg graft|.
Attachment #8533370 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 25•9 years ago
|
||
Comment on attachment 8534460 [details] [diff] [review] [beta] Proguard third-party libraries that ship with Fennec Approval Request Comment [Feature/regressing bug #]: general growth of code has pushed our APK size. [User impact if declined]: our APKs cannot be installed on certain (generally low-end) devices. [Describe test coverage new/current, TBPL]: nothing special. This changes only the amount of third-party library that we ship. It's possible we trim too much, but our tests won't catch that. We'd only see that, I think, when actively using the Chromecast features. *We should stage this to let it bake on central and even aurora for a few days or weeks.* [Risks and why]: we could hard crash if a symbol has been Proguarded out. As I said above, I anticipate this would only happen when actively using the Chromecast features. [String/UUID change made/needed]: none. To uplifter (RyanVM!): the central commit did not apply cleanly to beta with |hg graft|, so I've provided a branch patch.
Attachment #8534460 -
Flags: approval-mozilla-beta?
Updated•9 years ago
|
Updated•9 years ago
|
Attachment #8533370 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 26•9 years ago
|
||
Let's wait and take this on next week's beta
Updated•9 years ago
|
tracking-fennec: ? → 35+
Comment 28•9 years ago
|
||
(In reply to Lukas Blakk [:lsblakk] use ?needinfo from comment #26) > Let's wait and take this on next week's beta Forget-me-not Ping
Comment 29•9 years ago
|
||
An actual ping. To approve this for beta. See comment 25 for rational.
Flags: needinfo?(release-mgmt)
Flags: needinfo?(lsblakk)
Comment 30•9 years ago
|
||
Comment 25 mentions this should bake on central and aurora for days, possibly weeks so I was not intending to take it in this week's beta.
Flags: needinfo?(release-mgmt)
Flags: needinfo?(lsblakk)
Comment 31•9 years ago
|
||
(In reply to Lukas Blakk [:lsblakk] use ?needinfo from comment #30) > Comment 25 mentions this should bake on central and aurora for days, > possibly weeks so I was not intending to take it in this week's beta. Yeah, I'm not sure why Nick thought we'd need "weeks" of bake time.
Comment 32•9 years ago
|
||
This breaks Gingerbread usage in 35. We are nearly in mid beta so I would like to get GB users fixed asap.
Comment 33•9 years ago
|
||
(In reply to Mark Finkle (:mfinkle) from comment #31) > (In reply to Lukas Blakk [:lsblakk] use ?needinfo from comment #30) > > Comment 25 mentions this should bake on central and aurora for days, > > possibly weeks so I was not intending to take it in this week's beta. > > Yeah, I'm not sure why Nick thought we'd need "weeks" of bake time. Lukas - Let's try to get this in today's GTB for Beta. I'd rather have people around the next few days to deal with fallout, then wait for next week and have no one.
Flags: needinfo?(lsblakk)
Comment 34•9 years ago
|
||
Comment on attachment 8534460 [details] [diff] [review] [beta] Proguard third-party libraries that ship with Fennec understood - thanks for the update, approving
Flags: needinfo?(lsblakk)
Attachment #8534460 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 36•9 years ago
|
||
Attachment #8533370 -
Attachment is obsolete: true
Attachment #8618759 -
Flags: review+
Comment 37•9 years ago
|
||
Updated•5 years ago
|
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 37 → mozilla37
You need to log in
before you can comment on or make changes to this bug.
Description
•