fix(multipath): Store abandoned paths in bounded memory#688
Conversation
The previous version kept a full set of all abandoned paths. But the entire set is not sparse, there are only up to max_concurrent_multipath_paths sparse entries. All paths below that must always be abandoned. This bounds the memory taken by this set.
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/noq/pr/688/docs/noq/ Last updated: 2026-06-03T12:45:03Z |
Performance Comparison Report
|
| Scenario | noq | upstream | Delta | CPU (avg/max) |
|---|---|---|---|---|
| large-single | 5442.3 Mbps | 8039.7 Mbps | -32.3% | 93.2% / 98.1% |
| medium-concurrent | 5457.2 Mbps | 7900.0 Mbps | -30.9% | 93.3% / 99.0% |
| medium-single | 4160.8 Mbps | 4612.8 Mbps | -9.8% | 93.0% / 101.0% |
| small-concurrent | 3831.5 Mbps | 5309.8 Mbps | -27.8% | 98.1% / 153.0% |
| small-single | 3508.8 Mbps | 4663.0 Mbps | -24.8% | 96.0% / 152.0% |
Netsim Benchmarks (network simulation)
| Condition | noq | upstream | Delta |
|---|---|---|---|
| ideal | N/A | 4050.8 Mbps | N/A |
| lan | N/A | 810.3 Mbps | N/A |
| lossy | N/A | 69.9 Mbps | N/A |
| wan | N/A | 83.8 Mbps | N/A |
Summary
noq is 26.6% slower on average
41c4ed53dd729d8c1d4022a6528c843c4490b746 - artifacts
No results available
b41a6ae0b1a9b7b6603d5ff707dba18fbd6bfef8 - artifacts
Raw Benchmarks (localhost)
| Scenario | noq | upstream | Delta | CPU (avg/max) |
|---|---|---|---|---|
| large-single | 5830.4 Mbps | 8017.3 Mbps | -27.3% | 97.5% / 99.0% |
| medium-concurrent | 5482.4 Mbps | 7761.4 Mbps | -29.4% | 96.9% / 98.4% |
| medium-single | 4115.2 Mbps | 4749.5 Mbps | -13.4% | 95.2% / 97.8% |
| small-concurrent | 3772.8 Mbps | 5250.5 Mbps | -28.1% | 96.9% / 99.5% |
| small-single | 3617.7 Mbps | 4813.0 Mbps | -24.8% | 96.5% / 98.9% |
Netsim Benchmarks (network simulation)
| Condition | noq | upstream | Delta |
|---|---|---|---|
| ideal | 3058.8 Mbps | 4106.3 Mbps | -25.5% |
| lan | 782.4 Mbps | 810.4 Mbps | -3.4% |
| lossy | 69.9 Mbps | 55.8 Mbps | +25.1% |
| wan | 83.8 Mbps | 83.8 Mbps | ~0% |
Summary
noq is 24.8% slower on average
matheus23
left a comment
matheus23
left a comment
There was a problem hiding this comment.
Weeee this must've been at least a little fun writing.
Please add a small unit test that checks that (1) tests the set compacts correctly and (2) ensures it behaves like a normal set.
(proptests would actually be really neat for this but ... we don't have to proptest everything 🙃 )
| /// This and any lower numbered paths have been abandoned. | ||
| min: Option<PathId>, |
There was a problem hiding this comment.
| /// This and any lower numbered paths have been abandoned. | |
| min: Option<PathId>, | |
| /// This and any lower numbered paths have been abandoned. | |
| continuous_max: Option<PathId>, |
?
Sorry for the bikeshedding. min can give the impression that it would represent the minimum PathId among all abandoned ones.
divagant-martian
left a comment
divagant-martian
left a comment
There was a problem hiding this comment.
I like the wrapping api.
Originally I thought we could use a range set. The codebase already has one that I haven't checked if has "special" extra properties so that it could not be swapped for Rudi's range_collection, to use that one in both cases (acks - from what I remember, and here)
This is not pushback tho, this is a fine and elegant solution that helps with mem. These little kind of problems are fun to solve.
Agree that this needs tests
| self.set.last().copied().or(self.min) | ||
| } | ||
|
|
||
| /// Whether the the path is already abandoned. |
There was a problem hiding this comment.
| /// Whether the the path is already abandoned. | |
| /// Whether the path is already abandoned. |
| self.set.retain(|v| match self.min { | ||
| Some(ref min) if *v <= *min => false, | ||
| Some(ref mut min) if *v == min.next() => { | ||
| *min = *v; |
There was a problem hiding this comment.
| self.set.retain(|v| match self.min { | |
| Some(ref min) if *v <= *min => false, | |
| Some(ref mut min) if *v == min.next() => { | |
| *min = *v; | |
| self.set.retain(|v| match self.min.as_mut() { | |
| Some(min) if *v <= *min => false, | |
| Some(min) if *v == min.next() => { | |
| *min = *v; |
style preference, what do you think?
divagant-martian
changed the title
3 participants
Description
The previous version kept a full set of all abandoned paths. But the
entire set is not sparse, there are only up to
max_concurrent_multipath_paths sparse entries. All paths below that
must always be abandoned.
This bounds the memory taken by this set.
Breaking Changes
none
Notes & open questions
A kind of naive implementation, but I like it because it is
simple. You could do slightly more optimised versions. My favourite is
probably a bit vector for the sparse set. But would have to benchmark
to really know if this would have any impact and that seems overkill
for now.
You won't be able to go for a version that does not allocate since we
need to be able to modify the maximum number of allowed concurrent
paths to change during the lifetime of a connection, even if we don't
yet have the API for that today.
Replaces #683
Change checklist