"Breadcrumb" room sorting algorithm #2634
Conversation
This changes the approach from regenerating every time there's a change to incrementally fixing the room lists. Additionally, this forces the pin options on for people and implements the sticky room behaviour. Known bugs include newly joined rooms, invites, etc not sorting correctly.
They are not forced on, and do nothing.
This is a very blunt approach in that it ignores the sticky room.
When a room is read on another device, it should be re-sorted
When we load the page, all encrypted events arrive well after we've generated our initial grouping which can cause them to jump to the top of their categories wrongly. For direct chats, this meant that people who don't have a lot of unread messages would see ancient rooms bubbling to the top for no reason after the page has loaded. We still have to track when the last category change was (ie: when we switched from red -> grey) so that when the category doesn't exist in the list we can insert the room at the right place (the start of the last category when we switch beyond the order expected).
|
Early performance characteristics (based on my account; before optimization):
I'm not entirely sure how the old algorithm achieves 0.13ms initial generation time, but it is at least consistent given the same algorithm is used for incremental changes. Suspicion for the new initial list generation taking 400ms is lack of caching, so we'll see how that goes. |
We don't support dragging, so don't test it.
turt2live
changed the title
| } | ||
| } | ||
|
|
||
| _setRoomCategory(room, category) { |
There was a problem hiding this comment.
so big ...splitting it up would make it more digestible, but time I guess. Wouldn't spend time on this now, but maybe worth doing in the future.
There was a problem hiding this comment.
tbh I'm not sure how possible that is without long function definitions. With the exception of a few lines, everything directly modifies the collection being iterated on.
The algorithm is correctly applied when these are removed.
There was a problem hiding this comment.
generality lgtm, a bit worried about the perf regressions, and any potential breakage given the depth of the changes. Maybe we should merge this just behind an upcoming hotfix release to have it on /develop for as long as possible, wdyt @lampholder?
|
sorry, this technically wasn't back up for review while I'm investigating why its so bad at performance. Intention is to make it transparent, which will probably happen by the time we cut another point release. |
|
I lied: I thought this would take longer to optimize, but I was wrong. After staring at profiles for a few hours and making small changes I've managed to cut it down to the following characteristics:
|
|
ps: executive decision is to merge it asap for testing |
Caution: This does not use a labs flag and alters how the room list sorting is done.
This is being opened now to give people a point to test off of without impacting develop.
Fixes element-hq/element-web#8531
Fixes element-hq/element-web#8122
Fixes element-hq/element-web#7826
Fixes element-hq/element-web#7803
(well, not yet...)Fixes element-hq/element-web#8774
Known bugs:
UNCONFIRMED:Potentially high CPU usage trying to math all this outRoom for improvement: