1306021 - Reduce mach build field wastage by removing unneeded R.java copies

Status: RESOLVED FIXED

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

Description

[Andrzej Hunt :ahunt]

Our current mach builds waste a lot of fields by generating identical copies of R.java, and pasting them everywhere an R.java is needed.

Normal android builds (e.g. using gradle) apparently have a more complex way of invoking aapt to generate R.java's only containing the necessary fields for each package (I'm not hugely familiar with this)

If I completely disable proguard in mach builds (this requires replacing proguard invocations with 'cp' instead, otherwise we still run full proguard on libraries, and 1 pass of proguard on our own code), I receive an error similar to the following. (I've pasted the original error I got when trying to upgrade to support libraries 23.4.0, which is another manifestation of the same issue.)

 1:43.56 trouble writing output: Too many field references: 66810; max is 65536.

For a normal mach build apk, the field count for classes.dex is 46064, even though we clearly have a lot more fields at the dexing stage. This suggests that we're removing the unnecessary fields later in the build (i.e. we're nowhere near the limit). However 'dx' can't cope with too many fields, so we need to somehow cull the fields before then.

Fixing our aapt invocations would be nice, but is likely to be difficult. This is probably a waste of effort given how close we are to replacing the mach builds with gradle builds.

I've discovered that many of the R.java copies are unnecessary, and only exist as a result of cargo-culting existing libraries (including one or two examples by Yours Truly), removing them seems to ameliorate the problem. (Each copy of R.java has a few thousand fields, removing one of them is probably enough for the proguarded mach builds, I had to remove 2 for the completely-non-proguard custom builds that I was doing).

Comment 1

[Andrzej Hunt :ahunt]

I don't entirely understand how we're getting to 46k fields in the final output (seeing as we're trying to put close to 65k fields into classes.dex) - I don't see any proguard invocations after creating classes.dex, so maybe dx can cull the unnecessary fields, but it still balks at being given more than 65k fields in the first place?

Comment 2

[Andrzej Hunt :ahunt]

Attachment: Bug 1306021 - Remove unneeded R.java copies to reduce field wastage

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

Comment 3

[Andrzej Hunt :ahunt]

Comment on attachment 8795806 [details]
Bug 1306021 - Remove unneeded R.java copies to reduce field wastage

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

Comment 4

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8795806 [details]
Bug 1306021 - Remove unneeded R.java copies to reduce field wastage

https://reviewboard.mozilla.org/r/81742/#review80378

::: mobile/android/base/Makefile.in
(Diff revision 2)
> +# under the limit.
> +# Ideally we would fix our aapt invocations to correctly generate minimal copies of R.java for each
> +# package, but that seems redundant since gradle builds are able to correctly generate these files.
> +
>  # If native devices are enabled, add Google Play Services, build their resources
> -generated/android/support/v4/R.java: .aapt.deps ;

Consider commenting these lines with a note -- like `# (no resources) generated/android/support/v4/R.java ...`.

Comment 5

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8795806 [details]
Bug 1306021 - Remove unneeded R.java copies to reduce field wastage

https://reviewboard.mozilla.org/r/81742/#review80380

If it's green on try it's good for me.

It's worth noting that we still *generate* the unneeded R.java files (since the `aapt` command is untouched), we just don't compile them in, and hence don't DEX them.  Tricksy.

Well found!

Comment 6

[Andrzej Hunt :ahunt]

This looks green on try, so I'm going ahead (with the lines commented instead)! (note: the failures in RC3 on testBrowserDatabaseHelperUpgrades are unrelated - I was working on top of my database upgrade commits from another bug which causes that test to fail).

Comment 7

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/51a0d74d0b05d882d18e96cc3bcec6dcae1cd261
Bug 1306021 - Remove unneeded R.java copies to reduce field wastage r=nalexander

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/51a0d74d0b05

Comment 9

[Andrzej Hunt :ahunt]

Comment on attachment 8795806 [details]
Bug 1306021 - Remove unneeded R.java copies to reduce field wastage

This patch is needed in conjunction with Bug 1317880 to fix beta builds for android.

Approval Request Comment
[Feature/regressing bug #]: n/a.
[User impact if declined]: Beta can't build since we hit the dex field count limit.
[Describe test coverage new/current, TreeHerder]: Local build testing.
[Risks and why]: Low risk - removes unnecessary resource files from build, has been tested on nightly and is now on aurora too.
[String/UUID change made/needed]: none.

Comment 10

[Gerry Chang [:gchang]]

Comment on attachment 8795806 [details]
Bug 1306021 - Remove unneeded R.java copies to reduce field wastage

Fix a build issue for 51 beta for fennec. Beta51+.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b896c64a3243