From 9db137af441b57aa2dcbf66c051eaf095ebebdcd Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Mon, 10 Feb 2025 17:31:31 +0100 Subject: [PATCH] refactor(common): `LinkedChunk` can start by a gap. This patch removes the invariant stating that a `LinkedChunk` must start by a chunk of type items. This has never been really useful but it's now annoying to have this (with iterative loading of a `LinkedChunk` via the `EventCache`, it's now possible to get a gap as the first chunk). Let's remove this invariant. --- .../matrix-sdk-common/src/linked_chunk/mod.rs | 319 ++++++++++++------ 1 file changed, 210 insertions(+), 109 deletions(-) diff --git a/crates/matrix-sdk-common/src/linked_chunk/mod.rs b/crates/matrix-sdk-common/src/linked_chunk/mod.rs index 06bb4c63c..b3f0a7a5f 100644 --- a/crates/matrix-sdk-common/src/linked_chunk/mod.rs +++ b/crates/matrix-sdk-common/src/linked_chunk/mod.rs @@ -257,7 +257,6 @@ impl LinkedChunk { pub fn new() -> Self { Self { links: Ends { - // INVARIANT: The first chunk must always be an Items, not a Gap. first: Chunk::new_items_leaked(ChunkIdentifierGenerator::FIRST_IDENTIFIER), last: None, }, @@ -283,11 +282,7 @@ impl LinkedChunk { }); Self { - links: Ends { - // INVARIANT: The first chunk must always be an Items, not a Gap. - first: Chunk::new_items_leaked(first_chunk_identifier), - last: None, - }, + links: Ends { first: Chunk::new_items_leaked(first_chunk_identifier), last: None }, chunk_identifier_generator: ChunkIdentifierGenerator::new_from_scratch(), updates: Some(updates), marker: PhantomData, @@ -336,11 +331,12 @@ impl LinkedChunk { debug_assert!(last_chunk.is_last_chunk(), "`last_chunk` must be… the last chunk"); - // We need to update `self.last` if and only if `last_chunk` _is not_ the first - // chunk, and _is_ the last chunk (ensured by the `debug_assert!` above). + // We need to update `self.links.last` if and only if `last_chunk` _is not_ the + // first chunk, and _is_ the last chunk (ensured by the `debug_assert!` + // above). if last_chunk.is_first_chunk().not() { - // Maybe `last_chunk` is the same as the previous `self.last` chunk, but it's - // OK. + // Maybe `last_chunk` is the same as the previous `self.links.last` chunk, but + // it's OK. self.links.last = Some(last_chunk.as_ptr()); } } @@ -437,10 +433,10 @@ impl LinkedChunk { } }; - // We need to update `self.last` if and only if `chunk` _is not_ the first + // We need to update `self.links.last` if and only if `chunk` _is not_ the first // chunk, and _is_ the last chunk. if chunk.is_first_chunk().not() && chunk.is_last_chunk() { - // Maybe `chunk` is the same as the previous `self.last` chunk, but it's + // Maybe `chunk` is the same as the previous `self.links.last` chunk, but it's // OK. self.links.last = Some(chunk.as_ptr()); } @@ -505,8 +501,8 @@ impl LinkedChunk { chunk_ptr = Some(chunk.as_ptr()); - // We need to update `self.last` if and only if `chunk` _is_ the last chunk. The - // new last chunk is the chunk before `chunk`. + // We need to update `self.links.last` if and only if `chunk` _is_ the last + // chunk. The new last chunk is the chunk before `chunk`. if chunk.is_last_chunk() { self.links.last = chunk.previous; } @@ -586,36 +582,43 @@ impl LinkedChunk { .chunk_mut(chunk_identifier) .ok_or(Error::InvalidChunkIdentifier { identifier: chunk_identifier })?; - // If `item_index` is 0, we don't want to split the current items chunk to - // insert a new gap chunk, otherwise it would create an empty current items - // chunk. Let's handle this case in particular. - // - // Of course this optimisation applies if there is a previous chunk. Remember - // the invariant: a `Gap` cannot be the first chunk. - if item_index == 0 && chunk.is_items() && chunk.previous.is_some() { - let previous_chunk = chunk - .previous_mut() - // SAFETY: The `previous` chunk exists because we have tested - // `chunk.previous.is_some()` in the `if` statement. - .expect("Previous chunk must be present"); - - previous_chunk.insert_next( - Chunk::new_gap_leaked(self.chunk_identifier_generator.next(), content), - &mut self.updates, - ); - - // We don't need to update `self.last` because we have inserted a new chunk - // before `chunk`. - - return Ok(()); - } - let chunk = match &mut chunk.content { ChunkContent::Gap(..) => { return Err(Error::ChunkIsAGap { identifier: chunk_identifier }); } ChunkContent::Items(current_items) => { + // If `item_index` is 0, we don't want to split the current items chunk to + // insert a new gap chunk, otherwise it would create an empty current items + // chunk. Let's handle this case in particular. + if item_index == 0 { + let chunk_was_first = chunk.is_first_chunk(); + let chunk_was_last = chunk.is_last_chunk(); + + let new_chunk = chunk.insert_before( + Chunk::new_gap_leaked(self.chunk_identifier_generator.next(), content), + &mut self.updates, + ); + + let new_chunk_ptr = new_chunk.as_ptr(); + let chunk_ptr = chunk.as_ptr(); + + // `chunk` was the first: let's update `self.links.first`. + // + // If `chunk` was not the first but was the last, there is nothing to do, + // `self.links.last` is already up-to-date. + if chunk_was_first { + self.links.first = new_chunk_ptr; + + // `chunk` was the first __and__ the last: let's set `self.links.last`. + if chunk_was_last { + self.links.last = Some(chunk_ptr); + } + } + + return Ok(()); + } + let current_items_length = current_items.len(); if item_index >= current_items_length { @@ -663,10 +666,10 @@ impl LinkedChunk { } }; - // We need to update `self.last` if and only if `chunk` _is not_ the first + // We need to update `self.links.last` if and only if `chunk` _is not_ the first // chunk, and _is_ the last chunk. if chunk.is_first_chunk().not() && chunk.is_last_chunk() { - // Maybe `chunk` is the same as the previous `self.last` chunk, but it's + // Maybe `chunk` is the same as the previous `self.links.last` chunk, but it's // OK. self.links.last = Some(chunk.as_ptr()); } @@ -691,17 +694,26 @@ impl LinkedChunk { return Err(Error::ChunkIsItems { identifier: chunk_identifier }); }; - let next = chunk.next; + let chunk_was_first = chunk.is_first_chunk(); + let chunk_was_last = chunk.is_last_chunk(); + let next_ptr = chunk.next; + let previous_ptr = chunk.previous; + let position_of_next = chunk.next().map(|next| next.first_position()); chunk.unlink(&mut self.updates); let chunk_ptr = chunk.as_ptr(); - // If this ever changes, we may need to update self.links.first too. - debug_assert!(chunk.is_first_chunk().not(), "A gap cannot be the first chunk"); + // If the chunk is the first one, we need to update `self.links.first`… + if chunk_was_first { + // … if and only if there is a next chunk. + if let Some(next_ptr) = next_ptr { + self.links.first = next_ptr; + } + } - if chunk.is_last_chunk() { - self.links.last = chunk.previous; + if chunk_was_last { + self.links.last = previous_ptr; } // SAFETY: `chunk` is unlinked and not borrowed anymore. `LinkedChunk` doesn't @@ -709,10 +721,7 @@ impl LinkedChunk { let _chunk_boxed = unsafe { Box::from_raw(chunk_ptr.as_ptr()) }; // Return the first position of the next chunk, if any. - Ok(next.map(|next| { - let chunk = unsafe { next.as_ref() }; - chunk.first_position() - })) + Ok(position_of_next) } /// Replace the gap identified by `chunk_identifier`, by items. @@ -746,7 +755,7 @@ impl LinkedChunk { return Err(Error::ChunkIsItems { identifier: chunk_identifier }); }; - debug_assert!(chunk.is_first_chunk().not(), "A gap cannot be the first chunk"); + let chunk_was_first = chunk.is_first_chunk(); let maybe_last_chunk_ptr = { let items = items.into_iter(); @@ -774,7 +783,13 @@ impl LinkedChunk { // Get the pointer to `chunk`. chunk_ptr = chunk.as_ptr(); - // Update `self.last` if the gap chunk was the last chunk. + // Update `self.links.first` if the gap chunk was the first chunk. + if chunk_was_first { + self.links.first = new_chunk_ptr; + } + + // Update `self.links.last` if the gap (so the new) chunk was (is) the last + // chunk. if let Some(last_chunk_ptr) = maybe_last_chunk_ptr { self.links.last = Some(last_chunk_ptr); } @@ -1410,6 +1425,53 @@ impl Chunk { new_chunk } + /// Insert a new chunk before the current one. + /// + /// The respective [`Self::previous`] and [`Self::next`] of the current + /// and new chunk will be updated accordingly. + fn insert_before( + &mut self, + mut new_chunk_ptr: NonNull, + updates: &mut Option>, + ) -> &mut Self + where + Gap: Clone, + { + let new_chunk = unsafe { new_chunk_ptr.as_mut() }; + + // Update the previous chunk if any. + if let Some(previous_chunk) = self.previous_mut() { + // Link back to the new chunk. + previous_chunk.next = Some(new_chunk_ptr); + + // Link the new chunk to the next chunk. + new_chunk.previous = self.previous; + } + + // Link to the new chunk. + self.previous = Some(new_chunk_ptr); + // Link the new chunk to this one. + new_chunk.next = Some(self.as_ptr()); + + if let Some(updates) = updates.as_mut() { + let previous = new_chunk.previous().map(Chunk::identifier); + let new = new_chunk.identifier(); + let next = new_chunk.next().map(Chunk::identifier); + + match new_chunk.content() { + ChunkContent::Gap(gap) => { + updates.push(Update::NewGapChunk { previous, new, next, gap: gap.clone() }) + } + + ChunkContent::Items(..) => { + updates.push(Update::NewItemsChunk { previous, new, next }) + } + } + } + + new_chunk + } + /// Unlink this chunk. /// /// Be careful: `self` won't belong to `LinkedChunk` anymore, and should be @@ -2529,36 +2591,28 @@ mod tests { ); } - // Insert at the beginning of a chunk + it's the first chunk. + // Insert at the beginning of a chunk. The targeted chunk is the first chunk. + // `Ends::first` and `Ends::last` may be updated differently. { let position_of_a = linked_chunk.item_position(|item| *item == 'a').unwrap(); linked_chunk.insert_gap_at((), position_of_a)?; - // A new empty chunk is created as the first chunk. - assert_items_eq!(linked_chunk, [] [-] ['a'] [-] ['b', 'c'] ['d', 'e', 'f']); + // A new empty chunk is NOT created, i.e. `['a']` is not split into `[]` + + // `['a']` because it's a waste of space. + assert_items_eq!(linked_chunk, [-] ['a'] [-] ['b', 'c'] ['d', 'e', 'f']); assert_eq!( linked_chunk.updates().unwrap().take(), - &[ - DetachLastItems { at: Position(ChunkIdentifier(0), 0) }, - NewGapChunk { - previous: Some(ChunkIdentifier(0)), - new: ChunkIdentifier(4), - next: Some(ChunkIdentifier(2)), - gap: (), - }, - StartReattachItems, - NewItemsChunk { - previous: Some(ChunkIdentifier(4)), - new: ChunkIdentifier(5), - next: Some(ChunkIdentifier(2)), - }, - PushItems { at: Position(ChunkIdentifier(5), 0), items: vec!['a'] }, - EndReattachItems, - ] + &[NewGapChunk { + previous: None, + new: ChunkIdentifier(4), + next: Some(ChunkIdentifier(0)), + gap: (), + },] ); } - // Insert at the beginning of a chunk. + // Insert at the beginning of a chunk. The targeted chunk is not the first + // chunk. `Ends::first` and `Ends::last` may be updated differently. { let position_of_d = linked_chunk.item_position(|item| *item == 'd').unwrap(); linked_chunk.insert_gap_at((), position_of_d)?; @@ -2566,56 +2620,47 @@ mod tests { // A new empty chunk is NOT created, i.e. `['d', 'e', 'f']` is not // split into `[]` + `['d', 'e', 'f']` because it's a waste of // space. - assert_items_eq!(linked_chunk, [] [-] ['a'] [-] ['b', 'c'] [-] ['d', 'e', 'f']); + assert_items_eq!(linked_chunk, [-] ['a'] [-] ['b', 'c'] [-] ['d', 'e', 'f']); assert_eq!( linked_chunk.updates().unwrap().take(), &[NewGapChunk { previous: Some(ChunkIdentifier(3)), - new: ChunkIdentifier(6), + new: ChunkIdentifier(5), next: Some(ChunkIdentifier(1)), gap: (), }] ); } - // Insert in an empty chunk + it's the first chunk. - { - let position_of_first_empty_chunk = Position(ChunkIdentifier(0), 0); - assert_matches!( - linked_chunk.insert_gap_at((), position_of_first_empty_chunk), - Err(Error::InvalidItemIndex { index: 0 }) - ); - assert!(linked_chunk.updates().unwrap().take().is_empty()); - } - // Insert in an empty chunk. { // Replace a gap by empty items. let gap_identifier = linked_chunk.chunk_identifier(Chunk::is_gap).unwrap(); let position = linked_chunk.replace_gap_at([], gap_identifier)?.first_position(); - assert_items_eq!(linked_chunk, [] [-] ['a'] [-] ['b', 'c'] [] ['d', 'e', 'f']); + assert_items_eq!(linked_chunk, [-] ['a'] [-] ['b', 'c'] [] ['d', 'e', 'f']); + assert_eq!( linked_chunk.updates().unwrap().take(), &[ NewItemsChunk { - previous: Some(ChunkIdentifier(6)), - new: ChunkIdentifier(7), + previous: Some(ChunkIdentifier(5)), + new: ChunkIdentifier(6), next: Some(ChunkIdentifier(1)), }, - RemoveChunk(ChunkIdentifier(6)), + RemoveChunk(ChunkIdentifier(5)), ] ); linked_chunk.insert_gap_at((), position)?; - assert_items_eq!(linked_chunk, [] [-] ['a'] [-] ['b', 'c'] [-] [] ['d', 'e', 'f']); + assert_items_eq!(linked_chunk, [-] ['a'] [-] ['b', 'c'] [-] [] ['d', 'e', 'f']); assert_eq!( linked_chunk.updates().unwrap().take(), &[NewGapChunk { previous: Some(ChunkIdentifier(3)), - new: ChunkIdentifier(8), - next: Some(ChunkIdentifier(7)), + new: ChunkIdentifier(7), + next: Some(ChunkIdentifier(6)), gap: (), }] ); @@ -2643,10 +2688,10 @@ mod tests { { // It is impossible to get the item position inside a gap. It's only possible if // the item position is crafted by hand or is outdated. - let position_of_a_gap = Position(ChunkIdentifier(4), 0); + let position_of_a_gap = Position(ChunkIdentifier(2), 0); assert_matches!( linked_chunk.insert_gap_at((), position_of_a_gap), - Err(Error::ChunkIsAGap { identifier: ChunkIdentifier(4) }) + Err(Error::ChunkIsAGap { identifier: ChunkIdentifier(2) }) ); assert!(linked_chunk.updates().unwrap().take().is_empty()); } @@ -2766,13 +2811,54 @@ mod tests { ); } - assert_eq!(linked_chunk.num_items(), 13); + // Replace a gap at the beginning of the linked chunk. + { + let position_of_a = linked_chunk.item_position(|item| *item == 'a').unwrap(); + linked_chunk.insert_gap_at((), position_of_a).unwrap(); + assert_items_eq!( + linked_chunk, + [-] ['a', 'b'] ['d', 'e', 'f'] ['g', 'h'] ['l', 'm'] ['w', 'x', 'y'] ['z'] + ); + assert_eq!( + linked_chunk.updates().unwrap().take(), + &[NewGapChunk { + previous: None, + new: ChunkIdentifier(8), + next: Some(ChunkIdentifier(0)), + gap: (), + }] + ); + + let gap_identifier = linked_chunk.chunk_identifier(Chunk::is_gap).unwrap(); + assert_eq!(gap_identifier, ChunkIdentifier(8)); + + let new_chunk = linked_chunk.replace_gap_at(['p'], gap_identifier)?; + assert_eq!(new_chunk.identifier(), ChunkIdentifier(9)); + assert_items_eq!( + linked_chunk, + ['p'] ['a', 'b'] ['d', 'e', 'f'] ['g', 'h'] ['l', 'm'] ['w', 'x', 'y'] ['z'] + ); + assert_eq!( + linked_chunk.updates().unwrap().take(), + &[ + NewItemsChunk { + previous: Some(ChunkIdentifier(8)), + new: ChunkIdentifier(9), + next: Some(ChunkIdentifier(0)), + }, + PushItems { at: Position(ChunkIdentifier(9), 0), items: vec!['p'] }, + RemoveChunk(ChunkIdentifier(8)), + ] + ); + } + + assert_eq!(linked_chunk.num_items(), 14); Ok(()) } #[test] - fn test_remove_gap() -> Result<(), Error> { + fn test_remove_gap_at() -> Result<(), Error> { use super::Update::*; let mut linked_chunk = LinkedChunk::<3, char, ()>::new_with_update_history(); @@ -2780,31 +2866,38 @@ mod tests { // Ignore initial update. let _ = linked_chunk.updates().unwrap().take(); + linked_chunk.insert_gap_at((), Position(ChunkIdentifier(0), 0)).unwrap(); linked_chunk.push_items_back(['a', 'b']); linked_chunk.push_gap_back(()); linked_chunk.push_items_back(['l', 'm']); linked_chunk.push_gap_back(()); - assert_items_eq!(linked_chunk, ['a', 'b'] [-] ['l', 'm'] [-]); + assert_items_eq!(linked_chunk, [-] ['a', 'b'] [-] ['l', 'm'] [-]); assert_eq!( linked_chunk.updates().unwrap().take(), &[ + NewGapChunk { + previous: None, + new: ChunkIdentifier(1), + next: Some(ChunkIdentifier(0)), + gap: (), + }, PushItems { at: Position(ChunkIdentifier(0), 0), items: vec!['a', 'b'] }, NewGapChunk { previous: Some(ChunkIdentifier(0)), - new: ChunkIdentifier(1), + new: ChunkIdentifier(2), next: None, gap: (), }, NewItemsChunk { - previous: Some(ChunkIdentifier(1)), - new: ChunkIdentifier(2), - next: None, - }, - PushItems { at: Position(ChunkIdentifier(2), 0), items: vec!['l', 'm'] }, - NewGapChunk { previous: Some(ChunkIdentifier(2)), new: ChunkIdentifier(3), next: None, + }, + PushItems { at: Position(ChunkIdentifier(3), 0), items: vec!['l', 'm'] }, + NewGapChunk { + previous: Some(ChunkIdentifier(3)), + new: ChunkIdentifier(4), + next: None, gap: (), }, ] @@ -2819,20 +2912,28 @@ mod tests { assert_matches!(err, Error::InvalidChunkIdentifier { .. }); // Remove the gap in the middle. - let maybe_next = linked_chunk.remove_gap_at(ChunkIdentifier(1)).unwrap(); + let maybe_next = linked_chunk.remove_gap_at(ChunkIdentifier(2)).unwrap(); let next = maybe_next.unwrap(); // The next insert position at the start of the next chunk. - assert_eq!(next.chunk_identifier(), ChunkIdentifier(2)); + assert_eq!(next.chunk_identifier(), ChunkIdentifier(3)); assert_eq!(next.index(), 0); - assert_items_eq!(linked_chunk, ['a', 'b'] ['l', 'm'] [-]); - assert_eq!(linked_chunk.updates().unwrap().take(), &[RemoveChunk(ChunkIdentifier(1))]); + assert_items_eq!(linked_chunk, [-] ['a', 'b'] ['l', 'm'] [-]); + assert_eq!(linked_chunk.updates().unwrap().take(), &[RemoveChunk(ChunkIdentifier(2))]); // Remove the gap at the end. - let next = linked_chunk.remove_gap_at(ChunkIdentifier(3)).unwrap(); + let next = linked_chunk.remove_gap_at(ChunkIdentifier(4)).unwrap(); // It was the last chunk, so there's no next insert position. assert!(next.is_none()); + assert_items_eq!(linked_chunk, [-] ['a', 'b'] ['l', 'm']); + assert_eq!(linked_chunk.updates().unwrap().take(), &[RemoveChunk(ChunkIdentifier(4))]); + + // Remove the gap at the beginning. + let maybe_next = linked_chunk.remove_gap_at(ChunkIdentifier(1)).unwrap(); + let next = maybe_next.unwrap(); + assert_eq!(next.chunk_identifier(), ChunkIdentifier(0)); + assert_eq!(next.index(), 0); assert_items_eq!(linked_chunk, ['a', 'b'] ['l', 'm']); - assert_eq!(linked_chunk.updates().unwrap().take(), &[RemoveChunk(ChunkIdentifier(3))]); + assert_eq!(linked_chunk.updates().unwrap().take(), &[RemoveChunk(ChunkIdentifier(1))]); Ok(()) }