feat: In-place crypto #2385
feat: In-place crypto #2385
Conversation
Only in-place encryption so far, and only for the main data path. Fixes mozilla#2246 (eventually)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2385 +/- ##
=======================================
Coverage 95.29% 95.29%
=======================================
Files 114 114
Lines 36850 36887 +37
Branches 36850 36887 +37
=======================================
+ Hits 35117 35153 +36
- Misses 1727 1728 +1
Partials 6 6 ☔ View full report in Codecov by Sentry. |
Failed Interop TestsNone ❓ All resultsSucceeded Interop TestsNone ❓ Unsupported Interop TestsNone ❓ |
Benchmark resultsPerformance differences relative to 7a25920. decode 4096 bytes, mask ff: No change in performance detected. time: [11.859 µs 11.901 µs 11.949 µs]
change: [-0.2487% +0.3125% +1.0233%] (p = 0.35 > 0.05)
decode 1048576 bytes, mask ff: Change within noise threshold. time: [2.9217 ms 2.9324 ms 2.9451 ms]
change: [+0.6419% +1.1894% +1.7654%] (p = 0.00 < 0.05)
decode 4096 bytes, mask 7f: No change in performance detected. time: [19.773 µs 19.812 µs 19.859 µs]
change: [-0.5785% -0.1092% +0.3546%] (p = 0.66 > 0.05)
decode 1048576 bytes, mask 7f: Change within noise threshold. time: [5.0440 ms 5.0548 ms 5.0665 ms]
change: [-1.0334% -0.6788% -0.3454%] (p = 0.00 < 0.05)
decode 4096 bytes, mask 3f: No change in performance detected. time: [6.8936 µs 6.9181 µs 6.9500 µs]
change: [-0.5356% -0.0613% +0.3927%] (p = 0.81 > 0.05)
decode 1048576 bytes, mask 3f: No change in performance detected. time: [1.4141 ms 1.4196 ms 1.4264 ms]
change: [-0.4984% +0.0794% +0.6660%] (p = 0.84 > 0.05)
coalesce_acked_from_zero 1+1 entries: No change in performance detected. time: [98.591 ns 98.931 ns 99.275 ns]
change: [-1.1847% -0.5142% +0.0274%] (p = 0.10 > 0.05)
coalesce_acked_from_zero 3+1 entries: No change in performance detected. time: [116.49 ns 116.85 ns 117.23 ns]
change: [-0.6019% -0.1325% +0.2476%] (p = 0.57 > 0.05)
coalesce_acked_from_zero 10+1 entries: No change in performance detected. time: [116.22 ns 116.71 ns 117.28 ns]
change: [-0.5387% -0.0277% +0.4373%] (p = 0.92 > 0.05)
coalesce_acked_from_zero 1000+1 entries: No change in performance detected. time: [97.294 ns 97.432 ns 97.583 ns]
change: [-0.5025% +0.5665% +1.8769%] (p = 0.37 > 0.05)
RxStreamOrderer::inbound_frame(): Change within noise threshold. time: [111.47 ms 111.52 ms 111.56 ms]
change: [-0.1939% -0.1348% -0.0736%] (p = 0.00 < 0.05)
SentPackets::take_ranges: No change in performance detected. time: [5.3948 µs 5.5017 µs 5.6185 µs]
change: [-3.9735% -1.3343% +1.2731%] (p = 0.32 > 0.05)
transfer/pacing-false/varying-seeds: Change within noise threshold. time: [40.229 ms 40.304 ms 40.380 ms]
change: [-1.8575% -1.5939% -1.3286%] (p = 0.00 < 0.05)
transfer/pacing-true/varying-seeds: Change within noise threshold. time: [40.337 ms 40.405 ms 40.476 ms]
change: [-2.5264% -2.3114% -2.0862%] (p = 0.00 < 0.05)
transfer/pacing-false/same-seed: Change within noise threshold. time: [40.248 ms 40.301 ms 40.354 ms]
change: [-1.8674% -1.6855% -1.4966%] (p = 0.00 < 0.05)
transfer/pacing-true/same-seed: Change within noise threshold. time: [39.978 ms 40.035 ms 40.092 ms]
change: [-2.4841% -2.2905% -2.1003%] (p = 0.00 < 0.05)
1-conn/1-100mb-resp/mtu-1504 (aka. Download)/client: 💚 Performance has improved. time: [838.95 ms 847.68 ms 856.64 ms]
thrpt: [116.73 MiB/s 117.97 MiB/s 119.20 MiB/s]
change:
time: [-7.6936% -6.1620% -4.6550%] (p = 0.00 < 0.05)
thrpt: [+4.8822% +6.5666% +8.3348%]
1-conn/10_000-parallel-1b-resp/mtu-1504 (aka. RPS)/client: No change in performance detected. time: [316.24 ms 318.40 ms 320.60 ms]
thrpt: [31.191 Kelem/s 31.407 Kelem/s 31.622 Kelem/s]
change:
time: [-0.4975% +0.5384% +1.5541%] (p = 0.30 > 0.05)
thrpt: [-1.5303% -0.5355% +0.4999%]
1-conn/1-1b-resp/mtu-1504 (aka. HPS)/client: No change in performance detected. time: [34.105 ms 34.332 ms 34.573 ms]
thrpt: [28.924 elem/s 29.128 elem/s 29.321 elem/s]
change:
time: [-0.8174% -0.0060% +0.8076%] (p = 0.99 > 0.05)
thrpt: [-0.8012% +0.0060% +0.8241%]
1-conn/1-100mb-resp/mtu-1504 (aka. Upload)/client: No change in performance detected. time: [1.7018 s 1.7196 s 1.7374 s]
thrpt: [57.557 MiB/s 58.153 MiB/s 58.761 MiB/s]
change:
time: [-1.0411% +0.4555% +1.9886%] (p = 0.56 > 0.05)
thrpt: [-1.9498% -0.4535% +1.0521%]
Client/server transfer resultsTransfer of 33554432 bytes over loopback.
|
|
@mxinden when you have a moment, would you take a look at the borrow-checker issue in |
|
Took a quick look. let dcid = Self::opt(dcid_decoder.decode_cid(&mut decoder))?;
if decoder.remaining() < SAMPLE_OFFSET + SAMPLE_SIZE {
return Err(Error::InvalidPacket);
}
let header_len = decoder.offset();
return Ok((
Self {
packet_type: PacketType::Short,
dcid,
scid: None,
token: &[],
header_len,
version: None,
data,
},
&[],
&mut [],
));
I can take a deeper look and try to fix it. |
|
Thanks for the analysis! Wonder if we can make |
|
Ah, never seen this before. That would be error prone as the bytes within the range in |
|
The above described issue, namely that of diff --git a/neqo-transport/src/packet/mod.rs b/neqo-transport/src/packet/mod.rs
index 73b47bcc..779ca72b 100644
--- a/neqo-transport/src/packet/mod.rs
+++ b/neqo-transport/src/packet/mod.rs
@@ -563,7 +563,7 @@ pub struct PublicPacket<'a> {
/// The packet type.
packet_type: PacketType,
/// The recovered destination connection ID.
- dcid: ConnectionIdRef<'a>,
+ dcid: ConnectionId,
/// The source connection ID, if this is a long header packet.
scid: Option<ConnectionIdRef<'a>>,
/// Any token that is included in the packet (Retry always has a token; Initial sometimesThat leaves us with another issue, namely rustc not being able to infer that early returns of |
|
Okay, I got it. Let's take a look at /// `PublicPacket` holds information from packets that is public only. This allows for
/// processing of packets prior to decryption.
pub struct PublicPacket<'a> {
/// The packet type.
packet_type: PacketType,
/// The recovered destination connection ID.
dcid: ConnectionIdRef<'a>,
/// The source connection ID, if this is a long header packet.
scid: Option<ConnectionIdRef<'a>>,
/// Any token that is included in the packet (Retry always has a token; Initial sometimes
/// does). This is empty when there is no token.
token: &'a [u8],
/// The size of the header, not including the packet number.
header_len: usize,
/// Protocol version, if present in header.
version: Option<WireVersion>,
/// A reference to the entire packet, including the header.
data: &'a [u8],
}
This pull request introduces the following change: @@ -564,7 +574,7 @@ pub struct PublicPacket<'a> {
/// Protocol version, if present in header.
version: Option<WireVersion>,
/// A reference to the entire packet, including the header.
- data: &'a [u8],
+ data: &'a mut [u8],
}While An easy fix would be to make diff --git a/neqo-transport/src/packet/mod.rs b/neqo-transport/src/packet/mod.rs
index 73b47bcc..dc85bbd0 100644
--- a/neqo-transport/src/packet/mod.rs
+++ b/neqo-transport/src/packet/mod.rs
@@ -563,12 +563,12 @@ pub struct PublicPacket<'a> {
/// The packet type.
packet_type: PacketType,
/// The recovered destination connection ID.
- dcid: ConnectionIdRef<'a>,
+ dcid: ConnectionId,
/// The source connection ID, if this is a long header packet.
- scid: Option<ConnectionIdRef<'a>>,
+ scid: Option<ConnectionId>,
/// Any token that is included in the packet (Retry always has a token; Initial sometimes
/// does). This is empty when there is no token.
- token: &'a [u8],
+ token: Vec<u8>,
/// The size of the header, not including the packet number.
header_len: usize,
/// Protocol version, if present in header.The above, plus a couple of smaller lifetime changes resolve the borrow checker issues. I will propose a commit with my local changes. |
|
@larseggert let me know what you think of larseggert#34. Note that it only addresses the borrow-checker issues in Happy to look at the |
Only in-place encryption so far, and only for the main data path.
There is some support for in-place decryption, but I am running into borrow-checker issues, so this needs more work.
Fixes #2246 (eventually)