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

Use full TLS record size for application data on Windows #95595

Conversation

rzikm
Copy link
Member

@rzikm rzikm commented Dec 4, 2023

Don't reduce MaximumMessage by Header and Trailer sizes (they are not included in the size limit).

Previous behavior led to us not consuming input in multiples of power of 2 which led to unnecessary fragmenting of data into TLS packets. The behavior was most noticeable when using 16kB buffer in calls to Write(Async) functions. Where we would split it into two TLS packets with the latter containing only about 40 B of useful data (see wireshark captures below).

This PR also removes some unsafe annotations on the code path.

WireShark captures (Windows)

before (notice the 135B TLS packets)
image

after
image

Benchmarks (Windows)

The LargeReadWriteAsync benchmarks use 64kB buffers, so this PR should reduce the number of encryption calls by 1/5

Method Job Toolchain Mean Error StdDev Median Min Max Ratio RatioSD Allocated Alloc Ratio
ReadWriteAsync Job-VRYUDS \pr\corerun.exe 34.00 us 0.366 us 0.343 us 33.91 us 33.44 us 34.71 us 0.99 0.02 - NA
ReadWriteAsync Job-GONFII \main\corerun.exe 34.33 us 0.395 us 0.350 us 34.26 us 33.74 us 35.01 us 1.00 0.00 - NA
LargeReadWriteAsync Job-VRYUDS \pr\corerun.exe 57.22 us 1.710 us 1.969 us 57.91 us 53.08 us 59.97 us 0.88 0.04 491 B 1.01
LargeReadWriteAsync Job-GONFII \main\corerun.exe 65.05 us 1.296 us 1.492 us 65.03 us 62.89 us 68.19 us 1.00 0.00 484 B 1.00
ConcurrentReadWrite Job-VRYUDS \pr\corerun.exe 1,381.65 us 51.610 us 59.434 us 1,389.42 us 1,266.48 us 1,471.83 us 1.02 0.07 - 0.00
ConcurrentReadWrite Job-GONFII \main\corerun.exe 1,367.73 us 75.121 us 83.496 us 1,379.64 us 1,165.86 us 1,469.80 us 1.00 0.00 1 B 1.00
ConcurrentReadWriteLargeBuffer Job-VRYUDS \pr\corerun.exe 1,021.83 us 46.291 us 53.309 us 1,032.54 us 895.20 us 1,094.38 us 0.99 0.11 - 0.00
ConcurrentReadWriteLargeBuffer Job-GONFII \main\corerun.exe 1,043.22 us 70.858 us 81.600 us 1,038.13 us 838.79 us 1,156.26 us 1.00 0.00 4 B 1.00

@ghost
Copy link

ghost commented Dec 4, 2023

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

Issue Details

Closes #51478.

Author: rzikm
Assignees: -
Labels:

area-System.Net.Security

Milestone: -

@rzikm
Copy link
Member Author

rzikm commented Dec 5, 2023

This should wait until #87874 is merged to prevent conflicts. Some parts may be no longer necessary after that change.

@rzikm rzikm force-pushed the 51478-SslStream-should-do-a-better-job-of-allocating-buffer-for-encryption branch from bdf91e7 to 600563f Compare December 6, 2023 13:18
@rzikm rzikm changed the title SslStream encryption buffer allocation improvements Use full TLS record size for application data on Windows Dec 6, 2023
@rzikm rzikm marked this pull request as ready for review December 6, 2023 15:15
@rzikm
Copy link
Member Author

rzikm commented Dec 6, 2023

/azp run runtime-libraries-coreclr outerloop

@rzikm rzikm requested a review from wfurt December 6, 2023 15:15
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM

@rzikm
Copy link
Member Author

rzikm commented Dec 7, 2023

Many work items seem to be failing on CI (especially in the outerloop pipeline), but I went through the failures reported for System.Net.Security.Tests and System.Net.Http.Functional.Tests and the console output shows no failures.

@dotnet/dnceng Could you please have a look?

@garath
Copy link
Member

garath commented Dec 7, 2023

Could you please have a look?

We currently have no known ongoing issues.

Please point me to a particular error?

@wfurt
Copy link
Member

wfurt commented Dec 7, 2023

I'm not sure what @rzikm had in ind. But for example https://helixre107v0xdcypoyl9e7f.blob.core.windows.net/dotnet-runtime-refs-pull-95595-merge-b3746c6d13a44017a0/Common.Tests/1/console.a0e72f3a.log?helixlogtype=result

it seems like the tests executed but it is reported as failure.

category=OuterLoop -notrait category=IgnoreForCI -notrait category=failing  
  Discovering: Common.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  Common.Tests (found 6 of 268 test cases)
  Starting:    Common.Tests (parallel test collections = on, max threads = 2)
  Finished:    Common.Tests
=== TEST EXECUTION SUMMARY ===
   Common.Tests  Total: 30, Errors: 0, Failed: 0, Skipped: 0, Time: 1.516s

@garath
Copy link
Member

garath commented Dec 7, 2023

Looks like you're also running into #95762. Perhaps connected and something is significantly breaking test reporting in these runs?

@garath garath mentioned this pull request Dec 7, 2023
3 tasks
@rzikm rzikm merged commit 6253efe into dotnet:main Dec 8, 2023
108 of 119 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jan 8, 2024
@karelz karelz added this to the 9.0.0 milestone May 14, 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.

4 participants