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

New room list: large account performance #14035

Closed
turt2live opened this issue Jun 15, 2020 · 5 comments
Closed

New room list: large account performance #14035

turt2live opened this issue Jun 15, 2020 · 5 comments

Comments

@turt2live
Copy link
Member

Follow on from #14034 - make it not noticeable for large accounts.

@turt2live turt2live self-assigned this Jun 15, 2020
@turt2live turt2live removed their assignment Jun 28, 2020
@turt2live turt2live self-assigned this Jul 9, 2020
turt2live added a commit to matrix-org/matrix-react-sdk that referenced this issue Jul 10, 2020
For element-hq/element-web#14035

**This option is not recommended as it completely obliterates all chances of being able to support someone with a broken room list. It is intended for specific testing scenarios only.**
@turt2live
Copy link
Member Author

turt2live commented Jul 10, 2020

With the above PRs merged, performance is looking a lot better:
image

At 37.77ms per operation, most of which is 1-2ms function calls, we're doing pretty good. Incremental updates are well down from 100ms to just 3.15ms on average (previous metrics: matrix-org/matrix-react-sdk#4253). The remaining ~35ms are spent preparing for a render of the update which just took place.

We do still fire a boatload of updates though (as indicated by the flamegraph, which is only a tens of seconds of time).

When we disable all the logging, incremental generation falls by nearly a full millisecond.

Initial generation takes just 3.5ms on my account, without logging. With logging it's a bit longer but not by much.

Most of the rendering time appears to be in the depths of React - we have some control over this but overall the benefit of going into it much further isn't totally worth it at this point.

Although the numbers tell an amazing story, we're leaving this issue open until others with large accounts (Matthew) are happy with the overall performance. It's not expected to be perfect, but should be much closer to perfect than unusable.

@turt2live
Copy link
Member Author

for comparison: the old room list achieved 394ms initial generation and 1.5ms incremental, with the list before that getting 0.13ms initial and 0.26ms incremental.

The old old room list (~2018) got such great speeds because it wasn't doing anything terribly complicated, but with the "breadcrumb" (now called importance) algorithm there were a number of complex features introduced like sticky rooms and advanced sorting behaviours. When comparing the old room list versus this new one, the initial generation time is much better because we're tracking rooms differently - this is also what consequently increases the incremental time. We spend slightly more time making sure we're going to put a room in the right spot to make subsequent updates O(n) instead of exponential or logarithmic.

If we remove some safety code, we could probably shave a millisecond or two off the incremental time, however at a cost of a couple milliseconds I think it's worth keeping it in.

@turt2live
Copy link
Member Author

This got the seal of approval I was looking for: "it feels better".

@Half-Shot Half-Shot reopened this Jul 16, 2020
@Half-Shot
Copy link
Member

Half-Shot commented Jul 16, 2020

It's really not feeling better from my side. Admittedly, I am not running the latest and greatest hardware but I don't believe we are targeting those users anyway. I'll gather logs.

EDIT: Profiling information can be found at this permalink

The symptoms I'm seeing are incredibly slow app performance when scrolling, or clicking on a room. When searching for a room, the performance is near enough to how the old Riot behaved (i.e. showing a subset of the rooms gives good perf). Showing all rooms is abysmal however.

@turt2live
Copy link
Member Author

Please open new issues instead of reopening ones, it ruins our entire tracking process: #14539

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants