Introduce Certificate Compression Zlib Encoding/Decoding #2576
Conversation
Frosne
requested review from
KershawChang,
martinthomson,
larseggert and
mxinden
as code owners
Failed Interop TestsQUIC Interop Runner, client vs. server, differences relative to c1679d8. 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 c1679d8. 1-conn/1-100mb-resp/mtu-1504 (aka. Download)/client: No change in performance detected. time: [731.23 ms 735.37 ms 739.47 ms]
thrpt: [135.23 MiB/s 135.99 MiB/s 136.76 MiB/s]
change:
time: [-0.4332% +0.3781% +1.1959%] (p = 0.37 > 0.05)
thrpt: [-1.1818% -0.3767% +0.4351%]
1-conn/10_000-parallel-1b-resp/mtu-1504 (aka. RPS)/client: No change in performance detected. time: [351.12 ms 352.77 ms 354.43 ms]
thrpt: [28.214 Kelem/s 28.347 Kelem/s 28.480 Kelem/s]
change:
time: [-0.3657% +0.3107% +0.9876%] (p = 0.37 > 0.05)
thrpt: [-0.9779% -0.3097% +0.3671%]
1-conn/1-1b-resp/mtu-1504 (aka. HPS)/client: No change in performance detected. time: [24.952 ms 25.102 ms 25.257 ms]
thrpt: [39.594 elem/s 39.838 elem/s 40.078 elem/s]
change:
time: [-1.0446% -0.1804% +0.7256%] (p = 0.69 > 0.05)
thrpt: [-0.7204% +0.1808% +1.0557%]
1-conn/1-100mb-req/mtu-1504 (aka. Upload)/client: 💚 Performance has improved. time: [1.8023 s 1.8214 s 1.8420 s]
thrpt: [54.289 MiB/s 54.904 MiB/s 55.486 MiB/s]
change:
time: [-6.8089% -5.2273% -3.6394%] (p = 0.00 < 0.05)
thrpt: [+3.7768% +5.5157% +7.3064%]
decode 4096 bytes, mask ff: Change within noise threshold. time: [12.001 µs 12.037 µs 12.081 µs]
change: [-0.8513% -0.4616% -0.0633%] (p = 0.03 < 0.05)
decode 1048576 bytes, mask ff: 💚 Performance has improved. time: [2.9598 ms 2.9716 ms 2.9849 ms]
change: [-5.9212% -5.4027% -4.8453%] (p = 0.00 < 0.05)
decode 4096 bytes, mask 7f: Change within noise threshold. time: [20.002 µs 20.053 µs 20.110 µs]
change: [-1.1784% -0.8268% -0.4312%] (p = 0.00 < 0.05)
decode 1048576 bytes, mask 7f: 💚 Performance has improved. time: [4.7891 ms 4.8003 ms 4.8132 ms]
change: [-8.9311% -8.6384% -8.3110%] (p = 0.00 < 0.05)
decode 4096 bytes, mask 3f: 💚 Performance has improved. time: [6.3171 µs 6.3417 µs 6.3731 µs]
change: [-10.246% -9.6553% -8.8812%] (p = 0.00 < 0.05)
decode 1048576 bytes, mask 3f: 💔 Performance has regressed. time: [2.1492 ms 2.1560 ms 2.1631 ms]
change: [+19.308% +19.898% +20.467%] (p = 0.00 < 0.05)
1 streams of 1 bytes/multistream: Change within noise threshold. time: [72.303 µs 72.542 µs 72.787 µs]
change: [-1.5786% -1.1323% -0.6670%] (p = 0.00 < 0.05)
1000 streams of 1 bytes/multistream: 💔 Performance has regressed. time: [25.382 ms 25.418 ms 25.455 ms]
change: [+2.1935% +2.4147% +2.6257%] (p = 0.00 < 0.05)
10000 streams of 1 bytes/multistream: Change within noise threshold. time: [1.6991 s 1.7009 s 1.7027 s]
change: [+0.7265% +0.8839% +1.0290%] (p = 0.00 < 0.05)
1 streams of 1000 bytes/multistream: No change in performance detected. time: [73.721 µs 74.391 µs 75.507 µs]
change: [-2.6289% -0.7800% +1.1622%] (p = 0.49 > 0.05)
100 streams of 1000 bytes/multistream: No change in performance detected. time: [3.3718 ms 3.3788 ms 3.3863 ms]
change: [-0.1775% +0.1220% +0.4262%] (p = 0.42 > 0.05)
1000 streams of 1000 bytes/multistream: Change within noise threshold. time: [143.17 ms 143.24 ms 143.32 ms]
change: [+0.2676% +0.3417% +0.4145%] (p = 0.00 < 0.05)
coalesce_acked_from_zero 1+1 entries: No change in performance detected. time: [94.898 ns 95.275 ns 95.638 ns]
change: [-0.7177% -0.1690% +0.3524%] (p = 0.54 > 0.05)
coalesce_acked_from_zero 3+1 entries: No change in performance detected. time: [112.67 ns 112.95 ns 113.25 ns]
change: [-0.1580% +0.3799% +1.0512%] (p = 0.26 > 0.05)
coalesce_acked_from_zero 10+1 entries: No change in performance detected. time: [111.95 ns 112.40 ns 113.03 ns]
change: [-0.8708% -0.2912% +0.2511%] (p = 0.33 > 0.05)
coalesce_acked_from_zero 1000+1 entries: No change in performance detected. time: [93.277 ns 98.283 ns 108.86 ns]
change: [-1.0282% +1.9021% +6.8645%] (p = 0.44 > 0.05)
RxStreamOrderer::inbound_frame(): Change within noise threshold. time: [115.78 ms 115.83 ms 115.88 ms]
change: [+0.7897% +0.8501% +0.9144%] (p = 0.00 < 0.05)
SentPackets::take_ranges: No change in performance detected. time: [8.2557 µs 8.5202 µs 8.7681 µs]
change: [-4.4384% -1.7111% +1.0751%] (p = 0.24 > 0.05)
transfer/pacing-false/varying-seeds: Change within noise threshold. time: [35.900 ms 35.971 ms 36.043 ms]
change: [-1.1506% -0.8643% -0.5771%] (p = 0.00 < 0.05)
transfer/pacing-true/varying-seeds: Change within noise threshold. time: [36.846 ms 36.956 ms 37.065 ms]
change: [-1.1956% -0.7908% -0.3701%] (p = 0.00 < 0.05)
transfer/pacing-false/same-seed: Change within noise threshold. time: [35.517 ms 35.560 ms 35.602 ms]
change: [-0.9195% -0.7589% -0.5880%] (p = 0.00 < 0.05)
transfer/pacing-true/same-seed: No change in performance detected. time: [37.346 ms 37.414 ms 37.481 ms]
change: [-0.4140% -0.1642% +0.0806%] (p = 0.21 > 0.05)
Client/server transfer resultsPerformance differences relative to c1679d8. Transfer of 33554432 bytes over loopback, 30 runs. All unit-less numbers are in milliseconds.
|
| @@ -17,6 +17,7 @@ workspace = true | |||
|
|
|||
| [dependencies] | |||
| enum-map = { workspace = true } | |||
| flate2 = "1.1.1" | |||
There was a problem hiding this comment.
Could we add default-features = false and only enable those features actually needed?
| /// This asserts if no items are provided, or if any individual item is longer than | ||
| /// 255 octets in length. |
There was a problem hiding this comment.
Why does it assert rather then return an error?
| } | ||
| } | ||
|
|
||
| let encoding_zlib_name = CString::new("zlib").unwrap(); |
There was a problem hiding this comment.
| let encoding_zlib_name = CString::new("zlib").unwrap(); | |
| let encoding_zlib_name = CString::new("zlib")?; |
| @@ -17,6 +17,7 @@ workspace = true | |||
|
|
|||
| [dependencies] | |||
| enum-map = { workspace = true } | |||
| flate2 = "1.1.1" | |||
There was a problem hiding this comment.
Firefox is at a different version. Also pin in Cargo.lock.
| flate2 = "1.1.1" | |
| flate2 = "1.0.30" |
| @@ -103,6 +103,7 @@ impl Crypto { | |||
| // more configuration passed to server_enable_0rtt. | |||
| c.enable_0rtt()?; | |||
| } | |||
| agent.set_zlib_certificate_compression(false)?; | |||
There was a problem hiding this comment.
Is there a reason why we don't turn this on by default?
There was a problem hiding this comment.
I'm not sure that this is the right way to build this. In NSS, we don't include the compressor. That's so that the integration is left to the using code, especially Gecko, so that Gecko can reuse the (de)compressor that it has. Having neqo bring its own (de)compressor denies the application that flexibility.
I would prefer to see a plugin-based approach, similar to the one used by NSS.
Do I understand correctly that you suggest to provide only the interface? I.e. instead of set_zlib_certificate_compression we gonna have set_certificate_compression that takes as an input the SSLCertificateCompressionAlgorithm object? https://searchfox.org/mozilla-central/source/security/nss/lib/ssl/sslexp.h#1082 |
| encoded_bytes.as_ptr(), | ||
| encoded_bytes.len().try_into().unwrap(), | ||
| ); | ||
| return rv; |
There was a problem hiding this comment.
Did you run a linter over this?
| return rv; | |
| rv |
|
Right. I imagine you would define a trait that defines the compression interface. Then the code would hold a |
This patch: