-
Notifications
You must be signed in to change notification settings - Fork 359
refactor: Add profile cache for handle_remote_event #5827
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.
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
refactor: Add profile cache for handle_remote_event #5827
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5827 +/- ##
=======================================
Coverage 88.54% 88.54%
=======================================
Files 361 361
Lines 101421 101434 +13
Branches 101421 101434 +13
=======================================
+ Hits 89803 89818 +15
+ Misses 7410 7409 -1
+ Partials 4208 4207 -1 ☔ View full report in Codecov by Sentry. |
CodSpeed Performance ReportMerging #5827 will not alter performanceComparing Summary
|
Hywan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution!
How do you manage the user profile invalidation? If a user has updated its profile, how is it updated from the cache (the HashMap)?
|
That's the neat part, you don't! Jokes aside, when I discussed it with @poljar (and I think I confirmed it by looking at the code), by the time we reach this part of the flow we'd have already applied any changes caused by the state events, including the room member ones, so it should be fine to not invalidate the cache, since it'll be used just for the current vector diffs. I may be wrong though. |
Hywan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cache is local to handle_remote_events_with_diffs. I think it's correct. Let's go :-].
This should slightly improve the performance for loading timeline items since we might skip loading the sender profiles for some/most items.
In my local tests on Android, this can save up to 50ms for each timeline initialization, although the benchmark only shows a 1.7% improvement 🥲 . That said, it also says the the function that populates the cache is not called when it definitely is,
so it doesn't seem too reliable.
Signed-off-by: