Simplify sspicli interop marshaling #29031
Conversation
This makes it both faster and more AOT friendly
| @@ -38,27 +36,23 @@ public static unsafe byte[] ToByteArray(List<SslApplicationProtocol> application | |||
| } | |||
|
|
|||
| Sec_Application_Protocols protocols = new Sec_Application_Protocols(); | |||
| protocols.ProtocolListsSize = (uint)(ProtocolListConstSize + protocolListSize); | |||
| protocols.ProtocolExtenstionType = ApplicationProtocolNegotiationExt.ALPN; | |||
| protocols.ProtocolListsSize = (uint)((sizeof(Sec_Application_Protocols) - sizeof(uint)) + protocolListSize); | |||
There was a problem hiding this comment.
The previous code was semi-self-documenting as to what was being calculated. That's lost a bit here. Can you add a comment?
| @@ -55,11 +55,11 @@ internal static class WinHttpCertificateHelper | |||
| unsafe | |||
| { | |||
| var cppStruct = new Interop.Crypt32.CERT_CHAIN_POLICY_PARA(); | |||
| cppStruct.cbSize = (uint)Marshal.SizeOf<Interop.Crypt32.CERT_CHAIN_POLICY_PARA>(); | |||
| cppStruct.cbSize = (uint)sizeof(Interop.Crypt32.CERT_CHAIN_POLICY_PARA); | |||
There was a problem hiding this comment.
What are the rules regarding when it is safe to use sizeof() vs. Marshal.SizeOf<T>() ?
There was a problem hiding this comment.
Basically you can use sizeof unless there is marshaling logic involved that would change the size of the type, e.g. if a field was attributed with MarshalAs that would convert it from one thing to another of a different size during the marshaling process. If the type is blittable, sizeof is the way to go.
There was a problem hiding this comment.
It is safe to use sizeof for blittable types. Both sizeof() and Marshal.SizeOf<T>() return same value for blittable types, except that sizeof is faster.
We have some of the best practices for interop perf documented in: https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/interop-guidelines.md#definition
| protocols.ProtocolListSize = (short)protocolListSize; | ||
|
|
||
| Span<byte> pBuffer = new byte[protocolListSize]; | ||
| byte[] buffer = new byte[sizeof(Sec_Application_Protocols) + protocolListSize]; |
There was a problem hiding this comment.
This appears to be a larger value than it was before. Was it broken before?
There was a problem hiding this comment.
Oh, I see, this is replacing a second allocation made below. Nice.
There was a problem hiding this comment.
The original code allocated two buffers: First temporary buffer was protocolListSize, and second final buffer was sizeof(Sec_Application_Protocols) + protocolListSize. I got rid of the temporary buffer allocation while I was on it.
|
cc: @dotnet/ncl This is a good PR to learn about best-practice rules for optimum p/invokes. |
|
@dotnet-bot test Outerloop Windows x64 Debug Build please |
|
@dotnet-bot test Windows x86 Release Build please |
|
@dotnet-bot test Outerloop Windows x64 Debug Build please |
|
Outerloop failures all due to https://github.com/dotnet/corefx/issues/28631 |
* Delete unnecessary wrapper * Simplify sspicli interop marshaling This makes it both faster and more AOT friendly * CR feedback Commit migrated from dotnet/corefx@437e882
This makes it both faster and more AOT friendly
Contributes to dotnet/corert#5674