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

Rewrite Socket Task-based send/receive async operations on SocketAsyncEventArgs #16502

Merged
merged 3 commits into from Mar 3, 2017

Conversation

Projects
None yet
5 participants
@stephentoub
Member

stephentoub commented Feb 27, 2017

They're currently on top of the APM methods. This commit changes the ReceiveAsync/Send{To}Async operations to instead sit on top of a cached set of SocketAsyncEventArgs instances.

On a microbenchmark that connects two sockets and then does a million 1byte sends/receives from one to the other, top allocations before:
image
And top allocations after:
image

In a small change that forces all receives to be asynchronous by doing them before the send, top allocations before:
image
And top allocations after:
image
Throughput on the send/receive case also improved ~60%. Throughput on the receive/send case improved ~10%.

The worst case for this change is when only one send/receive is done per socket, since then it doesn't benefit from the SocketAsyncEventArgs caching. It takes two send/receives per socket to break even on allocation, and after that it's a definitive win.

With the task-based versions now generally much more efficient than the APM-based ones, I've switched NetworkStream.Read/WriteAsync to use them.

cc: @geoffkizer, @CIPop, @davidsh, @vancem

{
throw new ArgumentNullException(nameof(buffer.Array));
}
if (buffer.Offset < 0 || buffer.Offset > buffer.Array.Length)

This comment has been minimized.

@geoffkizer

geoffkizer Feb 27, 2017

Member

I'm curious, are the checks for Offset and Count really useful? I'm not sure how you could get into this scenario without serious hackage (e.g. unsafe code). And if you're doing that, you could very easily hack the Array pointer to point into space. Do we gain anything by this?

Also, I notice we don't do the same validation in the BufferList case.

@geoffkizer

geoffkizer Feb 27, 2017

Member

I'm curious, are the checks for Offset and Count really useful? I'm not sure how you could get into this scenario without serious hackage (e.g. unsafe code). And if you're doing that, you could very easily hack the Array pointer to point into space. Do we gain anything by this?

Also, I notice we don't do the same validation in the BufferList case.

This comment has been minimized.

@stephentoub

stephentoub Feb 27, 2017

Member

Maybe not; I was just keeping the same semantics that were there already. If we don't think it's possible, we could subsequently delete them from here and everywhere else the same checks exist.

@stephentoub

stephentoub Feb 27, 2017

Member

Maybe not; I was just keeping the same semantics that were there already. If we don't think it's possible, we could subsequently delete them from here and everywhere else the same checks exist.

@geoffkizer

This comment has been minimized.

Show comment
Hide comment
@geoffkizer

geoffkizer Feb 27, 2017

Member

LGTM. Awesome work here.

Member

geoffkizer commented Feb 27, 2017

LGTM. Awesome work here.

@stephentoub

This comment has been minimized.

Show comment
Hide comment
@stephentoub

stephentoub Feb 27, 2017

Member

Thanks for reviewing, @geoffkizer.

Member

stephentoub commented Feb 27, 2017

Thanks for reviewing, @geoffkizer.

@stephentoub

This comment has been minimized.

Show comment
Hide comment
@stephentoub

stephentoub Feb 28, 2017

Member

@dotnet-bot test Outerloop Ubuntu14.04 Debug please (something's hanging in the Pipes tests)

Member

stephentoub commented Feb 28, 2017

@dotnet-bot test Outerloop Ubuntu14.04 Debug please (something's hanging in the Pipes tests)

stephentoub added some commits Mar 1, 2017

Rewrite Socket Task-based send/receive async operations on SocketAsyn…
…cEventArgs

They're currently on top of the APM methods.  This commit changes the ReceiveAsync/Send{To}Async operations to instead sit on top of a cached set of SocketAsyncEventArgs instances.  We only cache one instance for each of send/receive, so concurrent usage when the cached instance is already being used fall back to wrapping APM as before.
Add a few more NetworkStream tests
Changing the implementation of Read/WriteAsync surfaced some missing coverage.
Account for exception type in NetworkStream.Read/WriteAsync
Prior to this change, NetworkStream.EndRead/Write would wrap SocketExceptions thrown from EndReceive/Send in an IOException, and thus Read/WriteAsync would propagate the IOException.  By changing Read/WriteAsync to delegate to ReceiveAsync/SendAsync instead, that wrapping was lost, so cases that previously threw IOException now may throw SocketException.  To address that, we could add a continuation to each Read/WriteAsync in case it fails, but that'd be adding allocations for the failure case, and they'd be pure overhead in the success case.  Instead, we pass a Boolean to the internal Read/WriteAsync helpers on Socket that indicates whether to just create a SocketException or to further wrap it in an IOException.  This adds minimal overhead while maintaining the same exception types.
@stephentoub

This comment has been minimized.

Show comment
Hide comment
@stephentoub

stephentoub Mar 3, 2017

Member

Everything passed, but I'm going to run all of the legs one more time.
@dotnet-bot test this please

Member

stephentoub commented Mar 3, 2017

Everything passed, but I'm going to run all of the legs one more time.
@dotnet-bot test this please

@stephentoub

This comment has been minimized.

Show comment
Hide comment
@stephentoub

stephentoub Mar 3, 2017

Member

@dotnet-bot test Outerloop Windows_NT Release please
@dotnet-bot test Outerloop Ubuntu14.04 Debug please
@dotnet-bot test Outerloop OSX Debug please
@dotnet-bot test outerloop PortableLinux Debug
@dotnet-bot test outerloop OpenSUSE42.1 Release
@dotnet-bot test outerloop CentOS7.1 Debug
@dotnet-bot test outerloop RHEL7.2 Release

Member

stephentoub commented Mar 3, 2017

@dotnet-bot test Outerloop Windows_NT Release please
@dotnet-bot test Outerloop Ubuntu14.04 Debug please
@dotnet-bot test Outerloop OSX Debug please
@dotnet-bot test outerloop PortableLinux Debug
@dotnet-bot test outerloop OpenSUSE42.1 Release
@dotnet-bot test outerloop CentOS7.1 Debug
@dotnet-bot test outerloop RHEL7.2 Release

@stephentoub

This comment has been minimized.

Show comment
Hide comment
@stephentoub

stephentoub Mar 3, 2017

Member

@dotnet-bot test OuterLoop RHEL7.2 Release please (known HTTP failure)
@dotnet-bot test OuterLoop PortableLinux Debug please (known HTTP failure)
@dotnet-bot test OuterLoop CentOS7.1 Debug please (known HTTP failure)

Member

stephentoub commented Mar 3, 2017

@dotnet-bot test OuterLoop RHEL7.2 Release please (known HTTP failure)
@dotnet-bot test OuterLoop PortableLinux Debug please (known HTTP failure)
@dotnet-bot test OuterLoop CentOS7.1 Debug please (known HTTP failure)

@stephentoub

This comment has been minimized.

Show comment
Hide comment
@stephentoub

stephentoub Mar 3, 2017

Member

@dotnet-bot test OuterLoop CentOS7.1 Debug please (known HTTP failure)

Member

stephentoub commented Mar 3, 2017

@dotnet-bot test OuterLoop CentOS7.1 Debug please (known HTTP failure)

@stephentoub stephentoub merged commit d851591 into dotnet:master Mar 3, 2017

22 checks passed

Help Message Build finished.
Details
Innerloop CentOS7.1 Debug Build and Test Build finished.
Details
Innerloop CentOS7.1 Release Build and Test Build finished.
Details
Innerloop OSX Debug Build and Test Build finished.
Details
Innerloop OSX Release Build and Test Build finished.
Details
Innerloop PortableLinux Debug Build and Test Build finished.
Details
Innerloop PortableLinux Release Build and Test Build finished.
Details
Innerloop Ubuntu14.04 Debug Build and Test Build finished.
Details
Innerloop Ubuntu14.04 Release Build and Test Build finished.
Details
Innerloop Windows_NT Debug Build and Test Build finished.
Details
Innerloop Windows_NT Release Build and Test Build finished.
Details
OuterLoop CentOS7.1 Debug Build finished.
Details
OuterLoop OSX Debug Build finished.
Details
OuterLoop OpenSUSE42.1 Release Build finished.
Details
OuterLoop PortableLinux Debug Build finished.
Details
OuterLoop RHEL7.2 Release Build finished.
Details
OuterLoop Ubuntu14.04 Debug Build finished.
Details
OuterLoop Windows_NT Debug Build finished.
Details
OuterLoop Windows_NT Release Build finished.
Details
Vertical uap Build Build finished.
Details
Vertical uapaot Build Build finished.
Details
Windows_NT Debug AllConfigurations Build Build finished.
Details

@stephentoub stephentoub deleted the stephentoub:socket_sendreceive_tasks branch Mar 3, 2017

@karelz karelz modified the milestone: 2.0.0 Mar 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment