feat: Cache the RoomListService's rooms
#2163
Conversation
That avoids recreating a timeline object every time a user calls `RoomListService::room()` with the same room id, so that should speed up operations like fetching the latest event for rooms we've already entered before.
This opens up a possible race condition where an embedder calls the method twice, generating two new rooms, but then it's fine as long as they don't cache them somewhere, which we expect they don't.
| None => return Err(Error::RoomNotFound(room_id.to_owned())), | ||
| }; | ||
|
|
||
| self.rooms.write().await.insert(room_id.to_owned(), room.clone()); |
There was a problem hiding this comment.
Seems sensible, though are we sure that we never need to remove items from this list?
There was a problem hiding this comment.
If a room has been left, yeah it should be removed. Ivan has ideas that involve using a RingBuffer instead here, for a limited amount of Rooms, but that limit would make this fix irrelevant.
We're spamming the embedder with Filled events for every room, after a background -> foreground transition, and that's likely the root cause to fix.
There was a problem hiding this comment.
The only clean solution I can think of, is to remove RoomListEntry::Invalidate entirely, thus reducing the number of Invalidate and Filled updates that are sent to the user. With that, combined to a latest_event stored inside RoomInfo (cf #2085), it would help to remove this cache entirely.
Let's consider this as a hot fix. I would add a TODO note though.
| None => return Err(Error::RoomNotFound(room_id.to_owned())), | ||
| }; | ||
|
|
||
| self.rooms.write().await.insert(room_id.to_owned(), room.clone()); |
There was a problem hiding this comment.
The only clean solution I can think of, is to remove RoomListEntry::Invalidate entirely, thus reducing the number of Invalidate and Filled updates that are sent to the user. With that, combined to a latest_event stored inside RoomInfo (cf #2085), it would help to remove this cache entirely.
Let's consider this as a hot fix. I would add a TODO note though.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #2163 +/- ##
=======================================
Coverage 76.08% 76.08%
=======================================
Files 163 163
Lines 17280 17285 +5
=======================================
+ Hits 13148 13152 +4
- Misses 4132 4133 +1
☔ View full report in Codecov by Sentry. |
That avoids recreating a timeline object every time a user calls
RoomListService::room()with the same room id, so that should speed up operations like fetching the latest event for rooms we've already entered before.Note: I haven't added explicit tests for the caching, because there are already some tests which make use of the same room id, so they would hit the caching paths.
Part of #2161