s2n-quic icon indicating copy to clipboard operation
s2n-quic copied to clipboard

Trade Bytes for a Trait

Open cholcombe973 opened this issue 2 years ago • 6 comments
trafficstars

Problem:

Currently when sending bytes over the wire the send call takes ownership of the bytes. I have a use case where I'd like to pass in something other than Bytes like an io-uring but I can't because this is currently tied to a struct instead of a trait. tokio-uring offers a nice speed improvement but that is lost if the bytes end up being copied again before being sent. I'm wondering if it would be possible to use a trait such as AsyncRead here instead of Bytes. The upside of this would be that it allows callers more flexibility in handing over data to s2n-quic.

Solution:

Requirements / Acceptance Criteria:

Out of scope:

cholcombe973 avatar Feb 01 '23 00:02 cholcombe973

I'm not sure we can support a trait without taking ownership of the bytes or copying. Even if we allow passing a AsyncRead, we'd still end up copying it into a Bytes buffer so we can manage it internally. That or we'd have to do a Box<dyn AsyncRead>, which would require another allocation and indirection.

Ideally, tokio-uring would be passing around Bytes/BytesMut, since that's what most of the ecosystem uses for zerocopy interfaces.

If I'm missing something here let me know, though!

camshaft avatar Feb 01 '23 01:02 camshaft

Would it be possible to devise and use OwnedRead and OwnedReadBuf traits, as suggested by Nick Cameron?

These might work for the SendStream, though we'd up needing to make SendStream generic over OwnedReadBuf.

ReceiveStream seems tricky because we'd need to come up with some sort of Factory that generates the correct buffers for use.

rrichardson avatar Feb 01 '23 15:02 rrichardson

I guess the Latest work from Nick is in https://github.com/nrc/async-io-traits

And the discussion is https://rust-lang.zulipchat.com/#narrow/stream/187312-wg-async/topic/IO.20traits

I'm still trying to digest all of it. I may or may not have intelligent suggestions afterwards..

rrichardson avatar Feb 01 '23 16:02 rrichardson

The problem with any kind of trait is we extensively use Bytes internally for tx and rx stream reassembly. Making this functionality generic over a trait would drastically complicate the code and likely make overall performance worse because we wouldn't be able to take advantage of the features Bytes offers (e.g. cloning, splitting, merging, etc).

I'm also not quite sure what problem we're trying to solve here. Tokio io_uring's buffer traits are implemented for Bytes. What buffer type are you wanting to use instead? As far as zero-copy byte buffers go, I'm not sure you can do much better than Bytes already does.

camshaft avatar Feb 01 '23 17:02 camshaft

tokio-uring has a FixedBuf and FixedBufRegistry that makes it possible/convenient to use io_uring's buffer pre-registration features which can greatly improve predictability and performance.

Any disk IO is conducted through tokio-uring. So if we read from a file and want to send that over s2n-quic, we have to Bytes::copy_from_slice or similar, because Bytes has no way (yet) of taking ownership of the FixedBuf.

Making this functionality generic over a trait would drastically complicate the code and likely make overall performance worse because

There is work on an "official" OwnedBuf which offers some of the more useful features of Bytes. e.g. being able to slice-off read portions of a buffer, etc.. There is no refcounting though, unfortunately.

The OwnedBuf will even have the ability to take ownership of a 3rd party buffer and destroy (recycle) it.
This means that we could use it to wrap the FixedBuf (after some enhancements), so all ownership and trait problems are solved.

rrichardson avatar Feb 01 '23 17:02 rrichardson

Would the ability to create a Bytes instance from an external buffer help here? This has been a frequent request on the bytes crate.

https://github.com/tokio-rs/bytes/issues/437 https://github.com/tokio-rs/bytes/issues/526 https://github.com/tokio-rs/bytes/issues/571

I find the situation with a dozen optimized buffer crates quite confusing and would love it if everybody could just standardise on bytes.

rklaehn avatar Feb 03 '23 08:02 rklaehn