Fix not showing display names in already synced rooms

[benjajaja]

Fixes #149

The display_names only got populated from the
OriginalSyncRoomMemberEvent. When opening an already synced room, display_names is not populated.

ClientWorker::get_room extends the returned FetchedRoom to include RoomMembers from Room::members_no_sync. This allows RoomState::new to populate the RoomInfo's display_names.

Thanks to @Benjamin-L for finding the source of the bug in that issue. It was mentioned that it might be problematic to iterate over all members of large rooms. The largest room I had available to test this was only about 200 members, but it seemed fine.

[Benjamin-L]

If you want a moderately large room to test with, #rust:matrix.org has 9,078 members. For a huge room, #matrix:matrix.org has 48,778.

(joining the latter room might induce a lot of load on your homeserver, so might be a good idea to do that on matrix.org or similar)

[Benjamin-L]

So I tested out this branch, and it does seem to fix the displayname bug, but it has a pretty significant perf impact. I ran a test opening the client to #rust:matrix.org:

2f51e96 (current head of benjajaja/fix_displayname_already_synced)

8943909 (current head of main)

The benjajaja/fix_displayname_already_synced build takes noticeably longer than main before the UI is visible when starting up. I don't have an easy way to measure this, but subjectively it's ~2s which lines up with the period of high cpu usage. This delay also occurs when opening a large room after startup.

I haven't tested with #matrix:matrix.org yet because I don't want to increase my HS database size by 1000%. Assuming scale linearly with user count, and it would have a ~13s period of frozen UI and high cpu usage.

---

My only real idea for improving this would be to fetch member details on-demand for each member, rather than all at once when the room is loaded. Unfortunately, this sounds much more complicated to implement, and likely prone to reintroducing the bug again in the future.

A second possibility would be to run the members_no_sync in the background, without blocking the UI on it. This wouldn't help with the cpu usage, but would probably help with the UI freezes.

It might be worth talking with some of the matrix-rust-sdk devs on to see if they have recommendations for handling rooms with large member counts. It's also probably worth noting that the bar for large-room perf is not very high. Element web handled the #matrix:matrix.org room very poorly last time I tried this (~1 year ago). Not sure about other clients.

[ulyssa]

I think that @Benjamin-L is right that the way to do this in order to avoid having to fetch the display names for all users in the room, and do it per-user, similar to how rooms signal while rendering visible content that they need more messages loaded:

• While looping over messages, their senders could be placed into a HashSet<OwnedUserId> if we don't have their display names available
• This set then gets copied into a HashSet<(OwnedRoomId, OwnedUserId)> in the ChatStore that indicates which room/user pairs need to have their membership information fetched
• An async loop in src/worker.rs periodically checks this set and loads information for just those users.

This does mean that user IDs might be displayed for a second or two when scroling back and seeing messages from users whose information hasn't been fetched, but it should be much lighter on the CPU and network.

[benjajaja]

Changing it to load it in worker/background.

@Benjamin-L thank you so much for perf-testing this. The current implementation is indeed unacceptably locking the UI.

@ulyssa I am not sure if we should only load displaynames for users that have messages currently in scrollback. Because then we get the "ID flashing before displayname" for any new incoming messages of a user that doesn't have some message in the recent scrollback. I would rather load members_no_sync at once on opening.

I also think we could use joined_members_no_sync which excludes invited- and left-members, unless I'm missing something.

[benjajaja]

Also perhaps room.display_name() and room.tags() in worker get_room could also be loaded in background, together with the members.

[Benjamin-L]

Changing it to load it in worker/background.

@Benjamin-L thank you so much for perf-testing this. The current implementation is indeed unacceptably locking the UI.

thanks :) If you push the background-loading code I should be able to run another test later today.

@ulyssa I am not sure if we should only load displaynames for users that have messages currently in scrollback. Because then we get the "ID flashing before displayname" for any new incoming messages of a user that doesn't have some message in the recent scrollback. I would rather load members_no_sync at once on opening.

We could probably avoid that problem for incoming messages by blocking the message event handlers on a call to get_member if the sender is not already in the members list. Another possibility for reducing (but not eliminating) flicker would be to do something like this instead of having the worker polling on a timer.

I also think we could use joined_members_no_sync which excludes invited- and left-members, unless I'm missing something.

I don't think that will work, since messages will stick around after a member leaves the room.

[benjajaja]

thanks :) If you push the background-loading code I should be able to run another test later today.

I have to do it properly, I just piggy-backed on the loading older messages thread. It does kinda work but it later re-fetches the members unnecessarily. I'll give it its own need_load flag or something, but I think I can still use the same task as load-older-messages.

We could probably avoid that problem for incoming messages by blocking the message event handlers on a call to get_member if the sender is not already in the members list. Another possibility for reducing (but not eliminating) flicker would be to do something like this instead of having the worker polling on a timer.

I would be very hesitant to block in the message event handler. Your solution of waking-up sounds much better, thanks!

I also think we could use joined_members_no_sync which excludes invited- and left-members, unless I'm missing something.

I don't think that will work, since messages will stick around after a member leaves the room.

Ah, of course.

[benjajaja]

@Benjamin-L this now has background-loading. If I understand correctly, this would play very nicely with your notify branch.

[Benjamin-L]

Ran a test with the new code (e71e02e).

This is pretty much what we would expect: it takes a few seconds before the displaynames pop in, and we still have the cpu spike, but the UI remains responsive the whole time. After the room was loaded for the first time, I didn't see any additional cpu spikes.

[ulyssa]

Thank you so much for fixing this @benjajaja , and thank you for testing its performance, @Benjamin-L !