-
-
Notifications
You must be signed in to change notification settings - Fork 376
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
Utilize stackalloc and access static byte data directly #767
Conversation
iamcarbon
commented
Feb 13, 2022
- Eliminates various allocations utilizing stackalloc
- Utilizes special syntax to access static byte data directly (see Refer directly to static data when ReadOnlySpan wraps arrays of bytes. dotnet/roslyn#24621)
@jstedfast ready for review. |
char[] chars = new char[count]; | ||
Span<char> chars = count < 16 | ||
? stackalloc char[16] | ||
: new char[count]; | ||
|
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.
Does using Span<> here really help?
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.
Okay, so if my quick research is correct (don't have time for a full reading because I'm about to run out), I guess using Span<> with stackalloc removes the need to declare the method unsafe
?
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 is always a WIN when in throughput on .NET 5+ [we eliminate an allocation and avoid zeroing the stack], but there's a small penalty on older runtimes where we don't have SkipLocalsInit.
You are also right in that stackallocing into a Span is safe (otherwise, we deal with an unsafe pointer when using its natural type)
var chars = new char[fieldNameLength]; | ||
Span<char> chars = fieldNameLength <= 32 | ||
? stackalloc char[32] | ||
: new char[fieldNameLength]; |
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.
Same question about Span<>, but also why 32 here but 16 elsewhere?
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 did some random checking to see the averages sizes and found them to be small or large (12 bytes or 100+ bytes). Standardizing on 32 bytes seems reasonable however.
@@ -312,9 +312,14 @@ protected Header (ParserOptions options, HeaderId id, string name, byte[] field, | |||
/// <param name="fieldNameLength">The length of the field name (not including trailing whitespace).</param> | |||
/// <param name="value">The raw value of the header.</param> | |||
/// <param name="invalid"><c>true</c> if the header field is invalid; othereise, <c>false</c>.</param> | |||
#if NET5_0_OR_GREATER | |||
[System.Runtime.CompilerServices.SkipLocalsInit] | |||
#endif |
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.
Not sure what this does...
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.
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.
Yep. This is what gives us the win. It's basically free to use the stack if we use uncleared memory -- while avoiding the allocation.
@@ -37,7 +37,7 @@ namespace MimeKit.Encodings { | |||
/// </remarks> | |||
public class Base64Decoder : IMimeDecoder | |||
{ | |||
static readonly byte[] base64_rank = new byte[256] { | |||
static ReadOnlySpan<byte> base64_rank => new byte[256] { | |||
255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255, |
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.
should this be static readonly ReadOnlySpan<byte> ...
? and why =>
vs =
?
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 is a really weird syntax that requires you to understand how Roslyn transforms it. It looks like it's allocating on every use, but Roslyn burns the data directly into the data file instead. This allows the code to access the data directly and eliminate various indirections and bounds checks.
Thanks for the link to https://vcsjones.dev/csharp-readonly-span-bytes-static/ in one of your commits. Very useful documentation. |
MimeKit/Utils/MimeUtils.cs
Outdated
@@ -508,6 +508,8 @@ public static string Quote (string text) | |||
return Quote (text.AsSpan ()); | |||
} | |||
|
|||
private static readonly char[] unquoteChars = new[] { '\r', '\n', '\t', '\\', '"' }; | |||
|
|||
/// <summary> |
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 prefer to define my "constants" at the top and using PascalCase.
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.
updated.
Pre-merge: BenchmarkDotNet=v0.13.1, OS=Windows 10.0.22000
Post-merge:
|
Just for fun: BenchmarkDotNet=v0.13.1, OS=Windows 10.0.22000
|
Ahh, .NET6.0! ❤️ |
If you really want to be blown away: BenchmarkDotNet=v0.13.1, OS=Windows 10.0.22000
BenchmarkDotNet=v0.13.1, OS=Windows 10.0.22000
BenchmarkDotNet=v0.13.1, OS=Windows 10.0.22000
|
Those are lovely performance improvements. Excited to see these improve even further on .NET7.0! |