refactor(proto): Move PathStatus state to the PacketNumberSpace - #780
refactor(proto): Move PathStatus state to the PacketNumberSpace#780flub wants to merge 3 commits into
Conversation
Performance Comparison Report
|
| Scenario | noq | upstream | Delta | CPU (avg/max) |
|---|---|---|---|---|
| large-single | 5347.2 Mbps | 8101.9 Mbps | -34.0% | 94.7% / 99.0% |
| medium-concurrent | 5258.7 Mbps | 7553.1 Mbps | -30.4% | 96.8% / 148.0% |
| medium-single | 4025.2 Mbps | 4524.8 Mbps | -11.0% | 96.7% / 150.0% |
| small-concurrent | 3877.4 Mbps | 5328.3 Mbps | -27.2% | 93.1% / 102.0% |
| small-single | 3472.1 Mbps | 4692.7 Mbps | -26.0% | 93.1% / 102.0% |
Netsim Benchmarks (network simulation)
| Condition | noq | upstream | Delta |
|---|---|---|---|
| ideal | 2988.7 Mbps | 4011.8 Mbps | -25.5% |
| lan | 782.4 Mbps | 810.3 Mbps | -3.4% |
| lossy | 69.8 Mbps | 59.7 Mbps | +17.0% |
| wan | 83.8 Mbps | 83.8 Mbps | ~0% |
Summary
noq is 26.3% slower on average
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/noq/pr/780/docs/noq/ Last updated: 2026-07-30T14:58:14Z |
|
The macos failure is not due to this PR I think, it is flaky... which is very interesting. I'll investigate but let it not bother you for the review. The esp32 failure is also something external. |
matheus23
left a comment
matheus23
left a comment
There was a problem hiding this comment.
Yeah I think this is correct, and the PR looks good besides a nit.
That said, I still really dislike how this is change is entirely inconsequential AFAIU, because path status frames are useless 🙃
| // pns can never be None, that would be a logical error. | ||
| let path_status = self.spaces[SpaceKind::Data] | ||
| .number_spaces | ||
| .get(path_id) | ||
| .map(|pns| pns.local_status()) | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
Are we sure we want to use unwrap_or_default() here instead of .expect()?
Just want to note this - I personally slightly lean in favor of failing loudly when invariants are broken, but I'll defer the final decision to you.
| // pns can never be None here, that would be a logical error. | ||
| let pns = self.spaces[SpaceKind::Data].number_spaces.get(&path_id); | ||
| let status = pns.map(|pns| pns.local_status()).unwrap_or_default(); |
| && !self.abandoned_paths.contains(path_id) | ||
| && path.data.validated | ||
| && path.data.local_status() == PathStatus::Available | ||
| && pns.map(|pns| pns.local_status()).unwrap_or_default() == PathStatus::Available |
There was a problem hiding this comment.
And here. (And probably more of those above)
There was a problem hiding this comment.
The path status belongs to the number space, not to a specific path
generation.
Not really, the packet number space is indexed as Initial, Handshake, Data(PathId) conceptually at least. So now PathId::Zero might have, based on code structure, three different path statuses: one for initial, one for handshake, one in the data space. So this change isn't exactly enforcing an invariant. It's changing a bad place for another bad place.
And it's not a free change either, as now site calls that used to be a single hashmap lookup are now two. For example:
let have_validated_status_available_space = self.paths.iter().any(|(path_id, path)| {
// pns can never be None here, that would be a logical error.
let pns = self.spaces[SpaceKind::Data].number_spaces.get(path_id);If the point it to remove it from path generation the place to put it would be the PathState (PathData's parent), not PacketNumerSpace
3 participants
Description
The path status belongs to the number space, not to a specific path
generation.
Part of #591.
Breaking Changes
n/a
Notes & open questions
Some invariants just show again how the current structure is badly
done. But the fallback with unwrap_or_default for PathStatus works
fine.
Also fixes an event being sent for an unknown path, that seems like
a weird choice.
Change checklist
proposed change and wrote an as clear and concise description as
they could.
intented effect.
cargo makepasses locally.