Socket benchmarks initial implementation #1970
Conversation
|
FYI, a related one I made to measure how many packets per second a set of machines can handle. https://github.com/sebastienros/PacketsPerSecond/blob/master/Program.cs |
|
Currently dealing with a task in QUIC, I plan to get back to this early next week. /cc: @CarnaViire |
|
It's ready for review now @CarnaViire, there are fields and methods which is duplicating with Tls but decided not to touch them for now, I'll address them as a follow-up later, want merge this first. |
| @@ -3,9 +3,18 @@ | |||
|
|
|||
| namespace System.Net.Benchmarks; | |||
|
|
|||
| internal abstract class SingleStreamConnection<TStream> : IConnection | |||
There was a problem hiding this comment.
There were no abstract method in this class, so I deleted it.
There was a problem hiding this comment.
I believe we'd need to revert the changes in this file.
It is abstract because it cannot be used directly. The protected InnerStream property is technically playing the role of the "abstract method". There's no actual abstract method bc the class is used as a base class in both server and client, and they have different consuming methods (AcceptInboundStreamAsync vs EstablishStreamAsync).
We can pull the full single-stream client connection implementation -- but only a part of server connection implementation -- "up". But it would still leave SingleStreamConnection abstract.
There's how I see it:
1. (move "up" from Tls to Common)
- ITlsBenchmarkClientConnection
+ IStreamingClientConnection<TOptions>2. add IStreamingServerConnection : IConnection with AcceptInboundStreamAsync method pulled up from ITlsBenchmarkServerConnection
3. (remove AcceptInboundStreamAsync from ITlsBenchmarkServerConnection)
- ITlsBenchmarkServerConnection : IConnection
+ ITlsBenchmarkServerConnection : IStreamingServerConnection4. Keep SingleStreamConnection abstract
5. (move "up" from Apps to Common)
- SslStreamClientConnection : SingleStreamConnection<SslStream>, ITlsBenchmarkClientConnection
+ SingleStreamConnectionClientConnection<TStream, TOptions> : SingleStreamConnection<TStream>, IStreamingClientConnection<TOptions>6. add SingleStreamServerConnection<TStream, TOptions> : SingleStreamConnection<TStream>, IStreamingClientConnection<TOptions> with AcceptInboundStreamAsync method impl pulled up from SslStreamBenchmarkServerConnection
7. (remove AcceptInboundStreamAsync from SslStreamBenchmarkServerConnection )
- SslStreamBenchmarkServerConnection : SingleStreamConnection<SslStream>, ITlsBenchmarkServerConnection
+ SslStreamBenchmarkServerConnection : SingleStreamServerConnection<SslStream>, ITlsBenchmarkServerConnectionThis definitely can be done as a follow-up (I mean, not in this PR), and it is even still up to discussion. It technically might be more trouble than worth, but it can support the positioning of "NetworkStream" benchmarks alongside Tls benchmarks as a group of "streaming" benchmarks.
| } | ||
| ' | ||
|
|
||
| connection-establishment: |
There was a problem hiding this comment.
It is generally better to have shorter names, if possible, bc it is easier to use them in command line :)
| connection-establishment: | |
| connect: |
| var elapsed = sw.ElapsedTicks * 1.0 / Stopwatch.Frequency; | ||
| return (successRequests / elapsed, exceptionRequests); | ||
| } | ||
| } |
There was a problem hiding this comment.
Can you pls unify the file endings so that all of the files end with a new line?
| } | ||
| sw.Restart(); | ||
| await using var connection = await establishConnectionAsync(options).ConfigureAwait(false); | ||
| await using var stream = await connection.EstablishStreamAsync(options).ConfigureAwait(false); |
There was a problem hiding this comment.
Wait, I am a bit confused... we discussed that we need socket benchmarks that call actual Socket APIs :) so that we can have/add scenarios that are impossible to do using Stream APIs (e.g. UDP socket benchmarks)
When we've discussed NetworkStream previously, in my mind that was about an additional benchmark, more intended to compare with SslStream to see the TLS overhead
There was a problem hiding this comment.
I see, looks like I got confused from our last discussion as well, and didn't actually realize I should be using direct socket APIs here, I'll change the design to use Socket APIs directly.
There was a problem hiding this comment.
As discussed offline: we won't rewrite the current NetworkStream benchmarks (but most possibly rename them to avoid confusion), and will implement raw Socket API ones alongside as a follow-up
|
|
||
| return await SocketBenchmarkClient.RunAsync(args).ConfigureAwait(false); | ||
|
|
||
| internal class SocketBenchmarkClient : BenchmarkClient<SocketClientOptions> |
There was a problem hiding this comment.
I was thinking, and I still believe we'd need to separate these benchmarks (which are technically NetworkStream benchmarks) from the to-be-added pure-Socket-API benchmarks. Otherwise it would be hard to navigate within the benchmark class -- this one is big enough already.
And creating a socket is technically a two-liner (var socket = new Socket(...); await socket.ConnectAsync(...);) and it can/should be potentially different in new benchmarks so we can maybe set some socket options based on passed parameters. So IMO there's not really much to reuse? Or am I missing something?
So, currently the proposal is to rename
SocketBenchmark -> NetworkStreamBenchmark,
src/System/Net/Benchmarks/Apps/Socket/ -> src/System/Net/Benchmarks/Apps/Streaming/,
and maybe even
src/System/Net/Benchmarks/Apps/Tls/ -> src/System/Net/Benchmarks/Apps/Streaming/Tls/
WDYT?
cc @rzikm for additional opinions
There was a problem hiding this comment.
I was thinking, and I still believe we'd need to separate these benchmarks (which are technically NetworkStream benchmarks) from the to-be-added pure-Socket-API benchmarks. Otherwise it would be hard to navigate within the benchmark class -- this one is big enough already.
After thinking a while on my own, I totally agree with this. I'm planning to change this as NetworkStreamBenchmark and add another Benchmark for pure Socket APIs, because we can extend those Socket Benchmarks with UDP and possibly(?) with other protocols as well in the future.
And creating a socket is technically a two-liner (var socket = new Socket(...); await socket.ConnectAsync(...);) and it can/should be potentially different in new benchmarks so we can maybe set some socket options based on passed parameters. So IMO there's not really much to reuse? Or am I missing something?
I also think that we don't have much to reuse here and it is highly possible that some benchmarks can customize their usages of socket while creating connection (even during connection?), maybe we can create a ConnectCallback or something like that as we have in SocketsHttpHandler. (This is what I'm thinking from top of my head)
So, currently the proposal is to rename
SocketBenchmark -> NetworkStreamBenchmark,
src/System/Net/Benchmarks/Apps/Socket/ -> src/System/Net/Benchmarks/Apps/Streaming/,
and maybe even
src/System/Net/Benchmarks/Apps/Tls/ -> src/System/Net/Benchmarks/Apps/Streaming/Tls/
The only question that comes in my mind is, should we keep QUIC under Tls folder here. Technically, it's a stream protocol as well but do we want to keep it as separated? Or you don't have any problem with moving it with Tls?
Adds benchmark for Socket
Three different scenarios implemented: