-
Notifications
You must be signed in to change notification settings - Fork 5k
Optimize '[Guid]' parsing on Native AOT #116324
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
base: main
Are you sure you want to change the base?
Optimize '[Guid]' parsing on Native AOT #116324
Conversation
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.
Pull Request Overview
This PR aims to optimize the parsing of [Guid] in Native AOT by improving the underlying UTF8 string decoding and Guid parsing logic. Key changes include:
- Adding a new DecodeStringUtf8 method in NativeFormatReader.String.cs to efficiently decode UTF8 strings.
- Introducing a GetRawStringDataUtf8 helper in ConstantStringValue to leverage the new decoding method.
- Refactoring Guid parsing in NativeFormatRuntimeNamedTypeInfo.cs to use a static local method that processes UTF8 data for performance gains.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
src/coreclr/tools/Common/Internal/NativeFormat/NativeFormatReader.String.cs | Added method for decoding UTF8 strings with boundary checks and copying to a destination span |
src/coreclr/tools/Common/Internal/Metadata/NativeFormat/NativeFormatReaderGen.cs | Added a helper that uses the new decode method to extract UTF8 encoded string data |
src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/TypeInfos/NativeFormat/NativeFormatRuntimeNamedTypeInfo.cs | Replaced direct Guid creation with a static local method that decodes and parses UTF8 data for Guid construction |
Comments suppressed due to low confidence (1)
src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/TypeInfos/NativeFormat/NativeFormatRuntimeNamedTypeInfo.cs:88
- [nitpick] Consider using a parameter name in the exception call that more clearly reflects the null context of the handle value rather than 'utf8Text', which might be misleading.
ArgumentNullException.Throw("utf8Text");
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas |
...Lib/src/System/Reflection/Runtime/TypeInfos/NativeFormat/NativeFormatRuntimeNamedTypeInfo.cs
Outdated
Show resolved
Hide resolved
Any perf numbers? |
This comment was marked as outdated.
This comment was marked as outdated.
@@ -63,6 +64,32 @@ public unsafe uint DecodeString(uint offset, out string value) | |||
return endOffset; | |||
} | |||
|
|||
public unsafe Guid ParseGuid(uint offset) | |||
{ | |||
#if NETFX_45 |
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 dead ifdef. You can delete it
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's another one in the method above, should I remove that one too?
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.
Mmh if I remove it, building fails with this at the Guid.Parse
call:
D:\git\runtime\src\coreclr\tools\Common\Internal\NativeFormat\NativeFormatReader.String.cs(75,31): error CS1503: Argume
nt 1: cannot convert from 'System.ReadOnlySpan<byte>' to 'System.ReadOnlySpan<char>' [D:\git\runtime\src\coreclr\tools\
aot\ILCompiler.MetadataTransform\ILCompiler.MetadataTransform.csproj]
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.
Hmm, that's because we have not picked up #105654 in the LKG SDK yet.
It should be fixed by https://github.com/dotnet/runtime/pull/116328/files#diff-8df3cd354bc584349d04ad5675b33c042d8b99b741b8b95af394c55e0f5001bfR3
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.
It seems like it's fine now after doing the changes Michal suggested. I removed that ifdef 👍
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.
Could you please delete the ifdef (and the define in .csproj) while you are on in, even though it is not strictly required for the rest?
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.
Yup, of course, but just to clarify, I need to wait for that PR you linked to be merged first, and then to update this branch, right?
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 do not think you need to wait for that PR. The earlier build break was caused by the new code that you have introduced. The existing code should compile just fine with the ifdef deleted.
{ | ||
// We don't really have a parameter, so just match the name of the 'Guid.ctor' parameter | ||
#if SYSTEM_PRIVATE_CORELIB | ||
ArgumentNullException.Throw("input"); |
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.
Where was this exception thrown before?
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.
By the Guid
constructor I think. The code was getting a ConstantStringValue
value, which immediatley returns in the constructor if the input handle is null. That leaves the string field set to default, and that's then directly passed to the Guid
constructor. I added this check here to try to match the semantics in case of failures as close as possible.
src/coreclr/tools/Common/Internal/NativeFormat/NativeFormatReader.String.cs
Outdated
Show resolved
Hide resolved
src/coreclr/tools/Common/Internal/Metadata/NativeFormat/NativeFormatReaderGen.cs
Outdated
Show resolved
Hide resolved
...Lib/src/System/Reflection/Runtime/TypeInfos/NativeFormat/NativeFormatRuntimeNamedTypeInfo.cs
Outdated
Show resolved
Hide resolved
/azp run runtime-nativeaot-outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
Could you please measure the perf improvement for Type.GUID? |
Yup yup, will set some time aside to whip together a small benchmark. Was chatting with @EgorBo who said he might look into adding NAOT support to the bot, but I might just do a simple thing locally for now to unblock this sooner 🙂 |
We're going to need to use
[Guid]
in some scenarios in CsWinRT 3.0 (see #114179), and while looking at theType.GUID
logic in Native AOT I saw some potential small perf improvements we could do. Figured I'd give it a shot and see if it's well received 😄