From 1c7bb195e5134ea61c8a6433ae72de95bcf3b2c6 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Tue, 14 Jul 2026 14:01:31 +0200 Subject: [PATCH] feat(common): Lazily create the first chunk in `LinkedChunk`. This patch updates the `LinkedChunk` to lazily create the first chunk. A bit of context: when a `LinkedChunk` is created, it was never empty: a first chunk with content `ChunkContent::Items` was always created. This was useful for many reasons, but it creates a problem when clearing the `LinkedChunk`: the first chunk was re-created to hold this invariant. This is not a problem when we work with in-memory data only. However, when the `linked_chunk::Update`s are consumed to be stored in a database, it starts being annoying when, for example, we want to clear the `LinkedChunk` because we want _to remove it_. This is the difference between a _reset_ (restarts from zero), and a _delete_. In both case, we need to clear the `LinkedChunk`; however in the _delete_ case, we don't want to recreate a first chunk! The solution is to create the first chunk lazily, when one needs to access it for the first time. Note that it makes `clear` no more unsafe, which is a good thing. Also, the `reset` method has been removed as it no longer makes sense: `clear` has the exact same behaviour. --- .../src/linked_chunk/as_vector.rs | 17 +- .../src/linked_chunk/lazy_loader.rs | 17 +- .../matrix-sdk-common/src/linked_chunk/mod.rs | 299 +++++++++++------- .../src/linked_chunk/updates.rs | 70 ++-- .../src/event_cache/caches/room/mod.rs | 21 +- .../src/event_cache/caches/thread/mod.rs | 19 +- .../tests/integration/event_cache/mod.rs | 8 +- .../tests/integration/event_cache/threads.rs | 4 +- 8 files changed, 255 insertions(+), 200 deletions(-) diff --git a/crates/matrix-sdk-common/src/linked_chunk/as_vector.rs b/crates/matrix-sdk-common/src/linked_chunk/as_vector.rs index 360784ca6..025906e96 100644 --- a/crates/matrix-sdk-common/src/linked_chunk/as_vector.rs +++ b/crates/matrix-sdk-common/src/linked_chunk/as_vector.rs @@ -512,7 +512,7 @@ mod tests { use imbl::{Vector, vector}; use super::{ - super::{Chunk, ChunkIdentifierGenerator, LinkedChunk, Update}, + super::{Chunk, ChunkIdentifier, ChunkIdentifierGenerator, LinkedChunk, Update}, VectorDiff, }; @@ -807,11 +807,10 @@ mod tests { assert_eq!(diffs.len(), 1); assert_matches!(&diffs[0], VectorDiff::Clear); - // 1 chunk in the `UpdateToVectorDiff` mapper. + // 0 chunk in the `UpdateToVectorDiff` mapper, because the new chunk is lazily + // created. let chunks = &as_vector.mapper.chunks; - assert_eq!(chunks.len(), 1); - assert_eq!(chunks[0].0, ChunkIdentifierGenerator::FIRST_IDENTIFIER); - assert_eq!(chunks[0].1, 0); + assert!(chunks.is_empty()); } // And we can push again. @@ -822,6 +821,14 @@ mod tests { assert_eq!(diffs.len(), 2); assert_matches!(&diffs[0], VectorDiff::Append { .. }); assert_matches!(&diffs[1], VectorDiff::Append { .. }); + + // 2 chunks in the `UpdateToVectorDiff` mapper. + let chunks = &as_vector.mapper.chunks; + assert_eq!(chunks.len(), 2); + assert_eq!(chunks[0].0, ChunkIdentifierGenerator::FIRST_IDENTIFIER); + assert_eq!(chunks[0].1, 3); + assert_eq!(chunks[1].0, ChunkIdentifier(1)); + assert_eq!(chunks[1].1, 1); } } diff --git a/crates/matrix-sdk-common/src/linked_chunk/lazy_loader.rs b/crates/matrix-sdk-common/src/linked_chunk/lazy_loader.rs index 0ea3e9e7d..ce490e7b6 100644 --- a/crates/matrix-sdk-common/src/linked_chunk/lazy_loader.rs +++ b/crates/matrix-sdk-common/src/linked_chunk/lazy_loader.rs @@ -57,10 +57,12 @@ pub fn from_last_chunk( // SAFETY: Pointer is convertible to a reference. unsafe { chunk_ptr.as_mut() }.lazy_previous = lazy_previous; + let updates = Some(ObservableUpdates::new()); + Ok(Some(LinkedChunk { - links: Ends { first: chunk_ptr, last: None }, + links: Ends::new_with_first_chunk(chunk_ptr, &updates), chunk_identifier_generator, - updates: Some(ObservableUpdates::new()), + updates, marker: PhantomData, })) } @@ -152,10 +154,10 @@ where // Update `links`. { // Remember the pointer to the `first_chunk`. - let old_first_chunk = links.first; + let old_first_chunk = *links.first_chunk_ptr(); // `new_first_chunk` becomes the new first chunk. - links.first = new_first_chunk; + *links.first_chunk_mut_ptr() = new_first_chunk; // Link the other way: `old_first_chunk` becomes the next chunk of the first // chunk. @@ -243,6 +245,8 @@ where } // The last chunk is now valid. + // + // Be sure to keep in-sync with `linked_chunk.links.replace_with` below. linked_chunk.chunk_identifier_generator = chunk_identifier_generator; // Take the `previous` chunk and consider it becomes the `lazy_previous`. @@ -257,7 +261,10 @@ where unsafe { chunk_ptr.as_mut() }.lazy_previous = lazy_previous; // Replace the first link with the new pointer. - linked_chunk.links.replace_with(chunk_ptr); + // + // SAFETY: The `linked_chunk.chunk_identifier_generator` has been updated + // accordingly a couple lines above. + unsafe { linked_chunk.links.replace_with(chunk_ptr) }; if let Some(updates) = linked_chunk.updates.as_mut() { // Clear the previous updates, as we're about to insert a clear they would be diff --git a/crates/matrix-sdk-common/src/linked_chunk/mod.rs b/crates/matrix-sdk-common/src/linked_chunk/mod.rs index 4e900b47f..392140859 100644 --- a/crates/matrix-sdk-common/src/linked_chunk/mod.rs +++ b/crates/matrix-sdk-common/src/linked_chunk/mod.rs @@ -102,7 +102,10 @@ use std::{ fmt::{self}, marker::PhantomData, ptr::NonNull, - sync::atomic::{self, AtomicU64}, + sync::{ + OnceLock, + atomic::{self, AtomicU64}, + }, }; pub use self::{as_vector::*, identifiers::*, order_tracker::OrderTracker, updates::*}; @@ -157,30 +160,111 @@ pub enum Error { /// referencing a subset of fields of a `LinkedChunk`. struct Ends { /// The first chunk. - first: NonNull>, + first: OnceLock>>, + /// The last chunk. last: Option>>, + + updates_pusher: Option>, } impl Ends { + /// Create a new [`Ends`]. + fn new(updates: &Option>) -> Self { + Self { + first: OnceLock::new(), + last: None, + updates_pusher: updates.as_ref().map(ObservableUpdates::new_pusher), + } + } + + /// Create a new [`Ends`] with a specific first chunk! + fn new_with_first_chunk( + first_chunk: NonNull>, + updates: &Option>, + ) -> Self { + Self { + first: { + let first = OnceLock::new(); + + // Initialise with `first_chunk`. + first.get_or_init(|| first_chunk); + + first + }, + last: None, + updates_pusher: updates.as_ref().map(ObservableUpdates::new_pusher), + } + } + + /// Lazily get an immutable pointer to the first chunk. + fn first_chunk_ptr(&self) -> &NonNull> { + self.first + // Lazily initialise during first access. + .get_or_init(|| { + let identifier = ChunkIdentifierGenerator::FIRST_IDENTIFIER; + + if let Some(updates) = self.updates_pusher.as_ref() { + updates.push(Update::NewItemsChunk { + previous: None, + new: identifier, + next: None, + }); + } + + Chunk::new_items_leaked(identifier) + }) + } + + /// Lazily get an mutable pointer to the first chunk. + fn first_chunk_mut_ptr(&mut self) -> &mut NonNull> { + // `OnceLock::get_or_init_mut` is unstable. We can fake it by using a combo of + // `get_or_init` + `get_mut`. + let _ = self.first_chunk_ptr(); + + self.first + .get_mut() + // SAFETY: `self.first` has been initialised by the call to `Self::first_chunk_ptr` + // above. The fact this method takes a `&mut self` also ensures an exclusive access to + // the `OnceLock`, providing the guarantee there is no other reader or writer to it, + // which makes it thread-safe. + .expect("`first` must have been initialised") + } + /// Get the first chunk, as an immutable reference. fn first_chunk(&self) -> &Chunk { - unsafe { self.first.as_ref() } + // SAFETY: The pointer to the first chunk has been correctly initialised and is + // convertible to a reference. + unsafe { self.first_chunk_ptr().as_ref() } } /// Get the first chunk, as a mutable reference. fn first_chunk_mut(&mut self) -> &mut Chunk { - unsafe { self.first.as_mut() } + // SAFETY: The pointer to the first chunk has been correctly initialised and is + // convertible to a mutable reference. + unsafe { self.first_chunk_mut_ptr().as_mut() } } /// Get the latest chunk, as an immutable reference. fn latest_chunk(&self) -> &Chunk { - unsafe { self.last.unwrap_or(self.first).as_ref() } + if let Some(last) = &self.last { + // SAFETY: The pointer to the last chunk has been correctly initialised and is + // convertible to a reference. + unsafe { last.as_ref() } + } else { + self.first_chunk() + } } /// Get the latest chunk, as a mutable reference. fn latest_chunk_mut(&mut self) -> &mut Chunk { - unsafe { self.last.as_mut().unwrap_or(&mut self.first).as_mut() } + if let Some(last) = &mut self.last { + // SAFETY: The pointer to the last chunk has been correctly initialised and is + // convertible to a mutable reference. + unsafe { last.as_mut() } + } else { + self.first_chunk_mut() + } } /// Get the chunk as a reference, from its identifier, if it exists. @@ -209,17 +293,12 @@ impl Ends { } } - /// Drop all the chunks, leaving the chunk in an uninitialized state, - /// because `Self::first` is a dangling pointer. - /// - /// # Safety - /// - /// The caller is responsible of ensuring that this is the last use of the - /// linked chunk, or that first will be re-initialized before any other use. - unsafe fn clear(&mut self) { + /// Drop all the chunks, the first chunk will be created lazily with the + /// identifier [`ChunkIdentifierGenerator::FIRST_IDENTIFIER`]. + fn clear(&mut self) { // Loop over all chunks, from the last to the first chunk, and drop them. // Take the latest chunk. - let mut current_chunk_ptr = self.last.or(Some(self.first)); + let mut current_chunk_ptr = self.last.or_else(|| self.first.get().copied()); // As long as we have another chunk… while let Some(chunk_ptr) = current_chunk_ptr { @@ -234,28 +313,31 @@ impl Ends { } // At this step, all chunks have been dropped, including `self.first`. - self.first = NonNull::dangling(); + self.first.take(); self.last = None; } /// Drop all chunks, and replace the first one with the one provided as an /// argument. - fn replace_with(&mut self, first_chunk: NonNull>) { - // SAFETY: we're resetting `self.first` afterwards. - unsafe { - self.clear(); - } - - // At this step, all chunks have been dropped, including `self.first`. - self.first = first_chunk; - } - - /// Drop all chunks, and re-create the default first one. /// - /// The default first chunk is an empty items chunk, with the identifier - /// [`ChunkIdentifierGenerator::FIRST_IDENTIFIER`]. - fn reset(&mut self) { - self.replace_with(Chunk::new_items_leaked(ChunkIdentifierGenerator::FIRST_IDENTIFIER)); + /// # Safety + /// + /// Be aware to not forget to update + /// [`LinkedChunk::chunk_identifier_generator`] because the first chunk has + /// the identifier [`ChunkIdentifierGenerator::FIRST_IDENTIFIER`]! + unsafe fn replace_with(&mut self, first_chunk: NonNull>) { + self.clear(); + + // At this step, all chunks have been dropped + // `self.first` is supposed to be uninitialised. Let's be sure. + let mut first_chunk = Some(first_chunk); + self.first.get_or_init(|| first_chunk.take().unwrap()); + + if first_chunk.is_some() { + unreachable!( + "`first` must be initialised to `first_chunk` because `clear` has been called" + ); + } } } @@ -290,13 +372,12 @@ impl Default for LinkedChunk { impl LinkedChunk { /// Create a new [`Self`]. pub fn new() -> Self { + let updates = None; + Self { - links: Ends { - first: Chunk::new_items_leaked(ChunkIdentifierGenerator::FIRST_IDENTIFIER), - last: None, - }, + links: Ends::new(&updates), chunk_identifier_generator: ChunkIdentifierGenerator::new_from_scratch(), - updates: None, + updates, marker: PhantomData, } } @@ -307,19 +388,12 @@ impl LinkedChunk { /// [`ObservableUpdates::take`] method must be called to consume and /// clean the updates. See [`Self::updates`]. pub fn new_with_update_history() -> Self { - let first_chunk_identifier = ChunkIdentifierGenerator::FIRST_IDENTIFIER; - - let mut updates = ObservableUpdates::new(); - updates.push(Update::NewItemsChunk { - previous: None, - new: first_chunk_identifier, - next: None, - }); + let updates = Some(ObservableUpdates::new()); Self { - links: Ends { first: Chunk::new_items_leaked(first_chunk_identifier), last: None }, + links: Ends::new(&updates), chunk_identifier_generator: ChunkIdentifierGenerator::new_from_scratch(), - updates: Some(updates), + updates, marker: PhantomData, } } @@ -327,7 +401,7 @@ impl LinkedChunk { /// Clear all the chunks. pub fn clear(&mut self) { // Clear `self.links`. - self.links.reset(); + self.links.clear(); // Clear `self.chunk_identifier_generator`. self.chunk_identifier_generator = ChunkIdentifierGenerator::new_from_scratch(); @@ -338,11 +412,6 @@ impl LinkedChunk { // useless. updates.clear_pending(); updates.push(Update::Clear); - updates.push(Update::NewItemsChunk { - previous: None, - new: ChunkIdentifierGenerator::FIRST_IDENTIFIER, - next: None, - }) } } @@ -632,7 +701,7 @@ impl LinkedChunk { // 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; + *self.links.first_chunk_mut_ptr() = new_chunk_ptr; // `chunk` was the first __and__ the last: let's set `self.links.last`. if chunk_was_last { @@ -741,7 +810,7 @@ impl LinkedChunk { 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; + *self.links.first_chunk_mut_ptr() = next_ptr; } } @@ -818,7 +887,7 @@ impl LinkedChunk { // Update `self.links.first` if the gap chunk was the first chunk. if chunk_was_first { - self.links.first = new_chunk_ptr; + *self.links.first_chunk_mut_ptr() = new_chunk_ptr; } // Update `self.links.last` if the gap (so the new) chunk was (is) the last @@ -1062,12 +1131,7 @@ impl Drop for LinkedChunk { // `Update::Clear` when `self` is dropped. Instead, we only care about // freeing memory correctly. Rust can take care of everything except the // pointers in `self.links`, hence the specific call to `self.links.clear()`. - // - // SAFETY: this is the last use of the linked chunk, so leaving it in a dangling - // state is fine. - unsafe { - self.links.clear(); - } + self.links.clear(); } } @@ -1643,7 +1707,7 @@ where fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> { formatter .debug_struct("LinkedChunk") - .field("first (deref)", unsafe { self.links.first.as_ref() }) + .field("first (deref)", self.links.first_chunk()) .field("last", &self.links.last) .finish_non_exhaustive() } @@ -1760,6 +1824,12 @@ mod tests { let mut linked_chunk = LinkedChunk::<3, char, ()>::new_with_update_history(); + // No chunk is created to start with. + assert!(linked_chunk.updates().unwrap().take().is_empty()); + + // However, as soon as the first chunk is read, the chunk is created. + let _ = linked_chunk.first_chunk(); + assert_eq!( linked_chunk.updates().unwrap().take(), &[NewItemsChunk { previous: None, new: ChunkIdentifier(0), next: None }] @@ -1772,15 +1842,15 @@ mod tests { let mut linked_chunk = LinkedChunk::<3, char, ()>::new_with_update_history(); - // Ignore initial update. - let _ = linked_chunk.updates().unwrap().take(); - linked_chunk.push_items_back(['a']); assert_items_eq!(linked_chunk, ['a']); assert_eq!( linked_chunk.updates().unwrap().take(), - &[PushItems { at: Position(ChunkIdentifier(0), 0), items: vec!['a'] }] + &[ + NewItemsChunk { previous: None, new: ChunkIdentifier(0), next: None }, + PushItems { at: Position(ChunkIdentifier(0), 0), items: vec!['a'] } + ] ); linked_chunk.push_items_back(['b', 'c']); @@ -1834,14 +1904,14 @@ mod tests { let mut linked_chunk = LinkedChunk::<3, char, ()>::new_with_update_history(); - // Ignore initial update. - let _ = linked_chunk.updates().unwrap().take(); - linked_chunk.push_items_back(['a']); assert_items_eq!(linked_chunk, ['a']); assert_eq!( linked_chunk.updates().unwrap().take(), - &[PushItems { at: Position(ChunkIdentifier(0), 0), items: vec!['a'] }] + &[ + NewItemsChunk { previous: None, new: ChunkIdentifier(0), next: None }, + PushItems { at: Position(ChunkIdentifier(0), 0), items: vec!['a'] } + ] ); linked_chunk.push_gap_back(()); @@ -2175,14 +2245,12 @@ mod tests { let mut linked_chunk = LinkedChunk::<3, char, ()>::new_with_update_history(); - // Ignore initial update. - let _ = linked_chunk.updates().unwrap().take(); - linked_chunk.push_items_back(['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: None, new: ChunkIdentifier(0), next: None }, PushItems { at: Position(ChunkIdentifier(0), 0), items: vec!['a', 'b', 'c'] }, NewItemsChunk { previous: Some(ChunkIdentifier(0)), @@ -2356,14 +2424,12 @@ mod tests { let mut linked_chunk = LinkedChunk::<3, char, ()>::new_with_update_history(); - // Ignore initial update. - let _ = linked_chunk.updates().unwrap().take(); - linked_chunk.push_items_back(['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: None, new: ChunkIdentifier(0), next: None }, PushItems { at: Position(ChunkIdentifier(0), 0), items: vec!['a', 'b', 'c'] }, NewItemsChunk { previous: Some(ChunkIdentifier(0)), @@ -2418,14 +2484,12 @@ mod tests { let mut linked_chunk = LinkedChunk::<3, char, ()>::new_with_update_history(); - // Ignore initial update. - let _ = linked_chunk.updates().unwrap().take(); - linked_chunk.push_items_back(['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: None, new: ChunkIdentifier(0), next: None }, PushItems { at: Position(ChunkIdentifier(0), 0), items: vec!['a', 'b', 'c'] }, NewItemsChunk { previous: Some(ChunkIdentifier(0)), @@ -2477,14 +2541,12 @@ mod tests { let mut linked_chunk = LinkedChunk::<3, char, ()>::new_with_update_history(); - // Ignore initial update. - let _ = linked_chunk.updates().unwrap().take(); - linked_chunk.push_items_back(['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h']); assert_items_eq!(linked_chunk, ['a', 'b', 'c'] ['d', 'e', 'f'] ['g', 'h']); assert_eq!( linked_chunk.updates().unwrap().take(), &[ + NewItemsChunk { previous: None, new: ChunkIdentifier(0), next: None }, PushItems { at: Position(ChunkIdentifier(0), 0), items: vec!['a', 'b', 'c'] }, NewItemsChunk { previous: Some(ChunkIdentifier(0)), @@ -2535,14 +2597,12 @@ mod tests { let mut linked_chunk = LinkedChunk::<3, char, ()>::new_with_update_history(); - // Ignore initial update. - let _ = linked_chunk.updates().unwrap().take(); - linked_chunk.push_items_back(['a', 'b', 'c', 'd', 'e']); assert_items_eq!(linked_chunk, ['a', 'b', 'c'] ['d', 'e']); assert_eq!( linked_chunk.updates().unwrap().take(), &[ + NewItemsChunk { previous: None, new: ChunkIdentifier(0), next: None }, PushItems { at: Position(ChunkIdentifier(0), 0), items: vec!['a', 'b', 'c'] }, NewItemsChunk { previous: Some(ChunkIdentifier(0)), @@ -2585,15 +2645,13 @@ mod tests { let mut linked_chunk = LinkedChunk::<3, char, ()>::new_with_update_history(); - // Ignore initial update. - let _ = linked_chunk.updates().unwrap().take(); - linked_chunk.push_items_back(['a', 'b', 'c']); linked_chunk.push_gap_back(()); assert_items_eq!(linked_chunk, ['a', 'b', 'c'] [-]); assert_eq!( linked_chunk.updates().unwrap().take(), &[ + NewItemsChunk { previous: None, new: ChunkIdentifier(0), next: None }, PushItems { at: Position(ChunkIdentifier(0), 0), items: vec!['a', 'b', 'c'] }, NewGapChunk { previous: Some(ChunkIdentifier(0)), @@ -2639,9 +2697,6 @@ mod tests { let mut linked_chunk = LinkedChunk::<3, char, ()>::new_with_update_history(); - // Ignore initial update. - let _ = linked_chunk.updates().unwrap().take(); - linked_chunk.push_items_back(['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k']); assert_items_eq!(linked_chunk, ['a', 'b', 'c'] ['d', 'e', 'f'] ['g', 'h', 'i'] ['j', 'k']); assert_eq!(linked_chunk.num_items(), 11); @@ -2855,14 +2910,12 @@ mod tests { let mut linked_chunk = LinkedChunk::<3, char, ()>::new_with_update_history(); - // Ignore initial update. - let _ = linked_chunk.updates().unwrap().take(); - linked_chunk.push_items_back(['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: None, new: ChunkIdentifier(0), next: None }, PushItems { at: Position(ChunkIdentifier(0), 0), items: vec!['a', 'b', 'c'] }, NewItemsChunk { previous: Some(ChunkIdentifier(0)), @@ -3017,9 +3070,6 @@ mod tests { let mut linked_chunk = LinkedChunk::<3, char, ()>::new_with_update_history(); - // Ignore initial update. - let _ = linked_chunk.updates().unwrap().take(); - linked_chunk.push_items_back(['a', 'b']); linked_chunk.push_gap_back(()); linked_chunk.push_items_back(['l', 'm']); @@ -3027,6 +3077,7 @@ mod tests { assert_eq!( linked_chunk.updates().unwrap().take(), &[ + NewItemsChunk { previous: None, new: ChunkIdentifier(0), next: None }, PushItems { at: Position(ChunkIdentifier(0), 0), items: vec!['a', 'b'] }, NewGapChunk { previous: Some(ChunkIdentifier(0)), @@ -3083,15 +3134,13 @@ mod tests { let mut linked_chunk = LinkedChunk::<3, char, ()>::new_with_update_history(); - // Ignore initial update. - let _ = linked_chunk.updates().unwrap().take(); - linked_chunk.push_items_back(['a', 'b']); linked_chunk.push_gap_back(()); assert_items_eq!(linked_chunk, ['a', 'b'] [-]); assert_eq!( linked_chunk.updates().unwrap().take(), &[ + NewItemsChunk { previous: None, new: ChunkIdentifier(0), next: None }, PushItems { at: Position(ChunkIdentifier(0), 0), items: vec!['a', 'b'] }, NewGapChunk { previous: Some(ChunkIdentifier(0)), @@ -3142,14 +3191,14 @@ mod tests { let mut linked_chunk = LinkedChunk::<3, char, ()>::new_with_update_history(); - // Ignore initial update. - let _ = linked_chunk.updates().unwrap().take(); - linked_chunk.push_items_back(['a', 'b']); assert_items_eq!(linked_chunk, ['a', 'b']); assert_eq!( linked_chunk.updates().unwrap().take(), - &[PushItems { at: Position(ChunkIdentifier(0), 0), items: vec!['a', 'b'] },] + &[ + NewItemsChunk { previous: None, new: ChunkIdentifier(0), next: None }, + PushItems { at: Position(ChunkIdentifier(0), 0), items: vec!['a', 'b'] }, + ] ); // Replace a gap at the beginning of the linked chunk. @@ -3202,9 +3251,6 @@ mod tests { let mut linked_chunk = LinkedChunk::<3, char, ()>::new_with_update_history(); - // 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(()); @@ -3214,6 +3260,7 @@ mod tests { assert_eq!( linked_chunk.updates().unwrap().take(), &[ + NewItemsChunk { previous: None, new: ChunkIdentifier(0), next: None }, NewGapChunk { previous: None, new: ChunkIdentifier(1), @@ -3281,10 +3328,6 @@ mod tests { fn test_remove_empty_last_chunk() { let mut linked_chunk = LinkedChunk::<3, char, ()>::new_with_update_history(); - // Ignore initial update. - let _ = linked_chunk.updates().unwrap().take(); - - assert_items_eq!(linked_chunk, []); assert!(linked_chunk.updates().unwrap().take().is_empty()); // Try to remove the first chunk. @@ -3392,6 +3435,13 @@ mod tests { let mut linked_chunk = LinkedChunk::<3, char, ()>::new_with_update_history(); + // No update to start with because the first chunk is lazily created. + assert!(linked_chunk.updates().unwrap().take().is_empty()); + + // Let's access the first chunk. + let _ = linked_chunk.first_chunk(); + + // Now we get the update! assert_eq!( linked_chunk.updates().unwrap().take(), &[NewItemsChunk { @@ -3401,18 +3451,23 @@ mod tests { }] ); + // When clearing… linked_chunk.clear(); + // … we see only `Clear` without `NewItemsChunk`! + assert_eq!(linked_chunk.updates().unwrap().take(), &[Clear]); + + // Let's access the first chunk. + let _ = linked_chunk.first_chunk(); + + // Now we get the update again! assert_eq!( linked_chunk.updates().unwrap().take(), - &[ - Clear, - NewItemsChunk { - previous: None, - new: ChunkIdentifierGenerator::FIRST_IDENTIFIER, - next: None - } - ] + &[NewItemsChunk { + previous: None, + new: ChunkIdentifierGenerator::FIRST_IDENTIFIER, + next: None + }] ); } @@ -3463,11 +3518,13 @@ mod tests { let mut first_loaded_chunk = Chunk::new_items_leaked(ChunkIdentifier(1)); unsafe { first_loaded_chunk.as_mut() }.lazy_previous = Some(first_chunk_identifier); + let updates = Some(ObservableUpdates::new()); + let mut linked_chunk = LinkedChunk::<3, char, ()> { - links: Ends { first: first_loaded_chunk, last: None }, + links: Ends::new_with_first_chunk(first_loaded_chunk, &updates), chunk_identifier_generator: ChunkIdentifierGenerator::new_from_previous_chunk_identifier(ChunkIdentifier(1)), - updates: Some(ObservableUpdates::new()), + updates, marker: PhantomData, }; diff --git a/crates/matrix-sdk-common/src/linked_chunk/updates.rs b/crates/matrix-sdk-common/src/linked_chunk/updates.rs index 7ea3a4004..c4a873df2 100644 --- a/crates/matrix-sdk-common/src/linked_chunk/updates.rs +++ b/crates/matrix-sdk-common/src/linked_chunk/updates.rs @@ -202,6 +202,25 @@ impl ObservableUpdates { last_token } + + /// Create a new [`ObservableUpdatesPusher`], privately. + pub(super) fn new_pusher(&self) -> ObservableUpdatesPusher { + ObservableUpdatesPusher { inner: self.inner.clone() } + } +} + +/// This type is similar to [`ObservableUpdates`] except it has a single `push` +/// method which takes a `&self` instead of a `&mut self` to accommodate a +/// particular need in `Ends` for lazily get the first chunk. +pub(super) struct ObservableUpdatesPusher { + inner: Arc>>, +} + +impl ObservableUpdatesPusher { + /// Push a new update, even if `&self` while we could expect a `&mut self`. + pub fn push(&self, update: Update) { + self.inner.write().unwrap().push(update); + } } /// A token used to represent readers that read the updates in @@ -451,7 +470,10 @@ mod tests { other_token }; - // There is an initial update. + // Let's trigger the chunk creation to simplify the test. + let _ = linked_chunk.first_chunk(); + + // There is an update. { let updates = linked_chunk.updates().unwrap(); @@ -698,16 +720,7 @@ mod tests { let updates_subscriber = linked_chunk.updates().unwrap().subscribe(); pin_mut!(updates_subscriber); - // Initial update, stream is ready. - assert_matches!( - updates_subscriber.as_mut().poll_next(&mut context), - Poll::Ready(Some(items)) => { - assert_eq!( - items, - &[NewItemsChunk { previous: None, new: ChunkIdentifier(0), next: None }] - ); - } - ); + // No initial update, stream is pending. assert_matches!(updates_subscriber.as_mut().poll_next(&mut context), Poll::Pending); assert_eq!(*counter_waker.number_of_wakeup.lock().unwrap(), 0); @@ -723,7 +736,10 @@ mod tests { Poll::Ready(Some(items)) => { assert_eq!( items, - &[PushItems { at: Position(ChunkIdentifier(0), 0), items: vec!['a'] }] + &[ + NewItemsChunk { previous: None, new: ChunkIdentifier(0), next: None }, + PushItems { at: Position(ChunkIdentifier(0), 0), items: vec!['a'] } + ] ); } ); @@ -792,28 +808,10 @@ mod tests { let updates_subscriber2 = linked_chunk.updates().unwrap().subscribe(); pin_mut!(updates_subscriber2); - // Initial updates, streams are ready. - assert_matches!( - updates_subscriber1.as_mut().poll_next(&mut context1), - Poll::Ready(Some(items)) => { - assert_eq!( - items, - &[NewItemsChunk { previous: None, new: ChunkIdentifier(0), next: None }] - ); - } - ); + // No initial updates, streams are pending. assert_matches!(updates_subscriber1.as_mut().poll_next(&mut context1), Poll::Pending); assert_eq!(*counter_waker1.number_of_wakeup.lock().unwrap(), 0); - assert_matches!( - updates_subscriber2.as_mut().poll_next(&mut context2), - Poll::Ready(Some(items)) => { - assert_eq!( - items, - &[NewItemsChunk { previous: None, new: ChunkIdentifier(0), next: None }] - ); - } - ); assert_matches!(updates_subscriber2.as_mut().poll_next(&mut context2), Poll::Pending); assert_eq!(*counter_waker2.number_of_wakeup.lock().unwrap(), 0); @@ -830,7 +828,10 @@ mod tests { Poll::Ready(Some(items)) => { assert_eq!( items, - &[PushItems { at: Position(ChunkIdentifier(0), 0), items: vec!['a'] }] + &[ + NewItemsChunk { previous: None, new: ChunkIdentifier(0), next: None }, + PushItems { at: Position(ChunkIdentifier(0), 0), items: vec!['a'] } + ] ); } ); @@ -840,7 +841,10 @@ mod tests { Poll::Ready(Some(items)) => { assert_eq!( items, - &[PushItems { at: Position(ChunkIdentifier(0), 0), items: vec!['a'] }] + &[ + NewItemsChunk { previous: None, new: ChunkIdentifier(0), next: None }, + PushItems { at: Position(ChunkIdentifier(0), 0), items: vec!['a'] } + ] ); } ); diff --git a/crates/matrix-sdk/src/event_cache/caches/room/mod.rs b/crates/matrix-sdk/src/event_cache/caches/room/mod.rs index 1bd0ee1c6..6df4dd7b4 100644 --- a/crates/matrix-sdk/src/event_cache/caches/room/mod.rs +++ b/crates/matrix-sdk/src/event_cache/caches/room/mod.rs @@ -1096,10 +1096,8 @@ mod timed_tests { Ok(RoomEventCacheUpdate::UpdateTimelineEvents(TimelineVectorDiffs { diffs, .. })) = stream.recv() ); - assert_eq!(diffs.len(), 2); + assert_eq!(diffs.len(), 1); assert_let!(VectorDiff::Clear = &diffs[0]); - assert_let!(VectorDiff::Append { values } = &diffs[1]); - assert!(values.is_empty()); // … same with a generic update. assert_let_timeout!( @@ -1116,16 +1114,13 @@ mod timed_tests { assert!(items.is_empty()); // The event cache store is fully empty. - let linked_chunk = from_all_chunks::<3, _, _>( - event_cache_store.load_all_chunks(LinkedChunkId::Room(room_id)).await.unwrap(), - ) - .unwrap() - .unwrap(); - - // Note: while the event cache store could return `None` here, clearing it will - // reset it to its initial form, maintaining the invariant that it - // contains a single items chunk that's empty. - assert_eq!(linked_chunk.num_items(), 0); + assert!( + event_cache_store + .load_all_chunks(LinkedChunkId::Room(room_id)) + .await + .unwrap() + .is_empty() + ); } #[async_test] diff --git a/crates/matrix-sdk/src/event_cache/caches/thread/mod.rs b/crates/matrix-sdk/src/event_cache/caches/thread/mod.rs index 0b9fda183..5f5ed298e 100644 --- a/crates/matrix-sdk/src/event_cache/caches/thread/mod.rs +++ b/crates/matrix-sdk/src/event_cache/caches/thread/mod.rs @@ -664,11 +664,8 @@ mod timed_tests { assert_matches!( thread_stream.recv().await, Ok(TimelineVectorDiffs { diffs, .. }) => { - assert_eq!(diffs.len(), 2); + assert_eq!(diffs.len(), 1); assert_matches!(&diffs[0], VectorDiff::Clear); - assert_matches!(&diffs[1], VectorDiff::Append { values } => { - assert!(values.is_empty()); - }); } ); @@ -699,19 +696,13 @@ mod timed_tests { assert!(thread_events.is_empty()); // The event cache store is totally empty. - let linked_chunk = from_all_chunks::<3, _, _>( + assert!( event_cache_store .load_all_chunks(LinkedChunkId::Thread(room_id, thread_root)) .await - .unwrap(), - ) - .unwrap() - .unwrap(); - - // Note: while the event cache store could return `None` here, clearing it will - // reset it to its initial form, maintaining the invariant that it - // contains a single items chunk that's empty. - assert_eq!(linked_chunk.num_items(), 0); + .unwrap() + .is_empty() + ); } #[async_test] diff --git a/crates/matrix-sdk/tests/integration/event_cache/mod.rs b/crates/matrix-sdk/tests/integration/event_cache/mod.rs index 3d01aa650..1d888d30f 100644 --- a/crates/matrix-sdk/tests/integration/event_cache/mod.rs +++ b/crates/matrix-sdk/tests/integration/event_cache/mod.rs @@ -184,10 +184,8 @@ async fn test_ignored_unignored() { Ok(RoomEventCacheUpdate::UpdateTimelineEvents(TimelineVectorDiffs { diffs, .. })) = room_stream.recv() ); - assert_eq!(diffs.len(), 2); + assert_eq!(diffs.len(), 1); assert_let!(VectorDiff::Clear = &diffs[0]); - assert_let!(VectorDiff::Append { values } = &diffs[1]); - assert!(values.is_empty()); } // We do receive the new event. @@ -2452,10 +2450,8 @@ async fn test_clear_all_rooms() { Ok(RoomEventCacheUpdate::UpdateTimelineEvents(TimelineVectorDiffs { diffs, .. })) = room_updates.recv() ); - assert_eq!(diffs.len(), 2); + assert_eq!(diffs.len(), 1); assert_let!(VectorDiff::Clear = &diffs[0]); - assert_let!(VectorDiff::Append { values } = &diffs[1]); - assert!(values.is_empty()); // The sleeping room should have been cleared too. let (maybe_last_chunk, _chunk_id_gen) = diff --git a/crates/matrix-sdk/tests/integration/event_cache/threads.rs b/crates/matrix-sdk/tests/integration/event_cache/threads.rs index 52cff9e6a..5ba14d355 100644 --- a/crates/matrix-sdk/tests/integration/event_cache/threads.rs +++ b/crates/matrix-sdk/tests/integration/event_cache/threads.rs @@ -183,10 +183,8 @@ async fn test_ignored_user_empties_threads() { // We do receive a clear. { assert_let_timeout!(Ok(TimelineVectorDiffs { diffs, .. }) = thread_stream.recv()); - assert_eq!(diffs.len(), 2); + assert_eq!(diffs.len(), 1); assert_let!(VectorDiff::Clear = &diffs[0]); - assert_let!(VectorDiff::Append { values } = &diffs[1]); - assert!(values.is_empty()); } // Receiving new events still works.