Closed
Bug 1468487
Opened 7 years ago
Closed 7 years ago
Method count is getting close to the 65k limit for some local build configurations
Categories
(Firefox Build System :: Android Studio and Gradle Integration, enhancement)
Firefox Build System
Android Studio and Gradle Integration
Tracking
(firefox63 fixed)
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: JanH, Assigned: petru)
References
Details
Attachments
(1 file)
... and upgrading the support library versions makes things worse.
Assignee | ||
Comment 1•7 years ago
|
||
As discussed here bug 1414054 comment 2 building with mach which just runs `officialWithoutGeckoBinariesNoMinApiPhotonDebug` is still possible.
Maybe we could enable multidex for the local builds to start?
And only when truly needed enable multidex on the official builds? (As multidex means an increase in apk size and a slightly slower first startup)
Comment 2•7 years ago
|
||
(In reply to Petru-Mugurel Lingurar[:petru] from comment #1)
> As discussed here bug 1414054 comment 2 building with mach which just runs
> `officialWithoutGeckoBinariesNoMinApiPhotonDebug` is still possible.
>
> Maybe we could enable multidex for the local builds to start?
> And only when truly needed enable multidex on the official builds? (As
> multidex means an increase in apk size and a slightly slower first startup)
Yes, we can do this. I tried to find my discussions of multiDex with Max Liu and couldn't; let me try again. OK, they were in https://bugzilla.mozilla.org/show_bug.cgi?id=1411654 and related bugs that you're on, Petru.
This should Just Work after enabling multiDex in the Gradle file _and_ replacing `GeckoApplication` with something that extends `MultiDexApplication` or whatever in the code. It's that part that's tricky. It's possible that we don't need to do that at all any more; I think it was needed perhaps for API v15 and below, which we no longer target?
So yes, it would be helpful if somebody could try to do this for non-official builds (local flavor dimension).
Assignee | ||
Comment 3•7 years ago
|
||
According to https://developer.android.com/studio/build/multidex
"Versions of the platform prior to Android 5.0 (API level 21) use the Dalvik runtime for executing app code. By default, Dalvik limits apps to a single classes.dex bytecode file per APK. In order to get around this limitation, you can add the multidex support library to your project"
So I think we can just include the dependencies in gradle, including `multiDexEnabled true` but only enable Multidex from inside the Application if the flavor is local and minSdk < 21.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → petru.lingurar
Comment hidden (mozreview-request) |
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8985316 [details]
Bug 1468487 - Enable multidex for local builds;
https://reviewboard.mozilla.org/r/250936/#review257376
If try is green and this works for you locally, I'm okay with it. (Try will verify that we don't accidentally lose some DEX code when we package/re-package.)
::: mobile/android/app/build.gradle:223
(Diff revision 1)
> implementation "com.android.support:cardview-v7:$support_library_version"
> implementation "com.android.support:recyclerview-v7:$support_library_version"
> implementation "com.android.support:design:$support_library_version"
> implementation "com.android.support:customtabs:$support_library_version"
> implementation "com.android.support:palette-v7:$support_library_version"
> + implementation "com.android.support:multidex:1.0.3"
Worth a comment:
```We always include support for multidexing even when we don't enable it at runtime. We could conditionally include support, but we'd need to generate the `Application` class or fork the file on disk.```
::: mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java:376
(Diff revision 1)
>
> + @Override
> + protected void attachBaseContext(Context base) {
> + super.attachBaseContext(base);
> +
> + // We want Multidex enabled only for local builds atm
nit: full sentence, no abbreviation:
```
// Enable multiDex only for local builds.
```
Attachment #8985316 -
Flags: review?(nalexander) → review+
Comment 6•7 years ago
|
||
Robocop and the android-* suites (lint, etc) would be good to test on try.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•7 years ago
|
||
Updated the patch with your suggestions, thanks!
For me the patch does it's job.
After importing the patch from bug 1385464 and doing a local build I see
> Total methods in app-local-withGeckoBinaries-minApi21-photon-debug.apk: 66991 (102.22% used)
> Total fields in app-local-withGeckoBinaries-minApi21-photon-debug.apk: 43603 (66.53% used)
> Total classes in app-local-withGeckoBinaries-minApi21-photon-debug.apk: 8706 (13.28% used)
I also pushed to try with all tests included, all seems fine - https://treeherder.mozilla.org/#/jobs?repo=try&revision=3acd7bd415941b3f744ff72f3c877c2f953db8df
Assignee | ||
Comment 9•7 years ago
|
||
After Andrei's recent patch for updating Google Play Services which added play-services-ads and ~10.000 the new method stats are:
> Total methods in app-official-withoutGeckoBinaries-noMinApi-photon-debug.apk: 76300 (116.43% used)
> Total fields in app-official-withoutGeckoBinaries-noMinApi-photon-debug.apk: 47925 (73.13% used)
> Total classes in app-official-withoutGeckoBinaries-noMinApi-photon-debug.apk: 8820 (13.46% used)
This is a very big increase in method count and in apk size and needs to be investigated on how can it's impact be reduced but probably we'll need to multidex on the official builds also.
Assignee | ||
Comment 10•7 years ago
|
||
Latest stats from mozilla-central from :app:countLocalWithGeckoBinariesMinApi21PhotonDebugDexMethods
> Total methods in app-local-withGeckoBinaries-minApi21-photon-debug.apk: 65624 (100.14% used)
> Total fields in app-local-withGeckoBinaries-minApi21-photon-debug.apk: 38869 (59.31% used)
> Total classes in app-local-withGeckoBinaries-minApi21-photon-debug.apk: 8418 (12.85% used)
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 11•7 years ago
|
||
Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4eb6f5567674
Enable multidex for local builds; r=nalexander
Keywords: checkin-needed
Comment 12•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Updated•6 years ago
|
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 63 → mozilla63
You need to log in
before you can comment on or make changes to this bug.
Description
•