headers icon indicating copy to clipboard operation
headers copied to clipboard

Allow `Header` to borrow from the header value

Open Marwes opened this issue 6 years ago • 2 comments

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.

Marwes avatar Dec 12 '19 16:12 Marwes

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!

seanmonstar avatar Dec 12 '19 20:12 seanmonstar

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.

Marwes avatar Dec 13 '19 09:12 Marwes