Repository: bearcove/loona (language: rust)

Issue:
Consider using HeaderMap<PieceStr>?
There's a lot of smarts in the http crate's HeaderMap and I'm guessing we'd win more performance than we'd lose (compared to just borrowing everything). Also the effort to port something from hyper to hring would be much smaller then.


Discussion / hints:
(Also, header values are usually the large ones, so those are the things we'd want to refcount, not the names)

(Although it would be a shame to do heap allocs for custom header names over and over and over and ov-)

Fix patch:
diff --git a/src/buffet/piece.rs b/src/buffet/piece.rs
index 348b4264..1c35b9d9 100644
--- a/src/buffet/piece.rs
+++ b/src/buffet/piece.rs
@@ -2,6 +2,7 @@
 
 use std::{fmt, ops::Deref, str::Utf8Error};
 
+use http::header::HeaderName;
 use tokio_uring::buf::IoBuf;
 
 use crate::{Roll, RollStr};
@@ -13,6 +14,7 @@ pub enum Piece {
     Static(&'static [u8]),
     Vec(Vec<u8>),
     Roll(Roll),
+    HeaderName(HeaderName),
 }
 
 impl From<&'static [u8]> for Piece {
@@ -45,6 +47,12 @@ impl From<PieceStr> for Piece {
     }
 }
 
+impl From<HeaderName> for Piece {
+    fn from(name: HeaderName) -> Self {
+        Piece::HeaderName(name)
+    }
+}
+
 impl Deref for Piece {
     type Target = [u8];
 
@@ -59,20 +67,12 @@ impl AsRef<[u8]> for Piece {
             Piece::Static(slice) => slice,
             Piece::Vec(vec) => vec.as_ref(),
             Piece::Roll(roll) => roll.as_ref(),
+            Piece::HeaderName(name) => name.as_str().as_bytes(),
         }
     }
 }
 
 impl Piece {
-    #[inline(always)]
-    pub fn as_io_buf(&self) -> &dyn IoBuf {
-        match self {
-            Piece::Static(slice) => slice,
-            Piece::Vec(vec) => vec,
-            Piece::Roll(roll) => roll,
-        }
-    }
-
     /// Decode as utf-8 (borrowed)
     pub fn as_str(&self) -> Result<&str, Utf8Error> {
         std::str::from_utf8(self.as_ref())
@@ -96,15 +96,30 @@ impl Piece {
 unsafe impl IoBuf for Piece {
     #[inline(always)]
     fn stable_ptr(&self) -> *const u8 {
-        IoBuf::stable_ptr(self.as_io_buf())
+        match self {
+            Piece::Static(s) => IoBuf::stable_ptr(s),
+            Piece::Vec(s) => IoBuf::stable_ptr(s),
+            Piece::Roll(s) => IoBuf::stable_ptr(s),
+            Piece::HeaderName(s) => s.as_str().as_ptr(),
+        }
     }
 
     fn bytes_init(&self) -> usize {
-        IoBuf::bytes_init(self.as_io_buf())
+        match self {
+            Piece::Static(s) => IoBuf::bytes_init(s),
+            Piece::Vec(s) => IoBuf::bytes_init(s),
+            Piece::Roll(s) => IoBuf::bytes_init(s),
+            Piece::HeaderName(s) => s.as_str().len(),
+        }
     }
 
     fn bytes_total(&self) -> usize {
-        IoBuf::bytes_total(self.as_io_buf())
+        match self {
+            Piece::Static(s) => IoBuf::bytes_total(s),
+            Piece::Vec(s) => IoBuf::bytes_total(s),
+            Piece::Roll(s) => IoBuf::bytes_total(s),
+            Piece::HeaderName(s) => s.as_str().len(),
+        }
     }
 }
 
@@ -160,7 +175,7 @@ impl From<Vec<Piece>> for PieceList {
     }
 }
 
-/// A piece of data with a stablea ddress that's _also_
+/// A piece of data with a stable address that's _also_
 /// valid utf-8.
 #[derive(Clone)]
 pub struct PieceStr {
diff --git a/src/h1/client.rs b/src/h1/client.rs
index 7cb10c91..e6ca4d75 100644
--- a/src/h1/client.rs
+++ b/src/h1/client.rs
@@ -8,7 +8,7 @@ use crate::{
     io::ReadWriteOwned,
     types::Request,
     util::{read_and_parse, write_all_list},
-    Body, Response, RollMut,
+    Body, HeadersExt, Response, RollMut,
 };
 
 use super::{
diff --git a/src/h1/encode.rs b/src/h1/encode.rs
index 351bec1b..87bba30a 100644
--- a/src/h1/encode.rs
+++ b/src/h1/encode.rs
@@ -14,12 +14,8 @@ pub(crate) fn encode_request(req: Request, list: &mut PieceList) -> eyre::Result
         Version::HTTP_11 => list.push(" HTTP/1.1\r\n"),
         _ => return Err(eyre::eyre!("unsupported HTTP version {:?}", req.version)),
     }
-    for header in req.headers {
-        list.push(header.name);
-        list.push(": ");
-        list.push(header.value);
-        list.push("\r\n");
-    }
+
+    encode_headers(req.headers, list)?;
     list.push("\r\n");
     Ok(())
 }
@@ -41,12 +37,26 @@ pub(crate) fn encode_response(res: Response, list: &mut PieceList) -> eyre::Resu
 }
 
 pub(crate) fn encode_headers(headers: Headers, list: &mut PieceList) -> eyre::Result<()> {
-    for header in headers {
-        list.push(header.name);
+    let mut last_header_name = None;
+    for (name, value) in headers {
+        match name {
+            Some(name) => {
+                last_header_name = Some(name.clone());
+                list.push(name);
+            }
+            None => {
+                let name = match last_header_name {
+                    Some(ref name) => name.clone(),
+                    None => unreachable!("HeaderMap's IntoIter violated its contract"),
+                };
+                list.push(name);
+            }
+        }
         list.push(": ");
-        list.push(header.value);
+        list.push(value);
         list.push("\r\n");
     }
+
     Ok(())
 }
 
diff --git a/src/h1/parse.rs b/src/h1/parse.rs
index a9466ed0..a3179d1b 100644
--- a/src/h1/parse.rs
+++ b/src/h1/parse.rs
@@ -3,7 +3,7 @@
 //! As of June 2022, the authoritative document for HTTP/1.1
 //! is https://www.rfc-editor.org/rfc/rfc9110
 
-use http::{StatusCode, Version};
+use http::{header::HeaderName, StatusCode, Version};
 use nom::{
     bytes::streaming::{tag, take, take_until, take_while1},
     combinator::{map_res, opt},
@@ -12,7 +12,7 @@ use nom::{
 };
 
 use crate::{
-    types::{Header, Headers, Request, Response},
+    types::{Headers, Request, Response},
     Method, PieceStr, Roll, RollStr,
 };
 
@@ -141,18 +141,17 @@ pub fn headers_and_crlf(mut i: Roll) -> IResult<Roll, Headers> {
             return Ok((i, headers));
         }
 
-        let (i2, (name, value)) = header(i)?;
-        headers.push(Header {
-            name: name.into(),
-            value: value.into(),
-        });
-        i = i2;
+        let (i_next, (name, value)) = header(i)?;
+        headers.append(name, value.into());
+        i = i_next;
     }
 }
 
 /// Parse a single header line
-fn header(i: Roll) -> IResult<Roll, (RollStr, Roll)> {
-    let (i, name) = map_res(take_until_and_consume(b":"), |s: Roll| s.to_string())(i)?;
+fn header(i: Roll) -> IResult<Roll, (HeaderName, Roll)> {
+    let (i, name) = map_res(take_until_and_consume(b":"), |s: Roll| {
+        HeaderName::from_bytes(&s[..])
+    })(i)?;
     let (i, value) = preceded(space1, take_until_and_consume(CRLF))(i)?;
 
     Ok((i, (name, value)))
diff --git a/src/h1/server.rs b/src/h1/server.rs
index 41c3875e..e9589977 100644
--- a/src/h1/server.rs
+++ b/src/h1/server.rs
@@ -7,7 +7,7 @@ use crate::{
     buffet::PieceList,
     io::WriteOwned,
     util::{read_and_parse, write_all_list, SemanticError},
-    Body, Headers, Piece, ReadWriteOwned, Request, Response, RollMut,
+    Body, Headers, HeadersExt, Piece, ReadWriteOwned, Request, Response, RollMut,
 };
 
 use super::{
diff --git a/src/types/headers.rs b/src/types/headers.rs
index bd1cdb20..6f0d0004 100644
--- a/src/types/headers.rs
+++ b/src/types/headers.rs
@@ -1,87 +1,40 @@
 //! Types for HTTP headers
 
-use smallvec::SmallVec;
+use http::{header, HeaderMap};
 
-use crate::{Piece, PieceStr};
+use crate::Piece;
 
-const HEADERS_SMALLVEC_CAPACITY: usize = 32;
+pub type Headers = HeaderMap<Piece>;
 
-#[derive(Default)]
-pub struct Headers {
-    // TODO: this could/should be a multimap. http's multimap is neat but doesn't
-    // support `Piece`/`PieceStr`. The `HeaderName` type should probably have three
-    // variants:
-    //   WellKnown (TransferEncoding, Connection, etc.)
-    //   &'static [u8] (custom)
-    //   AggSlice (proxied)
-    headers: SmallVec<[Header; HEADERS_SMALLVEC_CAPACITY]>,
-}
-
-impl Headers {
-    /// Append a new header. Does not replace anything.
-    pub fn push(&mut self, header: Header) {
-        self.headers.push(header);
-    }
-
-    /// Returns true if we have this key/value combination
-    pub fn has_kv(&self, k: impl AsRef<str>, v: impl AsRef<[u8]>) -> bool {
-        let k = k.as_ref();
-        let v = v.as_ref();
-
-        for h in self {
-            if h.name.eq_ignore_ascii_case(k) && h.value.eq_ignore_ascii_case(v) {
-                return true;
-            }
-        }
-        false
-    }
+pub trait HeadersExt {
+    /// Returns the content-length header
+    fn content_length(&self) -> Option<u64>;
 
     /// Returns true if we have a `connection: close` header
-    pub fn is_connection_close(&self) -> bool {
-        self.has_kv("connection", "close")
-    }
+    fn is_connection_close(&self) -> bool;
 
     /// Returns true if we have a `transfer-encoding: chunked` header
-    pub fn is_chunked_transfer_encoding(&self) -> bool {
-        self.has_kv("transfer-encoding", "chunked")
-    }
+    fn is_chunked_transfer_encoding(&self) -> bool;
+}
 
+impl HeadersExt for HeaderMap<Piece> {
     /// Returns the content-length header
-    pub fn content_length(&self) -> Option<u64> {
-        for h in self {
-            if h.name.eq_ignore_ascii_case("content-length") {
-                if let Some(l) = from_digits(&h.value[..]) {
-                    return Some(l);
-                }
-            }
-        }
-        None
+    fn content_length(&self) -> Option<u64> {
+        self.get(header::CONTENT_LENGTH)
+            .and_then(|s| from_digits(s))
     }
-}
 
-impl<'a> IntoIterator for &'a Headers {
-    type Item = &'a Header;
-    type IntoIter = impl Iterator<Item = Self::Item>;
-
-    fn into_iter(self) -> Self::IntoIter {
-        self.headers.iter()
+    fn is_connection_close(&self) -> bool {
+        self.get(header::CONNECTION)
+            .map_or(false, |value| value.eq_ignore_ascii_case(b"close"))
     }
-}
-
-impl IntoIterator for Headers {
-    type Item = Header;
-    type IntoIter = impl Iterator<Item = Self::Item>;
 
-    fn into_iter(self) -> Self::IntoIter {
-        self.headers.into_iter()
+    fn is_chunked_transfer_encoding(&self) -> bool {
+        self.get(header::TRANSFER_ENCODING)
+            .map_or(false, |value| value.eq_ignore_ascii_case(b"chunked"))
     }
 }
 
-pub struct Header {
-    pub name: PieceStr,
-    pub value: Piece,
-}
-
 fn from_digits(bytes: &[u8]) -> Option<u64> {
     // cannot use FromStr for u64, since it allows a signed prefix
     let mut result = 0u64;
diff --git a/src/types/mod.rs b/src/types/mod.rs
index 664c3cab..0423c396 100644
--- a/src/types/mod.rs
+++ b/src/types/mod.rs
@@ -39,8 +39,8 @@ impl Default for Request {
 impl Request {
     pub(crate) fn debug_print(&self) {
         debug!(method = %self.method, path = %self.path, version = ?self.version, "got request");
-        for h in &self.headers {
-            debug!(name = %h.name, value = ?h.value.as_str(), "got header");
+        for (name, value) in &self.headers {
+            debug!(%name, value = ?value.as_str(), "got header");
         }
     }
 }
@@ -70,8 +70,8 @@ impl Default for Response {
 impl Response {
     pub(crate) fn debug_print(&self) {
         debug!(code = %self.status, version = ?self.version, "got response");
-        for h in &self.headers {
-            debug!(name = %h.name, value = ?h.value.as_str(), "got header");
+        for (name, value) in &self.headers {
+            debug!(%name, value = ?value.as_str(), "got header");
         }
     }
 }


Test patch:
diff --git a/tests/integration_test.rs b/tests/integration_test.rs
index 46d71084..aa05f690 100644
--- a/tests/integration_test.rs
+++ b/tests/integration_test.rs
@@ -6,8 +6,8 @@ mod helpers;
 
 use bytes::BytesMut;
 use hring::{
-    h1, Body, BodyChunk, ChanRead, ChanWrite, Method, ReadWritePair, Request, Response, RollMut,
-    WriteOwned,
+    h1, Body, BodyChunk, ChanRead, ChanWrite, HeadersExt, Method, ReadWritePair, Request, Response,
+    RollMut, WriteOwned,
 };
 use http::StatusCode;
 use httparse::{Status, EMPTY_HEADER};