feat: Randomize CI packet number #2499
Conversation
WIP. Two potential issues: 1. Did I mess up the validation in `CryptoDxState::continuation()`? 2. See the `FIXME` in `Version::confirm_version()`. Fixes mozilla#2462 CC @omansfeld
Failed Interop TestsQUIC Interop Runner, client vs. server, differences relative to eb3e2a6. 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
|
Benchmark resultsPerformance differences relative to eb3e2a6. decode 4096 bytes, mask ff: No change in performance detected. time: [11.865 µs 11.911 µs 11.962 µs]
change: [+0.0023% +0.3730% +0.7267%] (p = 0.05 > 0.05)
decode 1048576 bytes, mask ff: No change in performance detected. time: [3.0668 ms 3.0760 ms 3.0868 ms]
change: [-0.2914% +0.1283% +0.5644%] (p = 0.55 > 0.05)
decode 4096 bytes, mask 7f: No change in performance detected. time: [19.767 µs 19.813 µs 19.865 µs]
change: [-0.2894% +0.1264% +0.6193%] (p = 0.60 > 0.05)
decode 1048576 bytes, mask 7f: No change in performance detected. time: [5.1412 ms 5.1520 ms 5.1634 ms]
change: [-0.3776% -0.0581% +0.2623%] (p = 0.73 > 0.05)
decode 4096 bytes, mask 3f: No change in performance detected. time: [6.8760 µs 6.9108 µs 6.9562 µs]
change: [-0.1045% +0.3173% +0.8251%] (p = 0.19 > 0.05)
decode 1048576 bytes, mask 3f: No change in performance detected. time: [1.7551 ms 1.7607 ms 1.7676 ms]
change: [-0.4074% +0.0636% +0.6146%] (p = 0.86 > 0.05)
1 streams of 1 bytes/multistream: No change in performance detected. time: [68.798 µs 69.360 µs 70.371 µs]
change: [-1.2242% +0.8016% +2.9149%] (p = 0.53 > 0.05)
1000 streams of 1 bytes/multistream: Change within noise threshold. time: [24.728 ms 24.767 ms 24.806 ms]
change: [+0.7068% +0.9091% +1.1247%] (p = 0.00 < 0.05)
10000 streams of 1 bytes/multistream: 💔 Performance has regressed. time: [1.6641 s 1.6660 s 1.6679 s]
change: [+1.0095% +1.1630% +1.3195%] (p = 0.00 < 0.05)
1 streams of 1000 bytes/multistream: No change in performance detected. time: [70.279 µs 70.851 µs 71.889 µs]
change: [-3.7524% -1.1530% +1.1388%] (p = 0.43 > 0.05)
100 streams of 1000 bytes/multistream: 💔 Performance has regressed. time: [3.3342 ms 3.3406 ms 3.3474 ms]
change: [+3.5920% +3.9093% +4.2248%] (p = 0.00 < 0.05)
1000 streams of 1000 bytes/multistream: 💔 Performance has regressed. time: [145.70 ms 145.78 ms 145.88 ms]
change: [+6.2013% +6.2904% +6.3845%] (p = 0.00 < 0.05)
coalesce_acked_from_zero 1+1 entries: No change in performance detected. time: [92.503 ns 92.815 ns 93.133 ns]
change: [-0.1145% +2.4447% +8.9192%] (p = 0.30 > 0.05)
coalesce_acked_from_zero 3+1 entries: No change in performance detected. time: [110.04 ns 110.34 ns 110.68 ns]
change: [-0.0131% +1.5025% +4.2996%] (p = 0.23 > 0.05)
coalesce_acked_from_zero 10+1 entries: No change in performance detected. time: [110.19 ns 110.77 ns 111.43 ns]
change: [-0.0848% +1.3116% +3.4977%] (p = 0.18 > 0.05)
coalesce_acked_from_zero 1000+1 entries: No change in performance detected. time: [91.468 ns 91.504 ns 91.545 ns]
change: [-1.2043% -0.1532% +0.8703%] (p = 0.79 > 0.05)
RxStreamOrderer::inbound_frame(): Change within noise threshold. time: [115.32 ms 115.37 ms 115.41 ms]
change: [-0.1802% -0.1216% -0.0643%] (p = 0.00 < 0.05)
SentPackets::take_ranges: No change in performance detected. time: [5.3141 µs 5.4993 µs 5.6994 µs]
change: [-16.718% -4.6892% +4.2003%] (p = 0.60 > 0.05)
transfer/pacing-false/varying-seeds: Change within noise threshold. time: [34.386 ms 34.451 ms 34.517 ms]
change: [-1.0133% -0.7358% -0.4605%] (p = 0.00 < 0.05)
transfer/pacing-true/varying-seeds: Change within noise threshold. time: [34.410 ms 34.469 ms 34.527 ms]
change: [-1.0390% -0.7966% -0.5473%] (p = 0.00 < 0.05)
transfer/pacing-false/same-seed: Change within noise threshold. time: [34.795 ms 34.856 ms 34.916 ms]
change: [+0.3264% +0.5567% +0.7844%] (p = 0.00 < 0.05)
transfer/pacing-true/same-seed: Change within noise threshold. time: [34.547 ms 34.602 ms 34.657 ms]
change: [-2.0059% -1.7627% -1.5087%] (p = 0.00 < 0.05)
1-conn/1-100mb-resp/mtu-1504 (aka. Download)/client: Change within noise threshold. time: [2.1900 s 2.1975 s 2.2053 s]
thrpt: [45.345 MiB/s 45.505 MiB/s 45.663 MiB/s]
change:
time: [-1.6458% -1.0290% -0.4638%] (p = 0.00 < 0.05)
thrpt: [+0.4660% +1.0397% +1.6733%]
1-conn/10_000-parallel-1b-resp/mtu-1504 (aka. RPS)/client: No change in performance detected. time: [387.29 ms 389.31 ms 391.36 ms]
thrpt: [25.552 Kelem/s 25.686 Kelem/s 25.821 Kelem/s]
change:
time: [-0.9330% -0.1693% +0.6008%] (p = 0.66 > 0.05)
thrpt: [-0.5972% +0.1696% +0.9418%]
1-conn/1-1b-resp/mtu-1504 (aka. HPS)/client: 💔 Performance has regressed. time: [29.233 ms 30.064 ms 30.882 ms]
thrpt: [32.382 elem/s 33.263 elem/s 34.208 elem/s]
change:
time: [+1.6212% +5.5274% +9.5398%] (p = 0.01 < 0.05)
thrpt: [-8.7090% -5.2379% -1.5953%]
1-conn/1-100mb-resp/mtu-1504 (aka. Upload)/client: 💔 Performance has regressed. time: [3.5594 s 3.5820 s 3.6069 s]
thrpt: [27.725 MiB/s 27.918 MiB/s 28.095 MiB/s]
change:
time: [+10.109% +11.075% +12.085%] (p = 0.00 < 0.05)
thrpt: [-10.782% -9.9709% -9.1805%]
Client/server transfer resultsPerformance differences relative to eb3e2a6. Transfer of 33554432 bytes over loopback, 30 runs. All unit-less numbers are in milliseconds.
|
| prev.used_pn.end, | ||
| ); | ||
| Err(Error::PacketNumberOverlap) | ||
| } else { | ||
| self.used_pn.start = next; | ||
| self.used_pn = next..max(next, self.used_pn.end); | ||
| Ok(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Edit: Sorry, this was supposed to be for the whole continuation function, I made a mistake highlighting the code block for commenting apparently
I don't understand why this function is being changed and how that relates to the initial packet number being non-zero. Maybe it has something to do with #2462 (comment)?
Per line 585f next == prev.used_pn.end == self.min_pn so aren't we effectively evaluating
} else if prev.used_pn.end > prev.used_pn.end {
...
}in line 590? Which can never be true and is also logically very different from what it was before (comparing to self.used_pn.start).
I also don't quite understand the debug message (line 592), because aren't we checking for an already used packet number in self here? To me "too new packet number" would mean it's too high but the expression checks for it being too low.
There was a problem hiding this comment.
This PR was a quick hack. Take it more as a pointer in the direction that you might want to take.
There was a problem hiding this comment.
Ah, misunderstood, thanks! I'll re-add it to my to-do's.
WIP. Two potential issues:
CryptoDxState::continuation()?FIXMEinVersion::confirm_version().Fixes #2462
CC @omansfeld