feat: Option to turn off SNI slicing #2387
Open
+39
−4
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Wireshark can't reassemble sliced CRYPTO frames, which causes QNS tests to fail bcause it then can't parse all packets. This PR adds an option to disable SNI slicing, and we do so by default when running in QNS.
larseggert
requested review from
KershawChang,
martinthomson and
mxinden
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2387 +/- ##
=======================================
Coverage 95.29% 95.29%
=======================================
Files 114 114
Lines 36850 36868 +18
Branches 36850 36868 +18
=======================================
+ Hits 35117 35135 +18
Misses 1727 1727
Partials 6 6 ☔ View full report in Codecov by Sentry. |
Failed Interop TestsQUIC Interop Runner, client vs. server, differences relative to 108fb8d. 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
|
|
That is a bummer. Especially since this gave us a signal whether all server implementations supported the sni-slicing feature. Do you see this being an issue? Is this temporary? |
Benchmark resultsPerformance differences relative to d62557f. decode 4096 bytes, mask ff: No change in performance detected. time: [11.858 µs 11.900 µs 11.946 µs]
change: [-0.0408% +0.5494% +1.3108%] (p = 0.13 > 0.05)
decode 1048576 bytes, mask ff: No change in performance detected. time: [2.8848 ms 2.8934 ms 2.9035 ms]
change: [-0.5520% -0.0848% +0.4000%] (p = 0.74 > 0.05)
decode 4096 bytes, mask 7f: No change in performance detected. time: [19.797 µs 19.854 µs 19.915 µs]
change: [-0.1237% +0.2410% +0.6058%] (p = 0.21 > 0.05)
decode 1048576 bytes, mask 7f: No change in performance detected. time: [5.0845 ms 5.0994 ms 5.1158 ms]
change: [-0.0822% +0.2983% +0.7117%] (p = 0.14 > 0.05)
decode 4096 bytes, mask 3f: No change in performance detected. time: [6.9166 µs 6.9549 µs 6.9980 µs]
change: [-0.0017% +0.7725% +1.6043%] (p = 0.07 > 0.05)
decode 1048576 bytes, mask 3f: No change in performance detected. time: [1.4125 ms 1.4167 ms 1.4222 ms]
change: [-0.5985% -0.1034% +0.3853%] (p = 0.69 > 0.05)
coalesce_acked_from_zero 1+1 entries: No change in performance detected. time: [98.721 ns 99.055 ns 99.428 ns]
change: [-0.5476% +0.0165% +0.5467%] (p = 0.95 > 0.05)
coalesce_acked_from_zero 3+1 entries: No change in performance detected. time: [116.81 ns 117.25 ns 117.75 ns]
change: [-0.3519% +0.1820% +0.7728%] (p = 0.54 > 0.05)
coalesce_acked_from_zero 10+1 entries: No change in performance detected. time: [116.25 ns 116.61 ns 117.06 ns]
change: [-0.4086% +0.1397% +0.6707%] (p = 0.63 > 0.05)
coalesce_acked_from_zero 1000+1 entries: No change in performance detected. time: [97.446 ns 97.596 ns 97.764 ns]
change: [-6.0501% -1.9018% +0.6904%] (p = 0.44 > 0.05)
RxStreamOrderer::inbound_frame(): Change within noise threshold. time: [111.48 ms 111.52 ms 111.57 ms]
change: [-0.1906% -0.1231% -0.0565%] (p = 0.00 < 0.05)
SentPackets::take_ranges: No change in performance detected. time: [5.4597 µs 5.6370 µs 5.8268 µs]
change: [-2.4041% +0.3028% +3.0465%] (p = 0.82 > 0.05)
transfer/pacing-false/varying-seeds: Change within noise threshold. time: [40.978 ms 41.058 ms 41.138 ms]
change: [-2.9466% -2.6762% -2.4098%] (p = 0.00 < 0.05)
transfer/pacing-true/varying-seeds: Change within noise threshold. time: [41.157 ms 41.230 ms 41.304 ms]
change: [-2.9183% -2.6784% -2.4073%] (p = 0.00 < 0.05)
transfer/pacing-false/same-seed: Change within noise threshold. time: [40.732 ms 40.795 ms 40.860 ms]
change: [-2.3883% -2.1893% -1.9696%] (p = 0.00 < 0.05)
transfer/pacing-true/same-seed: Change within noise threshold. time: [41.327 ms 41.381 ms 41.435 ms]
change: [-1.5395% -1.3447% -1.1595%] (p = 0.00 < 0.05)
1-conn/1-100mb-resp/mtu-1504 (aka. Download)/client: 💚 Performance has improved. time: [881.29 ms 892.25 ms 903.51 ms]
thrpt: [110.68 MiB/s 112.08 MiB/s 113.47 MiB/s]
change:
time: [-8.1148% -6.5662% -5.0395%] (p = 0.00 < 0.05)
thrpt: [+5.3070% +7.0276% +8.8314%]
1-conn/10_000-parallel-1b-resp/mtu-1504 (aka. RPS)/client: No change in performance detected. time: [317.71 ms 319.74 ms 321.77 ms]
thrpt: [31.078 Kelem/s 31.276 Kelem/s 31.476 Kelem/s]
change:
time: [-0.7001% +0.2225% +1.1917%] (p = 0.64 > 0.05)
thrpt: [-1.1777% -0.2220% +0.7050%]
1-conn/1-1b-resp/mtu-1504 (aka. HPS)/client: No change in performance detected. time: [34.303 ms 34.480 ms 34.671 ms]
thrpt: [28.843 elem/s 29.003 elem/s 29.152 elem/s]
change:
time: [-1.3756% -0.5077% +0.3429%] (p = 0.25 > 0.05)
thrpt: [-0.3417% +0.5103% +1.3948%]
1-conn/1-100mb-resp/mtu-1504 (aka. Upload)/client: Change within noise threshold. time: [1.7319 s 1.7475 s 1.7630 s]
thrpt: [56.723 MiB/s 57.226 MiB/s 57.741 MiB/s]
change:
time: [+0.5537% +1.9774% +3.2893%] (p = 0.00 < 0.05)
thrpt: [-3.1845% -1.9390% -0.5506%]
Client/server transfer resultsTransfer of 33554432 bytes over loopback.
|
|
I'd be very surprised if major servers wouldn't support it. We can file an issue with wireshark, but it's also kind of a feature when our connections can't be parsed :-) |
Haha, true. |
mxinden
approved these changes
Jan 23, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Wireshark can't reassemble sliced CRYPTO frames, which causes QNS tests to fail bcause it then can't parse all packets.
This PR adds an option to disable SNI slicing, and we do so by default when running in QNS.