-
Notifications
You must be signed in to change notification settings - Fork 201
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
Fix SafeHandle benchmarks and add string benchmarks #1238
Fix SafeHandle benchmarks and add string benchmarks #1238
Conversation
…g from the MarshalAs rehydration.
…se of the marshalling system (since only Forwarder should ever use it)
// If the parameter has [MarshalAs] marshalling, we resurface that | ||
// in the forwarding target since the built-in system understands it. | ||
// ICustomMarshaller marshalling requires additional information that we throw away earlier since it's unsupported, | ||
// so explicitly do not resurface a [MarshalAs(UnmanagdType.CustomMarshaller)] attribute. |
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.
// so explicitly do not resurface a [MarshalAs(UnmanagdType.CustomMarshaller)] attribute. | |
// so explicitly do not resurface a [MarshalAs(UnmanagedType.CustomMarshaler)] attribute. |
// If the parameter has [MarshalAs] marshalling, we resurface that | ||
// in the forwarding target since the built-in system understands it. | ||
// ICustomMarshaller marshalling requires additional information that we throw away earlier since it's unsupported, | ||
// so explicitly do not resurface a [MarshalAs(UnmanagdType.CustomMarshaller)] attribute. |
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.
The repeated comment seams unnecessary, since it is right at the top of TryRehydrateMarshalAsAttribute
?
[GeneratedDllImport(NativeExportsNE_Binary, EntryPoint = EntryPoints.UShort.ReverseInplace)] | ||
public static partial void Reverse_In([MarshalAs(UnmanagedType.LPTStr)] in string s); | ||
|
||
[GeneratedDllImport(NativeExportsNE_Binary, EntryPoint = EntryPoints.UShort.ReverseInplace)] |
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.
[GeneratedDllImport(NativeExportsNE_Binary, EntryPoint = EntryPoints.UShort.ReverseInplace)] | |
[GeneratedDllImport(NativeExportsNE_Binary, EntryPoint = EntryPoints.UShort.ReverseReplace)] |
[GeneratedDllImport(NativeExportsNE_Binary, EntryPoint = EntryPoints.UShort.ReverseInplace)] | ||
public static partial void Reverse_In([MarshalAs(UnmanagedType.LPWStr)] in string s); | ||
|
||
[GeneratedDllImport(NativeExportsNE_Binary, EntryPoint = EntryPoints.UShort.ReverseInplace)] |
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.
[GeneratedDllImport(NativeExportsNE_Binary, EntryPoint = EntryPoints.UShort.ReverseInplace)] | |
[GeneratedDllImport(NativeExportsNE_Binary, EntryPoint = EntryPoints.UShort.ReverseReplace)] |
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.
For all the Reverse_Replace_Ref
ones below too.
public static partial int ReturnLength([MarshalAs(UnmanagedType.LPTStr)] string s); | ||
|
||
[GeneratedDllImport(NativeExportsNE_Binary, EntryPoint = EntryPoints.UShort.ReturnLength, CharSet = CharSet.None)] | ||
public static partial int ReturnLength_IgnoreCharSet([MarshalAs(UnmanagedType.LPTStr)] string s); |
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.
Remove all the ReturnLength_IgnoreCharSet
ones?
Fix native side of SafeHandle benchmarks to handle asynchronous release (finalization).
Add benchmarks for all of our string marshalling.
Add support in the Forwarder marshaller to reconstitute the original MarshalAs attributes for a given parameter. This still doesn't handle the return value case, so some of the string return benchmarks are inaccurate in the Built-In jobs (StringReturn_LPUTF8Str in particular).This PR also adds a specialized API to enable emitting the required attributes to get good benchmarking of this scenario.The only regression that isn't due to the above issue with return values and Forwarders is with ANSI marshalling. The source generated stubs do not support the stackalloc optimization for ANSI string marshalling since there is no public API that enables it. As a result, by-value scenarios (so the by-value and return benchmarks) will show some regressions for the
_LPStr
benchmarks.Other than that regression, we see a pretty nice improvement in throughput for string marshalling of a few percent in most every case, with many cases having a 10% improvement or more (likely due to better inlining and more JIT optimizations being possible).