KEMBAR78
avoid allocationg ApplicationProtocol in common case by wfurt · Pull Request #69098 · dotnet/runtime · GitHub
Skip to content

Conversation

@wfurt
Copy link
Member

@wfurt wfurt commented May 10, 2022

contributes to #68951
For now, I updated Linux & Windows. Other platforms may be possible.

@wfurt wfurt requested a review from a team May 10, 2022 03:20
@wfurt wfurt self-assigned this May 10, 2022
@ghost
Copy link

ghost commented May 10, 2022

Tagging subscribers to this area: @dotnet/ncl, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

contributes to #68951
For now, I updated Linux & Windows. Other platforms may be possible.

Author: wfurt
Assignees: wfurt
Labels:

area-System.Net.Security

Milestone: -

alpnContext.ProtoNegoStatus == Interop.ApplicationProtocolNegotiationStatus.Success)
{
return alpnContext.Protocol;
if (alpnContext.ProtocolIdSize == s_http1.Length && alpnContext.Protocol.SequenceEqual(s_http1))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does the Windows one check ProtocolIdSize but the Linux one doesn't?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code originally looked different and I did not want to allocate the Span unless we need to. Perhaps that is cheap. Here I have the size available directly so I left the check in place. On Linux, The Interop function returns always Span so the code is somewhat different. I can remove the extra check if we want to.

Copy link
Member

@rzikm rzikm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, modulo existing comments

@wfurt wfurt merged commit ad6f5bc into dotnet:main May 16, 2022
@wfurt wfurt deleted the alpn branch May 16, 2022 20:43
@ghost ghost locked as resolved and limited conversation to collaborators Jun 16, 2022
@karelz karelz added this to the 7.0.0 milestone Jul 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants