-
Notifications
You must be signed in to change notification settings - Fork 358
fix(sdk): Latest Event lazy-loads the Room Event Cache #5947
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
base: main
Are you sure you want to change the base?
fix(sdk): Latest Event lazy-loads the Room Event Cache #5947
Conversation
CodSpeed Performance ReportMerging #5947 will improve performances by ×5Comparing Summary
Benchmarks breakdown
|
f5bff3c to
4e23e8b
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5947 +/- ##
==========================================
- Coverage 88.50% 88.50% -0.01%
==========================================
Files 362 362
Lines 103395 103405 +10
Branches 103395 103405 +10
==========================================
+ Hits 91512 91520 +8
+ Misses 7533 7529 -4
- Partials 4350 4356 +6 ☔ View full report in Codecov by Sentry. |
This patch removes `RoomRegistration` along with the full room registration mechanism. It's been introduced to remove contention around the `RegisteredRooms` lock, but it actually creates more async flows, which makes the `latest_events` logic a bit less predictable. By removing this room registration mechanism, our hope is to make the result more predictable and less buggy in appareance. Our real-life tests have shown that the lock contention isn't problematic, especially since `RoomLatestEventsReadGuard` and `RoomLatestEventsWriteGuard` have been introduced.
00aaafc to
e3730f8
Compare
Previously, `LatestEventValue` was always initialized from the `RoomEventCache`. Now, it is restored from the `RoomInfo`. First off, this is something we wanted to do since a long time. Second, it is more performant. Third, it allows the system to be lazier. Indeed, it's possible that when a `LatestEvent` is created, its correspond `RoomEventCache` doesn't exist yet. It happens during a sync when a room is new: latest events are registered, but their `RoomEventCache` aren't yet created. By postponing the use of `RoomEventCache`, the system is lazier and more solid. Bonus, less methods are async, which simplifes the workflow.
This `test_room_sorting` test was asserting a bug. With the last patch, this bug is now fixed, and the test must be fixed too.
eca6916 to
b32d012
Compare
The `LatestEventValue` can be `None` (the default value) but the Event Cache contains enough data to compute a `Remote(_)` one. The system lazily triggers a `LatestEventQueueUpdate` to achieve that.
Replacing a `LatestEventValue::None` by a `LatestEventValue::None` is ignored. It reduces the number of (useless) updates in the system.
6041304 to
2a35613
Compare
2a35613 to
437e5ce
Compare
|
I can approve this from the client side perspective, but someone else should probably take a deeper look at the code. |
8d0aec3 to
cf903f3
Compare
This patch simplifies the code after the recent refactorings. It uses `LatestEventValue::is_none()` to replace a `matches!`, and it replaces the last use of `new_remote` by `new_remote_with_power_levels` to finally rename this latter to `new_remote`.
cf903f3 to
351c604
Compare
This patch improves Latest Events in multiple ways.
First off, the system is lazier. Before, a
LatestEventValuewas initialized by computing aRemote(_)with the help of theRoomEventCache. Now, it is initialized by copying the value in theRoomInfo. It solves a bug where a latest event was registered but its correspondingRoomEventCachewas not created yet.Second, in addition to that, if the restored
LatestEventValueisNone, a lazy computation is asked from the Event Cache. It fixes a bug where theRoomInfohas noLatestEventValuebut it's possible to compute one from the Event Cache.Finally, replacing a
LatestEventValue::Noneby itself is ignored, thus reducing the number of updates in the system.