-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[Safe code] Clean up Guid.TryFormatX #110923
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
@EgorBot -amd -arm -profiler using System.Runtime.CompilerServices;
using BenchmarkDotNet.Attributes;
public class Benchmark
{
static Guid guid = Guid.Parse("{D420C46B-04DC-4ABD-BF32-5132E82EE7F7}");
[Benchmark]
public bool FormatX()
{
Span<char> buffer = stackalloc char[100];
bool result = guid.TryFormat(buffer, out int charsWritten, "X");
Consume(buffer.Slice(0, charsWritten));
return result;
}
[MethodImpl(MethodImplOptions.NoInlining)]
static void Consume(Span<char> _){}
} |
Tagging subscribers to this area: @dotnet/area-system-runtime |
This comment was marked as resolved.
This comment was marked as resolved.
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.
New version is 10-15% faster.
I'm guessing that's from HexsToCharsHexOutput
not being inlined before?
Yep, but the goal was to remove the unsafe code & maintain same perf as before 🙂 |
success = TryParseExactX(input, ref parseResult); | ||
break; | ||
} | ||
bool success = (char)(format[0] | 0x20) switch |
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 the cast to char
actually necessary here? It should work without it and that would make smaller IL and avoid an extra zero extension that the JIT has to otherwise remove here
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.
@tannergooding hm. I've removed the cast but the codegen hasn't changed. But I guess it is still makes sense to remove it as useless
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'd guess the JIT is recognizing it as unnecessary and removing it already; just something small that caught my eye as likely unneeded
/ba-g "browser-wasm linux Release LibraryTests is stuck" |
Contributes to #94941
This PR removes unsafe code from
Guid.TryFormatX
+ small clean up inTryParseExact
.New version is 10-15% faster.