From a1a5b85fd84a3e8035fc84565034f716c394a42b Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Tue, 19 Sep 2023 10:29:49 +0200 Subject: [PATCH 1/4] feat(ui): Remove one `Mutex`. This patch removes the `Mutex` around `Subscriber>` inside the `RoomListDynamicEntriesController`. The `Mutex` was necessary to get a mutable reference, so that `Subscriber::next_now` could have been used. However, it's not necessary to use `new_now` in this particular context. We can use `get` instead, which take an immutable reference, thus removing the need for the `Mutex`. A `Mutex` has a non-negligeable cost. This function can be used in a critical hot path, and must as fast as possible. --- .../src/room_list_service/room_list.rs | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/crates/matrix-sdk-ui/src/room_list_service/room_list.rs b/crates/matrix-sdk-ui/src/room_list_service/room_list.rs index 710bf4bac..abe5ba05b 100644 --- a/crates/matrix-sdk-ui/src/room_list_service/room_list.rs +++ b/crates/matrix-sdk-ui/src/room_list_service/room_list.rs @@ -12,10 +12,7 @@ // See the License for that specific language governing permissions and // limitations under the License. -use std::{ - future::ready, - sync::{Arc, Mutex}, -}; +use std::{future::ready, sync::Arc}; use async_cell::sync::AsyncCell; use async_rx::StreamExt as _; @@ -211,7 +208,7 @@ pub struct RoomListDynamicEntriesController { filter: Arc>, page_size: usize, limit: SharedObservable, - maximum_number_of_rooms: Mutex>>, + maximum_number_of_rooms: Subscriber>, } impl RoomListDynamicEntriesController { @@ -221,12 +218,7 @@ impl RoomListDynamicEntriesController { limit_stream: SharedObservable, maximum_number_of_rooms: Subscriber>, ) -> Self { - Self { - filter, - page_size, - limit: limit_stream, - maximum_number_of_rooms: Mutex::new(maximum_number_of_rooms), - } + Self { filter, page_size, limit: limit_stream, maximum_number_of_rooms } } /// Set the filter. @@ -251,7 +243,7 @@ impl RoomListDynamicEntriesController { /// Add one page, i.e. view `page_size` more entries in the room list if /// any. pub fn add_one_page(&self) { - let Some(max) = self.maximum_number_of_rooms.lock().unwrap().next_now() else { + let Some(max) = self.maximum_number_of_rooms.get() else { return; }; From c2166c50b1cc6f1026671c2ca872b3b3cbdb081e Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Tue, 19 Sep 2023 10:33:19 +0200 Subject: [PATCH 2/4] chore(ui): Remove a useless clone. It was necessary before 0f7e5ba4b080f99a6e473d2de2716d0697fa76fb, but it isn't anymore. --- crates/matrix-sdk-ui/src/room_list_service/room_list.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/matrix-sdk-ui/src/room_list_service/room_list.rs b/crates/matrix-sdk-ui/src/room_list_service/room_list.rs index abe5ba05b..533fb459f 100644 --- a/crates/matrix-sdk-ui/src/room_list_service/room_list.rs +++ b/crates/matrix-sdk-ui/src/room_list_service/room_list.rs @@ -135,7 +135,7 @@ impl RoomList { let dynamic_entries_controller = RoomListDynamicEntriesController::new( filter_fn_cell.clone(), page_size, - limit.clone(), + limit, list.maximum_number_of_rooms_stream(), ); From d0da4ae159d6b3f6edfb8c81570b73ebbad5fb28 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Tue, 19 Sep 2023 10:34:27 +0200 Subject: [PATCH 3/4] feat(ui): Update the `limit` if it's different. Use `SharedObservable::set_if_not_eq` instead of `::set`, so that it doesn't update the limit to the same value, which would be unnecesary in this case. --- crates/matrix-sdk-ui/src/room_list_service/room_list.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/matrix-sdk-ui/src/room_list_service/room_list.rs b/crates/matrix-sdk-ui/src/room_list_service/room_list.rs index 533fb459f..d69440efe 100644 --- a/crates/matrix-sdk-ui/src/room_list_service/room_list.rs +++ b/crates/matrix-sdk-ui/src/room_list_service/room_list.rs @@ -255,13 +255,13 @@ impl RoomListDynamicEntriesController { // `max - limit < page_size`, and that's perfectly fine. It's OK to have a // `limit` greater than `max`, but it's not OK to increase the limit // indefinitely. - self.limit.set(limit + self.page_size); + self.limit.set_if_not_eq(limit + self.page_size); } } /// Reset the one page, i.e. forget all pages and move back to the first /// page. pub fn reset_to_one_page(&self) { - self.limit.set(self.page_size); + self.limit.set_if_not_eq(self.page_size); } } From af7cf3148b85bc9debe94212a91105cf9af69525 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Tue, 19 Sep 2023 10:40:02 +0200 Subject: [PATCH 4/4] test(ui): Ensure that it's fine to `reset_to_one_page` multiple times. --- crates/matrix-sdk-ui/tests/integration/room_list_service.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crates/matrix-sdk-ui/tests/integration/room_list_service.rs b/crates/matrix-sdk-ui/tests/integration/room_list_service.rs index 4805aebf9..30709fe17 100644 --- a/crates/matrix-sdk-ui/tests/integration/room_list_service.rs +++ b/crates/matrix-sdk-ui/tests/integration/room_list_service.rs @@ -1851,6 +1851,10 @@ async fn test_dynamic_entries_stream() -> Result<(), Error> { } assert_pending!(dynamic_entries_stream); + // Let's reset to one page again, it should do nothing. + dynamic_entries.reset_to_one_page(); + assert_pending!(dynamic_entries_stream); + // Let's ask one more page again, because it's fun. dynamic_entries.add_one_page();