Support workers in CompatDexBuilder #14623

Bencodes · 2022-01-22T01:38:20Z

This also fixes the --use_workers_with_dexbuilder flag which would pass the worker execution requirements to CompatDexBuilder without it knowing how to handle the work requests.

Using a worker here should help with some OOMs issues, and drastically improves build performance. We saw a 29% improvement in clean build times and a 14% improvement in incremental build times where the ABI was invalidated.

larsrc-google · 2022-01-24T08:46:55Z

This looks good from a worker perspective. At a quick glance, CompatDexBuilder is thread-safe, which is good. It looks like it never truly supported workers, though, so maybe release the worker support first to weed out bugs in that before adding the complexity of multiplex workers?

Bencodes · 2022-01-24T17:09:41Z

so maybe release the worker support first to weed out bugs in that before adding the complexity of multiplex workers?

I can make that change if you think that's the safest path forward. We've been using a version of this internally for quite some time and have seen no issues so far (both worker and multiplex worker impls).

Bencodes · 2022-01-25T00:35:45Z

Reverted the multiplex changes out of this PR and will follow up in a second PR with those additional changes.

larsrc-google · 2022-01-31T11:27:41Z

How does the CompatDexBuilder normally output warnings and errors for the user to see?

Bencodes · 2022-01-31T18:39:17Z

D8#run (the tool that CompatDexBuilder calls into) dump warnings and errors directly into the standard system streams by way of DiagnosticsHandler https://r8.googlesource.com/r8/+/refs/heads/main/src/main/java/com/android/tools/r8/DiagnosticsHandler.java.

Bencodes · 2022-01-31T18:45:25Z

We can provide out own implementation of DiagnosticsHandler that gets passed into D8Command#builder and redirect that info directly into the PrintWriter that the work request provides. Bazel unfortunately seems to be using an older version of D8 that doesn't support passing a custom PrintWriter instance into the error/info/warning functions.

I see two options:

Replicate DiagnosticsHandler and replace the System.err/System.out usages with the PrintWriter that theWorkRequestHandler provides. Not super ideal because we'd be duplicating how D8 internally reports information to the user, and can become outdated between updates.
Mirror what's being done in the ResourceProcessorBusyBox pull request and swap out the system streams with our own catch-all streams that can be properly written to the work response. This is the approach that we took internally for our CompatDexBuilder multiplex worker implementation.

Bencodes · 2022-02-11T16:35:27Z

Pulled some benchmarks for this worker changes (includes multiplex). We saw a 29% improvement in clean build times and a 14% improvement in incremental build times where the ABI was invalidated.

Bencodes · 2022-02-24T17:25:39Z

@larsrc-google are there next steps here for getting in?

larsrc-google · 2022-02-07T15:22:04Z

src/tools/android/java/com/google/devtools/build/android/r8/CompatDexBuilder.java

+      return 1;
+    } finally {
+      // Write the captured buffer to the work response
+      synchronized (LOCK) {


You don't need a separate lock object, it would be enough to lock on buf.

larsrc-google · 2022-02-07T15:23:17Z

src/tools/android/java/com/google/devtools/build/android/r8/CompatDexBuilder.java

      throws IOException, InterruptedException, ExecutionException, OptionsParsingException {
    CompatDexBuilder compatDexBuilder = new CompatDexBuilder();
-    compatDexBuilder.processRequest(args);
+    if (args.length == 1 && args[0].equals("--persistent_worker")) {


It would be more future-proof if you test for args.length >= 1.

Updating using ImmutableSet.copyOf(args).contains("--persistent_worker") to better align with the example worker

bazel/src/test/java/com/google/devtools/build/lib/worker/ExampleWorkerMultiplexer.java

Line 71 in 6d1b972

if (ImmutableSet.copyOf(args).contains("--persistent_worker")) {

and so that we aren't making assumptions about argument positions.

Happy to revert back to args.length >= 1 && args[0].equals("--persistent_worker") if you think the extra copy isn't necessary though.

larsrc-google · 2022-03-07T10:48:15Z

src/tools/android/java/com/google/devtools/build/android/r8/CompatDexBuilder.java

+    } finally {
+      // Write the captured buffer to the work response
+      synchronized (buf) {
+        String captured = buf.toString().trim();


You should apply a charset (UTF_8) to the toString() method.

larsrc-google · 2022-03-07T10:48:51Z

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

    visibility = ["//src/test/java/com/google/devtools/build/android/r8:__pkg__"],
    deps = [
+        "//src/main/java/com/google/devtools/build/lib/worker:work_request_handlers",
+        "//src/main/java/com/google/devtools/build/lib/worker:worker",


Do you actually need to depend on :worker? That should only be for internal (server-side) use.

ted-xie · 2022-04-14T23:10:08Z

Hi Ben,

Thanks for this PR. We're in the process of importing it upstream now. We've discovered that this PR actually requires a manual update and release for the remote Android tools repo, so we'll need a little longer before we can close this out.

Prerequisite to merge #14623 RELNOTES: Advance android_tools_pkg version to 0.24.0. PiperOrigin-RevId: 442930486

Bencodes requested review from ahumesky and lberki as code owners January 22, 2022 01:38

Bencodes force-pushed the Support-multiplex–workers-in-CompatDexBuilder branch from c1f902d to 446f8bf Compare January 22, 2022 01:48

larsrc-google mentioned this pull request Jan 24, 2022

Add support for wrapping system streams in WorkRequestHandler #14201

Closed

nkoroste mentioned this pull request Jan 24, 2022

Make d8 dex builder compatible with persistent worker mode #10241

Closed

Bencodes force-pushed the Support-multiplex–workers-in-CompatDexBuilder branch from f5def1f to 8b42308 Compare January 25, 2022 00:28

Bencodes changed the title ~~Support multiplex workers in CompatDexBuilder~~ Support workers in CompatDexBuilder Jan 25, 2022

Bencodes force-pushed the Support-multiplex–workers-in-CompatDexBuilder branch 2 times, most recently from 9cb6a71 to dbc6116 Compare January 31, 2022 23:51

gregestren added the team-Performance Issues for Performance teams label Feb 2, 2022

gregestren assigned larsrc-google Feb 2, 2022

Bencodes force-pushed the Support-multiplex–workers-in-CompatDexBuilder branch 2 times, most recently from 4080687 to 89f8dc6 Compare February 9, 2022 05:28

Bencodes force-pushed the Support-multiplex–workers-in-CompatDexBuilder branch 4 times, most recently from fc45e04 to ebde27c Compare February 15, 2022 04:57

larsrc-google approved these changes Mar 7, 2022

View reviewed changes

Support multiplex workers in CompatDexBuilder

ba874ea

Bencodes force-pushed the Support-multiplex–workers-in-CompatDexBuilder branch from ebde27c to ba874ea Compare March 7, 2022 20:11

Bencodes force-pushed the Support-multiplex–workers-in-CompatDexBuilder branch from ba874ea to e4c794e Compare April 5, 2022 19:36

Bencodes requested a review from ted-xie as a code owner April 5, 2022 19:36

Bencodes force-pushed the Support-multiplex–workers-in-CompatDexBuilder branch from e4c794e to ba874ea Compare April 5, 2022 20:08

ahumesky added the team-Android Issues for Android team label Apr 15, 2022

bazel-io pushed a commit that referenced this pull request Apr 19, 2022

Advance android_tools_pkg version to 0.24.0

dac25e7

Prerequisite to merge #14623 RELNOTES: Advance android_tools_pkg version to 0.24.0. PiperOrigin-RevId: 442930486

bazel-io closed this in 7ce1c57 Apr 22, 2022

Bencodes mentioned this pull request Apr 22, 2022

Add multiplex worker support to dexbuilder #15321

Closed

Support workers in CompatDexBuilder #14623

Support workers in CompatDexBuilder #14623

Uh oh!

Conversation

Bencodes commented Jan 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

larsrc-google commented Jan 24, 2022

Uh oh!

Bencodes commented Jan 24, 2022

Uh oh!

Bencodes commented Jan 25, 2022

Uh oh!

larsrc-google commented Jan 31, 2022

Uh oh!

Bencodes commented Jan 31, 2022

Uh oh!

Bencodes commented Jan 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bencodes commented Feb 11, 2022

Uh oh!

Bencodes commented Feb 24, 2022

Uh oh!

larsrc-google Feb 7, 2022

Choose a reason for hiding this comment

Uh oh!

larsrc-google Feb 7, 2022

Choose a reason for hiding this comment

Uh oh!

Bencodes Mar 16, 2022

Choose a reason for hiding this comment

Uh oh!

larsrc-google Mar 7, 2022

Choose a reason for hiding this comment

Uh oh!

larsrc-google Mar 7, 2022

Choose a reason for hiding this comment

Uh oh!

ted-xie commented Apr 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Bencodes commented Jan 22, 2022 •

edited

Loading

Bencodes commented Jan 31, 2022 •

edited

Loading

ted-xie commented Apr 14, 2022 •

edited

Loading