Sliding Sync: No need to sort if the range is large enough to cover a…

…ll 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 #17672

changelog.d/17731.misc

@@ -0,0 +1 @@
+Small performance improvement in speeding up Sliding Sync.

synapse/handlers/sliding_sync/room_lists.py

@@ -373,8 +373,18 @@ async def _compute_interested_rooms_new_tables(
                     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()
                             )

tests/rest/client/sliding_sync/test_lists_filters.py

@@ -230,32 +230,21 @@ def test_filters_regardless_of_membership_server_left_room(self) -> None:
         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 @@ def test_filters_regardless_of_membership_server_left_room(self) -> None:
         }
         response_body, _ = self.do_sync(sync_body, since=from_token, tok=user1_tok)
 
+        # Make sure the response has the lists we requested
+        self.assertIncludes(
+            response_body["lists"].keys(),
+            {"all-list", "foo-list"},
+            exact=True,
+        )
+
         # 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],
-                }
-            ],
+        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,
         )
 
     def test_filters_is_dm(self) -> None:

tests/rest/client/sliding_sync/test_rooms_meta.py

@@ -935,7 +935,8 @@ def test_rooms_bump_stamp(self) -> None:
         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

tests/rest/client/sliding_sync/test_sliding_sync.py

@@ -797,11 +797,11 @@ def test_sort_list(self) -> None:
         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, 99]],
+                    "ranges": [[0, 1]],
                     "required_state": [],
                     "timeline_limit": 1,
                 }
@@ -810,25 +810,56 @@ def test_sort_list(self) -> None:
         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
+        # 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, 99],
-                    "room_ids": [room_id2, room_id1, room_id3],
+                    "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": {
+                    "ranges": [[0, 99]],
+                    "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"},
+        )
+        # 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:
         """
         Test that the `lists` `ranges` are sliced correctly. Both sides of each range