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

Remote: Fix performance regression in "upload missing inputs". #15890

Closed
wants to merge 1 commit into from

Conversation

coeuvre
Copy link
Member

@coeuvre coeuvre commented Jul 15, 2022

The regression was introduced in 702df84 where we essentially create a subscriber for each digest to subscribe the result of findMissingBlobs.

This change update the code to not create so many subscribers but maintain the same functionalities.

Fixes #15872.

@coeuvre coeuvre requested a review from a team as a code owner July 15, 2022 17:05
@coeuvre
Copy link
Member Author

coeuvre commented Jul 15, 2022

I tested locally that the "collect digests" part is back to normal. @clint-stripe Can you please give it a try?

@brentleyjones
Copy link
Contributor

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Jul 15, 2022
@clint-stripe
Copy link

Awesome! This does indeed fix it for our 600k-input action as well; the trace in perfetto looks much better!

image

@coeuvre coeuvre force-pushed the test-upload-inputs-head branch 3 times, most recently from 6c2b485 to a2be64d Compare July 16, 2022 08:24
@sgowroji sgowroji added team-Remote-Exec Issues and PRs for the Execution (Remote) team awaiting-review PR is awaiting review from an assigned reviewer labels Jul 18, 2022
@ckolli5
Copy link

ckolli5 commented Jul 18, 2022

@bazel-io fork 5.3.0

@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Jul 18, 2022
@clint-stripe
Copy link

@ckolli5 is this something we could cherry-pick into 5.2? We're stuck on Bazel 5.0 without this fix, and waiting until 5.3 is released in August is not ideal.

@meteorcloudy
Copy link
Member

@clint-stripe Making a 5.2.1 release to cherry-picking this change won't be much faster than the 5.3 release, we still have to go through the same release process. So please wait for 5.3, thanks!

@brentleyjones
Copy link
Contributor

brentleyjones commented Jul 20, 2022

It might be helpful to get an RC out for 5.3 (or some bazelisk pointable binary). We have lots of changes in the branch, but no built binaries to test yet.

@coeuvre coeuvre force-pushed the test-upload-inputs-head branch 2 times, most recently from a32df05 to d5363a3 Compare July 22, 2022 12:06
@sgowroji sgowroji added awaiting-user-response Awaiting a response from the author and removed awaiting-review PR is awaiting review from an assigned reviewer labels Jul 26, 2022
@meteorcloudy
Copy link
Member

@brentleyjones Actually, you can use USE_BAZEL_VERSION to point any commit on the release branch, Bazelisk will work. We'll get an rc out soonish!

@sgowroji sgowroji added awaiting-review PR is awaiting review from an assigned reviewer and removed awaiting-user-response Awaiting a response from the author labels Jul 28, 2022
coeuvre added a commit to coeuvre/bazel that referenced this pull request Jul 28, 2022
The regression was introduced in 702df84 where we essentially create a subscriber for each digest to subscribe the result of `findMissingBlobs`.

This change update the code to not create so many subscribers but maintain the same functionalities.

Fixes bazelbuild#15872.

Closes bazelbuild#15890.

PiperOrigin-RevId: 463826260
Change-Id: Id0b1c7c309fc9653a47c5df95c609b34e6510cde
ShreeM01 pushed a commit that referenced this pull request Jul 28, 2022
The regression was introduced in 702df84 where we essentially create a subscriber for each digest to subscribe the result of `findMissingBlobs`.

This change update the code to not create so many subscribers but maintain the same functionalities.

Fixes #15872.

Closes #15890.

PiperOrigin-RevId: 463826260
Change-Id: Id0b1c7c309fc9653a47c5df95c609b34e6510cde
@coeuvre coeuvre mentioned this pull request Aug 3, 2022
copybara-service bot pushed a commit that referenced this pull request Aug 4, 2022
Some test cases added to `RemoteCacheTest` by #15890 are flaky. This change try to deflake them.

Closes #16040.

PiperOrigin-RevId: 465288877
Change-Id: Ia187069b15de0697cf0b2d8ee5c6a93ff980d473
@ShreeM01 ShreeM01 removed the awaiting-review PR is awaiting review from an assigned reviewer label Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Remote-Exec Issues and PRs for the Execution (Remote) team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remote execution build time regression with Bazel 5.1+ when action has many inputs
8 participants