-
Notifications
You must be signed in to change notification settings - Fork 523
Conversation
705,348.96 rps -> 1,030,679.42 rps /cc @davidfowl @halter73 |
That's some SRS RPS 👍 |
1,069,312.35 on the 3rd wrk run (with Kestrel warmed up) |
If |
New numbers from SmurfLab with this PR, up from ~497k previously:
|
Try non pipelined? I'm also seeing an improvement there. |
Added precomputing Http Version bytes also |
Wow. With the addition of 371b2ad we break 600K:
|
And non-pipelined results:
|
Have one last additional item for Server and Date; just need to sort out tests |
Latest results, getting higher:
|
Added Server and Date precomputation; while still allowing swap out/clearing |
d9cf936
to
6c7684a
Compare
1111def
to
e9f1a8b
Compare
} | ||
} | ||
|
||
public void CopyFromAscii(byte[] data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this called CopyFromAscii?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought that's what you said? CopyAsciiFrom
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh; just the string method?
Add ProducingStart and ProducingComplete methods to ISocketOutput. These new methods can help prevent double buffering when encoding.
93c7e0c
to
5befd5c
Compare
I just filed dotnet/extensions#64 we might want to consider. It's related to this PR in that it would open a channel from app to server for pre-computed ascii byte[] response headers to be written. |
There's some duplication here with the CopyFrom logic. This looks much cleaner with @halter73 's changes |
@@ -29,6 +29,17 @@ public partial class Frame : FrameContext, IFrameControl | |||
private static readonly ArraySegment<byte> _emptyData = new ArraySegment<byte>(new byte[0]); | |||
private static readonly byte[] _hex = Encoding.ASCII.GetBytes("0123456789abcdef"); | |||
|
|||
private static readonly byte[] _bytesConnectionClose = Encoding.ASCII.GetBytes("\r\nConnection: close"); | |||
private static readonly byte[] _bytesConnectionKeepAlive = Encoding.ASCII.GetBytes("\r\nConnection: keep-alive"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should move these out of frame into a static class? Similar to reason phrases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Less clutter?
Seem to be spitting out the right length data but in lots of zero bytes; not sure I've integrated right. |
count += end.CopyFrom(_httpVersion == HttpVersionType.Http1_1 ? _bytesHttpVersion1_1 : _bytesHttpVersion1_0); | ||
count += end.CopyFrom(statusBytes); | ||
count += _responseHeaders.CopyTo(ref end); | ||
count += end.CopyFrom(_bytesEndHeaders, 0, _bytesEndHeaders.Length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. I was considering making CopyFrom mutate the iterator. I wonder if CopyTo should also mutate for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed to get the count for the count; though I have 27 failing tests I need to work out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh? absolutely CopyFrom should mutate the iterator, doesn't it? CopyTo can be changed to match.
ProducingComplete should only take (end), or (begin,end), but not the (count). The SocketOutput knows what to write based on the original tail's iterator returned, and the complete iterator passed in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the count is for bytesPrecompted rather than traversing the iterator to work it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The majority of the cases are where the entire response fits in one block, so having SocketOutput call begin.GetLength(end) will just subtract the index integers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. The count is only to simplify write-behind buffering calculation. We do traverse the blocks and count how many bytes we're actually writing before calling uv_write
.
85de4fa
to
438a707
Compare
Rebased on SocketOuput changes; fingers crossed for performance |
438a707
to
feb4040
Compare
RPS isn't great; but CPU is low, so I think there is high lock contention - making some minor adjusts to move work out of the locks. |
Reduced the in lock work; but also turned out I had receive side scaling turned off. When I turn that on... Oh my...
Best I've seen in my environment yet |
Nice. I'm merging this in now. I plan to make a few more changes, like changing the signature of ProducingComplete to not take a count, but that can be done in another PR. |
🏁 time to rebase all the things... |
Encode all the standard preamble once for all requests