Skip to content
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.

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

Support multiplex workers in ResourceProcessorBusyBox #14428

Conversation

Bencodes
Copy link
Contributor

@Bencodes Bencodes commented Dec 14, 2021

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:

@lberki lberki removed their request for review December 15, 2021 07:27
@meisterT
Copy link
Member

cc @larsrc-google

executionInfo.putAll(ExecutionRequirements.WORKER_MODE_ENABLED);

if (dataContext.isPersistentMultiplexBusyboxTools()) {
executionInfo.putAll(ExecutionRequirements.WORKER_MULTIPLEX_MODE_ENABLED);
Copy link
Contributor

Choose a reason for hiding this comment

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

So this flag would override --noexperimental_worker_multiplex?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

D'uh. You're right.

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@Bencodes Bencodes force-pushed the support-multiplex-workers-in-resourceprocessorbusybox branch from cb97b32 to 1b64c9c Compare January 12, 2022 18:46
@Bencodes Bencodes force-pushed the support-multiplex-workers-in-resourceprocessorbusybox branch from d4052d1 to a463a50 Compare January 24, 2022 17:39
String captured = buf.toString().trim();
buf.reset();
if (!captured.isEmpty()) {
pw.print(captured);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@Bencodes Bencodes force-pushed the support-multiplex-workers-in-resourceprocessorbusybox branch from 127449d to 23f76f1 Compare January 31, 2022 18:07
@gregestren gregestren added the team-Performance Issues for Performance teams label Feb 3, 2022
@Bencodes Bencodes force-pushed the support-multiplex-workers-in-resourceprocessorbusybox branch from 7ff4278 to 32557fd Compare February 6, 2022 17:33
@Bencodes Bencodes force-pushed the support-multiplex-workers-in-resourceprocessorbusybox branch from 32557fd to db2cc0f Compare February 9, 2022 05:31
synchronized (LOCK) {
String captured = buf.toString().trim();
buf.reset();
if (!captured.isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

} 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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@Bencodes Bencodes force-pushed the support-multiplex-workers-in-resourceprocessorbusybox branch from db2cc0f to 7ced7c5 Compare February 15, 2022 04:48
@Bencodes Bencodes force-pushed the support-multiplex-workers-in-resourceprocessorbusybox branch from 7ced7c5 to 6f08b15 Compare February 15, 2022 04:58
@ahumesky ahumesky added the team-Android Issues for Android team label Apr 15, 2022
@Bencodes Bencodes requested a review from ted-xie as a code owner April 22, 2022 11:45
@ckolli5 ckolli5 added the awaiting-review PR is awaiting review from an assigned reviewer label Apr 26, 2022
@nkoroste
Copy link
Contributor

nkoroste commented May 3, 2022

Any updates on this?

@ted-xie
Copy link
Contributor

ted-xie commented Jun 22, 2022

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 Bencodes deleted the support-multiplex-workers-in-resourceprocessorbusybox branch June 22, 2022 18:26
@Bencodes
Copy link
Contributor Author

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

@ted-xie ted-xie removed the awaiting-review PR is awaiting review from an assigned reviewer label Jun 22, 2022
aranguyen pushed a commit to aranguyen/bazel that referenced this pull request Jun 27, 2022
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
copybara-service bot pushed a commit that referenced this pull request Jun 28, 2022
Final step for PR #14428.

RELNOTES: None
PiperOrigin-RevId: 457734764
Change-Id: I2b14bf31c665a6b9803e21492c5d32502e25f0ea
copybara-service bot pushed a commit that referenced this pull request Jun 28, 2022
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Android Issues for Android team team-Performance Issues for Performance teams
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants