From dd8c7f739228405d55d6aa1c8a5fc28f22316ff2 Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Sat, 4 May 2024 11:32:49 +0800 Subject: [PATCH] perf: delete span merge err (#348) * perf: delete span merge err * fix: slicing of DeleteSpanWIthId * fix: sliceable err --- crates/fuzz/fuzz/Cargo.lock | 14 +- crates/fuzz/tests/test.rs | 261 ++++++++++++++++++ .../loro-internal/examples/automerge_x100.rs | 3 + crates/loro-internal/fuzz/Cargo.lock | 12 +- .../src/container/list/list_op.rs | 91 +++++- .../container/richtext/tracker/crdt_rope.rs | 2 +- 6 files changed, 366 insertions(+), 17 deletions(-) diff --git a/crates/fuzz/fuzz/Cargo.lock b/crates/fuzz/fuzz/Cargo.lock index 117ed0c0..9ab8e65f 100644 --- a/crates/fuzz/fuzz/Cargo.lock +++ b/crates/fuzz/fuzz/Cargo.lock @@ -234,9 +234,9 @@ dependencies = [ [[package]] name = "generic-btree" -version = "0.10.4" +version = "0.10.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e6f48456155f3a6d10b31a8510ebba3b3d38b01275fa17db8eb4033c1cf55846" +checksum = "210507e6dec78bb1304e52a174bd99efdd83894219bf20d656a066a0ce2fedc5" dependencies = [ "arref", "fxhash", @@ -384,7 +384,7 @@ dependencies = [ [[package]] name = "loro" -version = "0.4.0" +version = "0.5.0" dependencies = [ "either", "enum-as-inner 0.6.0", @@ -396,7 +396,7 @@ dependencies = [ [[package]] name = "loro-common" -version = "0.4.0" +version = "0.5.0" dependencies = [ "arbitrary", "enum-as-inner 0.6.0", @@ -411,7 +411,7 @@ dependencies = [ [[package]] name = "loro-delta" -version = "0.1.0" +version = "0.5.0" dependencies = [ "arrayvec", "enum-as-inner 0.5.1", @@ -422,7 +422,7 @@ dependencies = [ [[package]] name = "loro-internal" -version = "0.4.0" +version = "0.5.0" dependencies = [ "append-only-bytes", "arbitrary", @@ -456,7 +456,7 @@ dependencies = [ [[package]] name = "loro-rle" -version = "0.4.0" +version = "0.5.0" dependencies = [ "append-only-bytes", "arref", diff --git a/crates/fuzz/tests/test.rs b/crates/fuzz/tests/test.rs index 2ef066aa..97c7e7c3 100644 --- a/crates/fuzz/tests/test.rs +++ b/crates/fuzz/tests/test.rs @@ -5304,3 +5304,264 @@ fn test_tree_delete_nested() { ], ) } + +#[test] +fn test_text() { + test_multi_sites( + 5, + vec![ + FuzzTarget::Map, + FuzzTarget::List, + FuzzTarget::Text, + FuzzTarget::Tree, + FuzzTarget::MovableList, + ], + &mut [ + Handle { + site: 255, + target: 15, + container: 57, + action: Generic(GenericAction { + value: I32(757935405), + bool: true, + key: 805292845, + pos: 33041, + length: 0, + prop: 1238489669910396928, + }), + }, + Handle { + site: 45, + target: 45, + container: 45, + action: Generic(GenericAction { + value: I32(757935405), + bool: true, + key: 805292845, + pos: 3255307780432560401, + length: 18446743168229387565, + prop: 3255307777713581326, + }), + }, + Handle { + site: 45, + target: 45, + container: 45, + action: Generic(GenericAction { + value: I32(757935405), + bool: true, + key: 4291505453, + pos: 18388247646700638511, + length: 18446744073709507839, + prop: 5570344, + }), + }, + ], + ) +} + +#[test] +fn test_text_del_2() { + test_multi_sites( + 5, + vec![ + FuzzTarget::Map, + FuzzTarget::List, + FuzzTarget::Text, + FuzzTarget::Tree, + FuzzTarget::MovableList, + ], + &mut [ + Handle { + site: 42, + target: 45, + container: 253, + action: Generic(GenericAction { + value: I32(33554179), + bool: false, + key: 15616, + pos: 1339615555336169111, + length: 10909519737336631312, + prop: 10923365712002484737, + }), + }, + Sync { from: 151, to: 151 }, + Handle { + site: 191, + target: 0, + container: 2, + action: Generic(GenericAction { + value: I32(-1088190176), + bool: false, + key: 1898119453, + pos: 114672903794094449, + length: 2593958586217895690, + prop: 16131857654658175249, + }), + }, + Handle { + site: 17, + target: 17, + container: 17, + action: Generic(GenericAction { + value: I32(286331153), + bool: true, + key: 286331153, + pos: 1229782938247310353, + length: 1229782938247303441, + prop: 1229782938247303441, + }), + }, + Handle { + site: 21, + target: 17, + container: 17, + action: Generic(GenericAction { + value: I32(33554176), + bool: false, + key: 0, + pos: 1536, + length: 0, + prop: 1229782938247303424, + }), + }, + SyncAll, + SyncAll, + Handle { + site: 17, + target: 17, + container: 17, + action: Generic(GenericAction { + value: I32(286331153), + bool: true, + key: 0, + pos: 1229782864946528256, + length: 12080808152476417826, + prop: 10923366098543524643, + }), + }, + Handle { + site: 35, + target: 38, + container: 35, + action: Generic(GenericAction { + value: I32(587333693), + bool: false, + key: 2543294434, + pos: 4263285121861231497, + length: 59, + prop: 1518013315106421504, + }), + }, + Sync { from: 167, to: 35 }, + SyncAll, + Handle { + site: 0, + target: 0, + container: 49, + action: Generic(GenericAction { + value: Container(Text), + bool: false, + key: 0, + pos: 3602878813425696768, + length: 18446743185255244799, + prop: 8152360975528560127, + }), + }, + SyncAll, + Handle { + site: 80, + target: 0, + container: 61, + action: Generic(GenericAction { + value: I32(1161822054), + bool: true, + key: 269488146, + pos: 10883513199263901286, + length: 10923366098549554583, + prop: 2748041329745827735, + }), + }, + Handle { + site: 61, + target: 0, + container: 2, + action: Generic(GenericAction { + value: Container(Text), + bool: true, + key: 160113, + pos: 4438182027029079654, + length: 1229782938564925335, + prop: 1229785140740218641, + }), + }, + Handle { + site: 63, + target: 17, + container: 17, + action: Generic(GenericAction { + value: I32(286331153), + bool: true, + key: 286338065, + pos: 1229782938247303441, + length: 1229782938247303441, + prop: 1229782938247303441, + }), + }, + Handle { + site: 17, + target: 0, + container: 0, + action: Generic(GenericAction { + value: I32(0), + bool: false, + key: 1536, + pos: 0, + length: 1229782864946528256, + prop: 1229782938247303441, + }), + }, + SyncAll, + Handle { + site: 17, + target: 17, + container: 17, + action: Generic(GenericAction { + value: I32(286331153), + bool: true, + key: 17, + pos: 1229764173248856064, + length: 12080626724467843601, + prop: 10923366097000014759, + }), + }, + Handle { + site: 1, + target: 35, + container: 38, + action: Generic(GenericAction { + value: I32(33570048), + bool: true, + key: 2543313634, + pos: 3043090847611718039, + length: 15163, + prop: 1229783119343321088, + }), + }, + Sync { from: 167, to: 167 }, + Handle { + site: 1, + target: 191, + container: 35, + action: Generic(GenericAction { + value: I32(0), + bool: false, + key: 0, + pos: 0, + length: 0, + prop: 0, + }), + }, + ], + ) +} diff --git a/crates/loro-internal/examples/automerge_x100.rs b/crates/loro-internal/examples/automerge_x100.rs index 49559d1f..fd4c3935 100644 --- a/crates/loro-internal/examples/automerge_x100.rs +++ b/crates/loro-internal/examples/automerge_x100.rs @@ -30,4 +30,7 @@ fn main() { ); println!("Snapshot size {}", snapshot.len()); println!("Snapshot size after compression {}", compressed.len()); + let start = Instant::now(); + let _doc = LoroDoc::from_snapshot(&snapshot); + println!("Snapshot importing time {:?}", start.elapsed()); } diff --git a/crates/loro-internal/fuzz/Cargo.lock b/crates/loro-internal/fuzz/Cargo.lock index 249b0931..2137903c 100644 --- a/crates/loro-internal/fuzz/Cargo.lock +++ b/crates/loro-internal/fuzz/Cargo.lock @@ -203,9 +203,9 @@ dependencies = [ [[package]] name = "generic-btree" -version = "0.10.4" +version = "0.10.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e6f48456155f3a6d10b31a8510ebba3b3d38b01275fa17db8eb4033c1cf55846" +checksum = "210507e6dec78bb1304e52a174bd99efdd83894219bf20d656a066a0ce2fedc5" dependencies = [ "arref", "fxhash", @@ -353,7 +353,7 @@ dependencies = [ [[package]] name = "loro-common" -version = "0.4.0" +version = "0.5.0" dependencies = [ "arbitrary", "enum-as-inner 0.6.0", @@ -368,7 +368,7 @@ dependencies = [ [[package]] name = "loro-delta" -version = "0.1.0" +version = "0.5.0" dependencies = [ "arrayvec", "enum-as-inner 0.5.1", @@ -379,7 +379,7 @@ dependencies = [ [[package]] name = "loro-internal" -version = "0.4.0" +version = "0.5.0" dependencies = [ "append-only-bytes", "arbitrary", @@ -421,7 +421,7 @@ dependencies = [ [[package]] name = "loro-rle" -version = "0.4.0" +version = "0.5.0" dependencies = [ "append-only-bytes", "arref", diff --git a/crates/loro-internal/src/container/list/list_op.rs b/crates/loro-internal/src/container/list/list_op.rs index 82f8e379..4c2d51ec 100644 --- a/crates/loro-internal/src/container/list/list_op.rs +++ b/crates/loro-internal/src/container/list/list_op.rs @@ -112,9 +112,14 @@ impl HasLength for DeleteSpan { } } +/// Delete span that the initial id is `id_start`. +/// +/// This span may be reversed. #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] pub struct DeleteSpanWithId { + /// This is the target id with the smallest counter no matter whether the span is reversed. pub id_start: ID, + /// The deleted position span pub span: DeleteSpan, } @@ -171,13 +176,56 @@ impl Mergable for DeleteSpanWithId { where Self: Sized, { - self.id_end() == rhs.id_start && self.span.is_mergable(&rhs.span, &()) + let this = self.span; + let other = rhs.span; + // merge continuous deletions: + // note that the previous deletions will affect the position of the later deletions + match (self.span.bidirectional(), rhs.span.bidirectional()) { + (true, true) => { + (this.pos == other.pos && self.id_start.inc(1) == rhs.id_start) + || (this.pos == other.pos + 1 && self.id_start == rhs.id_start.inc(1)) + } + (true, false) => { + if this.pos == other.prev_pos() { + if other.signed_len > 0 { + self.id_start.inc(1) == rhs.id_start + } else { + self.id_start == rhs.id_end() + } + } else { + false + } + } + (false, true) => { + if this.next_pos() == other.pos { + if this.signed_len > 0 { + self.id_end() == rhs.id_start + } else { + self.id_start == rhs.id_start.inc(1) + } + } else { + false + } + } + (false, false) => { + if this.next_pos() == other.pos && this.direction() == other.direction() { + if self.span.signed_len > 0 { + self.id_end() == rhs.id_start + } else { + self.id_start == rhs.id_end() + } + } else { + false + } + } + } } fn merge(&mut self, rhs: &Self, _conf: &()) where Self: Sized, { + self.id_start.counter = rhs.id_start.counter.min(self.id_start.counter); self.span.merge(&rhs.span, &()) } } @@ -185,7 +233,27 @@ impl Mergable for DeleteSpanWithId { impl Sliceable for DeleteSpanWithId { fn slice(&self, from: usize, to: usize) -> Self { Self { - id_start: self.id_start.inc(from as i32), + id_start: if self.span.signed_len > 0 { + self.id_start.inc(from as i32) + } else { + // If the span is reversed, the id_start should be affected by `to` + // + // Example: + // + // a b c + // - - - <-- deletions happen backward + // 0 1 2 <-- counter of the IDs + // ↑ + // id_start + // + // If from=1, to=2 + // a b c + // - - - <-- deletions happen backward + // 0 1 2 <-- counter of the IDs + // ↑ + // id_start + self.id_start.inc((self.atom_len() - to) as i32) + }, span: self.span.slice(from, to), } } @@ -553,7 +621,7 @@ impl Sliceable for InnerListOp { } } -#[cfg(all(test, feature = "test_utils"))] +#[cfg(test)] mod test { use loro_common::ID; use rle::{Mergable, Sliceable}; @@ -643,4 +711,21 @@ mod test { assert_eq!(a.slice(0, 1).to_range(), 1..2); assert_eq!(a.slice(1, 2).to_range(), 0..1); } + + #[test] + fn mergeable() { + let a = DeleteSpan::new(14852, 1); + let mut a_with_id = DeleteSpanWithId { + id_start: ID::new(0, 9), + span: a, + }; + let b = DeleteSpan::new(14851, 1); + let b_with_id = DeleteSpanWithId { + id_start: ID::new(0, 8), + span: b, + }; + assert!(a_with_id.is_mergable(&b_with_id, &())); + a_with_id.merge(&b_with_id, &()); + assert!(a_with_id.span.signed_len == -2); + } } diff --git a/crates/loro-internal/src/container/richtext/tracker/crdt_rope.rs b/crates/loro-internal/src/container/richtext/tracker/crdt_rope.rs index 2fe0af79..a6409ffe 100644 --- a/crates/loro-internal/src/container/richtext/tracker/crdt_rope.rs +++ b/crates/loro-internal/src/container/richtext/tracker/crdt_rope.rs @@ -249,7 +249,7 @@ impl CrdtRope { let mut ans = SmallVec::with_capacity(len); for i in (0..len).rev() { let a = self.delete( - start_id.inc((len - i - 1) as i32), + start_id.inc(i as i32), pos + i, 1, false,