Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trade Bytes for a Trait #1619

Open
cholcombe973 opened this issue Feb 1, 2023 · 6 comments
Open

Trade Bytes for a Trait #1619

cholcombe973 opened this issue Feb 1, 2023 · 6 comments

Comments

@cholcombe973
Copy link

cholcombe973 commented Feb 1, 2023

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:

@camshaft
Copy link
Contributor

camshaft commented Feb 1, 2023

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!

@rrichardson
Copy link
Contributor

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
Copy link
Contributor

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..

@camshaft
Copy link
Contributor

camshaft commented Feb 1, 2023

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.

@rrichardson
Copy link
Contributor

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.

@rklaehn
Copy link

rklaehn commented Feb 3, 2023

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

tokio-rs/bytes#437
tokio-rs/bytes#526
tokio-rs/bytes#571

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants