fix(ui): Fix performance of TimelineEventHandler::deduplicate_local_timeline_item

[Hywan]

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.

Flamegraph of the TimelineEventHandler::add_item method (that calls deduplicate_local_timeline_item), before this patch:

And after:

---

• Address [meta] EventCache storage #3280.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.73%. Comparing base (47d0868) to head (e8edb9d).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4601      +/-   ##
==========================================
+ Coverage   85.72%   85.73%   +0.01%     
==========================================
  Files         291      291              
  Lines       33288    33292       +4     
==========================================
+ Hits        28536    28543       +7     
+ Misses       4752     4749       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[bnjbvr]

You don't need the rust-version bump if you use find_map(), otherwise lgtm.

[bnjbvr]

I think you can use a find_map() and not have to bump the minimum rust version, and it would be a bit more noob-friendly :)

[Hywan]

Iterator::find_map is not appropriate here. It returns the first non-None value. It means that we cannot differentiate the break the loop because we've found a value, vs. break the loop because no value can be found.

We need to stop the iteration when all local event timeline items have been traversed. This is not possible to represent that with Iterator::find_map. That's exactly the problem this patch is trying to fix :-).

[bnjbvr]

Why not a plain loop then… 😇

[Hywan]

That, that's a good question ;-).