fix: Use 1MB socket TX and RX buffers #2470
Conversation
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 #2470 +/- ##
==========================================
- Coverage 95.42% 95.41% -0.02%
==========================================
Files 115 115
Lines 36996 36996
Branches 36996 36996
==========================================
- Hits 35305 35301 -4
- Misses 1687 1689 +2
- Partials 4 6 +2
|
Failed Interop TestsQUIC Interop Runner, client vs. server, differences relative to 06e42ce. 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 3b8fac0. 1-conn/1-100mb-resp/mtu-1504 (aka. Download)/client: No change in performance detected. time: [708.93 ms 713.89 ms 718.86 ms]
thrpt: [139.11 MiB/s 140.08 MiB/s 141.06 MiB/s]
change:
time: [-1.0741% -0.0989% +0.8670%] (p = 0.84 > 0.05)
thrpt: [-0.8595% +0.0990% +1.0858%]
1-conn/10_000-parallel-1b-resp/mtu-1504 (aka. RPS)/client: No change in performance detected. time: [348.19 ms 349.95 ms 351.73 ms]
thrpt: [28.431 Kelem/s 28.575 Kelem/s 28.720 Kelem/s]
change:
time: [-0.0649% +0.6197% +1.3119%] (p = 0.08 > 0.05)
thrpt: [-1.2949% -0.6159% +0.0649%]
1-conn/1-1b-resp/mtu-1504 (aka. HPS)/client: No change in performance detected. time: [25.083 ms 25.257 ms 25.437 ms]
thrpt: [39.313 elem/s 39.593 elem/s 39.867 elem/s]
change:
time: [-0.4792% +0.4111% +1.3877%] (p = 0.38 > 0.05)
thrpt: [-1.3687% -0.4094% +0.4815%]
1-conn/1-100mb-resp/mtu-1504 (aka. Upload)/client: 💚 Performance has improved. time: [1.7304 s 1.7504 s 1.7709 s]
thrpt: [56.469 MiB/s 57.131 MiB/s 57.789 MiB/s]
change:
time: [-8.5830% -7.1515% -5.7332%] (p = 0.00 < 0.05)
thrpt: [+6.0819% +7.7023% +9.3888%]
decode 4096 bytes, mask ff: No change in performance detected. time: [11.999 µs 12.030 µs 12.069 µs]
change: [-0.1780% +0.3925% +0.9310%] (p = 0.20 > 0.05)
decode 1048576 bytes, mask ff: No change in performance detected. time: [3.0560 ms 3.0655 ms 3.0766 ms]
change: [-0.8493% -0.3092% +0.1956%] (p = 0.25 > 0.05)
decode 4096 bytes, mask 7f: No change in performance detected. time: [20.053 µs 20.116 µs 20.181 µs]
change: [-0.4139% +0.0553% +0.5256%] (p = 0.82 > 0.05)
decode 1048576 bytes, mask 7f: No change in performance detected. time: [5.4030 ms 5.4148 ms 5.4282 ms]
change: [-0.4107% -0.0609% +0.2623%] (p = 0.74 > 0.05)
decode 4096 bytes, mask 3f: No change in performance detected. time: [6.3276 µs 6.3636 µs 6.4051 µs]
change: [-0.4234% +0.1483% +0.7510%] (p = 0.63 > 0.05)
decode 1048576 bytes, mask 3f: No change in performance detected. time: [2.5058 ms 2.5126 ms 2.5197 ms]
change: [-0.6469% -0.2057% +0.2352%] (p = 0.37 > 0.05)
1 streams of 1 bytes/multistream: No change in performance detected. time: [72.599 µs 72.801 µs 73.006 µs]
change: [-0.3916% +0.0643% +0.5308%] (p = 0.78 > 0.05)
1000 streams of 1 bytes/multistream: Change within noise threshold. time: [25.262 ms 25.296 ms 25.329 ms]
change: [-0.4508% -0.2630% -0.0707%] (p = 0.01 < 0.05)
10000 streams of 1 bytes/multistream: No change in performance detected. time: [1.7015 s 1.7030 s 1.7045 s]
change: [-0.0984% +0.0396% +0.1769%] (p = 0.58 > 0.05)
1 streams of 1000 bytes/multistream: No change in performance detected. time: [73.869 µs 74.916 µs 76.392 µs]
change: [-0.3969% +1.1086% +2.9998%] (p = 0.27 > 0.05)
100 streams of 1000 bytes/multistream: Change within noise threshold. time: [3.4162 ms 3.4231 ms 3.4304 ms]
change: [+0.0318% +0.3256% +0.6008%] (p = 0.03 < 0.05)
1000 streams of 1000 bytes/multistream: No change in performance detected. time: [145.36 ms 145.44 ms 145.52 ms]
change: [-0.0322% +0.0479% +0.1242%] (p = 0.24 > 0.05)
coalesce_acked_from_zero 1+1 entries: No change in performance detected. time: [94.627 ns 94.955 ns 95.292 ns]
change: [-0.3754% +0.1986% +0.7914%] (p = 0.50 > 0.05)
coalesce_acked_from_zero 3+1 entries: No change in performance detected. time: [112.66 ns 112.96 ns 113.29 ns]
change: [-0.5928% -0.1222% +0.3217%] (p = 0.61 > 0.05)
coalesce_acked_from_zero 10+1 entries: No change in performance detected. time: [112.44 ns 113.01 ns 113.67 ns]
change: [-2.2240% -0.4405% +0.7023%] (p = 0.68 > 0.05)
coalesce_acked_from_zero 1000+1 entries: No change in performance detected. time: [92.645 ns 93.074 ns 93.561 ns]
change: [-2.1394% -0.7792% +0.6364%] (p = 0.29 > 0.05)
RxStreamOrderer::inbound_frame(): Change within noise threshold. time: [116.12 ms 116.18 ms 116.25 ms]
change: [-0.1525% -0.0810% -0.0107%] (p = 0.03 < 0.05)
SentPackets::take_ranges: No change in performance detected. time: [8.2465 µs 8.4724 µs 8.6749 µs]
change: [-3.8707% +0.0989% +3.9861%] (p = 0.96 > 0.05)
transfer/pacing-false/varying-seeds: Change within noise threshold. time: [35.722 ms 35.798 ms 35.875 ms]
change: [+0.6341% +0.9371% +1.2451%] (p = 0.00 < 0.05)
transfer/pacing-true/varying-seeds: Change within noise threshold. time: [35.930 ms 35.993 ms 36.058 ms]
change: [+1.2892% +1.5334% +1.7996%] (p = 0.00 < 0.05)
transfer/pacing-false/same-seed: Change within noise threshold. time: [35.748 ms 35.802 ms 35.857 ms]
change: [+0.5413% +0.7303% +0.9397%] (p = 0.00 < 0.05)
transfer/pacing-true/same-seed: Change within noise threshold. time: [36.128 ms 36.184 ms 36.240 ms]
change: [+0.4921% +0.7012% +0.9022%] (p = 0.00 < 0.05)
Client/server transfer resultsPerformance differences relative to 3b8fac0. Transfer of 33554432 bytes over loopback, 30 runs. All unit-less numbers are in milliseconds.
|
Sounds good. Or maybe I don't understand why the benches show a performance improvement, but the runs over loopback show a regression. |
|
@mxinden where would you suggest to add this in |
|
@mxinden alternatively, I'd suggest merging this to neqo now and switching over to any |
|
@larseggert how about something along the lines of: diff --git a/quinn-udp/src/unix.rs b/quinn-udp/src/unix.rs
index c39941d5..0b5bbd21 100644
--- a/quinn-udp/src/unix.rs
+++ b/quinn-udp/src/unix.rs
@@ -257,6 +257,14 @@ impl UdpSocketState {
self.may_fragment
}
+ pub fn set_send_buffer_size(&self, bytes: usize)-> io::Result<()> {
+ todo!();
+ }
+
+ pub fn set_rec_buffer_size(&self, bytes: usize) -> io::Result<()> {
+ todo!();
+ }
+
/// Returns true if we previously got an EINVAL error from `sendmsg` syscall.
fn sendmsg_einval(&self) -> bool {
self.sendmsg_einval.load(Ordering::Relaxed)
@@ -543,7 +551,7 @@ fn recv(io: SockRef<'_>, bufs: &mut [IoSliceMut<'_>], meta: &mut [RecvMeta]) ->
Ok(1)
} |
Do we also need the equivalent for Windows? |
|
Yes. I would say the more platforms the better. |
|
@mxinden let me know if quinn-rs/quinn#2179 is what you had in mind. |
We need to do the same for `neqo-glue`. Fixes mozilla#1962
|
On my Mac, I now see: Note that the send buffer size is apparently based on the Linux (from a QNS run): Windows (from CI): https://lists.freebsd.org/pipermail/freebsd-questions/2005-February/075624.html says:
This seems to indicate that we should not do this increase on macOS and BSDs? |
|
I also don't understand why loopback benchmark performance is going down. |
|
I assume more buffer space is not always better. This reminds me of the Source Buffer Management draft. https://stuartcheshire.github.io/draft-cheshire-sbm/draft-cheshire-sbm.html |
We currently don't. Something along the following lines should work. Want to try running a benchmark on your local Mac to see whether either of them triggers? diff --git a/Cargo.lock b/Cargo.lock
index 64bfcc1f..d6b8b6a7 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -1028,6 +1028,7 @@ name = "neqo-udp"
version = "0.12.2"
dependencies = [
"cfg_aliases",
+ "libc",
"log",
"neqo-common",
"quinn-udp",
diff --git a/neqo-udp/Cargo.toml b/neqo-udp/Cargo.toml
index 4abd402a..8ebc2e83 100644
--- a/neqo-udp/Cargo.toml
+++ b/neqo-udp/Cargo.toml
@@ -19,6 +19,7 @@ workspace = true
log = { workspace = true }
neqo-common = { path = "./../neqo-common" }
quinn-udp = { workspace = true }
+libc = "0.2"
[build-dependencies]
cfg_aliases = "0.2"
diff --git a/neqo-udp/src/lib.rs b/neqo-udp/src/lib.rs
index d498f5aa..0cfe0e74 100644
--- a/neqo-udp/src/lib.rs
+++ b/neqo-udp/src/lib.rs
@@ -74,7 +74,19 @@ pub fn send_inner(
src_ip: None,
};
- state.try_send(socket, &transmit)?;
+ if let Err(e) = state.try_send(socket, &transmit) {
+ match e.raw_os_error() {
+ Some(libc::ENOBUFS) => {
+ todo!();
+ }
+ Some(libc::EAGAIN) => {
+ todo!();
+ }
+ Some(_) | None => {}
+ }
+
+ return Err(e);
+ }
qtrace!(
"sent {} bytes from {} to {}",Related: Neither of them are currently exposed through |
It's different, in that I doubt that we actually fill that larger buffer, and they do. (If we filled it, we should IMO also see a throughput increase.) |
|
OK, with the fixed bencher the erratic performance seems to be fixed 🎉 I guess we'd want this one merged to see the full impact of #1868? |
| let send_buf = state.send_buffer_size((&socket).into())?; | ||
| if send_buf < ONE_MB { | ||
| qdebug!("Increasing send buffer size from {send_buf} to {ONE_MB}"); | ||
| state.set_send_buffer_size((&socket).into(), ONE_MB)?; |
There was a problem hiding this comment.
If this fails, should we retry with a smaller value? (Also for recv_buf.)
| state.set_recv_buffer_size((&socket).into(), ONE_MB)?; | ||
| } else { | ||
| qdebug!("Default receive buffer size is {recv_buf}, not changing"); | ||
| } | ||
|
|
There was a problem hiding this comment.
Should we add telemetry/stats about which sizes were actually in use for a connection?
We need to do the same for
neqo-glue.Fixes #1962