From 77a67de7df2b4951f52d94fff655d74ab173364b Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Mon, 3 Feb 2025 14:58:35 +0100 Subject: [PATCH] fix(ui): Fix performance of `ReadReceiptTimelineUpdate::apply`. This patch improves the performance of `ReadReceiptTimelineUpdate::apply`, which does 2 things: it calls `remove_old_receipt` and `add_new_receipt`. Both of them need an timeline item position. Until this patch, `rfind_event_by_id` was used and was the bottleneck. The improvement is twofold as is as follows. First off, when collecting data to create `ReadReceiptTimelineUpdate`, the timeline item position can be known ahead of time by using `EventMeta::timeline_item_index`. This data is not always available, for example if the timeline item isn't created yet. But let's try to collect these data if there are some. Second, inside `ReadReceiptTimelineUpdate::remove_old_receipt`, we use the timeline item position collected from `EventMeta` if it exists. Otherwise, let's fallback to a similar `rfind_event_by_id` pattern, without using intermediate types. It's more straightforward here: we don't need an `EventTimelineItemWithId`, we only need the position. Once the position is known, it is stored in `Self` (!), this is the biggest improvement here. Le't see why. Finally, inside `ReadReceiptTimelineUpdate::add_new_receipt`, we use the timeline item position collected from `EventMeta` if it exists, similarly to what `remove_old_receipt` does. Otherwise, let's fallback to an iterator to find the position. However, instead of iterating over **all** items, we can skip the first ones, up to the position of the timeline item holding the old receipt, so up to the position found by `remove_old_receipt`. I'm testing this patch with the `test_lazy_back_pagination` test in https://github.com/matrix-org/matrix-rust-sdk/pull/4594. With 10_000 events in the sync, the `ReadReceipts::maybe_update_read_receipt` method was taking 52% of the whole execution time. With this patch, it takes 8.1%. --- .../src/timeline/controller/read_receipts.rs | 87 ++++++++++++++++--- 1 file changed, 73 insertions(+), 14 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/controller/read_receipts.rs b/crates/matrix-sdk-ui/src/timeline/controller/read_receipts.rs index ea7ff6426..84055b5de 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/read_receipts.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/read_receipts.rs @@ -101,6 +101,7 @@ impl ReadReceipts { // Get old receipt. let old_receipt = self.get_latest(new_receipt.user_id, &new_receipt.receipt_type); + if old_receipt .as_ref() .is_some_and(|(old_receipt_event_id, _)| old_receipt_event_id == new_receipt.event_id) @@ -108,27 +109,35 @@ impl ReadReceipts { // The receipt has not changed so there is nothing to do. return; } + let old_event_id = old_receipt.map(|(event_id, _)| event_id); // Find receipts positions. let mut old_receipt_pos = None; + let mut old_item_pos = None; let mut old_item_event_id = None; let mut new_receipt_pos = None; + let mut new_item_pos = None; let mut new_item_event_id = None; + for (pos, event) in all_events.iter().rev().enumerate() { - if old_event_id == Some(&event.event_id) { + if old_receipt_pos.is_none() && old_event_id == Some(&event.event_id) { old_receipt_pos = Some(pos); } + // The receipt should appear on the first event that is visible. if old_receipt_pos.is_some() && old_item_event_id.is_none() && event.visible { + old_item_pos = event.timeline_item_index; old_item_event_id = Some(event.event_id.clone()); } - if new_receipt.event_id == event.event_id { + if new_receipt_pos.is_none() && new_receipt.event_id == event.event_id { new_receipt_pos = Some(pos); } + // The receipt should appear on the first event that is visible. if new_receipt_pos.is_some() && new_item_event_id.is_none() && event.visible { + new_item_pos = event.timeline_item_index; new_item_event_id = Some(event.event_id.clone()); } @@ -166,6 +175,7 @@ impl ReadReceipts { if let Some(old_event_id) = old_event_id.cloned() { self.remove_event_receipt_for_user(&old_event_id, new_receipt.user_id); } + // Add the new receipt to the new event. self.add_event_receipt_for_user( new_receipt.event_id.to_owned(), @@ -193,9 +203,12 @@ impl ReadReceipts { } let timeline_update = ReadReceiptTimelineUpdate { + old_item_pos, old_event_id: old_item_event_id, + new_item_pos, new_event_id: new_item_event_id, }; + timeline_update.apply( timeline_items, new_receipt.user_id.to_owned(), @@ -273,27 +286,51 @@ struct FullReceipt<'a> { /// A read receipt update in the timeline. #[derive(Clone, Debug, Default)] struct ReadReceiptTimelineUpdate { + /// The position of the timeline item that had the old receipt of the user, + /// if any. + old_item_pos: Option, /// The old event that had the receipt of the user, if any. old_event_id: Option, + /// The position of the timeline item that has the new receipt of the user, + /// if any. + new_item_pos: Option, /// The new event that has the receipt of the user, if any. new_event_id: Option, } impl ReadReceiptTimelineUpdate { /// Remove the old receipt from the corresponding timeline item. - fn remove_old_receipt(&self, items: &mut ObservableItemsTransaction<'_>, user_id: &UserId) { + fn remove_old_receipt(&mut self, items: &mut ObservableItemsTransaction<'_>, user_id: &UserId) { let Some(event_id) = &self.old_event_id else { // Nothing to do. return; }; - let Some((receipt_pos, event_item)) = rfind_event_by_id(items, event_id) else { + let item_pos = self.old_item_pos.or_else(|| { + items + .iter() + .enumerate() + .rev() + .filter_map(|(nth, item)| Some((nth, item.as_event()?))) + .find_map(|(nth, event_item)| { + (event_item.event_id() == Some(event_id)).then_some(nth) + }) + }); + + let Some(item_pos) = item_pos else { debug!(%event_id, %user_id, "inconsistent state: old event item for read receipt was not found"); return; }; - let event_item_id = event_item.internal_id.to_owned(); - let mut event_item = event_item.clone(); + self.old_item_pos = Some(item_pos); + + let event_item = &items[item_pos]; + let event_item_id = event_item.unique_id().to_owned(); + + let Some(mut event_item) = event_item.as_event().cloned() else { + warn!("received a read receipt for a virtual item, this should not be possible"); + return; + }; if let Some(remote_event_item) = event_item.as_remote_mut() { if remote_event_item.read_receipts.swap_remove(user_id).is_none() { @@ -303,7 +340,7 @@ impl ReadReceiptTimelineUpdate { receipt doesn't have a receipt for the user" ); } - items.replace(receipt_pos, TimelineItem::new(event_item, event_item_id)); + items.replace(item_pos, TimelineItem::new(event_item, event_item_id)); } else { warn!("received a read receipt for a local item, this should not be possible"); } @@ -321,18 +358,40 @@ impl ReadReceiptTimelineUpdate { return; }; - let Some((receipt_pos, event_item)) = rfind_event_by_id(items, &event_id) else { - // This can happen for new timeline items, the receipts will be loaded directly - // during construction of the item. + let item_pos = self.new_item_pos.or_else(|| { + items + .iter() + .enumerate() + // Don't iterate over all items if the `old_item_pos` is known: the `item_pos` + // for the new item is necessarily _after_ the old item. + .skip(self.old_item_pos.unwrap_or(0)) + .rev() + .filter_map(|(nth, item)| Some((nth, item.as_event()?))) + .find_map(|(nth, event_item)| { + (event_item.event_id() == Some(&event_id)).then_some(nth) + }) + }); + + let Some(item_pos) = item_pos else { + debug!(%event_id, %user_id, "inconsistent state: new event item for read receipt was not found"); return; }; - let event_item_id = event_item.internal_id.to_owned(); - let mut event_item = event_item.clone(); + debug_assert!( + item_pos >= self.old_item_pos.unwrap_or(0), + "The new receipt must be added on a timeline item that is _after_ the timeline item that was holding the old receipt"); + + let event_item = &items[item_pos]; + let event_item_id = event_item.unique_id().to_owned(); + + let Some(mut event_item) = event_item.as_event().cloned() else { + warn!("received a read receipt for a virtual item, this should not be possible"); + return; + }; if let Some(remote_event_item) = event_item.as_remote_mut() { remote_event_item.read_receipts.insert(user_id, receipt); - items.replace(receipt_pos, TimelineItem::new(event_item, event_item_id)); + items.replace(item_pos, TimelineItem::new(event_item, event_item_id)); } else { warn!("received a read receipt for a local item, this should not be possible"); } @@ -340,7 +399,7 @@ impl ReadReceiptTimelineUpdate { /// Apply this update to the timeline. fn apply( - self, + mut self, items: &mut ObservableItemsTransaction<'_>, user_id: OwnedUserId, receipt: Receipt,