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
This commit is contained in:
Zixuan Chen 2024-01-03 21:57:31 +08:00 committed by GitHub
parent 8be7cf0412
commit 13a3078408
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
9 changed files with 185 additions and 64 deletions

View file

@ -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<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();
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<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();
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,

View file

@ -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)]

View file

@ -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<F, R>(&self, idx: ContainerIdx, f: F) -> R
pub(crate) fn with_state<F, R>(&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<ContainerID, (ContainerIdx, LoroValue)> = 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<ContainerID, (ContainerIdx, LoroValue)> = 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)),
}
}

View file

@ -26,6 +26,7 @@ use loro_common::{IdSpan, LoroResult, ID};
#[derive(Debug)]
pub struct ListState {
id: ContainerID,
idx: ContainerIdx,
list: BTree<ListImpl>,
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<ListImpl> 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)
}

View file

@ -24,6 +24,7 @@ use super::ContainerState;
#[derive(Debug, Clone)]
pub struct MapState {
id: ContainerID,
idx: ContainerIdx,
map: FxHashMap<InternalString, MapValue>,
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,

View file

@ -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<LazyLoad<RichtextStateLoader, InnerState>>,
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(),

View file

@ -28,6 +28,7 @@ use super::ContainerState;
/// using flat representation
#[derive(Debug, Clone)]
pub struct TreeState {
id: ContainerID,
idx: ContainerIdx,
pub(crate) trees: FxHashMap<TreeID, TreeStateNode>,
pub(crate) deleted: FxHashSet<TreeID>,
@ -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();

View file

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

View file

@ -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();
}