From 62d4c9ca5dd482401a517cb1084270d26050e40b Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Thu, 8 Aug 2024 14:57:11 +0800 Subject: [PATCH] fix: add importGreaterUpdates variants to diffmode --- crates/fuzz/tests/test.rs | 52 ++++++++++++++++++- crates/loro-internal/src/dag.rs | 29 ++++++++--- crates/loro-internal/src/diff_calc.rs | 27 ++++++---- crates/loro-internal/src/handler.rs | 9 ++-- crates/loro-internal/src/oplog.rs | 6 ++- .../src/state/movable_list_state.rs | 3 +- 6 files changed, 102 insertions(+), 24 deletions(-) diff --git a/crates/fuzz/tests/test.rs b/crates/fuzz/tests/test.rs index a2a16f82..781b57e7 100644 --- a/crates/fuzz/tests/test.rs +++ b/crates/fuzz/tests/test.rs @@ -8102,7 +8102,7 @@ fn unknown_fuzz_err_1() { } #[test] -fn text_fuzz_err_1() { +fn diff_calc_fuzz_err_1() { test_multi_sites( 5, vec![FuzzTarget::All], @@ -8190,6 +8190,56 @@ fn text_fuzz_err_1() { ) } +#[test] +fn diff_calc_fuzz_err_2() { + test_multi_sites( + 5, + vec![FuzzTarget::All], + &mut [ + Handle { + site: 4, + target: 0, + container: 0, + action: Generic(GenericAction { + value: I32(-2105409536), + bool: false, + key: 4294967295, + pos: 137975431167, + length: 458752, + prop: 360287970189639680, + }), + }, + Handle { + site: 255, + target: 255, + container: 255, + action: Generic(GenericAction { + value: Container(Text), + bool: false, + key: 0, + pos: 12948890936913428480, + length: 12948890938015724467, + prop: 12948890938015724467, + }), + }, + Sync { from: 179, to: 179 }, + Handle { + site: 0, + target: 0, + container: 0, + action: Generic(GenericAction { + value: I32(825307392), + bool: true, + key: 825307441, + pos: 17361641481138352433, + length: 18302628885800892209, + prop: 65534, + }), + }, + ], + ) +} + #[test] fn minify() { minify_error( diff --git a/crates/loro-internal/src/dag.rs b/crates/loro-internal/src/dag.rs index 36cf37cc..75ade2bf 100644 --- a/crates/loro-internal/src/dag.rs +++ b/crates/loro-internal/src/dag.rs @@ -313,10 +313,6 @@ where D: DagNode + 'a, F: Fn(ID) -> Option<&'a D>, { - if a_id.is_empty() { - return (Default::default(), DiffMode::Import); - } - if b_id.is_empty() { return (Default::default(), DiffMode::Checkout); } @@ -486,6 +482,27 @@ where D: DagNode + 'a, F: Fn(ID) -> Option<&'a D>, { + if right.is_empty() { + return (Default::default(), DiffMode::Checkout); + } + + if left.is_empty() { + if right.len() == 1 { + let mut node_id = right[0]; + let mut node = get(node_id).unwrap(); + while node.deps().len() == 1 { + node_id = node.deps()[0]; + node = get(node_id).unwrap(); + } + + if node.deps().is_empty() { + return (Default::default(), DiffMode::Linear); + } + } + + return (Default::default(), DiffMode::ImportGreaterUpdates); + } + if left.len() == 1 && right.len() == 1 { let left = left[0]; let right = right[0]; @@ -510,7 +527,7 @@ where } } - let mut is_linear = left.len() == 1 && right.len() == 1; + let mut is_linear = left.len() <= 1 && right.len() == 1; let mut is_right_greater = true; let mut ans: Frontiers = Default::default(); let mut queue: BinaryHeap<(SmallVec<[OrdIdSpan; 1]>, NodeType)> = BinaryHeap::new(); @@ -609,7 +626,7 @@ where debug_assert!(ans.len() <= 1); DiffMode::Linear } else { - DiffMode::Import + DiffMode::ImportGreaterUpdates } } else { DiffMode::Checkout diff --git a/crates/loro-internal/src/diff_calc.rs b/crates/loro-internal/src/diff_calc.rs index cfaabb05..1b8332ed 100644 --- a/crates/loro-internal/src/diff_calc.rs +++ b/crates/loro-internal/src/diff_calc.rs @@ -12,8 +12,7 @@ use itertools::Itertools; use enum_dispatch::enum_dispatch; use fxhash::{FxHashMap, FxHashSet}; use loro_common::{ - CompactIdLp, ContainerID, Counter, HasCounterSpan, HasIdSpan, IdFull, IdLp, IdSpan, LoroValue, - PeerID, ID, + CompactIdLp, ContainerID, Counter, HasCounterSpan, IdFull, IdLp, IdSpan, LoroValue, PeerID, ID, }; use loro_delta::DeltaRope; use smallvec::SmallVec; @@ -84,14 +83,21 @@ pub(crate) enum DiffMode { /// For example, we may need to compare the current register's lamport with the update's lamport to decide /// what's the new value. /// - /// It is faster than the checkout mode, but it is still slower than the linear mode. + /// It has stricter requirements than `Checkout`: /// - /// - The difference between the import mode and the checkout mode: in import mode, target version > current version. - /// So when calculating the `DiffCalculator` doesn't need to rely on `ContainerHistoryCache`, except for the Tree container. - /// - The difference between the import mode and the linear mode: in linear mode, all the imported updates are ordered, no concurrent update exists. - /// So there is no need to build CRDTs for the calculation + /// - The target version vector must be greater than the current version vector. Import, + /// This mode is used when the user imports new updates and all the updates are guaranteed to greater than the current version. + /// + /// It has stricter requirements than `Import`. + /// - All the updates are greater than the current version. No update is concurrent to the current version. + /// - So LCA is always the `from` version + ImportGreaterUpdates, /// This mode is used when we don't need to build CRDTs to calculate the difference. It is the fastest mode. + /// + /// It has stricter requirements than `ImportGreaterUpdates`. + /// - In `ImportGreaterUpdates`, all the updates are guaranteed to be greater than the current version. + /// - In `Linear`, all the updates are ordered, no concurrent update exists. Linear, } @@ -322,7 +328,6 @@ impl DiffCalculator { if d != *depth { *depth = d; all.push((*depth, container_idx)); - calc.finish_this_round(); continue; } } @@ -530,7 +535,7 @@ impl DiffCalculatorTrait for MapDiffCalculator { mut on_new_container: impl FnMut(&ContainerID), ) -> (InternalDiff, DiffMode) { match self.current_mode { - DiffMode::Checkout => { + DiffMode::Checkout | DiffMode::Import => { let mut changed = Vec::new(); let group = oplog .history_cache @@ -580,7 +585,7 @@ impl DiffCalculatorTrait for MapDiffCalculator { (InternalDiff::Map(MapDelta { updated }), DiffMode::Checkout) } - DiffMode::Import | DiffMode::Linear => { + DiffMode::ImportGreaterUpdates | DiffMode::Linear => { let changed = std::mem::take(&mut self.changed); let mode = self.current_mode; // Reset this field to avoid we use `has_all` to cache the diff calc and use it next round @@ -1331,7 +1336,7 @@ impl DiffCalculatorTrait for MovableListDiffCalculator { }; assert_eq!(diff_mode, DiffMode::Checkout); - let is_checkout = matches!(self.current_mode, DiffMode::Checkout); + let is_checkout = matches!(self.current_mode, DiffMode::Checkout | DiffMode::Import); let mut element_changes: FxHashMap = if is_checkout { FxHashMap::default() } else { diff --git a/crates/loro-internal/src/handler.rs b/crates/loro-internal/src/handler.rs index 8966a305..601efcc7 100644 --- a/crates/loro-internal/src/handler.rs +++ b/crates/loro-internal/src/handler.rs @@ -32,7 +32,7 @@ use std::{ sync::{Arc, Mutex, Weak}, }; -use tracing::{debug, error, info, instrument}; +use tracing::{debug, error, info, instrument, trace}; mod tree; pub use tree::TreeHandler; @@ -1066,6 +1066,7 @@ impl Handler { diff: Diff, container_remap: &mut FxHashMap, ) -> LoroResult<()> { + trace!("apply_diff: {:?}", &diff); let on_container_remap = &mut |old_id, new_id| { container_remap.insert(old_id, new_id); }; @@ -1689,10 +1690,10 @@ impl TextHandler { } PosType::Bytes => { if pos + len > self.len_utf8() { - error!("pos={} len={} len_event={}", pos, len, self.len_event()); + error!("pos={} len={} len_bytes={}", pos, len, self.len_utf8()); return Err(LoroError::OutOfBound { pos: pos + len, - len: self.len_event(), + len: self.len_utf8(), info: format!("Position: {}:{}", file!(), line!()).into_boxed_str(), }); } @@ -3793,7 +3794,7 @@ pub mod counter { mod test { use super::{HandlerTrait, TextDelta}; - + use crate::loro::LoroDoc; use crate::version::Frontiers; use crate::{fx_map, ToJson}; diff --git a/crates/loro-internal/src/oplog.rs b/crates/loro-internal/src/oplog.rs index 13360042..53c2e881 100644 --- a/crates/loro-internal/src/oplog.rs +++ b/crates/loro-internal/src/oplog.rs @@ -554,8 +554,12 @@ impl OpLog { } }; - let (common_ancestors, diff_mode) = + let (common_ancestors, mut diff_mode) = self.dag.find_common_ancestor(from_frontiers, to_frontiers); + if diff_mode == DiffMode::Checkout && to > from { + diff_mode = DiffMode::Import; + } + let common_ancestors_vv = self.dag.frontiers_to_vv(&common_ancestors).unwrap(); // go from lca to merged_vv let diff = common_ancestors_vv.diff(&merged_vv).right; diff --git a/crates/loro-internal/src/state/movable_list_state.rs b/crates/loro-internal/src/state/movable_list_state.rs index 709104da..841c61d9 100644 --- a/crates/loro-internal/src/state/movable_list_state.rs +++ b/crates/loro-internal/src/state/movable_list_state.rs @@ -1097,7 +1097,8 @@ impl ContainerState for MovableListState { match self.inner.elements().get(&elem_id).cloned() { Some(elem) => { // Update value if needed - if elem.value != value + if value_id.is_some() + && elem.value != value && (!need_compare || elem.value_id < value_id.unwrap()) { maybe_moved.remove(&elem_id);