Add new optional SocketPipe transport #1480
Conversation
…get it working without adding a new enum
…hack to get it working without adding a new enum" This reverts commit 6ccb808.
src/BenchmarksApps/Kestrel/PlatformBenchmarks/SocketPipes/SocketPipeWriter.cs
Outdated
Show resolved
Hide resolved
| options.IOQueueCount = threadCount; | ||
| } | ||
| #if NETCOREAPP5_0 | ||
| typeof(SocketTransportOptions).GetProperty("WaitForDataBeforeAllocatingBuffer")?.SetValue(options, false); |
There was a problem hiding this comment.
@sebastienros @halter73 What's the deal with this? Don't we have the bits yet?
There was a problem hiding this comment.
The aspnet repos might not be up to date with the latest sdk. Alternatively we can force it in the driver command line.
There was a problem hiding this comment.
This was merged a while ago. It should work with a recent enough $(MicrosoftAspNetCoreAppPackageVersion). We probably need to look at updating the csproj or dependencies.props.
There was a problem hiding this comment.
I set --sdk 5.0.100-preview.4.20202.8 in the baseline KPI runs.
Let me try to update the sdk in the aspnet repos, I did it last time.
src/BenchmarksApps/Kestrel/PlatformBenchmarks/BenchmarkConfigurationHelpers.cs
Outdated
Show resolved
Hide resolved
src/BenchmarksApps/Kestrel/PlatformBenchmarks/SocketPipes/KestrelServerOptionsSetup.cs
Outdated
Show resolved
Hide resolved
| { | ||
| // more or less a copy of https://github.com/dotnet/aspnetcore/blob/master/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketAwaitableEventArgs.cs | ||
| // but without PipeScheduler (always inlining) | ||
| internal sealed class SocketAwaitableEventArgs : SocketAsyncEventArgs, ICriticalNotifyCompletion |
There was a problem hiding this comment.
Do you need this at all? You can use the SocketTask APIs that do this internally. Kestrel only does this because it's trying to schedule directly to the IOQueue from continuations.
There was a problem hiding this comment.
The cost of awaiting socket.ReadAsync is quite big, by using this custom class I was able to get +15k RPS. I am going to create an issue in runtime repo
There was a problem hiding this comment.
The cost of awaiting socket.ReadAsync is quite big, by using this custom class I was able to get +15k RPS.
"quite big" is ~15K out of ~1065k in a benchmark that's basically seeing how fast it can call this method? To me that's not worth all the extra code and complexity and diverging from how we want developers to use Socket.
I am going to create an issue in runtime repo
Recommending what? If you have concrete ideas for improving it, great, I'd love to see it get faster, but it has to account for a bunch of things you're not here, like the possibility it might be used concurrently, being used via an interface via ValueTask, that there might be a custom SynchronizationContext/TaskScheduler to be used, possible flowing of ExecutionContext, etc.
|
|
||
| var array = _array; | ||
| var args = _awaitableEventArgs; | ||
| args.SetBuffer(new Memory<byte>(array, _length, array.Length - _length)); |
There was a problem hiding this comment.
You should be able to call _socket.ReadAsync here directly instead of using SocketAsyncEventArgs (see https://github.com/dotnet/runtime/blob/e22cf553e11d500c1523034f2c7ff745014e0629/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs#L18).
There was a problem hiding this comment.
I know, but it's slow. I am going to create an issue in runtime repo
src/BenchmarksApps/Kestrel/PlatformBenchmarks/SocketPipes/SocketPipeWriter.cs
Show resolved
Hide resolved
| var args = _awaitableEventArgs; | ||
| args.SetBuffer(new Memory<byte>(array, _length, array.Length - _length)); | ||
|
|
||
| if (_socket.ReceiveAsync(args)) |
There was a problem hiding this comment.
We shouldn't wait to receive more data if everything hasn't been examined yet. There may be no more data coming.
src/BenchmarksApps/Kestrel/PlatformBenchmarks/SocketPipes/SocketPipeReader.cs
Outdated
Show resolved
Hide resolved
|
|
||
| _offset = 0; | ||
|
|
||
| return new ValueTask<FlushResult>(new FlushResult(isCanceled: false, isCompleted: true)); |
There was a problem hiding this comment.
This isCompleted should also only be true if the socket is closed. In Kestrel, we expect PipeWriter.FlushAsync() to never throw. Our transports just log, trip a ConnectionClosed token and return completed FlushResults.
src/BenchmarksApps/Kestrel/PlatformBenchmarks/SocketPipes/SocketPipeWriter.cs
Outdated
Show resolved
Hide resolved
|
I know this isn't a "real" transport, so a lot of my comments are FYI. We can decide how important (or not) they are to address. If this is just to get a sort of upper bound on performance, this is probably fine once we get rid of the extraneous code. |
|
@adamsitnik time to close this? |
This PR adds a simplified transport that is still using the abstraction of pipes, but it has it's own simplified logic that does not take any locks and does not use any extra tasks
For JSON Platform we jump from 920k to 1065k, in a separate PR I am going to send some improvements that take it to 1100k
@davidfowl could you please take a look?