SslStream should avoid copying read buffer data when it already has another frame to process

[geoffkizer]

The logic in SslStream.ReadAsyncInternal calls ResetReadBuffer whenever it finishes processing the previous decrypted frame and thus needs to decrypt another frame. However, this is unnecessarily aggressive. We should only do this when we are about to actually issue a read against the underlying stream. If we already have a frame buffered, then we won't read and will just process that frame immediately, which means the copy was pointless.

This might be a good reason to use ArrayBuffer here, since it simplifies some of the buffer management.

Related to #49000
Related to #49019

[ghost]

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

Issue Details

---

The logic in SslStream.ReadAsyncInternal calls ResetReadBuffer whenever it finishes processing the previous decrypted frame and thus needs to decrypt another frame. However, this is unnecessarily aggressive. We should only do this when we are about to actually issue a read against the underlying stream. If we already have a frame buffered, then we won't read and will just process that frame immediately, which means the copy was pointless.

This might be a good reason to use ArrayBuffer here, since it simplifies some of the buffer management.

Related to #49000
Related to #49019

Author:	geoffkizer
Assignees:	-
Labels:	area-System.Net.Security, tenet-performance, untriaged
Milestone:	-

[ManickaP]

Triage: unless we have concrete complaints about this hitting customers, we might leave it to future. If this proves to be easy we might do it in 6.0.

[wfurt]

the copy is probably there to make sure there is enough room for the data. There may be enough space even with the read as the buffer may be bigger and next frame may have less then maximum size. In the other hand I'm not sure how often this would happen. I'm wondering if it would make sense add some counters so we have better visibility to some of the operations.

[geoffkizer]

@wfurt if you are optimizing this to decrypt multiple frames in a single read operation (when available), then I think this would just fall out of the work you are already doing.

That is, we should only copy the read buffer when we run out of frames to process and need to do a read on the underlying stream.

[geoffkizer]

@wfurt I think you fixed this with your recent PR right?

[wfurt]

fixed with #50815.