Improve performance (#474)

* benchmark: router.route

* refactor(server): reduce allocations

* one clone less

* one less box

* avoid allocations in StripPrefixEndpoint

* middleware: lazy cookies

* fix: do not set content type twice

* cleanup

* fix workflow

* fix double block

Author: dignifiedquire

.github/workflows/ci.yaml

@@ -45,7 +45,13 @@ jobs:
       uses: actions-rs/cargo@v1
       with:
         command: check
-        args: --all --benches --bins --examples --tests --features unstable
+        args: --all --bins --examples --tests --features unstable
+
+    - name: check benches
+      uses: actions-rs/cargo@v1
+      with:
+        command: check
+        args: --benches --features __internal__bench
 
     - name: tests
       uses: actions-rs/cargo@v1

Cargo.toml

@@ -28,6 +28,8 @@ default = ["h1-server"]
 h1-server = ["async-h1"]
 docs = ["unstable"]
 unstable = []
+# DO NOT USE. Only exists to expose internals so they can be benchmarked.
+__internal__bench = []
 
 [dependencies]
 async-h1 = { version = "1.1.2", optional = true }
@@ -50,8 +52,14 @@ juniper = "0.14.1"
 portpicker = "0.1.0"
 serde = { version = "1.0.102", features = ["derive"] }
 surf = "2.0.0-alpha.1"
+criterion = "0.3.1"
 
 [[test]]
 name = "nested"
 path = "tests/nested.rs"
 required-features = ["unstable"]
+
+[[bench]]
+name = "router"
+harness = false
+

benches/router.rs

@@ -0,0 +1,23 @@
+use criterion::{black_box, criterion_group, criterion_main, Criterion};
+use http_types::Method;
+use tide::router::Router;
+
+fn criterion_benchmark(c: &mut Criterion) {
+    let mut router = Router::<()>::new();
+    router.add(
+        "hello",
+        Method::Get,
+        Box::new(|_| async move { Ok("hello world") }),
+    );
+
+    c.bench_function("route-match", |b| {
+        b.iter(|| black_box(router.route("/hello", Method::Get)))
+    });
+
+    c.bench_function("route-root", |b| {
+        b.iter(|| black_box(router.route("", Method::Get)))
+    });
+}
+
+criterion_group!(benches, criterion_benchmark);
+criterion_main!(benches);

src/cookies/middleware.rs

@@ -2,7 +2,7 @@ use crate::response::CookieEvent;
 use crate::utils::BoxFuture;
 use crate::{Middleware, Next, Request};
 
-use crate::http::cookies::{Cookie, CookieJar};
+use crate::http::cookies::{Cookie, CookieJar, Delta};
 use crate::http::headers;
 
 use std::sync::{Arc, RwLock};
@@ -41,27 +41,32 @@ impl<State: Send + Sync + 'static> Middleware<State> for CookiesMiddleware {
             let cookie_jar = if let Some(cookie_data) = ctx.local::<CookieData>() {
                 cookie_data.content.clone()
             } else {
-                // no cookie data in local context, so we need to create it
                 let cookie_data = CookieData::from_request(&ctx);
+                // no cookie data in local context, so we try to create it
                 let content = cookie_data.content.clone();
                 ctx = ctx.set_local(cookie_data);
                 content
             };
 
             let mut res = next.run(ctx).await?;
 
+            // Don't do anything if there are no cookies.
+            if res.cookie_events.is_empty() {
+                return Ok(res);
+            }
+
+            let jar = &mut *cookie_jar.write().unwrap();
+
             // add modifications from response to original
             for cookie in res.cookie_events.drain(..) {
                 match cookie {
-                    CookieEvent::Added(cookie) => cookie_jar.write().unwrap().add(cookie.clone()),
-                    CookieEvent::Removed(cookie) => {
-                        cookie_jar.write().unwrap().remove(cookie.clone())
-                    }
+                    CookieEvent::Added(cookie) => jar.add(cookie.clone()),
+                    CookieEvent::Removed(cookie) => jar.remove(cookie.clone()),
                 }
             }
 
             // iterate over added and removed cookies
-            for cookie in cookie_jar.read().unwrap().delta() {
+            for cookie in jar.delta() {
                 let encoded_cookie = cookie.encoded().to_string();
                 res = res.append_header(headers::SET_COOKIE, encoded_cookie);
             }
@@ -70,16 +75,48 @@ impl<State: Send + Sync + 'static> Middleware<State> for CookiesMiddleware {
     }
 }
 
-#[derive(Debug, Clone)]
+#[derive(Debug, Default, Clone)]
 pub(crate) struct CookieData {
-    pub(crate) content: Arc<RwLock<CookieJar>>,
+    pub(crate) content: Arc<RwLock<LazyJar>>,
+}
+
+#[derive(Debug, Default, Clone)]
+/// Wrapper around `CookieJar`, that initializes only when actually used.
+pub(crate) struct LazyJar(Option<CookieJar>);
+
+impl LazyJar {
+    fn add(&mut self, cookie: Cookie<'static>) {
+        self.get_jar().add(cookie)
+    }
+
+    fn remove(&mut self, cookie: Cookie<'static>) {
+        self.get_jar().remove(cookie)
+    }
+
+    fn delta(&mut self) -> Delta<'_> {
+        self.get_jar().delta()
+    }
+
+    pub(crate) fn get(&self, name: &str) -> Option<&Cookie<'static>> {
+        if let Some(jar) = &self.0 {
+            return jar.get(name);
+        }
+        None
+    }
+
+    fn get_jar(&mut self) -> &mut CookieJar {
+        if self.0.is_none() {
+            self.0 = Some(CookieJar::new());
+        }
+
+        self.0.as_mut().unwrap()
+    }
 }
 
 impl CookieData {
     pub(crate) fn from_request<S>(req: &Request<S>) -> Self {
-        let mut jar = CookieJar::new();
-
-        if let Some(cookie_headers) = req.header(&headers::COOKIE) {
+        let jar = if let Some(cookie_headers) = req.header(&headers::COOKIE) {
+            let mut jar = CookieJar::new();
             for cookie_header in cookie_headers {
                 // spec says there should be only one, so this is permissive
                 for pair in cookie_header.as_str().split(';') {
@@ -88,9 +125,13 @@ impl CookieData {
                     }
                 }
             }
-        }
 
-        Self {
+            LazyJar(Some(jar))
+        } else {
+            LazyJar::default()
+        };
+
+        CookieData {
             content: Arc::new(RwLock::new(jar)),
         }
     }

src/lib.rs

@@ -193,7 +193,11 @@ mod redirect;
 mod request;
 mod response;
 mod route;
+
+#[cfg(not(feature = "__internal__bench"))]
 mod router;
+#[cfg(feature = "__internal__bench")]
+pub mod router;
 mod server;
 mod utils;
 

src/request.rs

@@ -182,12 +182,6 @@ impl<State> Request<State> {
             .parse()
     }
 
-    pub(crate) fn rest(&self) -> Option<&str> {
-        self.route_params
-            .last()
-            .and_then(|params| params.find("--tide-path-rest"))
-    }
-
     /// Reads the entire request body into a byte buffer.
     ///
     /// This method can be called after the body has already been read, but will
@@ -300,12 +294,10 @@ impl<State> Request<State> {
     /// returns a `Cookie` by name of the cookie.
     #[must_use]
     pub fn cookie(&self, name: &str) -> Option<Cookie<'static>> {
-        let cookie_data = self
-            .local::<CookieData>()
-            .expect("should always be set by the cookies middleware");
-
-        let locked_jar = cookie_data.content.read().unwrap();
-        locked_jar.get(name).cloned()
+        if let Some(cookie_data) = self.local::<CookieData>() {
+            return cookie_data.content.read().unwrap().get(name).cloned();
+        }
+        None
     }
 
     /// Get the length of the body.
@@ -386,3 +378,9 @@ impl<'a, State> IntoIterator for &'a mut Request<State> {
         self.request.iter_mut()
     }
 }
+
+pub(crate) fn rest(route_params: &[Params]) -> Option<&str> {
+    route_params
+        .last()
+        .and_then(|params| params.find("--tide-path-rest"))
+}

src/response.rs

@@ -289,7 +289,6 @@ impl From<http::Response> for Response {
 impl From<String> for Response {
     fn from(s: String) -> Self {
         let mut res = http_types::Response::new(StatusCode::Ok);
-        res.set_content_type(http_types::mime::PLAIN);
         res.set_body(s);
         Self {
             res,
@@ -301,7 +300,6 @@ impl From<String> for Response {
 impl<'a> From<&'a str> for Response {
     fn from(s: &'a str) -> Self {
         let mut res = http_types::Response::new(StatusCode::Ok);
-        res.set_content_type(http_types::mime::PLAIN);
         res.set_body(String::from(s));
         Self {
             res,

src/route.rs

@@ -275,14 +275,20 @@ impl<E> Clone for StripPrefixEndpoint<E> {
 }
 
 impl<State, E: Endpoint<State>> Endpoint<State> for StripPrefixEndpoint<E> {
-    fn call<'a>(&'a self, mut req: crate::Request<State>) -> BoxFuture<'a, crate::Result> {
-        let rest = req.rest().unwrap_or("");
-        let uri = req.uri();
-        let mut new_uri = uri.clone();
-        new_uri.set_path(rest);
+    fn call<'a>(&'a self, req: crate::Request<State>) -> BoxFuture<'a, crate::Result> {
+        let crate::Request {
+            state,
+            mut request,
+            route_params,
+        } = req;
 
-        *req.request.url_mut() = new_uri;
+        let rest = crate::request::rest(&route_params).unwrap_or_else(|| "");
+        request.url_mut().set_path(&rest);
 
-        self.0.call(req)
+        self.0.call(crate::Request {
+            state,
+            request,
+            route_params,
+        })
     }
 }

src/router.rs

@@ -10,42 +10,38 @@ use crate::{Request, Response, StatusCode};
 /// Internally, we have a separate state machine per http method; indexing
 /// by the method first allows the table itself to be more efficient.
 #[allow(missing_debug_implementations)]
-pub(crate) struct Router<State> {
+pub struct Router<State> {
     method_map: HashMap<http_types::Method, MethodRouter<Box<DynEndpoint<State>>>>,
     all_method_router: MethodRouter<Box<DynEndpoint<State>>>,
 }
 
 /// The result of routing a URL
-pub(crate) struct Selection<'a, State> {
+#[allow(missing_debug_implementations)]
+pub struct Selection<'a, State> {
     pub(crate) endpoint: &'a DynEndpoint<State>,
     pub(crate) params: Params,
 }
 
 impl<State: 'static> Router<State> {
-    pub(crate) fn new() -> Self {
-        Self {
+    pub fn new() -> Self {
+        Router {
             method_map: HashMap::default(),
             all_method_router: MethodRouter::new(),
         }
     }
 
-    pub(crate) fn add(
-        &mut self,
-        path: &str,
-        method: http_types::Method,
-        ep: Box<DynEndpoint<State>>,
-    ) {
+    pub fn add(&mut self, path: &str, method: http_types::Method, ep: Box<DynEndpoint<State>>) {
         self.method_map
             .entry(method)
             .or_insert_with(MethodRouter::new)
             .add(path, ep)
     }
 
-    pub(crate) fn add_all(&mut self, path: &str, ep: Box<DynEndpoint<State>>) {
+    pub fn add_all(&mut self, path: &str, ep: Box<DynEndpoint<State>>) {
         self.all_method_router.add(path, ep)
     }
 
-    pub(crate) fn route(&self, path: &str, method: http_types::Method) -> Selection<'_, State> {
+    pub fn route(&self, path: &str, method: http_types::Method) -> Selection<'_, State> {
         if let Some(Match { handler, params }) = self
             .method_map
             .get(&method)

src/server.rs

@@ -340,10 +340,26 @@ impl<State: Send + Sync + 'static> Server<State> {
     where
         R: From<http_types::Response>,
     {
-        let req = Request::new(self.state.clone(), req.into(), Vec::new());
-        match self.call(req).await {
-            Ok(res) => {
-                let res: http_types::Response = res.into();
+        let req = req.into();
+        let Self {
+            router,
+            state,
+            middleware,
+        } = self.clone();
+
+        let method = req.method().to_owned();
+        let Selection { endpoint, params } = router.route(&req.url().path(), method);
+        let route_params = vec![params];
+        let req = Request::new(state, req, route_params);
+
+        let next = Next {
+            endpoint,
+            next_middleware: &middleware,
+        };
+
+        match next.run(req).await {
+            Ok(value) => {
+                let res: http_types::Response = value.into();
                 // We assume that if an error was manually cast to a
                 // Response that we actually want to send the body to the
                 // client. At this point we don't scrub the message.