fix(proto): count udp_rx.ios per udp datagram instead of quic packet#725
fix(proto): count udp_rx.ios per udp datagram instead of quic packet#725divagant-martian wants to merge 4 commits into
Conversation
Performance Comparison Report
|
| Scenario | noq | upstream | Delta | CPU (avg/max) |
|---|---|---|---|---|
| large-single | 5256.0 Mbps | 7947.4 Mbps | -33.9% | 97.4% / 98.7% |
| medium-concurrent | 5538.6 Mbps | 7774.8 Mbps | -28.8% | 96.1% / 97.7% |
| medium-single | 4032.5 Mbps | 4470.4 Mbps | -9.8% | 96.5% / 98.4% |
| small-concurrent | 3889.9 Mbps | 5135.4 Mbps | -24.3% | 98.0% / 100.0% |
| small-single | 3497.6 Mbps | 4584.4 Mbps | -23.7% | 95.4% / 97.8% |
Netsim Benchmarks (network simulation)
| Condition | noq | upstream | Delta |
|---|---|---|---|
| ideal | 3086.4 Mbps | 4022.9 Mbps | -23.3% |
| lan | 782.4 Mbps | 803.8 Mbps | -2.7% |
| lossy | 55.9 Mbps | 69.8 Mbps | -20.0% |
| wan | 83.8 Mbps | 83.8 Mbps | ~0% |
Summary
noq is 24.8% slower on average
5f02c8afa0cb1989d58eb53083299a22356488d0 - artifacts
Raw Benchmarks (localhost)
| Scenario | noq | upstream | Delta | CPU (avg/max) |
|---|---|---|---|---|
| large-single | 5878.0 Mbps | 7878.1 Mbps | -25.4% | 97.5% / 98.8% |
| medium-concurrent | 5493.0 Mbps | 7788.0 Mbps | -29.5% | 96.5% / 98.0% |
| medium-single | 3942.6 Mbps | 4730.2 Mbps | -16.7% | 96.7% / 98.6% |
| small-concurrent | 3842.6 Mbps | 5142.5 Mbps | -25.3% | 97.6% / 99.7% |
| small-single | 3572.0 Mbps | 4666.6 Mbps | -23.5% | 96.1% / 98.4% |
Netsim Benchmarks (network simulation)
| Condition | noq | upstream | Delta |
|---|---|---|---|
| ideal | 3044.8 Mbps | 4078.5 Mbps | -25.3% |
| lan | 782.4 Mbps | 810.4 Mbps | -3.5% |
| lossy | 69.8 Mbps | 59.1 Mbps | +18.1% |
| wan | 83.8 Mbps | 83.8 Mbps | ~0% |
Summary
noq is 24.2% slower on average
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/noq/pr/725/docs/noq/ Last updated: 2026-06-25T21:06:34Z |
matheus23
left a comment
matheus23
left a comment
There was a problem hiding this comment.
Would be neat to take our learnings about the nuance of stats to the doc comments of said stats.
E.g. we could add the invariants we expect (ios <= datagrams) to the docstring, and the fact that discarded datagrams are not counted towards ios or datagrams.
| let rx = &mut self.path_stats.for_path(path_id).udp_rx; | ||
| rx.datagrams += 1; | ||
| rx.bytes += first_decode.len() as u64; | ||
| rx.ios += 1; |
There was a problem hiding this comment.
It's weird that rx.ios == rx.datagrams in this case.
For one, shouldn't ios count a full GRO batch as one operation, but datagrams should count each one in the batch?
Also, I think even if we Ah shit, ignore me we need to know the Path ID. This is better anyways, because then early_discard_packet above, we should count it in ios, but not in datagrams?bytes and datagrams can be better in relation to each other.
There was a problem hiding this comment.
This is rather difficult on the receive side. You'd have to do it in the EndpointDriver I guess but you don't have enough information there let alone access to the stats. I guess you could do something convoluted like keep the counter there per CID and than send the counter over with each datagram delivered and than keep per CID stats over here that can be summed to return the right per-path and per-connection values at query time.
But that's pretty convoluted. And looking at RecvState::poll_socket I'm not even sure it'll be that simple to achieve. I think this might not be worth it.
There was a problem hiding this comment.
Yeah totally fair. So this will mean that currently ios will always be equal to datagrams? I'm fine with that. Maybe at some point we'll be refactoring things so it can become a useful stat.
| #[test] | ||
| fn udp_rx_ios_overcounts_batched_udp_datagrams() { | ||
| // this is a regression test |
There was a problem hiding this comment.
I like to just put "regression_" into the test name itself. The test name also seems to assert that ios should overcount datagrams, which is backwards (I personally interpret test names like that, and I thought that's somewhat common).
| #[test] | |
| fn udp_rx_ios_overcounts_batched_udp_datagrams() { | |
| // this is a regression test | |
| #[test] | |
| fn regression_udp_rx_ios_lt_datagrams() { | |
| // this is a regression test |
| let rx = &mut self.path_stats.for_path(path_id).udp_rx; | ||
| rx.datagrams += 1; | ||
| rx.bytes += first_decode.len() as u64; | ||
| rx.ios += 1; |
There was a problem hiding this comment.
This is rather difficult on the receive side. You'd have to do it in the EndpointDriver I guess but you don't have enough information there let alone access to the stats. I guess you could do something convoluted like keep the counter there per CID and than send the counter over with each datagram delivered and than keep per CID stats over here that can be summed to return the right per-path and per-connection values at query time.
But that's pretty convoluted. And looking at RecvState::poll_socket I'm not even sure it'll be that simple to achieve. I think this might not be worth it.
|
The more I check these |
3 participants
Description
Fixes counting of rx io operations. Adds a regression test with the broken invariant
Breaking Changes
n/a
Notes & open questions
n/a
Change checklist