From b78211ca65ae49b0794b004f80ec8350eb39abcf Mon Sep 17 00:00:00 2001 From: Orion Henry Date: Sun, 11 Dec 2022 10:56:20 -0800 Subject: [PATCH] change opid to (u32,u32) - 10% performance uptick (#473) --- rust/automerge/src/automerge.rs | 15 ++++---- rust/automerge/src/change.rs | 2 +- rust/automerge/src/clock.rs | 20 +++++------ .../src/columnar/column_range/key.rs | 4 +-- .../src/columnar/column_range/obj_id.rs | 2 +- .../src/columnar/column_range/opid.rs | 2 +- .../src/columnar/column_range/opid_list.rs | 2 +- .../src/columnar/encoding/properties.rs | 2 +- rust/automerge/src/op_set.rs | 14 ++++---- rust/automerge/src/op_tree.rs | 2 +- rust/automerge/src/op_tree/iter.rs | 2 +- rust/automerge/src/transaction/inner.rs | 2 +- rust/automerge/src/types.rs | 36 +++++++++++-------- rust/automerge/src/types/opids.rs | 5 +-- 14 files changed, 60 insertions(+), 50 deletions(-) diff --git a/rust/automerge/src/automerge.rs b/rust/automerge/src/automerge.rs index 7a5340e67..5502456c6 100644 --- a/rust/automerge/src/automerge.rs +++ b/rust/automerge/src/automerge.rs @@ -487,7 +487,7 @@ impl Automerge { // do a direct get here b/c this could be foriegn and not be within the array // bounds let obj = if self.ops.m.actors.cache.get(*idx) == Some(actor) { - ObjId(OpId(*ctr, *idx)) + ObjId(OpId::new(*ctr, *idx)) } else { // FIXME - make a real error let idx = self @@ -496,7 +496,7 @@ impl Automerge { .actors .lookup(actor) .ok_or(AutomergeError::Fail)?; - ObjId(OpId(*ctr, idx)) + ObjId(OpId::new(*ctr, idx)) }; if let Some(obj_type) = self.ops.object_type(&obj) { Ok((obj, obj_type)) @@ -859,23 +859,26 @@ impl Automerge { .iter_ops() .enumerate() .map(|(i, c)| { - let id = OpId(change.start_op().get() + i as u64, actor); + let id = OpId::new(change.start_op().get() + i as u64, actor); let key = match &c.key { EncodedKey::Prop(n) => Key::Map(self.ops.m.props.cache(n.to_string())), EncodedKey::Elem(e) if e.is_head() => Key::Seq(ElemId::head()), EncodedKey::Elem(ElemId(o)) => { - Key::Seq(ElemId(OpId::new(actors[o.actor()], o.counter()))) + Key::Seq(ElemId(OpId::new(o.counter(), actors[o.actor()]))) } }; let obj = if c.obj.is_root() { ObjId::root() } else { - ObjId(OpId(c.obj.opid().counter(), actors[c.obj.opid().actor()])) + ObjId(OpId::new( + c.obj.opid().counter(), + actors[c.obj.opid().actor()], + )) }; let pred = c .pred .iter() - .map(|p| OpId::new(actors[p.actor()], p.counter())); + .map(|p| OpId::new(p.counter(), actors[p.actor()])); let pred = self.ops.m.sorted_opids(pred); ( obj, diff --git a/rust/automerge/src/change.rs b/rust/automerge/src/change.rs index 198c68fbc..b5cae7dfd 100644 --- a/rust/automerge/src/change.rs +++ b/rust/automerge/src/change.rs @@ -356,7 +356,7 @@ pub(crate) mod gen { (0_u64..10) .prop_map(|num_ops| { (0..num_ops) - .map(|counter| OpId::new(0, counter)) + .map(|counter| OpId::new(counter, 0)) .collect::>() }) .prop_flat_map(move |opids| { diff --git a/rust/automerge/src/clock.rs b/rust/automerge/src/clock.rs index 11890ffba..791253238 100644 --- a/rust/automerge/src/clock.rs +++ b/rust/automerge/src/clock.rs @@ -59,8 +59,8 @@ impl Clock { } pub(crate) fn covers(&self, id: &OpId) -> bool { - if let Some(data) = self.0.get(&id.1) { - data.max_op >= id.0 + if let Some(data) = self.0.get(&id.actor()) { + data.max_op >= id.counter() } else { false } @@ -123,16 +123,16 @@ mod tests { clock.include(1, ClockData { max_op: 20, seq: 1 }); clock.include(2, ClockData { max_op: 10, seq: 2 }); - assert!(clock.covers(&OpId(10, 1))); - assert!(clock.covers(&OpId(20, 1))); - assert!(!clock.covers(&OpId(30, 1))); + assert!(clock.covers(&OpId::new(10, 1))); + assert!(clock.covers(&OpId::new(20, 1))); + assert!(!clock.covers(&OpId::new(30, 1))); - assert!(clock.covers(&OpId(5, 2))); - assert!(clock.covers(&OpId(10, 2))); - assert!(!clock.covers(&OpId(15, 2))); + assert!(clock.covers(&OpId::new(5, 2))); + assert!(clock.covers(&OpId::new(10, 2))); + assert!(!clock.covers(&OpId::new(15, 2))); - assert!(!clock.covers(&OpId(1, 3))); - assert!(!clock.covers(&OpId(100, 3))); + assert!(!clock.covers(&OpId::new(1, 3))); + assert!(!clock.covers(&OpId::new(100, 3))); } #[test] diff --git a/rust/automerge/src/columnar/column_range/key.rs b/rust/automerge/src/columnar/column_range/key.rs index 5283fc392..70ea8e1e0 100644 --- a/rust/automerge/src/columnar/column_range/key.rs +++ b/rust/automerge/src/columnar/column_range/key.rs @@ -167,11 +167,11 @@ impl<'a> KeyIter<'a> { Ok(Some(Key::Prop(string))) } (Some(None) | None, Some(Some(0)), Some(None) | None) => { - Ok(Some(Key::Elem(ElemId(OpId(0, 0))))) + Ok(Some(Key::Elem(ElemId(OpId::new(0, 0))))) } (Some(Some(actor)), Some(Some(ctr)), Some(None) | None) => match ctr.try_into() { //Ok(ctr) => Some(Ok(Key::Elem(ElemId(OpId(ctr, actor as usize))))), - Ok(ctr) => Ok(Some(Key::Elem(ElemId(OpId::new(actor as usize, ctr))))), + Ok(ctr) => Ok(Some(Key::Elem(ElemId(OpId::new(ctr, actor as usize))))), Err(_) => Err(DecodeColumnError::invalid_value( "counter", "negative value for counter", diff --git a/rust/automerge/src/columnar/column_range/obj_id.rs b/rust/automerge/src/columnar/column_range/obj_id.rs index f6525b445..6a3e2ef0c 100644 --- a/rust/automerge/src/columnar/column_range/obj_id.rs +++ b/rust/automerge/src/columnar/column_range/obj_id.rs @@ -133,7 +133,7 @@ impl<'a> ObjIdIter<'a> { .map_err(|e| DecodeColumnError::decode_raw("counter", e))?; match (actor, counter) { (None | Some(None), None | Some(None)) => Ok(Some(ObjId::root())), - (Some(Some(a)), Some(Some(c))) => Ok(Some(ObjId(OpId(c, a as usize)))), + (Some(Some(a)), Some(Some(c))) => Ok(Some(ObjId(OpId::new(c, a as usize)))), (_, Some(Some(0))) => Ok(Some(ObjId::root())), (Some(None) | None, _) => Err(DecodeColumnError::unexpected_null("actor")), (_, Some(None) | None) => Err(DecodeColumnError::unexpected_null("counter")), diff --git a/rust/automerge/src/columnar/column_range/opid.rs b/rust/automerge/src/columnar/column_range/opid.rs index 592f6041d..ae95d758b 100644 --- a/rust/automerge/src/columnar/column_range/opid.rs +++ b/rust/automerge/src/columnar/column_range/opid.rs @@ -105,7 +105,7 @@ impl<'a> OpIdIter<'a> { .map_err(|e| DecodeColumnError::decode_raw("counter", e))?; match (actor, counter) { (Some(Some(a)), Some(Some(c))) => match c.try_into() { - Ok(c) => Ok(Some(OpId(c, a as usize))), + Ok(c) => Ok(Some(OpId::new(c, a as usize))), Err(_) => Err(DecodeColumnError::invalid_value( "counter", "negative value encountered", diff --git a/rust/automerge/src/columnar/column_range/opid_list.rs b/rust/automerge/src/columnar/column_range/opid_list.rs index 03b92ccf2..12279c08f 100644 --- a/rust/automerge/src/columnar/column_range/opid_list.rs +++ b/rust/automerge/src/columnar/column_range/opid_list.rs @@ -203,7 +203,7 @@ impl<'a> OpIdListIter<'a> { .map_err(|e| DecodeColumnError::decode_raw("counter", e))?; match (actor, counter) { (Some(Some(a)), Some(Some(ctr))) => match ctr.try_into() { - Ok(ctr) => p.push(OpId(ctr, a as usize)), + Ok(ctr) => p.push(OpId::new(ctr, a as usize)), Err(_e) => { return Err(DecodeColumnError::invalid_value( "counter", diff --git a/rust/automerge/src/columnar/encoding/properties.rs b/rust/automerge/src/columnar/encoding/properties.rs index a6345cadb..a3bf1ed0a 100644 --- a/rust/automerge/src/columnar/encoding/properties.rs +++ b/rust/automerge/src/columnar/encoding/properties.rs @@ -139,7 +139,7 @@ pub(crate) fn option_splice_scenario< } pub(crate) fn opid() -> impl Strategy + Clone { - (0..(i64::MAX as usize), 0..(i64::MAX as u64)).prop_map(|(actor, ctr)| OpId(ctr, actor)) + (0..(i64::MAX as usize), 0..(i64::MAX as u64)).prop_map(|(actor, ctr)| OpId::new(ctr, actor)) } pub(crate) fn elemid() -> impl Strategy + Clone { diff --git a/rust/automerge/src/op_set.rs b/rust/automerge/src/op_set.rs index 09bc256a3..1f5a4486a 100644 --- a/rust/automerge/src/op_set.rs +++ b/rust/automerge/src/op_set.rs @@ -55,7 +55,11 @@ impl OpSetInternal { if id == types::ROOT { ExId::Root } else { - ExId::Id(id.0, self.m.actors.cache[id.1].clone(), id.1) + ExId::Id( + id.counter(), + self.m.actors.cache[id.actor()].clone(), + id.actor(), + ) } } @@ -355,13 +359,7 @@ impl OpSetMetadata { } pub(crate) fn lamport_cmp(&self, left: OpId, right: OpId) -> Ordering { - match (left, right) { - (OpId(0, _), OpId(0, _)) => Ordering::Equal, - (OpId(0, _), OpId(_, _)) => Ordering::Less, - (OpId(_, _), OpId(0, _)) => Ordering::Greater, - (OpId(a, x), OpId(b, y)) if a == b => self.actors[x].cmp(&self.actors[y]), - (OpId(a, _), OpId(b, _)) => a.cmp(&b), - } + left.lamport_cmp(&right, &self.actors.cache) } pub(crate) fn sorted_opids>(&self, opids: I) -> OpIds { diff --git a/rust/automerge/src/op_tree.rs b/rust/automerge/src/op_tree.rs index 909a75a78..7de00dc32 100644 --- a/rust/automerge/src/op_tree.rs +++ b/rust/automerge/src/op_tree.rs @@ -325,7 +325,7 @@ mod tests { use super::*; fn op() -> Op { - let zero = OpId(0, 0); + let zero = OpId::new(0, 0); Op { id: zero, action: amp::OpType::Put(0.into()), diff --git a/rust/automerge/src/op_tree/iter.rs b/rust/automerge/src/op_tree/iter.rs index 5f2114c89..0b19f359e 100644 --- a/rust/automerge/src/op_tree/iter.rs +++ b/rust/automerge/src/op_tree/iter.rs @@ -262,7 +262,7 @@ mod tests { fn op(counter: u64) -> Op { Op { action: OpType::Put(ScalarValue::Uint(counter)), - id: OpId(counter, 0), + id: OpId::new(counter, 0), key: Key::Map(0), succ: Default::default(), pred: Default::default(), diff --git a/rust/automerge/src/transaction/inner.rs b/rust/automerge/src/transaction/inner.rs index c9567b68d..2099acef0 100644 --- a/rust/automerge/src/transaction/inner.rs +++ b/rust/automerge/src/transaction/inner.rs @@ -240,7 +240,7 @@ impl TransactionInner { } fn next_id(&mut self) -> OpId { - OpId(self.start_op.get() + self.pending_ops() as u64, self.actor) + OpId::new(self.start_op.get() + self.pending_ops() as u64, self.actor) } fn next_insert(&mut self, key: Key, value: ScalarValue) -> Op { diff --git a/rust/automerge/src/types.rs b/rust/automerge/src/types.rs index b5da60d70..7bbf4353f 100644 --- a/rust/automerge/src/types.rs +++ b/rust/automerge/src/types.rs @@ -3,10 +3,12 @@ use crate::legacy as amp; use serde::{Deserialize, Serialize}; use std::borrow::Cow; use std::cmp::Eq; +use std::cmp::Ordering; use std::fmt; use std::fmt::Display; use std::str::FromStr; use tinyvec::{ArrayVec, TinyVec}; +//use crate::indexed_cache::IndexedCache; mod opids; pub(crate) use opids::OpIds; @@ -253,17 +255,6 @@ pub(crate) trait Exportable { fn export(&self) -> Export; } -impl OpId { - #[inline] - pub(crate) fn counter(&self) -> u64 { - self.0 - } - #[inline] - pub(crate) fn actor(&self) -> usize { - self.1 - } -} - impl Exportable for ObjId { fn export(&self) -> Export { if self.0 == ROOT { @@ -421,11 +412,28 @@ impl Key { } #[derive(Debug, Clone, PartialOrd, Ord, Eq, PartialEq, Copy, Hash, Default)] -pub(crate) struct OpId(pub(crate) u64, pub(crate) usize); +pub(crate) struct OpId(u32, u32); impl OpId { - pub(crate) fn new(actor: usize, counter: u64) -> Self { - Self(counter, actor) + pub(crate) fn new(counter: u64, actor: usize) -> Self { + Self(counter as u32, actor as u32) + } + + #[inline] + pub(crate) fn counter(&self) -> u64 { + self.0 as u64 + } + + #[inline] + pub(crate) fn actor(&self) -> usize { + self.1 as usize + } + + #[inline] + pub(crate) fn lamport_cmp(&self, other: &OpId, actors: &[ActorId]) -> Ordering { + self.0 + .cmp(&other.0) + .then_with(|| actors[self.1 as usize].cmp(&actors[other.1 as usize])) } } diff --git a/rust/automerge/src/types/opids.rs b/rust/automerge/src/types/opids.rs index 3ebac93c5..eaeed471b 100644 --- a/rust/automerge/src/types/opids.rs +++ b/rust/automerge/src/types/opids.rs @@ -129,7 +129,8 @@ mod tests { fn gen_opid(actors: Vec) -> impl Strategy { (0..actors.len()).prop_flat_map(|actor_idx| { - (Just(actor_idx), 0..u64::MAX).prop_map(|(actor_idx, counter)| OpId(counter, actor_idx)) + (Just(actor_idx), 0..u64::MAX) + .prop_map(|(actor_idx, counter)| OpId::new(counter, actor_idx)) }) } @@ -190,7 +191,7 @@ mod tests { (OpId(0, _), OpId(0, _)) => Ordering::Equal, (OpId(0, _), OpId(_, _)) => Ordering::Less, (OpId(_, _), OpId(0, _)) => Ordering::Greater, - (OpId(a, x), OpId(b, y)) if a == b => actors[*x].cmp(&actors[*y]), + (OpId(a, x), OpId(b, y)) if a == b => actors[*x as usize].cmp(&actors[*y as usize]), (OpId(a, _), OpId(b, _)) => a.cmp(b), } }