sdk-base: split handle_account_data and process_direct_rooms
#4010
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4010 +/- ##
==========================================
- Coverage 84.27% 84.26% -0.02%
==========================================
Files 266 266
Lines 28336 28341 +5
==========================================
+ Hits 23880 23881 +1
- Misses 4456 4460 +4 ☔ View full report in Codecov by Sentry. |
crates/matrix-sdk-base/src/client.rs
Outdated
| #[instrument(skip_all)] | ||
| pub(crate) async fn process_direct_rooms( | ||
| &self, | ||
| events: Vec<AnyGlobalAccountDataEvent>, |
There was a problem hiding this comment.
| events: Vec<AnyGlobalAccountDataEvent>, | |
| events: &[AnyGlobalAccountDataEvent], |
A Vec demands an allocation, but in this code, we may not need one. So requiring a slice is better here. Read my other comments to see how it translates.
crates/matrix-sdk-base/src/client.rs
Outdated
| if has_new_direct_room_data { | ||
| self.handle_account_data(&response.account_data.events, &mut changes).await; | ||
| self.process_direct_rooms(global_account_data_events, &mut changes).await; |
There was a problem hiding this comment.
Don't pass the Vec but a reference to it, so basically a slice (because a Vec derefs to a slice)
| self.process_direct_rooms(global_account_data_events, &mut changes).await; | |
| self.process_direct_rooms(&global_account_data_events, &mut changes).await; |
crates/matrix-sdk-base/src/client.rs
Outdated
| } else if let Ok(Some(direct_account_data)) = | ||
| self.store.get_account_data_event(GlobalAccountDataEventType::Direct).await | ||
| { | ||
| debug!("Found direct room data in the Store, applying it"); | ||
| self.handle_account_data(&[direct_account_data], &mut changes).await; | ||
| if let Ok(direct_account_data) = direct_account_data.deserialize() { | ||
| self.process_direct_rooms(vec![direct_account_data], &mut changes).await; |
There was a problem hiding this comment.
And finally here, don't allocate a vector (which does an allocation on the heap, this is why it's costly), but simply pass a slice of one element.
| self.process_direct_rooms(vec![direct_account_data], &mut changes).await; | |
| self.process_direct_rooms(&[direct_account_data], &mut changes).await; |
| let global_account_data_events = if !account_data.is_empty() { | ||
| self.handle_account_data(&account_data.global, &mut changes).await | ||
| } else { | ||
| Vec::new() |
There was a problem hiding this comment.
Note: Here this is fine to get a Vec::new(), it does zero allocation.
I know it's different from what we've talked in the above comments, but I'm sharing some Rust knowledge.
| if has_new_direct_room_data { | ||
| self.handle_account_data(&account_data.global, &mut changes).await; | ||
| self.process_direct_rooms(global_account_data_events, &mut changes).await; |
There was a problem hiding this comment.
| self.process_direct_rooms(global_account_data_events, &mut changes).await; | |
| self.process_direct_rooms(&global_account_data_events, &mut changes).await; |
| } else if let Ok(Some(direct_account_data)) = | ||
| self.store.get_account_data_event(GlobalAccountDataEventType::Direct).await | ||
| { | ||
| debug!("Found direct room data in the Store, applying it"); | ||
| self.handle_account_data(&[direct_account_data], &mut changes).await; | ||
| if let Ok(direct_account_data) = direct_account_data.deserialize() { | ||
| self.process_direct_rooms(vec![direct_account_data], &mut changes).await; |
There was a problem hiding this comment.
| self.process_direct_rooms(vec![direct_account_data], &mut changes).await; | |
| self.process_direct_rooms(&[direct_account_data], &mut changes).await; |
This removes a couple of TODOs in the codebase.
jmartinesp
force-pushed
the
misc/split-handle-account-data-and-process-direct-rooms
branch
from
6ab5172 to
c2015f5
Compare
|
Thanks for the comments, I think all those are fixed, so I'll merge it now. |
jmartinesp
deleted the
misc/split-handle-account-data-and-process-direct-rooms
branch
Follow up for #3990.
This removes a couple of TODOs in the codebase where we were running
handle_account_datatwice to be able to update the direct rooms after the rooms had been created in the store. Now these responsibilities are split in two separate functions.Signed-off-by: