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

LengthDelimitedCodec could support encoding borrowed buffers #6116

Open
GoldsteinE opened this issue Oct 29, 2023 · 2 comments
Open

LengthDelimitedCodec could support encoding borrowed buffers #6116

GoldsteinE opened this issue Oct 29, 2023 · 2 comments
Labels
A-tokio-util Area: The tokio-util crate C-feature-request Category: A feature request. M-codec Module: tokio-util/codec S-breaking-change A breaking change that requires manual coordination to be released.

Comments

@GoldsteinE
Copy link

GoldsteinE commented Oct 29, 2023

Is your feature request related to a problem? Please describe.
Consider this code:

let mut stream = FramedWrite::new(stream, LengthDelimitedCodec::new());
let mut buf = vec![0_u8; max_message_size];
loop {
    let message = gen_message(&mut buf);
    // an allocation here:
    stream.feed(Bytes::copy_from_slice(&message)).await?;
}

This allocation is unneeded. impl Encoder<Bytes> for LengthDelimitedCodec just reslices src, so impl<'a> Encoder<&'a [u8]> for LengthDelimitedCodec is just as possible.

Describe the solution you'd like
I first thought that just adding the missing impl would be enough, but unfortunately having two Encoder<_> impls breaks type inference, so this is a backwards incompatible change, oh well.

Some possible options:

  1. Make it opt-in via some kind of type tag, i.e.

    pub struct LengthDelimitedCodec<Item = Bytes> { ... } 

    All the existing methods would have to be implemented on the <Bytes> variant. <&[u8]> variant would need new methods like ::new_borrowed() or something.

  2. Make a wrapper type.

  3. Just wait for the next breaking release and break it then.

Describe alternatives you've considered

Don’t do it. Cases like the above one will continue to require unneccesary allocation, which is kind of sad, but not world-ending.

Additional context

I’m willing to send a PR if some possible path of solution is accepted.

@GoldsteinE GoldsteinE added A-tokio Area: The main tokio crate C-feature-request Category: A feature request. labels Oct 29, 2023
@GoldsteinE
Copy link
Author

GoldsteinE commented Oct 29, 2023

This commit shows how such an impl is both possible and trivial.

GoldsteinE@ecd7ab3

The only thing that makes it hard is backwards compatibility, as far as I understand.

@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 Oct 30, 2023
@Darksonn
Copy link
Contributor

Makes sense. We can add this to the list of things to fix in the next breaking release.

@Darksonn Darksonn added S-blocked-on-msrv Status: need an MSRV bump to progress S-breaking-change A breaking change that requires manual coordination to be released. and removed S-blocked-on-msrv Status: need an MSRV bump to progress labels May 11, 2024
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 S-breaking-change A breaking change that requires manual coordination to be released.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants