cloudflare / quiche

🥧 Savoury implementation of the QUIC transport protocol and HTTP/3
https://docs.quic.tech/quiche/
BSD 2-Clause "Simplified" License
9.1k stars 684 forks source link

Immediate datagram sending #1618

Open colinmarc opened 9 months ago

colinmarc commented 9 months ago

I'm working on an application that sends lots and lots of datagrams (for video), and it strikes me that the current API for datagrams is not very flexible or optimized. This seems to be a result of integrating the datagram functionality into the normal send path, despite datagrams not needing to be interleaved with stream packets at all, per my lackluster understanding of QUIC.

In the worst case, I have a Bytes with some data I want to send. I call dgram_send(&buf). The packet first gets copied into the queue here:

https://github.com/cloudflare/quiche/blob/883a616f0bf3258f1898e0fad0c3707c630f81b0/quiche/src/lib.rs#L5347

(Note that there's a dgram_send_vec which doesn't copy, but it's pretty tricky to have a Vec at that point.)

Then, when I call send, I have no control over whether the datagrams are prioritized. If they are picked out of the queue, there's another copy into the packet buffer:

https://github.com/cloudflare/quiche/blob/883a616f0bf3258f1898e0fad0c3707c630f81b0/quiche/src/lib.rs#L3975-L3976

I may be misunderstanding some nuance of QUIC's internal state tracking, but for me the ideal API would be an immediate packet construction helper, similar to how retry or negotiate_version work:

/// Writes a complete datagram packet to out.
pub fn datagram(&mut self, data: &[u8], out: &mut [u8]) -> Result<usize>;

A bonus would be if it could somehow handle data and out being overlapping, to allow adding the header and encrypting in-place. I'm not familiar enough with rust's slice type to know if that's easy or not. Otherwise it would be another fn:

/// Writes a datagram header to the provider buffer and encrypts
/// in-place. The offset indicated by `data_off` must be greater than
/// XXX to allow space for the header. The resulting tuple indicates
/// the begin and end offsets of the completed packet.
pub fn datagram_inplace(&mut self, buf: &mut [u8], data_off: usize) -> Result<(usize, usize)>;

Thanks for reading! I'm happy to open a PR for this if the API seems workable.

LPardue commented 9 months ago

Thanks for the questions / proposal.

We've talked in the past about optimizing the send path for datagrams, primarily around reducing copies. The idea was loosely to use traits and pass those in rather than the actual data, then can the framing and packetization can happen later during quiche's regular send().

The suggestion to support a more immediate mode of packet creation is interesting. But it adds yet another thing for apps to have to worry about. In addition it also complicates the model we have around congestion control and packet pacing. That's not something retry or version negotiation need to care about. Reducing the efficacy of the congestion controller can undo any perf enhancements that reducing copies might achieve.

The trait approach aligns with our current model and is probably minor API change. Maybe your interested in trying that out?

colinmarc commented 9 months ago

Hi, thanks for the quick response!

For the trait, would Deref<Target=[u8]> be right? That would support Bytes and smart pointers like Arc<[u8]>, as well as Vec. That would only eliminate one of the copies, but that's better than nothing.

colinmarc commented 9 months ago

Thinking about this some more, I wonder if it would be better to imitate Buf from the bytes package, or support it directly under a feature flag. The type is sort of an "infallible read with length", and can handle things like a chain of byte slices.