From 5687e81d9fc220f17c5420adb6f94c5ef7ad50ff Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Sat, 17 Jul 2021 01:06:23 +0100 Subject: [PATCH] rework IntoPatterns trait and codegen (#372) --- Cargo.toml | 5 + actix-router/CHANGES.md | 2 + actix-router/Cargo.toml | 11 +- actix-router/benches/router.rs | 194 +++++++++++++++++++++++++++++++++ actix-router/src/lib.rs | 122 ++++++++------------- actix-router/src/resource.rs | 66 ++++++----- actix-router/src/router.rs | 4 +- 7 files changed, 300 insertions(+), 104 deletions(-) create mode 100644 actix-router/benches/router.rs diff --git a/Cargo.toml b/Cargo.toml index 5bf72300..6ed9b50a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -27,3 +27,8 @@ actix-utils = { path = "actix-utils" } bytestring = { path = "bytestring" } local-channel = { path = "local-channel" } local-waker = { path = "local-waker" } + +[profile.release] +lto = true +opt-level = 3 +codegen-units = 1 diff --git a/actix-router/CHANGES.md b/actix-router/CHANGES.md index 77251c21..9ec87716 100644 --- a/actix-router/CHANGES.md +++ b/actix-router/CHANGES.md @@ -6,6 +6,7 @@ * Fix segment interpolation leaving `Path` in unintended state after matching. [#368] * Path tail pattern now works as expected after a dynamic segment (e.g. `/user/{uid}/*`). [#366] * Fixed a bug where, in multi-patterns, static patterns are interpreted as regex. [#366] +* Re-work `IntoPatterns` trait. [#372] * Rename `Path::{len => segment_count}` to be more descriptive of it's purpose. [#370] * Alias `ResourceDef::{resource_path => resource_path_from_iter}` pending eventual deprecation. [#371] * Alias `ResourceDef::{resource_path_named => resource_path_from_map}` pending eventual deprecation. [#371] @@ -15,6 +16,7 @@ [#368]: https://github.com/actix/actix-net/pull/368 [#370]: https://github.com/actix/actix-net/pull/370 [#371]: https://github.com/actix/actix-net/pull/371 +[#372]: https://github.com/actix/actix-net/pull/372 ## 0.4.0 - 2021-06-06 diff --git a/actix-router/Cargo.toml b/actix-router/Cargo.toml index 216d4bff..02bd99ad 100644 --- a/actix-router/Cargo.toml +++ b/actix-router/Cargo.toml @@ -20,12 +20,17 @@ path = "src/lib.rs" default = ["http"] [dependencies] +bytestring = ">=0.1.5, <2" +http = { version = "0.2.3", optional = true } +log = "0.4" regex = "1.5" serde = "1" -bytestring = ">=0.1.5, <2" -log = "0.4" -http = { version = "0.2.3", optional = true } [dev-dependencies] http = "0.2.3" serde = { version = "1", features = ["derive"] } +criterion = { version = "0.3", features = ["html_reports"] } + +[[bench]] +name = "router" +harness = false diff --git a/actix-router/benches/router.rs b/actix-router/benches/router.rs new file mode 100644 index 00000000..a428b9f1 --- /dev/null +++ b/actix-router/benches/router.rs @@ -0,0 +1,194 @@ +//! Based on https://github.com/ibraheemdev/matchit/blob/master/benches/bench.rs + +use criterion::{black_box, criterion_group, criterion_main, Criterion}; + +macro_rules! register { + (colon) => {{ + register!(finish => ":p1", ":p2", ":p3", ":p4") + }}; + (brackets) => {{ + register!(finish => "{p1}", "{p2}", "{p3}", "{p4}") + }}; + (regex) => {{ + register!(finish => "(.*)", "(.*)", "(.*)", "(.*)") + }}; + (finish => $p1:literal, $p2:literal, $p3:literal, $p4:literal) => {{ + let arr = [ + concat!("/authorizations"), + concat!("/authorizations/", $p1), + concat!("/applications/", $p1, "/tokens/", $p2), + concat!("/events"), + concat!("/repos/", $p1, "/", $p2, "/events"), + concat!("/networks/", $p1, "/", $p2, "/events"), + concat!("/orgs/", $p1, "/events"), + concat!("/users/", $p1, "/received_events"), + concat!("/users/", $p1, "/received_events/public"), + concat!("/users/", $p1, "/events"), + concat!("/users/", $p1, "/events/public"), + concat!("/users/", $p1, "/events/orgs/", $p2), + concat!("/feeds"), + concat!("/notifications"), + concat!("/repos/", $p1, "/", $p2, "/notifications"), + concat!("/notifications/threads/", $p1), + concat!("/notifications/threads/", $p1, "/subscription"), + concat!("/repos/", $p1, "/", $p2, "/stargazers"), + concat!("/users/", $p1, "/starred"), + concat!("/user/starred"), + concat!("/user/starred/", $p1, "/", $p2), + concat!("/repos/", $p1, "/", $p2, "/subscribers"), + concat!("/users/", $p1, "/subscriptions"), + concat!("/user/subscriptions"), + concat!("/repos/", $p1, "/", $p2, "/subscription"), + concat!("/user/subscriptions/", $p1, "/", $p2), + concat!("/users/", $p1, "/gists"), + concat!("/gists"), + concat!("/gists/", $p1), + concat!("/gists/", $p1, "/star"), + concat!("/repos/", $p1, "/", $p2, "/git/blobs/", $p3), + concat!("/repos/", $p1, "/", $p2, "/git/commits/", $p3), + concat!("/repos/", $p1, "/", $p2, "/git/refs"), + concat!("/repos/", $p1, "/", $p2, "/git/tags/", $p3), + concat!("/repos/", $p1, "/", $p2, "/git/trees/", $p3), + concat!("/issues"), + concat!("/user/issues"), + concat!("/orgs/", $p1, "/issues"), + concat!("/repos/", $p1, "/", $p2, "/issues"), + concat!("/repos/", $p1, "/", $p2, "/issues/", $p3), + concat!("/repos/", $p1, "/", $p2, "/assignees"), + concat!("/repos/", $p1, "/", $p2, "/assignees/", $p3), + concat!("/repos/", $p1, "/", $p2, "/issues/", $p3, "/comments"), + concat!("/repos/", $p1, "/", $p2, "/issues/", $p3, "/events"), + concat!("/repos/", $p1, "/", $p2, "/labels"), + concat!("/repos/", $p1, "/", $p2, "/labels/", $p3), + concat!("/repos/", $p1, "/", $p2, "/issues/", $p3, "/labels"), + concat!("/repos/", $p1, "/", $p2, "/milestones/", $p3, "/labels"), + concat!("/repos/", $p1, "/", $p2, "/milestones/"), + concat!("/repos/", $p1, "/", $p2, "/milestones/", $p3), + concat!("/emojis"), + concat!("/gitignore/templates"), + concat!("/gitignore/templates/", $p1), + concat!("/meta"), + concat!("/rate_limit"), + concat!("/users/", $p1, "/orgs"), + concat!("/user/orgs"), + concat!("/orgs/", $p1), + concat!("/orgs/", $p1, "/members"), + concat!("/orgs/", $p1, "/members", $p2), + concat!("/orgs/", $p1, "/public_members"), + concat!("/orgs/", $p1, "/public_members/", $p2), + concat!("/orgs/", $p1, "/teams"), + concat!("/teams/", $p1), + concat!("/teams/", $p1, "/members"), + concat!("/teams/", $p1, "/members", $p2), + concat!("/teams/", $p1, "/repos"), + concat!("/teams/", $p1, "/repos/", $p2, "/", $p3), + concat!("/user/teams"), + concat!("/repos/", $p1, "/", $p2, "/pulls"), + concat!("/repos/", $p1, "/", $p2, "/pulls/", $p3), + concat!("/repos/", $p1, "/", $p2, "/pulls/", $p3, "/commits"), + concat!("/repos/", $p1, "/", $p2, "/pulls/", $p3, "/files"), + concat!("/repos/", $p1, "/", $p2, "/pulls/", $p3, "/merge"), + concat!("/repos/", $p1, "/", $p2, "/pulls/", $p3, "/comments"), + concat!("/user/repos"), + concat!("/users/", $p1, "/repos"), + concat!("/orgs/", $p1, "/repos"), + concat!("/repositories"), + concat!("/repos/", $p1, "/", $p2), + concat!("/repos/", $p1, "/", $p2, "/contributors"), + concat!("/repos/", $p1, "/", $p2, "/languages"), + concat!("/repos/", $p1, "/", $p2, "/teams"), + concat!("/repos/", $p1, "/", $p2, "/tags"), + concat!("/repos/", $p1, "/", $p2, "/branches"), + concat!("/repos/", $p1, "/", $p2, "/branches/", $p3), + concat!("/repos/", $p1, "/", $p2, "/collaborators"), + concat!("/repos/", $p1, "/", $p2, "/collaborators/", $p3), + concat!("/repos/", $p1, "/", $p2, "/comments"), + concat!("/repos/", $p1, "/", $p2, "/commits/", $p3, "/comments"), + concat!("/repos/", $p1, "/", $p2, "/commits"), + concat!("/repos/", $p1, "/", $p2, "/commits/", $p3), + concat!("/repos/", $p1, "/", $p2, "/readme"), + concat!("/repos/", $p1, "/", $p2, "/keys"), + concat!("/repos/", $p1, "/", $p2, "/keys", $p3), + concat!("/repos/", $p1, "/", $p2, "/downloads"), + concat!("/repos/", $p1, "/", $p2, "/downloads", $p3), + concat!("/repos/", $p1, "/", $p2, "/forks"), + concat!("/repos/", $p1, "/", $p2, "/hooks"), + concat!("/repos/", $p1, "/", $p2, "/hooks", $p3), + concat!("/repos/", $p1, "/", $p2, "/releases"), + concat!("/repos/", $p1, "/", $p2, "/releases/", $p3), + concat!("/repos/", $p1, "/", $p2, "/releases/", $p3, "/assets"), + concat!("/repos/", $p1, "/", $p2, "/stats/contributors"), + concat!("/repos/", $p1, "/", $p2, "/stats/commit_activity"), + concat!("/repos/", $p1, "/", $p2, "/stats/code_frequency"), + concat!("/repos/", $p1, "/", $p2, "/stats/participation"), + concat!("/repos/", $p1, "/", $p2, "/stats/punch_card"), + concat!("/repos/", $p1, "/", $p2, "/statuses/", $p3), + concat!("/search/repositories"), + concat!("/search/code"), + concat!("/search/issues"), + concat!("/search/users"), + concat!("/legacy/issues/search/", $p1, "/", $p2, "/", $p3, "/", $p4), + concat!("/legacy/repos/search/", $p1), + concat!("/legacy/user/search/", $p1), + concat!("/legacy/user/email/", $p1), + concat!("/users/", $p1), + concat!("/user"), + concat!("/users"), + concat!("/user/emails"), + concat!("/users/", $p1, "/followers"), + concat!("/user/followers"), + concat!("/users/", $p1, "/following"), + concat!("/user/following"), + concat!("/user/following/", $p1), + concat!("/users/", $p1, "/following", $p2), + concat!("/users/", $p1, "/keys"), + concat!("/user/keys"), + concat!("/user/keys/", $p1), + ]; + std::array::IntoIter::new(arr) + }}; +} + +fn call() -> impl Iterator { + let arr = [ + "/authorizations", + "/user/repos", + "/repos/rust-lang/rust/stargazers", + "/orgs/rust-lang/public_members/nikomatsakis", + "/repos/rust-lang/rust/releases/1.51.0", + ]; + + std::array::IntoIter::new(arr) +} + +fn compare_routers(c: &mut Criterion) { + let mut group = c.benchmark_group("Compare Routers"); + + let mut actix = actix_router::Router::::build(); + for route in register!(brackets) { + actix.path(route, true); + } + let actix = actix.finish(); + group.bench_function("actix", |b| { + b.iter(|| { + for route in call() { + let mut path = actix_router::Path::new(route); + black_box(actix.recognize(&mut path).unwrap()); + } + }); + }); + + let regex_set = regex::RegexSet::new(register!(regex)).unwrap(); + group.bench_function("regex", |b| { + b.iter(|| { + for route in call() { + black_box(regex_set.matches(route)); + } + }); + }); + + group.finish(); +} + +criterion_group!(benches, compare_routers); +criterion_main!(benches); diff --git a/actix-router/src/lib.rs b/actix-router/src/lib.rs index 5850b103..53ae9db6 100644 --- a/actix-router/src/lib.rs +++ b/actix-router/src/lib.rs @@ -40,98 +40,71 @@ impl ResourcePath for bytestring::ByteString { } } -/// Helper trait for type that could be converted to path pattern -pub trait IntoPattern { - fn is_single(&self) -> bool; - - fn patterns(&self) -> Vec; +/// One or many patterns. +#[derive(Debug, Clone)] +pub enum Patterns { + Single(String), + List(Vec), } -impl IntoPattern for String { - fn is_single(&self) -> bool { - true - } +/// Helper trait for type that could be converted to one or more path pattern. +pub trait IntoPatterns { + fn patterns(&self) -> Patterns; +} - fn patterns(&self) -> Vec { - vec![self.clone()] +impl IntoPatterns for String { + fn patterns(&self) -> Patterns { + Patterns::Single(self.clone()) } } -impl<'a> IntoPattern for &'a String { - fn is_single(&self) -> bool { - true - } - - fn patterns(&self) -> Vec { - vec![self.as_str().to_string()] +impl<'a> IntoPatterns for &'a String { + fn patterns(&self) -> Patterns { + Patterns::Single((*self).clone()) } } -impl<'a> IntoPattern for &'a str { - fn is_single(&self) -> bool { - true - } - - fn patterns(&self) -> Vec { - vec![(*self).to_string()] +impl<'a> IntoPatterns for &'a str { + fn patterns(&self) -> Patterns { + Patterns::Single((*self).to_owned()) } } -impl> IntoPattern for Vec { - fn is_single(&self) -> bool { - self.len() == 1 - } +impl> IntoPatterns for Vec { + fn patterns(&self) -> Patterns { + let mut patterns = self.iter().map(|v| v.as_ref().to_owned()); - fn patterns(&self) -> Vec { - self.iter().map(|v| v.as_ref().to_string()).collect() - } -} - -macro_rules! array_patterns (($tp:ty, $num:tt) => { - impl IntoPattern for [$tp; $num] { - fn is_single(&self) -> bool { - $num == 1 + match patterns.size_hint() { + (1, _) => Patterns::Single(patterns.next().unwrap()), + _ => Patterns::List(patterns.collect()), } + } +} - fn patterns(&self) -> Vec { - self.iter().map(|v| v.to_string()).collect() +macro_rules! array_patterns_single (($tp:ty) => { + impl IntoPatterns for [$tp; 1] { + fn patterns(&self) -> Patterns { + Patterns::Single(self[0].to_owned()) } } }); -array_patterns!(&str, 1); -array_patterns!(&str, 2); -array_patterns!(&str, 3); -array_patterns!(&str, 4); -array_patterns!(&str, 5); -array_patterns!(&str, 6); -array_patterns!(&str, 7); -array_patterns!(&str, 8); -array_patterns!(&str, 9); -array_patterns!(&str, 10); -array_patterns!(&str, 11); -array_patterns!(&str, 12); -array_patterns!(&str, 13); -array_patterns!(&str, 14); -array_patterns!(&str, 15); -array_patterns!(&str, 16); +macro_rules! array_patterns_multiple (($tp:ty, $str_fn:expr, $($num:tt) +) => { + // for each array length specified in $num + $( + impl IntoPatterns for [$tp; $num] { + fn patterns(&self) -> Patterns { + Patterns::List(self.iter().map($str_fn).collect()) + } + } + )+ +}); -array_patterns!(String, 1); -array_patterns!(String, 2); -array_patterns!(String, 3); -array_patterns!(String, 4); -array_patterns!(String, 5); -array_patterns!(String, 6); -array_patterns!(String, 7); -array_patterns!(String, 8); -array_patterns!(String, 9); -array_patterns!(String, 10); -array_patterns!(String, 11); -array_patterns!(String, 12); -array_patterns!(String, 13); -array_patterns!(String, 14); -array_patterns!(String, 15); -array_patterns!(String, 16); +array_patterns_single!(&str); +array_patterns_multiple!(&str, |&v| v.to_owned(), 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16); + +array_patterns_single!(String); +array_patterns_multiple!(String, |v| v.clone(), 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16); #[cfg(feature = "http")] mod url; @@ -140,10 +113,11 @@ mod url; pub use self::url::{Quoter, Url}; #[cfg(feature = "http")] -mod http_support { - use super::ResourcePath; +mod http_impls { use http::Uri; + use super::ResourcePath; + impl ResourcePath for Uri { fn path(&self) -> &str { self.path() diff --git a/actix-router/src/resource.rs b/actix-router/src/resource.rs index ad3d47e2..cb0264ae 100644 --- a/actix-router/src/resource.rs +++ b/actix-router/src/resource.rs @@ -8,8 +8,10 @@ use std::{ use regex::{escape, Regex, RegexSet}; -use crate::path::{Path, PathItem}; -use crate::{IntoPattern, Resource, ResourcePath}; +use crate::{ + path::{Path, PathItem}, + IntoPatterns, Patterns, Resource, ResourcePath, +}; const MAX_DYNAMIC_SEGMENTS: usize = 16; @@ -25,9 +27,6 @@ const REGEX_FLAGS: &str = "(?s-m)"; pub struct ResourceDef { id: u16, - /// Pattern type. - pat_type: PatternType, - /// Optional name of resource definition. Defaults to "". name: String, @@ -35,6 +34,9 @@ pub struct ResourceDef { // TODO: Sort of, in dynamic set pattern type it is blank, consider change to option. pattern: String, + /// Pattern type. + pat_type: PatternType, + /// List of elements that compose the pattern, in order. /// /// `None` with pattern type is DynamicSet. @@ -75,29 +77,43 @@ impl ResourceDef { /// Parse path pattern and create new `Pattern` instance. /// /// Panics if path pattern is malformed. - pub fn new(path: T) -> Self { - if path.is_single() { - ResourceDef::from_single_pattern(&path.patterns()[0], false) - } else { - let mut data = Vec::new(); - let mut re_set = Vec::new(); + pub fn new(path: T) -> Self { + match path.patterns() { + Patterns::Single(pattern) => ResourceDef::from_single_pattern(&pattern, false), - for pattern in path.patterns() { - match ResourceDef::parse(&pattern, false, true) { - (PatternType::Dynamic(re, names), _) => { - re_set.push(re.as_str().to_owned()); - data.push((re, names)); - } - _ => unreachable!(), - } - } - - ResourceDef { + // since zero length pattern sets are possible + // just return a useless `ResourceDef` + Patterns::List(patterns) if patterns.is_empty() => ResourceDef { id: 0, - pat_type: PatternType::DynamicSet(RegexSet::new(re_set).unwrap(), data), - elements: None, name: String::new(), - pattern: "".to_owned(), + pattern: String::new(), + pat_type: PatternType::DynamicSet(RegexSet::empty(), Vec::new()), + elements: None, + }, + + Patterns::List(patterns) => { + let mut re_set = Vec::with_capacity(patterns.len()); + let mut pattern_data = Vec::new(); + + for pattern in patterns { + match ResourceDef::parse(&pattern, false, true) { + (PatternType::Dynamic(re, names), _) => { + re_set.push(re.as_str().to_owned()); + pattern_data.push((re, names)); + } + _ => unreachable!(), + } + } + + let pattern_re_set = RegexSet::new(re_set).unwrap(); + + ResourceDef { + id: 0, + name: String::new(), + pattern: String::new(), + pat_type: PatternType::DynamicSet(pattern_re_set, pattern_data), + elements: None, + } } } } diff --git a/actix-router/src/router.rs b/actix-router/src/router.rs index aeb1aa36..007ef495 100644 --- a/actix-router/src/router.rs +++ b/actix-router/src/router.rs @@ -1,4 +1,4 @@ -use crate::{IntoPattern, Resource, ResourceDef, ResourcePath}; +use crate::{IntoPatterns, Resource, ResourceDef, ResourcePath}; #[derive(Debug, Copy, Clone, PartialEq)] pub struct ResourceId(pub u16); @@ -88,7 +88,7 @@ pub struct RouterBuilder { impl RouterBuilder { /// Register resource for specified path. - pub fn path( + pub fn path( &mut self, path: P, resource: T,