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

adding option of using buffered image reader for faster dmverity hashing #2013

Merged

Conversation

SethHollandsworth
Copy link
Contributor

related PR: #1887

We previously set the default to an unbuffered reader for the sake of memory efficiency. This PR adds the option back in to use a buffered reader for users with ample available memory. This allows for faster dmverity hashing if needed.

Testing in a VM with a 1.5GB image, an unbuffered reader uses 1/4 the RAM but takes 3x longer for dmverity hashing (from 20s up to 1m). With a swing that large either way, it would be nice to give users the option to choose which they want for use in the confcom plugin. Thanks!

@SethHollandsworth SethHollandsworth requested a review from a team as a code owner January 30, 2024 21:08
verboseFlag = "verbose"
outputDirFlag = "out-dir"
dockerFlag = "docker"
bufferedOpenerFlag = "buffered-opener"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: buffered-reader?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the sdk we're using calls it opener, I think it would be simpler to keep it as that

Copy link
Contributor

Choose a reason for hiding this comment

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

If these are external facing, "reader" makes more sense than "opener".

Signed-off-by: Seth Hollandsworth <sethho@microsoft.com>
@anmaxvl anmaxvl merged commit 5921abb into microsoft:main Feb 8, 2024
18 of 19 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.

3 participants