perf(transport/recovery): optimize SentPackets::take_ranges
#2245
Conversation
There was a problem hiding this comment.
Once this benchmark is approved, I will split out the benchmark into a separate pull request. Merging that second pull request first will give us a baseline (i.e. main branch) before merging this pull request.
There was a problem hiding this comment.
Pull request merging benchmark only: #2253
`SentPackets` keep track of all inflight packets. On receiving an ACK, the acked packet ranges are removed via `SentPackets:take_ranges`. In normal network scenarios one can assume that the amount of packets before a range is small (e.g. due to reordering) or zero, while the amount of packets after a range is large, i.e. all remaining in-flight packets. The previous implementation assumed the opposite, leading to an overhead linear to the amount of packets after the range. This commit changes the algorithm such that it is linear to the amount of packets before the range, which again, is assumed to be smaller than the amount of packets after the acked range.
mxinden
force-pushed
the
sent-packets
branch
from
28ecab5
to
1de870b
Compare
There was a problem hiding this comment.
Running the benchmark above, the patch below shows a 38x improvement over current main.
$ critcmp main patch
group main patch
----- ---- -----
SentPackets::take_ranges 38.46 156.8±13.11µs ? ?/sec 1.00 4.1±1.20µs ? ?/sec
$ cat /proc/cpuinfo
model name : 12th Gen Intel(R) Core(TM) i9-12900HK
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2245 +/- ##
==========================================
- Coverage 95.37% 95.37% -0.01%
==========================================
Files 112 112
Lines 36569 36593 +24
==========================================
+ Hits 34879 34900 +21
- Misses 1690 1693 +3 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Failed Interop TestsQUIC Interop Runner, client vs. server neqo-latest as client
neqo-latest as server
All resultsSucceeded Interop TestsQUIC Interop Runner, client vs. server neqo-latest as client
neqo-latest as server
Unsupported Interop TestsQUIC Interop Runner, client vs. server neqo-latest as client
neqo-latest as server
|
neqo-transport/src/recovery/sent.rs
Outdated
| // According to RFC 9000 19.3.1 ACK ranges are in descending order: | ||
| // | ||
| // > Each ACK Range consists of alternating Gap and ACK Range Length | ||
| // > values in **descending packet number order**. | ||
| // | ||
| // <https://www.rfc-editor.org/rfc/rfc9000.html#section-19.3.1> | ||
| // | ||
| // Thus none of the following ACK ranges will acknowledge packets in | ||
| // `after_acked_range`. Let's put those back early. |
There was a problem hiding this comment.
Because you are relying on this, you should therefore make some assertions to guarantee that it is true. debug_assert!() is fine.
There was a problem hiding this comment.
Good point. Done in d7454d0.
| } | ||
|
|
||
| // Put remaining non-acked packets back. | ||
| self.packets.extend(packets); |
There was a problem hiding this comment.
Does this need to be changed to the following as well?
if self.packets.is_empty() {
self.packets = after_acked_range;
} else {
self.packets.extend(after_acked_range);
}If the first range is contiguous with the end, then this might be faster. It's not clear to me whether the additional condition is worth it though.
There was a problem hiding this comment.
Can you think of a scenario where:
- the range is contiguous with the end
- AND the number of packets before the range (here
packets) is high?
I can only think of the following contrived scenario. Say that we have 1_000 packets in flight. Say that we receive an ACK range for packet 990 - 1_000, thus packets would be very large and self.packets.extend(packets) costly. But I would argue that in such case, self.packets.extend(packets) is the least important problem we have to worry about.
The above aside, I assume packets will always be small, thus self.packets = packets would not be much faster than self.packets.extend(packets), and thus I don't think the above conditional on self.packets.is_empty() is worth it.
Agreed @martinthomson?
There was a problem hiding this comment.
Yeah, that's worth noting. A brief note to that effect might be good.
Co-authored-by: Martin Thomson <[email protected]> Signed-off-by: Max Inden <[email protected]>
Co-authored-by: Martin Thomson <[email protected]> Signed-off-by: Max Inden <[email protected]>
| // <https://www.rfc-editor.org/rfc/rfc9000.html#section-19.3.1> | ||
| if let Some(prev_start) = previous_range_start { | ||
| debug_assert!( | ||
| *range.end() < prev_start, |
There was a problem hiding this comment.
Given that ACK ranges are encoded on the wire relative to the previous range they can not overlap.
Given that range is inclusive, its length can not be zero.
Thus we need a < and not <=.
Please correct me if I am missing something.
There was a problem hiding this comment.
Would debug_assert!(prev_start.is_none_or(|s| s > *range.end())) work better?
I don't think that you need a big print statement: it will never happen, except in a test.
| // <https://www.rfc-editor.org/rfc/rfc9000.html#section-19.3.1> | ||
| if let Some(prev_start) = previous_range_start { | ||
| debug_assert!( | ||
| *range.end() < prev_start, |
There was a problem hiding this comment.
Would debug_assert!(prev_start.is_none_or(|s| s > *range.end())) work better?
I don't think that you need a big print statement: it will never happen, except in a test.
| } | ||
|
|
||
| // Put remaining non-acked packets back. |
There was a problem hiding this comment.
| // Put remaining non-acked packets back. | |
| // Put remaining non-acked packets back. | |
| // This is inefficient if there the acknowledged packets include the last | |
| // sent packet AND there is a large unacknowledged span of packets. | |
| // That's rare enough that we won't do anything special for that case. |
SentPacketskeep track of all inflight packets. On receiving an ACK, the ackedpacket ranges are removed via
SentPackets:take_ranges.In normal network scenarios one can assume that the amount of packets before a
range is small (e.g. due to reordering) or zero, while the amount of packets
after a range is large, i.e. all remaining in-flight packets.
The previous implementation assumed the opposite, leading to an overhead linear
to the amount of packets after the range. This commit changes the algorithm such
that it is linear to the amount of packets before the range, which again, is
assumed to be smaller than the amount of packets after the acked range.
Fixes #2242.