From 62eb1996d917fb1928bdb9bba40d78a6eefe0bbd Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 17 Sep 2025 16:27:08 +0200 Subject: [PATCH] feat(base): Add `new_latest_event_timestamp` and `new_latest_event_is_local`. This patch adds 2 methods on `RoomInfo`: `new_latest_event_timestamp` and `new_latest_event_is_local`, which respectively returns `LatestEventValue::timestamp` and `LatestEventValue::is_local`. The goal is to avoid cloning a `LatestEventValue` when it's useless. For example, in the room list sorters! This patch also updates the room list sorters `recency` and `latest_event` to use these new methods. It improves the speed and throughput by 18%. --- crates/matrix-sdk-base/src/latest_event.rs | 12 ++ .../matrix-sdk-base/src/room/latest_event.rs | 14 ++ .../room_list_service/sorters/latest_event.rs | 144 ++++++++++++------ .../src/room_list_service/sorters/recency.rs | 46 ++---- 4 files changed, 133 insertions(+), 83 deletions(-) diff --git a/crates/matrix-sdk-base/src/latest_event.rs b/crates/matrix-sdk-base/src/latest_event.rs index 32c5dc6ec..0faa6e74e 100644 --- a/crates/matrix-sdk-base/src/latest_event.rs +++ b/crates/matrix-sdk-base/src/latest_event.rs @@ -62,6 +62,18 @@ impl LatestEventValue { | Self::LocalCannotBeSent(LocalLatestEventValue { timestamp, .. }) => Some(*timestamp), } } + + /// Check whether the [`LatestEventValue`] represents a local value or not, + /// i.e. it is [`LocalIsSending`] or [`LocalCannotBeSent`]. + /// + /// [`LocalIsSending`]: LatestEventValue::LocalIsSending + /// [`LocalCannotBeSent`]: LatestEventValue::LocalCannotBeSent + pub fn is_local(&self) -> bool { + match self { + Self::LocalIsSending(_) | Self::LocalCannotBeSent(_) => true, + Self::None | Self::Remote(_) => false, + } + } } /// Represents the value for [`LatestEventValue::Remote`]. diff --git a/crates/matrix-sdk-base/src/room/latest_event.rs b/crates/matrix-sdk-base/src/room/latest_event.rs index d572dfd37..0ea5e1a3d 100644 --- a/crates/matrix-sdk-base/src/room/latest_event.rs +++ b/crates/matrix-sdk-base/src/room/latest_event.rs @@ -15,6 +15,7 @@ #[cfg(feature = "e2e-encryption")] use std::{collections::BTreeMap, num::NonZeroUsize}; +use ruma::MilliSecondsSinceUnixEpoch; #[cfg(feature = "e2e-encryption")] use ruma::{OwnedRoomId, events::AnySyncTimelineEvent, serde::Raw}; @@ -35,10 +36,23 @@ impl Room { } /// Return the [`LatestEventValue`] of this room. + /// + /// Note that it clones the [`LatestEventValue`]! This can be add pressure + /// on the memory if used in a hot path. pub fn new_latest_event(&self) -> LatestEventValue { self.inner.read().new_latest_event.clone() } + /// Return the value of [`LatestEventValue::timestamp`]. + pub fn new_latest_event_timestamp(&self) -> Option { + self.inner.read().new_latest_event.timestamp() + } + + /// Return the value of [`LatestEventValue::is_local`]. + pub fn new_latest_event_is_local(&self) -> bool { + self.inner.read().new_latest_event.is_local() + } + /// Return the most recent few encrypted events. When the keys come through /// to decrypt these, the most recent relevant one will replace /// latest_event. (We can't tell which one is relevant until diff --git a/crates/matrix-sdk-ui/src/room_list_service/sorters/latest_event.rs b/crates/matrix-sdk-ui/src/room_list_service/sorters/latest_event.rs index 47088d741..8aafae7f7 100644 --- a/crates/matrix-sdk-ui/src/room_list_service/sorters/latest_event.rs +++ b/crates/matrix-sdk-ui/src/room_list_service/sorters/latest_event.rs @@ -14,45 +14,35 @@ use std::cmp::Ordering; -use matrix_sdk::latest_events::LatestEventValue; - use super::{Room, Sorter}; -fn cmp(latest_events: F, left: &Room, right: &Room) -> Ordering +fn cmp(are_latest_events_locals: F, left: &Room, right: &Room) -> Ordering where - F: Fn(&Room, &Room) -> (LatestEventValue, LatestEventValue), + F: Fn(&Room, &Room) -> (bool, bool), { // We want local latest event to come first. When there is a remote latest event // or no latest event, we don't want to sort them. - match latest_events(left, right) { - // `None` == `None`. - // `None` == `Remote`. - // `Remote` == `None`. - // `Remote` == `Remote`. - ( - LatestEventValue::None | LatestEventValue::Remote(_), - LatestEventValue::None | LatestEventValue::Remote(_), - ) => Ordering::Equal, + match are_latest_events_locals(left, right) { + // `false` and `false`, i.e.: + // - `None` == `None`. + // - `None` == `Remote`. + // - `Remote` == `None`. + // - `Remote` == `Remote`. + (false, false) => Ordering::Equal, - // `None` > `Local*`. - // `Remote` > `Local*`. - ( - LatestEventValue::None | LatestEventValue::Remote(_), - LatestEventValue::LocalIsSending(_) | LatestEventValue::LocalCannotBeSent(_), - ) => Ordering::Greater, + // `false` and `true`, i.e.: + // - `None` > `Local*`. + // - `Remote` > `Local*`. + (false, true) => Ordering::Greater, - // `Local*` < `None`. - // `Local*` < `Remote`. - ( - LatestEventValue::LocalIsSending(_) | LatestEventValue::LocalCannotBeSent(_), - LatestEventValue::None | LatestEventValue::Remote(_), - ) => Ordering::Less, + // `true` and `false`, i.e.: + // - `Local*` < `None`. + // - `Local*` < `Remote`. + (true, false) => Ordering::Less, - // `Local*` == `Local*` - ( - LatestEventValue::LocalIsSending(_) | LatestEventValue::LocalCannotBeSent(_), - LatestEventValue::LocalIsSending(_) | LatestEventValue::LocalCannotBeSent(_), - ) => Ordering::Equal, + // `true` and `false, i.e.: + // - `Local*` == `Local*` + (true, true) => Ordering::Equal, } } @@ -61,9 +51,18 @@ where /// ([`LatestEventValue::LocalIsSending`] or /// [`LatestEventValue::LocalCannotBeSent`]) come first, and latest event /// representing a remote event ([`LatestEventValue::Remote`]) come last. +/// +/// [`LatestEventValue::LocalIsSending`]: matrix_sdk_base::latest_event::LatestEventValue::LocalIsSending +/// [`LatestEventValue::LocalCannotBeSent`]: matrix_sdk_base::latest_event::LatestEventValue::LocalCannotBeSent +/// [`LatestEventValue::Remote`]: matrix_sdk_base::latest_event::LatestEventValue::Remote pub fn new_sorter() -> impl Sorter { - let latest_events = - |left: &Room, right: &Room| (left.new_latest_event(), right.new_latest_event()); + let latest_events = |left: &Room, right: &Room| { + // Be careful. This method is called **a lot** in the context of a sorter. Using + // `Room::new_latest_event` would be dramatic as it returns a clone of the + // `LatestEventValue`. It's better to use the more specific method + // `Room::new_latest_event_is_local`. + (left.new_latest_event_is_local(), right.new_latest_event_is_local()) + }; move |left, right| -> Ordering { cmp(latest_events, left, right) } } @@ -71,7 +70,7 @@ pub fn new_sorter() -> impl Sorter { #[cfg(test)] mod tests { use matrix_sdk::{ - latest_events::{LocalLatestEventValue, RemoteLatestEventValue}, + latest_events::{LatestEventValue, LocalLatestEventValue, RemoteLatestEventValue}, store::SerializableEventContent, test_utils::logged_in_client_with_server, }; @@ -143,22 +142,34 @@ mod tests { // `None` and `None`. { - assert_eq!(cmp(|_, _| (none(), none()), &room_a, &room_b), Ordering::Equal); + assert_eq!( + cmp(|_, _| (none().is_local(), none().is_local()), &room_a, &room_b), + Ordering::Equal + ); } // `None` and `Remote`. { - assert_eq!(cmp(|_, _| (none(), remote()), &room_a, &room_b), Ordering::Equal); + assert_eq!( + cmp(|_, _| (none().is_local(), remote().is_local()), &room_a, &room_b), + Ordering::Equal + ); } // `Remote` and `None`. { - assert_eq!(cmp(|_, _| (remote(), none()), &room_a, &room_b), Ordering::Equal); + assert_eq!( + cmp(|_, _| (remote().is_local(), none().is_local()), &room_a, &room_b), + Ordering::Equal + ); } // `Remote` and `None`. { - assert_eq!(cmp(|_, _| (remote(), remote()), &room_a, &room_b), Ordering::Equal); + assert_eq!( + cmp(|_, _| (remote().is_local(), remote().is_local()), &room_a, &room_b), + Ordering::Equal + ); } } @@ -172,11 +183,15 @@ mod tests { // `None` and `Local*`. { assert_eq!( - cmp(|_, _| (none(), local_is_sending()), &room_a, &room_b), + cmp(|_, _| (none().is_local(), local_is_sending().is_local()), &room_a, &room_b), Ordering::Greater ); assert_eq!( - cmp(|_, _| (none(), local_cannot_be_sent()), &room_a, &room_b), + cmp( + |_, _| (none().is_local(), local_cannot_be_sent().is_local()), + &room_a, + &room_b + ), Ordering::Greater ); } @@ -184,11 +199,15 @@ mod tests { // `Remote` and `Local*`. { assert_eq!( - cmp(|_, _| (remote(), local_is_sending()), &room_a, &room_b), + cmp(|_, _| (remote().is_local(), local_is_sending().is_local()), &room_a, &room_b), Ordering::Greater ); assert_eq!( - cmp(|_, _| (remote(), local_cannot_be_sent()), &room_a, &room_b), + cmp( + |_, _| (remote().is_local(), local_cannot_be_sent().is_local()), + &room_a, + &room_b + ), Ordering::Greater ); } @@ -203,9 +222,16 @@ mod tests { // `Local*` and `None`. { - assert_eq!(cmp(|_, _| (local_is_sending(), none()), &room_a, &room_b), Ordering::Less); assert_eq!( - cmp(|_, _| (local_cannot_be_sent(), none()), &room_a, &room_b), + cmp(|_, _| (local_is_sending().is_local(), none().is_local()), &room_a, &room_b), + Ordering::Less + ); + assert_eq!( + cmp( + |_, _| (local_cannot_be_sent().is_local(), none().is_local()), + &room_a, + &room_b + ), Ordering::Less ); } @@ -213,11 +239,15 @@ mod tests { // `Local*` and `Remote`. { assert_eq!( - cmp(|_, _| (local_is_sending(), remote()), &room_a, &room_b), + cmp(|_, _| (local_is_sending().is_local(), remote().is_local()), &room_a, &room_b), Ordering::Less ); assert_eq!( - cmp(|_, _| (local_cannot_be_sent(), remote()), &room_a, &room_b), + cmp( + |_, _| (local_cannot_be_sent().is_local(), remote().is_local()), + &room_a, + &room_b + ), Ordering::Less ); } @@ -233,19 +263,35 @@ mod tests { // `Local*` and `Local*`. { assert_eq!( - cmp(|_, _| (local_is_sending(), local_is_sending()), &room_a, &room_b), + cmp( + |_, _| (local_is_sending().is_local(), local_is_sending().is_local()), + &room_a, + &room_b + ), Ordering::Equal ); assert_eq!( - cmp(|_, _| (local_is_sending(), local_cannot_be_sent()), &room_a, &room_b), + cmp( + |_, _| (local_is_sending().is_local(), local_cannot_be_sent().is_local()), + &room_a, + &room_b + ), Ordering::Equal ); assert_eq!( - cmp(|_, _| (local_cannot_be_sent(), local_is_sending()), &room_a, &room_b), + cmp( + |_, _| (local_cannot_be_sent().is_local(), local_is_sending().is_local()), + &room_a, + &room_b + ), Ordering::Equal ); assert_eq!( - cmp(|_, _| (local_cannot_be_sent(), local_cannot_be_sent()), &room_a, &room_b), + cmp( + |_, _| (local_cannot_be_sent().is_local(), local_cannot_be_sent().is_local()), + &room_a, + &room_b + ), Ordering::Equal ); } diff --git a/crates/matrix-sdk-ui/src/room_list_service/sorters/recency.rs b/crates/matrix-sdk-ui/src/room_list_service/sorters/recency.rs index 10b3f66b7..b99bc03e4 100644 --- a/crates/matrix-sdk-ui/src/room_list_service/sorters/recency.rs +++ b/crates/matrix-sdk-ui/src/room_list_service/sorters/recency.rs @@ -14,10 +14,6 @@ use std::cmp::Ordering; -// TODO @hywan: remove once the `TODO` in `extract_rank` is solved. -#[allow(unused)] -use matrix_sdk::latest_events::LatestEventValue; - use super::{Room, Sorter}; fn cmp(ranks: F, left: &Room, right: &Room) -> Ordering @@ -64,6 +60,7 @@ where /// /// [`RoomInfo::recency_stamp`]: matrix_sdk_base::RoomInfo::recency_stamp /// [`RoomInfo::new_latest_event`]: matrix_sdk_base::RoomInfo::new_latest_event +/// [`LatestEventValue::None`]: matrix_sdk_base::latest_event::LatestEventValue::None pub fn new_sorter() -> impl Sorter { let ranks = |left: &Room, right: &Room| extract_rank(left, right); @@ -84,43 +81,27 @@ type Rank = u64; /// `RoomInfo::recency_stamp` is not a timestamp, while `LatestEventValue` uses /// a timestamp. fn extract_rank(left: &Room, right: &Room) -> (Option, Option) { - // TODO @hywan: calling `LatestEventValue::timestamp` here is apparently - // causing issues. While investigating, let's disable this part of the code, - // and let's fallback to the recency stamp (as it was the case before). - (left.recency_stamp().map(Into::into), right.recency_stamp().map(Into::into)) - - /* - match (left.new_latest_event(), right.new_latest_event()) { - // One of both rooms has NO latest event value. Let's fallback to the recency stamp from the - // `RoomInfo` for both room. - (LatestEventValue::None, _) | (_, LatestEventValue::None) => { + // Be careful. This method is called **a lot** in the context of a sorter. Using + // `Room::new_latest_event` would be dramatic as it returns a clone of the + // `LatestEventValue`. It's better to use the more specific method + // `Room::new_latest_event_timestamp`. + match (left.new_latest_event_timestamp(), right.new_latest_event_timestamp()) { + // One of the two rooms has no latest event timestamp. Let's fallback to + // the recency stamp from the `RoomInfo` for both room. + (None, _) | (_, None) => { (left.recency_stamp().map(Into::into), right.recency_stamp().map(Into::into)) } - // Both rooms have a non-`None` latest event. We can use their timestamps as a rank. - ( - left @ LatestEventValue::Remote(_) - | left @ LatestEventValue::LocalIsSending(_) - | left @ LatestEventValue::LocalCannotBeSent(_), - right @ LatestEventValue::Remote(_) - | right @ LatestEventValue::LocalIsSending(_) - | right @ LatestEventValue::LocalCannotBeSent(_), - ) => ( - left.timestamp().map(|ms| ms.get().into()), - right.timestamp().map(|ms| ms.get().into()), - ), + // Both rooms have a timestamp. We can use them as a rank. + (Some(left), Some(right)) => (Some(left.get().into()), Some(right.get().into())), } - */ } #[cfg(test)] mod tests { - // TODO @hywan: remove once the `TODO` in `extract_rank` is solved. - #![allow(unused)] - use matrix_sdk::{ RoomRecencyStamp, - latest_events::{LocalLatestEventValue, RemoteLatestEventValue}, + latest_events::{LatestEventValue, LocalLatestEventValue, RemoteLatestEventValue}, store::SerializableEventContent, test_utils::logged_in_client_with_server, }; @@ -229,8 +210,6 @@ mod tests { } } - // TODO @hywan: uncomment once the `TODO` in `extract_rank` is solved. - /* #[async_test] async fn test_extract_recency_stamp_with_remote_or_local() { let (client, server) = logged_in_client_with_server().await; @@ -254,7 +233,6 @@ mod tests { } } } - */ #[async_test] async fn test_with_two_recency_stamps() {