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

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

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)]:
+                        # Optimization: If we are asking for the full range, we don't
-                            # If we are asking for the full range, we don't need to sort the list.
+                        # 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 @@ 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(
+        self.assertIncludes(
-            list(response_body["lists"].keys()),
-            ["all-list", "foo-list"],
             response_body["lists"].keys(),
+            {"all-list", "foo-list"},
         )
 
         # Make sure the lists have the correct rooms
-        self.assertListEqual(
+        self.assertIncludes(
-            list(response_body["lists"]["all-list"]["ops"]),
+            set(response_body["lists"]["all-list"]["ops"][0]["room_ids"]),
-            [
+            {space_room_id, room_id},
-                {
+            exact=True,
-                    "op": "SYNC",
-                    "range": [0, 99],
-                    "room_ids": [space_room_id, room_id],
-                }
-            ],
         )
-        self.assertListEqual(
+        self.assertIncludes(
-            list(response_body["lists"]["foo-list"]["ops"]),
+            set(response_body["lists"]["foo-list"]["ops"][0]["room_ids"]),
-            [
+            {space_room_id},
-                {
+            exact=True,
-                    "op": "SYNC",
-                    "range": [0, 99],
-                    "room_ids": [space_room_id],
-                }
-            ],
         )
 
         # 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`
+        # Make sure the response has the lists we requested
-        self.assertListEqual(
+        self.assertIncludes(
-            list(response_body["lists"]["all-list"]["ops"]),
+            response_body["lists"].keys(),
-            [
+            {"all-list", "foo-list"},
-                {
+            exact=True,
-                    "op": "SYNC",
-                    "range": [0, 99],
-                    "room_ids": [space_room_id, room_id],
-                }
-            ],
         )
-        self.assertListEqual(
+
-            list(response_body["lists"]["foo-list"]["ops"]),
+        # 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"]),
-                    "op": "SYNC",
+            {space_room_id, room_id},
-                    "range": [0, 99],
+            exact=True,
-                    "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 @@ 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

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(
+        self.assertIncludes(
-            list(response_body["lists"].keys()),
-            ["foo-list"],
             response_body["lists"].keys(),
+            {"foo-list"},
         )
-
+        # Since the range includes all of the rooms, we don't sort the list
-        # Make sure the list is sorted in the way we expect
+        self.assertEqual(
-        self.assertListEqual(
+            len(response_body["lists"]["foo-list"]["ops"]),
-            list(response_body["lists"]["foo-list"]["ops"]),
+            1,
-            [
-                {
-                    "op": "SYNC",
-                    "range": [0, 99],
-                    "room_ids": [room_id2, room_id1, room_id3],
-                }
-            ],
             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:
         """