This repository was archived by the owner on Dec 18, 2018. It is now read-only.
Making performance improvements to sockets#2366
Closed
davidfowl wants to merge 2 commits into
Closed
Conversation
Member
Author
|
Failures:
I believe this is one that @pakrym encountered when porting to the latest pipelines
|
Member
Author
|
Also the OSX logs had this: |
davidfowl
force-pushed
the
davidfowl/sockets-perf
branch
from
7f29d52 to
c7044fe
Compare
benaadams
reviewed
Mar 6, 2018
| var t = (Tuple<Action<object>, object>)s; | ||
| t.Item1(t.Item2); | ||
| }, | ||
| Tuple.Create(action, state)); |
Member
Author
There was a problem hiding this comment.
For now this is only used per connection so not a big deal
Contributor
|
There is something wrong with this (in principle) :( Numbers are good though :) I think corefx needs a fix |
halter73
suggested changes
Mar 7, 2018
| _error = socketError; | ||
| _bytesTransfered = bytesTransferred; | ||
| Interlocked.Exchange(ref _callback, _callbackCompleted)?.Invoke(); | ||
| var continuation = Interlocked.Exchange(ref _callback, _callbackCompleted); |
Member
There was a problem hiding this comment.
Isn't this double scheduling true-async reads? If so, shouldn't this readerScheduler be switched to PipeScheduler.Inline for the socket transport to avoid double dispatching (or even triple dispatching by the time the data flows through the request body pipe as currently implemented).
Member
|
So I talked to @davidfowl about the purpose of double dispatching, and his theory is that without this change, a single socket could dominate the usage of the limited I/O threads for as long as there's data already available in the OS's input buffer by the time ReceiveAsync is called. I figured if that's the case, certain connections must be starving others of resources in dev, and that would mean this change should reduce wrk's reported standard deviation for latency. It looks like @davidfowl is on to something, because the latency numbers for the Socket transport are immensely better with this PR. Json WindowsSocketsBeforeAfterSo not only did this chage reduce the standard deviation for latency from 93.79ms to 14.06ms, it also reduced the average latency from 7.18ms to 1.36ms. The 99th latency percentile is even more impressive. The 50/75/90th percentiles are slightly better than before (note the difference between ms and us), but the 99th percentile went from 10.06ms to 0.99ms! It's interesting that the 99th percentile for latency is still lower than the average after than this change. It really shows that my stats prof was right about the mean being heavily impacted by outliers. |
Contributor
|
@davidfowl @halter73 very interesting find 👍 |
Member
Author
Bumping up IO threads doesn't solve the problem. We just need to efficiently get off the IO thread if the next read is synchronous. @halter73 As an alternative, we could try dispatching if we're not on a thread pool thread and IO completes synchronously. Edit: Thinking about it though, it's pretty similar to what's there today since the read loop is always going to be on an IO thread... |
|
cc: @vancem @geoffkizer |
Member
Author
- Run SocketAwaitable continuations asynchronously - Run using the global thread pool queues instead of using local queues.
davidfowl
force-pushed
the
davidfowl/sockets-perf
branch
from
c7044fe to
6d65f86
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
of using local queues.
Theories:
Plaintext Linux
Libuv:
Thread count default
Thread count 2
Sockets:
Before:
After:
Plaintext Windows
Libuv:
Thread count default
Thread count 2
Sockets:
Before
After
Json Linux
Libuv
Sockets
Before
After
Json Windows
Libuv
Sockets
Before
After