Skip to content

Cache dex syntethic context in dex builder #20

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Cache dex syntethic context in dex builder #20

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e837ba2
Cache dex syntethic context in dex builder
mgalindo-sc Dec 1, 2023
6dabad1
Add tests for CompatDexBuilder cache
mgalindo-sc Dec 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions src/test/java/com/google/devtools/build/android/r8/CompatDexBuilderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<CompatDexBuilder.DexingKeyR8, CompatDexBuilder.DexingEntryR8> 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);

Comment on lines +145 to +149
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Return value of processRequest is ignored

processRequest returns a non-zero exit code on failure.
Asserting the result will surface unexpected errors during the test run and prevent false positives.

-int exitA = compatDexBuilder.processRequest(dexCache, diagnostics, Arrays.asList(args), pw);
-assertThat(exitA).isEqualTo(0);
+int exitA = compatDexBuilder.processRequest(dexCache, diagnostics, Arrays.asList(args), pw);
+assertThat(exitA).isEqualTo(0);

Repeat for the second invocation.

Committable suggestion skipped: line range outside the PR's diff.

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: <synthetic-binary-name>;<context-binary-name>\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
Expand Down
59 changes: 44 additions & 15 deletions src/tools/android/java/com/google/devtools/build/android/r8/CompatDexBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -73,7 +74,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;
Expand Down Expand Up @@ -113,7 +114,7 @@ public void finished() {

private static class DexConsumer implements DexIndexedConsumer {

final ContextConsumer contextConsumer = new ContextConsumer();
ContextConsumer contextConsumer = new ContextConsumer();
byte[] bytes;

@Override
Expand All @@ -131,6 +132,10 @@ void setBytes(byte[] byteCode) {
this.bytes = byteCode;
}

void setContextConsumer(ContextConsumer contextConsumer) {
this.contextConsumer = contextConsumer;
}

ContextConsumer getContextConsumer() {
return contextConsumer;
}
Expand All @@ -148,17 +153,17 @@ public static void main(String[] args)
PrintStream realStdErr = System.err;

// Set up dexer cache
Cache<DexingKeyR8, byte[]> dexCache =
Cache<DexingKeyR8, DexingEntryR8> 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))
Comment on lines 160 to 161
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The cache weight calculation includes key.classfileContent().length. Is it necessary to include the entire class file content length in the weight? Could a hash of the class file content be used instead to reduce memory usage and improve performance?

                      return key.classfileContent().length + value.dexContent().length;

.weigher(
new Weigher<DexingKeyR8, byte[]>() {
new Weigher<DexingKeyR8, DexingEntryR8>() {
@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();
Expand Down Expand Up @@ -186,8 +191,9 @@ public int weigh(DexingKeyR8 key, byte[] value) {
}
}

private int processRequest(
@Nullable Cache<DexingKeyR8, byte[]> dexCache,
@VisibleForTesting
int processRequest(
@Nullable Cache<DexingKeyR8, DexingEntryR8> dexCache,
DiagnosticsHandler diagnosticsHandler,
List<String> args,
PrintWriter pw) {
Expand All @@ -205,7 +211,7 @@ private int processRequest(

@SuppressWarnings("JdkObsolete")
private void dexEntries(
@Nullable Cache<DexingKeyR8, byte[]> dexCache,
@Nullable Cache<DexingKeyR8, DexingEntryR8> dexCache,
List<String> args,
DiagnosticsHandler dexDiagnosticsHandler)
throws IOException, InterruptedException, ExecutionException, OptionsParsingException {
Expand Down Expand Up @@ -332,7 +338,7 @@ private void dexEntries(
}

private DexConsumer dexEntry(
@Nullable Cache<DexingKeyR8, byte[]> dexCache,
@Nullable Cache<DexingKeyR8, DexingEntryR8> dexCache,
ZipFile zipFile,
ZipEntry classEntry,
CompilationMode mode,
Expand All @@ -349,18 +355,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;
Comment on lines +359 to 372
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Shared ContextConsumer instance may be read concurrently

On a cache hit the same ContextConsumer instance is handed out to every new DexConsumer:

consumer.setContextConsumer(cachedDexEntry.contextConsumer());

Even though we currently never mutate it after caching, future refactors (or a D8 upgrade) could call acceptSyntheticInfo again, causing racy writes on a cached object.
Safer pattern:

-        consumer.setContextConsumer(cachedDexEntry.contextConsumer());
+        // copy-by-value to guarantee immutability
+        ContextConsumer ctxCopy = new ContextConsumer();
+        ctxCopy.sythesizedPrimaryClass =
+            cachedDexEntry.contextConsumer().sythesizedPrimaryClass;
+        ctxCopy.contextOfSynthesizedClass =
+            cachedDexEntry.contextConsumer().contextOfSynthesizedClass;
+        consumer.setContextConsumer(ctxCopy);

(or cache the String mapping as proposed earlier).

}
builder.addClassProgramData(
Expand All @@ -371,7 +379,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;
}
Expand All @@ -396,6 +406,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
Expand Down
Loading