Skip to content

Conversation

@Hywan
Copy link
Member

@Hywan Hywan commented Dec 11, 2025

This patch improves Latest Events in multiple ways.

First off, the system is lazier. Before, a LatestEventValue was initialized by computing a Remote(_) with the help of the RoomEventCache. Now, it is initialized by copying the value in the RoomInfo. It solves a bug where a latest event was registered but its corresponding RoomEventCache was not created yet.

Second, in addition to that, if the restored LatestEventValue is None, a lazy computation is asked from the Event Cache. It fixes a bug where the RoomInfo has no LatestEventValue but it's possible to compute one from the Event Cache.

Finally, replacing a LatestEventValue::None by itself is ignored, thus reducing the number of updates in the system.


@codspeed-hq
Copy link

codspeed-hq bot commented Dec 11, 2025

CodSpeed Performance Report

Merging #5947 will improve performances by ×5

Comparing Hywan:fix-sdk-latest-event-lazy-load-room-event-cache (351c604) with main (ce65317)

Summary

⚡ 1 improvement
✅ 49 untouched

Benchmarks breakdown

Benchmark BASE HEAD Change
Create[1000 rooms × 1000 events] 30.7 ms 6.1 ms ×5

@Hywan Hywan force-pushed the fix-sdk-latest-event-lazy-load-room-event-cache branch from f5bff3c to 4e23e8b Compare December 12, 2025 07:41
@codecov
Copy link

codecov bot commented Dec 12, 2025

Codecov Report

❌ Patch coverage is 95.14066% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.50%. Comparing base (f2ba338) to head (351c604).
⚠️ Report is 10 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...matrix-sdk/src/latest_events/room_latest_events.rs 77.08% 8 Missing and 3 partials ⚠️
crates/matrix-sdk/src/latest_events/mod.rs 96.38% 2 Missing and 4 partials ⚠️
...rates/matrix-sdk/src/latest_events/latest_event.rs 98.85% 0 Missing and 2 partials ⚠️
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.
📢 Have feedback on the report? Share it here.

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.
@Hywan Hywan force-pushed the fix-sdk-latest-event-lazy-load-room-event-cache branch 2 times, most recently from 00aaafc to e3730f8 Compare December 12, 2025 14:23
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.
@Hywan Hywan force-pushed the fix-sdk-latest-event-lazy-load-room-event-cache branch 2 times, most recently from eca6916 to b32d012 Compare December 12, 2025 14:32
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.
@Hywan Hywan force-pushed the fix-sdk-latest-event-lazy-load-room-event-cache branch from 6041304 to 2a35613 Compare December 12, 2025 14:38
@Hywan Hywan force-pushed the fix-sdk-latest-event-lazy-load-room-event-cache branch from 2a35613 to 437e5ce Compare December 12, 2025 14:39
@Hywan Hywan marked this pull request as ready for review December 12, 2025 14:43
@Hywan Hywan requested a review from a team as a code owner December 12, 2025 14:43
@Hywan Hywan requested review from andybalaam and jmartinesp and removed request for a team and andybalaam December 12, 2025 14:43
@jmartinesp
Copy link
Contributor

I can approve this from the client side perspective, but someone else should probably take a deeper look at the code.

@Hywan Hywan requested a review from poljar December 12, 2025 15:03
@Hywan Hywan force-pushed the fix-sdk-latest-event-lazy-load-room-event-cache branch from 8d0aec3 to cf903f3 Compare December 12, 2025 15:11
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`.
@Hywan Hywan force-pushed the fix-sdk-latest-event-lazy-load-room-event-cache branch from cf903f3 to 351c604 Compare December 12, 2025 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants