-
Notifications
You must be signed in to change notification settings - Fork 226
Reduce allocations in ChecksumUtilities.BytesToString #12359
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
Reduce allocations in ChecksumUtilities.BytesToString #12359
Conversation
This method was previously allocating a string for each byte being converted and then allocating another string for the stringbuilder.ToString call. Instead: if >NET9 use Convert.ToHexStringLower else create stack alloc'ed char[] and put converted chars in there and allocate a single string from that
DustinCampbell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice find!
src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/ChecksumUtilities.cs
Outdated
Show resolved
Hide resolved
| return buffer.ToString(); | ||
|
|
||
| static char GetHexChar(int value) | ||
| => (char)(value < 10 ? '0' + value : 'a' + (value - 10)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this faster than indexing into a pre-allocated array of chars?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably slightly slower, but I didn't want to duplicate HexConverter.CharToHexLookup, especially as this won't be used OOP once we move to net9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't duplicate all of that either! I was just thinking of something like this:
private static char GetHexChar(int value) => s_hexChars[value];
private static readonly char[] s_hexChars = new[] { '0', '1', '2', '3', '4', '5', '6', '7, '8', '9', 'a', 'b', 'c', 'd', 'e', 'f' };There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, that's definitely into the micro-optimization level. I agree that it's not needed for just netstandard2.0. 👍
src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/ChecksumUtilities.cs
Outdated
Show resolved
Hide resolved
2) Avoid ImmutableCollectionsMarshal, and just pass the span to Convert.ToHexString
DustinCampbell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀🚀🚀🚀🚀🚀
This method was previously allocating a string for each byte being converted and then allocating another string for the stringbuilder.ToString call. Instead:
This method was accounting for 1.4% of allocations during the typing scenario in the razoreditingtests.ScrollingAndTypingInCohosting speedometer test. The final string allocation was only 0.3%, which is equivalent to what the new implementation should allocate.