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: """