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

Add multiplex worker support to dexbuilder #15321

Conversation

Bencodes
Copy link
Contributor

Adding full support for multiplexed dexbuilder workers. This PR is a continuation of #14623 (comment) where we added basic worker support.

Related issue: #10241

@Bencodes Bencodes force-pushed the support-use_multiplex_workers_with_dexbuilder branch from ac96cd3 to 705bc91 Compare April 22, 2022 19:49
@sgowroji sgowroji added team-Android Issues for Android team awaiting-review PR is awaiting review from an assigned reviewer labels Apr 25, 2022
@lberki lberki removed their request for review April 25, 2022 06:43
@nkoroste
Copy link
Contributor

nkoroste commented May 3, 2022

Any updates on this?

@ted-xie ted-xie added awaiting-user-response Awaiting a response from the author awaiting-review PR is awaiting review from an assigned reviewer and removed awaiting-review PR is awaiting review from an assigned reviewer awaiting-user-response Awaiting a response from the author labels Jun 17, 2022
@ted-xie
Copy link
Contributor

ted-xie commented Jun 17, 2022

Oops, sorry for messing with the labels - I got some of my browser tabs mixed up. From a glance I don't see any major red flags; I'll discuss this internally.

Copy link
Contributor

@ted-xie ted-xie left a comment

Choose a reason for hiding this comment

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

Overall looks good - would you mind adding a test for this flag in src/test/shell/bazel/android/android_integration_test.sh?

Copy link
Contributor

@ted-xie ted-xie left a comment

Choose a reason for hiding this comment

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

Actually, disregard my previous comment. I'll take care of the integration test. Overall LGTM, I'll push this through the internal processes.

@ahumesky
Copy link
Contributor

Rather than adding a separate flag, there's --modify_execution_info:

https://bazel.build/reference/command-line-reference#flag--modify_execution_info

So I think we could do the same thing with:

--modify_execution_info=DexBuilder=+supports-multiplex-workers

I believe that --modify_execution_info was added after --use_workers_with_dexbuilder was added, so that's why we have that hardcoded flag. We could actually remove --use_workers_with_dexbuilder as well and replace it with --modify_execution_info.

--modify_execution_info is admittedly a little clunkier than a specially purposed flag, but on the other hand, it might be better than proliferating several more flags.

@larsrc-google
Copy link
Contributor

Very much agree that we shouldn't add another tool-specific flag.

I'm starting to regret the --experimental_worker_multiplex flag being a boolean instead of a include/exclude list.

@Bencodes
Copy link
Contributor Author

Bencodes commented Jul 22, 2022

What about instead of flag proliferation we introduce an expansion CLI flag that sets all of the necessary --modify_execution_info requirements for Android multiplex workers?

@Bencodes
Copy link
Contributor Author

The CLI expansion solution would look something like this. #15950

@ahumesky
Copy link
Contributor

Thanks everyone, let's go with #15950

@nkoroste
Copy link
Contributor

should we close this in favor of #15950 to avoid confusion

@ahumesky
Copy link
Contributor

Yes that sounds good

@ahumesky ahumesky closed this Oct 18, 2022
copybara-service bot pushed a commit that referenced this pull request Oct 18, 2022
Alternative approach to #15321 (comment) that leverages an expansion CLI flag instead of proliferating unique experimental multiplex flags.

Closes #15950.

PiperOrigin-RevId: 482004859
Change-Id: I176288fbc7934dc74b92e144aa0c43f30cb5471d
@Bencodes Bencodes deleted the support-use_multiplex_workers_with_dexbuilder branch October 18, 2022 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review PR is awaiting review from an assigned reviewer team-Android Issues for Android team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants