Cache dex syntethic context in dex builder #20
Conversation
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
|
I imported this and started running our internal tests against it. Also, is there a test we can add to |
Added a new test. I had to expose |
WalkthroughThis change refactors the Changes
Sequence Diagram(s)sequenceDiagram
participant Test as CompatDexBuilderTest
participant Builder as CompatDexBuilder
participant Cache as Guava Cache
participant DexConsumer
participant Output as Output ZIP
Test->>Builder: create CompatDexBuilder instance
Test->>Cache: create Guava Cache
loop For each input JAR
Test->>Builder: processRequest(cache, ...)
Builder->>Cache: lookup DexingKeyR8
alt Cache miss
Builder->>DexConsumer: create and process
DexConsumer->>Builder: provide dex bytes + context
Builder->>Cache: store DexingEntryR8 (bytes + context)
else Cache hit
Cache->>Builder: return DexingEntryR8
Builder->>DexConsumer: restore context from entry
end
Builder->>Output: write .class.dex and synthetic-contexts.map
end
Test->>Output: verify expected files and mapping
Assessment against linked issues
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @arvi18, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request addresses a data race condition that occurs when using workers with multiple libraries containing the same class. This is more likely when building multiple android_binary targets in a single invocation. The issue arises because when a cached entry is found, only the dex content is used, and the synthetic info is ignored, leading to incorrect dex archive creation and potential failures during dex merging. To resolve this, the PR caches the dex synthetic context in the dex builder. The changes involve modifying CompatDexBuilder.java to store and retrieve the ContextConsumer along with the dex bytecode in the cache. A new test case, compileWithCachedSyntheticLambdas, is added to CompatDexBuilderTest.java to verify the fix.
Highlights
- Data Race Fix: Addresses a data race when using workers with multiple libraries containing the same class by caching dex synthetic context.
- Cache Implementation: Modifies
CompatDexBuilder.javato store and retrieveContextConsumeralong with dex bytecode in the cache. - Test Case Addition: Adds
compileWithCachedSyntheticLambdastest case toCompatDexBuilderTest.javato verify the fix. - DexingEntryR8 Creation: Introduces
DexingEntryR8to encapsulate both dex content andContextConsumerfor caching.
Changelog
Click here to see the changelog
- src/test/java/com/google/devtools/build/android/r8/CompatDexBuilderTest.java
- Added imports for
DiagnosticsHandler,Cache,CacheBuilder,PrintWriter, andArrays. - Added a new test case
compileWithCachedSyntheticLambdasto verify caching of synthetic context information.
- Added imports for
- src/tools/android/java/com/google/devtools/build/android/r8/CompatDexBuilder.java
- Added
@VisibleForTestingannotation. - Made
ContextConsumera public static class. - Modified
DexConsumerto hold a non-finalContextConsumer. - Added setter method
setContextConsumerinDexConsumer. - Changed the dex cache to store
DexingEntryR8instead ofbyte[]. - Modified
processRequestanddexEntriesto use the new cache type. - Updated
dexEntryto retrieve and set theContextConsumerfrom the cache. - Created
DexingEntryR8to store both dex content andContextConsumerin the cache.
- Added
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A race condition's dread,
Threads entangled, ahead.
Cache the context,
Avoid the pretext,
Now the build runs instead.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request addresses a data race issue in the CompatDexBuilder when using workers with multiple libraries containing the same class. The changes involve caching synthetic context information along with dexed classes to ensure correct dex archive creation. The new test case compileWithCachedSyntheticLambdas validates this fix. Overall, the changes seem well-structured and address the reported issue.
Summary of Findings
- Cache Key Considerations: The current cache key
DexingKeyR8includesclassfileContent. While this ensures that changes to the class file invalidate the cache, it might be too granular. Consider whether other factors (e.g., compiler flags) should also be part of the key, or whether the entire classfile content is necessary, or if a hash of the classfile would suffice. - Error Handling in processRequest: The
processRequestmethod catches several exceptions but only prints a stack trace forIOException,InterruptedException, andExecutionException. Consider logging the exception message forOptionsParsingExceptionas well for consistency.
Merge Readiness
The pull request appears to address the data race issue effectively. The addition of the compileWithCachedSyntheticLambdas test provides confidence in the fix. However, before merging, it would be beneficial to consider the cache key considerations and error handling improvements mentioned in the review comments. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging.
| // .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)) |
There was a problem hiding this comment.
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;![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/tools/android/java/com/google/devtools/build/android/r8/CompatDexBuilder.java (1)
77-113: 💡 Verification agent🧩 Analysis chain
Public
ContextConsumerexposes mutable state outside the builder
ContextConsumeris nowpublic, yet its mutable fields (sythesizedPrimaryClass,contextOfSynthesizedClass) are package-private and can be modified from anywhere in the codebase.
This breaks encapsulation and makes incorrect external mutation (or even accidental reads during mutation) possible, which can easily re-introduce the data-race the PR is trying to solve.Two minimal mitigations:
-public static class ContextConsumer implements SyntheticInfoConsumer { +// Keep visibility package-private unless tests require otherwise. +static class ContextConsumer implements SyntheticInfoConsumer {AND/OR wrap the two fields in
private volatileand expose them only through the existing accessor, keeping the mutable state hidden.A more future-proof approach is to store the immutable mapping string in the cache instead of the whole
ContextConsumer, then construct a fresh, empty consumer on every cache hit.
That avoids sharing mutable objects across threads entirely.
🏁 Script executed:
#!/bin/bash # Show the declaration and context of ContextConsumer grep -n "class ContextConsumer" src/tools/android/java/com/google/devtools/build/android/r8/CompatDexBuilder.java # List all references to ContextConsumer in the repo (to check external usage) rg -n "ContextConsumer" .Length of output: 1935
Limit
ContextConsumervisibility and encapsulate its mutable stateTo prevent accidental external mutation (and re-introducing the data race), update
CompatDexBuilder.javaas follows:• In
src/tools/android/java/com/google/devtools/build/android/r8/CompatDexBuilder.javaat line 77, change the nested class to package-private:- public static class ContextConsumer implements SyntheticInfoConsumer { + static class ContextConsumer implements SyntheticInfoConsumer {• Hide and synchronize its mutable fields by making them
private volatile, exposing them only throughgetContextMapping():- ClassReference sythesizedPrimaryClass = null; - ClassReference contextOfSynthesizedClass = null; + private volatile ClassReference sythesizedPrimaryClass = null; + private volatile ClassReference contextOfSynthesizedClass = null;These two changes ensure callers can’t mutate internal state directly.
🧹 Nitpick comments (4)
src/tools/android/java/com/google/devtools/build/android/r8/CompatDexBuilder.java (2)
157-169: Cache weigher ignores synthetic-context size
Weigheronly countsdexContent().length, ignoring the (typically small) size of the synthetic-context map held insideContextConsumer.
If large amounts of synthetic data are ever stored, the cache may exceed its intended memory bound.- return key.classfileContent().length + value.dexContent().length; + return key.classfileContent().length + + value.dexContent().length + // synthetic info is very small but make the cost explicit + + 64;(optional – at least leave a comment clarifying why it is ignored).
409-425:DexingEntryR8stores a mutable object – breaks AutoValue immutability contractAutoValue classes are intended to be value holders; embedding a mutable
ContextConsumerviolates that expectation and the class’ generatedequals/hashCodesemantics (instance identity vs. content equality).
Caching an immutable DTO (e.g. the mapping string or anImmutableList<String>) would avoid surprises and simplify equality checks in the future.src/test/java/com/google/devtools/build/android/r8/CompatDexBuilderTest.java (2)
135-139:PrintWriteris never closed
PrintWriter pw = new PrintWriter(System.err);opens a buffered writer that should be flushed/closed.
Since it wrapsSystem.erryou may omit it altogether and passnulltoprocessRequest, or use try-with-resources:- DiagnosticsHandler diagnostics = new DiagnosticsHandler() {}; - PrintWriter pw = new PrintWriter(System.err); + DiagnosticsHandler diagnostics = new DiagnosticsHandler() {}; + try (PrintWriter pw = new PrintWriter(System.err)) { + ... + }
150-176: Missing existence assertion foroutputZipBThe second build artefact is later inspected but its existence is never checked.
Add a quick guard to fail fast if the build silently failed:final Path outputZipB = temp.getRoot().toPath().resolve("outB.zip"); ... assertThat(Files.exists(outputZipB)).isTrue();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/test/java/com/google/devtools/build/android/r8/CompatDexBuilderTest.java(2 hunks)src/tools/android/java/com/google/devtools/build/android/r8/CompatDexBuilder.java(11 hunks)
🔇 Additional comments (1)
src/tools/android/java/com/google/devtools/build/android/r8/CompatDexBuilder.java (1)
135-138: 🛠️ Refactor suggestion
setContextConsumeris unsynchronised – potential visibility issues
setContextConsumerwrites a reference that is subsequently read from another thread whengetContextConsumer()is invoked afterFuture#get.
Although today both calls happen in the same thread, the contract ofExecutorServicedoes not guarantee that future changes will respect this ordering. Marking the fieldvolatile(or synchronising the setter/getter) eliminates any out-of-order hazard at negligible cost.- ContextConsumer contextConsumer = new ContextConsumer(); + volatile ContextConsumer contextConsumer = new ContextConsumer();Likely an incorrect or invalid review comment.
| 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; |
There was a problem hiding this comment.
🛠️ 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).
| 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); | ||
|
|
There was a problem hiding this comment.
🛠️ 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.
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
Fixes: bazelbuild/rules_android#300
Summary by CodeRabbit