Skip to content

Commit f6f7ea2

Browse files
committed
fix(ui): Fix performance of TimelineEventHandler::deduplicate_local_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
1 parent 57919f5 commit f6f7ea2

File tree

1 file changed

+42
-12
lines changed

1 file changed

+42
-12
lines changed

crates/matrix-sdk-ui/src/timeline/event_handler.rs

Lines changed: 42 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
// See the License for the specific language governing permissions and
1313
// limitations under the License.
1414

15-
use std::sync::Arc;
15+
use std::{ops::ControlFlow, sync::Arc};
1616

1717
use as_variant::as_variant;
1818
use indexmap::IndexMap;
@@ -51,7 +51,7 @@ use ruma::{
5151
use tracing::{debug, error, field::debug, info, instrument, trace, warn};
5252

5353
use super::{
54-
algorithms::{rfind_event_by_id, rfind_event_item},
54+
algorithms::rfind_event_by_id,
5555
controller::{
5656
ObservableItemsTransaction, ObservableItemsTransactionEntry, PendingEdit, PendingEditKind,
5757
TimelineMetadata, TimelineStateTransaction,
@@ -1261,18 +1261,48 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> {
12611261
event_id: Option<&EventId>,
12621262
transaction_id: Option<&TransactionId>,
12631263
) -> Option<Arc<TimelineItem>> {
1264-
// Start with the canonical case: detect a local timeline item that matches
1265-
// `event_id` or `transaction_id`.
1266-
if let Some((local_timeline_item_index, local_timeline_item)) =
1267-
rfind_event_item(items, |event_timeline_item| {
1268-
if event_timeline_item.is_local_echo() {
1269-
event_id == event_timeline_item.event_id()
1270-
|| (transaction_id.is_some()
1271-
&& transaction_id == event_timeline_item.transaction_id())
1264+
// Detect a local timeline item that matches `event_id` or `transaction_id`.
1265+
if let Some((local_timeline_item_index, local_timeline_item)) = items
1266+
.iter()
1267+
// Get the index of each item.
1268+
.enumerate()
1269+
// Iterate from the end to the start.
1270+
.rev()
1271+
// Use a `Iterator::try_fold` to produce a single value, and to stop the iterator
1272+
// when a non local event timeline item is met. We want to stop iterating when:
1273+
//
1274+
// - a duplicate local event timeline item has been found,
1275+
// - a non local event timeline item is met,
1276+
// - a non event timeline is met.
1277+
//
1278+
// Indeed, it is a waste of time to iterate over all items in `items`. Local event
1279+
// timeline items are necessarily at the end of `items`: as soon as they have been
1280+
// iterated, we can stop the entire iteration.
1281+
.try_fold((), |(), (nth, timeline_item)| {
1282+
let Some(event_timeline_item) = timeline_item.as_event() else {
1283+
// Not an event timeline item? Stop iterating here.
1284+
return ControlFlow::Break(None);
1285+
};
1286+
1287+
// Not a local event timeline item? Stop iterating here.
1288+
if !event_timeline_item.is_local_echo() {
1289+
return ControlFlow::Break(None);
1290+
}
1291+
1292+
if event_id == event_timeline_item.event_id()
1293+
|| (transaction_id.is_some()
1294+
&& transaction_id == event_timeline_item.transaction_id())
1295+
{
1296+
// A duplicate local event timeline item has been found!
1297+
ControlFlow::Break(Some((nth, event_timeline_item)))
12721298
} else {
1273-
false
1299+
// This local event timeline is not the one we are looking for. Continue our
1300+
// search.
1301+
ControlFlow::Continue(())
12741302
}
12751303
})
1304+
.break_value()
1305+
.flatten()
12761306
{
12771307
trace!(
12781308
?event_id,
@@ -1281,7 +1311,7 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> {
12811311
"Removing local timeline item"
12821312
);
12831313

1284-
transfer_details(new_event_timeline_item, &local_timeline_item);
1314+
transfer_details(new_event_timeline_item, local_timeline_item);
12851315

12861316
// Remove the local timeline item.
12871317
return Some(items.remove(local_timeline_item_index));

0 commit comments

Comments
 (0)