Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Cache the RoomListService's rooms #2163

Merged
merged 3 commits into from
Jun 28, 2023
Merged

feat: Cache the RoomListService's rooms #2163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ccd5e87
feat: Cache the `RoomListService`'s rooms
bnjbvr Jun 27, 2023
e3df974
chore: remove useless async
bnjbvr Jun 27, 2023
98fed1d
chore: use smaller critical sections
bnjbvr Jun 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 19 additions & 5 deletions crates/matrix-sdk-ui/src/room_list/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ mod room;
mod room_list;
mod state;

use std::{future::ready, sync::Arc};
use std::{collections::BTreeMap, future::ready, sync::Arc};

use async_stream::stream;
use eyeball::{shared::Observable, Subscriber};
Expand All @@ -87,11 +87,14 @@ use ruma::{
};
pub use state::*;
use thiserror::Error;
use tokio::sync::RwLock;

/// The [`RoomListService`] type. See the module's documentation to learn more.
#[derive(Debug)]
pub struct RoomListService {
sliding_sync: Arc<SlidingSync>,
/// Room cache, to avoid recreating `Room`s everytime users fetch them.
rooms: Arc<RwLock<BTreeMap<OwnedRoomId, Room>>>,
state: Observable<State>,
}

Expand Down Expand Up @@ -155,7 +158,7 @@ impl RoomListService {
.map(Arc::new)
.map_err(Error::SlidingSync)?;

Ok(Self { sliding_sync, state: Observable::new(State::Init) })
Ok(Self { sliding_sync, state: Observable::new(State::Init), rooms: Default::default() })
}

/// Start to sync the room list.
Expand Down Expand Up @@ -282,10 +285,21 @@ impl RoomListService {

/// Get a [`Room`] if it exists.
pub async fn room(&self, room_id: &RoomId) -> Result<Room, Error> {
match self.sliding_sync.get_room(room_id).await {
Some(room) => Room::new(self.sliding_sync.clone(), room).await,
None => Err(Error::RoomNotFound(room_id.to_owned())),
{
let rooms = self.rooms.read().await;
if let Some(room) = rooms.get(room_id) {
return Ok(room.clone());
}
}

let room = match self.sliding_sync.get_room(room_id).await {
Some(room) => Room::new(self.sliding_sync.clone(), room)?,
None => return Err(Error::RoomNotFound(room_id.to_owned())),
};

self.rooms.write().await.insert(room_id.to_owned(), room.clone());
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems sensible, though are we sure that we never need to remove items from this list?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.


Ok(room)
}

#[cfg(any(test, feature = "testing"))]
Expand Down
2 changes: 1 addition & 1 deletion crates/matrix-sdk-ui/src/room_list/room.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ struct RoomInner {

impl Room {
/// Create a new `Room`.
pub(super) async fn new(
pub(super) fn new(
sliding_sync: Arc<SlidingSync>,
sliding_sync_room: SlidingSyncRoom,
) -> Result<Self, Error> {
Expand Down