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

Could BufStream be flattened? #111

Closed
aturon opened this issue Sep 14, 2018 · 6 comments
Closed

Could BufStream be flattened? #111

aturon opened this issue Sep 14, 2018 · 6 comments

Comments

@aturon
Copy link

aturon commented Sep 14, 2018

As currently defined, BufStream produces a stream of Buf values, which in turn may contain multiple [u8] slices. I was curious what the tradeoffs were around, instead, having BufStream yield AsRef<[u8]> directly, thus "flattening" these two layers. I'm pretty sure there are straightforward ways to translate in either direction, but I may be missing something.

@seanmonstar
Copy link
Collaborator

If I have a buffer and need to provide two slices into it, (say, chunked decoding found a chunked header in the middle), being able to yield the buffer directly as an impl Buf where the middle is skipped would be more efficient than cloning the two parts into 2 owned things. Something like Bytes allows that to be done cheaply, but even then, it'd be more efficient to only need to poll once instead of twice.

Additionally, Buf allows people to know how much of the buffer has been "used", via advance, in case they want to record as data is written or consumed, or do something like flow control by releasing H2 window capacity.

@aturon
Copy link
Author

aturon commented Sep 14, 2018

@seanmonstar Yeah, I was imagining using something like Bytes to deal with that problem, and was imagining the state machine for polling would have a fast path for the case where you have several slices you're trying to produce. I was also thinking this could be wrapped in a transformation from Stream<Item = impl Buf> to Stream<Item = impl AsRef<[u8]> or some such. Granted, we could (probably more easily) set things up in the reverse direction; I was mostly looking to see if the core BufStream definition could be simplified.

cc @cramertj as well

@seanmonstar
Copy link
Collaborator

In hyper 0.11, it was just a Stream<Item = impl AsRef<[u8]>>, but since additional things were wanted for 0.12, the Payload trait (which partly inspires the BufStream trait) was changed to yield impl Buf. Yielding a Buf is generally more powerful.

@aturon
Copy link
Author

aturon commented Sep 14, 2018

@seanmonstar are there discussions where I can read up about this? Would love to know more!

@seanmonstar
Copy link
Collaborator

There's not much, but I found hyperium/hyper#1508

@carllerche
Copy link
Owner

We cannot assume Bytes is being used in the leaf nodes. There are many cases in which Bytes cannot be used (off the top of my head mmap).

If the T: Buf is a rope backed by mmaped chunks, I don't know how you would be able to impl a BufStream w/ your proposal.

Also, either way there is a trait bound, so I don't see the win.

If you want to continue this discussion, we can move to the BufStream PR here: tokio-rs/tokio#611

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

3 participants