From e837ba2bbdca9bb45f703d1e0d30f33ff1cc3e3f Mon Sep 17 00:00:00 2001 From: Mauricio Galindo Date: Fri, 1 Dec 2023 11:14:20 -0800 Subject: [PATCH 1/2] Cache dex syntethic context in dex builder There is a data race when using workers when there are multiple libraries that include the same class. This is more likely to happen when building multiple android_binary targets in a single invocation. When a cached entry is found, only the dex content is used and the sythetic info is ignored. In those instances, the dex archive will be incorrectly created and might lead to failures during dex merging --- .../build/android/r8/CompatDexBuilder.java | 55 ++++++++++++++----- 1 file changed, 41 insertions(+), 14 deletions(-) diff --git a/src/tools/android/java/com/google/devtools/build/android/r8/CompatDexBuilder.java b/src/tools/android/java/com/google/devtools/build/android/r8/CompatDexBuilder.java index 9d99799c25c923..05afdd87bc5cb7 100644 --- a/src/tools/android/java/com/google/devtools/build/android/r8/CompatDexBuilder.java +++ b/src/tools/android/java/com/google/devtools/build/android/r8/CompatDexBuilder.java @@ -73,7 +73,7 @@ public class CompatDexBuilder { private static final long ONE_MEG = 1024 * 1024; - private static class ContextConsumer implements SyntheticInfoConsumer { + public static class ContextConsumer implements SyntheticInfoConsumer { // After compilation this will be non-null iff the compiled class is a D8 synthesized class. ClassReference sythesizedPrimaryClass = null; @@ -113,7 +113,7 @@ public void finished() { private static class DexConsumer implements DexIndexedConsumer { - final ContextConsumer contextConsumer = new ContextConsumer(); + ContextConsumer contextConsumer = new ContextConsumer(); byte[] bytes; @Override @@ -131,6 +131,10 @@ void setBytes(byte[] byteCode) { this.bytes = byteCode; } + void setContextConsumer(ContextConsumer contextConsumer) { + this.contextConsumer = contextConsumer; + } + ContextConsumer getContextConsumer() { return contextConsumer; } @@ -148,17 +152,17 @@ public static void main(String[] args) PrintStream realStdErr = System.err; // Set up dexer cache - Cache dexCache = + Cache dexCache = CacheBuilder.newBuilder() // Use at most 200 MB for cache and leave at least 25 MB of heap space alone. For // reference: // .class & class.dex files are around 1-5 KB, so this fits ~30K-35K class-dex pairs. .maximumWeight(min(Runtime.getRuntime().maxMemory() - 25 * ONE_MEG, 200 * ONE_MEG)) .weigher( - new Weigher() { + new Weigher() { @Override - public int weigh(DexingKeyR8 key, byte[] value) { - return key.classfileContent().length + value.length; + public int weigh(DexingKeyR8 key, DexingEntryR8 value) { + return key.classfileContent().length + value.dexContent().length; } }) .build(); @@ -187,7 +191,7 @@ public int weigh(DexingKeyR8 key, byte[] value) { } private int processRequest( - @Nullable Cache dexCache, + @Nullable Cache dexCache, DiagnosticsHandler diagnosticsHandler, List args, PrintWriter pw) { @@ -205,7 +209,7 @@ private int processRequest( @SuppressWarnings("JdkObsolete") private void dexEntries( - @Nullable Cache dexCache, + @Nullable Cache dexCache, List args, DiagnosticsHandler dexDiagnosticsHandler) throws IOException, InterruptedException, ExecutionException, OptionsParsingException { @@ -332,7 +336,7 @@ private void dexEntries( } private DexConsumer dexEntry( - @Nullable Cache dexCache, + @Nullable Cache dexCache, ZipFile zipFile, ZipEntry classEntry, CompilationMode mode, @@ -349,18 +353,20 @@ private DexConsumer dexEntry( .setMinApiLevel(minSdkVersion) .setDisableDesugaring(true) .setIntermediate(true); - byte[] cachedDexBytes = null; + + DexingEntryR8 cachedDexEntry = null; byte[] classFileBytes = null; try (InputStream stream = zipFile.getInputStream(classEntry)) { classFileBytes = ByteStreams.toByteArray(stream); if (dexCache != null) { // If the cache exists, check for cache validity. - cachedDexBytes = + cachedDexEntry = dexCache.getIfPresent(DexingKeyR8.create(mode, minSdkVersion, classFileBytes)); } - if (cachedDexBytes != null) { + if (cachedDexEntry != null) { // Cache hit: quit early and return the data - consumer.setBytes(cachedDexBytes); + consumer.setBytes(cachedDexEntry.dexContent()); + consumer.setContextConsumer(cachedDexEntry.contextConsumer()); return consumer; } builder.addClassProgramData( @@ -371,7 +377,9 @@ private DexConsumer dexEntry( D8.run(builder.build(), executor); // After dexing finishes, store the dexed output into the cache. if (dexCache != null) { - dexCache.put(DexingKeyR8.create(mode, minSdkVersion, classFileBytes), consumer.getBytes()); + dexCache.put( + DexingKeyR8.create(mode, minSdkVersion, classFileBytes), + DexingEntryR8.create(consumer.getBytes(), consumer.getContextConsumer())); } return consumer; } @@ -396,6 +404,25 @@ public static DexingKeyR8 create( public abstract byte[] classfileContent(); } + /** + * Represents a cache entry in the dex cache. + */ + @AutoValue + public abstract static class DexingEntryR8 { + public static DexingEntryR8 create( + byte[] dexContent, ContextConsumer contextConsumer) { + return new AutoValue_CompatDexBuilder_DexingEntryR8( + dexContent, contextConsumer); + } + + @SuppressWarnings("mutable") + public abstract byte[] dexContent(); + + @SuppressWarnings("mutable") + public abstract ContextConsumer contextConsumer(); + } + + /** * Custom implementation of DiagnosticsHandler that writes the info/warning diagnostics messages * to original System#err stream instead of the WorkerResponse output. This keeps the Bazel From 6dabad18d6030cda99b7d8482ddf5cde440e1b71 Mon Sep 17 00:00:00 2001 From: Mauricio Galindo Date: Tue, 5 Dec 2023 13:36:03 -0800 Subject: [PATCH 2/2] Add tests for CompatDexBuilder cache --- .../android/r8/CompatDexBuilderTest.java | 53 +++++++++++++++++++ .../build/android/r8/CompatDexBuilder.java | 4 +- 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/src/test/java/com/google/devtools/build/android/r8/CompatDexBuilderTest.java b/src/test/java/com/google/devtools/build/android/r8/CompatDexBuilderTest.java index 8c88caedff91f9..d2bdcbf7c75d30 100644 --- a/src/test/java/com/google/devtools/build/android/r8/CompatDexBuilderTest.java +++ b/src/test/java/com/google/devtools/build/android/r8/CompatDexBuilderTest.java @@ -18,14 +18,19 @@ import com.android.tools.r8.D8; import com.android.tools.r8.D8Command; +import com.android.tools.r8.DiagnosticsHandler; import com.android.tools.r8.OutputMode; +import com.google.common.cache.Cache; +import com.google.common.cache.CacheBuilder; import com.google.common.collect.ImmutableList; import com.google.devtools.common.options.OptionsParsingException; import java.io.BufferedReader; import java.io.IOException; import java.io.InputStreamReader; +import java.io.PrintWriter; import java.nio.file.Files; import java.nio.file.Path; +import java.util.Arrays; import java.util.HashSet; import java.util.Set; import java.util.concurrent.ExecutionException; @@ -122,6 +127,54 @@ public void compileWithSyntheticLambdas() throws Exception { } } + @Test + public void compileWithCachedSyntheticLambdas() throws Exception { + // Test synthetic context information is cached alongside dexed classes + CompatDexBuilder compatDexBuilder = new CompatDexBuilder(); + Cache dexCache = + CacheBuilder.newBuilder().build(); + DiagnosticsHandler diagnostics = new DiagnosticsHandler() {}; + PrintWriter pw = new PrintWriter(System.err); + + final String contextName = "com/google/devtools/build/android/r8/testdata/lambda/Lambda"; + final String inputJar = System.getProperty("CompatDexBuilderTests.lambda"); + final Path outputZipA = temp.getRoot().toPath().resolve("outA.zip"); + final Path outputZipB = temp.getRoot().toPath().resolve("outB.zip"); + + String[] args = new String[] {"--input_jar", inputJar, "--output_zip", outputZipA.toString()}; + compatDexBuilder.processRequest(dexCache, diagnostics, Arrays.asList(args), pw); + + args = new String[] {"--input_jar", inputJar, "--output_zip", outputZipB.toString()}; + compatDexBuilder.processRequest(dexCache, diagnostics, Arrays.asList(args), pw); + + Path[] outputZips = new Path[] {outputZipA, outputZipB}; + for (Path outputZip: outputZips) { + try (ZipFile zipFile = new ZipFile(outputZip.toFile(), UTF_8)) { + assertThat(zipFile.getEntry(contextName + ".class.dex")).isNotNull(); + ZipEntry entry = zipFile.getEntry("META-INF/synthetic-contexts.map"); + assertThat(entry).isNotNull(); + try (BufferedReader reader = + new BufferedReader(new InputStreamReader(zipFile.getInputStream(entry), UTF_8))) { + String line = reader.readLine(); + assertThat(line).isNotNull(); + // Format of mapping is: ;\n + int sep = line.indexOf(';'); + String syntheticNameInMap = line.substring(0, sep); + String contextNameInMap = line.substring(sep + 1); + // The synthetic will be prefixed by the context type. This checks the synthetic name + // is larger than the context to avoid hardcoding the synthetic names, which may change. + assertThat(syntheticNameInMap).startsWith(contextName); + assertThat(syntheticNameInMap).isNotEqualTo(contextName); + // Check expected context. + assertThat(contextNameInMap).isEqualTo(contextName); + // Only one synthetic and its context should be present. + line = reader.readLine(); + assertThat(line).isNull(); + } + } + } + } + @Test public void compileTwoClassesAndRun() throws Exception { // Run CompatDexBuilder on dexMergeSample.jar diff --git a/src/tools/android/java/com/google/devtools/build/android/r8/CompatDexBuilder.java b/src/tools/android/java/com/google/devtools/build/android/r8/CompatDexBuilder.java index 05afdd87bc5cb7..9fad9521853bd3 100644 --- a/src/tools/android/java/com/google/devtools/build/android/r8/CompatDexBuilder.java +++ b/src/tools/android/java/com/google/devtools/build/android/r8/CompatDexBuilder.java @@ -33,6 +33,7 @@ import com.android.tools.r8.references.ClassReference; import com.android.tools.r8.utils.StringDiagnostic; import com.google.auto.value.AutoValue; +import com.google.common.annotations.VisibleForTesting; import com.google.common.cache.Cache; import com.google.common.cache.CacheBuilder; import com.google.common.cache.Weigher; @@ -190,7 +191,8 @@ public int weigh(DexingKeyR8 key, DexingEntryR8 value) { } } - private int processRequest( + @VisibleForTesting + int processRequest( @Nullable Cache dexCache, DiagnosticsHandler diagnosticsHandler, List args,