perf: no more channels for UDP send/recv #1579

Frando · 2023-10-05T11:03:40Z

Description

Do not use channels for the hot path of sending and receiving QUIC UDP packets.

Put PeerMap into a Mutex so that we can access it for the address remapping directly in the AsyncUdpSocket poll_send/poll_recv methods
poll_recv and poll_send the underlying UDP socket directly in MagicSock as AsyncUdpSocket and only forward Derp, Disco and Stun packages over channels

Notes & open questions

Change checklist

Self-review.
Documentation updates if relevant.
Tests if relevant.

Arqu · 2023-10-05T15:16:20Z

/netsim

github-actions · 2023-10-05T15:30:08Z

net-less-channels.37feb8d177d41507c33b391e80cf9dee1d74aae1
Perf report:

test	case	throughput_gbps	throughput_transfer
iroh_latency_20ms	1_to_1	3.15	3.70
iroh_latency_20ms	1_to_3	6.80	6.86
iroh_latency_20ms	1_to_5	8.88	8.88
iroh_latency_20ms	1_to_10	7.75	7.19
iroh_latency_20ms	2_to_2	4.59	4.72
iroh_latency_20ms	2_to_4	10.60	11.61
iroh_latency_20ms	2_to_6	12.65	13.04
iroh_latency_20ms	2_to_10	15.44	15.39
iroh	1_to_1	2.43	2.20
iroh	1_to_3	6.86	7.15
iroh	1_to_5	9.41	9.70
iroh	1_to_10	7.60	7.09
iroh	2_to_2	6.10	7.09
iroh	2_to_4	10.54	10.82
iroh	2_to_6	13.03	13.91
iroh	2_to_10	16.35	15.70
iroh_latency_200ms	1_to_1	2.71	3.05
iroh_latency_200ms	1_to_3	7.66	8.47
iroh_latency_200ms	1_to_5	8.01	8.15
iroh_latency_200ms	1_to_10	7.31	6.88
iroh_latency_200ms	2_to_2	6.07	7.08
iroh_latency_200ms	2_to_4	10.50	11.24
iroh_latency_200ms	2_to_6	13.39	13.95
iroh_latency_200ms	2_to_10	15.08	14.69

Arqu · 2023-10-05T15:52:35Z

/netsim

github-actions · 2023-10-05T16:05:58Z

net-less-channels.37feb8d177d41507c33b391e80cf9dee1d74aae1
Perf report:

test	case	throughput_gbps	throughput_transfer
iroh_latency_20ms	1_to_1	2.27	2.45
iroh_latency_20ms	1_to_3	7.00	6.74
iroh_latency_20ms	1_to_5	8.79	9.07
iroh_latency_20ms	1_to_10	8.18	7.68
iroh_latency_20ms	2_to_2	5.97	6.27
iroh_latency_20ms	2_to_4	10.20	10.32
iroh_latency_20ms	2_to_6	13.02	13.24
iroh_latency_20ms	2_to_10	14.61	14.55
iroh	1_to_1	2.95	3.37
iroh	1_to_3	6.43	6.21
iroh	1_to_5	8.20	8.38
iroh	1_to_10	7.57	7.01
iroh	2_to_2	6.16	7.17
iroh	2_to_4	11.55	13.24
iroh	2_to_6	12.41	12.83
iroh	2_to_10	16.12	16.04
iroh_latency_200ms	1_to_1	3.15	3.69
iroh_latency_200ms	1_to_3	7.19	7.31
iroh_latency_200ms	1_to_5	7.35	6.76
iroh_latency_200ms	1_to_10	7.58	7.05
iroh_latency_200ms	2_to_2	5.89	6.13
iroh_latency_200ms	2_to_4	11.23	12.17
iroh_latency_200ms	2_to_6	12.86	13.49
iroh_latency_200ms	2_to_10	15.27	15.48

Frando · 2023-10-05T18:19:50Z

/netsim

github-actions · 2023-10-05T18:36:18Z

net-less-channels.08f3eec5ffeb6254f730a08abcec5d3f3010f261
Perf report:

test	case	throughput_gbps	throughput_transfer
iroh_latency_20ms	1_to_1	4.15	3.80
iroh_latency_20ms	1_to_3	8.76	7.97
iroh_latency_20ms	1_to_5	7.35	6.63
iroh_latency_20ms	1_to_10	6.95	6.33
iroh_latency_20ms	2_to_2	8.15	7.42
iroh_latency_20ms	2_to_4	12.39	12.50
iroh_latency_20ms	2_to_6	14.70	14.15
iroh_latency_20ms	2_to_10	13.27	12.31
iroh	1_to_1	3.38	3.07
iroh	1_to_3	7.34	6.69
iroh	1_to_5	7.29	6.57
iroh	1_to_10	6.48	5.86
iroh	2_to_2	7.85	7.20
iroh	2_to_4	13.11	12.60
iroh	2_to_6	13.71	12.47
iroh	2_to_10	13.82	12.74
iroh_latency_200ms	1_to_1	3.27	2.98
iroh_latency_200ms	1_to_3	7.95	8.02
iroh_latency_200ms	1_to_5	7.35	6.64
iroh_latency_200ms	1_to_10	6.97	6.29
iroh_latency_200ms	2_to_2	8.00	7.32
iroh_latency_200ms	2_to_4	12.41	12.52
iroh_latency_200ms	2_to_6	13.46	12.61
iroh_latency_200ms	2_to_10	12.77	11.62

1:1 ported from https://github.com/quinn-rs/quinn/blob/main/bench/ for comparable results

we should remove all methods that return MutexGuards, they are too dangerous to misuse.

flub · 2023-10-06T07:28:53Z

iroh-net/src/magicsock.rs

+                    } else {
+                        &self.pconn4
+                    };
+                    let n = ready!(conn.poll_send(&self.udp_state, cx, &transmits))?;


can this ? result in a situation where there's a problem with the udp socket but the derp channel would still work however we never tried because we bailed out?

good catch, might be. made it warn log instead in the latest commit and not return.

it is a good question when we should return an error to quinn and when to fail silently..?

ideally we should only error to quinn when all three options fail

we have the same issue on recv at the moment I believe

Frando · 2023-10-09T08:43:41Z

So I looked into how quinn parses packets, and found a reliable way to make it ignore the non-quic packets (stun and disco) that we pass right through to quinn with the approach in this PR. By setting grease_quic_bit to false in the endpoint config, quinn ignores all packets that do not have the quic FIXED_BIT set to 1. So if we set the first byte of non-quic packets to 0, they are ignored directly (and without much perf overhead). I'd guess that's a fine approach to keep then (because it lets us keep the buffers without restructuring them if there's a non-quic packet inside).

We still have failures though, which I'll investigate next.

dignifiedquire · 2023-10-11T18:49:14Z

iroh-net/src/magicsock.rs

+        }
+        // TODO: This is the remaining alloc on the hot path for send.
+        // Unfortunately I don't see a way around this because we have do modify the transmits.
+        let mut transmits = transmits[..n].to_vec();


we could use a fixed buffer, that might help

Added a fixed buffer, so one alloc less. Instead there's a parking_lot::Mutex::lock but it should have no contention ever so fast.

We will still allocate when sending over Derp, because the derp actor expectes a Vec<Bytes>. Not sure if there's a way to reduce allocs here as well, likely not - would defer to followup.

If this is always or frequently a single Bytes we could reach for mallvec for such issues.

iroh-net/src/magicsock.rs

dignifiedquire · 2023-10-18T20:59:40Z

iroh-net/src/magicsock/endpoint.rs

+                // This logs "ping is too new" for each send whenever the endpoint does *not* need
+                // a ping. Pretty sure this is not a useful log, but maybe there was a reason?
+                // if !needs_ping {
+                //     debug!("ping is too new: {}ms", elapsed.as_millis());


it was useful to understand why pings would not be sent, maybe set it to trace?

Frando · 2023-10-19T06:59:12Z

/netsim

github-actions · 2023-10-19T07:12:13Z

net-less-channels.8d0009b3e2b86235eafcaea555b0186ec3555820
Perf report:

test	case	throughput_gbps	throughput_transfer
iroh_latency_20ms	1_to_1	3.89	3.58
iroh_latency_20ms	1_to_3	7.98	7.26
iroh_latency_20ms	1_to_5	7.20	6.72
iroh_latency_20ms	1_to_10	7.22	6.49
iroh_latency_20ms	2_to_2	5.79	5.27
iroh_latency_20ms	2_to_4	12.63	12.15
iroh_latency_20ms	2_to_6	15.29	13.94
iroh_latency_20ms	2_to_10	14.35	13.12
iroh	1_to_1	2.72	2.48
iroh	1_to_3	8.92	8.15
iroh	1_to_5	8.14	7.36
iroh	1_to_10	7.29	6.58
iroh	2_to_2	4.70	5.10
iroh	2_to_4	13.54	12.39
iroh	2_to_6	16.25	15.61
iroh	2_to_10	14.75	13.35
iroh_latency_200ms	1_to_1	3.05	2.76
iroh_latency_200ms	1_to_3	7.87	7.54
iroh_latency_200ms	1_to_5	8.08	7.48
iroh_latency_200ms	1_to_10	7.01	6.30
iroh_latency_200ms	2_to_2	7.58	6.94
iroh_latency_200ms	2_to_4	13.76	12.59
iroh_latency_200ms	2_to_6	14.64	13.34
iroh_latency_200ms	2_to_10	14.25	12.99

iroh-net/bench/Cargo.toml

dignifiedquire · 2023-10-19T08:36:42Z

iroh-net/src/magicsock.rs

+    udp_state: quinn_udp::UdpState,
+
+    // Send buffer used in `poll_send_udp`
+    send_buffer: parking_lot::Mutex<Vec<quinn_udp::Transmit>>,


nit: I think the length of this doesn't actually change over time, so we could potentially use Box<[Transmit]> instead

@Frando I filed quinn-rs/quinn#1692 so hopefully we can get rid of this buffer entirely

Sounds good. Let's defer to that then for now.

iroh-net/src/magicsock.rs

dignifiedquire · 2023-10-19T08:57:58Z

iroh-net/src/magicsock/endpoint.rs

@@ -260,12 +259,21 @@ impl Endpoint {
                }
            }

+            let addr = pong.from.as_socket_addr();
+            let trust_best_addr_until = pong.pong_at + Duration::from_secs(60 * 60);


we should really change that number (but lets do that in a follow up)

dignifiedquire · 2023-10-19T08:59:58Z

iroh-net/src/magicsock/endpoint.rs

+    }
+
+    pub(super) fn read<T>(&self, f: impl FnOnce(&PeerMapInner) -> T) -> T {
+        let inner = self.inner.lock();


would it be worth it to use a RwLock instead of a Mutex?

Good question, I don't know? Most accesses need mut access (especially all calls from poll_send and poll_recv). So don't think it would matter much.
This benchmark suggests a slight plus for Mutex when most accesses need write locks, but diff is minor: https://gist.github.com/Amanieu/6a4b4151b89b78224992106f9bc4374f

dignifiedquire

some small comments left, but other than, looks good

Frando force-pushed the net-less-channels branch from b9721f7 to 0c96fb0 Compare October 5, 2023 14:37

Frando changed the title ~~(wip/ignore) less channels on hot path in iroh-net~~ refactor: no more channels for UDP send/recv Oct 5, 2023

Frando added 10 commits October 6, 2023 01:51

bench: add iroh-net benchmark

35c1dff

1:1 ported from https://github.com/quinn-rs/quinn/blob/main/bench/ for comparable results

put peermap behind a mutex

c759763

we should remove all methods that return MutexGuards, they are too dangerous to misuse.

do not use channels for udp recv

5eeab77

make things work properly

fab4eca

fix

2480dbf

one less PeerMap

f91e44b

less peermap locking

824469b

no more channels for send on quic

1cc984b

fix tests

d5e86d2

cleanup

5cbfe03

flub reviewed Oct 6, 2023

View reviewed changes

fix: better error handling and logging in poll_send

d49c8f3

Frando force-pushed the net-less-channels branch 2 times, most recently from b506be1 to d49c8f3 Compare October 6, 2023 17:10

Frando added 2 commits October 7, 2023 16:21

refactor: use peermap directly for add_peer_addr

aa4a575

fix: actually ignore non-quic packets in quinn

73f5cc8

dignifiedquire reviewed Oct 11, 2023

View reviewed changes

iroh-net/src/magicsock.rs Outdated Show resolved Hide resolved

dignifiedquire reviewed Oct 11, 2023

View reviewed changes

iroh-net/src/magicsock.rs Outdated Show resolved Hide resolved

dignifiedquire reviewed Oct 11, 2023

View reviewed changes

iroh-net/src/magicsock.rs Outdated Show resolved Hide resolved

dignifiedquire reviewed Oct 11, 2023

View reviewed changes

iroh-net/src/magicsock.rs Outdated Show resolved Hide resolved

dignifiedquire reviewed Oct 18, 2023

View reviewed changes

feat: improve tracing in iroh-net

d2b2a24

Frando force-pushed the net-less-channels branch from c6504f1 to d2b2a24 Compare October 18, 2023 21:07

Frando added 3 commits October 19, 2023 00:26

fix: clippy and logs

46c4418

fix: log pong receives

2ade0c4

fix: make quinn ignore unmapped destinations

8d0009b

Frando force-pushed the net-less-channels branch from 0374438 to 8d0009b Compare October 18, 2023 23:46

bench: fix missing debug impls

f9045ef

dignifiedquire reviewed Oct 19, 2023

View reviewed changes

iroh-net/bench/Cargo.toml Outdated Show resolved Hide resolved