Guarantee classes are packed with their inner synthetic lambdas during sharding #16369

mauriciogg · 2022-10-01T00:46:07Z

If a desugared lambda is packed in a different shard than the one from its original class D8 will fail with the following message:

Attempt at compiling intermediate artifact without its context

Changed sharding code so that it treats a class and synthetic lambda classes as a single unit.

Fixes #16368

Bencodes · 2022-10-04T21:17:51Z

@mauriciogg i'm still able to repro this using this repo:

Error in bazel-out/darwin_arm64-dbg/bin/apps/app/_dx/bin_devDebug/shard9.jar.dex.zip:com/lyft/android/camera/gallery/GalleryService$$ExternalSyntheticLambda2.class.dex:
Attempt at compiling intermediate artifact without its context
Error in bazel-out/darwin_arm64-dbg/bin/apps/app/_dx/bin_devDebug/shard9.jar.dex.zip:com/lyft/android/camera/gallery/GalleryService$$ExternalSyntheticLambda0.class.dex:
Attempt at compiling intermediate artifact without its context
Error in bazel-out/darwin_arm64-dbg/bin/apps/app/_dx/bin_devDebug/shard9.jar.dex.zip:com/lyft/android/camera/gallery/GalleryService$$ExternalSyntheticLambda10.class.dex:
Attempt at compiling intermediate artifact without its context
Error in bazel-out/darwin_arm64-dbg/bin/apps/app/_dx/bin_devDebug/shard9.jar.dex.zip:com/lyft/android/camera/gallery/GalleryService$$ExternalSyntheticLambda1.class.dex:
Attempt at compiling intermediate artifact without its context
Merge failed: Compilation failed to complete, origin: bazel-out/darwin_arm64-dbg/bin/apps/app/_dx/bin_devDebug/shard9.jar.dex.zip:com/lyft/android/camera/gallery/GalleryService$$ExternalSyntheticLambda1.class.dex
Exception in thread "main" com.android.tools.r8.CompilationFailedException: Merge failed: Compilation failed to complete, origin: bazel-out/darwin_arm64-dbg/bin/apps/app/_dx/bin_devDebug/shard9.jar.dex.zip:com/lyft/android/camera/gallery/GalleryService$$ExternalSyntheticLambda1.class.dex
        at com.google.devtools.build.android.r8.DexFileMerger.main(DexFileMerger.java:389)

mauriciogg · 2022-10-04T23:05:59Z

@Bencodes

@mauriciogg i'm still able to repro this using this repo:

Error in bazel-out/darwin_arm64-dbg/bin/apps/app/_dx/bin_devDebug/shard9.jar.dex.zip:com/lyft/android/camera/gallery/GalleryService$$ExternalSyntheticLambda2.class.dex:
Attempt at compiling intermediate artifact without its context
Error in bazel-out/darwin_arm64-dbg/bin/apps/app/_dx/bin_devDebug/shard9.jar.dex.zip:com/lyft/android/camera/gallery/GalleryService$$ExternalSyntheticLambda0.class.dex:
Attempt at compiling intermediate artifact without its context
Error in bazel-out/darwin_arm64-dbg/bin/apps/app/_dx/bin_devDebug/shard9.jar.dex.zip:com/lyft/android/camera/gallery/GalleryService$$ExternalSyntheticLambda10.class.dex:
Attempt at compiling intermediate artifact without its context
Error in bazel-out/darwin_arm64-dbg/bin/apps/app/_dx/bin_devDebug/shard9.jar.dex.zip:com/lyft/android/camera/gallery/GalleryService$$ExternalSyntheticLambda1.class.dex:
Attempt at compiling intermediate artifact without its context
Merge failed: Compilation failed to complete, origin: bazel-out/darwin_arm64-dbg/bin/apps/app/_dx/bin_devDebug/shard9.jar.dex.zip:com/lyft/android/camera/gallery/GalleryService$$ExternalSyntheticLambda1.class.dex
Exception in thread "main" com.android.tools.r8.CompilationFailedException: Merge failed: Compilation failed to complete, origin: bazel-out/darwin_arm64-dbg/bin/apps/app/_dx/bin_devDebug/shard9.jar.dex.zip:com/lyft/android/camera/gallery/GalleryService$$ExternalSyntheticLambda1.class.dex
        at com.google.devtools.build.android.r8.DexFileMerger.main(DexFileMerger.java:389)

are all GalleryService$$ExternalSyntheticLambda in the same shard that gallery service? If so it might be that lambdas itself need to appear in order as well and my code does just sorts alphabetically which misses the ordering of lambdas.

Bencodes · 2022-10-05T00:48:19Z

It looks like they are being split between two dex files.

➜ zipinfo bazel-out/darwin_arm64-dbg/bin/apps/app/_dx/bin_devDebug/shard9.jar.dex.zip | grep "GalleryService"
-rw----     1.0 fat     1268 bx stor 69-Dec-31 16:00 com/lyft/android/camera/gallery/GalleryService$$ExternalSyntheticLambda0.class.dex
-rw----     1.0 fat     1284 bx stor 69-Dec-31 16:00 com/lyft/android/camera/gallery/GalleryService$$ExternalSyntheticLambda1.class.dex
-rw----     1.0 fat     1308 bx stor 69-Dec-31 16:00 com/lyft/android/camera/gallery/GalleryService$$ExternalSyntheticLambda10.class.dex
-rw----     1.0 fat     1392 bx stor 69-Dec-31 16:00 com/lyft/android/camera/gallery/GalleryService$$ExternalSyntheticLambda2.class.dex
-rw----     1.0 fat     1332 bx stor 69-Dec-31 16:00 com/lyft/android/camera/gallery/GalleryService$$ExternalSyntheticLambda3.class.dex

While the main class is in:

➜ zipinfo bazel-out/darwin_arm64-dbg/bin/apps/app/_dx/bin_devDebug/shard10.jar.dex.zip | grep "GalleryService"
-rw----     1.0 fat     1344 bx stor 69-Dec-31 16:00 com/lyft/android/camera/gallery/GalleryService$$ExternalSyntheticLambda4.class.dex
-rw----     1.0 fat     1272 bx stor 69-Dec-31 16:00 com/lyft/android/camera/gallery/GalleryService$$ExternalSyntheticLambda5.class.dex
-rw----     1.0 fat     1288 bx stor 69-Dec-31 16:00 com/lyft/android/camera/gallery/GalleryService$$ExternalSyntheticLambda6.class.dex
-rw----     1.0 fat     1396 bx stor 69-Dec-31 16:00 com/lyft/android/camera/gallery/GalleryService$$ExternalSyntheticLambda7.class.dex
-rw----     1.0 fat     1400 bx stor 69-Dec-31 16:00 com/lyft/android/camera/gallery/GalleryService$$ExternalSyntheticLambda8.class.dex
-rw----     1.0 fat     1448 bx stor 69-Dec-31 16:00 com/lyft/android/camera/gallery/GalleryService$$ExternalSyntheticLambda9.class.dex
-rw----     1.0 fat     8956 bx stor 69-Dec-31 16:00 com/lyft/android/camera/gallery/GalleryService.class.dex
-rw----     1.0 fat     2584 bx stor 69-Dec-31 16:00 com/lyft/android/camera/gallery/GalleryService_Factory.class.dex

If a desugared lambda is packed in a different shard than the one from its original class D8 will fail with the following message: Attempt at compiling intermediate artifact without its context Changed sharding code so that it treats a class and an inner class as a single unit.

ahumesky · 2022-10-06T02:24:03Z

Thanks all, let us know when this is good to review. I'll probably need to get this reviewed by a few folks and run global presubmit tests internally, so it might take a little bit to get this imported.

mauriciogg · 2022-10-06T22:57:42Z

It looks like they are being split between two dex files.

➜ zipinfo bazel-out/darwin_arm64-dbg/bin/apps/app/_dx/bin_devDebug/shard9.jar.dex.zip | grep "GalleryService"
-rw----     1.0 fat     1268 bx stor 69-Dec-31 16:00 com/lyft/android/camera/gallery/GalleryService$$ExternalSyntheticLambda0.class.dex
-rw----     1.0 fat     1284 bx stor 69-Dec-31 16:00 com/lyft/android/camera/gallery/GalleryService$$ExternalSyntheticLambda1.class.dex
-rw----     1.0 fat     1308 bx stor 69-Dec-31 16:00 com/lyft/android/camera/gallery/GalleryService$$ExternalSyntheticLambda10.class.dex
-rw----     1.0 fat     1392 bx stor 69-Dec-31 16:00 com/lyft/android/camera/gallery/GalleryService$$ExternalSyntheticLambda2.class.dex
-rw----     1.0 fat     1332 bx stor 69-Dec-31 16:00 com/lyft/android/camera/gallery/GalleryService$$ExternalSyntheticLambda3.class.dex

While the main class is in:

➜ zipinfo bazel-out/darwin_arm64-dbg/bin/apps/app/_dx/bin_devDebug/shard10.jar.dex.zip | grep "GalleryService"
-rw----     1.0 fat     1344 bx stor 69-Dec-31 16:00 com/lyft/android/camera/gallery/GalleryService$$ExternalSyntheticLambda4.class.dex
-rw----     1.0 fat     1272 bx stor 69-Dec-31 16:00 com/lyft/android/camera/gallery/GalleryService$$ExternalSyntheticLambda5.class.dex
-rw----     1.0 fat     1288 bx stor 69-Dec-31 16:00 com/lyft/android/camera/gallery/GalleryService$$ExternalSyntheticLambda6.class.dex
-rw----     1.0 fat     1396 bx stor 69-Dec-31 16:00 com/lyft/android/camera/gallery/GalleryService$$ExternalSyntheticLambda7.class.dex
-rw----     1.0 fat     1400 bx stor 69-Dec-31 16:00 com/lyft/android/camera/gallery/GalleryService$$ExternalSyntheticLambda8.class.dex
-rw----     1.0 fat     1448 bx stor 69-Dec-31 16:00 com/lyft/android/camera/gallery/GalleryService$$ExternalSyntheticLambda9.class.dex
-rw----     1.0 fat     8956 bx stor 69-Dec-31 16:00 com/lyft/android/camera/gallery/GalleryService.class.dex
-rw----     1.0 fat     2584 bx stor 69-Dec-31 16:00 com/lyft/android/camera/gallery/GalleryService_Factory.class.dex

@Bencodes I can't repo: I don't see how it could be sharded that way. Looks like dexes are being sort in reverse order. The main class being in shard10 while the inner classes being in shard9. Should be the other way:

      ImmutableList<Map.Entry<String, ZipFile>> files =
          deduped
              .entrySet()
              .stream()
                   // ZipEntryComparator::compareClassNames: Ensure inner classes sort second
                   //   a = a.replace('$', '0');
                   //   b = b.replace('$', '0');
              .sorted(Comparator.comparing(e -> e.getKey(), ZipEntryComparator::compareClassNames))
              .collect(ImmutableList.toImmutableList());

My code (AFAICT) did not change the iteration order. Can you share the ordering of the dexes building without my code?

mauriciogg · 2022-10-07T01:52:22Z

@ahumesky I think the tests are failing in CI because they need the r8 jar being pulled from CI, but instead the bazel binary (version 5.3.1) uses the one from the sdk, which happens to be a very old one. Is there a way to disable those tests for the time being?

Bencodes · 2022-10-07T22:09:53Z

Some follow up here - there is some misleading branching of login inside of Bazel that might trip people up. If you set dex_shards on your android_binary, the Android binary will enter an entirely different branch of logic for sharding, which fails. When removing that dex_shards our builds hit the new sharding logic.

I re-applied the latest patches that you pushed yesterday which seems to have broken DexFileSplitter. We are getting this error now at runtime:

(15:23:25) ERROR: /private/var/tmp/_bazel_blee/499a001013731d09bffd82f8601a3161/external/bazel_tools/src/tools/android/java/com/google/devtools/build/android/dexer/BUILD:41:12: Building external/bazel_tools/src/tools/android/java/com/google/devtools/build/android/dexer/DexFileSplitter.jar (5 source files) and running annotation processors (AutoValueProcessor) [for tool] failed: (Exit 1): java failed: error executing command (from target @bazel_tools//src/too
ls/android/java/com/google/devtools/build/android/dexer:DexFileSplitter) external/lyft_jdk11/bin/java -XX:-CompactStrings '--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED' '--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED' ... (remaining 16 arguments skipped)
external/bazel_tools/src/tools/android/java/com/google/devtools/build/android/dexer/DexFileSplitter.java:20: error: package com.android.dex does not exist
import com.android.dex.Dex;
                      ^
external/bazel_tools/src/tools/android/java/com/google/devtools/build/android/dexer/DexFileSplitter.java:21: error: package com.android.dex does not exist
import com.android.dex.DexFormat;
                      ^
                      
                      
                      ^
  symbol:   class Dex
  location: class DexEntry
external/bazel_tools/src/tools/android/java/com/google/devtools/build/android/dexer/DexFileSplitter.java:378: error: cannot find symbol
      public Dex getDexFile() {
             ^
  symbol:   class Dex
  location: class DexEntry
external/bazel_tools/src/tools/android/java/com/google/devtools/build/android/dexer/DexFileSplitter.java:114: error: cannot find symbol
      defaultValue = "" + DexFormat.MAX_MEMBER_IDX,
                         ^

mauriciogg · 2022-10-07T22:35:54Z

Some follow up here - there is some misleading branching of login inside of Bazel that might trip people up. If you set dex_shards on your android_binary, the Android binary will enter an entirely different branch of logic for sharding, which fails. When removing that dex_shards our builds hit the new sharding logic.

I re-applied the latest patches that you pushed yesterday which seems to have broken DexFileSplitter. We are getting this error now at runtime:

(15:23:25) ERROR: /private/var/tmp/_bazel_blee/499a001013731d09bffd82f8601a3161/external/bazel_tools/src/tools/android/java/com/google/devtools/build/android/dexer/BUILD:41:12: Building external/bazel_tools/src/tools/android/java/com/google/devtools/build/android/dexer/DexFileSplitter.jar (5 source files) and running annotation processors (AutoValueProcessor) [for tool] failed: (Exit 1): java failed: error executing command (from target @bazel_tools//src/too
ls/android/java/com/google/devtools/build/android/dexer:DexFileSplitter) external/lyft_jdk11/bin/java -XX:-CompactStrings '--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED' '--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED' ... (remaining 16 arguments skipped)
external/bazel_tools/src/tools/android/java/com/google/devtools/build/android/dexer/DexFileSplitter.java:20: error: package com.android.dex does not exist
import com.android.dex.Dex;
                      ^
external/bazel_tools/src/tools/android/java/com/google/devtools/build/android/dexer/DexFileSplitter.java:21: error: package com.android.dex does not exist
import com.android.dex.DexFormat;
                      ^
                      
                      
                      ^
  symbol:   class Dex
  location: class DexEntry
external/bazel_tools/src/tools/android/java/com/google/devtools/build/android/dexer/DexFileSplitter.java:378: error: cannot find symbol
      public Dex getDexFile() {
             ^
  symbol:   class Dex
  location: class DexEntry
external/bazel_tools/src/tools/android/java/com/google/devtools/build/android/dexer/DexFileSplitter.java:114: error: cannot find symbol
      defaultValue = "" + DexFormat.MAX_MEMBER_IDX,
                         ^

@Bencodes oh yeah you have to remove dex_shards otherwise it does something completely different.
For the runtime issues do you have these commits?:
af1623a
6b25cb2

thats the only thing I can think that could be causing those issues

Bencodes · 2022-10-07T22:37:06Z

Yeah we have both commits.

mauriciogg · 2022-10-07T22:54:17Z

ugh android_remote_tools.WORKSPACE probably needs to be updated. Although not sure I can do that myself.

Bencodes · 2022-10-07T22:57:03Z

Actually hold up. I think it might have been user error on my part. The android_tools.tar.gz was renamed to android_tools.tar so my local testing scripts were pulling from a stale output.

Bencodes · 2022-10-07T23:04:19Z

Alright @mauriciogg confirmed that this PR does in fact work for us now.

mauriciogg · 2022-10-07T23:13:08Z

@Bencodes thanks!

Bencodes · 2022-10-07T23:14:44Z

src/test/java/com/google/devtools/build/android/dexer/BUILD

@@ -47,7 +48,10 @@ java_library(

 java_library(
    name = "testdata",
-    srcs = glob(["testdata/**/*.java"]),
+    srcs = glob(


Might need to run buildifier and google-java-format against the changes in this PR.

The code will get formatted automatically on import I think, so I don't think we have to manually invoke the formatter

ahumesky · 2022-10-11T03:49:19Z

Regarding android_remote_tools.WORKSPACE, it doesn't look like it needs to be modified, but if it does, you can make changes to it, but we'll need to make a new release of the remote tools and then update Bazel's dependency on it. It's usually not that much work.

For getting the tests to pass on CI, I think you can use the r8 jar from google's maven repo here:

bazel/WORKSPACE

Line 417 in 9ff9041

name = "android_gmaven_r8_for_testing",

by changing this dependency:

bazel/src/tools/android/java/com/google/devtools/build/android/r8/BUILD

Lines 55 to 58 in 9ff9041

    
           ] + select({ 
        
               "//external:has_androidsdk": ["//external:android/d8_jar_import"], 
        
               "//conditions:default": [], 
        
           }),

to @android_gmaven_r8_for_testing//jar

mauriciogg · 2022-10-12T01:50:49Z

Regarding android_remote_tools.WORKSPACE, it doesn't look like it needs to be modified, but if it does, you can make changes to it, but we'll need to make a new release of the remote tools and then update Bazel's dependency on it. It's usually not that much work.

For getting the tests to pass on CI, I think you can use the r8 jar from google's maven repo here:

bazel/WORKSPACE

Line 417 in 9ff9041

name = "android_gmaven_r8_for_testing",

by changing this dependency:

bazel/src/tools/android/java/com/google/devtools/build/android/r8/BUILD

Lines 55 to 58 in 9ff9041

] + select({

"//external:has_androidsdk": ["//external:android/d8_jar_import"],

"//conditions:default": [],

}),

to @android_gmaven_r8_for_testing//jar

I ended up adding that dependency on the test build file src/test/java/com/google/devtools/build/android/dexer/BUILD because I didnt want to include a test dependency in non test code, but doing it this way the gmaven jar has to always come up first in the classpath which is somewhat brittle- so let me know if you want me to move it into the r8 BUILD file.

ahumesky · 2022-10-12T20:38:28Z

Yeah good point about test code vs non-test code. Luckily //src/tools/android/java/com/google/devtools/build/android/r8:r8 is visible to only test code at the moment, so I think it would be fine to add testonly = True. It's really only for tests anyway, because the R8 source code in there gets embedded into bazel and compiled later to avoid bundling the D8/R8 jars within bazel. This is all for the desugaring classes used in the test which come from the r8 target, so it seems less surprising to change the deps there, which also avoids the class path brittleness.

mauriciogg · 2022-10-12T22:02:04Z

Yeah good point about test code vs non-test code. Luckily //src/tools/android/java/com/google/devtools/build/android/r8:r8 is visible to only test code at the moment, so I think it would be fine to add testonly = True. It's really only for tests anyway, because the R8 source code in there gets embedded into bazel and compiled later to avoid bundling the D8/R8 jars within bazel. This is all for the desugaring classes used in the test which come from the r8 target, so it seems less surprising to change the deps there, which also avoids the class path brittleness.

Done

ahumesky · 2022-10-14T20:06:36Z

I ran this against our internal global presubmit tests, and I'm getting back about 900 test targets failing with

Exception in thread "main" java.lang.IllegalStateException: Impossible to fit <foo>.class.dex and all of its inner classes in a single shard
	at com.google.common.base.Preconditions.checkState(Preconditions.java:600)
	at com.google.devtools.build.android.dexer.DexFileSplitter.processDexEntries(DexFileSplitter.java:353)
	at com.google.devtools.build.android.dexer.DexFileSplitter.processDexFiles(DexFileSplitter.java:295)
	at com.google.devtools.build.android.dexer.DexFileSplitter.splitIntoShards(DexFileSplitter.java:188)
	at com.google.devtools.build.android.dexer.DexFileSplitter.main(DexFileSplitter.java:141)

I'll look into what's going on. It would be a little surprising if there were classes with so many inner classes that they don't fit into a shard though.

ahumesky · 2022-10-14T23:38:26Z

So the root cause of the vast majority of the failures is from a generated file for protos, and has over 5,000 inner classes, with enough methods that it's bigger than what 1 shard can contain. Not totally sure offhand what can be done about this case. I'll check with the D8 team. There are 2 other root causes for the rest of the failures, they're probably similar situations.

mauriciogg · 2022-10-15T00:25:26Z

So the root cause of the vast majority of the failures is from a generated file for protos, and has over 5,000 inner classes, with enough methods that it's bigger than what 1 shard can contain. Not totally sure offhand what can be done about this case. I'll check with the D8 team. There are 2 other root causes for the rest of the failures, they're probably similar situations.

I havent confirmed this, but I think the error only happens with classes generated from desugaring lambdas, so in theory those are the only ones needed to be packed with the outer class. If thats, the case we could be smarted here and place sort lambdas right after the outer class and allow splits on other inner classes.

ahumesky · 2022-10-21T00:42:01Z

@mauriciogg were you able to test if including just the classes from desugared lambdas in a shard worked? I can re-run the internal tests with a prototype. I also asked the D8 team if that would work.

mauriciogg · 2022-10-22T14:11:16Z

@mauriciogg were you able to test if including just the classes from desugared lambdas in a shard worked? I can re-run the internal tests with a prototype. I also asked the D8 team if that would work.

Works in our codebase: I changed isOuter to isLambda and processDexFiles to use that instead :

  private boolean isLambda(String classFile) {
    String components[] = classFile.split("/");
    return components[components.length-1].contains("ExternalSyntheticLambda");
  }
 ....

  private void processDexFiles(
    for (Map.Entry<String, ZipFile> entry : filesToProcess) {
      String filename = entry.getKey();
      if (filter.test(filename)) {
        boolean isLambda = isLambda(filename);

	 // TODO: optimize- this currently creates a list of size one for most cases
	 if (!isLambda) {
	    if (!zipEntries.isEmpty()) {
		processDexEntries(zipEntries);
	    }
	    zipEntries.clear();
	}
        zipEntries.add(entry);
      }
    }
    processDexEntries(zipEntries);
  }

Also the comparison function changed to sort lambdas first as well:

    // Make sure lambdas come before any other inner class.
    a = a.replace("ExternalSyntheticLambda", "0");
    b = b.replace("ExternalSyntheticLambda", "0");

I need to fix the tests and then I'll update this PR accordingly, but I'd also like to wait for the official D8 team response just to make sure we are outputting what d8 expects.

mauriciogg · 2022-10-24T20:16:35Z

@ahumesky updated to do the split only on lambdas

arunkumar9t2 · 2022-11-07T18:53:15Z

We faced this when we migrated to v6 and we were able to workaround it temporarily by downgrading R8 to 3.0.64. This is the commit that changed the behavior in R8 if that helps https://r8-review.googlesource.com/c/r8/+/60228

ahumesky · 2022-11-08T00:32:42Z

@mauriciogg thanks, i started rerunning the tests, should have results by tomorrow

ahumesky · 2022-11-11T01:12:08Z

I ran this latest version against our internal tests, and everything passed, however in talking with the D8 team, they brought up that there are other kinds of synthetic classes that could be generated, so this only happened to work for now. They suggested to go back to grouping inner classes with their enclosing class, and simply removing the check that they'll all fit into a single dex. With multidex, it shouldn't matter for the dexmerger. I reverted the imported change to group by inner/enclosing class, and took out the check, and I started rerunning all the tests against that.

lukaciko · 2023-01-11T10:16:29Z

We seem to have hit the issue with one of those other kinds of synthetic classes. With Bazel 6.0.0, this PR applied on top and when not setting dex_shards we still get:

Error in bazel-out/<foo>/15.shard.zip:<bar>/MobileEnhancedDialogsEventFactory$EnhanceButtonTooltip-IA.class.dex:
Attempt at compiling intermediate artifact without its context
Merge failed: Compilation failed to complete, origin: bazel-out/<foo>/dexsplits/app/15.shard.zip:<bar>/MobileEnhancedDialogsEventFactory$EnhanceButtonTooltip-IA.class.dex
Exception in thread "main" com.android.tools.r8.CompilationFailedException: Merge failed: Compilation failed to complete, origin: bazel-out/<foo>/dexsplits/app/15.shard.zip:<bar>/MobileEnhancedDialogsEventFactory$EnhanceButtonTooltip-IA.class.dex
	at com.google.devtools.build.android.r8.DexFileMerger.main(DexFileMerger.java:389)

Using zipinfo I see MobileEnhancedDialogsEventFactory$EnhanceButtonTooltip-IA.class.dex is in 15.shard.zip and MobileEnhancedDialogsEventFactory$EnhanceButtonTooltip.class.dex is in 16.shard.zip while other -IA classes are in the same dex shard as their counterparts.

I don't know what these -IA classes are, but when I use a bytecode inspector I see they have a .source "D8$$SyntheticClass" line so I'm assuming they're synthetic classes generated by D8.

lukaciko · 2023-01-27T13:13:00Z

We ran into this issue with another kind of synthetic classes, ExternalSyntheticBackportWithForwarding0, when using Bazel 6 + this change + handling the -IA classes I mentioned above.

Merge failed: Compilation failed to complete, origin: bazel-out/darwin_arm64-fastbuild/bin/<foo>/_dx/app/shard21.jar.dex.zip:io/reactivex/rxjava3/internal/operators/observable/ObservableReplay$$ExternalSyntheticBackportWithForwarding0.class.dex

Looking forward to a generic fix. Until then it is too risky for us to start using Bazel 6.

mauriciogg · 2023-01-27T19:11:35Z

We ran into this issue with another kind of synthetic classes, ExternalSyntheticBackportWithForwarding0, when using Bazel 6 + this change + handling the -IA classes I mentioned above.
Merge failed: Compilation failed to complete, origin: bazel-out/darwin_arm64-fastbuild/bin/<foo>/_dx/app/shard21.jar.dex.zip:io/reactivex/rxjava3/internal/operators/observable/ObservableReplay$$ExternalSyntheticBackportWithForwarding0.class.dex
Looking forward to a generic fix. Until then it is too risky for us to start using Bazel 6.

A generic fix would be to just group all inner classes. Looks like support for this already exists in mobile-install https://github.com/bazelbuild/rules_android/blob/pre-alpha/src/tools/ak/dex/dex.go#L265

ahumesky · 2023-02-03T23:26:09Z

I ran our internal tests against this, and ran into a lot of failures -- looking through the code it was because this bit of code:

https://github.com/bazelbuild/bazel/pull/16369/files#diff-f01bb745669c01a82d58e3c3cbd6a07f570b297f292cf906634bf4d3a0712089R352-R357

somehow became just

    if (tracker.track(dexFiles)) {
      nextShard();
    }

leading to a number of dexfiles going over the 64k limit because not all the dexes were counted. That took a while to figure out. Not sure what happened there...

In any event, I've reimported the change with the correct code, and adjusted it to group by inner class again, and started rerunning all our tests. I'll hopefully have results on Monday. We should be able to get this reviewed and imported in time for 6.1.0.

ahumesky · 2023-02-07T01:47:19Z

Looking at the results, and there are many failures like:

Impossible to fit _____ and all of its synthetic lambda classes in a single shard

I didn't update the error message -- it's really "and all of its inner classes".

I looked into one of the apps with the failure, and looking at all the classes it looked like it didn't have any classes with 64k+ inner classes, so this will need a little more investigating.

ahumesky · 2023-02-24T04:43:21Z

It looks like we have some classes that have enough inner classes to exceed the dex limits for a single dex, so it doesn't look like grouping by just inner classes will work. With help from the D8 folks, I reworked the change so that it groups only classes that are truly synthetic classes, which have an annotation on them from R8.

Since I can't readily repro the original problem, it would be very helpful if those of you who are running into this can try out the updated PR:
https://bazel-review.googlesource.com/c/bazel/+/211231

lukaciko · 2023-02-24T09:27:58Z

@ahumesky thanks for the update. With that patch we're still seeing the issue with proto -IA classses:

Exception in thread "main" com.android.tools.r8.CompilationFailedException: Merge failed: Compilation failed to complete, origin: bazel-out/darwin_arm64-fastbuild/bin/src/main/java/<foo>/dexsplits/app/8.shard.zip:<bar>/proto/FooClass$Builder-IA.class.dex
	at com.google.devtools.build.android.r8.DexFileMerger.main(DexFileMerger.java:38

FooClass$Builder-IA.class.dex is annotated with SynthesizedClassV2, so that shouldn't be a problem. The next shard contains:

FooClass.class.dex
FooClass$1.class.dex
FooClass$Builder.class.dex
FooClassOrBuilder.class.dex
FooClassOuterClass.class.dex

Edit: I believe there's an error around processSyntheticClassesDexEntries. It should check if syntheticInnerClasses and regularClasses fit into the current dex shard, not just the former.

ahumesky · 2023-02-25T00:39:25Z

Thanks for trying this and for the detailed report -- I think the problem is that the code doesn't actually make sure that the outer class is with the synthetic classes. The outer class + its synthetic inner classes have to fit into one shard, but the "regular" inner classes can go into any shard. Because the outer class is being put in with the "regular" classes, it could potentially be placed into the next shard.

So I think this bit:

        if (isOuterClass || !isR8SyntheticClass(dexEntry.getDexFile())) {
          regularClasses.add(dexEntry);
        } else {
          syntheticInnerClasses.add(dexEntry);
        }

needs to be:

        if (isOuterClass || isR8SyntheticClass(dexEntry.getDexFile())) {
          // Outer classes are never synthetic classes, however since the synthetic classes have to
          // go with their outer class, add the outer class with the synthetic classes.
          syntheticInnerClasses.add(dexEntry);
        } else {
          regularClasses.add(dexEntry);
        }

For completeness, I've uploaded a new patch with the above to https://bazel-review.googlesource.com/c/bazel/+/211231

But all the above said, after talking through all of this with the D8 team, they're a bit worried about how tightly coupled this all is to implementation details of D8 (the annotation, the annotation class name, the naming conventions), not to mention it's not really great for performance to have to parse the dex files. (The code is also getting pretty complicated.)

The D8 team proposes:

Update CompatDexBuilder to use the D8 DexFilePerClassFile consumer instead of DexIndexed (this should be a no op for the build as the current build there is already doing one dex per class file, but it uses another D8 API such that it can get the synthetic info).
Update the D8 DexFilePerClassFile accept function to also receive the "synthetic context" information if the compiled class is a compiler synthesized class.
Update CompatDexBuilder to write the synthetic context information to some build meta info for later access by the DexFileSplitter. Something like this is already done for the desugar dependencies which are written to a proto in META-INF/desugar_deps.
Update DexFileSplitter to use the meta-inf of synthetic classes to group with the right contexts.

The D8 team would handle 1 and 2, and the bazel team and/or community could do 3 and 4. This avoids coupling to D8 implementation details and having to parse dex files and hopefully reduces the code complexity a bit.

ahumesky · 2023-08-04T00:54:17Z

This should be fixed in Bazel 6.3.0:
#18808

mauriciogg requested review from ahumesky, ted-xie and kevin1e100 as code owners October 1, 2022 00:46

sgowroji added team-Android Issues for Android team awaiting-user-response Awaiting a response from the author awaiting-review PR is awaiting review from an assigned reviewer and removed awaiting-user-response Awaiting a response from the author labels Oct 1, 2022

mauriciogg force-pushed the mau-dex-sharding-fix branch from 882e691 to d2f03bc Compare October 6, 2022 02:11

Bencodes reviewed Oct 7, 2022

View reviewed changes

mauriciogg force-pushed the mau-dex-sharding-fix branch from 458afff to d2f03bc Compare October 12, 2022 21:45

mauriciogg and others added 2 commits October 12, 2022 14:47

clean code

a6919b9

Use r8 from maven instead of sdk

1c3f1bb

mauriciogg force-pushed the mau-dex-sharding-fix branch from 6393df1 to 1c3f1bb Compare October 12, 2022 22:01

split on lambdas

00863f0

mauriciogg changed the title ~~Guarantee classes are packed with their inner classes during sharding~~ Guarantee classes are packed with their inner synthetic lambdas during sharding Oct 24, 2022

update comments

464ab8a

lukaciko mentioned this pull request Jan 23, 2023

D8 dex merger fails when a synthetic class is placed on a different shard than its container class #16368

Closed

lukaciko mentioned this pull request Feb 27, 2023

Some DexShardsToMerge/DexMerger outputs do not get downloaded when using BwtB #17603

Closed

arunkumar9t2 mentioned this pull request Apr 14, 2023

Downgrade R8 to 3.0.64 #18107

Closed

ahumesky closed this Aug 4, 2023

github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Aug 4, 2023

Guarantee classes are packed with their inner synthetic lambdas during sharding #16369

Guarantee classes are packed with their inner synthetic lambdas during sharding #16369

Conversation

mauriciogg commented Oct 1, 2022 • edited Loading

Bencodes commented Oct 4, 2022

mauriciogg commented Oct 4, 2022 • edited Loading

Bencodes commented Oct 5, 2022

ahumesky commented Oct 6, 2022

mauriciogg commented Oct 6, 2022

mauriciogg commented Oct 7, 2022

Bencodes commented Oct 7, 2022 • edited Loading

mauriciogg commented Oct 7, 2022

Bencodes commented Oct 7, 2022

mauriciogg commented Oct 7, 2022

Bencodes commented Oct 7, 2022

Bencodes commented Oct 7, 2022

mauriciogg commented Oct 7, 2022

Bencodes Oct 7, 2022

Choose a reason for hiding this comment

ahumesky Oct 11, 2022

Choose a reason for hiding this comment

ahumesky commented Oct 11, 2022

mauriciogg commented Oct 12, 2022

ahumesky commented Oct 12, 2022

mauriciogg commented Oct 12, 2022

ahumesky commented Oct 14, 2022

ahumesky commented Oct 14, 2022

mauriciogg commented Oct 15, 2022

ahumesky commented Oct 21, 2022

mauriciogg commented Oct 22, 2022 • edited Loading

mauriciogg commented Oct 24, 2022

arunkumar9t2 commented Nov 7, 2022 • edited Loading

ahumesky commented Nov 8, 2022

ahumesky commented Nov 11, 2022

lukaciko commented Jan 11, 2023

lukaciko commented Jan 27, 2023

mauriciogg commented Jan 27, 2023

ahumesky commented Feb 3, 2023

ahumesky commented Feb 7, 2023

ahumesky commented Feb 24, 2023

lukaciko commented Feb 24, 2023 • edited Loading

ahumesky commented Feb 25, 2023 • edited Loading

ahumesky commented Aug 4, 2023

mauriciogg commented Oct 1, 2022 •

edited

Loading

mauriciogg commented Oct 4, 2022 •

edited

Loading

Bencodes commented Oct 7, 2022 •

edited

Loading

mauriciogg commented Oct 22, 2022 •

edited

Loading

arunkumar9t2 commented Nov 7, 2022 •

edited

Loading

lukaciko commented Feb 24, 2023 •

edited

Loading

ahumesky commented Feb 25, 2023 •

edited

Loading