Closed Bug 1468487 Opened 5 years ago Closed 4 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)

enhancement
Not set
normal

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.
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)
(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).
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: nobody → petru.lingurar
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+
Robocop and the android-* suites (lint, etc) would be good to test on try.
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
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.
Blocks: 1463376
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)
Keywords: checkin-needed
Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4eb6f5567674
Enable multidex for local builds; r=nalexander
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4eb6f5567674
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
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.