Rewrite Socket Task-based send/receive async operations on SocketAsyncEventArgs #16502
Conversation
bef6f13 to
13755f4
Compare
| { | ||
| throw new ArgumentNullException(nameof(buffer.Array)); | ||
| } | ||
| if (buffer.Offset < 0 || buffer.Offset > buffer.Array.Length) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
LGTM. Awesome work here. |
|
Thanks for reviewing, @geoffkizer. |
0284351 to
a5218ff
Compare
|
@dotnet-bot test Outerloop Ubuntu14.04 Debug please (something's hanging in the Pipes tests) |
455f06f to
9a4fdbb
Compare
Changing the implementation of Read/WriteAsync surfaced some missing coverage.
…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.
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.
9a4fdbb to
14ae08d
Compare
|
Everything passed, but I'm going to run all of the legs one more time. |
|
@dotnet-bot test Outerloop Windows_NT Release please |
|
@dotnet-bot test OuterLoop RHEL7.2 Release please (known HTTP failure) |
|
@dotnet-bot test OuterLoop CentOS7.1 Debug please (known HTTP failure) |
…ceive_tasks Rewrite Socket Task-based send/receive async operations on SocketAsyncEventArgs Commit migrated from dotnet/corefx@d851591
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:
And top allocations after:
In a small change that forces all receives to be asynchronous by doing them before the send, top allocations before:
And top allocations after:
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