Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Sliding Sync: No need to sort if the range is large enough to cover all of the rooms #17731

Merged
merged 3 commits into from
Sep 19, 2024
Merged

Sliding Sync: No need to sort if the range is large enough to cover all of the rooms #17731

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
77e6e24
No need to sort if the range is big enough
MadLittleMods Sep 18, 2024
b8812b4
Add changelog
MadLittleMods Sep 18, 2024
6794987
Fix tests
MadLittleMods Sep 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/17731.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Small performance improvement in speeding up Sliding Sync.
14 changes: 12 additions & 2 deletions synapse/handlers/sliding_sync/room_lists.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,8 +357,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()
)
Expand Down
64 changes: 25 additions & 39 deletions tests/rest/client/sliding_sync/test_lists_filters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down
3 changes: 2 additions & 1 deletion tests/rest/client/sliding_sync/test_rooms_meta.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
49 changes: 40 additions & 9 deletions tests/rest/client/sliding_sync/test_sliding_sync.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -686,11 +686,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,
}
Expand All @@ -699,25 +699,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
Expand Down
Loading