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
This commit is contained in:
Firstyear 2020-09-11 12:39:05 +10:00 committed by GitHub
parent bab44028f1
commit 29566b8f99
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
7 changed files with 221 additions and 106 deletions

80
Cargo.lock generated
View file

@ -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",

View file

@ -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"

View file

@ -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"

View file

@ -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<Value>),
}
#[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<u64, Box<Entry<EntrySealed, EntryCommitted>>>,
@ -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)
})

View file

@ -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<dyn IdxKeyToRef + 'a> 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<H: Hasher>(&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<dyn IdlCacheKeyToRef + 'a> 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<H: Hasher>(&self, state: &mut H) {
self.keyref().hash(state)
}
}
impl<'a> PartialOrd for (dyn IdlCacheKeyToRef + 'a) {
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
self.keyref().partial_cmp(&other.keyref())
}
}
impl<'a> Ord for (dyn IdlCacheKeyToRef + 'a) {
fn cmp(&self, other: &Self) -> Ordering {
self.keyref().cmp(&other.keyref())
}
}

View file

@ -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,

View file

@ -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<FilterValid> {
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<Self> {
fn resolve_idx(fc: FilterComp, ev: &Event, idxmeta: &HashSet<IdxKey>) -> Option<Self> {
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()),