From 7ef8f8884ef39fba3e9d907e5005c06297fea965 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 19 Jul 2024 12:20:10 +0100 Subject: [PATCH 1/5] Add a cache on get_rooms_for_local_user_where_membership_is --- synapse/api/constants.py | 2 +- synapse/handlers/initial_sync.py | 2 +- synapse/handlers/search.py | 2 +- .../server_notices/server_notices_manager.py | 4 +-- synapse/storage/_base.py | 6 +++++ synapse/storage/databases/main/cache.py | 6 +++++ synapse/storage/databases/main/roommember.py | 26 +++++++++++++++---- tests/handlers/test_sync.py | 1 + tests/rest/client/test_account.py | 2 +- tests/storage/test_roommember.py | 2 +- 10 files changed, 41 insertions(+), 12 deletions(-) diff --git a/synapse/api/constants.py b/synapse/api/constants.py index 12d18137e07..286efe275f8 100644 --- a/synapse/api/constants.py +++ b/synapse/api/constants.py @@ -50,7 +50,7 @@ class Membership: KNOCK: Final = "knock" LEAVE: Final = "leave" BAN: Final = "ban" - LIST: Final = {INVITE, JOIN, KNOCK, LEAVE, BAN} + LIST: Final = (INVITE, JOIN, KNOCK, LEAVE, BAN) class PresenceState: diff --git a/synapse/handlers/initial_sync.py b/synapse/handlers/initial_sync.py index bd3c87f5f4e..8849503f52b 100644 --- a/synapse/handlers/initial_sync.py +++ b/synapse/handlers/initial_sync.py @@ -135,7 +135,7 @@ async def _snapshot_all_rooms( memberships.append(Membership.LEAVE) room_list = await self.store.get_rooms_for_local_user_where_membership_is( - user_id=user_id, membership_list=memberships + user_id=user_id, membership_list=tuple(memberships) ) user = UserID.from_string(user_id) diff --git a/synapse/handlers/search.py b/synapse/handlers/search.py index a7d52fa6483..80661325270 100644 --- a/synapse/handlers/search.py +++ b/synapse/handlers/search.py @@ -278,7 +278,7 @@ async def _search( # TODO: Search through left rooms too rooms = await self.store.get_rooms_for_local_user_where_membership_is( requester.user.to_string(), - membership_list=[Membership.JOIN], + membership_list=(Membership.JOIN,), # membership_list=[Membership.JOIN, Membership.LEAVE, Membership.Ban], ) room_ids = {r.room_id for r in rooms} diff --git a/synapse/server_notices/server_notices_manager.py b/synapse/server_notices/server_notices_manager.py index 001a290e87e..e9cdc628d54 100644 --- a/synapse/server_notices/server_notices_manager.py +++ b/synapse/server_notices/server_notices_manager.py @@ -114,7 +114,7 @@ async def maybe_get_notice_room_for_user(self, user_id: str) -> Optional[str]: return None rooms = await self._store.get_rooms_for_local_user_where_membership_is( - user_id, [Membership.INVITE, Membership.JOIN] + user_id, (Membership.INVITE, Membership.JOIN) ) for room in rooms: # it's worth noting that there is an asymmetry here in that we @@ -262,7 +262,7 @@ async def maybe_invite_user_to_room(self, user_id: str, room_id: str) -> None: # Check whether the user has already joined or been invited to this room. If # that's the case, there is no need to re-invite them. joined_rooms = await self._store.get_rooms_for_local_user_where_membership_is( - user_id, [Membership.INVITE, Membership.JOIN] + user_id, (Membership.INVITE, Membership.JOIN) ) for room in joined_rooms: if room.room_id == room_id: diff --git a/synapse/storage/_base.py b/synapse/storage/_base.py index 881888fa93f..066f3d08ae4 100644 --- a/synapse/storage/_base.py +++ b/synapse/storage/_base.py @@ -120,6 +120,9 @@ def _invalidate_state_caches( "get_user_in_room_with_profile", (room_id, user_id) ) self._attempt_to_invalidate_cache("get_rooms_for_user", (user_id,)) + self._attempt_to_invalidate_cache( + "_get_rooms_for_local_user_where_membership_is_inner", (user_id,) + ) # Purge other caches based on room state. self._attempt_to_invalidate_cache("get_room_summary", (room_id,)) @@ -146,6 +149,9 @@ def _invalidate_state_caches_all(self, room_id: str) -> None: self._attempt_to_invalidate_cache("does_pair_of_users_share_a_room", None) self._attempt_to_invalidate_cache("get_user_in_room_with_profile", None) self._attempt_to_invalidate_cache("get_rooms_for_user", None) + self._attempt_to_invalidate_cache( + "_get_rooms_for_local_user_where_membership_is_inner", None + ) self._attempt_to_invalidate_cache("get_room_summary", (room_id,)) def _attempt_to_invalidate_cache( diff --git a/synapse/storage/databases/main/cache.py b/synapse/storage/databases/main/cache.py index 2d6b75e47ed..26b8e1a1727 100644 --- a/synapse/storage/databases/main/cache.py +++ b/synapse/storage/databases/main/cache.py @@ -331,6 +331,9 @@ def _invalidate_caches_for_event( "get_invited_rooms_for_local_user", (state_key,) ) self._attempt_to_invalidate_cache("get_rooms_for_user", (state_key,)) + self._attempt_to_invalidate_cache( + "_get_rooms_for_local_user_where_membership_is_inner", (state_key,) + ) self._attempt_to_invalidate_cache( "did_forget", @@ -393,6 +396,9 @@ def _invalidate_caches_for_room_events(self, room_id: str) -> None: self._attempt_to_invalidate_cache("get_thread_id_for_receipts", None) self._attempt_to_invalidate_cache("get_invited_rooms_for_local_user", None) self._attempt_to_invalidate_cache("get_rooms_for_user", None) + self._attempt_to_invalidate_cache( + "_get_rooms_for_local_user_where_membership_is_inner", None + ) self._attempt_to_invalidate_cache("did_forget", None) self._attempt_to_invalidate_cache("get_forgotten_rooms_for_user", None) self._attempt_to_invalidate_cache("get_references_for_event", None) diff --git a/synapse/storage/databases/main/roommember.py b/synapse/storage/databases/main/roommember.py index f62d9f705dc..a03aea91c1d 100644 --- a/synapse/storage/databases/main/roommember.py +++ b/synapse/storage/databases/main/roommember.py @@ -385,7 +385,7 @@ async def get_invited_rooms_for_local_user( """ return await self.get_rooms_for_local_user_where_membership_is( - user_id, [Membership.INVITE] + user_id, (Membership.INVITE,) ) async def get_knocked_at_rooms_for_local_user( @@ -401,7 +401,7 @@ async def get_knocked_at_rooms_for_local_user( """ return await self.get_rooms_for_local_user_where_membership_is( - user_id, [Membership.KNOCK] + user_id, (Membership.KNOCK,) ) async def get_invite_for_local_user_in_room( @@ -445,9 +445,7 @@ async def get_rooms_for_local_user_where_membership_is( if not membership_list: return [] - rooms = await self.db_pool.runInteraction( - "get_rooms_for_local_user_where_membership_is", - self._get_rooms_for_local_user_where_membership_is_txn, + rooms = await self._get_rooms_for_local_user_where_membership_is_inner( user_id, membership_list, ) @@ -466,6 +464,24 @@ async def get_rooms_for_local_user_where_membership_is( return [room for room in rooms if room.room_id not in rooms_to_exclude] + @cached(max_entries=1000, tree=True) + async def _get_rooms_for_local_user_where_membership_is_inner( + self, + user_id: str, + membership_list: Collection[str], + ) -> Sequence[RoomsForUser]: + if not membership_list: + return [] + + rooms = await self.db_pool.runInteraction( + "get_rooms_for_local_user_where_membership_is", + self._get_rooms_for_local_user_where_membership_is_txn, + user_id, + membership_list, + ) + + return rooms + def _get_rooms_for_local_user_where_membership_is_txn( self, txn: LoggingTransaction, diff --git a/tests/handlers/test_sync.py b/tests/handlers/test_sync.py index 77aafa492e0..fa55f769167 100644 --- a/tests/handlers/test_sync.py +++ b/tests/handlers/test_sync.py @@ -211,6 +211,7 @@ def test_unknown_room_version(self) -> None: # Blow away caches (supported room versions can only change due to a restart). self.store.get_rooms_for_user.invalidate_all() + self.store._get_rooms_for_local_user_where_membership_is_inner.invalidate_all() self.store._get_event_cache.clear() self.store._event_ref.clear() diff --git a/tests/rest/client/test_account.py b/tests/rest/client/test_account.py index a85ea994dec..77caab24898 100644 --- a/tests/rest/client/test_account.py +++ b/tests/rest/client/test_account.py @@ -535,7 +535,7 @@ def test_pending_invites(self) -> None: # Check that the membership of @invitee:test in the room is now "leave". memberships = self.get_success( store.get_rooms_for_local_user_where_membership_is( - invitee_id, [Membership.LEAVE] + invitee_id, (Membership.LEAVE,) ) ) self.assertEqual(len(memberships), 1, memberships) diff --git a/tests/storage/test_roommember.py b/tests/storage/test_roommember.py index 418b5561088..e2f19e25e30 100644 --- a/tests/storage/test_roommember.py +++ b/tests/storage/test_roommember.py @@ -68,7 +68,7 @@ def test_one_member(self) -> None: rooms_for_user = self.get_success( self.store.get_rooms_for_local_user_where_membership_is( - self.u_alice, [Membership.JOIN] + self.u_alice, (Membership.JOIN,) ) ) From cc98c953b872c84f423a6dc8346ceb358b403e5d Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 19 Jul 2024 12:21:01 +0100 Subject: [PATCH 2/5] Newsfile --- changelog.d/17460.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/17460.misc diff --git a/changelog.d/17460.misc b/changelog.d/17460.misc new file mode 100644 index 00000000000..fd99da5a95b --- /dev/null +++ b/changelog.d/17460.misc @@ -0,0 +1 @@ +Add cache to `get_rooms_for_local_user_where_membership_is` to speed up sliding sync. From 1cf4305c8a5bd360b58368f40a6a086957c7fc2e Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 19 Jul 2024 14:36:03 +0100 Subject: [PATCH 3/5] Use frozensets --- synapse/storage/databases/main/roommember.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/synapse/storage/databases/main/roommember.py b/synapse/storage/databases/main/roommember.py index a03aea91c1d..2e0e6afac5a 100644 --- a/synapse/storage/databases/main/roommember.py +++ b/synapse/storage/databases/main/roommember.py @@ -445,6 +445,10 @@ async def get_rooms_for_local_user_where_membership_is( if not membership_list: return [] + # Convert membership list to frozen set as a) it needs to be hashable, + # and b) we don't care about the order. + membership_list = frozenset(membership_list) + rooms = await self._get_rooms_for_local_user_where_membership_is_inner( user_id, membership_list, From e809ace372ae6ad02e342d4ad636d529d41d62c9 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 19 Jul 2024 14:37:34 +0100 Subject: [PATCH 4/5] Revert some of the changes --- synapse/handlers/initial_sync.py | 2 +- synapse/handlers/search.py | 2 +- synapse/server_notices/server_notices_manager.py | 4 ++-- synapse/storage/databases/main/roommember.py | 4 ++-- tests/rest/client/test_account.py | 2 +- tests/storage/test_roommember.py | 2 +- 6 files changed, 8 insertions(+), 8 deletions(-) diff --git a/synapse/handlers/initial_sync.py b/synapse/handlers/initial_sync.py index 8849503f52b..bd3c87f5f4e 100644 --- a/synapse/handlers/initial_sync.py +++ b/synapse/handlers/initial_sync.py @@ -135,7 +135,7 @@ async def _snapshot_all_rooms( memberships.append(Membership.LEAVE) room_list = await self.store.get_rooms_for_local_user_where_membership_is( - user_id=user_id, membership_list=tuple(memberships) + user_id=user_id, membership_list=memberships ) user = UserID.from_string(user_id) diff --git a/synapse/handlers/search.py b/synapse/handlers/search.py index 80661325270..a7d52fa6483 100644 --- a/synapse/handlers/search.py +++ b/synapse/handlers/search.py @@ -278,7 +278,7 @@ async def _search( # TODO: Search through left rooms too rooms = await self.store.get_rooms_for_local_user_where_membership_is( requester.user.to_string(), - membership_list=(Membership.JOIN,), + membership_list=[Membership.JOIN], # membership_list=[Membership.JOIN, Membership.LEAVE, Membership.Ban], ) room_ids = {r.room_id for r in rooms} diff --git a/synapse/server_notices/server_notices_manager.py b/synapse/server_notices/server_notices_manager.py index e9cdc628d54..001a290e87e 100644 --- a/synapse/server_notices/server_notices_manager.py +++ b/synapse/server_notices/server_notices_manager.py @@ -114,7 +114,7 @@ async def maybe_get_notice_room_for_user(self, user_id: str) -> Optional[str]: return None rooms = await self._store.get_rooms_for_local_user_where_membership_is( - user_id, (Membership.INVITE, Membership.JOIN) + user_id, [Membership.INVITE, Membership.JOIN] ) for room in rooms: # it's worth noting that there is an asymmetry here in that we @@ -262,7 +262,7 @@ async def maybe_invite_user_to_room(self, user_id: str, room_id: str) -> None: # Check whether the user has already joined or been invited to this room. If # that's the case, there is no need to re-invite them. joined_rooms = await self._store.get_rooms_for_local_user_where_membership_is( - user_id, (Membership.INVITE, Membership.JOIN) + user_id, [Membership.INVITE, Membership.JOIN] ) for room in joined_rooms: if room.room_id == room_id: diff --git a/synapse/storage/databases/main/roommember.py b/synapse/storage/databases/main/roommember.py index 2e0e6afac5a..640ab123f00 100644 --- a/synapse/storage/databases/main/roommember.py +++ b/synapse/storage/databases/main/roommember.py @@ -385,7 +385,7 @@ async def get_invited_rooms_for_local_user( """ return await self.get_rooms_for_local_user_where_membership_is( - user_id, (Membership.INVITE,) + user_id, [Membership.INVITE] ) async def get_knocked_at_rooms_for_local_user( @@ -401,7 +401,7 @@ async def get_knocked_at_rooms_for_local_user( """ return await self.get_rooms_for_local_user_where_membership_is( - user_id, (Membership.KNOCK,) + user_id, [Membership.KNOCK] ) async def get_invite_for_local_user_in_room( diff --git a/tests/rest/client/test_account.py b/tests/rest/client/test_account.py index 77caab24898..a85ea994dec 100644 --- a/tests/rest/client/test_account.py +++ b/tests/rest/client/test_account.py @@ -535,7 +535,7 @@ def test_pending_invites(self) -> None: # Check that the membership of @invitee:test in the room is now "leave". memberships = self.get_success( store.get_rooms_for_local_user_where_membership_is( - invitee_id, (Membership.LEAVE,) + invitee_id, [Membership.LEAVE] ) ) self.assertEqual(len(memberships), 1, memberships) diff --git a/tests/storage/test_roommember.py b/tests/storage/test_roommember.py index e2f19e25e30..418b5561088 100644 --- a/tests/storage/test_roommember.py +++ b/tests/storage/test_roommember.py @@ -68,7 +68,7 @@ def test_one_member(self) -> None: rooms_for_user = self.get_success( self.store.get_rooms_for_local_user_where_membership_is( - self.u_alice, (Membership.JOIN,) + self.u_alice, [Membership.JOIN] ) ) From d7f35bf9448f44ad7811dacb53cd90893f922479 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 19 Jul 2024 14:47:04 +0100 Subject: [PATCH 5/5] Use frozenset for LIST --- synapse/api/constants.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/api/constants.py b/synapse/api/constants.py index 286efe275f8..85001d96765 100644 --- a/synapse/api/constants.py +++ b/synapse/api/constants.py @@ -50,7 +50,7 @@ class Membership: KNOCK: Final = "knock" LEAVE: Final = "leave" BAN: Final = "ban" - LIST: Final = (INVITE, JOIN, KNOCK, LEAVE, BAN) + LIST: Final = frozenset((INVITE, JOIN, KNOCK, LEAVE, BAN)) class PresenceState: