perf: Parallelize creating stores to make session restoration faster#6544
perf: Parallelize creating stores to make session restoration faster#6544jmartinesp merged 2 commits intomainfrom
Conversation
jmartinesp
force-pushed
the
perf/try-creating-stores-concurrently
branch
from
40e438b to
6d2db3e
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6544 +/- ##
==========================================
- Coverage 89.93% 89.92% -0.02%
==========================================
Files 381 381
Lines 106780 106773 -7
Branches 106780 106773 -7
==========================================
- Hits 96036 96014 -22
- Misses 7096 7107 +11
- Partials 3648 3652 +4 ☔ View full report in Codecov by Sentry. |
jmartinesp
force-pushed
the
perf/try-creating-stores-concurrently
branch
2 times, most recently
from
039ae72 to
dd5136d
Compare
jmartinesp
force-pushed
the
perf/try-creating-stores-concurrently
branch
from
dd5136d to
edeffe8
Compare
Hywan
left a comment
Hywan
left a comment
There was a problem hiding this comment.
It looks great, thank you! Would be nice to have a benchmark for that, though it's not particularly “critical”. It might become critical when the client is built in a constrained process, like NSE on iOS for example.
| use tokio::sync::Mutex; | ||
| use tokio::sync::OnceCell; | ||
| #[cfg(feature = "sqlite")] | ||
| use tokio::try_join; |
There was a problem hiding this comment.
I guess the best way to handle this is by declaring try_join in matrix_sdk_common::executor:
- for the case of
#[cfg(not(target_family = "wasm"))], re-exporttokio::try_join. - for the case of
#[cfg(target_family = "wasm")], re-exportfutures_util::try_join.
Even better, if you don't want to bother with that, you can use futures_util::try_join in all cases, without touching matrix_sdk_common. That way you remove the dependencies to tokio for this case.
| if let Some(ref cache_path) = cache_path { | ||
| config = config.path(cache_path); | ||
| } | ||
| let caches_config = if let Some(ref cache_path) = cache_path { |
There was a problem hiding this comment.
You can rename caches_config to cache_with_path. I think it's more self-explanatory, thoughts?
There was a problem hiding this comment.
config_with_cache_path?
| use serde::de::DeserializeOwned; | ||
| use tokio::sync::{Mutex, OnceCell, RwLock, RwLockReadGuard, broadcast}; | ||
| use tokio::{ | ||
| join, |
There was a problem hiding this comment.
Same idea: maybe use futures_util here?
I think there is ("Restore session (Client reload)") it's just not displaying any changes. |
|
Maybe because the stores aren't big enough to take a noticeable time in the benchmark. Okay. Let's keep things as they are for now. |
jmartinesp
force-pushed
the
perf/try-creating-stores-concurrently
branch
from
edfc283 to
3eaa4a3
Compare
2 participants
Changes
Clientinstances.get_or_initforClient::event_cacheandClient::thread_subscription_catchup. This may not do much, since the intialization is actually sync unless there was already some value (maybe not worth it?).Codspeed doesn't seem to return different results, but I could definitely see changes when tested in a real client:
CHANGELOG.mdfiles.Signed-off-by: