From 422f3ecec1ffb4fa352f83d7ec8b3327f36a93c8 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 8 Oct 2024 11:17:23 +0100 Subject: [PATCH] Sliding sync: omit bump stamp when it is unchanged (#17788) This saves some DB lookups in rooms --- changelog.d/17788.misc | 1 + synapse/handlers/sliding_sync/__init__.py | 54 +++++++++--- synapse/rest/client/sync.py | 4 +- synapse/types/handlers/sliding_sync.py | 3 +- .../client/sliding_sync/test_rooms_meta.py | 86 +++++++++++++++++++ 5 files changed, 136 insertions(+), 12 deletions(-) create mode 100644 changelog.d/17788.misc diff --git a/changelog.d/17788.misc b/changelog.d/17788.misc new file mode 100644 index 0000000000..1ef6f6e2ba --- /dev/null +++ b/changelog.d/17788.misc @@ -0,0 +1 @@ +Sliding sync minor performance improvement by omitting unchanged data from incremental responses. diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index cb6a0b9f35..8c12cea8eb 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -1057,22 +1057,42 @@ class SlidingSyncHandler: ) ) - # Figure out the last bump event in the room - # - # 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 + # Figure out the last bump event in the room. If the bump stamp hasn't + # changed we omit it from the response. + bump_stamp = None + + 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 - # membership token. + # Try and get a bump stamp 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( diff --git a/synapse/rest/client/sync.py b/synapse/rest/client/sync.py index 364cf40153..122708e933 100644 --- a/synapse/rest/client/sync.py +++ b/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 diff --git a/synapse/types/handlers/sliding_sync.py b/synapse/types/handlers/sliding_sync.py index 5dd2c9d411..aae60fddea 100644 --- a/synapse/types/handlers/sliding_sync.py +++ b/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 diff --git a/tests/rest/client/sliding_sync/test_rooms_meta.py b/tests/rest/client/sliding_sync/test_rooms_meta.py index c619dd83fb..0a8b2c02c2 100644 --- a/tests/rest/client/sliding_sync/test_rooms_meta.py +++ b/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,