Should Payload data be `Buf` instead of `AsRef<[u8]>`?

[seanmonstar]

The current Payload trait only requires that chunks be able to represent a slice of bytes. There may be cases where a user has several non-contiguous slices they would like to send. The Buf trait makes it possible to represent those as a single chunk, without having to copy to a single contiguous array. Additionally, AsyncWrite::write_buf can use that knowledge to call writev.

Currently, in that situation, a user must either copy all the data into 1 buffer before sending to hyper, or send multiple chunks. hyper will by default not merge the chunks, and try to use writev itself, so users can already benefit. However, that benefit may not be obvious. I'm wondering if allowing the chunk to be a Buf itself would be more obvious.

Some potential downsides: it's a little more complicated to implement a custom Buf than a AsRef<[u8]>, though the common types already do. Additionally, it ties API compatibility to the Buf trait, which is expect to have some breaking changes. However, hyper already publicly exposes a dependency on Bytes from the same crate, and AsyncWrite which also depends on Buf, so probably not a big deal.

/cc @scottlamb

[seanmonstar]

cc @carllerche

[carllerche]

The Buf trait is kind of like a more specialized Iterator<Item = u8>. IntoBuf is like IntoIterator. You can approximate the answer to "when should I use Buf or IntoBuf" by thinking about Iterator and IntoIterator.

Roughly speaking, structs should usually be generic over Buf. This is assuming that the struct impl only needs to iterate the Buf one time (since Buf has no seek).

"Leaf" APIs should take IntoBuf for convenience. This allows passing in &my_vec and what not. The leaf apis would immediately convert from IntoBuf -> Buf.

If one needs to iterate a buffer multiple types, then taking IntoBuf is necessary. However, IntoBuf takes self by ownership, so the strategy is to bound with where for<'a> &'a T: IntoBuf. This allows calling my_t.into_buf() multiple times (and iterate the type multiple times).

I hope this helps.

[seanmonstar]

Also cc @sfackler, I believe I've noticed you have a custom body implementation.

Some additional pros of requiring Buf is that a custom Buf is able to know when partial writes happen, and react, because of the advance function. Some examples of what things could do are: update some other flow control mechanism, free some resources, record more exact write stats, or recognize when an error occurs and know how much of a buffer had successfully been written in a drop check.

[sfackler]

I already use Bytes for my body implementation, so this'll be fine for me!

[scottlamb]

Thanks for ccing me, and sorry I'm a little late to the party.

I'm a little confused about the desired benefits. From the first comment, it sounds to me like you're confident that the pre-existing writev optimization you mentioned is working well in all the same situations, but you're afraid application writers might not know this. Is that accurate?

• If so, it seems like a shame to complicate the interface. Could documentation changes accomplish the same result? (I'd also think anyone who really cares about that will probably end up looking more closely anyway with a profiling tool / strace / etc. to see if whatever they're doing is actually working.)
• If not, what's the gap you're concerned about? I suppose maybe that there's a race between the application adding a bunch of chunks from another thread and hyper consuming them; if hyper is too fast, less gets combined?

fwiw, I mentioned on reddit the idea of a different chunk bound than AsRef<[u8]>, but I had a different set of benefits in mind. Maybe bytes::Buf is a step toward those as well, but if so the next step isn't obvious to me:

• selectively wrapping blocking around the dereferences (which is something I'd love to do). Does Buf help with this? It's not obvious to me how, but maybe there's some other work here I'm unaware of in tokio/bytes/etc. I guess it might be possible to do this with Buf (or even the existing AsRef<[u8]>) if blocking were split into start_blocking and end_blocking (the former could be called in bytes/bytes_vec, and the latter on drop), but maybe there's too error-prone an interface because it adds the potential of the calls being unbalanced.
• perhaps using sendfile instead of accessing the bytes at all, when writing directly to a socket (http or some fancy Linux thing that tells the kernel to do the outbound encryption.) (This isn't something I personally have any plans now to use, but iirc someone else was asking about it a while back.) Would bytes::Buf get some hook for this? (I'm not actually sure how it'd work anyway with the support for writev merging across chunks; I guess if you already have a non-empty IoVec might merge this chunk into, you'd need some way of deciding not do to that so you can later call sendfile instead. So a couple new methods, one to say if it wants sendfile, another to actually use sendfile.)

[seanmonstar]

confident that the pre-existing writev optimization you mentioned is working well in all the same situations

So, sort of. Say you have 2 large chunks available immediately. You can make them available in Payload::poll_data, and hyper will receive them and push them into basically a VecDeque of buffers to write, and then poll_data says NotReady (or Ready(None)), hyper will prepare a flush of its queue, using writev. So, for users that weren't really trying, hyper will already reduce the amount of copies done.

But, maybe you have a whole bunch of slices ready immediately. You could push them all individually onto poll_data, and just benefit from the internal VecDeque, but maybe you could gain a little bit of performance if it were just a single Buf (and thus, only a single poll_data call), and have some optimized way of accessing all the slices over hyper's generic VecDeque.

wrapping blocking around the dereferences

hyper doesn't actually dereference the slice, it passes it directly to AsyncWrite::write/AsyncWrite::write_buf. So, for a TcpStream, the dereference would happen in the kernel. If your application anticipates needing to block then, you could provide an AsyncRead + AsyncWrite that wraps its write calls in blocking, and hyper is none-the-wiser.

perhaps using sendfile instead of accessing the bytes at all

I think this would something about Payload, not the individual chunks, right? ... Or, perhaps not, if you needed to send multiple files, or only regions of certain files? I've been thinking about this on and off, and I think it could be done best with specialization. If it is for each Data chunk, then you could have a FileRegion trait implemented on your Data chunk, and AsyncWrite::write_buf can specialize on that trait, like how Vec::extend does.

[scottlamb]

maybe you could gain a little bit of performance if it were just a single Buf (and thus, only a single poll_data call), and have some optimized way of accessing all the slices over hyper's generic VecDeque.

Interesting, thanks. It's not clear to me at first glance that a poll_data call or the VecDeque operations would be expensive, but I'm sure you've thought about this more than I have.

wrapping blocking around the dereferences

Sorry, I was oversimplifying by partitioning the world into "my application" vs "hyper" without thinking about the details of how hyper interacts with tokio and such. But hyper is the thing my application is sending the chunks to. If I want to provide an AsyncWrite implementation on the other side that selectively uses blocking, how can I pass to it (through hyper) whether this chunk is one that needs blocking or not?

I think this would something about Payload, not the individual chunks, right? ... Or, perhaps not, if you needed to send multiple files, or only regions of certain files?

There definitely exist applications that do both those things. For example, my moonfire-nvr composes arbitrary-length .mp4 entities from metadata it assembles on-the-fly and from parts of one or more on-disk files.

I've been thinking about this on and off, and I think it could be done best with specialization.

Interesting. I'll have to study how that works. Maybe it's also the answer for my question above about blocking. Too bad specialization's still unstable, though.