fix: find_path

crates/loro-core/proptest-regressions/dag/test.txt

@@ -5,3 +5,5 @@
 # It is recommended to check this file in to source control so that
 # everyone who runs the test benefits from these saved cases.
 cc 6fa5e507d2f2f20cb7f5d8a40762b5dfbe9832884ba1ec4fdd98afb3618a1495 # shrinks to before_merged_insertions = [], after_merged_insertions = []
+cc 33ec71c1f62792545519d525dcd8cdd8a4a666640aada85fc78bfe2d2b92591d # shrinks to mut interactions = [Interaction { dag_idx: 0, merge_with: None, len: 1 }, Interaction { dag_idx: 1, merge_with: None, len: 2 }, Interaction { dag_idx: 1, merge_with: Some(0), len: 1 }]
+cc cbbf51fd3f0bad25aede701c73321a1ead8738e3052655595bd29aa378bc0655 # shrinks to interactions = [Interaction { dag_idx: 1, merge_with: None, len: 3 }, Interaction { dag_idx: 1, merge_with: None, len: 3 }, Interaction { dag_idx: 4, merge_with: Some(1), len: 1 }, Interaction { dag_idx: 0, merge_with: None, len: 1 }, Interaction { dag_idx: 2, merge_with: Some(0), len: 1 }, Interaction { dag_idx: 0, merge_with: None, len: 1 }, Interaction { dag_idx: 2, merge_with: Some(0), len: 1 }, Interaction { dag_idx: 3, merge_with: Some(0), len: 1 }]

crates/loro-core/src/dag.rs

@@ -12,7 +12,7 @@ use std::{
 };
 
 use fxhash::{FxHashMap, FxHashSet};
-use rle::{HasLength, Sliceable};
+use rle::HasLength;
 use smallvec::SmallVec;
 mod iter;
 mod mermaid;
@@ -22,7 +22,7 @@ mod test;
 use crate::{
     change::Lamport,
     id::{ClientID, Counter, ID},
-    span::{CounterSpan, HasId, HasIdSpan, HasLamport, HasLamportSpan, IdSpan},
+    span::{HasId, HasIdSpan, HasLamport, HasLamportSpan, IdSpan},
     version::{VersionVector, VersionVectorDiff},
 };
 
@@ -119,7 +119,26 @@ impl<T: Dag> DagUtils for T {
 
     #[inline(always)]
     fn find_path(&self, from: ID, to: ID) -> VersionVectorDiff {
-        find_path(&|id: ID| self.get(id), from, to)
+        let mut ans = VersionVectorDiff::default();
+        _find_common_ancestor(
+            &|v| self.get(v),
+            from,
+            to,
+            &mut |span, node_type| {
+                // dbg!(span, node_type);
+                match node_type {
+                    NodeType::A => ans.merge_left(span),
+                    NodeType::B => ans.merge_right(span),
+                    NodeType::Shared => {
+                        ans.subtract_start_left(span);
+                        ans.subtract_start_right(span);
+                    }
+                }
+            },
+            true,
+        );
+
+        ans
     }
 
     #[inline(always)]
@@ -244,15 +263,25 @@ impl<'a> OrdIdSpan<'a> {
         D: DagNode + 'a,
         F: Fn(ID) -> Option<&'a D>,
     {
-        let m = get(id)?;
+        let span = get(id)?;
-        let diff = id.counter - m.id_start().counter;
+        let span_id = span.id_start();
         Some(OrdIdSpan {
-            id: id.inc(-diff),
+            id: span_id,
-            lamport: m.lamport_start(),
+            lamport: span.lamport_start(),
-            deps: m.deps(),
+            deps: span.deps(),
-            len: diff as usize + 1,
+            len: (id.counter - span_id.counter) as usize + 1,
         })
     }
+
+    #[inline]
+    fn get_min(&self) -> OrdIdSpan<'a> {
+        OrdIdSpan {
+            id: self.id,
+            lamport: self.lamport,
+            deps: &self.deps[0..0],
+            len: 1,
+        }
+    }
 }
 
 #[inline(always)]
@@ -261,60 +290,26 @@ where
     D: DagNode + 'a,
     F: Fn(ID) -> Option<&'a D>,
 {
-    _find_common_ancestor(get, a_id, b_id, &mut |_, _| {})
+    _find_common_ancestor(get, a_id, b_id, &mut |_, _| {}, false)
-}
+        .into_iter()
-
+        .map(|x| ID::new(x.0, x.1))
-fn find_path<'a, F, D>(get: &'a F, left_id: ID, right_id: ID) -> VersionVectorDiff
+        .collect()
-where
-    D: DagNode + 'a,
-    F: Fn(ID) -> Option<&'a D>,
-{
-    let mut ans = VersionVectorDiff::default();
-    let ancestors =
-        _find_common_ancestor(
-            get,
-            left_id,
-            right_id,
-            &mut |span, node_type| match node_type {
-                NodeType::A => ans.merge_left(span),
-                NodeType::B => ans.merge_right(span),
-                NodeType::Shared => {}
-            },
-        );
-    let vv: VersionVector = ancestors.into_iter().collect();
-    for (client, span) in ans.to_left.iter_mut() {
-        if let Some(CounterSpan { from: _, to }) = vv.intersect_span(&IdSpan {
-            client_id: *client,
-            counter: *span,
-        }) {
-            span.from = to;
-        }
-    }
-
-    for (client, span) in ans.to_right.iter_mut() {
-        if let Some(CounterSpan { from: _, to }) = vv.intersect_span(&IdSpan {
-            client_id: *client,
-            counter: *span,
-        }) {
-            span.from = to;
-        }
-    }
-
-    ans
 }
 
+/// - deep whether keep searching until the min of non-shared node is found
 fn _find_common_ancestor<'a, F, D, G>(
     get: &'a F,
     a_id: ID,
     b_id: ID,
     notify: &mut G,
-) -> SmallVec<[ID; 2]>
+    deep: bool,
+) -> FxHashMap<ClientID, Counter>
 where
     D: DagNode + 'a,
     F: Fn(ID) -> Option<&'a D>,
     G: FnMut(IdSpan, NodeType),
 {
-    let mut ans: SmallVec<[ID; 2]> = SmallVec::new();
+    let mut ans: FxHashMap<ClientID, Counter> = Default::default();
     let mut queue: BinaryHeap<(OrdIdSpan, NodeType)> = BinaryHeap::new();
     queue.push((OrdIdSpan::from_dag_node(a_id, get).unwrap(), NodeType::A));
     queue.push((OrdIdSpan::from_dag_node(b_id, get).unwrap(), NodeType::B));
@@ -330,6 +325,7 @@ where
     // type count in the queue. if both are zero, we can stop
     let mut a_count = 1;
     let mut b_count = 1;
+    let mut min = None;
     while let Some((node, mut node_type)) = queue.pop() {
         match node_type {
             NodeType::A => a_count -= 1,
@@ -337,9 +333,20 @@ where
             NodeType::Shared => {}
         }
 
+        if node_type != NodeType::Shared {
+            if let Some(min) = &mut min {
+                let node_start = node.get_min();
+                if node_start < *min {
+                    *min = node_start;
+                }
+            } else {
+                min = Some(node.get_min())
+            }
+        }
+
         // pop the same node in the queue
         while let Some((other_node, other_type)) = queue.peek() {
-            if node.id_last() == other_node.id_last() {
+            if node.id_span() == other_node.id_span() {
                 if node_type == *other_type {
                     match node_type {
                         NodeType::A => a_count -= 1,
@@ -351,10 +358,7 @@ where
                         if visited.get(&node.id.client_id).map(|(_, t)| *t)
                             != Some(NodeType::Shared)
                         {
-                            ans.push(ID {
+                            ans.insert(node.id.client_id, other_node.id_last().counter);
-                                client_id: node.id.client_id,
-                                counter: other_node.id_last().counter,
-                            });
                         }
                         node_type = NodeType::Shared;
                     }
@@ -374,13 +378,12 @@ where
         // detect whether client is visited by other
         if let Some((ctr, visited_type)) = visited.get_mut(&node.id.client_id) {
             debug_assert!(*ctr >= node.id_last().counter);
-            if *visited_type != NodeType::Shared && *visited_type != node_type {
+            if *visited_type == NodeType::Shared {
+                node_type = NodeType::Shared;
+            } else if *visited_type != node_type {
                 // if node_type is shared, ans should already contains it or its descendance
                 if node_type != NodeType::Shared {
-                    ans.push(ID {
+                    ans.insert(node.id.client_id, node.id_last().counter);
-                        client_id: node.id.client_id,
-                        counter: node.id_last().counter,
-                    });
                 }
                 *visited_type = NodeType::Shared;
                 node_type = NodeType::Shared;
@@ -399,7 +402,10 @@ where
             NodeType::Shared => {}
         }
 
-        if a_count == 0 && b_count == 0 {
+        if a_count == 0
+            && b_count == 0
+            && (!deep || min.is_none() || &node <= min.as_ref().unwrap())
+        {
             break;
         }
 

crates/loro-core/src/dag/test.rs

@@ -1,5 +1,6 @@
 #![cfg(test)]
 
+use proptest::prelude::*;
 use std::cmp::Ordering;
 
 use super::*;
@@ -123,7 +124,7 @@ impl TestDag {
         let counter = self.version_vec.entry(client_id).or_insert(0);
         let id = ID::new(client_id, *counter);
         *counter += len as Counter;
-        let deps = std::mem::replace(&mut self.frontier, vec![id]);
+        let deps = std::mem::replace(&mut self.frontier, vec![id.inc(len as Counter - 1)]);
         if deps.len() == 1 && deps[0].client_id == client_id {
             // can merge two op
             let arr = self.nodes.get_mut(&client_id).unwrap();
@@ -277,6 +278,21 @@ impl Interaction {
     }
 }
 
+prop_compose! {
+    fn gen_interaction(num: usize) (
+            dag_idx in 0..num,
+            merge_with in 0..num,
+            length in 1..10,
+            should_merge in 0..2
+        ) -> Interaction {
+        Interaction {
+            dag_idx,
+            merge_with: if should_merge == 1 && merge_with != dag_idx { Some(merge_with) } else { None },
+            len: length as usize,
+        }
+    }
+}
+
 fn preprocess(interactions: &mut [Interaction], num: i32) {
     for interaction in interactions.iter_mut() {
         interaction.dag_idx %= num as usize;
@@ -414,7 +430,7 @@ mod get_version_vector {
 
 #[cfg(test)]
 mod find_path {
-    use crate::fx_map;
+    use crate::{fx_map, span::CounterSpan, tests::PROPTEST_FACTOR_10};
 
     use super::*;
 
@@ -486,14 +502,17 @@ mod find_path {
         );
     }
 
-    fn proptest(dag_num: i32, interactions: &mut [Interaction]) {
+    fn test_find_path(
-        preprocess(interactions, dag_num);
+        dag_num: i32,
+        mut interactions: Vec<Interaction>,
+    ) -> Result<(), TestCaseError> {
+        preprocess(&mut interactions, dag_num);
         let mut dags = Vec::new();
         for i in 0..dag_num {
             dags.push(TestDag::new(i as ClientID));
         }
 
-        for interaction in interactions {
+        for interaction in interactions.iter_mut() {
             interaction.apply(&mut dags);
         }
 
@@ -508,16 +527,99 @@ mod find_path {
             nodes.push((node, vv));
         }
 
+        // println!("{}", a.mermaid());
         for (i, (node, vv)) in nodes.iter().enumerate() {
+            if i > 3 {
+                break;
+            }
+
             for (j, (other_node, other_vv)) in nodes.iter().enumerate() {
                 if i == j {
                     continue;
                 }
+
                 let actual = a.find_path(node.id, other_node.id);
-                // let expected = find_path(*vv, *other_vv);
+                let expected = vv.clone() - other_vv.clone();
-                // assert_eq!(actual, expected);
+                prop_assert_eq!(
+                    actual,
+                    expected,
+                    "\ni={} j={} node={} other={}",
+                    i,
+                    j,
+                    node.id,
+                    other_node.id
+                );
             }
         }
+
+        Ok(())
+    }
+
+    #[test]
+    fn issue() {
+        if let Err(err) = test_find_path(
+            5,
+            vec![
+                Interaction {
+                    dag_idx: 1,
+                    merge_with: None,
+                    len: 3,
+                },
+                Interaction {
+                    dag_idx: 1,
+                    merge_with: None,
+                    len: 3,
+                },
+                Interaction {
+                    dag_idx: 4,
+                    merge_with: Some(1),
+                    len: 1,
+                },
+                Interaction {
+                    dag_idx: 0,
+                    merge_with: None,
+                    len: 1,
+                },
+                Interaction {
+                    dag_idx: 2,
+                    merge_with: Some(0),
+                    len: 1,
+                },
+                Interaction {
+                    dag_idx: 0,
+                    merge_with: None,
+                    len: 1,
+                },
+                Interaction {
+                    dag_idx: 2,
+                    merge_with: Some(0),
+                    len: 1,
+                },
+                Interaction {
+                    dag_idx: 3,
+                    merge_with: Some(0),
+                    len: 1,
+                },
+            ],
+        ) {
+            panic!("{}", err);
+        }
+    }
+
+    proptest! {
+        #[test]
+        fn proptest_path(
+            interactions in prop::collection::vec(gen_interaction(5), 0..50 * PROPTEST_FACTOR_10),
+        ) {
+            test_find_path(5, interactions)?;
+        }
+
+        #[test]
+        fn proptest_path_large(
+            interactions in prop::collection::vec(gen_interaction(10), 0..10 * PROPTEST_FACTOR_10 * PROPTEST_FACTOR_10 + 10),
+        ) {
+            test_find_path(10, interactions)?;
+        }
     }
 }
 
@@ -619,7 +721,6 @@ mod find_common_ancestors {
 }
 
 mod find_common_ancestors_proptest {
-    use proptest::prelude::*;
 
     use crate::{
         array_mut_ref,
@@ -630,21 +731,6 @@ mod find_common_ancestors_proptest {
 
     use super::*;
 
-    prop_compose! {
-        fn gen_interaction(num: usize) (
-                dag_idx in 0..num,
-                merge_with in 0..num,
-                length in 1..10,
-                should_merge in 0..2
-            ) -> Interaction {
-            Interaction {
-                dag_idx,
-                merge_with: if should_merge == 1 && merge_with != dag_idx { Some(merge_with) } else { None },
-                len: length as usize,
-            }
-        }
-    }
-
     proptest! {
         #[test]
         fn test_2dags(

crates/loro-core/src/version.rs

@@ -56,6 +56,16 @@ impl VersionVectorDiff {
         merge(&mut self.to_right, span);
     }
 
+    #[inline]
+    pub fn subtract_start_left(&mut self, span: IdSpan) {
+        subtract_start(&mut self.to_left, span);
+    }
+
+    #[inline]
+    pub fn subtract_start_right(&mut self, span: IdSpan) {
+        subtract_start(&mut self.to_right, span);
+    }
+
     pub fn get_id_spans_left(&self) -> impl Iterator<Item = IdSpan> + '_ {
         self.to_left.iter().map(|(client_id, span)| IdSpan {
             client_id: *client_id,
@@ -71,6 +81,14 @@ impl VersionVectorDiff {
     }
 }
 
+fn subtract_start(m: &mut FxHashMap<ClientID, CounterSpan>, target: IdSpan) {
+    if let Some(span) = m.get_mut(&target.client_id) {
+        if span.from < target.counter.to {
+            span.from = target.counter.to;
+        }
+    }
+}
+
 fn merge(m: &mut FxHashMap<ClientID, CounterSpan>, target: IdSpan) {
     if let Some(span) = m.get_mut(&target.client_id) {
         span.from = span.from.min(target.counter.from);