Closed
Bug 1256427
Opened 8 years ago
Closed 8 years ago
Investigate replacing AppCompat methods with built-in framework methods, post GB, to save APK size
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox48 fixed)
RESOLVED
FIXED
Firefox 48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: mcomella, Assigned: mcomella)
References
Details
Attachments
(3 files, 1 obsolete file)
There are some methods that the AppCompat library implements for GB backwards compat but is available from API 15+ – our support range (see bug 1256426). Since we proguard, we might be able to shave off a few bytes by moving to the framework methods, rather than the AppCompat methods. This bug is to investigate if it saves APK size (or perhaps we can figure that out in bug 1256426) and if so, move to the framework methods.
Assignee | ||
Comment 1•8 years ago
|
||
via: ag --nofilename "import android.support" mobile/android/base/java | sort | uniq
Assignee | ||
Comment 2•8 years ago
|
||
(Note: list in comment 1 needs to be curated to APIs available in the framework from v15+) bug 1256426 has many uses and could be expensive to fix – DialogFragment sees limited usage [1], perhaps we should see the benefit of moving that to the framework DialogFragment first. [1]: https://mxr.mozilla.org/mozilla-central/search?string=support.v4.app.DialogFr&find=mobile%2Fandroid&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
Assignee | ||
Comment 4•8 years ago
|
||
DialogFragment requires a surprising amount of work (you have to change a few Loaders to the framework, one of which relies on SimpleCursorLoader, which uses the support library's AsyncLoader. bug 1256426 also required a surprising amount of work so I'm merging it into this.
Assignee | ||
Comment 5•8 years ago
|
||
We're looking to investigate changes in APK size after proguarding. Review commit: https://reviewboard.mozilla.org/r/39819/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/39819/
Assignee | ||
Comment 6•8 years ago
|
||
Worth mentioning: a disadvantage to moving to the framework is that features may be added in the latest versions of Android, which get back-ported via the AppCompat libraries, and if we spend effort moving to the framework, we may have to undo those efforts in-order use AppCompat again (and we only need to use AppCompat once to take a hit to our APK again).
Assignee | ||
Comment 7•8 years ago
|
||
try before removal: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3884867ea85b We should grab the built APK and compare it to the size of the APK after the patches remove Pair (an easy item to wean off the support library).
Assignee | ||
Comment 8•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=61da0ac6e194
Assignee | ||
Updated•8 years ago
|
Blocks: fatfennec
Summary: Investigate replacing AppCompat methods with built-in framework methods, post GB → Investigate replacing AppCompat methods with built-in framework methods, post GB, to save APK size
Assignee | ||
Comment 10•8 years ago
|
||
delta size of classes.dex: 5070532 - 5069936 = 596 bytes fwiw, Pair in the support library is quite small (80 lines!) [1] and now that I think about it, other classes in the support library may be using the implementation so we may have not removed its use entirely. Notably, FragmentTransaction uses it, which we do use from the support library (though perhaps we don't use the method that uses it, so Pair could get removed anyway). I'm not sure if it's worth going forward here – let's get public opinion. Nick, given my experiment results above, do you think it'd be worth trying to replace uses of the support library directly with the framework? Strangely though, libxul changed in size between the two builds – I'm confused that the builds don't seem deterministic! Maybe the Java code has a similar discrepancy! [1]: https://github.com/android/platform_frameworks_support/blob/master/v4/java/android/support/v4/util/Pair.java
Flags: needinfo?(michael.l.comella) → needinfo?(nalexander)
Comment 11•8 years ago
|
||
> Nick, given my experiment results above, do you think it'd be worth trying
> to replace uses of the support library directly with the framework?
I am less concerned with the size gains than I am with the stability trade-offs. The support library is fixed and shipped with the App; it should behave identically across versions. (Of course it doesn't, but it should.) In general the framework is more likely to have bugs addressed, but of course it is also more likely to have new bugs introduced. For core things (Fragments, Preferences, Loaders, Cursors), I think we should be using the framework, since there are subtle incompatibilities and bugs with the support libraries. This agrees with my general sentiment: I prefer to use the support libraries only when needed.
sebastian, do you have a strong opinion?
Flags: needinfo?(nalexander) → needinfo?(s.kaspari)
Comment 12•8 years ago
|
||
Not easy. In general: system framework: ----------------- PRO: * Less code / Smaller APK size * Hopefully quite stable because released rarely CONS: * Updates without our control * We can only use the lowest common subset of features - unless we build our own (tiny) "support libs" * Implementation can differ depending on Android version support library: ---------------- PRO: * Updates are more in our control (the major version of the support library should match the targetSdkVersion) * Backports of new features * Implementation is somewhat stable across Android versions CONS: * More code / bigger APK * Has been partially buggy in the past - sometimes an update means exchanging old bugs for new bugs. For most small things I would go and use the system framework if the thing I want is supported by our minSdkVersion (-> your Pair example). But as you pointed out it doesn't necessarily make sense to try to clean up the code base in order to shrink the app - especially as the implementation of the support library can and does change - maybe with the next support library update the cleaned up class will be back. For the big parts (AppCompatActivity) I would use the support library exclusively. Just because this allows us to move to new system features faster (Android P comes with a new style? Great. As soon as AppCompat supports it we can have it). But then there are things like fragments: With a minSdkVersion of 15 now we could use system fragments and I would like to use the system fragment. But if you use them with another support library feature or a third-party library then they are often build on top of the support fragment: For example if we want to have material design preferences everywhere then we want to use PreferenceFragmentCompat - that's a support fragment and requires FragmentActivity aka. AppCompatActivity. Actually I would only look at the library level (except if there's some substantial size difference): We import support v4 so it's okay to use whatever is in there. I don't think it is worth our time optimizing the usage of the support library so that proguard might be able to remove more parts of it - especially because the library is not in our control anyways. Is this helpful?
Flags: needinfo?(s.kaspari)
Comment 13•8 years ago
|
||
(In reply to Sebastian Kaspari (:sebastian) from comment #12) <snip> > Is this helpful? For me, yes -- your view of the major issues and trade-offs is consonant with my view. I think you are slightly more in favour of using the support libraries to access fresh styles earlier; that's not a thing I see as much in my day-to-day work, since most of my work isn't directly user facing. Thanks!
Assignee | ||
Comment 14•8 years ago
|
||
Comment on attachment 8730373 [details] MozReview Request: Bug 1256427 - Move to LruCache in framework from support lib. r=sebastian Might as well land this.
Attachment #8730373 -
Flags: review?(s.kaspari)
Updated•8 years ago
|
Assignee: nobody → michael.l.comella
Comment 15•8 years ago
|
||
Comment on attachment 8730373 [details] MozReview Request: Bug 1256427 - Move to LruCache in framework from support lib. r=sebastian https://reviewboard.mozilla.org/r/39819/#review38131 Go go go! :)
Attachment #8730373 -
Flags: review?(s.kaspari) → review+
Assignee | ||
Comment 16•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/8f2121d6764a413e0f7e6c004d787be7303d2a7e Bug 1256427 - Investigate moving from Pair in support library to framework.
Comment 17•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8f2121d6764a
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Assignee | ||
Comment 18•8 years ago
|
||
Let's try to look at some of the other small things we can swap out.
Assignee | ||
Comment 19•8 years ago
|
||
I'v noticed that *Compat classes appear to sometimes add functionality as the framework adds functionality and shouldn't be removed because they might be useful later, whereas classes with the same name (e.g. LruCache) are intended to implement something not implemented on older versions and can probably be removed.
Attachment #8730360 -
Attachment is obsolete: true
Assignee | ||
Comment 20•8 years ago
|
||
Comment on attachment 8730373 [details] MozReview Request: Bug 1256427 - Move to LruCache in framework from support lib. r=sebastian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39819/diff/1-2/
Attachment #8730373 -
Attachment description: MozReview Request: Bug 1256427 - Investigate moving from Pair in support library to framework. → MozReview Request: Bug 1256427 - Move to LruCache in framework from support lib. r=sebastian
Assignee | ||
Comment 21•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/42055/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/42055/
Attachment #8733978 -
Flags: review?(s.kaspari)
Assignee | ||
Updated•8 years ago
|
Attachment #8730373 -
Flags: review+ → review?(s.kaspari)
Assignee | ||
Comment 22•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c1619d54b2a
Comment 23•8 years ago
|
||
Comment on attachment 8733978 [details] MozReview Request: Bug 1256427 - Move to AtomicFile in framework from support lib. r=sebastian https://reviewboard.mozilla.org/r/42055/#review38515
Attachment #8733978 -
Flags: review?(s.kaspari) → review+
Updated•8 years ago
|
Attachment #8730373 -
Flags: review?(s.kaspari) → review+
Comment 24•8 years ago
|
||
Comment on attachment 8730373 [details] MozReview Request: Bug 1256427 - Move to LruCache in framework from support lib. r=sebastian https://reviewboard.mozilla.org/r/39819/#review38517
Assignee | ||
Comment 25•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/358a57aa077a81210fd12e1fb8fa4e6ec0039fdd Bug 1256427 - Move to LruCache in framework from support lib. r=sebastian https://hg.mozilla.org/integration/fx-team/rev/57572ba7a904bde259fa2f5a841cc06e7c816a55 Bug 1256427 - Move to AtomicFile in framework from support lib. r=sebastian
Assignee | ||
Comment 26•8 years ago
|
||
Let's continue this work in bug 1259256.
Comment 27•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/358a57aa077a https://hg.mozilla.org/mozilla-central/rev/57572ba7a904
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Assignee | ||
Comment 28•8 years ago
|
||
<bc> Does anyone have an idea about recent occurence of PROCESS-CRASH | java-exception | java.lang.NoClassDefFoundError: android.util.AtomicFile at org.mozilla.gecko.dlc.catalog.DownloadContentCatalog.<init>(DownloadContentCatalog.java:44) on Samsung GS3 which also opens the Crash reporter even though MOZ_CRASHREPORTER_NO_REPORT=1 ? AtomicFile is API 17+ – I goofed. Backing out...
Assignee | ||
Comment 29•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/5e287404e4f8fd81330cd704e42fba5e3baa5f49 Bug 1256427 - Backed out changeset 57572ba7a904 for crash on API 15-16.
Comment 30•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5e287404e4f8
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•