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

Allow Header to borrow from the header value #59

Open
Marwes opened this issue Dec 12, 2019 · 2 comments
Open

Allow Header to borrow from the header value #59

Marwes opened this issue Dec 12, 2019 · 2 comments

Comments

@Marwes
Copy link
Contributor

Marwes commented Dec 12, 2019

Currently all typed headers must copy and allocate their own storage for any string data it needs to store. This seems a bit wasteful in common headers such as Cookie where we should really just be able to borrow the existing value instead of copying the entire thing.

I'd like to propose a small change to the Header trait such that it takes a lifetime which allows the decode method to be implemented in a way that borrows the HeaderValues taken as argument.

pub trait Header<'value> {
    /// The name of this header.
    fn name() -> &'static HeaderName;

    /// Decode this type from an iterator of `HeaderValue`s.
    fn decode<I>(values: &mut I) -> Result<Self, Error>
    where
        Self: Sized,
        I: Iterator<Item = &'value HeaderValue>;

    /// Encode this type to a `HeaderMap`.
    ///
    /// This function should be infallible. Any errors converting to a
    /// `HeaderValue` should have been caught when parsing or constructing
    /// this value.
    fn encode<E: Extend<HeaderValue>>(&self, values: &mut E);
}

Drawbacks

  • Increased complexity
  • Some headers (Cookie) merges the HeaderValue which forces a Cow or the merging must be replaced by storing multiple headers in a Vec/SmallVec instead.
@seanmonstar
Copy link
Member

Hm, yea this could be interesting! Since HeaderValue is backed by Bytes and thus cheap to clone, I prioritized an easier user API, but if this doesn't make the experience any worse for them, we could try something like it!

@Marwes
Copy link
Contributor Author

Marwes commented Dec 13, 2019

Ah, wasn't aware that Bytes would upgrade a Vec based allocation into an Arc owned one on clone. In that case there is less gain than I would expect.

Still, ifBytes/HeaderValue is created as a Vec-like, this could avoid the upgrade at least, which would still remove one allocation + copy of the value.

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

2 participants