Skip to content

Commit

Permalink
fix(ui): Fix performance of `TimelineEventHandler::deduplicate_local_…
Browse files Browse the repository at this point in the history
…timeline_item`.

This patch drastically improves the performance of
`TimelineEventHandler::deduplicate_local_timeline_item`.

Before this patch, `rfind_event_item` was used to iterate over all
timeline items: for each item in reverse order, if it was an event
timeline item, and if it was a local event timeline item, and if it was
matching the event ID or transaction ID, then a duplicate was found.

The problem is the following: all items are traversed.

However, local event timeline items are always at the back of the items.
Even virtual timeline items are before local event timeline items. Thus,
it is not necessary to traverse all items. It is possible to stop the
iteration as soon as (i) a non event timeline item is met, or (ii) a non
local event timeline item is met.

This patch updates
`TimelineEventHandler::deduplicate_local_timeline_item` to replace to
use of `rfind_event_item` by a custom iterator that stops as soon as a
non event timeline item, or a non local event timeline item, is met, or
—of course— when a local event timeline item is a duplicate.

To do so, [`Iterator::try_fold`] is probably the best companion.
[`Iterator::try_find`] would have been nice, but it is available on
nightlies, not on stable versions of Rust. However, many methods in
`Iterator` are using `try_fold`, like `find` or any other methods that
need to do a “short-circuit”. Anyway, `try_fold` works pretty nice here,
and does exactly what we need.

Our use of `try_fold` is to return a `ControlFlow<Option<(usize,
TimelineItem)>, ()>`. After `try_fold`, we call
`ControlFlow::break_value`, which returns an `Option`. Hence the need
to call `Option::flatten` at the end to get a single `Option` instead of
having an `Option<Option<(usize, TimelineItem)>>`.

I'm testing this patch with the `test_lazy_back_pagination` test in
#4594. With 10_000
events in the sync, the test was taking 13s to run on my machine. With
this patch, it takes 10s to run. It's a 23% improvement. This
`deduplicate_local_timeline_item` method was taking a large part of the
computation according to the profiler. With this patch, this method is
barely visible in the profiler it is so small.

[`Iterator::try_fold`]: https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.try_fold
[`Iterator::try_find`]: https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.try_find
  • Loading branch information
Hywan committed Feb 1, 2025
1 parent 57919f5 commit d8ac3b5
Showing 1 changed file with 41 additions and 11 deletions.
52 changes: 41 additions & 11 deletions crates/matrix-sdk-ui/src/timeline/event_handler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

use std::sync::Arc;
use std::{ops::ControlFlow, sync::Arc};

use as_variant::as_variant;
use indexmap::IndexMap;
Expand Down Expand Up @@ -51,7 +51,7 @@ use ruma::{
use tracing::{debug, error, field::debug, info, instrument, trace, warn};

use super::{
algorithms::{rfind_event_by_id, rfind_event_item},
algorithms::rfind_event_by_id,
controller::{
ObservableItemsTransaction, ObservableItemsTransactionEntry, PendingEdit, PendingEditKind,
TimelineMetadata, TimelineStateTransaction,
Expand Down Expand Up @@ -1261,18 +1261,48 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> {
event_id: Option<&EventId>,
transaction_id: Option<&TransactionId>,
) -> Option<Arc<TimelineItem>> {
// Start with the canonical case: detect a local timeline item that matches
// `event_id` or `transaction_id`.
if let Some((local_timeline_item_index, local_timeline_item)) =
rfind_event_item(items, |event_timeline_item| {
if event_timeline_item.is_local_echo() {
event_id == event_timeline_item.event_id()
|| (transaction_id.is_some()
&& transaction_id == event_timeline_item.transaction_id())
// Detect a local timeline item that matches `event_id` or `transaction_id`.
if let Some((local_timeline_item_index, local_timeline_item)) = items
.iter()
// Get the index of each item.
.enumerate()
// Iterate from the end to the start.
.rev()
// Use a `Iterator::try_fold` to produce a single value, and to stop the iterator
// when a non local event timeline item is met. We want to stop iterating when:
//
// - a duplicate local event timeline item has been found,
// - a non local event timeline item is met,
// - a non event timeline is met.
//
// Indeed, it is a waste of time to iterate over all items in `items`. Local event
// timeline items are necessarily at the end of `items`: as soon as they have been
// iterated, we can stop the entire iteration.
.try_fold((), |(), (nth, timeline_item)| {
let Some(event_timeline_item) = timeline_item.as_event() else {
// Not an event timeline item? Stop iterating here.
return ControlFlow::Break(None);
};

// Not a local event timeline item? Stop iterating here.
if !event_timeline_item.is_local_echo() {
return ControlFlow::Break(None);
}

if event_id == event_timeline_item.event_id()
|| (transaction_id.is_some()
&& transaction_id == event_timeline_item.transaction_id())
{
// A duplicate local event timeline item has been found!
ControlFlow::Break(Some((nth, event_timeline_item)))
} else {
false
// This local event timeline is not the one we are looking for. Continue our
// search.
ControlFlow::Continue(())
}
})
.break_value()
.flatten()
{
trace!(
?event_id,
Expand Down

0 comments on commit d8ac3b5

Please sign in to comment.