refactor(tests): Introduce ConnPairBuilder#680
Conversation
This introduces a ConnPairBuilder to construct the ConnPair and switches everything over to use it. The individual ConnPair construction methods were becoming unwieldy. There are probably a few style arguments to be had, but the current code doesn't look to bad. We can keep tweaking this.
Performance Comparison Report
|
| Scenario | noq | upstream | Delta | CPU (avg/max) |
|---|---|---|---|---|
| large-single | 5903.3 Mbps | 7951.1 Mbps | -25.8% | 97.1% / 98.7% |
| medium-concurrent | 5226.9 Mbps | 7744.3 Mbps | -32.5% | 95.9% / 97.8% |
| medium-single | 4094.6 Mbps | 4646.0 Mbps | -11.9% | 96.2% / 98.7% |
| small-concurrent | 3880.4 Mbps | 5267.0 Mbps | -26.3% | 97.4% / 99.7% |
| small-single | 3547.6 Mbps | 4746.4 Mbps | -25.3% | 96.7% / 99.0% |
Netsim Benchmarks (network simulation)
| Condition | noq | upstream | Delta |
|---|---|---|---|
| ideal | 3113.5 Mbps | 3933.9 Mbps | -20.9% |
| lan | 782.3 Mbps | 810.4 Mbps | -3.5% |
| lossy | 69.8 Mbps | 69.8 Mbps | ~0% |
| wan | 83.8 Mbps | 83.8 Mbps | ~0% |
Summary
noq is 24.3% slower on average
1d921bc97e67988149ee102723582dbddf1ad218 - artifacts
No results available
e283b37d1bfd75df832221a410a04309a6092570 - artifacts
No results available
22737663afd31cd23fe32bd9a3d9583fc3cedc8c - artifacts
No results available
5f4a15567d3ad809bbd8f9802e7194e2529a76a8 - artifacts
Netsim Benchmarks (network simulation)
| Condition | noq | upstream | Delta |
|---|---|---|---|
| ideal | 3079.6 Mbps | 3968.4 Mbps | -22.4% |
Summary
noq is 22.4% slower on average
3d66e03e956da1c66feb79e5ea2fb162f71ecfbd - artifacts
No results available
f127b4660d8264a4a82e953c911052f6ce26b025 - artifacts
Raw Benchmarks (localhost)
| Scenario | noq | upstream | Delta | CPU (avg/max) |
|---|---|---|---|---|
| large-single | 5717.4 Mbps | 7746.0 Mbps | -26.2% | 93.0% / 98.0% |
| medium-concurrent | 5763.7 Mbps | 7351.7 Mbps | -21.6% | 95.6% / 102.0% |
| medium-single | 3816.3 Mbps | 4749.4 Mbps | -19.6% | 94.3% / 102.0% |
| small-concurrent | 3967.9 Mbps | 5389.5 Mbps | -26.4% | 99.6% / 153.0% |
| small-single | 3551.0 Mbps | 4836.7 Mbps | -26.6% | 98.2% / 151.0% |
Netsim Benchmarks (network simulation)
| Condition | noq | upstream | Delta |
|---|---|---|---|
| ideal | 3114.2 Mbps | N/A | N/A |
| lan | 782.4 Mbps | N/A | N/A |
| lossy | 65.5 Mbps | N/A | N/A |
| wan | 83.8 Mbps | N/A | N/A |
Summary
noq is 24.1% slower on average
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/noq/pr/680/docs/noq/ Last updated: 2026-06-01T11:57:39Z |
divagant-martian
left a comment
divagant-martian
left a comment
There was a problem hiding this comment.
minor comments :)
| let mut builder = ConnPair::builder().with_multipath(); | ||
| builder | ||
| .server_transport_cfg | ||
| .max_concurrent_multipath_paths(6); | ||
| builder | ||
| .client_transport_cfg | ||
| .max_concurrent_multipath_paths(6); | ||
| let mut pair = builder.connect(); |
There was a problem hiding this comment.
| let mut builder = ConnPair::builder().with_multipath(); | |
| builder | |
| .server_transport_cfg | |
| .max_concurrent_multipath_paths(6); | |
| builder | |
| .client_transport_cfg | |
| .max_concurrent_multipath_paths(6); | |
| let mut pair = builder.connect(); | |
| let mut cfg = TransportConfig::default(); | |
| cfg.max_concurrent_multipath_paths(6); | |
| let mut pair = ConnPair::builder().with_transport_cfg(cfg).connect(); |
The with_multipath I think is not doing anything followed by updating the number of paths for both
There was a problem hiding this comment.
The big difference is that ConnPairBuilder::default does initialise the TransportConfig with more than just TransportConfig::default(). This is kind of why I ended up making the ConnpPairBuilder::server_transport_cfg fields public because it was more convenient to extend this pattern of extending the default-for-tests TransportConfig.
Writing this down another nice way could be to have TransportConfig::default_for_test constructor. And than you could do the pattern you suggest here easily. Would folks prefer that? The bit that slightly discourages me from that is that the default_for_test code would live in a file far away from the test code.
There was a problem hiding this comment.
also, i could change this test to expect 8 max concurrent paths. In a way that would make more sense to be honest. I think 6 is fairly arbitrary.
| } | ||
|
|
||
| /// Sets an [`EndpointConfig`] for only the server. | ||
| pub(super) fn with_server_endpoint_config(mut self, cfg: EndpointConfig) -> Self { |
There was a problem hiding this comment.
could we use consistent naming for the config/cfg sufixed methods?
Co-authored-by: Diva Martínez <26765164+divagant-martian@users.noreply.github.com>
|
PTAL: the major change is changing the builder methods that took no argument into |
matheus23
left a comment
matheus23
left a comment
There was a problem hiding this comment.
Still happy with it. Even better!
3 participants
Description
This introduces a ConnPairBuilder to construct the ConnPair and
switches everything over to use it. The individual ConnPair
construction methods were becoming unwieldy.
There are probably a few style arguments to be had, but the current
code doesn't look to bad. We can keep tweaking this.
Breaking Changes
none
Notes & open questions
split off from #675 and taken a bit further.
Change checklist