1370156 - Consider enabling jumboMode for bigger string table

Status: RESOLVED FIXED

(Firefox Build System :: Android Studio and Gradle Integration, enhancement)

Description

[Teng-pao Yu [:tyu]]

Bug 1341990 is overwhelming our string table (Default maximum: 65536)[1] during Gradle build. Following are some statistics regarding our current string table usage:

                   mach build / Gradle build
before 1341990[1]     58419   /   64097
after 1341990[2]      58419   /   70840

This is observed by running `jar xvf app.apk classes.dex && hexdump -s 56 -n 4 -e '1/4 "%d\n"' classes.dex` as suggested from https://stackoverflow.com/a/40743786/3591480 (Verification on this approach could be needed.)

Seems like it is already a must on Gradle build and we are closing to 65536 on mach build too.

[1] Can't find a canonical source for jumbo mode but maybe these would be useful:
https://developers.soundcloud.com/blog/congratulations-you-have-a-lot-of-code-remedying-androids-method-limit-part-1
https://source.android.com/devices/tech/dalvik/dalvik-bytecode

Comment 1

[Teng-pao Yu [:tyu]]

I've filed Bug 1370158 to keep track why only Gradle build is influenced by Bug 1341990. My guess is something is optimized in mach build.

Comment 2

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1370156 - Add Gradle-integration into moz.build notes.

DO NOT BUILD because NPOTB

Review commit: https://reviewboard.mozilla.org/r/145878/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/145878/

Comment 3

[Nick Alexander :nalexander [he/him]]

I pushed a commit with some docs about how the moz.build (Makefile-based) and Gradle build systems interact.  It's background to this ticket.

See also Bug 1286677, which is the "we should think about this" version of this ticket.

There are several things at play here:

1. string count vs. method count
2. jumbo mode (in moz.build and Gradle)
3. why Bug 1341990 ballooned the Gradle build but not the moz.build
4. multiDex mode (in moz.build and Gradle)
5. debug vs. release build types

1. and 2.: jumbo mode will address the string count problems.  It's easy to enable jumbo mode in both moz.build and Gradle.  I'd like to understand 3. before charging ahead with jumbo mode, since there shouldn't be much difference here.

3. The after builds are at [1], the before builds are at [2].

I find:

⋊> ~/Downloads jar xvf target-before-make.apk classes.dex ; and hexdump -s 56 -n 4 -e '1/4 "%d\n"' classes.dex                                                                                                                  52590
⋊> ~/Downloads jar xvf target-after-make.apk classes.dex ; and hexdump -s 56 -n 4 -e '1/4 "%d\n"' classes.dex                                                                                                                58488
⋊> ~/Downloads jar xvf target-before-gradle.apk classes.dex ; and hexdump -s 56 -n 4 -e '1/4 "%d\n"' classes.dex                                                                                                               59743
⋊> ~/Downloads jar xvf target-after-gradle.apk classes.dex ; and hexdump -s 56 -n 4 -e '1/4 "%d\n"' classes.dex                                                                                                              65583

So I see ~6000 strings in both Make and Gradle.  That's good, 'cuz I have no idea why they would have different rates of change (the Proguard configurations are very similar).

4. We have multiDex mode enabled for the `local` Gradle configuration, but not `localOld` or `official`.  There's a performance cost (in theory), and at one time I was worried about API levels supporting `multiDex`, but maybe those issues are solved now.  If it's safe, I'd love to DEX HLS and Sync and Google Play Services and a bunch of things separately, so we don't need to load that code at startup time.

It's not reasonable to try to multiDex in moz.build.  If we *need* multiDex, we have to use Gradle to produce our release builds of Fennec.

5. I'd like to keep `debug` builds working, 'cuz otherwise it's too painful to develop.  We could enable `multiDex` for all `local` builds/all `debug` builds.

My conclusion is that we should immediately turn on jumbo mode (this ticket), both in moz.build and Gradle.  And we need to figure out what the `multiDex` compatibility story really is, which I don't have time for right now.

:tyu, can you 1. test with --force-jumbo in Makefile.in and Gradle and 2. dig into multiDex?

[1] https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=ffaa07672466b06cd748b07a34cf95377afdde41
[2] https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=96e18bec9fc8a5ce623c16167c12756bbe190d73

Comment 4

[Teng-pao Yu [:tyu]]

(In reply to Nick Alexander :nalexander from comment #3)
> So I see ~6000 strings in both Make and Gradle.  That's good, 'cuz I have no
> idea why they would have different rates of change (the Proguard
> configurations are very similar).
> 

1. Great to see both builds are balloning, I'll take a look at what's going on on my local device. (Maybe it's because I'm building x86? I'll look into this later.)

> 4. We have multiDex mode enabled for the `local` Gradle configuration, but
> not `localOld` or `official`.  There's a performance cost (in theory), and
> at one time I was worried about API levels supporting `multiDex`, but maybe
> those issues are solved now.  If it's safe, I'd love to DEX HLS and Sync and
> Google Play Services and a bunch of things separately, so we don't need to
> load that code at startup time.

We'll need multidex support library for that. and we did already have it in our code base.

https://developer.android.com/studio/build/multidex.html#limitations

There are some limitations, mentioned here, and if you're wondering what linearalloc limit is

I've read this to figure out:

https://rschilling.wordpress.com/category/android/linearalloc/

Simply put: There are still risk with multi-dex-support-library that app may fail at run time.

The limit is extended from 5MB to 8MB since ICS(4.0) and since we're targeting 4.1

the risk is not that high but safety is not guaranteed. My personal opinion is we'll need them

as we pour new features so we could give it a try and guard our users with some testings.

> 
> It's not reasonable to try to multiDex in moz.build.  If we *need* multiDex,
> we have to use Gradle to produce our release builds of Fennec.
> 

Sounds like yet another good reason to go for Gradle :)

> 5. I'd like to keep `debug` builds working, 'cuz otherwise it's too painful
> to develop.  We could enable `multiDex` for all `local` builds/all `debug`
> builds.
> 
> My conclusion is that we should immediately turn on jumbo mode (this
> ticket), both in moz.build and Gradle.  And we need to figure out what the
> `multiDex` compatibility story really is, which I don't have time for right
> now.
> 
> :tyu, can you 1. test with --force-jumbo in Makefile.in and Gradle and 2.
> dig into multiDex?

Since its soft code freeze now, it may not be a good timing to change the behavior of our release product.

I'm thinking of landing just flip jumbomode to debugs builds to quick fix this problem and maybe add this to release later.

I'll also have some time to make sure this is a good idea, too.

(Though seems like with Gradle plugin 2.1.0 jumbo mode is default on, this sounds promising.)

For the same reason I'll try --force-jumbo later.

> 
> [1]
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> central&revision=ffaa07672466b06cd748b07a34cf95377afdde41
> [2]
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> central&revision=96e18bec9fc8a5ce623c16167c12756bbe190d73

Comment 5

[Nick Alexander :nalexander [he/him]]

> We'll need multidex support library for that. and we did already have it in
> our code base.

We already have it for Gradle, but for the `local` configuration and not the `official` configuration (which is the one that will be used eventually in automation).  We definitely don't have multiDex support in moz.build.

> Simply put: There are still risk with multi-dex-support-library that app may
> fail at run time.

Yes, but I don't think we're at particularly high risk for the linear allocation limit.

> The limit is extended from 5MB to 8MB since ICS(4.0) and since we're
> targeting 4.1

Our classes.dex is small, and could be profitably split into largely independent pieces.  HLS, for example, is completely isolated.

> 
> the risk is not that high but safety is not guaranteed. My personal opinion
> is we'll need them
> 
> as we pour new features so we could give it a try and guard our users with
> some testings.

I concur.

> > It's not reasonable to try to multiDex in moz.build.  If we *need* multiDex,
> > we have to use Gradle to produce our release builds of Fennec.
> > 
> 
> Sounds like yet another good reason to go for Gradle :)

Aye, and it might be a blocker.

> > 5. I'd like to keep `debug` builds working, 'cuz otherwise it's too painful
> > to develop.  We could enable `multiDex` for all `local` builds/all `debug`
> > builds.
> > 
> > My conclusion is that we should immediately turn on jumbo mode (this
> > ticket), both in moz.build and Gradle.  And we need to figure out what the
> > `multiDex` compatibility story really is, which I don't have time for right
> > now.
> > 
> > :tyu, can you 1. test with --force-jumbo in Makefile.in and Gradle and 2.
> > dig into multiDex?
> 
> Since its soft code freeze now, it may not be a good timing to change the
> behavior of our release product.

I'm really not concerned about jumbo mode, even for release.  More below.

> I'm thinking of landing just flip jumbomode to debugs builds to quick fix
> this problem and maybe add this to release later.

Yes.

> I'll also have some time to make sure this is a good idea, too.
> 
> (Though seems like with Gradle plugin 2.1.0 jumbo mode is default on, this
> sounds promising.)
> 
> For the same reason I'll try --force-jumbo later.

We should definitely enable jumbo mode in all Gradle debug configurations: local developers need it!

We should probably enable jumbo mode in all Gradle configurations: it's the future.

I'd prefer to enable jumbo mode in moz.build so that the Gradle configuration and the moz.build configuration stay as close together as possible.  This will increase our APK size (a little) but should be safe in the meantime.

:tyu, what do you think?

Comment 6

[Teng-pao Yu [:tyu]]

> We should definitely enable jumbo mode in all Gradle debug configurations:
> local developers need it!
> 
> We should probably enable jumbo mode in all Gradle configurations: it's the
> future.
> 
> I'd prefer to enable jumbo mode in moz.build so that the Gradle
> configuration and the moz.build configuration stay as close together as
> possible.  This will increase our APK size (a little) but should be safe in
> the meantime.
> 
> :tyu, what do you think?

I think it's a good idea to keep Gradle and mach build as consistent as possible.

I agree with the idea there is barely any risk with jumbo mode, if any. However, I still feel like it is worth it to avoid surprising our Sheriffs by landing this right before merge. This could be a good practice concerning we're in the post-dawn world without Aurora.

So I'm proposing yet another idea: maybe we could land them together but after the merge day (June 12th). What's your opinion on this plan?

I'd also like to invite :janH to give me some feedback regarding this plan. Do you think it is acceptable to drag this problem a little bit longer to not interfere the merge?

Comment 7

[Teng-pao Yu [:tyu]]

Some other random findings:

I've took a look into `jar xvf app.apk classes.dex && hexdump -s 56 -n 4 -e '1/4 "%d\n"' classes.dex` and indeed we are obtaining this string_ids_size from the header. (ref: https://source.android.com/devices/tech/dalvik/dex-format#header-item )

So it's a definite item rather than an approximate so I have no idea why 

⋊> ~/Downloads jar xvf target-after-gradle.apk classes.dex ; and hexdump -s 56 -n 4 -e '1/4 "%d\n"' classes.dex                                                                                                              65583

mentioned earlier is not problematic, it's over 65536 already!

Comment 8

[Teng-pao Yu [:tyu]]

Attachment: Bug 1370156 part 1 - Enable jumbo mode: in mach build;

Review commit: https://reviewboard.mozilla.org/r/146636/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146636/

Comment 9

[Teng-pao Yu [:tyu]]

Attachment: Bug 1370156 part 2 - Enable jumbo mode: in all Gradle builds;

Review commit: https://reviewboard.mozilla.org/r/146628/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146628/

Comment 10

[Teng-pao Yu [:tyu]]

The try is here :) https://treeherder.mozilla.org/#/jobs?repo=try&revision=9614fa4b974d0e1984fa805e849957d4c5f8d47a

I've verified jumbo is there by utilizing `./dexdump -d ~/Downloads/target-before-mach.apk | grep jumbo | head`

One thing that was unexpected is:

** The Gradle build on CI is already utilizing Jumbo mode!! **

:janH You're right again, there are some unexpected differences in the Treeherder Gradle build.

So this basically answers:

> ⋊> ~/Downloads jar xvf target-after-gradle.apk classes.dex ; and hexdump -s
> 56 -n 4 -e '1/4 "%d\n"' classes.dex                                         
> 65583
> 
> mentioned earlier is not problematic, it's over 65536 already!

I suspect it could be caused by build machine running a newer version then we suggested in mach bootstrap and is defaulting jumbo? I'll dig into this later.

Some more stats regarding the file sizes:

tyu-40227:Downloads tyu$ stat -f%z target-before-mach.apk 
36107561
tyu-40227:Downloads tyu$ stat -f%z target-after-mach.apk 
36110736
tyu-40227:Downloads tyu$ stat -f%z target-before-gradle.apk 
36733410
tyu-40227:Downloads tyu$ stat -f%z target-after-gradle.apk 
36736009

That would be 0.008% for mach and 0.007% for Gradle. Benchmarks are made upon
https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=302eb1670544f4e8d0347de03526c4fd160480e1 [before]
and after is the aforementioned try.

Comment 11

[Teng-pao Yu [:tyu]]

Comment on attachment 8875220 [details]
Bug 1370156 part 2 - Enable jumbo mode: in all Gradle builds;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146628/diff/1-2/

Comment 12

[Jan Henning [:JanH]]

(In reply to Teng-pao Yu [:tyu] from comment #6)
> I'd also like to invite :janH to give me some feedback regarding this plan.
> Do you think it is acceptable to drag this problem a little bit longer to
> not interfere the merge?

No objections - I can work around this until then.

Comment 13

[Nick Alexander :nalexander [he/him]]

(In reply to Teng-pao Yu [:tyu] from comment #6)
> > We should definitely enable jumbo mode in all Gradle debug configurations:
> > local developers need it!
> > 
> > We should probably enable jumbo mode in all Gradle configurations: it's the
> > future.
> > 
> > I'd prefer to enable jumbo mode in moz.build so that the Gradle
> > configuration and the moz.build configuration stay as close together as
> > possible.  This will increase our APK size (a little) but should be safe in
> > the meantime.
> > 
> > :tyu, what do you think?
> 
> I think it's a good idea to keep Gradle and mach build as consistent as
> possible.
> 
> I agree with the idea there is barely any risk with jumbo mode, if any.
> However, I still feel like it is worth it to avoid surprising our Sheriffs
> by landing this right before merge. This could be a good practice concerning
> we're in the post-dawn world without Aurora.
> 
> So I'm proposing yet another idea: maybe we could land them together but
> after the merge day (June 12th). What's your opinion on this plan?

That's sensible.  Let's roll with this.

> I'd also like to invite :janH to give me some feedback regarding this plan.
> Do you think it is acceptable to drag this problem a little bit longer to
> not interfere the merge?

We can safely turn on jumboMode for all debug Gradle builds in the meantime.  That should address (part of) :janh's issue.

Comment 14

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8875219 [details]
Bug 1370156 part 1 - Enable jumbo mode: in mach build;

https://reviewboard.mozilla.org/r/146636/#review150998

Comment 15

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8875220 [details]
Bug 1370156 part 2 - Enable jumbo mode: in all Gradle builds;

https://reviewboard.mozilla.org/r/146628/#review151000

This is fine for globally.

I think we can land (at any time) a patch around https://dxr.mozilla.org/mozilla-central/source/mobile/android/app/build.gradle#73 that adds:

```
buildTypes {
    debug {
        dexOptions {
            jumboMode = true
        }
    }
}
```

Comment 16

[Teng-pao Yu [:tyu]]

Comment on attachment 8875219 [details]
Bug 1370156 part 1 - Enable jumbo mode: in mach build;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146636/diff/1-2/

Comment 17

[Teng-pao Yu [:tyu]]

Comment on attachment 8875220 [details]
Bug 1370156 part 2 - Enable jumbo mode: in all Gradle builds;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146628/diff/2-3/

Comment 18

[Teng-pao Yu [:tyu]]

I've setup jumboMode for Gradle debug builds in 1367768, they will be landed soon and I've update the patches in this bug to first revert the debug-only jumbo mode config along with what was already reviewed, so depends on 1367768.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ba5082603190
part 1 - Enable jumbo mode: in mach build; r=nalexander
https://hg.mozilla.org/integration/autoland/rev/3062e322a49f
part 2 - Enable jumbo mode: in all Gradle builds; r=nalexander

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ba5082603190
https://hg.mozilla.org/mozilla-central/rev/3062e322a49f