feat(sdk,ui): SlidingSync::subscribe_to_rooms has a new cancel_in_flight_request argument

[Hywan]

This patch adds a new cancel_in_flight_request argument to SlidingSync::subscribe_to_rooms, which tells the method cancel the in-flight request if any.

This patch also updates RoomListService::subscribe_to_rooms to turn this new argument to true if the state machine isn't in a “starting” state.

The problem it's solving is the following:

• some apps starts the room list service
• a first request is sent with pos = None
• the server calculates a new session (which can be expensive)
• the app subscribes to a set of rooms
• a second request is immediately sent with pos = None again
• the server does possibly NOT cancel its previous calculations, but starts a new session and its calculations

This is pretty expensive for the server. This patch makes so that the immediate room subscriptions will be part of the second request, with the first request not being cancelled.

[codecov]

Codecov Report

Attention: Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.

Project coverage is 84.30%. Comparing base (31e6df7) to head (cdb3169).
Report is 23 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/matrix-sdk-ui/src/room_list_service/mod.rs	75.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3981   +/-   ##
=======================================
  Coverage   84.29%   84.30%           
=======================================
  Files         267      267           
  Lines       28336    28339    +3     
=======================================
+ Hits        23887    23890    +3     
  Misses       4449     4449           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[MatMaul]

Thanks for that. The main drawback being that updating the currently visible rooms could be quite slower since we then fetch rooms by 100 instead of 20.

Another way out on the app side would be to not start the SyncService until the UI calls subscribe_to_rooms with the list of visible rooms.

[Hywan]

Thanks for your feedback. Here are a couple of random questions:

• Are you sure it's gonna impact the apps?
• Why room subscriptions are slow?
• Why not reducing 100 subscriptions to 20 or 30 then?

This mechanism is already implemented inside Element X iOS. This is basically porting this mechanism from Swift/iOS to the Rust SDK, so that it can benefit to anyone. It's proven to be performant for iOS at least.

[MatMaul]

Honestly, after thinking a bit more I think it's good enough and the impact will be minimal :)

I don't think room subscriptions are slow, it's just that they will come a bit later in the sync.

To my understanding what will be happening is:

• first req is a range [0,19] one. before this change it would get cancelled and replaced by a [0,19] + room subscriptions of the visible list.
• after this patch, the initial range [0,19] is not cancelled, and the room subscriptions will only be used in the next req, which will be a range [0,99].

So the currently visible rooms will update only after the second [0,99] is complete.

I don't think it's a big problem in the end since the [0,19] will include the recent ones, and if the visible ones are not already in it they will get push down the list (and probably not visible), so the update can wait the next sync.

On a more global note, it would be a good idea I think to switch the Growing algorithm with some kind of exponential increment.
Currently it increments by 100 on each req, with an exponential factor of 1.2 and a first req at 20 it would do approx:

• 20
• 36
• 74
• 177
• etc...

I think if we put this in place, we can even remove completely the specific case of the first [0,19], since it would be included.

[poljar]

Seems sensible, though doesn't match to the description, please double check.

[poljar]

turn this new argument to true if the state machine isn't in a “starting” state.

This seems to set it to true when we're in the Running state, are you sure this is correct? State::Init sounds more like it would fit the description.

[Hywan]

It is correct yes. Init and Recovering are the “starting” states, thus we don't want to cancel in-flight requests. SettingUp and Running are states where we have a pos, so we can cancel in-flight requests here.

The state machine is like this: Init -> SettingUp -> Running. In case of an error, it depends on the previous state before the error, but basically if we were in Running, we jump back to Recovering -> Running. So Init and Recovering are basically the “starting” states here.

[Hywan]

@MatMaul We don't want an exponential range API, because it would load more and more data, until reaching really big response sizes. Also, resetting the exponential range to its initial value could indeed replace a selective range, but that's basically the same behaviour. Considering we don't want huge range, but keep ranges of +100 rooms, I think the current design should be kept for the moment.