perf: no more channels for UDP send/recv #1579
Conversation
Frando
force-pushed
the
net-less-channels
branch
from
b9721f7
to
0c96fb0
Compare
|
/netsim |
|
|
|
/netsim |
|
|
Frando
changed the title
|
/netsim |
|
|
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.
iroh-net/src/magicsock.rs
Outdated
| } else { | ||
| &self.pconn4 | ||
| }; | ||
| let n = ready!(conn.poll_send(&self.udp_state, cx, &transmits))?; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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..?
There was a problem hiding this comment.
ideally we should only error to quinn when all three options fail
There was a problem hiding this comment.
we have the same issue on recv at the moment I believe
Frando
force-pushed
the
net-less-channels
branch
2 times, most recently
from
b506be1
to
d49c8f3
Compare
|
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 We still have failures though, which I'll investigate next. |
iroh-net/src/magicsock.rs
Outdated
| } | ||
| // 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(); |
There was a problem hiding this comment.
we could use a fixed buffer, that might help
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
If this is always or frequently a single Bytes we could reach for mallvec for such issues.
Frando
force-pushed
the
net-less-channels
branch
from
c6504f1
to
d2b2a24
Compare
Frando
force-pushed
the
net-less-channels
branch
from
0374438
to
8d0009b
Compare
|
/netsim |
|
|
| udp_state: quinn_udp::UdpState, | ||
|
|
||
| // Send buffer used in `poll_send_udp` | ||
| send_buffer: parking_lot::Mutex<Vec<quinn_udp::Transmit>>, |
There was a problem hiding this comment.
nit: I think the length of this doesn't actually change over time, so we could potentially use Box<[Transmit]> instead
There was a problem hiding this comment.
@Frando I filed quinn-rs/quinn#1692 so hopefully we can get rid of this buffer entirely
There was a problem hiding this comment.
Sounds good. Let's defer to that then for now.
| @@ -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); | |||
There was a problem hiding this comment.
we should really change that number (but lets do that in a follow up)
| } | ||
|
|
||
| pub(super) fn read<T>(&self, f: impl FnOnce(&PeerMapInner) -> T) -> T { | ||
| let inner = self.inner.lock(); |
There was a problem hiding this comment.
would it be worth it to use a RwLock instead of a Mutex?
There was a problem hiding this comment.
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
## 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. --------- Co-authored-by: dignifiedquire <[email protected]> Co-authored-by: Diva M <[email protected]>
Description
Do not use channels for the hot path of sending and receiving QUIC UDP packets.
PeerMapinto aMutexso that we can access it for the address remapping directly in theAsyncUdpSocketpoll_send/poll_recvmethodspoll_recvandpoll_sendthe underlying UDP socket directly inMagicSock as AsyncUdpSocketand only forward Derp, Disco and Stun packages over channelsNotes & open questions
Change checklist