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

add ReceiveFromAsync and SendToAsync with SocketAddress overload #90086

Merged
merged 11 commits into from
Aug 8, 2023

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Aug 7, 2023

contributes to #30797
fixes #87397

This is the last big step. There is no longer Internals.SocketAddress.
That allows to use provided SocketAddress without any additional processing.

I realized that the receivedSocketAddress from #88970 did not sue the approved name -> this fixes it makes sync & async same.

There is probably more room for cleanup but this still should be much better - even for the legacy overloads.

also fixes #78448

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Aug 7, 2023

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

Issue Details

contributes to #30797
fixes #87397

This is the last big step. There is no longer Internals.SocketAddress.
That allows to use provided SocketAddress without any additional processing.

I realized that the receivedSocketAddress from #88970 did not sue the approved name -> this fixes it makes sync & async same.

There is probably more room for cleanup but this still should be much better - even for the legacy overloads.

Author: wfurt
Assignees: wfurt
Labels:

area-System.Net.Sockets

Milestone: 8.0.0

@stephentoub
Copy link
Member

contributes to #30797

What's more to do on that issue?

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.

Can you share perf numbers?

Span<byte> addressBuffer2 = stackalloc byte[IPAddressParserStatics.IPv6AddressBytes];
SocketAddressPal.GetIPv6Address(socketAddressBuffer, addressBuffer1, out uint scopeid);
endPoint.Address.TryWriteBytes(addressBuffer2, out _);
return endPoint.Address.ScopeId == (long)scopeid && addressBuffer1.SequenceEqual(addressBuffer2);
Copy link
Member

Choose a reason for hiding this comment

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

Can we compare the scope id before doing the TryWriteBytes above?

src/libraries/Common/src/System/Net/SocketAddress.cs Outdated Show resolved Hide resolved
{
ThrowIfDisposed();
ArgumentNullException.ThrowIfNull(receivedAddress, nameof(receivedAddress));
Copy link
Member

Choose a reason for hiding this comment

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

Please ensure we have tests for all argument validation across all the new methods.

@wfurt
Copy link
Member Author

wfurt commented Aug 7, 2023

contributes to #30797

What's more to do on that issue?

nothing for 8.0. Plan to write some summary there and either keep it if we want to expose it on EventArg for consistency, open separate issue for it or close it with comment and wait if there is need for it.

@wfurt
Copy link
Member Author

wfurt commented Aug 8, 2023

This should be ready for another review round @stephentoub
I also added tests and fixes for #78448 since that is the same area.
I'll get perf comparison today

Interop.Winsock.EnsureInitialized();

IntPtr INVALID_SOCKET = (IntPtr)(-1);
IntPtr socket = INVALID_SOCKET;
try
{
socket = Interop.Winsock.WSASocketW(af, DgramSocketType, 0, IntPtr.Zero, 0, (int)Interop.Winsock.SocketConstructorFlags.WSA_FLAG_NO_HANDLE_INHERIT);
Copy link
Member Author

Choose a reason for hiding this comment

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

This broke Socket.OSSupportsUnixDomainSockets as Windows only support Stream for UDS.

@wfurt
Copy link
Member Author

wfurt commented Aug 8, 2023

Method Job Toolchain Mean Error StdDev Median Min Max Ratio RatioSD Gen0 Allocated Alloc Ratio
ConnectAcceptAsync Job-ZDTGEN \PR\corerun.exe 761.8 us 315.49 us 363.32 us 513.1 us 471.6 us 1,547.8 us 1.49 0.75 - 1344 B 1.00
ConnectAcceptAsync Job-VRCVXQ \main\corerun.exe 502.9 us 15.49 us 15.90 us 509.7 us 481.3 us 522.3 us 1.00 0.00 - 1345 B 1.00
SendAsyncThenReceiveAsync_Task Job-ZDTGEN \PR\corerun.exe 104,494.1 us 2,054.93 us 2,018.22 us 104,477.0 us 100,835.1 us 108,956.7 us 1.00 0.04 - - NA
SendAsyncThenReceiveAsync_Task Job-VRCVXQ \main\corerun.exe 104,136.5 us 3,092.13 us 3,436.90 us 102,974.0 us 99,681.8 us 112,911.4 us 1.00 0.00 - - NA
ReceiveAsyncThenSendAsync_Task Job-ZDTGEN \PR\corerun.exe 685,473.6 us 13,392.59 us 14,329.93 us 682,778.8 us 660,879.8 us 712,324.7 us 0.96 0.15 - - NA
ReceiveAsyncThenSendAsync_Task Job-VRCVXQ \main\corerun.exe 728,471.7 us 78,202.80 us 90,058.46 us 713,186.3 us 510,999.9 us 885,721.3 us 1.00 0.00 - - NA
SendAsyncThenReceiveAsync_SocketAsyncEventArgs Job-ZDTGEN \PR\corerun.exe 102,279.1 us 1,540.97 us 1,366.03 us 102,311.2 us 100,069.7 us 104,280.6 us 0.96 0.05 - - NA
SendAsyncThenReceiveAsync_SocketAsyncEventArgs Job-VRCVXQ \main\corerun.exe 104,810.3 us 4,388.80 us 5,054.15 us 102,546.4 us 98,808.3 us 113,480.5 us 1.00 0.00 - - NA
ReceiveAsyncThenSendAsync_SocketAsyncEventArgs Job-ZDTGEN \PR\corerun.exe 674,034.8 us 44,428.74 us 47,538.26 us 684,631.1 us 486,587.7 us 698,983.0 us 0.94 0.08 - - NA
ReceiveAsyncThenSendAsync_SocketAsyncEventArgs Job-VRCVXQ \main\corerun.exe 719,407.7 us 29,227.37 us 33,658.29 us 702,072.3 us 683,461.1 us 782,602.0 us 1.00 0.00 - - NA
ReceiveFromAsyncThenSendToAsync_Task Job-ZDTGEN \PR\corerun.exe 1,587,406.7 us 73,418.09 us 84,548.38 us 1,585,315.7 us 1,407,020.1 us 1,751,406.0 us 0.99 0.08 - 4321064 B 0.75
ReceiveFromAsyncThenSendToAsync_Task Job-VRCVXQ \main\corerun.exe 1,610,837.5 us 96,790.05 us 103,564.27 us 1,616,955.2 us 1,373,973.8 us 1,811,356.2 us 1.00 0.00 - 5761064 B 1.00
SendToThenReceiveFrom Job-ZDTGEN \PR\corerun.exe 164,729.9 us 1,880.36 us 1,758.89 us 164,669.6 us 162,245.6 us 168,004.6 us 0.98 0.02 500.0000 4320440 B 0.75
SendToThenReceiveFrom Job-VRCVXQ \main\corerun.exe 168,548.9 us 3,004.79 us 2,810.68 us 167,155.2 us 164,936.9 us 173,277.2 us 1.00 0.00 - 5761408 B 1.00

shows ~ 25% allocation reduction in legacy SendTo/ReceiveFrom. We can still probably do better - tabling for 9.0

@wfurt wfurt merged commit 17d13fa into dotnet:main Aug 8, 2023
102 of 104 checks passed
@wfurt wfurt deleted the receiveFromAsync branch August 8, 2023 23:23
@ghost ghost locked as resolved and limited conversation to collaborators Sep 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
2 participants