refactor(tests): Introduce ConnPairBuilder#680
Open
flub wants to merge 2 commits into
Open
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
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/noq/pr/680/docs/noq/ Last updated: 2026-05-29T15:43:17Z |
4 tasks
Collaborator
divagant-martian
left a comment
divagant-martian
left a comment
There was a problem hiding this comment.
minor comments :)
|
|
||
| /// Sets an explicit [`ServerConfig`]. | ||
| /// | ||
| /// Not this means the [`Self::server_transport_cfg`] will be unused. |
Collaborator
There was a problem hiding this comment.
Suggested change
| /// Not this means the [`Self::server_transport_cfg`] will be unused. | |
| /// Note this means the [`Self::server_transport_cfg`] will be unused. |
| server_cfg: ServerConfig, | ||
| client_cfg: ClientConfig, | ||
| ) -> Self { | ||
| fn with_default_endpoint(server_cfg: ServerConfig, client_cfg: ClientConfig) -> Self { |
Collaborator
There was a problem hiding this comment.
is this fn worth keeping?
Comment on lines
+1473
to
+1480
| 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(); |
Collaborator
There was a problem hiding this comment.
Suggested change
| 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
| } | ||
|
|
||
| /// Sets an [`EndpointConfig`] for only the server. | ||
| pub(super) fn with_server_endpoint_config(mut self, cfg: EndpointConfig) -> Self { |
Collaborator
There was a problem hiding this comment.
could we use consistent naming for the config/cfg sufixed methods?
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
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