Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Timeline rendering optimizations #6143

Merged
merged 9 commits into from
Jun 9, 2021

Conversation

robintown
Copy link
Member

@robintown robintown commented Jun 5, 2021

I managed to improve timeline rendering performance significantly by caching a few frequently hit settings values in RoomContext, and introducing a faster function to determine whether events have text to display, rather than abusing textForEvent.

These changes were targeted at improving MELS performance, since having a large number of member events in one place is one of the easiest ways to get noticeable lag, but none of the slowdowns I saw ended up being specific to MELS.

Before (scrolling through #keepassxc:matrix.org):
before
After (scrolling through #keepassxc:matrix.org):
after

Closes element-hq/element-web#17537, because I was specifically targeting the slowdowns highlighted in that issue.
Closes element-hq/element-web#10371, because this changes the operations referred to in that issue to no longer be expensive.

I also think this should be a big enough improvement to say:
Closes element-hq/element-web#11963.
Closes element-hq/element-web#9061.
Closes element-hq/element-web#7015.
…though I would like confirmation on those, since in my testing even though I visited the same rooms with hundreds of grouped member events, I couldn't really reproduce the claims of ~1 minute hangs. I think I've fixed what is likely to have caused those, though.

An accidental variable shadowing was preventing setting watcher
callbacks from being fired.

Signed-off-by: Robin Townsend <[email protected]>
Signed-off-by: Robin Townsend <[email protected]>
Signed-off-by: Robin Townsend <[email protected]>
@robintown robintown changed the title Cache settings values for determining whether events are hidden Timeline rendering optimizations Jun 7, 2021
@t3chguy t3chguy requested a review from a team June 7, 2021 10:24
Copy link
Contributor

@germain-gg germain-gg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for working on this!
That makes a lot of sense and it seems that you managed to get some quite significant improvements that will increase the overall smoothness of the application

I did not realise that the SettingsStore calls were that expensive and caching the values looks like a good addition to what we have.
I am wondering whether that caching could exist within the SettingsStore directly so that the entire application can benefit from those performance improvement? But I am not overly familiar with the difficulty this would entail

The changes in TextForEvent seem quite reasonable. The only thing that I am unsure about is that now the function name does not match with the return type of those function.
I can imagine that we could

  • Rename the functions
  • Migrate to TypeScript and have more explicit naming

To illustrate what I'm saying, before:

function textForMemberEvent(ev: MatrixEvent): string | null

After:

function textForMemberEvent(ev: MatrixEvent): () => string | null

@robintown
Copy link
Member Author

@gsouquet Yeah, there are very probably some general SettingsStore optimizations that would be good to look into in the future, but generally, caching and watching individual settings as needed seems to be the way to go, since that then enables components to dynamically update whenever those setting values happen to change (which is particularly useful for the timeline, as it can be open in the background while you flip switches in the settings menu). Michael pointed out to me the other day that there is a useSetting hook to make this super easy for functional components, so there's no reason not to use it.

Adding types to TextForEvent is a great idea, I'll get to that tomorrow evening :)

@robintown robintown requested a review from germain-gg June 9, 2021 01:59
Copy link
Contributor

@germain-gg germain-gg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks really good to me!
Definitely improving one of the most important interaction in the platform

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants