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

implement synchronous completion support in Sockets #15141

Merged
merged 1 commit into from Jan 16, 2017

Conversation

Projects
None yet
5 participants
@geoffkizer
Member

geoffkizer commented Jan 12, 2017

We complete all I/O asynchronously, even when the underlying OS operation actually completed synchronously.

Fix this so that both Windows and Unix can do synchronous socket completions.

@stephentoub @CIPop @davidsh @Priya91

var tup = (Tuple<Action<IntPtr, byte[], int, SocketError>, IntPtr, byte[], int>)args;
tup.Item1(tup.Item2, tup.Item3, tup.Item4, SocketError.Success);
}, Tuple.Create(callback, acceptedFd, socketAddress, socketAddressLen));
}

This comment has been minimized.

@stephentoub

stephentoub Jan 12, 2017

Member

Seeing this code get deleted makes me smile :)

@stephentoub

stephentoub Jan 12, 2017

Member

Seeing this code get deleted makes me smile :)

This comment has been minimized.

@geoffkizer

geoffkizer Jan 13, 2017

Member

I know what you mean 👍

@geoffkizer

geoffkizer Jan 13, 2017

Member

I know what you mean 👍

@stephentoub

Nice job, @geoffkizer. Do you have any perf measurements you can share on the impact of this on some microbenchmarks? I'd expect it to be fairly significant. Once it's merged in I'll plan to rerun some of my experiments with the managed ClientWebSocket implementation; I'd seen some notable perf hits because of this, so hopefully it'll see some sizeable improvements.

@geoffkizer

This comment has been minimized.

Show comment
Hide comment
@geoffkizer

geoffkizer Jan 13, 2017

Member

I did preliminary perf runs a while back and they were quite good. I will rerun on this change to confirm and get the latest results.

Member

geoffkizer commented Jan 13, 2017

I did preliminary perf runs a while back and they were quite good. I will rerun on this change to confirm and get the latest results.

@geoffkizer

This comment has been minimized.

Show comment
Hide comment
@geoffkizer

geoffkizer Jan 13, 2017

Member

@dotnet-bot
test outerloop Windows_NT debug
test outerloop Windows_NT release
test outerloop OSX debug
test outerloop OSX release
test outerloop Ubuntu14.04 debug
test outerloop Ubuntu14.04 release

Member

geoffkizer commented Jan 13, 2017

@dotnet-bot
test outerloop Windows_NT debug
test outerloop Windows_NT release
test outerloop OSX debug
test outerloop OSX release
test outerloop Ubuntu14.04 debug
test outerloop Ubuntu14.04 release

@stephentoub

This comment has been minimized.

Show comment
Hide comment
@stephentoub

stephentoub Jan 13, 2017

Member

@dotnet-bot test outerloop Ubuntu14.04 debug
@dotnet-bot test outerloop Windows_NT debug
@dotnet-bot test outerloop Windows_NT release

Member

stephentoub commented Jan 13, 2017

@dotnet-bot test outerloop Ubuntu14.04 debug
@dotnet-bot test outerloop Windows_NT debug
@dotnet-bot test outerloop Windows_NT release

@geoffkizer

This comment has been minimized.

Show comment
Hide comment
@geoffkizer

geoffkizer Jan 16, 2017

Member

Quick and dirty perf results on my machine:

For my simple request/response test, req/s went from 61016 to 70101, or about 15% improvement.

This is Windows, my crappy laptop, client GC, using SocketAsyncEventArgs.

The improvement I previously saw on bigger hardware was larger, but I don't have access to that hardware at the moment.

Member

geoffkizer commented Jan 16, 2017

Quick and dirty perf results on my machine:

For my simple request/response test, req/s went from 61016 to 70101, or about 15% improvement.

This is Windows, my crappy laptop, client GC, using SocketAsyncEventArgs.

The improvement I previously saw on bigger hardware was larger, but I don't have access to that hardware at the moment.

@geoffkizer

This comment has been minimized.

Show comment
Hide comment
@geoffkizer

geoffkizer Jan 16, 2017

Member

@dotnet-bot Innerloop Windows_NT Debug Build and Test

Member

geoffkizer commented Jan 16, 2017

@dotnet-bot Innerloop Windows_NT Debug Build and Test

@geoffkizer

This comment has been minimized.

Show comment
Hide comment
@geoffkizer

geoffkizer Jan 16, 2017

Member

@dotnet-bot test Innerloop Windows_NT Debug Build and Test

Member

geoffkizer commented Jan 16, 2017

@dotnet-bot test Innerloop Windows_NT Debug Build and Test

@geoffkizer geoffkizer merged commit 684af05 into dotnet:master Jan 16, 2017

8 checks passed

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 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

@karelz karelz modified the milestone: 2.0.0 Jan 21, 2017

dotnet-bot added a commit that referenced this pull request Jan 13, 2018

Move SafeBuffer and a few other files to shared CoreLib partition (#1…
…5141)

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

dotnet-bot added a commit that referenced this pull request Jan 13, 2018

Move SafeBuffer and a few other files to shared CoreLib partition (#1…
…5141)

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

safern added a commit that referenced this pull request Jan 16, 2018

Move SafeBuffer and a few other files to shared CoreLib partition (#1…
…5141)

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

safern added a commit that referenced this pull request Jan 16, 2018

Move SafeBuffer and a few other files to shared CoreLib partition (#1…
…5141)

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment