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

Perf: Small performance improvement for CalculateChecksumForPath #11214

Merged
merged 1 commit into from
Mar 15, 2023

Conversation

vcsjones
Copy link
Contributor

@vcsjones vcsjones commented Feb 8, 2023

This is a small performance improvement for CalculateChecksumForPath:

  1. MD5 hashes are stack-buffer size friendly, so switch to the buffer-writing MD5.HashData and write to the stack.
  2. Replace BitConverter.ToString(..).Replace(..) with Convert.ToHexString. This eliminates a 32 character string allocation.

Note that this method could be further optimized to make it near allocation-less at the cost of readability, adding complexity. and using unsafe code. But it isn't clear to me how "hot" this code path is. Right now the allocations scale with the length of the path. However, these changes are small and don't harm the readability of the method.

Benchmarks:

Method PathLength Mean Error StdDev Gen0 Allocated
After 260 815.4 ns 5.43 ns 5.07 ns 0.0591 376 B
Before 260 1,054.6 ns 2.77 ns 2.59 ns 0.0839 536 B

Validation
How did you test these changes?

  • Built and ran the app
  • Tested the changes for accessibility

Screenshots (optional)
Not really applicable.

@yaira2
Copy link
Member

yaira2 commented Feb 8, 2023

For what it's worth, I don't think we're actually using this right now but it's a good reminder for us to implement #3722

@0x5bfa
Copy link
Member

0x5bfa commented Mar 15, 2023

@yaira2 To use stackalloc will be helpful if they are going to calculate considerably long binary data like file content. In this case, they calculate path.

Copy link
Member

@hishitetsu hishitetsu left a comment

Choose a reason for hiding this comment

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

LGTM, but actually, this code does not appear to be used at all.

it's a good reminder for us to implement #3722

Yes. In fact, thanks to @vcsjones, we were able to implement #3722! I appreciate it very much.
dotnet/runtime#63757

@yaira2 yaira2 merged commit 3271055 into files-community:main Mar 15, 2023
@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels Mar 15, 2023
@yaira2
Copy link
Member

yaira2 commented Mar 15, 2023

@vcsjones thank you!

@vcsjones vcsjones deleted the opt-checksum branch March 15, 2023 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants