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

Double protect output stream of verify store #1180

Merged
merged 1 commit into from
Jul 19, 2024

Conversation

allada
Copy link
Member

@allada allada commented Jul 19, 2024

VerifyStore's job is to protect corrupted data from being received. It is possible for the down-stream component to try and accept data before receiving the EOF. To keep this from being a problem, we have VeifyStore do the checks when conditions are met before sending the last chunk of data.


This change is Reviewable

VerifyStore's job is to protect corrupted data from being received.
It is possible for the down-stream component to try and accept data
before receiving the EOF. To keep this from being a problem, we
have VeifyStore do the checks when conditions are met before sending
the last chunk of data.
Copy link
Member

@aaronmondal aaronmondal left a comment

Choose a reason for hiding this comment

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

:lgtm:

nit: Reconsider whether this is really something that should be enabled by default. With the S3 store it only affected a part of the system where we knew that this was an issue.

In contrast, since the VerifyStore is often used in conjunction with the MemoryStore it might be in a hotter path and the loop here might be called a lot more.

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Installation / macos-13, Remote / large-ubuntu-22.04, macos-13, and 1 discussions need to be resolved


nativelink-store/src/verify_store.rs line 100 at r1 (raw file):

            }

            if done {

nit: The branching with the done and chunk.is_empty() is a bit confusing and I'm not sure whether this loop is easy enough for the compiler to unroll. Instead, there might be a more "inline-y" way to map out these branches to increase the potential for the compiler to specialize the branches.

Copy link
Member Author

@allada allada left a comment

Choose a reason for hiding this comment

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

This should not be causing any performance regression. No additional buffering is taking place, this is just changing the logic to finish hasing before sending it instead of on next loop cycle.

Reviewable status: 1 of 1 LGTMs obtained, and 1 discussions need to be resolved

Copy link
Member Author

@allada allada left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 1 LGTMs obtained, and 1 discussions need to be resolved


nativelink-store/src/verify_store.rs line 100 at r1 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

nit: The branching with the done and chunk.is_empty() is a bit confusing and I'm not sure whether this loop is easy enough for the compiler to unroll. Instead, there might be a more "inline-y" way to map out these branches to increase the potential for the compiler to specialize the branches.

To be honest, I looked over this a few times and could not come up with an easier way to express it :-(

@allada allada merged commit e6542e6 into TraceMachina:main Jul 19, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants