From a9c0e27eb760c0d042e84e6f7beda167611ef148 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 19 Sep 2024 03:33:34 -0500 Subject: [PATCH] Sliding Sync: No need to sort if the range is large enough to cover all of the rooms (#17731) No need to sort if the range is large enough to cover all of the rooms in the list. Previously, we would only do this optimization if the range was exactly large enough. Follow-up to https://github.com/element-hq/synapse/pull/17672 --- changelog.d/17731.misc | 1 + synapse/handlers/sliding_sync/room_lists.py | 14 +++- .../client/sliding_sync/test_lists_filters.py | 66 ++++++++----------- .../client/sliding_sync/test_rooms_meta.py | 3 +- .../client/sliding_sync/test_sliding_sync.py | 61 ++++++++++++----- 5 files changed, 87 insertions(+), 58 deletions(-) create mode 100644 changelog.d/17731.misc diff --git a/changelog.d/17731.misc b/changelog.d/17731.misc new file mode 100644 index 0000000000..d5df74b4c9 --- /dev/null +++ b/changelog.d/17731.misc @@ -0,0 +1 @@ +Small performance improvement in speeding up Sliding Sync. diff --git a/synapse/handlers/sliding_sync/room_lists.py b/synapse/handlers/sliding_sync/room_lists.py index 8457526a45..a423de74bf 100644 --- a/synapse/handlers/sliding_sync/room_lists.py +++ b/synapse/handlers/sliding_sync/room_lists.py @@ -373,8 +373,18 @@ class SlidingSyncRoomLists: ops: List[SlidingSyncResult.SlidingWindowList.Operation] = [] if list_config.ranges: - if list_config.ranges == [(0, len(filtered_sync_room_map) - 1)]: - # If we are asking for the full range, we don't need to sort the list. + # Optimization: If we are asking for the full range, we don't + # need to sort the list. + if ( + # We're looking for a single range that covers the entire list + len(list_config.ranges) == 1 + # Range starts at 0 + and list_config.ranges[0][0] == 0 + # And the range extends to the end of the list or more. Each + # side is inclusive. + and list_config.ranges[0][1] + >= len(filtered_sync_room_map) - 1 + ): sorted_room_info: List[RoomsForUserType] = list( filtered_sync_room_map.values() ) diff --git a/tests/rest/client/sliding_sync/test_lists_filters.py b/tests/rest/client/sliding_sync/test_lists_filters.py index 16e4e8edbc..c59f6aedc4 100644 --- a/tests/rest/client/sliding_sync/test_lists_filters.py +++ b/tests/rest/client/sliding_sync/test_lists_filters.py @@ -230,32 +230,21 @@ class SlidingSyncFiltersTestCase(SlidingSyncBase): response_body, from_token = self.do_sync(sync_body, tok=user1_tok) # Make sure the response has the lists we requested - self.assertListEqual( - list(response_body["lists"].keys()), - ["all-list", "foo-list"], + self.assertIncludes( response_body["lists"].keys(), + {"all-list", "foo-list"}, ) # Make sure the lists have the correct rooms - self.assertListEqual( - list(response_body["lists"]["all-list"]["ops"]), - [ - { - "op": "SYNC", - "range": [0, 99], - "room_ids": [space_room_id, room_id], - } - ], + self.assertIncludes( + set(response_body["lists"]["all-list"]["ops"][0]["room_ids"]), + {space_room_id, room_id}, + exact=True, ) - self.assertListEqual( - list(response_body["lists"]["foo-list"]["ops"]), - [ - { - "op": "SYNC", - "range": [0, 99], - "room_ids": [space_room_id], - } - ], + self.assertIncludes( + set(response_body["lists"]["foo-list"]["ops"][0]["room_ids"]), + {space_room_id}, + exact=True, ) # Everyone leaves the encrypted space room @@ -284,26 +273,23 @@ class SlidingSyncFiltersTestCase(SlidingSyncBase): } response_body, _ = self.do_sync(sync_body, since=from_token, tok=user1_tok) - # Make sure the lists have the correct rooms even though we `newly_left` - self.assertListEqual( - list(response_body["lists"]["all-list"]["ops"]), - [ - { - "op": "SYNC", - "range": [0, 99], - "room_ids": [space_room_id, room_id], - } - ], + # Make sure the response has the lists we requested + self.assertIncludes( + response_body["lists"].keys(), + {"all-list", "foo-list"}, + exact=True, ) - self.assertListEqual( - list(response_body["lists"]["foo-list"]["ops"]), - [ - { - "op": "SYNC", - "range": [0, 99], - "room_ids": [space_room_id], - } - ], + + # Make sure the lists have the correct rooms even though we `newly_left` + self.assertIncludes( + set(response_body["lists"]["all-list"]["ops"][0]["room_ids"]), + {space_room_id, room_id}, + exact=True, + ) + self.assertIncludes( + set(response_body["lists"]["foo-list"]["ops"][0]["room_ids"]), + {space_room_id}, + exact=True, ) def test_filters_is_dm(self) -> None: diff --git a/tests/rest/client/sliding_sync/test_rooms_meta.py b/tests/rest/client/sliding_sync/test_rooms_meta.py index 40743d17eb..8d6039715f 100644 --- a/tests/rest/client/sliding_sync/test_rooms_meta.py +++ b/tests/rest/client/sliding_sync/test_rooms_meta.py @@ -935,7 +935,8 @@ class SlidingSyncRoomsMetaTestCase(SlidingSyncBase): op = response_body["lists"]["foo-list"]["ops"][0] self.assertEqual(op["op"], "SYNC") self.assertEqual(op["range"], [0, 1]) - # Note that we don't order the ops anymore, so we need to compare sets. + # Note that we don't sort the rooms when the range includes all of the rooms, so + # we just assert that the rooms are included self.assertIncludes(set(op["room_ids"]), {room_id1, room_id2}, exact=True) # The `bump_stamp` for room1 should point at the latest message (not the diff --git a/tests/rest/client/sliding_sync/test_sliding_sync.py b/tests/rest/client/sliding_sync/test_sliding_sync.py index 1126258c43..c2cfb29866 100644 --- a/tests/rest/client/sliding_sync/test_sliding_sync.py +++ b/tests/rest/client/sliding_sync/test_sliding_sync.py @@ -797,7 +797,38 @@ class SlidingSyncTestCase(SlidingSyncBase): self.helper.send(room_id1, "activity in room1", tok=user1_tok) self.helper.send(room_id2, "activity in room2", tok=user1_tok) - # Make the Sliding Sync request + # Make the Sliding Sync request where the range includes *some* of the rooms + sync_body = { + "lists": { + "foo-list": { + "ranges": [[0, 1]], + "required_state": [], + "timeline_limit": 1, + } + } + } + response_body, _ = self.do_sync(sync_body, tok=user1_tok) + + # Make sure it has the foo-list we requested + self.assertIncludes( + response_body["lists"].keys(), + {"foo-list"}, + ) + # Make sure the list is sorted in the way we expect (we only sort when the range + # doesn't include all of the room) + self.assertListEqual( + list(response_body["lists"]["foo-list"]["ops"]), + [ + { + "op": "SYNC", + "range": [0, 1], + "room_ids": [room_id2, room_id1], + } + ], + response_body["lists"]["foo-list"], + ) + + # Make the Sliding Sync request where the range includes *all* of the rooms sync_body = { "lists": { "foo-list": { @@ -810,24 +841,24 @@ class SlidingSyncTestCase(SlidingSyncBase): response_body, _ = self.do_sync(sync_body, tok=user1_tok) # Make sure it has the foo-list we requested - self.assertListEqual( - list(response_body["lists"].keys()), - ["foo-list"], + self.assertIncludes( response_body["lists"].keys(), + {"foo-list"}, ) - - # Make sure the list is sorted in the way we expect - self.assertListEqual( - list(response_body["lists"]["foo-list"]["ops"]), - [ - { - "op": "SYNC", - "range": [0, 99], - "room_ids": [room_id2, room_id1, room_id3], - } - ], + # Since the range includes all of the rooms, we don't sort the list + self.assertEqual( + len(response_body["lists"]["foo-list"]["ops"]), + 1, response_body["lists"]["foo-list"], ) + op = response_body["lists"]["foo-list"]["ops"][0] + self.assertEqual(op["op"], "SYNC") + self.assertEqual(op["range"], [0, 99]) + # Note that we don't sort the rooms when the range includes all of the rooms, so + # we just assert that the rooms are included + self.assertIncludes( + set(op["room_ids"]), {room_id1, room_id2, room_id3}, exact=True + ) def test_sliced_windows(self) -> None: """