Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Hywan
Copy link
Member

@Hywan Hywan commented Feb 1, 2025

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:

Screenshot 2025-02-01 at 11 59 37

And after:

Screenshot 2025-02-01 at 11 26 08

@Hywan Hywan requested a review from a team as a code owner February 1, 2025 11:02
@Hywan Hywan requested review from andybalaam and poljar and removed request for a team and andybalaam February 1, 2025 11:02
Copy link

codecov bot commented Feb 1, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.74%. Comparing base (57919f5) to head (f6f7ea2).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4601      +/-   ##
==========================================
+ Coverage   85.73%   85.74%   +0.01%     
==========================================
  Files         291      291              
  Lines       33288    33292       +4     
==========================================
+ Hits        28538    28545       +7     
+ Misses       4750     4747       -3     

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

…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
matrix-org#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
@Hywan Hywan force-pushed the fix-ui-performance-deduplicate-local-timeline-item branch from d8ac3b5 to f6f7ea2 Compare February 1, 2025 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant