diff --git a/crates/matrix-sdk/src/event_cache/linked_chunk/as_vector.rs b/crates/matrix-sdk/src/event_cache/linked_chunk/as_vector.rs index c80b5be00..86cc071d2 100644 --- a/crates/matrix-sdk/src/event_cache/linked_chunk/as_vector.rs +++ b/crates/matrix-sdk/src/event_cache/linked_chunk/as_vector.rs @@ -28,20 +28,38 @@ use super::{ ChunkIdentifier, }; -type Offset = usize; +/// A type alias to represent a chunk's length. This is purely for commodity. type ChunkLength = usize; pin_project! { + /// A type used to transform a `Stream>>` into + /// a `Stream>>`. Basically, it helps to consume + // a [`LinkedChunk`] as if it was an [`ObservableVector`]. pub struct AsVectorSubscriber { + // The inner `UpdatesSubscriber`. #[pin] updates_subscriber: UpdatesSubscriber, + // Pairs of all known chunks and their respective length. This is the only + // required data for this algorithm. chunks: VecDeque<(ChunkIdentifier, ChunkLength)>, } } impl AsVectorSubscriber { - pub(super) fn new( + /// Create a new [`Self`]. + /// + /// `updates_subscriber` is simply `UpdatesSubscriber`. + /// `initial_chunk_lengths` must be pairs of all chunk identifiers with the + /// associated chunk length. The pairs must be in the exact same order than + /// in [`LinkedChunk`]. + /// + /// # Safety + /// + /// This method is marked as unsafe only to attract the caller's attention + /// on the order of pairs. If the order of pairs are incorrect, the entire + /// algorithm will not work properly and is very likely to panic. + pub(super) unsafe fn new( updates_subscriber: UpdatesSubscriber, initial_chunk_lengths: VecDeque<(ChunkIdentifier, ChunkLength)>, ) -> Self { @@ -65,7 +83,86 @@ where let mut diffs = Vec::with_capacity(updates.len()); - let mut reattaching = false; + // A flag specifying when updates are reattaching detached items. + // + // Why is it useful? + // + // Imagine a `LinkedChunk::<3, char, ()>` containing `['a', 'b', 'c'] ['d']`. If + // one wants to insert [`w`, x`, 'y', 'z'] at position + // `Position(ChunkIdentifier(0), 1)`, i.e. at the position of `b`, here is what + // happens: + // + // 1. `LinkedChunk` will split off `['a', 'b', 'c']` at index 1, the chunk + // becomes `['a']` and `b` and `c` are _detached_, thus we have: + // + // ['a'] ['d'] + // + // 2. `LinkedChunk` will then insert `w`, `x`, `y` and `z` to get: + // + // ['a', 'w', 'x'] ['y', 'z'] ['d'] + // + // 3. `LinkedChunk` will now reattach `b` and `c` after `z`, like so: + // + // ['a', 'w', 'x'] ['y', 'z', 'b'] ['c'] ['d'] + // + // This detaching/reattaching approach makes it reliable and safe. Good. Now, + // what updates are we going to receive for each step? + // + // Step 1, detaching last items: + // + // ``` + // Update::DetachLastItems { at: Position(ChunkIdentifier(0), 1) } + // ``` + // + // Step 2, inserting new items: + // + // ``` + // Update::PushItems { + // position_hint: Position(ChunkIdentifier(0), 1), + // items: vec!['w', 'x'], + // } + // Update::NewItemsChunk { + // previous: Some(ChunkIdentifier(0)), + // new: ChunkIdentifier(2), + // next: Some(ChunkIdentifier(1)), + // } + // Update::PushItems { + // position_hint: Position(ChunkIdentifier(2), 0), + // items: vec!['y', 'z'], + // } + // ``` + // + // Step 3, reattaching detached items: + // + // ``` + // Update::ReattachItems + // Update::PushItems { + // position_hint: Position(ChunkIdentifier(2), 2), + // items: vec!['b'] + // } + // Update::NewItemsChunk { + // previous: Some(ChunkIdentifier(2)), + // new: ChunkIdentifier(3), + // next: Some(ChunkIdentifier(1)), + // } + // Update::PushItems { + // position_hint: Position(ChunkIdentifier(3), 0), + // items: vec!['c'], + // } + // Update::ReattachItemsDone + // ``` + // + // To ensure an optimised behaviour of this algorithm: + // + // * `Update::DetachLastItems` must not emit `VectorDiff::Remove`, + // + // * `Update::PushItems` must not emit `VectorDiff::Insert`s or + // `VectorDiff::Append`s if it happens after `ReattachItems` and before + // `ReattachItemsDone`. However, `Self::chunks` must always be updated. + // + // From the `VectorDiff` “point of view”, this optimisation aims at avoiding + // removing items to push them again later. + let mut mute_push_items = false; for update in updates { match update { @@ -73,31 +170,51 @@ where | Update::NewGapChunk { previous, new, next, .. } => { match (previous, next) { // New chunk at the end. - (Some(_previous), None) => { - // TODO: chec `previous` is correct + (Some(previous), None) => { + debug_assert!( + matches!(this.chunks.back(), Some((p, _)) if *p == previous), + "Inserting new chunk at the end: The previous chunk is invalid" + ); + this.chunks.push_back((new, 0)); } // New chunk at the beginning. - (None, Some(_next)) => { - // TODO: check `next` is correct + (None, Some(next)) => { + debug_assert!( + matches!(this.chunks.front(), Some((n, _)) if *n == next), + "Inserting new chunk at the end: The previous chunk is invalid" + ); + this.chunks.push_front((new, 0)); } // New chunk is inserted between 2 chunks. - (Some(_previous), Some(next)) => { - // TODO: check `previous` is correct + (Some(previous), Some(next)) => { let next_chunk_index = this .chunks .iter() .position(|(chunk_identifier, _)| *chunk_identifier == next) - .expect("next chunk not found"); + // SAFETY: Assuming `LinkedChunk` and `Updates` are not buggy, and + // assuming `Self::chunks` is correctly initialized, it is not + // possible to insert a chunk between two chunks where one does not + // exist. If this predicate fails, it means `LinkedChunk` or + // `Updates` contain a bug. + .expect("Inserting new chunk: The chunk is not found"); + + debug_assert!( + matches!(this.chunks.get(next_chunk_index - 1), Some((p, _)) if *p == previous), + "Inserting new chunk: The previous chunk is invalid" + ); this.chunks.insert(next_chunk_index, (new, 0)); } (None, None) => { - unreachable!("?"); + unreachable!( + "Inserting new chunk with no previous nor next chunk identifiers \ + is impossible" + ); } } } @@ -109,9 +226,15 @@ where .position(|(chunk_identifier, _)| { *chunk_identifier == expected_chunk_identifier }) - .expect("oops 4"); + // SAFETY: Assuming `LinkedChunk` and `Updates` are not buggy, and assuming + // `Self::chunks` is correctly initialized, it is not possible to remove a + // chunk that does not exist. If this predicate fails, it means + // `LinkedChunk` or `Updates` contain a bug. + .expect("Removing a chunk: The chunk is not found"); - this.chunks.remove(chunk_index).unwrap(); + // It's OK to ignore the result. The `chunk_index` exists because it's been + // found, and we don't care about its associated value. + let _ = this.chunks.remove(chunk_index); } Update::PushItems { position_hint: position, items } => { @@ -134,11 +257,16 @@ where ControlFlow::Continue(..) => None, } } - .expect("`ChunkIdentifier` must exist"); + // SAFETY: Assuming `LinkedChunk` and `Updates` are not buggy, and assuming + // `Self::chunks` is correctly initialized, it is not possible to push items on + // a chunk that does not exist. If this predicate fails, it means `LinkedChunk` + // or `Updates` contain a bug. + .expect("Pushing items: The chunk is not found"); *chunk_length += items.len(); - if reattaching { + // See `mute_push_items` to learn more. + if mute_push_items { continue; } @@ -164,17 +292,23 @@ where .find_map(|(chunk_identifier, length)| { (*chunk_identifier == expected_chunk_identifier).then(|| length) }) - .expect("oops 3"); + // SAFETY: Assuming `LinkedChunk` and `Updates` are not buggy, and assuming + // `Self::chunks` is correctly initialized, it is not possible to detach + // items from a chunk that does not exist. If this predicate fails, it means + // `LinkedChunk` or `Updates` contain a bug. + .expect("Detach last items: The chunk is not found"); *length = new_length; } Update::ReattachItems => { - reattaching = true; + // Entering the `reattaching` mode. + mute_push_items = true; } Update::ReattachItemsDone => { - reattaching = false; + // Exiting the `reattaching` mode. + mute_push_items = false; } } } @@ -199,40 +333,126 @@ mod tests { assert_pending!(as_vector); - linked_chunk.push_items_back(['a', 'b']); - linked_chunk.push_items_back(['c', 'd', 'e']); + linked_chunk.push_items_back(['a', 'b', 'c', 'd']); + #[rustfmt::skip] + assert_items_eq!(linked_chunk, ['a', 'b', 'c'] ['d']); + // From an `ObservableVector` point of view, it would look like: + // + // 0 1 2 3 4 + // +---+---+---+---+ + // | a | b | c | d | + // +---+---+---+---+ + // ^^^^^^^^^^^^^^^^ + // | + // new assert_next_eq!( as_vector, &[ - VectorDiff::Append { values: vector!['a', 'b'] }, - VectorDiff::Append { values: vector!['c'] }, - VectorDiff::Append { values: vector!['d', 'e'] }, + VectorDiff::Append { values: vector!['a', 'b', 'c'] }, + VectorDiff::Append { values: vector!['d'] }, ] ); linked_chunk - .insert_items_at(['f', 'g'], linked_chunk.item_position(|item| *item == 'b').unwrap()) + .insert_items_at( + ['w', 'x', 'y', 'z'], + linked_chunk.item_position(|item| *item == 'b').unwrap(), + ) .unwrap(); + assert_items_eq!(linked_chunk, ['a', 'w', 'x'] ['y', 'z', 'b'] ['c'] ['d']); + // From an `ObservableVector` point of view, it would look like: + // + // 0 1 2 3 4 5 6 7 8 + // +---+---+---+---+---+---+---+---+ + // | a | w | x | y | z | b | c | d | + // +---+---+---+---+---+---+---+---+ + // ^^^^^^^^^^^^^^^^ + // | + // new assert_next_eq!( as_vector, &[ - VectorDiff::Insert { index: 1, value: 'f' }, - VectorDiff::Insert { index: 2, value: 'g' }, + VectorDiff::Insert { index: 1, value: 'w' }, + VectorDiff::Insert { index: 2, value: 'x' }, + VectorDiff::Insert { index: 3, value: 'y' }, + VectorDiff::Insert { index: 4, value: 'z' }, ] ); linked_chunk.push_gap_back(()); - linked_chunk.push_items_back(['h', 'i']); + linked_chunk.push_items_back(['e', 'f', 'g', 'h']); + assert_items_eq!( + linked_chunk, + ['a', 'w', 'x'] ['y', 'z', 'b'] ['c'] ['d'] [-] ['e', 'f', 'g'] ['h'] + ); - assert_next_eq!(as_vector, &[VectorDiff::Append { values: vector!['h', 'i'] }]); + // From an `ObservableVector` point of view, it would look like: + // + // 0 1 2 3 4 5 6 7 8 9 10 11 12 + // +---+---+---+---+---+---+---+---+---+---+---+---+ + // | a | w | x | y | z | b | c | d | e | f | g | h | + // +---+---+---+---+---+---+---+---+---+---+---+---+ + // ^^^^^^^^^^^^^^^^ + // | + // new + assert_next_eq!( + as_vector, + &[ + VectorDiff::Append { values: vector!['e', 'f', 'g'] }, + VectorDiff::Append { values: vector!['h'] } + ] + ); linked_chunk - .replace_gap_at(['j'], linked_chunk.chunk_identifier(|chunk| chunk.is_gap()).unwrap()) + .replace_gap_at( + ['i', 'j', 'k', 'l'], + linked_chunk.chunk_identifier(|chunk| chunk.is_gap()).unwrap(), + ) .unwrap(); + assert_items_eq!( + linked_chunk, + ['a', 'w', 'x'] ['y', 'z', 'b'] ['c'] ['d'] ['i', 'j', 'k'] ['l'] ['e', 'f', 'g'] ['h'] + ); - assert_next_eq!(as_vector, &[VectorDiff::Insert { index: 7, value: 'j' }]); + // From an `ObservableVector` point of view, it would look like: + // + // 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 + // +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+ + // | a | w | x | y | z | b | c | d | i | j | k | l | e | f | g | h | + // +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+ + // ^^^^^^^^^^^^^^^^ + // | + // new + assert_next_eq!( + as_vector, + vec![ + VectorDiff::Insert { index: 8, value: 'i' }, + VectorDiff::Insert { index: 9, value: 'j' }, + VectorDiff::Insert { index: 10, value: 'k' }, + VectorDiff::Insert { index: 11, value: 'l' }, + ] + ); + + linked_chunk + .insert_items_at(['m'], linked_chunk.item_position(|item| *item == 'a').unwrap()) + .unwrap(); + assert_items_eq!( + linked_chunk, + ['m', 'a', 'w'] ['x'] ['y', 'z', 'b'] ['c'] ['d'] ['i', 'j', 'k'] ['l'] ['e', 'f', 'g'] ['h'] + ); + + // From an `ObservableVector` point of view, it would look like: + // + // 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 + // +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+ + // | m | a | w | x | y | z | b | c | d | i | j | k | l | e | f | g | h | + // +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+ + // ^^^^ + // | + // new + assert_next_eq!(as_vector, vec![VectorDiff::Insert { index: 0, value: 'm' }]); drop(linked_chunk); assert_closed!(as_vector); diff --git a/crates/matrix-sdk/src/event_cache/linked_chunk/mod.rs b/crates/matrix-sdk/src/event_cache/linked_chunk/mod.rs index c86107f4c..39638a88b 100644 --- a/crates/matrix-sdk/src/event_cache/linked_chunk/mod.rs +++ b/crates/matrix-sdk/src/event_cache/linked_chunk/mod.rs @@ -732,9 +732,11 @@ impl LinkedChunk { )) } - self.updates - .as_mut() - .map(|updates| AsVectorSubscriber::new(updates.subscribe(), initial_chunk_lengths)) + self.updates.as_mut().map(|updates| { + // SAFETY: The order of chunk pairs inside `initial_chunk_lengths` is exactly + // the same as chunks in `Self`. + unsafe { AsVectorSubscriber::new(updates.subscribe(), initial_chunk_lengths) } + }) } }