This repository was archived by the owner on Sep 11, 2024. It is now read-only.
Reward users for using stable ordering in their room list - #5047
Merged
Conversation
This cuts the render time in half (from ~448ms to ~200ms on my account) per received event, as we're no longer re-mounting the entire room list and instead just the section(s) we care about.
Any time we though that the room list had to re-render we were dynamically creating a new addRoomFn, which would signal to the sublist that it needed to re-render. The only reason we wrap the function from the aesthetics is to provide theoretical tiling/multiaccount support through use of different dispatchers, however considering that's not a reality yet we can just use a default dispatcher when none is supplied.
We can ignore off-screen updates, so do that. See diff for more details on what we're doing.
In 7b97c30 we reduced the RoomList updates to just added/removed sublists, but didn't consider that we might also have to handle lengths of those sublists changing enough for us to fix the sticky headers.
In 9969b01 we stopped updating the sublist whenever we felt like it, which indirectly froze message previews for room tiles (badges, unread state, etc were unaffected because that is managed by a different store). To fix this, we simply have to listen for changes and perform an update.
... because it's faster. Also we don't need to diff the array here.
Collaborator
How far away is that later commit? Something that will come before the next release? |
Member
Author
|
Should be today, if not Monday. Small/medium accounts won't even notice, just the large accounts that opted to show massive numbers of rooms. |
jryans
approved these changes
Jul 24, 2020
jryans
left a comment
jryans
left a comment
Collaborator
There was a problem hiding this comment.
Looks good to me, let's make sure to restore filtering perf before release though. 😄
See commit diff for details.
Member
Author
|
@jryans can I get a quick look at the fixes for the e2e tests? They're a bit more involved. |
jryans
approved these changes
Jul 24, 2020
Co-authored-by: J. Ryan Stinnett <jryans@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is probably best reviewed commit-by-commit, however a
summarysmall novel explaining the theory is included here.This optimizes the path for room list renders by ensuring that the scope of those renders is limited to only what is absolutely required. Users with a stable room list ordering will see the most benefit of this change, however large accounts with the activity & unread first options selected will see some gain too. Users with stable ordering will see the most benefit as they have the highest chance of getting updates to their rooms which are out of view or can be no-oped (no order change means we don't need to remount the entire sublist).
Larger accounts (particularly those with activity sorting and unread first enabled) will still see a hit for a delta in their room list, but changes which don't reorder their list should be a lot better. Due to account size this is expected to be relatively rare though: it is more likely that an argument will break out in a room and cause it to stay in the same position at the top of the room list, reducing the impact of renders. The happy paths for larger accounts are still improved (~100ms in gains in my testing, all told) though we are reaching diminishing returns on handling this use case. Large accounts are still recommended to show as little rooms as possible to rely on filtering instead.
However, this regresses filtering slightly in that it now can take 2x as long on the initial filter, with subsequent iterations being a whole lot faster. This is expected to be improved in a later commit.