Support multiplex workers in ResourceProcessorBusyBox #14428

Bencodes · 2021-12-14T23:04:40Z

Adding support for multiplex workers inside of ResourceProcessorBusyBox and moving it's worker implementation over to the generic work request handler.

These PRs need to land for this to work:

For those not on rolling releases, the other required PRs that have already merged are:

meisterT · 2021-12-20T07:35:57Z

cc @larsrc-google

larsrc-google · 2022-01-10T13:19:27Z

src/main/java/com/google/devtools/build/lib/rules/android/BusyBoxActionBuilder.java

      executionInfo.putAll(ExecutionRequirements.WORKER_MODE_ENABLED);
+
+      if (dataContext.isPersistentMultiplexBusyboxTools()) {
+        executionInfo.putAll(ExecutionRequirements.WORKER_MULTIPLEX_MODE_ENABLED);


So this flag would override --noexperimental_worker_multiplex?

Passing --noexperimental_worker_multiplex would disable multiplex workers entirely while isPersistentMultiplexBusyboxTools just instructs BusyBoxActionBuilder to register it's actions with with supports-multiplex-worker specified as part of it's execution info.

D'uh. You're right.

larsrc-google · 2022-01-12T09:10:12Z

src/tools/android/java/com/google/devtools/build/android/ResourceProcessorBusyBox.java

+                            String s = buf.toString().trim();
+                            buf.reset();
+                            if (!s.isEmpty()) {
+                              pw.print(s);


This sends everything sent to stdout/err into the output field of the response. However, since System.out/err are global, output from all threads gets interleaved here. These should be sent to realStdErr, any actual error messages from processing this request should go to pw. See discussion in #14201

Merging #14201 first would be the preferred solution here so that we aren't having to duplicate any of the system stream remapping logic between workers.

Pushed the changes to swap System.err in for System.out.

Also worth noting that swapping System.err in for System.out technically changes where the outputs are delivered for this rule, where the prior implementation would write the captured outputs directly to the work response output vs. the new implementation dumps them to System.err and only writes the contents of the PrintWriter provided by WorkRequestHandler to the work response output.

Then you need to be sure that that's the desired behaviour. Things the user should see should go into the work response output, things that should go into the log should go to System.out.

I'm going to roll back to the previous behavior to preserve the existing logs/error delivery that exists today.

Bencodes · 2022-01-24T19:45:45Z

src/tools/android/java/com/google/devtools/build/android/ResourceProcessorBusyBox.java

+      String captured = buf.toString().trim();
+      buf.reset();
+      if (!captured.isEmpty()) {
+        pw.print(captured);


This preserves the previous logging behavior seen here https://github.com/bazelbuild/bazel/pull/14428/files#diff-6bbb0aaf4cf2d425734021815617e2b6203223abe7dc76ca8030a90e9f043873L204

src/tools/android/java/com/google/devtools/build/android/ResourceProcessorBusyBox.java

larsrc-google · 2022-01-31T11:33:50Z

src/tools/android/java/com/google/devtools/build/android/ResourceProcessorBusyBox.java

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


You have a race condition here: buf is shared among all threads, so if something gets written between this line and the next, it gets lost. Synchronizing on buf should be OK, though.

Updated but synchronizing on a static object to avoid having to synchronize on function arguments.

larsrc-google · 2022-02-14T09:04:36Z

src/tools/android/java/com/google/devtools/build/android/ResourceProcessorBusyBox.java

+      synchronized (LOCK) {
+        String captured = buf.toString().trim();
+        buf.reset();
+        if (!captured.isEmpty()) {


The if is not really necessary. Printing nothing should not be an issue.

larsrc-google · 2022-02-14T09:04:38Z

src/tools/android/java/com/google/devtools/build/android/ResourceProcessorBusyBox.java

+    } finally {
+      // Write the captured buffer to the work response. We synchronize to avoid race conditions
+      // while reading from and calling reset on the shared ByteArrayOutputStream.
+      synchronized (LOCK) {


You don't need an extra lock object. Locking on buf would be sufficient and clearer.

nkoroste · 2022-05-03T19:15:32Z

Any updates on this?

ted-xie · 2022-06-22T18:25:04Z

Hi @Bencodes @nkoroste!

We've merged this PR upstream; thanks so much for your contribution! In order for this feature to actually work, we'll have to update some remote repository pointers. I'll tag this PR in the description for that change, and you'll be notified when it lands.

Bencodes · 2022-06-22T18:26:53Z

Awesome, thanks for taking this one across the finish line @ted-xie!

Adding support for multiplex workers inside of `ResourceProcessorBusyBox` and moving it's worker implementation over to the generic work request handler. These PRs need to land for this to work: - bazelbuild#14424 - bazelbuild#14425 - bazelbuild#14427 For those not on rolling releases, the other required PRs that have already merged are: - bazelbuild#14144 - bazelbuild#14145 - bazelbuild#14146 Closes bazelbuild#14428. PiperOrigin-RevId: 456561596 Change-Id: I098d5a323ac6558ad0f5f8190e29f45a7a37b4d4

Final step for PR #14428. RELNOTES: None PiperOrigin-RevId: 457734764 Change-Id: I2b14bf31c665a6b9803e21492c5d32502e25f0ea

Should have been included in #14428 but was not caught before submission. RELNOTES: Test for experimental multiplexed persistent resource processor. PiperOrigin-RevId: 457743677 Change-Id: Ifd746ec716d45aa3065f4b15ad715d52155008c6

Bencodes requested review from ahumesky and lberki as code owners December 14, 2021 23:04

lberki removed their request for review December 15, 2021 07:27

larsrc-google reviewed Jan 10, 2022

View reviewed changes

larsrc-google reviewed Jan 12, 2022

View reviewed changes

Bencodes force-pushed the support-multiplex-workers-in-resourceprocessorbusybox branch from cb97b32 to 1b64c9c Compare January 12, 2022 18:46

Bencodes force-pushed the support-multiplex-workers-in-resourceprocessorbusybox branch from d4052d1 to a463a50 Compare January 24, 2022 17:39

Bencodes commented Jan 24, 2022

View reviewed changes

src/tools/android/java/com/google/devtools/build/android/ResourceProcessorBusyBox.java Show resolved Hide resolved

larsrc-google reviewed Jan 31, 2022

View reviewed changes

Bencodes force-pushed the support-multiplex-workers-in-resourceprocessorbusybox branch from 127449d to 23f76f1 Compare January 31, 2022 18:07

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

gregestren assigned larsrc-google Feb 3, 2022

Bencodes force-pushed the support-multiplex-workers-in-resourceprocessorbusybox branch from 7ff4278 to 32557fd Compare February 6, 2022 17:33

Bencodes mentioned this pull request Feb 9, 2022

Make ManifestMergerAction worker compatible #14427

Closed

Bencodes force-pushed the support-multiplex-workers-in-resourceprocessorbusybox branch from 32557fd to db2cc0f Compare February 9, 2022 05:31

larsrc-google approved these changes Feb 14, 2022

View reviewed changes

Bencodes force-pushed the support-multiplex-workers-in-resourceprocessorbusybox branch from db2cc0f to 7ced7c5 Compare February 15, 2022 04:48

Support multiplex workers in ResourceProcessorBusyBox

6f08b15

Bencodes force-pushed the support-multiplex-workers-in-resourceprocessorbusybox branch from 7ced7c5 to 6f08b15 Compare February 15, 2022 04:58

arunkumar9t2 mentioned this pull request Mar 13, 2022

Support Multiplexed workers for Android Resource processing bazelbuild/rules_android#30

Open

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

Bencodes requested a review from ted-xie as a code owner April 22, 2022 11:45

ckolli5 added the awaiting-review PR is awaiting review from an assigned reviewer label Apr 26, 2022

copybara-service bot closed this in 4cfd32d Jun 22, 2022

Bencodes deleted the support-multiplex-workers-in-resourceprocessorbusybox branch June 22, 2022 18:26

ted-xie removed the awaiting-review PR is awaiting review from an assigned reviewer label Jun 22, 2022

copybara-service bot pushed a commit that referenced this pull request Jun 28, 2022

Update remote android_tools to 0.26.0.

6367a51

Final step for PR #14428. RELNOTES: None PiperOrigin-RevId: 457734764 Change-Id: I2b14bf31c665a6b9803e21492c5d32502e25f0ea

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support multiplex workers in ResourceProcessorBusyBox #14428

Support multiplex workers in ResourceProcessorBusyBox #14428

Bencodes commented Dec 14, 2021 •

edited

Loading

meisterT commented Dec 20, 2021

larsrc-google Jan 10, 2022

Bencodes Jan 10, 2022

larsrc-google Jan 12, 2022

larsrc-google Jan 12, 2022

Bencodes Jan 12, 2022

Bencodes Jan 12, 2022

Bencodes Jan 12, 2022

larsrc-google Jan 24, 2022

Bencodes Jan 24, 2022

Bencodes Jan 24, 2022

larsrc-google Jan 31, 2022

Bencodes Feb 9, 2022

larsrc-google Feb 14, 2022

larsrc-google Feb 14, 2022

Bencodes Feb 15, 2022

nkoroste commented May 3, 2022

ted-xie commented Jun 22, 2022

Bencodes commented Jun 22, 2022

Support multiplex workers in ResourceProcessorBusyBox #14428

Support multiplex workers in ResourceProcessorBusyBox #14428

Conversation

Bencodes commented Dec 14, 2021 • edited Loading

meisterT commented Dec 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkoroste commented May 3, 2022

ted-xie commented Jun 22, 2022

Bencodes commented Jun 22, 2022

Bencodes commented Dec 14, 2021 •

edited

Loading