-
Notifications
You must be signed in to change notification settings - Fork 142
fix: Bump max_datagram_frame_size
TP to 64K
#3012
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Per the RFC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR increases the maximum datagram frame size from 1200 bytes to 64KB (65536 bytes) to align with RFC specifications for QUIC transport parameters.
- Introduces a new constant
MAX_DATAGRAM_FRAME_SIZE
set to 65536 bytes - Updates the default
datagram_size
parameter to use this new constant instead of the hardcoded 1200 value
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In favor. Also discussed with Martin in the past who suggested the same.
I guess we could reduce the amount slightly like we do in fn max_datagram_size
. That said, the sender will do this calculation dynamically anyways, I assume.
neqo/neqo-transport/src/connection/mod.rs
Lines 3805 to 3851 in 77ed715
/// Returns the current max size of a datagram that can fit into a packet. | |
/// The value will change over time depending on the encoded size of the | |
/// packet number, ack frames, etc. | |
/// | |
/// # Errors | |
/// The function returns `NotAvailable` if datagrams are not enabled. | |
/// # Panics | |
/// Basically never, because that unwrap won't fail. | |
pub fn max_datagram_size(&self) -> Res<u64> { | |
let max_dgram_size = self.quic_datagrams.remote_datagram_size(); | |
if max_dgram_size == 0 { | |
return Err(Error::NotAvailable); | |
} | |
let version = self.version(); | |
let Some((epoch, tx)) = self | |
.crypto | |
.states() | |
.select_tx(self.version, PacketNumberSpace::ApplicationData) | |
else { | |
return Err(Error::NotAvailable); | |
}; | |
let path = self.paths.primary().ok_or(Error::NotAvailable)?; | |
let mtu = path.borrow().plpmtu(); | |
let mut buffer = Vec::new(); | |
let encoder = Encoder::new_borrowed_vec(&mut buffer); | |
let (_, mut builder) = Self::build_packet_header( | |
&path.borrow(), | |
epoch, | |
encoder, | |
tx, | |
&self.address_validation, | |
version, | |
false, | |
usize::MAX, | |
); | |
_ = Self::add_packet_number( | |
&mut builder, | |
tx, | |
self.loss_recovery | |
.largest_acknowledged_pn(PacketNumberSpace::ApplicationData), | |
); | |
let data_len_possible = | |
u64::try_from(mtu.saturating_sub(tx.expansion() + builder.len() + 1))?; | |
Ok(min(data_len_possible, max_dgram_size)) | |
} |
I suggest we merge as is.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Lars Eggert <lars@eggert.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Branch | fix-max-dgram-size |
Testbed | On-prem |
Click to view all benchmark results
Benchmark | Latency | Benchmark Result nanoseconds (ns) (Result Δ%) | Upper Boundary nanoseconds (ns) (Limit %) |
---|---|---|---|
1-conn/1-100mb-req/mtu-1504 (aka. Upload)/client | 📈 view plot 🚷 view threshold | 205,870,000.00 ns(-1.49%)Baseline: 208,992,146.89 ns | 217,974,172.16 ns (94.45%) |
1-conn/1-100mb-resp/mtu-1504 (aka. Download)/client | 📈 view plot 🚷 view threshold | 199,490,000.00 ns(-1.74%)Baseline: 203,021,807.91 ns | 212,839,215.39 ns (93.73%) |
1-conn/1-1b-resp/mtu-1504 (aka. HPS)/client | 📈 view plot 🚷 view threshold | 28,602,000.00 ns(+0.66%)Baseline: 28,413,694.92 ns | 28,869,854.81 ns (99.07%) |
1-conn/10_000-parallel-1b-resp/mtu-1504 (aka. RPS)/client | 📈 view plot 🚷 view threshold | 286,820,000.00 ns(-2.69%)Baseline: 294,745,480.23 ns | 306,091,540.25 ns (93.70%) |
1-streams/each-1000-bytes/simulated-time | 📈 view plot 🚷 view threshold | 119,100,000.00 ns(+0.66%)Baseline: 118,316,101.69 ns | 120,893,216.90 ns (98.52%) |
1-streams/each-1000-bytes/wallclock-time | 📈 view plot 🚷 view threshold | 584,270.00 ns(-2.35%)Baseline: 598,312.43 ns | 623,161.41 ns (93.76%) |
1000-streams/each-1-bytes/simulated-time | 📈 view plot 🚷 view threshold | 15,000,000,000.00 ns(+0.05%)Baseline: 14,991,870,056.50 ns | 15,010,382,977.82 ns (99.93%) |
1000-streams/each-1-bytes/wallclock-time | 📈 view plot 🚷 view threshold | 13,580,000.00 ns(-4.47%)Baseline: 14,215,378.53 ns | 14,993,045.40 ns (90.58%) |
1000-streams/each-1000-bytes/simulated-time | 📈 view plot 🚷 view threshold | 19,014,000,000.00 ns(+0.55%)Baseline: 18,910,824,858.76 ns | 19,160,134,896.70 ns (99.24%) |
1000-streams/each-1000-bytes/wallclock-time | 📈 view plot 🚷 view threshold | 48,423,000.00 ns(-7.41%)Baseline: 52,295,632.77 ns | 58,750,075.96 ns (82.42%) |
RxStreamOrderer::inbound_frame() | 📈 view plot 🚷 view threshold | 107,990,000.00 ns(-1.68%)Baseline: 109,839,096.05 ns | 112,006,508.88 ns (96.41%) |
coalesce_acked_from_zero 1+1 entries | 📈 view plot 🚷 view threshold | 88.11 ns(-0.58%)Baseline: 88.63 ns | 89.31 ns (98.66%) |
coalesce_acked_from_zero 10+1 entries | 📈 view plot 🚷 view threshold | 105.61 ns(-0.46%)Baseline: 106.10 ns | 107.09 ns (98.62%) |
coalesce_acked_from_zero 1000+1 entries | 📈 view plot 🚷 view threshold | 88.92 ns(-1.01%)Baseline: 89.83 ns | 94.47 ns (94.13%) |
coalesce_acked_from_zero 3+1 entries | 📈 view plot 🚷 view threshold | 106.29 ns(-0.30%)Baseline: 106.61 ns | 107.58 ns (98.81%) |
decode 1048576 bytes, mask 3f | 📈 view plot 🚷 view threshold | 1,592,100.00 ns(-0.03%)Baseline: 1,592,540.11 ns | 1,599,563.14 ns (99.53%) |
decode 1048576 bytes, mask 7f | 📈 view plot 🚷 view threshold | 5,076,800.00 ns(+0.38%)Baseline: 5,057,420.34 ns | 5,077,496.74 ns (99.99%) |
decode 1048576 bytes, mask ff | 📈 view plot 🚷 view threshold | 3,037,000.00 ns(+0.18%)Baseline: 3,031,646.89 ns | 3,043,507.66 ns (99.79%) |
decode 4096 bytes, mask 3f | 📈 view plot 🚷 view threshold | 8,307.90 ns(+0.14%)Baseline: 8,296.68 ns | 8,344.45 ns (99.56%) |
decode 4096 bytes, mask 7f | 📈 view plot 🚷 view threshold | 20,007.00 ns(-0.00%)Baseline: 20,007.66 ns | 20,086.19 ns (99.61%) |
decode 4096 bytes, mask ff | 📈 view plot 🚷 view threshold | 11,606.00 ns(-1.07%)Baseline: 11,731.70 ns | 11,977.38 ns (96.90%) |
sent::Packets::take_ranges | 📈 view plot 🚷 view threshold | 4,551.70 ns(-4.09%)Baseline: 4,745.86 ns | 4,990.23 ns (91.21%) |
transfer/pacing-false/same-seed/simulated-time/run | 📈 view plot 🚷 view threshold | 25,710,000,000.00 ns(+1.83%)Baseline: 25,247,657,142.86 ns | 25,741,436,848.78 ns (99.88%) |
transfer/pacing-false/same-seed/wallclock-time/run | 📈 view plot 🚷 view threshold | 25,588,000.00 ns(-1.68%)Baseline: 26,024,485.71 ns | 27,081,387.58 ns (94.49%) |
transfer/pacing-false/varying-seeds/simulated-time/run | 📈 view plot 🚷 view threshold | 25,200,000,000.00 ns(+0.13%)Baseline: 25,166,891,428.57 ns | 25,211,865,728.34 ns (99.95%) |
transfer/pacing-false/varying-seeds/wallclock-time/run | 📈 view plot 🚷 view threshold | 25,666,000.00 ns(-2.28%)Baseline: 26,265,240.00 ns | 27,607,715.79 ns (92.97%) |
transfer/pacing-true/same-seed/simulated-time/run | 📈 view plot 🚷 view threshold | 25,675,000,000.00 ns(+0.28%)Baseline: 25,602,914,285.71 ns | 25,679,901,444.16 ns (99.98%) |
transfer/pacing-true/same-seed/wallclock-time/run | 📈 view plot 🚷 view threshold | 26,981,000.00 ns(-1.73%)Baseline: 27,456,108.57 ns | 28,771,169.31 ns (93.78%) |
transfer/pacing-true/varying-seeds/simulated-time/run | 📈 view plot 🚷 view threshold | 25,014,000,000.00 ns(+0.08%)Baseline: 24,994,165,714.29 ns | 25,043,764,573.22 ns (99.88%) |
transfer/pacing-true/varying-seeds/wallclock-time/run | 📈 view plot 🚷 view threshold | 25,932,000.00 ns(-3.22%)Baseline: 26,795,885.71 ns | 28,172,420.04 ns (92.05%) |
Is the failing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a fix in dgram_no_allowed
, i.e. setting the transport parameter lower for the client, such that the server's datagram leads to a protocol violation on the client side.
neqo/neqo-transport/src/connection/tests/datagram.rs
Lines 396 to 409 in b90ded6
#[test] | |
fn dgram_no_allowed() { | |
let mut client = default_client(); | |
let mut server = default_server(); | |
connect_force_idle(&mut client, &mut server); | |
let out = server | |
.test_write_frames(InsertDatagram { data: DATA_MTU }, now()) | |
.dgram() | |
.unwrap(); | |
client.process_input(out, now()); | |
assert_error(&client, &CloseReason::Transport(Error::ProtocolViolation)); | |
} |
Otherwise good to merge from my end.
Co-authored-by: Max Inden <mail@max-inden.de> Signed-off-by: Lars Eggert <lars@eggert.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
This is what the |
I am fine removing it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3012 +/- ##
==========================================
- Coverage 93.37% 93.36% -0.02%
==========================================
Files 123 124 +1
Lines 35887 35972 +85
Branches 35887 35972 +85
==========================================
+ Hits 33511 33584 +73
- Misses 1533 1543 +10
- Partials 843 845 +2
|
Failed Interop TestsQUIC Interop Runner, client vs. server, differences relative to 7334a18. 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
|
Client/server transfer resultsPerformance differences relative to 737d97e. Transfer of 33554432 bytes over loopback, min. 100 runs. All unit-less numbers are in milliseconds.
Download data for |
Benchmark resultsPerformance differences relative to 737d97e. 1-conn/1-100mb-resp/mtu-1504 (aka. Download)/client: Change within noise threshold.time: [199.25 ms 199.49 ms 199.73 ms] thrpt: [500.67 MiB/s 501.27 MiB/s 501.88 MiB/s] change: time: [−0.8261% −0.5946% −0.3607%] (p = 0.00 < 0.05) thrpt: [+0.3620% +0.5981% +0.8330%] 1-conn/10_000-parallel-1b-resp/mtu-1504 (aka. RPS)/client: No change in performance detected.time: [285.38 ms 286.82 ms 288.27 ms] thrpt: [34.690 Kelem/s 34.865 Kelem/s 35.041 Kelem/s] change: time: [−1.3455% −0.5305% +0.2985%] (p = 0.21 > 0.05) thrpt: [−0.2976% +0.5333% +1.3638%] 1-conn/1-1b-resp/mtu-1504 (aka. HPS)/client: No change in performance detected.time: [28.500 ms 28.602 ms 28.727 ms] thrpt: [34.811 B/s 34.962 B/s 35.088 B/s] change: time: [−0.4419% +0.0754% +0.6149%] (p = 0.78 > 0.05) thrpt: [−0.6111% −0.0753% +0.4439%] 1-conn/1-100mb-req/mtu-1504 (aka. Upload)/client: No change in performance detected.time: [205.48 ms 205.87 ms 206.41 ms] thrpt: [484.46 MiB/s 485.75 MiB/s 486.67 MiB/s] change: time: [−0.1615% +0.0958% +0.4015%] (p = 0.53 > 0.05) thrpt: [−0.3999% −0.0957% +0.1618%] decode 4096 bytes, mask ff: No change in performance detected.time: [11.578 µs 11.606 µs 11.640 µs] change: [−0.7535% −0.2767% +0.2375%] (p = 0.28 > 0.05) decode 1048576 bytes, mask ff: No change in performance detected.time: [3.0249 ms 3.0370 ms 3.0517 ms] change: [−0.5984% +0.0479% +0.6633%] (p = 0.88 > 0.05) decode 4096 bytes, mask 7f: No change in performance detected.time: [19.954 µs 20.007 µs 20.066 µs] change: [−0.6084% −0.0382% +0.4491%] (p = 0.90 > 0.05) decode 1048576 bytes, mask 7f: No change in performance detected.time: [5.0501 ms 5.0768 ms 5.1188 ms] change: [−0.2885% +0.3125% +1.1176%] (p = 0.48 > 0.05) decode 4096 bytes, mask 3f: No change in performance detected.time: [8.2721 µs 8.3079 µs 8.3501 µs] change: [−0.7963% −0.1396% +0.4284%] (p = 0.68 > 0.05) decode 1048576 bytes, mask 3f: No change in performance detected.time: [1.5853 ms 1.5921 ms 1.6003 ms] change: [−0.6709% −0.0363% +0.6131%] (p = 0.92 > 0.05) 1-streams/each-1000-bytes/wallclock-time: No change in performance detected.time: [581.71 µs 584.27 µs 587.26 µs] change: [−1.1716% −0.4543% +0.2297%] (p = 0.22 > 0.05) 1000-streams/each-1-bytes/wallclock-time: Change within noise threshold.time: [13.555 ms 13.580 ms 13.605 ms] change: [+0.2028% +0.4730% +0.7497%] (p = 0.00 < 0.05) 1000-streams/each-1-bytes/simulated-time: No change in performance detected.time: [14.985 s 15.000 s 15.015 s] thrpt: [66.599 B/s 66.666 B/s 66.733 B/s] change: time: [−0.0503% +0.0787% +0.2105%] (p = 0.26 > 0.05) thrpt: [−0.2101% −0.0786% +0.0503%] 1000-streams/each-1000-bytes/wallclock-time: No change in performance detected.time: [48.230 ms 48.423 ms 48.623 ms] change: [−0.0711% +0.4733% +1.0069%] (p = 0.10 > 0.05) coalesce_acked_from_zero 1+1 entries: No change in performance detected.time: [87.814 ns 88.114 ns 88.420 ns] change: [−1.4165% −0.5643% +0.1083%] (p = 0.17 > 0.05) coalesce_acked_from_zero 3+1 entries: No change in performance detected.time: [105.96 ns 106.29 ns 106.63 ns] change: [−0.2942% +0.1684% +0.6566%] (p = 0.49 > 0.05) coalesce_acked_from_zero 10+1 entries: No change in performance detected.time: [105.18 ns 105.61 ns 106.17 ns] change: [−0.5481% −0.0412% +0.4544%] (p = 0.88 > 0.05) coalesce_acked_from_zero 1000+1 entries: No change in performance detected.time: [88.802 ns 88.925 ns 89.068 ns] change: [−0.6517% +0.0720% +0.7676%] (p = 0.85 > 0.05) RxStreamOrderer::inbound_frame(): Change within noise threshold.time: [107.81 ms 107.99 ms 108.27 ms] change: [−1.1313% −0.9590% −0.6946%] (p = 0.00 < 0.05) sent::Packets::take_ranges: No change in performance detected.time: [4.4896 µs 4.5517 µs 4.6040 µs] change: [−3.0061% −0.2807% +2.7037%] (p = 0.85 > 0.05) transfer/pacing-false/varying-seeds/wallclock-time/run: Change within noise threshold.time: [25.620 ms 25.666 ms 25.718 ms] change: [+2.2721% +2.5507% +2.8324%] (p = 0.00 < 0.05) transfer/pacing-false/varying-seeds/simulated-time/run: No change in performance detected.time: [25.162 s 25.200 s 25.238 s] thrpt: [162.29 KiB/s 162.54 KiB/s 162.79 KiB/s] change: time: [−0.1159% +0.0955% +0.2965%] (p = 0.38 > 0.05) thrpt: [−0.2956% −0.0954% +0.1160%] transfer/pacing-true/varying-seeds/wallclock-time/run: Change within noise threshold.time: [25.877 ms 25.932 ms 25.988 ms] change: [+0.0333% +0.3347% +0.6684%] (p = 0.05 < 0.05) transfer/pacing-true/varying-seeds/simulated-time/run: Change within noise threshold.time: [24.976 s 25.014 s 25.051 s] thrpt: [163.51 KiB/s 163.75 KiB/s 164.00 KiB/s] change: time: [+0.0630% +0.2864% +0.5005%] (p = 0.01 < 0.05) thrpt: [−0.4980% −0.2856% −0.0629%] transfer/pacing-false/same-seed/wallclock-time/run: Change within noise threshold.time: [25.554 ms 25.588 ms 25.639 ms] change: [+1.7580% +1.9114% +2.1233%] (p = 0.00 < 0.05) transfer/pacing-false/same-seed/simulated-time/run: No change in performance detected.time: [25.710 s 25.710 s 25.710 s] thrpt: [159.31 KiB/s 159.31 KiB/s 159.31 KiB/s] change: time: [+0.0000% +0.0000% +0.0000%] (p = NaN > 0.05) thrpt: [+0.0000% +0.0000% +0.0000%] transfer/pacing-true/same-seed/wallclock-time/run: Change within noise threshold.time: [26.948 ms 26.981 ms 27.019 ms] change: [+2.2656% +2.4820% +2.6816%] (p = 0.00 < 0.05) transfer/pacing-true/same-seed/simulated-time/run: No change in performance detected.time: [25.675 s 25.675 s 25.675 s] thrpt: [159.53 KiB/s 159.53 KiB/s 159.53 KiB/s] change: time: [+0.0000% +0.0000% +0.0000%] (p = NaN > 0.05) thrpt: [+0.0000% +0.0000% +0.0000%] Download data for |
Per the RFC.