fix: More migration tests and related fixes #2170
Conversation
This expands the migration test quite a bit. It now tests alll the way until retirement of the original path, for both port-only (`rebinding_port`) and address-and-port (`rebinding_address_and_port`) changes. `rebinding_address_and_port` is succeeding, but `rebinding_port` is currently failing. That's because we treat it differently for some reason. If we replace `Paths::find_path_with_rebinding` with `Paths::find_path`, i.e., do proper path validation when only the port changes, the test succeeds. Leaving this out in case I'm missing something about the intent of the difference.
|
In a related QNS test, we have the problem that the client does not issue a new connection ID when the server retires the path, and then we time out because the client wont accept any more packets from the server. I was hoping to reproduce this with this test, but it's behaving differently. Sigh. |
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
|
We need to do a path challenge even if only the remote port changes.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2170 +/- ##
==========================================
- Coverage 95.39% 95.38% -0.02%
==========================================
Files 112 112
Lines 36447 36424 -23
==========================================
- Hits 34768 34742 -26
- Misses 1679 1682 +3 ☔ View full report in Codecov by Sentry. |
larseggert
changed the title
larseggert
requested review from
KershawChang,
martinthomson and
mxinden
as code owners
| let path = self.paths.find_path_with_rebinding( | ||
| let path = self.paths.find_path( |
There was a problem hiding this comment.
For others, relevant discussion happening on #2176 (review).
Benchmark resultsPerformance differences relative to 05b4af9. coalesce_acked_from_zero 1+1 entries: 💚 Performance has improved. time: [92.335 ns 92.659 ns 92.996 ns]
change: [-7.3425% -6.8272% -6.3022%] (p = 0.00 < 0.05)
coalesce_acked_from_zero 3+1 entries: 💚 Performance has improved. time: [103.04 ns 103.45 ns 103.89 ns]
change: [-13.133% -12.456% -11.850%] (p = 0.00 < 0.05)
coalesce_acked_from_zero 10+1 entries: 💚 Performance has improved. time: [102.52 ns 102.85 ns 103.29 ns]
change: [-12.535% -11.970% -11.249%] (p = 0.00 < 0.05)
coalesce_acked_from_zero 1000+1 entries: 💚 Performance has improved. time: [82.192 ns 82.304 ns 82.441 ns]
change: [-15.685% -14.862% -14.024%] (p = 0.00 < 0.05)
RxStreamOrderer::inbound_frame(): Change within noise threshold. time: [111.01 ms 111.08 ms 111.14 ms]
change: [-0.6841% -0.6080% -0.5382%] (p = 0.00 < 0.05)
transfer/pacing-false/varying-seeds: No change in performance detected. time: [26.581 ms 27.629 ms 28.700 ms]
change: [-3.2835% +2.4803% +8.5897%] (p = 0.40 > 0.05)
transfer/pacing-true/varying-seeds: 💔 Performance has regressed. time: [37.653 ms 39.376 ms 41.107 ms]
change: [+4.0220% +10.836% +18.580%] (p = 0.00 < 0.05)
transfer/pacing-false/same-seed: No change in performance detected. time: [26.583 ms 27.357 ms 28.140 ms]
change: [-0.0002% +4.2663% +8.8339%] (p = 0.05 > 0.05)
transfer/pacing-true/same-seed: No change in performance detected. time: [42.654 ms 44.385 ms 46.119 ms]
change: [-2.5686% +3.5698% +9.9124%] (p = 0.26 > 0.05)
1-conn/1-100mb-resp/mtu-1500 (aka. Download)/client: No change in performance detected. time: [862.82 ms 870.21 ms 877.87 ms]
thrpt: [113.91 MiB/s 114.91 MiB/s 115.90 MiB/s]
change:
time: [-2.2936% -0.9725% +0.3551%] (p = 0.16 > 0.05)
thrpt: [-0.3539% +0.9821% +2.3474%]
1-conn/10_000-parallel-1b-resp/mtu-1500 (aka. RPS)/client: Change within noise threshold. time: [317.07 ms 320.08 ms 323.14 ms]
thrpt: [30.947 Kelem/s 31.242 Kelem/s 31.539 Kelem/s]
change:
time: [-2.8723% -1.5638% -0.1460%] (p = 0.03 < 0.05)
thrpt: [+0.1462% +1.5887% +2.9572%]
1-conn/1-1b-resp/mtu-1500 (aka. HPS)/client: No change in performance detected. time: [33.735 ms 33.928 ms 34.134 ms]
thrpt: [29.297 elem/s 29.474 elem/s 29.643 elem/s]
change:
time: [-1.4937% -0.6399% +0.2095%] (p = 0.15 > 0.05)
thrpt: [-0.2091% +0.6440% +1.5163%]
1-conn/1-100mb-resp/mtu-1500 (aka. Upload)/client: 💔 Performance has regressed. time: [1.7960 s 1.8132 s 1.8307 s]
thrpt: [54.623 MiB/s 55.150 MiB/s 55.680 MiB/s]
change:
time: [+1.8747% +3.2103% +4.5459%] (p = 0.00 < 0.05)
thrpt: [-4.3482% -3.1105% -1.8402%]
1-conn/1-100mb-resp/mtu-65536 (aka. Download)/client: No change in performance detected. time: [111.24 ms 111.52 ms 111.80 ms]
thrpt: [894.48 MiB/s 896.70 MiB/s 898.95 MiB/s]
change:
time: [-3.2188% -0.8808% +0.5089%] (p = 0.60 > 0.05)
thrpt: [-0.5063% +0.8886% +3.3258%]
1-conn/10_000-parallel-1b-resp/mtu-65536 (aka. RPS)/client: No change in performance detected. time: [315.87 ms 319.21 ms 322.52 ms]
thrpt: [31.006 Kelem/s 31.327 Kelem/s 31.659 Kelem/s]
change:
time: [-0.5464% +0.8715% +2.3404%] (p = 0.24 > 0.05)
thrpt: [-2.2869% -0.8640% +0.5494%]
1-conn/1-1b-resp/mtu-65536 (aka. HPS)/client: No change in performance detected. time: [33.993 ms 34.191 ms 34.410 ms]
thrpt: [29.061 elem/s 29.248 elem/s 29.418 elem/s]
change:
time: [-0.6993% +0.0433% +0.7674%] (p = 0.91 > 0.05)
thrpt: [-0.7616% -0.0433% +0.7042%]
1-conn/1-100mb-resp/mtu-65536 (aka. Upload)/client: No change in performance detected. time: [268.70 ms 294.77 ms 335.48 ms]
thrpt: [298.08 MiB/s 339.25 MiB/s 372.16 MiB/s]
change:
time: [-6.0914% +5.1517% +21.201%] (p = 0.52 > 0.05)
thrpt: [-17.493% -4.8993% +6.4865%]
Client/server transfer resultsTransfer of 33554432 bytes over loopback.
|
Signed-off-by: Lars Eggert <[email protected]>
There was a problem hiding this comment.
A couple of clarifying questions. Sorry in case I am missing something obvious. None of them are blocking.
Other than the discussion in #2176 (review), this looks good to me.
| let empty_cid = seqno == CONNECTION_ID_SEQNO_EMPTY | ||
| || self | ||
| .connection_ids | ||
| .cids | ||
| .iter() | ||
| .any(|c| c.seqno == seqno && c.cid.is_empty()); |
There was a problem hiding this comment.
| let empty_cid = seqno == CONNECTION_ID_SEQNO_EMPTY | |
| || self | |
| .connection_ids | |
| .cids | |
| .iter() | |
| .any(|c| c.seqno == seqno && c.cid.is_empty()); | |
| let empty_cid = self | |
| .connection_ids | |
| .cids | |
| .iter() | |
| .any(|c| c.seqno == seqno && c.cid.is_empty()); |
Do I understand correctly, that this would be functionally the same, but less efficient?
There was a problem hiding this comment.
Yes. (I'm not in love with the CONNECTION_ID_SEQNO_EMPTY hack, esp. since we only seem to use the magic value on one end of the connection.)
There was a problem hiding this comment.
Given that self.connection_ids is likely very small, is the optimization needed? In other words, do you expect a performance impact when accepting the suggestion above?
There was a problem hiding this comment.
Not sure. I mostly kept it in so the CONNECTION_ID_SEQNO_EMPTY special case is more visible :-)
| @@ -314,6 +314,10 @@ impl ConnectionIdEntry<[u8; 16]> { | |||
| stats.new_connection_id += 1; | |||
| true | |||
| } | |||
|
|
|||
| pub fn is_empty(&self) -> bool { | |||
There was a problem hiding this comment.
| pub fn is_empty(&self) -> bool { | |
| pub fn is_zero_len(&self) -> bool { |
RFC 9000 refers to this as "zero-length Connection ID", right? How about adopting that wording here?
There was a problem hiding this comment.
We seem to calling these "empty", e.g., CONNECTION_ID_SEQNO_EMPTY so I stuck with that. (You're right that that's different from the RFC.)
| let empty_cid = seqno == CONNECTION_ID_SEQNO_EMPTY | ||
| || self | ||
| .connection_ids | ||
| .cids | ||
| .iter() | ||
| .any(|c| c.seqno == seqno && c.cid.is_empty()); |
There was a problem hiding this comment.
Given that self.connection_ids is likely very small, is the optimization needed? In other words, do you expect a performance impact when accepting the suggestion above?
This expands the migration test quite a bit. It now tests all the way until retirement of the original path, for both port-only (
rebinding_port) and address-and-port (rebinding_address_and_port) changes, with and without zero-len client CIDs.This now includes #2182 and #2176.