From 61c27ca58b8b0ef62cae6fabbb136f56b976ac64 Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Fri, 21 Oct 2022 00:57:35 +0800 Subject: [PATCH] fix: fix a few bugs --- .../src/container/text/text_container.rs | 13 +++++-- .../loro-core/src/container/text/tracker.rs | 4 +- .../src/container/text/tracker/cursor_map.rs | 35 ++++++++++++++---- crates/loro-core/src/dag/iter.rs | 6 ++- crates/loro-core/src/dag/test.rs | 2 +- crates/loro-core/src/log_store.rs | 37 ++++++++++++------- crates/loro-core/tests/test.rs | 3 +- crates/rle/src/rle_tree/cursor.rs | 10 ++++- 8 files changed, 77 insertions(+), 33 deletions(-) diff --git a/crates/loro-core/src/container/text/text_container.rs b/crates/loro-core/src/container/text/text_container.rs index 39108e13..4629c4dc 100644 --- a/crates/loro-core/src/container/text/text_container.rs +++ b/crates/loro-core/src/container/text/text_container.rs @@ -1,4 +1,4 @@ -use rle::RleTree; +use rle::{RleTree, Sliceable}; use smallvec::SmallVec; use crate::{ @@ -104,6 +104,7 @@ impl Container for TextContainer { } // TODO: move main logic to tracker module + // TODO: we don't need op proxy, only ids are enough fn apply(&mut self, op: &OpProxy, store: &LogStore) { let new_op_id = op.id_last(); // TODO: may reduce following two into one op @@ -111,7 +112,6 @@ impl Container for TextContainer { let path_to_store_head = store.find_path(&common, store.frontier()); let mut common_vv = store.vv(); common_vv.retreat(&path_to_store_head.right); - let mut latest_head: SmallVec<[ID; 2]> = store.frontier().into(); latest_head.push(new_op_id); if common.is_empty() || !common.iter().all(|x| self.tracker.contains(*x)) { @@ -121,8 +121,13 @@ impl Container for TextContainer { for iter in store.iter_partial(&common, path.right) { self.tracker.retreat(&iter.retreat); self.tracker.forward(&iter.forward); - for op in iter.data.ops.iter() { + // TODO: avoid this clone + let change = iter + .data + .slice(iter.slice.start as usize, iter.slice.end as usize); + for op in change.ops.iter() { if op.container == self.id { + // TODO: convert op to local self.tracker.apply(op.id, &op.content) } } @@ -133,7 +138,6 @@ impl Container for TextContainer { let path = store.find_path(&latest_head, store.frontier()); self.tracker.retreat(&path.left); for effect in self.tracker.iter_effects(path.left) { - dbg!(&effect); match effect { Effect::Del { pos, len } => self.state.delete_range(Some(pos), Some(pos + len)), Effect::Ins { pos, content } => { @@ -156,6 +160,7 @@ impl Container for TextContainer { let content = v.as_ref(); match content { ListSlice::Slice(range) => ans_str.push_str(&self.raw_str.get_str(range)), + ListSlice::RawStr(raw) => ans_str.push_str(raw), _ => unreachable!(), } } diff --git a/crates/loro-core/src/container/text/tracker.rs b/crates/loro-core/src/container/text/tracker.rs index f5221e1e..ef22b039 100644 --- a/crates/loro-core/src/container/text/tracker.rs +++ b/crates/loro-core/src/container/text/tracker.rs @@ -160,10 +160,10 @@ impl Tracker { self.content.update_at_cursors_twice( &[&to_set_as_applied, &to_delete], &mut |v| { - v.status.apply(StatusChange::SetAsFuture); + v.status.apply(StatusChange::SetAsCurrent); }, &mut |v: &mut YSpan| { - v.status.apply(StatusChange::UndoDelete); + v.status.apply(StatusChange::Delete); }, &mut make_notify(&mut self.id_to_cursor), ) diff --git a/crates/loro-core/src/container/text/tracker/cursor_map.rs b/crates/loro-core/src/container/text/tracker/cursor_map.rs index 75b871aa..064cb998 100644 --- a/crates/loro-core/src/container/text/tracker/cursor_map.rs +++ b/crates/loro-core/src/container/text/tracker/cursor_map.rs @@ -1,4 +1,5 @@ use std::{ + borrow::Cow, fmt::Debug, ops::{Deref, DerefMut}, ptr::NonNull, @@ -11,8 +12,12 @@ use rle::{ rle_tree::{node::LeafNode, Position, SafeCursor, SafeCursorMut, UnsafeCursor}, HasLength, Mergable, RleVec, Sliceable, ZeroElement, }; +use smallvec::SmallVec; -use crate::{id::ID, span::IdSpan}; +use crate::{ + id::ID, + span::{HasId, IdSpan}, +}; use super::y_span::{YSpan, YSpanTreeTrait}; @@ -62,6 +67,16 @@ impl Marker { } } + pub fn get_first_span( + &self, + id_span: IdSpan, + ) -> Option> { + let mut ans = self.get_spans(id_span); + // SAFETY: inner invariants ensures that the cursor is valid + ans.sort_by_cached_key(|x| unsafe { x.as_ref() }.id.counter); + ans.into_iter().next() + } + pub fn get_spans(&self, id_span: IdSpan) -> Vec> { match self { Marker::Insert { ptr, len: _ } => { @@ -215,9 +230,10 @@ pub(super) struct IdSpanQueryResult<'a> { } #[derive(EnumAsInner)] -pub enum FirstCursorResult<'a> { +pub enum FirstCursorResult { + // TODO: REMOVE id field? Ins(ID, UnsafeCursor<'static, YSpan, YSpanTreeTrait>), - Del(ID, &'a RleVec), + Del(ID, RleVec), } impl CursorMap { @@ -249,16 +265,21 @@ impl CursorMap { } pub fn get_first_cursors_at_id_span(&self, span: IdSpan) -> Option { + // TODO: do we need this index for (id, marker) in self.get_range_with_index(span.min_id().into(), span.end_id().into()) { let id: ID = id.into(); match marker { Marker::Insert { .. } => { - let spans = marker.get_spans(span); - if !spans.is_empty() { - return Some(FirstCursorResult::Ins(id, spans[0])); + if let Some(cursor) = marker.get_first_span(span) { + return Some(FirstCursorResult::Ins(span.id_start(), cursor)); } } - Marker::Delete(del) => return Some(FirstCursorResult::Del(id, del)), + Marker::Delete(del) => { + return Some(FirstCursorResult::Del( + span.id_start(), + del.slice((span.id_start().counter - id.counter) as usize, del.len()), + )) + } } } diff --git a/crates/loro-core/src/dag/iter.rs b/crates/loro-core/src/dag/iter.rs index 953146d6..1ad8037f 100644 --- a/crates/loro-core/src/dag/iter.rs +++ b/crates/loro-core/src/dag/iter.rs @@ -173,7 +173,9 @@ impl<'a, T: DagNode> Iterator for DagIteratorVV<'a, T> { } } -/// visit every span in the target IdSpanVector +/// Visit every span in the target IdSpanVector. +/// It's guaranteed that the spans are visited in causal order, and each span is visited only once. +/// When visiting a span, we will checkout to the version where the span was created pub(crate) struct DagPartialIter<'a, Dag> { dag: &'a Dag, frontier: SmallVec<[ID; 2]>, @@ -248,7 +250,7 @@ impl<'a, T: DagNode + 'a, D: Dag> Iterator for DagPartialIter<'a, D> { } else { 0 }; - let slice_end = if counter.end > target_span.end { + let slice_end = if counter.end < target_span.end { counter.end - counter.start } else { target_span.end - counter.start diff --git a/crates/loro-core/src/dag/test.rs b/crates/loro-core/src/dag/test.rs index 1e3b6710..1e172e1a 100644 --- a/crates/loro-core/src/dag/test.rs +++ b/crates/loro-core/src/dag/test.rs @@ -635,7 +635,7 @@ mod find_path { #[test] fn proptest_path_large( - interactions in prop::collection::vec(gen_interaction(10), 0..5 * PROPTEST_FACTOR_10 * PROPTEST_FACTOR_10 + 10), + interactions in prop::collection::vec(gen_interaction(10), 0..2 * PROPTEST_FACTOR_10 * PROPTEST_FACTOR_10 + 10), ) { test_find_path(10, interactions)?; } diff --git a/crates/loro-core/src/log_store.rs b/crates/loro-core/src/log_store.rs index 1c80953c..7706ba22 100644 --- a/crates/loro-core/src/log_store.rs +++ b/crates/loro-core/src/log_store.rs @@ -55,6 +55,7 @@ pub type LogStoreWeakRef = Weak>; /// TODO: Refactor we need to move the things about the current state out of LogStore (container, latest_lamport, ..) pub struct LogStore { changes: FxHashMap>, + vv: VersionVector, cfg: Configure, latest_lamport: Lamport, latest_timestamp: Timestamp, @@ -83,6 +84,7 @@ impl LogStore { frontier: Default::default(), container, to_self: x.clone(), + vv: Default::default(), _pin: PhantomPinned, }) }) @@ -102,6 +104,7 @@ impl LogStore { .into_iter() .filter(|x| !self_vv.includes_id(x.last_id())) { + check_import_change_valid(&change); // TODO: cache pending changes assert!(change.deps.iter().all(|x| self_vv.includes_id(*x))); self.apply_remote_change(change) @@ -116,6 +119,9 @@ impl LogStore { let mut changes = self.get_changes_slice(span.id_span()); ans.append(&mut changes); } + for change in ans.iter_mut() { + self.change_to_export_format(change); + } ans } @@ -137,7 +143,7 @@ impl LogStore { } } - fn change_to_export_format(&self, mut change: Change) { + fn change_to_export_format(&self, change: &mut Change) { let container_manager = self.container.read().unwrap(); for op in change.ops.vec_mut().iter_mut() { let container = container_manager.get(op.container.clone()).unwrap(); @@ -200,6 +206,7 @@ impl LogStore { )); self.latest_lamport = lamport + change.len() as u32 - 1; self.latest_timestamp = timestamp; + self.vv.set_end(change.id_end()); self.changes .entry(self.this_client_id) .or_insert_with(RleVec::new) @@ -218,7 +225,7 @@ impl LogStore { } } - // TODO: find a way to remove this clone? + // TODO: find a way to remove this clone? we don't need change in apply method actually let change = self.push_change(change).clone(); // Apply ops. @@ -243,7 +250,7 @@ impl LogStore { self.latest_timestamp = change.timestamp; } - todo!("update vv"); + self.vv.set_end(change.id_end()); } pub(crate) fn get_op_content(&self, id_span: IdSpan) -> Option { @@ -316,15 +323,19 @@ impl Dag for LogStore { } fn vv(&self) -> crate::VersionVector { - self.changes - .iter() - .map(|(client, changes)| { - changes - .vec() - .last() - .map(|x| x.id_last()) - .unwrap_or_else(|| ID::new(*client, 0)) - }) - .collect() + self.vv.clone() + } +} + +fn check_import_change_valid(change: &Change) { + for op in change.ops.iter() { + if let Some((slice, _)) = op + .content + .as_normal() + .and_then(|x| x.as_list()) + .and_then(|x| x.as_insert()) + { + assert!(slice.as_raw_str().is_some()) + } } } diff --git a/crates/loro-core/tests/test.rs b/crates/loro-core/tests/test.rs index d61e38f9..81826d30 100644 --- a/crates/loro-core/tests/test.rs +++ b/crates/loro-core/tests/test.rs @@ -2,7 +2,6 @@ use ctor::ctor; use loro_core::container::Container; use loro_core::LoroCore; -#[ignore] #[test] fn test() { let mut store = LoroCore::new(Default::default(), Some(10)); @@ -14,7 +13,7 @@ fn test() { let value = value.as_string().unwrap(); assert_eq!(value.as_str(), "avvxxbc"); drop(text_container); - let mut store_b = LoroCore::new(Default::default(), Some(10)); + let mut store_b = LoroCore::new(Default::default(), Some(11)); store_b.import(store.export(Default::default())); let mut text_container = store_b.get_text_container("haha".into()); let value = text_container.get_value(); diff --git a/crates/rle/src/rle_tree/cursor.rs b/crates/rle/src/rle_tree/cursor.rs index 55c6be8f..2070cfea 100644 --- a/crates/rle/src/rle_tree/cursor.rs +++ b/crates/rle/src/rle_tree/cursor.rs @@ -180,8 +180,14 @@ impl<'tree, T: Rle, A: RleTreeTrait> UnsafeCursor<'tree, T, A> { /// /// we need to make sure that the leaf is still valid pub unsafe fn get_index(&self) -> A::Int { - let index = A::get_index(self.leaf.as_ref(), self.index); - index + A::Int::from_usize(self.offset).unwrap() + let leaf = self.leaf.as_ref(); + let index = A::get_index(leaf, self.index); + let item = self.as_ref(); + if item.len() == 0 { + index + } else { + index + A::Int::from_usize(self.offset).unwrap() + } } /// move cursor forward