feat: Option to turn off SNI slicing #2387
Merged
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.31% +0.01%
==========================================
Files 114 114
Lines 36850 36868 +18
Branches 36850 36868 +18
==========================================
+ Hits 35117 35139 +22
+ Misses 1727 1725 -2
+ Partials 6 4 -2 ☔ 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.845 µs 11.882 µs 11.925 µs]
change: [-0.2198% +0.5172% +1.4384%] (p = 0.34 > 0.05)
decode 1048576 bytes, mask ff: No change in performance detected. time: [2.8843 ms 2.8935 ms 2.9043 ms]
change: [-0.5616% -0.0814% +0.4584%] (p = 0.75 > 0.05)
decode 4096 bytes, mask 7f: No change in performance detected. time: [19.793 µs 19.848 µs 19.907 µs]
change: [+0.0344% +0.5925% +1.3451%] (p = 0.06 > 0.05)
decode 1048576 bytes, mask 7f: No change in performance detected. time: [5.0712 ms 5.0824 ms 5.0951 ms]
change: [-0.3651% -0.0370% +0.3181%] (p = 0.83 > 0.05)
decode 4096 bytes, mask 3f: No change in performance detected. time: [6.8933 µs 6.9162 µs 6.9459 µs]
change: [-0.2965% +0.1305% +0.6171%] (p = 0.60 > 0.05)
decode 1048576 bytes, mask 3f: No change in performance detected. time: [1.4127 ms 1.4168 ms 1.4223 ms]
change: [-0.5841% -0.0933% +0.3952%] (p = 0.75 > 0.05)
coalesce_acked_from_zero 1+1 entries: Change within noise threshold. time: [99.298 ns 99.601 ns 99.908 ns]
change: [-0.0313% +0.5313% +1.0143%] (p = 0.04 < 0.05)
coalesce_acked_from_zero 3+1 entries: Change within noise threshold. time: [117.70 ns 118.13 ns 118.59 ns]
change: [+0.3851% +0.8524% +1.3121%] (p = 0.00 < 0.05)
coalesce_acked_from_zero 10+1 entries: Change within noise threshold. time: [117.38 ns 117.96 ns 118.59 ns]
change: [+0.5106% +1.1788% +2.0322%] (p = 0.00 < 0.05)
coalesce_acked_from_zero 1000+1 entries: No change in performance detected. time: [98.391 ns 98.535 ns 98.711 ns]
change: [-5.0833% -0.8934% +1.7277%] (p = 0.77 > 0.05)
RxStreamOrderer::inbound_frame(): Change within noise threshold. time: [112.55 ms 112.60 ms 112.66 ms]
change: [+0.7750% +0.8463% +0.9171%] (p = 0.00 < 0.05)
SentPackets::take_ranges: No change in performance detected. time: [5.3963 µs 5.4714 µs 5.5415 µs]
change: [-3.0689% -0.6532% +1.7871%] (p = 0.60 > 0.05)
transfer/pacing-false/varying-seeds: Change within noise threshold. time: [41.265 ms 41.338 ms 41.412 ms]
change: [-2.2843% -2.0123% -1.7447%] (p = 0.00 < 0.05)
transfer/pacing-true/varying-seeds: Change within noise threshold. time: [41.325 ms 41.404 ms 41.482 ms]
change: [-2.5321% -2.2687% -2.0145%] (p = 0.00 < 0.05)
transfer/pacing-false/same-seed: Change within noise threshold. time: [41.195 ms 41.257 ms 41.320 ms]
change: [-1.2798% -1.0815% -0.8641%] (p = 0.00 < 0.05)
transfer/pacing-true/same-seed: Change within noise threshold. time: [41.251 ms 41.318 ms 41.384 ms]
change: [-1.6947% -1.4954% -1.2827%] (p = 0.00 < 0.05)
1-conn/1-100mb-resp/mtu-1504 (aka. Download)/client: 💚 Performance has improved. time: [875.13 ms 884.81 ms 894.67 ms]
thrpt: [111.77 MiB/s 113.02 MiB/s 114.27 MiB/s]
change:
time: [-8.7811% -7.3453% -5.8344%] (p = 0.00 < 0.05)
thrpt: [+6.1959% +7.9276% +9.6264%]
1-conn/10_000-parallel-1b-resp/mtu-1504 (aka. RPS)/client: No change in performance detected. time: [317.02 ms 319.29 ms 321.59 ms]
thrpt: [31.096 Kelem/s 31.320 Kelem/s 31.544 Kelem/s]
change:
time: [-0.8705% +0.0814% +1.0479%] (p = 0.87 > 0.05)
thrpt: [-1.0370% -0.0813% +0.8781%]
1-conn/1-1b-resp/mtu-1504 (aka. HPS)/client: Change within noise threshold. time: [34.065 ms 34.262 ms 34.481 ms]
thrpt: [29.001 elem/s 29.186 elem/s 29.355 elem/s]
change:
time: [-1.9991% -1.1342% -0.2006%] (p = 0.01 < 0.05)
thrpt: [+0.2010% +1.1472% +2.0399%]
1-conn/1-100mb-resp/mtu-1504 (aka. Upload)/client: No change in performance detected. time: [1.6884 s 1.7057 s 1.7233 s]
thrpt: [58.030 MiB/s 58.626 MiB/s 59.227 MiB/s]
change:
time: [-1.8273% -0.4583% +1.0378%] (p = 0.53 > 0.05)
thrpt: [-1.0272% +0.4604% +1.8613%]
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
|
@mxinden turns out we can leave slicing on for some of the tests, so we will get alerts if some servers don't deal with it well. |
|
Good news. Thanks. |
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.