New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Too many field references in Uber R.java jar to fit in a single dex #828

Closed
kageiit opened this Issue Aug 3, 2016 · 13 comments

Comments

Projects
None yet
6 participants
@kageiit
Contributor

kageiit commented Aug 3, 2016

In 4cf0eb7 , the list of fields to be included in R.java was changed from computing from the resource xml references and just using the R.txt from prebuilt aars instead. This means whatever is in R.txt gets treated as if it was declared in the package that is in the aar's manifest.

This becomes a problem in large apps that consume many prebuilt aars. If these aars depend on libraries like the android support library, gms etc, the field references of used resources coming from such dependencies will be counted against each prebuilt aar.

For example, if gms had a color resource

public static class color {
    public static final int common_google_signin_btn_text_dark=0x7f0700b0;
}

referenced in an app and aarX and aarY (both depended on by app) depend on gms, the Uber R.java jar will contain both com.aarX.R.color.common_google_signin_btn_text_dark and com.aarY.R.color.common_google_signin_btn_text_dark even if the app only refers to com.aarX.R.color.common_google_signin_btn_text_dark in its code.

This leads to a situation where all the R.java files cannot fit in a single jar to be dexed/predex because the number of filed references is > 65535.

com.facebook.buck.step.StepFailedException: Failed on step dx_&&_write_file with an exception:
field ID not in [0, 0xffff]: 65536
com.android.dex.DexIndexOverflowException: field ID not in [0, 0xffff]: 65536

trim_resource_ids does not help much because the field reference count in the custom dx packaged with buck only looks at field names and not the fully qualified name of field references for simplicity.

Had a long conversation regarding this with @dreiss yesterday. I seem to see two possible solutions to this

  • Make the referenced resource calculation in dx more fine grained by including the package name + resource name instead of just the resource name
  • Provide an option to list a primary_dex_resource_patterns and split the Uber R.java jar into multiple jars if linear_alloc_splt_dex is enabled so resources needed in the primary dex can still be kept like today.

If there are any other workarounds or other solutions, would love to know so we can try the best alternative

@dreiss

This comment has been minimized.

Member

dreiss commented Aug 3, 2016

I have a feeling that undoing 4cf0eb7 could open up edge cases that would be a pain to deal with. I'm now thinking that doing the fully-qualified name tracking is the best way to go. Want to give it a shot and see whether the perf impact is measurable?

@kageiit

This comment has been minimized.

Contributor

kageiit commented Aug 3, 2016

I opened a PR for the fully-qualified name approach. Ran a clean build on our codebase

HEAD - 88.7s [100%] (1086/1086 JOBS, 1086 UPDATED, 1086 [100.0%] CACHE MISS)

With fully qualified names - 91.8s [100%] (1086/1086 JOBS, 1086 UPDATED, 1086 [100.0%] CACHE MISS)

I also profiled the two most expensive dx steps from our build trace

HEAD:

Title   dx
Category    buck
Start   
13,572.447 ms
Wall Duration   
15,425.481 ms

Title   dx
Category    buck
Start   
9,368.561 ms
Wall Duration   
5,632.914 ms

With fully qualified names:

Title   dx
Category    buck
Start   
13,767.272 ms
Wall Duration   
26,444.564 ms

Title   dx
Category    buck
Start   
11,440.091 ms
Wall Duration   
7,884.260 ms

I think the overall performance impact during build is not significant using this approach.

@kageiit

This comment has been minimized.

Contributor

kageiit commented Aug 3, 2016

As a comparison, the number of fields in the uber R.java jar dex went down from 60570 to 2091

@dreiss

This comment has been minimized.

Member

dreiss commented Aug 4, 2016

Wait, wall duration went from 15 seconds to 26 seconds? That doesn't seem possible...

@kageiit

This comment has been minimized.

Contributor

kageiit commented Aug 4, 2016

Wait, wall duration went from 15 seconds to 26 seconds? That doesn't seem possible...

It is probably because we do some extra work for supporting robolectric tests in the MergeResourcesStep. I will profile the time taken to compute the referenced fields by getting the time in Millis before and after the computation for the change to see the impact more isolated

@kageiit

This comment has been minimized.

Contributor

kageiit commented Aug 4, 2016

I got the time before and after https://github.com/facebook/buck/blob/master/src/com/facebook/buck/android/TrimUberRDotJava.java#L119

and here are the results

Current
Time to compute referenced resources : 290.765277 ms

With FQ name
Time to compute referenced resources : 193.802684 ms

I think it is because the time gained back by not writing back more entries to the file is more than the time lost comparing with the FQ names

@dreiss

This comment has been minimized.

Member

dreiss commented Aug 5, 2016

What about the time in computeReferencedResources in dexer/Main?

@kageiit

This comment has been minimized.

Contributor

kageiit commented Aug 5, 2016

What about the time in computeReferencedResources in dexer/Main?

The time was 1~3 ms for that operation across most dexes in both cases

@zongwu233

This comment has been minimized.

zongwu233 commented Aug 17, 2016

@kageiit I encountered this problem too.
trouble writing output: Too many field references: 70201; max is 65536.
I found that the failed step is to dex Uber R.java . My project have some modules, so the generated R values is too many.
I merge your PR #830 in my local repo ,but still get the same error.Am I missing something?My project have some modules,and the generated R values are too many.

@zongwu233

This comment has been minimized.

zongwu233 commented Aug 18, 2016

I solve this problem in this way:
split Uber R.java into two parts ,one is the ${packagename}.R.java and the rest R.java.
${packagename}.R.java as UberRDotJava is trimed, compiled and dexed as before, and the rest R.java is compiled, dexed and add into preDexedLibraries .
then primer dex's fields is much less than before .

zongwu233 pushed a commit to zongwu233/buck that referenced this issue Aug 18, 2016

@HomHomLin

This comment has been minimized.

HomHomLin commented Sep 8, 2016

@zongwu233 Very useful.

@oboolean

This comment has been minimized.

oboolean commented Oct 9, 2016

@zongwu233 i still get the same error, even used the newest buck. see the issue #926

@ChaitanyaPramod

This comment has been minimized.

ChaitanyaPramod commented Feb 24, 2017

Hi, some us continue to see a similar issue over at #926 which appears to be strongly related to this issue. Can we get some help with #926?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment