Skip to content
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

avoid allocations for TLS handshake #87874

Merged
merged 13 commits into from Dec 6, 2023
Merged

avoid allocations for TLS handshake #87874

merged 13 commits into from Dec 6, 2023

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Jun 21, 2023

There are several places where we use Span.ToArray() to get data returned by native layer to something we can write to underlying stream. One day, we may write Memory<byte> wrapper but for now I replaced them with renting buffer from buffer pool and Span.CopyTo.

Since the PAL already deals with renting, I also updated Encrypt method to simplify it. It is already renting buffer but it will guess upfront and than we would pass it in via ref and then get actually length via extra variable. I changed that to simple out where all PAL flavors would rent just what is needed and and we would always return it up in generic SslStream code.

We already return SecurityStatusPal in most cases so we would now return ProtoclToken (that also includes SecurityStatusPal). The is needed as we cannot longer depend on size of the array as renting may give us more than asked for.
The structure is little bit bigger but I don't think it would would be significant. We could pass is via out/ref parameter if this is concern.

contributes to #68951

@wfurt wfurt added this to the 8.0.0 milestone Jun 21, 2023
@wfurt wfurt requested review from stephentoub and rzikm June 21, 2023 18:04
@wfurt wfurt self-assigned this Jun 21, 2023
@ghost
Copy link

ghost commented Jun 21, 2023

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

Issue Details

There are several places where we use Span.ToArray() to get data returned by native layer to something we can write to underlying stream. One day, we may write Memory<byte> wrapper but for now I replaced them with renting buffer from buffer pool and Span.CopyTo.

Since the PAL already deals with renting, I also updated Encrypt method to simplify it. It is already renting buffer but it will guess upfront and than we would pass it in via ref and then get actually length via extra variable. I changed that to simple out where all PAL flavors would rent just what is needed and and we would always return it up in generic SslStream code.

We already return SecurityStatusPal in most cases so we would now return ProtoclToken (that also includes SecurityStatusPal). The is needed as we cannot longer depend on size of the array as renting may give us more than asked for.
The structure is little bit bigger but I don't think it would would be significant. We could pass is via out/ref parameter if this is concern.

contributes to #68951

Author: wfurt
Assignees: wfurt
Labels:

area-System.Net.Security

Milestone: 8.0.0

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.

looks promising so far, I am looking forward for measurements.

@davidfowl
Copy link
Member

This change makes me so happy? This will nuke the remaining byte[] allocations yes?

@wfurt
Copy link
Member Author

wfurt commented Aug 11, 2023

Please take a look @stephentoub if you have chance. Not critical for 8.0 IMHO. I wish we have some longevity tests or way to verify that we do not leak the rented buffers. From top, this is only called in to places (Renegotiate & ForceHandshake) and fallback should be covered by the Finally clause.

@karelz
Copy link
Member

karelz commented Aug 29, 2023

@stephentoub will you get a chance to take look? (not 8.0 critical)

@karelz karelz modified the milestones: 8.0.0, 9.0.0 Sep 12, 2023
@wfurt wfurt marked this pull request as draft September 28, 2023 21:34
@wfurt wfurt marked this pull request as ready for review October 27, 2023 04:03
@wfurt
Copy link
Member Author

wfurt commented Oct 27, 2023

this is ready for another round @rzikm @stephentoub.

The change got bigger but hopefully easier to maintain and understand. Most of the operations are now part of the ProtocolToken. It is now passed as ref through PAL laters to places where new buffer is needed.
That also hopefully simplified the logic in Windows safe handle where may get data in two tries.

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

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

We should ensure we've got good stress test coverage of this. Thanks!

@wfurt wfurt merged commit 71f3bf1 into dotnet:main Dec 6, 2023
111 checks passed
@wfurt wfurt deleted the sslBuffers2 branch December 6, 2023 02:59
@github-actions github-actions bot locked and limited conversation to collaborators Jan 5, 2024
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.

None yet

6 participants