Sliding sync: omit bump stamp when it is unchanged (#17788)

This saves some DB lookups in rooms

changelog.d/17788.misc

@@ -0,0 +1 @@
+Sliding sync minor performance improvement by omitting unchanged data from incremental responses.

synapse/handlers/sliding_sync/__init__.py

@@ -1057,22 +1057,42 @@ class SlidingSyncHandler:
                     )
                 )
 
-        # Figure out the last bump event in the room
+        # Figure out the last bump event in the room. If the bump stamp hasn't
-        #
+        # changed we omit it from the response.
-        # By default, just choose the membership event position for any non-join membership
+        bump_stamp = None
-        bump_stamp = room_membership_for_user_at_to_token.event_pos.stream
+
+        always_return_bump_stamp = (
+            # We use the membership event position for any non-join
+            room_membership_for_user_at_to_token.membership != Membership.JOIN
+            # We didn't fetch any timeline events but we should still check for
+            # a bump_stamp that might be somewhere
+            or limited is None
+            # There might be a bump event somewhere before the timeline events
+            # that we fetched, that we didn't previously send down
+            or limited is True
+            # Always give the client some frame of reference if this is the
+            # first time they are seeing the room down the connection
+            or initial
+        )
+
         # If we're joined to the room, we need to find the last bump event before the
         # `to_token`
         if room_membership_for_user_at_to_token.membership == Membership.JOIN:
-            # Try and get a bump stamp, if not we just fall back to the
+            # Try and get a bump stamp
-            # membership token.
             new_bump_stamp = await self._get_bump_stamp(
-                room_id, to_token, timeline_events
+                room_id,
+                to_token,
+                timeline_events,
+                check_outside_timeline=always_return_bump_stamp,
             )
             if new_bump_stamp is not None:
                 bump_stamp = new_bump_stamp
 
-        if bump_stamp < 0:
+        if bump_stamp is None and always_return_bump_stamp:
+            # By default, just choose the membership event position for any non-join membership
+            bump_stamp = room_membership_for_user_at_to_token.event_pos.stream
+
+        if bump_stamp is not None and bump_stamp < 0:
             # We never want to send down negative stream orderings, as you can't
             # sensibly compare positive and negative stream orderings (they have
             # different meanings).
@@ -1165,14 +1185,23 @@ class SlidingSyncHandler:
 
     @trace
     async def _get_bump_stamp(
-        self, room_id: str, to_token: StreamToken, timeline: List[EventBase]
+        self,
+        room_id: str,
+        to_token: StreamToken,
+        timeline: List[EventBase],
+        check_outside_timeline: bool,
     ) -> Optional[int]:
-        """Get a bump stamp for the room, if we have a bump event
+        """Get a bump stamp for the room, if we have a bump event and it has
+        changed.
 
         Args:
             room_id
             to_token: The upper bound of token to return
             timeline: The list of events we have fetched.
+            limited: If the timeline was limited.
+            check_outside_timeline: Whether we need to check for bump stamp for
+                events before the timeline if we didn't find a bump stamp in
+                the timeline events.
         """
 
         # First check the timeline events we're returning to see if one of
@@ -1192,6 +1221,11 @@ class SlidingSyncHandler:
                 if new_bump_stamp > 0:
                     return new_bump_stamp
 
+        if not check_outside_timeline:
+            # If we are not a limited sync, then we know the bump stamp can't
+            # have changed.
+            return None
+
         # We can quickly query for the latest bump event in the room using the
         # sliding sync tables.
         latest_room_bump_stamp = await self.store.get_latest_bump_stamp_for_room(

synapse/rest/client/sync.py

@@ -1010,11 +1010,13 @@ class SlidingSyncRestServlet(RestServlet):
         serialized_rooms: Dict[str, JsonDict] = {}
         for room_id, room_result in rooms.items():
             serialized_rooms[room_id] = {
-                "bump_stamp": room_result.bump_stamp,
                 "notification_count": room_result.notification_count,
                 "highlight_count": room_result.highlight_count,
             }
 
+            if room_result.bump_stamp is not None:
+                serialized_rooms[room_id]["bump_stamp"] = room_result.bump_stamp
+
             if room_result.joined_count is not None:
                 serialized_rooms[room_id]["joined_count"] = room_result.joined_count
 

synapse/types/handlers/sliding_sync.py

@@ -158,6 +158,7 @@ class SlidingSyncResult:
                 name changes to mark the room as unread and bump it to the top. For
                 encrypted rooms, we just have to consider any activity as a bump because we
                 can't see the content and the client has to figure it out for themselves.
+                This may not be included if there hasn't been a change.
             joined_count: The number of users with membership of join, including the client's
                 own user ID. (same as sync `v2 m.joined_member_count`)
             invited_count: The number of users with membership of invite. (same as sync v2
@@ -193,7 +194,7 @@ class SlidingSyncResult:
         limited: Optional[bool]
         # Only optional because it won't be included for invite/knock rooms with `stripped_state`
         num_live: Optional[int]
-        bump_stamp: int
+        bump_stamp: Optional[int]
         joined_count: Optional[int]
         invited_count: Optional[int]
         notification_count: int

tests/rest/client/sliding_sync/test_rooms_meta.py

@@ -1096,6 +1096,92 @@ class SlidingSyncRoomsMetaTestCase(SlidingSyncBase):
 
         self.assertGreater(response_body["rooms"][room_id]["bump_stamp"], 0)
 
+    def test_rooms_bump_stamp_no_change_incremental(self) -> None:
+        """Test that the bump stamp is omitted if there has been no change"""
+
+        user1_id = self.register_user("user1", "pass")
+        user1_tok = self.login(user1_id, "pass")
+
+        room_id1 = self.helper.create_room_as(
+            user1_id,
+            tok=user1_tok,
+        )
+
+        # Make the Sliding Sync request
+        sync_body = {
+            "lists": {
+                "foo-list": {
+                    "ranges": [[0, 1]],
+                    "required_state": [],
+                    "timeline_limit": 100,
+                }
+            }
+        }
+        response_body, from_token = self.do_sync(sync_body, tok=user1_tok)
+
+        # Initial sync so we expect to see a bump stamp
+        self.assertIn("bump_stamp", response_body["rooms"][room_id1])
+
+        # Send an event that is not in the bump events list
+        self.helper.send_event(
+            room_id1, type="org.matrix.test", content={}, tok=user1_tok
+        )
+
+        response_body, from_token = self.do_sync(
+            sync_body, since=from_token, tok=user1_tok
+        )
+
+        # There hasn't been a change to the bump stamps, so we ignore it
+        self.assertNotIn("bump_stamp", response_body["rooms"][room_id1])
+
+    def test_rooms_bump_stamp_change_incremental(self) -> None:
+        """Test that the bump stamp is included if there has been a change, even
+        if its not in the timeline"""
+
+        user1_id = self.register_user("user1", "pass")
+        user1_tok = self.login(user1_id, "pass")
+
+        room_id1 = self.helper.create_room_as(
+            user1_id,
+            tok=user1_tok,
+        )
+
+        # Make the Sliding Sync request
+        sync_body = {
+            "lists": {
+                "foo-list": {
+                    "ranges": [[0, 1]],
+                    "required_state": [],
+                    "timeline_limit": 2,
+                }
+            }
+        }
+        response_body, from_token = self.do_sync(sync_body, tok=user1_tok)
+
+        # Initial sync so we expect to see a bump stamp
+        self.assertIn("bump_stamp", response_body["rooms"][room_id1])
+        first_bump_stamp = response_body["rooms"][room_id1]["bump_stamp"]
+
+        # Send a bump event at the start.
+        self.helper.send(room_id1, "test", tok=user1_tok)
+
+        # Send events that are not in the bump events list to fill the timeline
+        for _ in range(5):
+            self.helper.send_event(
+                room_id1, type="org.matrix.test", content={}, tok=user1_tok
+            )
+
+        response_body, from_token = self.do_sync(
+            sync_body, since=from_token, tok=user1_tok
+        )
+
+        # There was a bump event in the timeline gap, so we should see the bump
+        # stamp be updated.
+        self.assertIn("bump_stamp", response_body["rooms"][room_id1])
+        second_bump_stamp = response_body["rooms"][room_id1]["bump_stamp"]
+
+        self.assertGreater(second_bump_stamp, first_bump_stamp)
+
     def test_rooms_bump_stamp_invites(self) -> None:
         """
         Test that `bump_stamp` is present and points to the membership event,