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

Support for non-contiguous buffers in tokio_util::codec::Encoder #4122

Open
lilyball opened this issue Sep 20, 2021 · 1 comment
Open

Support for non-contiguous buffers in tokio_util::codec::Encoder #4122

lilyball opened this issue Sep 20, 2021 · 1 comment
Labels
A-tokio-util Area: The tokio-util crate C-feature-request Category: A feature request. M-codec Module: tokio-util/codec

Comments

@lilyball
Copy link

Is your feature request related to a problem? Please describe.
tokio_util::codec::Encoder does not support zero-copy writes. It also makes it very awkward to support a variable-width length-prefixed frame when the size of the encoded data is not known prior to writing the data.

Today, the Encoder::encode() method takes a &mut BytesMut, which is a contiguous buffer. This means there's no way to add data to it without copying the data. In the variable-width length-prefixed scenario, this requires writing the data first in order to calculate the length (and therefore the size of the encoded length), so this also involves a copy (either writing to a side buffer, or writing to the BytesMut prior to shifting the data to fit the encoded length).

This is especially frustrating as the lower-level method that Framed uses to actually write out the data works with non-contiguous buffers, but Framed does not take advantage of that.

Describe the solution you'd like
Change the type from BytesMut to something that supports non-contiguous buffers. I don't know offhand what existing type would work for this, but it's something that should support the same basic functionality of BytesMut while also allowing for non-contiguous buffers.

From the perspective of Framed, once the data has been written by the Encoder impl, Framed really only cares that it implements Buf. So this new type could perhaps support appending T where T: Buf + 'static, thus allowing it to take ownership of arbitrary chunks of data.

As for the variable-width length-prefixed scenario, where it makes sense to write into the existing allocated space if there is enough, what we'd like is to be able to write into the existing allocated space, then insert the length prefix before the written data. There are multiple possible APIs here, and I'm not sure what the best one is, but ultimately I want to write into the allocated space and then write the length without copying data unnecessarily. The buffer type could support various operations that split its data into separate chunks (without copying data) in order to insert or reorder them.

Incidentally, this would also help tokio_util::codec::length_delimited::LengthDelimitedCodec, which has to copy the data from the Bytes item into the buffer, but with this new approach it would just append it as a chunk.

Describe alternatives you've considered
Instead of a new type, we could try changing the encode() API such that instead of being given a &mut BytesMut, it could be given a BytesMut and required to return a Result<Self::Buffer, Self::Error> where Self::Buffer is something like type Buffer: Buf and the documentation can suggest setting it to BytesMut in the simple case (existing encoders can just return the input BytesMut).

This approach also allows for preventing Encoders from altering previously-written data. The trait can instead guarantee that the input BytesMut has a len() == 0 (it being provided mostly as a container for existing allocated space). And it can be created by calling buffer.split_off(buffer.len()).

An upside here is when the buffer has to be extended, it doesn't have to copy all of the previously-written data, only the new frame.

A downside here is if the Encoder impl doesn't return the input BytesMut, then any additional reserved capacity in it will be thrown away.

Additional context
AsyncWrite gained vectored writes in #3149, it's very annoying that tokio_util::codec doesn't support this.

@lilyball lilyball added A-tokio Area: The main tokio crate C-feature-request Category: A feature request. labels Sep 20, 2021
@Darksonn Darksonn added A-tokio-util Area: The tokio-util crate M-codec Module: tokio-util/codec and removed A-tokio Area: The main tokio crate labels Sep 21, 2021
@Darksonn
Copy link
Contributor

While something like this could make sense, it seems like it would be an entirely new API rather than a change to the codec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio-util Area: The tokio-util crate C-feature-request Category: A feature request. M-codec Module: tokio-util/codec
Projects
None yet
Development

No branches or pull requests

2 participants