Sliding Sync: Speed up incremental sync by avoiding extra work (#17665)

Speed up incremental sync by avoiding extra work. We first look at the
state delta changes and only fetch and calculate further derived things
if they have changed.

changelog.d/17665.misc

@@ -0,0 +1 @@
+Speed up incremental Sliding Sync requests by avoiding extra work.

synapse/handlers/sliding_sync/__init__.py

@@ -44,6 +44,7 @@
 )
 from synapse.types import (
     JsonDict,
+    MutableStateMap,
     PersistedEventPosition,
     Requester,
     RoomStreamToken,
@@ -753,26 +754,78 @@ async def get_room_sync_data(
         # indicate to the client that a state reset happened. Perhaps we should indicate
         # this by setting `initial: True` and empty `required_state`.
 
-        # Check whether the room has a name set
-        name_state_ids = await self.get_current_state_ids_at(
-            room_id=room_id,
-            room_membership_for_user_at_to_token=room_membership_for_user_at_to_token,
-            state_filter=StateFilter.from_types([(EventTypes.Name, "")]),
-            to_token=to_token,
-        )
-        name_event_id = name_state_ids.get((EventTypes.Name, ""))
+        # Get the changes to current state in the token range from the
+        # `current_state_delta_stream` table.
+        #
+        # For incremental syncs, we can do this first to determine if something relevant
+        # has changed and strategically avoid fetching other costly things.
+        room_state_delta_id_map: MutableStateMap[str] = {}
+        name_event_id: Optional[str] = None
+        membership_changed = False
+        name_changed = False
+        avatar_changed = False
+        if initial:
+            # Check whether the room has a name set
+            name_state_ids = await self.get_current_state_ids_at(
+                room_id=room_id,
+                room_membership_for_user_at_to_token=room_membership_for_user_at_to_token,
+                state_filter=StateFilter.from_types([(EventTypes.Name, "")]),
+                to_token=to_token,
+            )
+            name_event_id = name_state_ids.get((EventTypes.Name, ""))
+        else:
+            assert from_bound is not None
 
-        room_membership_summary: Mapping[str, MemberSummary]
+            # TODO: Limit the number of state events we're about to send down
+            # the room, if its too many we should change this to an
+            # `initial=True`?
+            deltas = await self.store.get_current_state_deltas_for_room(
+                room_id=room_id,
+                from_token=from_bound,
+                to_token=to_token.room_key,
+            )
+            for delta in deltas:
+                # TODO: Handle state resets where event_id is None
+                if delta.event_id is not None:
+                    room_state_delta_id_map[(delta.event_type, delta.state_key)] = (
+                        delta.event_id
+                    )
+
+                if delta.event_type == EventTypes.Member:
+                    membership_changed = True
+                elif delta.event_type == EventTypes.Name and delta.state_key == "":
+                    name_changed = True
+                elif (
+                    delta.event_type == EventTypes.RoomAvatar and delta.state_key == ""
+                ):
+                    avatar_changed = True
+
+        room_membership_summary: Optional[Mapping[str, MemberSummary]] = None
         empty_membership_summary = MemberSummary([], 0)
-        if room_membership_for_user_at_to_token.membership in (
-            Membership.LEAVE,
-            Membership.BAN,
-        ):
-            # TODO: Figure out how to get the membership summary for left/banned rooms
-            room_membership_summary = {}
-        else:
-            room_membership_summary = await self.store.get_room_summary(room_id)
-            # TODO: Reverse/rewind back to the `to_token`
+        # We need the room summary for:
+        #  - Always for initial syncs (or the first time we send down the room)
+        #  - When the room has no name, we need `heroes`
+        #  - When the membership has changed so we need to give updated `heroes` and
+        #    `joined_count`/`invited_count`.
+        #
+        # Ideally, instead of just looking at `name_changed`, we'd check if the room
+        # name is not set but this is a good enough approximation that saves us from
+        # having to pull out the full event. This just means, we're generating the
+        # summary whenever the room name changes instead of only when it changes to
+        # `None`.
+        if initial or name_changed or membership_changed:
+            # We can't trace the function directly because it's cached and the `@cached`
+            # decorator doesn't mix with `@trace` yet.
+            with start_active_span("get_room_summary"):
+                if room_membership_for_user_at_to_token.membership in (
+                    Membership.LEAVE,
+                    Membership.BAN,
+                ):
+                    # TODO: Figure out how to get the membership summary for left/banned rooms
+                    room_membership_summary = {}
+                else:
+                    room_membership_summary = await self.store.get_room_summary(room_id)
+                    # TODO: Reverse/rewind back to the `to_token`
 
         # `heroes` are required if the room name is not set.
         #
@@ -786,7 +839,12 @@ async def get_room_sync_data(
         # TODO: Should we also check for `EventTypes.CanonicalAlias`
         # (`m.room.canonical_alias`) as a fallback for the room name? see
         # https://github.com/matrix-org/matrix-spec-proposals/pull/3575#discussion_r1671260153
-        if name_event_id is None:
+        #
+        # We need to fetch the `heroes` if the room name is not set. But we only need to
+        # get them on initial syncs (or the first time we send down the room) or if the
+        # membership has changed which may change the heroes.
+        if name_event_id is None and (initial or (not initial and membership_changed)):
+            assert room_membership_summary is not None
             hero_user_ids = extract_heroes_from_room_summary(
                 room_membership_summary, me=user.to_string()
             )
@@ -904,9 +962,15 @@ async def get_room_sync_data(
 
         # We need this base set of info for the response so let's just fetch it along
         # with the `required_state` for the room
-        meta_room_state = [(EventTypes.Name, ""), (EventTypes.RoomAvatar, "")] + [
+        hero_room_state = [
             (EventTypes.Member, hero_user_id) for hero_user_id in hero_user_ids
         ]
+        meta_room_state = list(hero_room_state)
+        if initial or name_changed:
+            meta_room_state.append((EventTypes.Name, ""))
+        if initial or avatar_changed:
+            meta_room_state.append((EventTypes.RoomAvatar, ""))
+
         state_filter = StateFilter.all()
         if required_state_filter != StateFilter.all():
             state_filter = StateFilter(
@@ -929,21 +993,22 @@ async def get_room_sync_data(
         else:
             assert from_bound is not None
 
-            # TODO: Limit the number of state events we're about to send down
-            # the room, if its too many we should change this to an
-            # `initial=True`?
-            deltas = await self.store.get_current_state_deltas_for_room(
-                room_id=room_id,
-                from_token=from_bound,
-                to_token=to_token.room_key,
-            )
-            # TODO: Filter room state before fetching events
-            # TODO: Handle state resets where event_id is None
             events = await self.store.get_events(
-                [d.event_id for d in deltas if d.event_id]
+                state_filter.filter_state(room_state_delta_id_map).values()
             )
             room_state = {(s.type, s.state_key): s for s in events.values()}
 
+            # If the membership changed and we have to get heroes, get the remaining
+            # heroes from the state
+            if hero_user_ids:
+                hero_membership_state = await self.get_current_state_at(
+                    room_id=room_id,
+                    room_membership_for_user_at_to_token=room_membership_for_user_at_to_token,
+                    state_filter=StateFilter.from_types(hero_room_state),
+                    to_token=to_token,
+                )
+                room_state.update(hero_membership_state)
+
         required_room_state: StateMap[EventBase] = {}
         if required_state_filter != StateFilter.none():
             required_room_state = required_state_filter.filter_state(room_state)
@@ -1050,6 +1115,20 @@ async def get_room_sync_data(
 
         set_tag(SynapseTags.RESULT_PREFIX + "initial", initial)
 
+        joined_count: Optional[int] = None
+        if initial or membership_changed:
+            assert room_membership_summary is not None
+            joined_count = room_membership_summary.get(
+                Membership.JOIN, empty_membership_summary
+            ).count
+
+        invited_count: Optional[int] = None
+        if initial or membership_changed:
+            assert room_membership_summary is not None
+            invited_count = room_membership_summary.get(
+                Membership.INVITE, empty_membership_summary
+            ).count
+
         return SlidingSyncResult.RoomResult(
             name=room_name,
             avatar=room_avatar,
@@ -1065,12 +1144,8 @@ async def get_room_sync_data(
             unstable_expanded_timeline=unstable_expanded_timeline,
             num_live=num_live,
             bump_stamp=bump_stamp,
-            joined_count=room_membership_summary.get(
-                Membership.JOIN, empty_membership_summary
-            ).count,
-            invited_count=room_membership_summary.get(
-                Membership.INVITE, empty_membership_summary
-            ).count,
+            joined_count=joined_count,
+            invited_count=invited_count,
             # TODO: These are just dummy values. We could potentially just remove these
             # since notifications can only really be done correctly on the client anyway
             # (encrypted rooms).

synapse/rest/client/sync.py

@@ -1011,12 +1011,16 @@ async def encode_rooms(
         for room_id, room_result in rooms.items():
             serialized_rooms[room_id] = {
                 "bump_stamp": room_result.bump_stamp,
-                "joined_count": room_result.joined_count,
-                "invited_count": room_result.invited_count,
                 "notification_count": room_result.notification_count,
                 "highlight_count": room_result.highlight_count,
             }
 
+            if room_result.joined_count is not None:
+                serialized_rooms[room_id]["joined_count"] = room_result.joined_count
+
+            if room_result.invited_count is not None:
+                serialized_rooms[room_id]["invited_count"] = room_result.invited_count
+
             if room_result.name:
                 serialized_rooms[room_id]["name"] = room_result.name
 

synapse/types/handlers/sliding_sync.py

@@ -197,8 +197,8 @@ class StrippedHero:
         # Only optional because it won't be included for invite/knock rooms with `stripped_state`
         num_live: Optional[int]
         bump_stamp: int
-        joined_count: int
-        invited_count: int
+        joined_count: Optional[int]
+        invited_count: Optional[int]
         notification_count: int
         highlight_count: int
 
@@ -207,6 +207,12 @@ def __bool__(self) -> bool:
                 # If this is the first time the client is seeing the room, we should not filter it out
                 # under any circumstance.
                 self.initial
+                # We need to let the client know if any of the info has changed
+                or self.name is not None
+                or self.avatar is not None
+                or bool(self.heroes)
+                or self.joined_count is not None
+                or self.invited_count is not None
                 # We need to let the client know if there are any new events
                 or bool(self.required_state)
                 or bool(self.timeline_events)