From 13a307840852dc1b0776466a6662b474aeae6f54 Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Wed, 3 Jan 2024 21:57:31 +0800 Subject: [PATCH] Refactor container states and add consistency checks (#237) * refactor: make container states can get their ids * test: add utils to test consistency between the current states and diff calc * test: add state diff calc consistency check --- crates/loro-internal/src/handler.rs | 18 +-- crates/loro-internal/src/loro.rs | 19 +++ crates/loro-internal/src/state.rs | 124 +++++++++++++----- crates/loro-internal/src/state/list_state.rs | 17 ++- crates/loro-internal/src/state/map_state.rs | 8 +- .../loro-internal/src/state/richtext_state.rs | 11 +- crates/loro-internal/src/state/tree_state.rs | 32 +++-- crates/loro-internal/tests/autocommit.rs | 1 + crates/loro-internal/tests/richtext.rs | 19 +++ 9 files changed, 185 insertions(+), 64 deletions(-) diff --git a/crates/loro-internal/src/handler.rs b/crates/loro-internal/src/handler.rs index 5132cb5e..0cbe87b8 100644 --- a/crates/loro-internal/src/handler.rs +++ b/crates/loro-internal/src/handler.rs @@ -773,7 +773,7 @@ impl ListHandler { pub fn get_child_handler(&self, index: usize) -> Handler { let mutex = &self.state.upgrade().unwrap(); - let state = mutex.lock().unwrap(); + let mut state = mutex.lock().unwrap(); let container_id = state.with_state(self.container_idx, |state| { state .as_list_state() @@ -857,13 +857,14 @@ impl ListHandler { /// Get value at given index, if it's a container, return a handler to the container pub fn get_(&self, index: usize) -> Option { let mutex = &self.state.upgrade().unwrap(); - let doc_state = &mutex.lock().unwrap(); + let doc_state = &mut mutex.lock().unwrap(); + let arena = doc_state.arena.clone(); doc_state.with_state(self.container_idx, |state| { let a = state.as_list_state().unwrap(); match a.get(index) { Some(v) => { if let LoroValue::Container(id) = v { - let idx = doc_state.arena.register_container(id); + let idx = arena.register_container(id); Some(ValueOrContainer::Container(Handler::new( self.txn.clone(), idx, @@ -883,7 +884,7 @@ impl ListHandler { I: FnMut(ValueOrContainer), { let mutex = &self.state.upgrade().unwrap(); - let doc_state = &mutex.lock().unwrap(); + let doc_state = &mut mutex.lock().unwrap(); let arena = doc_state.arena.clone(); doc_state.with_state(self.container_idx, |state| { let a = state.as_list_state().unwrap(); @@ -1016,7 +1017,7 @@ impl MapHandler { I: FnMut(&str, ValueOrContainer), { let mutex = &self.state.upgrade().unwrap(); - let doc_state = mutex.lock().unwrap(); + let mut doc_state = mutex.lock().unwrap(); let arena = doc_state.arena.clone(); doc_state.with_state(self.container_idx, |state| { let a = state.as_map_state().unwrap(); @@ -1053,7 +1054,7 @@ impl MapHandler { pub fn get_child_handler(&self, key: &str) -> Handler { let mutex = &self.state.upgrade().unwrap(); - let state = mutex.lock().unwrap(); + let mut state = mutex.lock().unwrap(); let container_id = state.with_state(self.container_idx, |state| { state .as_map_state() @@ -1102,13 +1103,14 @@ impl MapHandler { /// Get the value at given key, if value is a container, return a handler to the container pub fn get_(&self, key: &str) -> Option { let mutex = &self.state.upgrade().unwrap(); - let doc_state = mutex.lock().unwrap(); + let mut doc_state = mutex.lock().unwrap(); + let arena = doc_state.arena.clone(); doc_state.with_state(self.container_idx, |state| { let a = state.as_map_state().unwrap(); let value = a.get(key); match value { Some(LoroValue::Container(container_id)) => { - let idx = doc_state.arena.register_container(container_id); + let idx = arena.register_container(container_id); Some(ValueOrContainer::Container(Handler::new( self.txn.clone(), idx, diff --git a/crates/loro-internal/src/loro.rs b/crates/loro-internal/src/loro.rs index 13960ea6..6008f05d 100644 --- a/crates/loro-internal/src/loro.rs +++ b/crates/loro-internal/src/loro.rs @@ -727,6 +727,25 @@ impl LoroDoc { let oplog = self.oplog.lock().unwrap(); oplog.len_changes() } + + /// This method compare the consistency between the current doc state + /// and the state calculated by diff calculator from beginning. + /// + /// Panic when it's not consistent + pub fn check_state_diff_calc_consistency_slow(&self) { + self.commit_then_stop(); + assert!( + !self.is_detached(), + "Cannot check consistency in detached mode" + ); + let bytes = self.export_from(&Default::default()); + let doc = Self::new(); + doc.import(&bytes).unwrap(); + let mut calculated_state = doc.app_state().try_lock().unwrap(); + let mut current_state = self.app_state().try_lock().unwrap(); + current_state.check_is_the_same(&mut calculated_state); + self.renew_txn_if_auto_commit(); + } } #[cfg(test)] diff --git a/crates/loro-internal/src/state.rs b/crates/loro-internal/src/state.rs index b46a6f37..b5c958c4 100644 --- a/crates/loro-internal/src/state.rs +++ b/crates/loro-internal/src/state.rs @@ -57,6 +57,7 @@ pub struct DocState { #[enum_dispatch] pub(crate) trait ContainerState: Clone { fn container_idx(&self) -> ContainerIdx; + fn container_id(&self) -> &ContainerID; fn is_state_empty(&self) -> bool; @@ -131,19 +132,16 @@ pub enum State { } impl State { - #[allow(unused)] - pub fn new_list(idx: ContainerIdx) -> Self { - Self::ListState(ListState::new(idx)) + pub fn new_list(id: ContainerID, idx: ContainerIdx) -> Self { + Self::ListState(ListState::new(id, idx)) } - #[allow(unused)] - pub fn new_map(idx: ContainerIdx) -> Self { - Self::MapState(MapState::new(idx)) + pub fn new_map(id: ContainerID, idx: ContainerIdx) -> Self { + Self::MapState(MapState::new(id, idx)) } - #[allow(unused)] - pub fn new_richtext(idx: ContainerIdx) -> Self { - Self::RichtextState(RichtextState::new(idx)) + pub fn new_richtext(id: ContainerID, idx: ContainerIdx) -> Self { + Self::RichtextState(RichtextState::new(id, idx)) } } @@ -307,10 +305,10 @@ impl DocState { diff.bring_back = true; } if diff.bring_back { - let state = self - .states - .entry(diff.idx) - .or_insert_with(|| create_state(idx)); + let state = self.states.entry(diff.idx).or_insert_with(|| { + let id = self.arena.idx_to_id(idx).unwrap(); + create_state(id, idx) + }); let state_diff = state.to_diff(&self.arena, &self.global_txn, &self.weak_state); if diff.diff.is_none() && state_diff.is_empty() { // empty diff, skip it @@ -346,7 +344,10 @@ impl DocState { continue; }; let idx = diff.idx; - let state = self.states.entry(idx).or_insert_with(|| create_state(idx)); + let state = self.states.entry(idx).or_insert_with(|| { + let id = self.arena.idx_to_id(idx).unwrap(); + create_state(id, idx) + }); if self.in_txn { state.start_txn(); @@ -395,10 +396,10 @@ impl DocState { } pub fn apply_local_op(&mut self, raw_op: &RawOp, op: &Op) -> LoroResult<()> { - let state = self - .states - .entry(op.container) - .or_insert_with(|| create_state(op.container)); + let state = self.states.entry(op.container).or_insert_with(|| { + let id = self.arena.idx_to_id(op.container).unwrap(); + create_state(id, op.container) + }); if self.in_txn { state.start_txn(); @@ -428,7 +429,10 @@ impl DocState { decode_ctx: StateSnapshotDecodeContext, ) { let idx = self.arena.register_container(&cid); - let state = self.states.entry(idx).or_insert_with(|| create_state(idx)); + let state = self.states.entry(idx).or_insert_with(|| { + let id = self.arena.idx_to_id(idx).unwrap(); + create_state(id, idx) + }); state.import_from_snapshot_ops(decode_ctx); } @@ -527,27 +531,30 @@ impl DocState { id: I, ) -> Option<&mut richtext_state::RichtextState> { let id: ContainerIdRaw = id.into(); + let cid; let idx = match id { - ContainerIdRaw::Root { name } => Some(self.arena.register_container( - &crate::container::ContainerID::Root { + ContainerIdRaw::Root { name } => { + cid = crate::container::ContainerID::Root { name, container_type: crate::ContainerType::Text, - }, - )), - ContainerIdRaw::Normal { id: _ } => self - .arena - .id_to_idx(&id.with_type(crate::ContainerType::Text)), + }; + Some(self.arena.register_container(&cid)) + } + ContainerIdRaw::Normal { id: _ } => { + cid = id.with_type(crate::ContainerType::Text); + self.arena.id_to_idx(&cid) + } }; let idx = idx.unwrap(); self.states .entry(idx) - .or_insert_with(|| State::new_richtext(idx)) + .or_insert_with(|| State::new_richtext(cid, idx)) .as_richtext_state_mut() } #[inline(always)] - pub(crate) fn with_state(&self, idx: ContainerIdx, f: F) -> R + pub(crate) fn with_state(&mut self, idx: ContainerIdx, f: F) -> R where F: FnOnce(&State) -> R, { @@ -555,7 +562,11 @@ impl DocState { if let Some(state) = state { f(state) } else { - f(&create_state(idx)) + let id = self.arena.idx_to_id(idx).unwrap(); + let state = create_state(id, idx); + let ans = f(&state); + self.states.insert(idx, state); + ans } } @@ -568,7 +579,11 @@ impl DocState { if let Some(state) = state { f(state) } else { - f(&mut create_state(idx)) + let id = self.arena.idx_to_id(idx).unwrap(); + let mut state = create_state(id, idx); + let ans = f(&mut state); + self.states.insert(idx, state); + ans } } @@ -849,6 +864,45 @@ impl DocState { Ok(()) } + + /// Check whether two [DocState]s are the same. Panic if not. + /// + /// This is only used for test. + pub(crate) fn check_is_the_same(&mut self, other: &mut Self) { + let self_id_to_states: FxHashMap = self + .states + .values_mut() + .map(|state| { + let id = state.container_id().clone(); + (id, (state.container_idx(), state.get_value())) + }) + .collect(); + let mut other_id_to_states: FxHashMap = other + .states + .values_mut() + .map(|state| { + let id = state.container_id().clone(); + (id, (state.container_idx(), state.get_value())) + }) + .collect(); + + for (id, (idx, value)) in self_id_to_states { + let other_state = other_id_to_states.remove(&id).unwrap_or_else(|| { + panic!("id: {:?}, path: {:?} is missing", id, self.get_path(idx)); + }); + assert_eq!( + value, + other_state.1, + "id: {:?}, path: {:?}", + id, + self.get_path(idx) + ); + } + + if !other_id_to_states.is_empty() { + panic!("other has more states {:#?}", &other_id_to_states); + } + } } struct SubContainerDiffPatch { @@ -942,12 +996,12 @@ impl SubContainerDiffPatch { } } -pub fn create_state(idx: ContainerIdx) -> State { +pub fn create_state(id: ContainerID, idx: ContainerIdx) -> State { match idx.get_type() { - ContainerType::Map => State::MapState(MapState::new(idx)), - ContainerType::List => State::ListState(ListState::new(idx)), - ContainerType::Text => State::RichtextState(RichtextState::new(idx)), - ContainerType::Tree => State::TreeState(TreeState::new(idx)), + ContainerType::Map => State::MapState(MapState::new(id, idx)), + ContainerType::List => State::ListState(ListState::new(id, idx)), + ContainerType::Text => State::RichtextState(RichtextState::new(id, idx)), + ContainerType::Tree => State::TreeState(TreeState::new(id, idx)), } } diff --git a/crates/loro-internal/src/state/list_state.rs b/crates/loro-internal/src/state/list_state.rs index c57e3596..9cebd15a 100644 --- a/crates/loro-internal/src/state/list_state.rs +++ b/crates/loro-internal/src/state/list_state.rs @@ -26,6 +26,7 @@ use loro_common::{IdSpan, LoroResult, ID}; #[derive(Debug)] pub struct ListState { + id: ContainerID, idx: ContainerIdx, list: BTree, in_txn: bool, @@ -36,6 +37,7 @@ pub struct ListState { impl Clone for ListState { fn clone(&self) -> Self { Self { + id: self.id.clone(), idx: self.idx, list: self.list.clone(), in_txn: false, @@ -151,9 +153,10 @@ impl UseLengthFinder for ListImpl { // FIXME: update child_container_to_leaf impl ListState { - pub fn new(idx: ContainerIdx) -> Self { + pub fn new(id: ContainerID, idx: ContainerIdx) -> Self { let tree = BTree::new(); Self { + id, idx, list: tree, in_txn: false, @@ -330,6 +333,10 @@ impl ContainerState for ListState { self.idx } + fn container_id(&self) -> &ContainerID { + &self.id + } + fn is_state_empty(&self) -> bool { self.list.is_empty() } @@ -562,10 +569,10 @@ mod test { #[test] fn test() { - let mut list = ListState::new(ContainerIdx::from_index_and_type( - 0, - loro_common::ContainerType::List, - )); + let mut list = ListState::new( + id("abc"), + ContainerIdx::from_index_and_type(0, loro_common::ContainerType::List), + ); fn id(name: &str) -> ContainerID { ContainerID::new_root(name, crate::ContainerType::List) } diff --git a/crates/loro-internal/src/state/map_state.rs b/crates/loro-internal/src/state/map_state.rs index a8c9d90d..c1acc055 100644 --- a/crates/loro-internal/src/state/map_state.rs +++ b/crates/loro-internal/src/state/map_state.rs @@ -24,6 +24,7 @@ use super::ContainerState; #[derive(Debug, Clone)] pub struct MapState { + id: ContainerID, idx: ContainerIdx, map: FxHashMap, in_txn: bool, @@ -35,6 +36,10 @@ impl ContainerState for MapState { self.idx } + fn container_id(&self) -> &ContainerID { + &self.id + } + fn is_state_empty(&self) -> bool { self.map.is_empty() } @@ -211,8 +216,9 @@ impl ContainerState for MapState { } impl MapState { - pub fn new(idx: ContainerIdx) -> Self { + pub fn new(id: ContainerID, idx: ContainerIdx) -> Self { Self { + id, idx, map: FxHashMap::default(), in_txn: false, diff --git a/crates/loro-internal/src/state/richtext_state.rs b/crates/loro-internal/src/state/richtext_state.rs index c0a059d8..c4603500 100644 --- a/crates/loro-internal/src/state/richtext_state.rs +++ b/crates/loro-internal/src/state/richtext_state.rs @@ -5,7 +5,7 @@ use std::{ use fxhash::FxHashMap; use generic_btree::rle::{HasLength, Mergeable}; -use loro_common::{LoroResult, LoroValue, ID}; +use loro_common::{ContainerID, LoroResult, LoroValue, ID}; use crate::{ arena::SharedArena, @@ -32,6 +32,7 @@ use super::ContainerState; #[derive(Debug)] pub struct RichtextState { + id: ContainerID, idx: ContainerIdx, pub(crate) state: Box>, in_txn: bool, @@ -40,8 +41,9 @@ pub struct RichtextState { impl RichtextState { #[inline] - pub fn new(idx: ContainerIdx) -> Self { + pub fn new(id: ContainerID, idx: ContainerIdx) -> Self { Self { + id, idx, state: Box::new(LazyLoad::new_dst(Default::default())), in_txn: false, @@ -77,6 +79,7 @@ impl RichtextState { impl Clone for RichtextState { fn clone(&self) -> Self { Self { + id: self.id.clone(), idx: self.idx, state: self.state.clone(), in_txn: false, @@ -145,6 +148,10 @@ impl ContainerState for RichtextState { self.idx } + fn container_id(&self) -> &ContainerID { + &self.id + } + fn is_state_empty(&self) -> bool { match &*self.state { LazyLoad::Src(s) => s.is_empty(), diff --git a/crates/loro-internal/src/state/tree_state.rs b/crates/loro-internal/src/state/tree_state.rs index 34aa1929..1604c498 100644 --- a/crates/loro-internal/src/state/tree_state.rs +++ b/crates/loro-internal/src/state/tree_state.rs @@ -28,6 +28,7 @@ use super::ContainerState; /// using flat representation #[derive(Debug, Clone)] pub struct TreeState { + id: ContainerID, idx: ContainerIdx, pub(crate) trees: FxHashMap, pub(crate) deleted: FxHashSet, @@ -68,7 +69,7 @@ struct TreeUndoItem { } impl TreeState { - pub fn new(idx: ContainerIdx) -> Self { + pub fn new(id: ContainerID, idx: ContainerIdx) -> Self { let mut trees = FxHashMap::default(); trees.insert( TreeID::delete_root().unwrap(), @@ -87,6 +88,7 @@ impl TreeState { let mut deleted = FxHashSet::default(); deleted.insert(TreeID::delete_root().unwrap()); Self { + id, idx, trees, deleted, @@ -238,6 +240,10 @@ impl ContainerState for TreeState { self.idx } + fn container_id(&self) -> &ContainerID { + &self.id + } + fn is_state_empty(&self) -> bool { self.trees.is_empty() } @@ -605,20 +611,20 @@ mod tests { #[test] fn test_tree_state() { - let mut state = TreeState::new(ContainerIdx::from_index_and_type( - 0, - loro_common::ContainerType::Tree, - )); + let mut state = TreeState::new( + ContainerID::new_normal(ID::new(0, 0), loro_common::ContainerType::Tree), + ContainerIdx::from_index_and_type(0, loro_common::ContainerType::Tree), + ); state.mov(ID1, None, ID::NONE_ID).unwrap(); state.mov(ID2, Some(ID1), ID::NONE_ID).unwrap(); } #[test] fn tree_convert() { - let mut state = TreeState::new(ContainerIdx::from_index_and_type( - 0, - loro_common::ContainerType::Tree, - )); + let mut state = TreeState::new( + ContainerID::new_normal(ID::new(0, 0), loro_common::ContainerType::Tree), + ContainerIdx::from_index_and_type(0, loro_common::ContainerType::Tree), + ); state.mov(ID1, None, ID::NONE_ID).unwrap(); state.mov(ID2, Some(ID1), ID::NONE_ID).unwrap(); let roots = Forest::from_tree_state(&state.trees); @@ -631,10 +637,10 @@ mod tests { #[test] fn delete_node() { - let mut state = TreeState::new(ContainerIdx::from_index_and_type( - 0, - loro_common::ContainerType::Tree, - )); + let mut state = TreeState::new( + ContainerID::new_normal(ID::new(0, 0), loro_common::ContainerType::Tree), + ContainerIdx::from_index_and_type(0, loro_common::ContainerType::Tree), + ); state.mov(ID1, None, ID::NONE_ID).unwrap(); state.mov(ID2, Some(ID1), ID::NONE_ID).unwrap(); state.mov(ID3, Some(ID2), ID::NONE_ID).unwrap(); diff --git a/crates/loro-internal/tests/autocommit.rs b/crates/loro-internal/tests/autocommit.rs index d9dcb766..e12b8a99 100644 --- a/crates/loro-internal/tests/autocommit.rs +++ b/crates/loro-internal/tests/autocommit.rs @@ -21,6 +21,7 @@ fn auto_commit() { doc_b.import(&bytes).unwrap(); doc_a.import(&doc_b.export_snapshot()).unwrap(); assert_eq!(text_a.get_value(), text_b.get_value()); + doc_a.check_state_diff_calc_consistency_slow(); } #[test] diff --git a/crates/loro-internal/tests/richtext.rs b/crates/loro-internal/tests/richtext.rs index 6837f99e..f5fb81a2 100644 --- a/crates/loro-internal/tests/richtext.rs +++ b/crates/loro-internal/tests/richtext.rs @@ -121,6 +121,8 @@ fn case0() { &doc_a, json!([{"insert":"Hello New World","attributes":{"bold":true}}]), ); + doc_a.check_state_diff_calc_consistency_slow(); + doc_b.check_state_diff_calc_consistency_slow(); } #[test] @@ -134,6 +136,8 @@ fn case1() { &doc_a, json!([{"insert":"Hello World","attributes":{"bold":true}}]), ); + doc_a.check_state_diff_calc_consistency_slow(); + doc_b.check_state_diff_calc_consistency_slow(); } #[test] @@ -148,6 +152,8 @@ fn case2() { &doc_a, json!([{"insert":"Hello a ","attributes":{"bold":false}},{"insert":"World","attributes":{"bold":true}}]), ); + doc_a.check_state_diff_calc_consistency_slow(); + doc_b.check_state_diff_calc_consistency_slow(); } /// | Name | Text | @@ -168,6 +174,8 @@ fn case3() { &doc_a, json!([{"insert":"Hello a "},{"insert":"World","attributes":{"bold":true}}]), ); + doc_a.check_state_diff_calc_consistency_slow(); + doc_b.check_state_diff_calc_consistency_slow(); } /// | Name | Text | @@ -194,6 +202,8 @@ fn case5() { &doc_a, json!([{"insert":"Hey","attributes":{"link":true}},{"insert":" World"}]), ); + doc_a.check_state_diff_calc_consistency_slow(); + doc_b.check_state_diff_calc_consistency_slow(); } /// When insert a new character after "Hello", the new char should be bold but not link @@ -230,6 +240,7 @@ fn case6() { {"insert": " World"} ]), ); + doc.check_state_diff_calc_consistency_slow(); } /// @@ -256,6 +267,8 @@ fn case7() { {"insert":" jumped over the dog."} ]), ); + doc_a.check_state_diff_calc_consistency_slow(); + doc_b.check_state_diff_calc_consistency_slow(); } /// | Name | Text | @@ -283,6 +296,8 @@ fn case8() { {"insert": "."} ]), ); + doc_a.check_state_diff_calc_consistency_slow(); + doc_b.check_state_diff_calc_consistency_slow(); } /// | Name | Text | @@ -307,6 +322,8 @@ fn case9() { {"insert": "."} ]), ); + doc_a.check_state_diff_calc_consistency_slow(); + doc_b.check_state_diff_calc_consistency_slow(); } #[test] @@ -331,4 +348,6 @@ fn insert_after_link() { {"insert": "a fox jumped."}, ]), ); + doc_a.check_state_diff_calc_consistency_slow(); + doc_b.check_state_diff_calc_consistency_slow(); }