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

SslStream should do a better job of allocating buffer for encryption #51478

Closed
geoffkizer opened this issue Apr 19, 2021 · 8 comments
Closed
Labels
area-System.Net.Security enhancement Product code improvement that does NOT require public API changes/additions tenet-performance Performance related issue
Milestone

Comments

@geoffkizer
Copy link
Contributor

See discussion here: #51060

Currently we do two things:
(1) Add a fixed FrameOverhead (now 64 bytes, previously 32) to the input size when allocating a buffer to encrypt into;
(2) Later, in SslStreamPal.EncryptMessage, we will detect a buffer that is too small and allocate a new, temporary buffer to use instead.

This results in two problems:
(1) We are often allocating more than we need to, since the actual header/trailer size is usually less than 64 bytes.
(2) If 64 bytes is not enough, we will silently fall back to a much less performant path, i.e. allocating lots of unpooled buffers. We saw this previously when FrameOverhead was 32 bytes, which is why we changed it to 64.

We know the exactly header and trailer size before we actually do the encryption. We should be able to allocate the exact size needed and remove the unperformant fallback in SslStreamPal.EncryptMessage. This is slightly complicated by renegotiation, but we should be able to just make this work.

@benaadams @wfurt

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Apr 19, 2021
@ghost
Copy link

ghost commented Apr 19, 2021

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

Issue Details

See discussion here: #51060

Currently we do two things:
(1) Add a fixed FrameOverhead (now 64 bytes, previously 32) to the input size when allocating a buffer to encrypt into;
(2) Later, in SslStreamPal.EncryptMessage, we will detect a buffer that is too small and allocate a new, temporary buffer to use instead.

This results in two problems:
(1) We are often allocating more than we need to, since the actual header/trailer size is usually less than 64 bytes.
(2) If 64 bytes is not enough, we will silently fall back to a much less performant path, i.e. allocating lots of unpooled buffers. We saw this previously when FrameOverhead was 32 bytes, which is why we changed it to 64.

We know the exactly header and trailer size before we actually do the encryption. We should be able to allocate the exact size needed and remove the unperformant fallback in SslStreamPal.EncryptMessage. This is slightly complicated by renegotiation, but we should be able to just make this work.

@benaadams @wfurt

Author: geoffkizer
Assignees: -
Labels:

area-System.Net.Security, untriaged

Milestone: -

@wfurt
Copy link
Member

wfurt commented Apr 20, 2021

While I see benefits of this, this will get us even more on path of understanding and tracking TLS internal.

@geoffkizer
Copy link
Contributor Author

Is that bad? Should we be doing the opposite instead?

@wfurt
Copy link
Member

wfurt commented Apr 20, 2021

I'm not sure if that is bad. We did make a step towards that when we start parsing some TLS frames. No we will need to possibly stay on top of new ciphers and protocol changes. Since there are not frequent that still seem acceptable.

There may be way how to query that after TLS handshakes is done and algorithms set.

If we can ever write non-contiguous chinks to Stream, this may become relevant as we would be able to write header, data and trailer in separate buffers.

@ManickaP ManickaP added enhancement Product code improvement that does NOT require public API changes/additions and removed untriaged New issue has not been triaged by the area owner labels Apr 22, 2021
@ManickaP ManickaP added this to the Future milestone Apr 22, 2021
@ManickaP
Copy link
Member

Triage: this is a follow up, the main problem is solved, this can be done in the future.

@wfurt wfurt modified the milestones: Future, 8.0.0 Nov 16, 2022
@wfurt
Copy link
Member

wfurt commented Nov 16, 2022

triage: we should look into it if time permits.

@karelz karelz modified the milestones: 8.0.0, Future Jun 9, 2023
@karelz karelz added the tenet-performance Performance related issue label Jun 9, 2023
@karelz karelz modified the milestones: Future, 9.0.0 Jul 18, 2023
@rzikm rzikm self-assigned this Nov 29, 2023
@ghost ghost added in-pr There is an active PR which will close this issue when it is merged and removed in-pr There is an active PR which will close this issue when it is merged labels Dec 4, 2023
@rzikm
Copy link
Member

rzikm commented Jan 16, 2024

Fixed by #87874

@rzikm rzikm closed this as completed Jan 16, 2024
@rzikm rzikm removed their assignment Jan 16, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Feb 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Security enhancement Product code improvement that does NOT require public API changes/additions tenet-performance Performance related issue
Projects
None yet
Development

No branches or pull requests

5 participants