NIOPerformanceTester: Add benchmarks for datagram channel #2198
Conversation
This file contains 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
|
@swift-nio-bot test perf please |
1 similar comment
|
@swift-nio-bot test perf please |
performance reportbuild id: 145 timestamp: Fri Jun 17 18:29:19 UTC 2022 results
comparison
significant differences found |
Signed-off-by: Si Beaumont <[email protected]>
simonjbeaumont
force-pushed
the
sb/udp-perf-tests
branch
from
8e1bf6a to
ee9ea6e
Compare
|
@Lukasa I think this is ready for a review. |
Lukasa
requested changes
Jul 4, 2022
| } | ||
| } | ||
|
|
||
| final class DatagramBootstrapCreateBenchmark: DatagramClientBenchmark, Benchmark { |
There was a problem hiding this comment.
Does this need to inherit? It doesn't seem to use this well.
There was a problem hiding this comment.
You're right—throughout this PR—that some of these benchmark classes inherit very little from the base. This one inherits almost nothing.
The reason I went this way is because I planned to port this code over to the allocation test framework where it would be nice for everything to share some common scaffolding.
It also makes it clear that everything is set up the same for every benchmark, and then we'll just benchmark a different part of the flow in the critical loop. This is motivated by the discussion in the issue #2187. It would be nice to make it very clear that in all of these tests, we establish a control, and change and measure just one thing.
WDYT?
|
|
||
| func run() throws -> Int { | ||
| for _ in 1...self.iterations { | ||
| try! self.clientBootstrap.bind(to: self.localhostPickPort).flatMap { $0.close() }.wait() |
There was a problem hiding this comment.
Same here, I don't think this inheritance serves us for this benchmark.
| } | ||
| } | ||
|
|
||
| final class DatagramChannelConnectBenchmark: DatagramClientBenchmark, Benchmark { |
There was a problem hiding this comment.
Nor does it really serve us much here, I think.
|
|
||
| func run() throws -> Int { | ||
| for _ in 1...self.iterations { | ||
| try! self.clientChannel.writeAndFlush(self.payload).wait() |
There was a problem hiding this comment.
I think this needs some signal to delay the finishing of the test until the server has read everything. Otherwise this test will be very variable based on how fast the server is reading.
There was a problem hiding this comment.
Ah, that's an interesting one. I noticed that the current allocation benchmark we have waits for the server to read everything. However, I found that when I increased the iterations in here to get the runtime we want for the perf benchmarks, I would see some non-uniformity and some lost packets, even on localhost. IIUC there's nothing to guarantee that all the packets would arrive, even on localhost.
Before opening this PR, I had something in here to assert-at-least-one-echo-response-was-received but I thought better of it.
This test currently measures the client sending out the datagrams, at which point we've measured all of NIO's involvement in getting the packets out the door.
You're right that we're missing some test of the read path. We could consider adding a test where the client continually sends payloads and we fulfil a promise only when the server has seen a given number of responses?
WDYT?
There was a problem hiding this comment.
I think that latter idea is the way to go.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Motivation:
#2084 recently added preliminary support for connected datagram sockets which presents opportunities for performance optimisations in some use cases. In order to start optimising these flows there should be some benchmarks in place.
As described in #2187, connected-mode UDP introduces two additional dimensions:
AddressedEnvelope(when connected).It also explains the kind of flows we're interested in benchmarking.
Modifications:
NIOPerformanceTesterfor various datagram channel flows.Result: