Closed Bug 1256427 Opened 5 years ago Closed 5 years ago

Investigate replacing AppCompat methods with built-in framework methods, post GB, to save APK size

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

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.
via:
  ag --nofilename "import android.support" mobile/android/base/java | sort | uniq
(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
Duplicate of this bug: 1256426
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.
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).
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).
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
NI self to compare output.
Flags: needinfo?(michael.l.comella)
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)
> 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)
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)
(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!
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)
Assignee: nobody → michael.l.comella
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+
https://hg.mozilla.org/mozilla-central/rev/8f2121d6764a
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Let's try to look at some of the other small things we can swap out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
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
Attachment #8730373 - Flags: review+ → review?(s.kaspari)
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+
Attachment #8730373 - Flags: review?(s.kaspari) → review+
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
Let's continue this work in bug 1259256.
https://hg.mozilla.org/mozilla-central/rev/358a57aa077a
https://hg.mozilla.org/mozilla-central/rev/57572ba7a904
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
<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...
Duplicate of this bug: 1260485
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.