Skip to content

DigestUtils: avoid throwing on invalid digest function name#20574

Closed
christianscott wants to merge 2 commits intobazelbuild:masterfrom
christianscott:christianscott-no_throw_on_invalid_digest_name
Closed

DigestUtils: avoid throwing on invalid digest function name#20574
christianscott wants to merge 2 commits intobazelbuild:masterfrom
christianscott:christianscott-no_throw_on_invalid_digest_name

Conversation

@christianscott
Copy link
Copy Markdown
Contributor

@christianscott christianscott commented Dec 17, 2023

9% of samples in a profile of one of our builds were inside the fillInStackTrace() method. Collecting the valid names into a hashset avoids needing to construct errors every time an invalid digest function name is passed into this function.

Tested with Bazel 6.4.0. Our codebase is not yet compatible with Bazel 7.

I have not investigated why this function was receiving so many invalid names.

Before:

Screenshot 2023-12-15 at 2 39 01 pm

After:

Screenshot 2023-12-15 at 2 43 10 pm

My understanding is that this will not speed up builds directly, but it will allow BEP events to be processed more quickly.

@christianscott christianscott requested a review from a team as a code owner December 17, 2023 04:38
@google-cla
Copy link
Copy Markdown

google-cla Bot commented Dec 17, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@github-actions github-actions Bot added team-Remote-Exec Issues and PRs for the Execution (Remote) team awaiting-review PR is awaiting review from an assigned reviewer labels Dec 17, 2023
tjgq
tjgq requested changes Dec 17, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@tjgq tjgq left a comment

Choose a reason for hiding this comment

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

Ouch, 11% is pretty bad! Great find.

Comment thread src/main/java/com/google/devtools/build/lib/remote/util/DigestUtil.java Outdated
Comment thread src/main/java/com/google/devtools/build/lib/remote/util/DigestUtil.java Outdated
@christianscott christianscott requested a review from tjgq December 17, 2023 22:05
christianscott
christianscott commented Dec 17, 2023
View reviewed changes
Comment thread src/main/java/com/google/devtools/build/lib/remote/util/DigestUtil.java
}

private static final ImmutableSet<String> DIGEST_FUNCTION_NAMES =
Arrays.stream(DigestFunction.Value.values()).map(v -> v.name()).collect(toImmutableSet());
Copy link
Copy Markdown
Contributor Author

@christianscott christianscott Dec 17, 2023

Choose a reason for hiding this comment

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

btw – I could not find a java style guide for the bazel source code, so I assume you use 4 space continuations based on other parts of the codebase

Copy link
Copy Markdown
Collaborator

@fmeum fmeum Dec 18, 2023

Choose a reason for hiding this comment

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

What helped me for contributions: The style guide is https://google.github.io/styleguide/javaguide.html and you can use https://github.com/google/google-java-format and its IDE plugins for formatting.

Copy link
Copy Markdown
Contributor Author

@christianscott christianscott Dec 18, 2023

Choose a reason for hiding this comment

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

Oh of course it's the google Java style guide. Thanks @fmeum!

@christianscott
Copy link
Copy Markdown
Contributor Author

christianscott commented Dec 18, 2023

@tjgq can I assume those CI failures are flakes?

@christianscott christianscott mentioned this pull request Dec 18, 2023
Open
@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 Dec 18, 2023
@coeuvre coeuvre added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-user-response Awaiting a response from the author labels Dec 18, 2023
@github-actions github-actions Bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Dec 19, 2023
@fmeum
Copy link
Copy Markdown
Collaborator

fmeum commented Dec 19, 2023

@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 Dec 19, 2023
@iancha1992
Copy link
Copy Markdown
Member

iancha1992 commented Dec 20, 2023

@bazel-io fork 7.1.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 Dec 20, 2023
@bazel-io bazel-io mentioned this pull request Dec 20, 2023
Closed
bazel-io pushed a commit to bazel-io/bazel that referenced this pull request Dec 21, 2023
@christianscott @bazel-io
DigestUtils: avoid throwing on invalid digest function name
af7b876 
9% of samples in a profile of one of our builds were inside the `fillInStackTrace()` method. Collecting the valid names into a hashset avoids needing to construct errors every time an invalid digest function name is passed into this function.

Tested with Bazel 6.4.0. Our codebase is not yet compatible with Bazel 7.

I have not investigated why this function was receiving so many invalid names.

Before:

![Screenshot 2023-12-15 at 2 39 01 pm](https://github.com/bazelbuild/bazel/assets/18002432/be4bd311-ca73-46ec-a06d-93bb0ca9c6ba)

After:

![Screenshot 2023-12-15 at 2 43 10 pm](https://github.com/bazelbuild/bazel/assets/18002432/64b15739-538f-4752-aafd-6b2c94886595)

My understanding is that this will not speed up builds directly, but it will allow BEP events to be processed more quickly.

Closes bazelbuild#20574.

PiperOrigin-RevId: 592094151
Change-Id: Ie23241c9ec40e59ba2aac1fc83e4830340260f45
@bazel-io bazel-io mentioned this pull request Dec 21, 2023
Merged
github-merge-queue Bot pushed a commit that referenced this pull request Jan 11, 2024
@bazel-io @christianscott
@iancha1992 @meteorcloudy
[7.1.0] DigestUtils: avoid throwing on invalid digest function name (#…
6d020ed 
…20650)

9% of samples in a profile of one of our builds were inside the
`fillInStackTrace()` method. Collecting the valid names into a hashset
avoids needing to construct errors every time an invalid digest function
name is passed into this function.

Tested with Bazel 6.4.0. Our codebase is not yet compatible with Bazel
7.

I have not investigated why this function was receiving so many invalid
names.

Before:

![Screenshot 2023-12-15 at 2 39 01
pm](https://github.com/bazelbuild/bazel/assets/18002432/be4bd311-ca73-46ec-a06d-93bb0ca9c6ba)

After:

![Screenshot 2023-12-15 at 2 43 10
pm](https://github.com/bazelbuild/bazel/assets/18002432/64b15739-538f-4752-aafd-6b2c94886595)

My understanding is that this will not speed up builds directly, but it
will allow BEP events to be processed more quickly.

Closes #20574.

Commit
cb08d62

PiperOrigin-RevId: 592094151
Change-Id: Ie23241c9ec40e59ba2aac1fc83e4830340260f45

Co-authored-by: Christian Scott <[email protected]>
Co-authored-by: Ian (Hee) Cha <[email protected]>
Co-authored-by: Yun Peng <[email protected]>
@iancha1992
Copy link
Copy Markdown
Member

iancha1992 commented Feb 23, 2024

The changes in this PR have been included in Bazel 7.1.0 RC1. Please test out the release candidate and report any issues as soon as possible. If you're using Bazelisk, you can point to the latest RC by setting USE_BAZEL_VERSION=last_rc.
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coeuvre coeuvre coeuvre approved these changes

@tjgq tjgq Awaiting requested review from tjgq

+1 more reviewer

@fmeum fmeum fmeum left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

team-Remote-Exec Issues and PRs for the Execution (Remote) team

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@christianscott @fmeum @iancha1992 @tjgq @coeuvre @bazel-io @sgowroji