From 2ea41a3239d1ae7d4d281d483157a4342aaa04d7 Mon Sep 17 00:00:00 2001 From: pasta Date: Mon, 18 Mar 2024 11:41:04 -0500 Subject: [PATCH 1/3] refactor: remove expensive clones from verify_consistency_of_operations; convert API to slice There was never any need for this function to take in a &Vec; we should never do that. Instead take in a slice. Additionally, we refactored some of the loop logic to use .take instead of a check on each iteration to see if we were at the end. We also combined two duplicate for loops into 1. Finally, we significantly refactor the return type to avoid a lot of very expensive cloning. Previously, we were doing something on the magnitude of 9 clones (of expensive underlying objects including vectors) per input GroveDbOp. All of these have been removed. We instead return references to these values instead of clones of them. This does mean that the lifetime of the return value is tied to the underlying data the slice refers to; but especially as I cannot even find an instance where we use these return values, this feels like a very fair trade off. --- grovedb/src/batch/mod.rs | 104 ++++++++++++++++++--------------------- 1 file changed, 49 insertions(+), 55 deletions(-) diff --git a/grovedb/src/batch/mod.rs b/grovedb/src/batch/mod.rs index f6a4e7ea..ba58f674 100644 --- a/grovedb/src/batch/mod.rs +++ b/grovedb/src/batch/mod.rs @@ -552,14 +552,12 @@ impl GroveDbOp { } /// Verify consistency of operations - pub fn verify_consistency_of_operations(ops: &Vec) -> GroveDbOpConsistencyResults { - let ops_len = ops.len(); + pub fn verify_consistency_of_operations(ops: &[GroveDbOp]) -> GroveDbOpConsistencyResults { // operations should not have any duplicates let mut repeated_ops = vec![]; - for (i, op) in ops.iter().enumerate() { - if i == ops_len { - continue; - } // Don't do last one + let mut same_path_key_ops = vec![]; + // Exclude the last item + for (i, op) in ops.iter().take(ops.len() - 1).enumerate() { let count = ops .split_at(i + 1) .1 @@ -567,80 +565,74 @@ impl GroveDbOp { .filter(|¤t_op| current_op == op) .count() as u16; if count > 1 { - repeated_ops.push((op.clone(), count)); + repeated_ops.push((op, count)); } - } - let mut same_path_key_ops = vec![]; - - // No double insert or delete of same key in same path - for (i, op) in ops.iter().enumerate() { - if i == ops_len { - continue; - } // Don't do last one + // No double insert or delete of same key in same path let mut doubled_ops = ops .split_at(i + 1) .1 .iter() .filter_map(|current_op| { if current_op.path == op.path && current_op.key == op.key { - Some(current_op.op.clone()) + Some(¤t_op.op) } else { None } }) - .collect::>(); + .collect::>(); if !doubled_ops.is_empty() { - doubled_ops.push(op.op.clone()); - same_path_key_ops.push((op.path.clone(), op.key.clone(), doubled_ops)); + doubled_ops.push(&op.op); + same_path_key_ops.push((&op.path, &op.key, doubled_ops)); } } let inserts = ops .iter() .filter_map(|current_op| match current_op.op { - Op::Insert { .. } | Op::Replace { .. } => Some(current_op.clone()), + Op::Insert { .. } | Op::Replace { .. } => Some(current_op), _ => None, }) - .collect::>(); + .collect::>(); let deletes = ops .iter() .filter_map(|current_op| { if let Op::Delete = current_op.op { - Some(current_op.clone()) + Some(current_op) } else { None } }) - .collect::>(); - - let mut insert_ops_below_deleted_ops = vec![]; + .collect::>(); - // No inserts under a deleted path - for deleted_op in deletes.iter() { - let mut deleted_qualified_path = deleted_op.path.clone(); - deleted_qualified_path.push(deleted_op.key.clone()); - let inserts_with_deleted_ops_above = inserts - .iter() - .filter_map(|inserted_op| { - if deleted_op.path.len() < inserted_op.path.len() - && deleted_qualified_path - .iterator() - .zip(inserted_op.path.iterator()) - .all(|(a, b)| a == b) - { - Some(inserted_op.clone()) - } else { - None - } - }) - .collect::>(); - if !inserts_with_deleted_ops_above.is_empty() { - insert_ops_below_deleted_ops - .push((deleted_op.clone(), inserts_with_deleted_ops_above)); - } - } + let insert_ops_below_deleted_ops = deletes + .iter() + .filter_map(|&deleted_op| { + let mut deleted_qualified_path = deleted_op.path.clone(); + deleted_qualified_path.push(deleted_op.key.clone()); + let inserts_with_deleted_ops_above = inserts + .iter() + .filter_map(|&inserted_op| { + if deleted_op.path.len() < inserted_op.path.len() + && deleted_qualified_path + .iterator() + .zip(inserted_op.path.iterator()) + .all(|(a, b)| a == b) + { + Some(inserted_op) + } else { + None + } + }) + .collect::>(); + if !inserts_with_deleted_ops_above.is_empty() { + Some((deleted_op, inserts_with_deleted_ops_above)) + } else { + None + } + }) + .collect::)>>(); GroveDbOpConsistencyResults { repeated_ops, @@ -652,14 +644,16 @@ impl GroveDbOp { /// Results of a consistency check on an operation batch #[derive(Debug)] -pub struct GroveDbOpConsistencyResults { - repeated_ops: Vec<(GroveDbOp, u16)>, // the u16 is count - same_path_key_ops: Vec<(KeyInfoPath, KeyInfo, Vec)>, - insert_ops_below_deleted_ops: Vec<(GroveDbOp, Vec)>, /* the deleted op first, - * then inserts under */ +pub struct GroveDbOpConsistencyResults<'a> { + repeated_ops: Vec<(&'a GroveDbOp, u16)>, // the u16 is count + same_path_key_ops: Vec<(&'a KeyInfoPath, &'a KeyInfo, Vec<&'a Op>)>, + insert_ops_below_deleted_ops: Vec<(&'a GroveDbOp, Vec<&'a GroveDbOp>)>, /* the deleted op + * first, + * then inserts + * under */ } -impl GroveDbOpConsistencyResults { +impl GroveDbOpConsistencyResults<'_> { /// Check if results are empty pub fn is_empty(&self) -> bool { self.repeated_ops.is_empty() From d998701d8a380e98cd11301759f3ae3060ebba68 Mon Sep 17 00:00:00 2001 From: pasta Date: Mon, 18 Mar 2024 13:17:28 -0500 Subject: [PATCH 2/3] chore: use filter --- grovedb/src/batch/mod.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/grovedb/src/batch/mod.rs b/grovedb/src/batch/mod.rs index ba58f674..b6e33e3d 100644 --- a/grovedb/src/batch/mod.rs +++ b/grovedb/src/batch/mod.rs @@ -589,19 +589,19 @@ impl GroveDbOp { let inserts = ops .iter() - .filter_map(|current_op| match current_op.op { - Op::Insert { .. } | Op::Replace { .. } => Some(current_op), - _ => None, + .filter(|current_op| match current_op.op { + Op::Insert { .. } | Op::Replace { .. } => true, + _ => false, }) .collect::>(); let deletes = ops .iter() - .filter_map(|current_op| { + .filter(|current_op| { if let Op::Delete = current_op.op { - Some(current_op) + true } else { - None + false } }) .collect::>(); From 3ece0700d4c64c8c0ff4f37b79dc221593c82b32 Mon Sep 17 00:00:00 2001 From: pasta Date: Fri, 22 Mar 2024 11:13:56 -0500 Subject: [PATCH 3/3] chore: use matches! --- grovedb/src/batch/mod.rs | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/grovedb/src/batch/mod.rs b/grovedb/src/batch/mod.rs index b6e33e3d..d4174b5e 100644 --- a/grovedb/src/batch/mod.rs +++ b/grovedb/src/batch/mod.rs @@ -589,21 +589,12 @@ impl GroveDbOp { let inserts = ops .iter() - .filter(|current_op| match current_op.op { - Op::Insert { .. } | Op::Replace { .. } => true, - _ => false, - }) + .filter(|current_op| matches!(current_op.op, Op::Insert { .. } | Op::Replace { .. })) .collect::>(); let deletes = ops .iter() - .filter(|current_op| { - if let Op::Delete = current_op.op { - true - } else { - false - } - }) + .filter(|current_op| matches!(current_op.op, Op::Delete)) .collect::>(); let insert_ops_below_deleted_ops = deletes