From 29566b8f997fffd135926be8747131b970a434aa Mon Sep 17 00:00:00 2001 From: Firstyear Date: Fri, 11 Sep 2020 12:39:05 +1000 Subject: [PATCH] 259 reduce clones (#319) Fixes #259. Thanks to the linked worked example, we can reduce a problematic set of clones during filter metadata injection. Previously we had to create a new hashset every time we went to resolve index metadata in queries, but with this change we can now just use the copy-on-write hashset instead. This will improve cache access, reduces clones, and more. In a cargo test run this takes out nearly 15% of the execution time (on my system reducing the test time by nearly 35 seconds). https://github.com/sunshowers/borrow-complex-key-example/blob/master/src/lib.rs --- Cargo.lock | 80 +++++++-------- kanidmd/Cargo.toml | 2 +- kanidmd/server.toml | 2 +- kanidmd/src/lib/be/idl_arc_sqlite.rs | 55 +++++------ kanidmd/src/lib/be/idxkey.rs | 141 +++++++++++++++++++++++++++ kanidmd/src/lib/be/mod.rs | 17 +--- kanidmd/src/lib/filter.rs | 30 +++--- 7 files changed, 221 insertions(+), 106 deletions(-) create mode 100644 kanidmd/src/lib/be/idxkey.rs diff --git a/Cargo.lock b/Cargo.lock index 3d3befec2..04a30a88d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -199,9 +199,9 @@ dependencies = [ [[package]] name = "async-mutex" -version = "1.2.0" +version = "1.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "065de1ccf10280d0d75c2f3a71a970ee1007c85c51aa3e7deee1df100f1dfadb" +checksum = "66941c2577c4fa351e4ce5fdde8f86c69b88d623f3b955be1bc7362a23434632" dependencies = [ "event-listener", ] @@ -576,9 +576,9 @@ dependencies = [ [[package]] name = "concread" -version = "0.2.1" +version = "0.2.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "04dd709cffc030293a6ed69455add2a686def9c8c443504d9d65f5a9b0cd6393" +checksum = "f80b0af539d993d0954653a4e93fef28eb9e67d36b3b40522f3666b68b3f054c" dependencies = [ "ahash 0.4.4", "crossbeam", @@ -644,7 +644,7 @@ dependencies = [ "percent-encoding", "rand", "sha2 0.9.1", - "time 0.2.17", + "time 0.2.18", "version_check", ] @@ -660,7 +660,7 @@ dependencies = [ "publicsuffix", "serde", "serde_json", - "time 0.2.17", + "time 0.2.18", "url", ] @@ -1215,9 +1215,9 @@ dependencies = [ [[package]] name = "getrandom" -version = "0.1.14" +version = "0.1.15" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7abc8dd8451921606d809ba32e95b6111925cd2906060d2dcc29c070220503eb" +checksum = "fc587bc0ec293155d5bfa6b9891ec18a1e330c234f896ea47fbada4cadbe47e6" dependencies = [ "cfg-if", "libc", @@ -1438,7 +1438,7 @@ dependencies = [ "serde", "serde_derive", "smallvec", - "time 0.2.17", + "time 0.2.18", ] [[package]] @@ -1515,9 +1515,9 @@ checksum = "dc6f3ad7b9d11a0c00842ff8de1b60ee58661048eb8049ed33c73594f359d7e6" [[package]] name = "js-sys" -version = "0.3.44" +version = "0.3.45" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "85a7e2c92a4804dd459b86c339278d0fe87cf93757fae222c3fa3ae75458bc73" +checksum = "ca059e81d9486668f12d455a4ea6daa600bd408134cd17e3d3fb5a32d1f016f8" dependencies = [ "wasm-bindgen", ] @@ -1698,9 +1698,9 @@ dependencies = [ [[package]] name = "libc" -version = "0.2.76" +version = "0.2.77" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "755456fae044e6fa1ebbbd1b3e902ae19e73097ed4ed87bb79934a867c007bc3" +checksum = "f2f96b10ec2560088a8e76961b00d47107b3a625fecb76dedb29ee7ccbf98235" [[package]] name = "libnss" @@ -1903,9 +1903,9 @@ dependencies = [ [[package]] name = "net2" -version = "0.2.34" +version = "0.2.35" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2ba7c918ac76704fb42afcbbb43891e72731f3dcca3bef2a19786297baf14af7" +checksum = "3ebc3ec692ed7c9a255596c67808dee269f64655d8baf7b4f0638e51ba1d6853" dependencies = [ "cfg-if", "libc", @@ -2278,9 +2278,9 @@ checksum = "eba180dafb9038b050a4c280019bbedf9f2467b61e5d892dcad585bb57aadc5a" [[package]] name = "proc-macro2" -version = "1.0.20" +version = "1.0.21" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "175c513d55719db99da20232b06cda8bab6b83ec2d04e3283edf0213c37c1a29" +checksum = "36e28516df94f3dd551a587da5357459d9b36d945a7c37c3557928c1c2ff2a2c" dependencies = [ "unicode-xid", ] @@ -2482,7 +2482,7 @@ dependencies = [ "serde", "serde_json", "serde_urlencoded", - "time 0.2.17", + "time 0.2.18", "tokio", "tokio-tls", "url", @@ -2820,9 +2820,9 @@ dependencies = [ [[package]] name = "socket2" -version = "0.3.12" +version = "0.3.15" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "03088793f677dce356f3ccc2edb1b314ad191ab702a5de3faf49304f7e104918" +checksum = "b1fa70dc5c8104ec096f4fe7ede7a221d35ae13dcd19ba1ad9a81d2cab9a1c44" dependencies = [ "cfg-if", "libc", @@ -2943,9 +2943,9 @@ dependencies = [ [[package]] name = "subtle" -version = "2.2.3" +version = "2.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "502d53007c02d7605a05df1c1a73ee436952781653da5d0bf57ad608f66932c1" +checksum = "343f3f510c2915908f155e94f17220b19ccfacf2a64a2a5d8004f2c3e311e7fd" [[package]] name = "syn" @@ -3066,9 +3066,9 @@ dependencies = [ [[package]] name = "time" -version = "0.2.17" +version = "0.2.18" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ca7ec98a72285d12e0febb26f0847b12d54be24577618719df654c66cadab55d" +checksum = "12785163ae8a1cbb52a5db39af4a5baabd3fe49f07f76f952f89d7e89e5ce531" dependencies = [ "const_fn", "libc", @@ -3215,9 +3215,9 @@ dependencies = [ [[package]] name = "tracing-core" -version = "0.1.15" +version = "0.1.16" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4f0e00789804e99b20f12bc7003ca416309d28a6f495d6af58d1e2c2842461b5" +checksum = "5bcf46c1f1f06aeea2d6b81f3c863d0930a596c86ad1920d4e5bad6dd1d7119a" dependencies = [ "lazy_static", ] @@ -3386,9 +3386,9 @@ checksum = "1a143597ca7c7793eff794def352d41792a93c481eb1042423ff7ff72ba2c31f" [[package]] name = "wasm-bindgen" -version = "0.2.67" +version = "0.2.68" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f0563a9a4b071746dd5aedbc3a28c6fe9be4586fb3fbadb67c400d4f53c6b16c" +checksum = "1ac64ead5ea5f05873d7c12b545865ca2b8d28adfc50a49b84770a3a97265d42" dependencies = [ "cfg-if", "serde", @@ -3398,9 +3398,9 @@ dependencies = [ [[package]] name = "wasm-bindgen-backend" -version = "0.2.67" +version = "0.2.68" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bc71e4c5efa60fb9e74160e89b93353bc24059999c0ae0fb03affc39770310b0" +checksum = "f22b422e2a757c35a73774860af8e112bff612ce6cb604224e8e47641a9e4f68" dependencies = [ "bumpalo", "lazy_static", @@ -3413,9 +3413,9 @@ dependencies = [ [[package]] name = "wasm-bindgen-futures" -version = "0.4.17" +version = "0.4.18" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "95f8d235a77f880bcef268d379810ea6c0af2eacfa90b1ad5af731776e0c4699" +checksum = "b7866cab0aa01de1edf8b5d7936938a7e397ee50ce24119aef3e1eaa3b6171da" dependencies = [ "cfg-if", "js-sys", @@ -3425,9 +3425,9 @@ dependencies = [ [[package]] name = "wasm-bindgen-macro" -version = "0.2.67" +version = "0.2.68" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "97c57cefa5fa80e2ba15641578b44d36e7a64279bc5ed43c6dbaf329457a2ed2" +checksum = "6b13312a745c08c469f0b292dd2fcd6411dba5f7160f593da6ef69b64e407038" dependencies = [ "quote", "wasm-bindgen-macro-support", @@ -3435,9 +3435,9 @@ dependencies = [ [[package]] name = "wasm-bindgen-macro-support" -version = "0.2.67" +version = "0.2.68" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "841a6d1c35c6f596ccea1f82504a192a60378f64b3bb0261904ad8f2f5657556" +checksum = "f249f06ef7ee334cc3b8ff031bfc11ec99d00f34d86da7498396dc1e3b1498fe" dependencies = [ "proc-macro2", "quote", @@ -3448,15 +3448,15 @@ dependencies = [ [[package]] name = "wasm-bindgen-shared" -version = "0.2.67" +version = "0.2.68" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "93b162580e34310e5931c4b792560108b10fd14d64915d7fff8ff00180e70092" +checksum = "1d649a3145108d7d3fbcde896a468d1bd636791823c9921135218ad89be08307" [[package]] name = "web-sys" -version = "0.3.44" +version = "0.3.45" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "dda38f4e5ca63eda02c059d243aa25b5f35ab98451e518c51612cd0f1bd19a47" +checksum = "4bf6ef87ad7ae8008e15a355ce696bed26012b7caa21605188cfd8214ab51e2d" dependencies = [ "js-sys", "wasm-bindgen", diff --git a/kanidmd/Cargo.toml b/kanidmd/Cargo.toml index 279193401..1fb6ac0ae 100644 --- a/kanidmd/Cargo.toml +++ b/kanidmd/Cargo.toml @@ -68,7 +68,7 @@ structopt = { version = "0.3", default-features = false } time = "0.1" hashbrown = "0.8" -concread = "^0.2" +concread = "^0.2.3" # concread = { path = "../../concread", features = ["asynch"] } # concread = { path = "../../concread" } # crossbeam = "0.7" diff --git a/kanidmd/server.toml b/kanidmd/server.toml index a25c06b87..b0affc918 100644 --- a/kanidmd/server.toml +++ b/kanidmd/server.toml @@ -5,4 +5,4 @@ db_fs_type = "zfs" tls_ca = "../insecure/ca.pem" tls_cert = "../insecure/cert.pem" tls_key = "../insecure/key.pem" -log_level = "verbose" +log_level = "perfbasic" diff --git a/kanidmd/src/lib/be/idl_arc_sqlite.rs b/kanidmd/src/lib/be/idl_arc_sqlite.rs index 1df5b954a..f4361df22 100644 --- a/kanidmd/src/lib/be/idl_arc_sqlite.rs +++ b/kanidmd/src/lib/be/idl_arc_sqlite.rs @@ -2,6 +2,7 @@ use crate::audit::AuditScope; use crate::be::idl_sqlite::{ FsType, IdlSqlite, IdlSqliteReadTransaction, IdlSqliteTransaction, IdlSqliteWriteTransaction, }; +use crate::be::idxkey::{IdlCacheKey, IdlCacheKeyRef, IdlCacheKeyToRef}; use crate::be::{IdRawEntry, IDL}; use crate::entry::{Entry, EntryCommitted, EntrySealed}; use crate::value::IndexType; @@ -38,30 +39,6 @@ enum NameCacheValue { S(Box), } -#[derive(Debug, Clone, Ord, PartialOrd, Eq, PartialEq, Hash)] -struct IdlCacheKey { - a: String, - i: IndexType, - k: String, -} - -/* -impl Borrow<(&str, &IndexType, &str)> for IdlCacheKey { - #[inline] - fn borrow(&self) -> &(&str, &IndexType, &str) { - &(self.a.as_str(), &self.i, self.k.as_str()) - } -} - -impl From<(&str, &IndexType, &str)> for IdlCacheKey { - fn from((a, i, k): (&str, &IndexType, &str)) -> IdlCacheKey { - IdlCacheKey { - a: a.to_string(), i: (*i).clone(), k: k.to_string() - } - } -} -*/ - pub struct IdlArcSqlite { db: IdlSqlite, entry_cache: ARCache>>, @@ -184,14 +161,25 @@ macro_rules! get_idl { $idx_key:expr ) => {{ lperf_trace_segment!($audit, "be::idl_arc_sqlite::get_idl", || { - // TODO #259: Find a way to implement borrow for this properly + // SEE ALSO #259: Find a way to implement borrow for this properly. + // I don't think this is possible. When we make this dyn, the arc + // needs the dyn trait to be sized so that it *could* claim a clone + // for hit tracking reasons. That also means that we need From and + // some other traits that just seem incompatible. And in the end, + // we clone a few times in arc, and if we miss we need to insert anyway + // + // So the best path could be to replace IdlCacheKey with a compressed + // or smaller type. Perhaps even a small cache of the IdlCacheKeys that + // are allocated to reduce some allocs? Probably over thinking it at + // this point. + // // First attempt to get from this cache. - let cache_key = IdlCacheKey { - a: $attr.to_string(), - i: $itype.clone(), - k: $idx_key.to_string(), + let cache_key = IdlCacheKeyRef { + a: $attr, + i: $itype, + k: $idx_key, }; - let cache_r = $self.idl_cache.get(&cache_key); + let cache_r = $self.idl_cache.get(&cache_key as &dyn IdlCacheKeyToRef); // If hit, continue. if let Some(ref data) = cache_r { ltrace!( @@ -206,7 +194,12 @@ macro_rules! get_idl { // If miss, get from db *and* insert to the cache. let db_r = $self.db.get_idl($audit, $attr, $itype, $idx_key)?; if let Some(ref idl) = db_r { - $self.idl_cache.insert(cache_key, Box::new(idl.clone())) + let ncache_key = IdlCacheKey { + a: $attr.to_string(), + i: $itype.clone(), + k: $idx_key.to_string(), + }; + $self.idl_cache.insert(ncache_key, Box::new(idl.clone())) } Ok(db_r) }) diff --git a/kanidmd/src/lib/be/idxkey.rs b/kanidmd/src/lib/be/idxkey.rs new file mode 100644 index 000000000..04decfc7c --- /dev/null +++ b/kanidmd/src/lib/be/idxkey.rs @@ -0,0 +1,141 @@ +use crate::value::IndexType; +use std::borrow::Borrow; +use std::cmp::Ordering; +use std::hash::{Hash, Hasher}; + +// Huge props to https://github.com/sunshowers/borrow-complex-key-example/blob/master/src/lib.rs + +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub struct IdxKey { + pub attr: String, + pub itype: IndexType, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub struct IdxKeyRef<'a> { + pub attr: &'a str, + pub itype: &'a IndexType, +} + +impl<'a> IdxKeyRef<'a> { + pub fn new(attr: &'a str, itype: &'a IndexType) -> Self { + IdxKeyRef { attr, itype } + } +} + +pub trait IdxKeyToRef { + fn keyref<'k>(&'k self) -> IdxKeyRef<'k>; +} + +impl<'a> IdxKeyToRef for IdxKeyRef<'a> { + fn keyref<'k>(&'k self) -> IdxKeyRef<'k> { + // Copy the self. + *self + } +} + +impl IdxKeyToRef for IdxKey { + fn keyref<'a>(&'a self) -> IdxKeyRef<'a> { + IdxKeyRef { + attr: self.attr.as_str(), + itype: &self.itype, + } + } +} + +impl<'a> Borrow for IdxKey { + fn borrow(&self) -> &(dyn IdxKeyToRef + 'a) { + self + } +} + +impl<'a> PartialEq for (dyn IdxKeyToRef + 'a) { + fn eq(&self, other: &Self) -> bool { + self.keyref().eq(&other.keyref()) + } +} + +impl<'a> Eq for (dyn IdxKeyToRef + 'a) {} + +impl<'a> Hash for (dyn IdxKeyToRef + 'a) { + fn hash(&self, state: &mut H) { + self.keyref().hash(state) + } +} + +// ===== idlcachekey ====== + +#[derive(Debug, Clone, Ord, PartialOrd, Eq, PartialEq, Hash)] +pub struct IdlCacheKey { + pub a: String, + pub i: IndexType, + pub k: String, +} + +#[derive(Debug, Clone, Copy, Ord, PartialOrd, Eq, PartialEq, Hash)] +pub struct IdlCacheKeyRef<'a> { + pub a: &'a str, + pub i: &'a IndexType, + pub k: &'a str, +} + +/* +impl<'a> IdlCacheKeyRef<'a> { + pub fn new(a: &'a str, i: &'a IndexType, k: &'a str) -> Self { + IdlCacheKeyRef { a, i, k } + } +} +*/ + +pub trait IdlCacheKeyToRef { + fn keyref<'k>(&'k self) -> IdlCacheKeyRef<'k>; +} + +impl<'a> IdlCacheKeyToRef for IdlCacheKeyRef<'a> { + fn keyref<'k>(&'k self) -> IdlCacheKeyRef<'k> { + // Copy the self + *self + } +} + +impl IdlCacheKeyToRef for IdlCacheKey { + fn keyref<'k>(&'k self) -> IdlCacheKeyRef<'k> { + IdlCacheKeyRef { + a: self.a.as_str(), + i: &self.i, + k: &self.k.as_str(), + } + } +} + +impl<'a> Borrow for IdlCacheKey { + fn borrow(&self) -> &(dyn IdlCacheKeyToRef + 'a) { + self + } +} + +impl<'a> PartialEq for (dyn IdlCacheKeyToRef + 'a) { + fn eq(&self, other: &Self) -> bool { + self.keyref().eq(&other.keyref()) + } +} + +impl<'a> Eq for (dyn IdlCacheKeyToRef + 'a) {} + +impl<'a> Hash for (dyn IdlCacheKeyToRef + 'a) { + fn hash(&self, state: &mut H) { + self.keyref().hash(state) + } +} + +impl<'a> PartialOrd for (dyn IdlCacheKeyToRef + 'a) { + fn partial_cmp(&self, other: &Self) -> Option { + self.keyref().partial_cmp(&other.keyref()) + } +} + +impl<'a> Ord for (dyn IdlCacheKeyToRef + 'a) { + fn cmp(&self, other: &Self) -> Ordering { + self.keyref().cmp(&other.keyref()) + } +} diff --git a/kanidmd/src/lib/be/mod.rs b/kanidmd/src/lib/be/mod.rs index c4e73bfc2..4824e1905 100644 --- a/kanidmd/src/lib/be/mod.rs +++ b/kanidmd/src/lib/be/mod.rs @@ -23,6 +23,9 @@ pub mod dbentry; pub mod dbvalue; mod idl_arc_sqlite; mod idl_sqlite; +pub(crate) mod idxkey; + +pub(crate) use self::idxkey::{IdxKey, IdxKeyRef, IdxKeyToRef}; use crate::be::idl_arc_sqlite::{ IdlArcSqlite, IdlArcSqliteReadTransaction, IdlArcSqliteTransaction, @@ -35,20 +38,6 @@ pub use crate::be::idl_sqlite::FsType; const FILTER_SEARCH_TEST_THRESHOLD: usize = 8; const FILTER_EXISTS_TEST_THRESHOLD: usize = 0; -#[derive(Debug, Clone, PartialEq, Eq, Hash)] -pub struct IdxKey { - pub attr: String, - pub itype: IndexType, -} - -/* -#[derive(Debug)] -pub(crate) struct IdxKeyRef<'a> { - pub attr: &'a str, - pub itype: &'a IndexType, -} -*/ - #[derive(Debug, Clone)] pub enum IDL { ALLIDS, diff --git a/kanidmd/src/lib/filter.rs b/kanidmd/src/lib/filter.rs index 8cbebed25..90db3c2c9 100644 --- a/kanidmd/src/lib/filter.rs +++ b/kanidmd/src/lib/filter.rs @@ -9,7 +9,7 @@ //! [`Entry`]: ../entry/struct.Entry.html use crate::audit::AuditScope; -use crate::be::IdxKey; +use crate::be::{IdxKey, IdxKeyRef, IdxKeyToRef}; use crate::event::{Event, EventOrigin}; use crate::ldap::ldap_attr_filter_map; use crate::schema::SchemaTransaction; @@ -280,15 +280,7 @@ impl Filter { Ok(Filter { state: FilterValidResolved { inner: match idxmeta { - Some(idx) => { - // Convert it to a reference set. - - // TODO #259: Fix this to use borrows - let idx_ref: HashSet<(&String, &IndexType)> = - idx.iter().map(|ikey| (&ikey.attr, &ikey.itype)).collect(); - - FilterResolved::resolve_idx(self.state.inner.clone(), ev, &idx_ref) - } + Some(idx) => FilterResolved::resolve_idx(self.state.inner.clone(), ev, idx), None => FilterResolved::resolve_no_idx(self.state.inner.clone(), ev), } .ok_or(OperationError::FilterUUIDResolution)?, @@ -970,22 +962,21 @@ impl FilterResolved { } } - fn resolve_idx( - fc: FilterComp, - ev: &Event, - idxmeta: &HashSet<(&String, &IndexType)>, - ) -> Option { + fn resolve_idx(fc: FilterComp, ev: &Event, idxmeta: &HashSet) -> Option { match fc { FilterComp::Eq(a, v) => { - let idx = idxmeta.contains(&(&a, &IndexType::EQUALITY)); + let idxkref = IdxKeyRef::new(&a, &IndexType::EQUALITY); + let idx = idxmeta.contains(&idxkref as &dyn IdxKeyToRef); Some(FilterResolved::Eq(a, v, idx)) } FilterComp::Sub(a, v) => { - let idx = idxmeta.contains(&(&a, &IndexType::SUBSTRING)); + let idxkref = IdxKeyRef::new(&a, &IndexType::SUBSTRING); + let idx = idxmeta.contains(&idxkref as &dyn IdxKeyToRef); Some(FilterResolved::Sub(a, v, idx)) } FilterComp::Pres(a) => { - let idx = idxmeta.contains(&(&a, &IndexType::PRESENCE)); + let idxkref = IdxKeyRef::new(&a, &IndexType::PRESENCE); + let idx = idxmeta.contains(&idxkref as &dyn IdxKeyToRef); Some(FilterResolved::Pres(a, idx)) } FilterComp::LessThan(a, v) => { @@ -1026,7 +1017,8 @@ impl FilterResolved { FilterComp::SelfUUID => match &ev.origin { EventOrigin::User(e) => { let uuid_s = "uuid".to_string(); - let idx = idxmeta.contains(&(&uuid_s, &IndexType::EQUALITY)); + let idxkref = IdxKeyRef::new(&uuid_s, &IndexType::EQUALITY); + let idx = idxmeta.contains(&idxkref as &dyn IdxKeyToRef); Some(FilterResolved::Eq( uuid_s, PartialValue::new_uuid(*e.get_uuid()),