From 76f7c91e441cd21f518b2fa79903cb4d03caaae3 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 11 Sep 2024 13:16:57 +0100 Subject: [PATCH] Sliding sync: don't fetch room summary for named rooms. (#17683) For rooms with a name we can skip fetching a full room summary, as we don't need to calculate heroes, and instead just fetch the room counts directly. This also changes things to not return counts and heroes for non-joined rooms. For left/banned rooms we were returning zero values anyway, and for invite/knock rooms we don't really want to leak such information (even if some of is included in the stripped state). --- changelog.d/17683.misc | 1 + synapse/handlers/sliding_sync/__init__.py | 78 +++++++++---------- synapse/storage/_base.py | 2 + synapse/storage/databases/main/roommember.py | 37 ++++++--- synapse/storage/databases/main/state.py | 8 ++ .../main/delta/87/03_current_state_index.sql | 19 +++++ .../client/sliding_sync/test_rooms_meta.py | 52 ++++++------- 7 files changed, 121 insertions(+), 76 deletions(-) create mode 100644 changelog.d/17683.misc create mode 100644 synapse/storage/schema/main/delta/87/03_current_state_index.sql diff --git a/changelog.d/17683.misc b/changelog.d/17683.misc new file mode 100644 index 000000000..11a10ff85 --- /dev/null +++ b/changelog.d/17683.misc @@ -0,0 +1 @@ +Speed up sliding sync by reducing amount of data pulled out of the database for large rooms. diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index b097ac57a..04493494a 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -784,32 +784,10 @@ class SlidingSyncHandler: ): avatar_changed = True + # We only need the room summary for calculating heroes, however if we do + # fetch it then we can use it to calculate `joined_count` and + # `invited_count`. room_membership_summary: Optional[Mapping[str, MemberSummary]] = None - empty_membership_summary = MemberSummary([], 0) - # We need the room summary for: - # - Always for initial syncs (or the first time we send down the room) - # - When the room has no name, we need `heroes` - # - When the membership has changed so we need to give updated `heroes` and - # `joined_count`/`invited_count`. - # - # Ideally, instead of just looking at `name_changed`, we'd check if the room - # name is not set but this is a good enough approximation that saves us from - # having to pull out the full event. This just means, we're generating the - # summary whenever the room name changes instead of only when it changes to - # `None`. - if initial or name_changed or membership_changed: - # We can't trace the function directly because it's cached and the `@cached` - # decorator doesn't mix with `@trace` yet. - with start_active_span("get_room_summary"): - if room_membership_for_user_at_to_token.membership in ( - Membership.LEAVE, - Membership.BAN, - ): - # TODO: Figure out how to get the membership summary for left/banned rooms - room_membership_summary = {} - else: - room_membership_summary = await self.store.get_room_summary(room_id) - # TODO: Reverse/rewind back to the `to_token` # `heroes` are required if the room name is not set. # @@ -828,11 +806,45 @@ class SlidingSyncHandler: # get them on initial syncs (or the first time we send down the room) or if the # membership has changed which may change the heroes. if name_event_id is None and (initial or (not initial and membership_changed)): - assert room_membership_summary is not None + # We need the room summary to extract the heroes from + if room_membership_for_user_at_to_token.membership != Membership.JOIN: + # TODO: Figure out how to get the membership summary for left/banned rooms + # For invite/knock rooms we don't include the information. + room_membership_summary = {} + else: + room_membership_summary = await self.store.get_room_summary(room_id) + # TODO: Reverse/rewind back to the `to_token` + hero_user_ids = extract_heroes_from_room_summary( room_membership_summary, me=user.to_string() ) + # Fetch the membership counts for rooms we're joined to. + # + # Similarly to other metadata, we only need to calculate the member + # counts if this is an initial sync or the memberships have changed. + joined_count: Optional[int] = None + invited_count: Optional[int] = None + if ( + initial or membership_changed + ) and room_membership_for_user_at_to_token.membership == Membership.JOIN: + # If we have the room summary (because we calculated heroes above) + # then we can simply pull the counts from there. + if room_membership_summary is not None: + empty_membership_summary = MemberSummary([], 0) + + joined_count = room_membership_summary.get( + Membership.JOIN, empty_membership_summary + ).count + + invited_count = room_membership_summary.get( + Membership.INVITE, empty_membership_summary + ).count + else: + member_counts = await self.store.get_member_counts(room_id) + joined_count = member_counts.get(Membership.JOIN, 0) + invited_count = member_counts.get(Membership.INVITE, 0) + # Fetch the `required_state` for the room # # No `required_state` for invite/knock rooms (just `stripped_state`) @@ -1090,20 +1102,6 @@ class SlidingSyncHandler: set_tag(SynapseTags.RESULT_PREFIX + "initial", initial) - joined_count: Optional[int] = None - if initial or membership_changed: - assert room_membership_summary is not None - joined_count = room_membership_summary.get( - Membership.JOIN, empty_membership_summary - ).count - - invited_count: Optional[int] = None - if initial or membership_changed: - assert room_membership_summary is not None - invited_count = room_membership_summary.get( - Membership.INVITE, empty_membership_summary - ).count - return SlidingSyncResult.RoomResult( name=room_name, avatar=room_avatar, diff --git a/synapse/storage/_base.py b/synapse/storage/_base.py index d22160b85..d6deb077c 100644 --- a/synapse/storage/_base.py +++ b/synapse/storage/_base.py @@ -112,6 +112,7 @@ class SQLBaseStore(metaclass=ABCMeta): self._attempt_to_invalidate_cache( "get_number_joined_users_in_room", (room_id,) ) + self._attempt_to_invalidate_cache("get_member_counts", (room_id,)) self._attempt_to_invalidate_cache("get_local_users_in_room", (room_id,)) # There's no easy way of invalidating this cache for just the users @@ -153,6 +154,7 @@ class SQLBaseStore(metaclass=ABCMeta): self._attempt_to_invalidate_cache("get_current_hosts_in_room", (room_id,)) self._attempt_to_invalidate_cache("get_users_in_room_with_profiles", (room_id,)) self._attempt_to_invalidate_cache("get_number_joined_users_in_room", (room_id,)) + self._attempt_to_invalidate_cache("get_member_counts", (room_id,)) self._attempt_to_invalidate_cache("get_local_users_in_room", (room_id,)) 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) diff --git a/synapse/storage/databases/main/roommember.py b/synapse/storage/databases/main/roommember.py index 8df760e8a..db03729cf 100644 --- a/synapse/storage/databases/main/roommember.py +++ b/synapse/storage/databases/main/roommember.py @@ -312,18 +312,10 @@ class RoomMemberWorkerStore(EventsWorkerStore, CacheInvalidationWorkerStore): # We do this all in one transaction to keep the cache small. # FIXME: get rid of this when we have room_stats - # Note, rejected events will have a null membership field, so - # we we manually filter them out. - sql = """ - SELECT count(*), membership FROM current_state_events - WHERE type = 'm.room.member' AND room_id = ? - AND membership IS NOT NULL - GROUP BY membership - """ + counts = self._get_member_counts_txn(txn, room_id) - txn.execute(sql, (room_id,)) res: Dict[str, MemberSummary] = {} - for count, membership in txn: + for membership, count in counts.items(): res.setdefault(membership, MemberSummary([], count)) # Order by membership (joins -> invites -> leave (former insiders) -> @@ -369,6 +361,31 @@ class RoomMemberWorkerStore(EventsWorkerStore, CacheInvalidationWorkerStore): "get_room_summary", _get_room_summary_txn ) + @cached() + async def get_member_counts(self, room_id: str) -> Mapping[str, int]: + """Get a mapping of number of users by membership""" + + return await self.db_pool.runInteraction( + "get_member_counts", self._get_member_counts_txn, room_id + ) + + def _get_member_counts_txn( + self, txn: LoggingTransaction, room_id: str + ) -> Dict[str, int]: + """Get a mapping of number of users by membership""" + + # Note, rejected events will have a null membership field, so + # we we manually filter them out. + sql = """ + SELECT count(*), membership FROM current_state_events + WHERE type = 'm.room.member' AND room_id = ? + AND membership IS NOT NULL + GROUP BY membership + """ + + txn.execute(sql, (room_id,)) + return {membership: count for count, membership in txn} + @cached() async def get_number_joined_users_in_room(self, room_id: str) -> int: return await self.db_pool.simple_select_one_onecol( diff --git a/synapse/storage/databases/main/state.py b/synapse/storage/databases/main/state.py index c5caaf56b..ca31122ad 100644 --- a/synapse/storage/databases/main/state.py +++ b/synapse/storage/databases/main/state.py @@ -736,6 +736,7 @@ class MainStateBackgroundUpdateStore(RoomMemberWorkerStore): CURRENT_STATE_INDEX_UPDATE_NAME = "current_state_members_idx" EVENT_STATE_GROUP_INDEX_UPDATE_NAME = "event_to_state_groups_sg_index" DELETE_CURRENT_STATE_UPDATE_NAME = "delete_old_current_state_events" + MEMBERS_CURRENT_STATE_UPDATE_NAME = "current_state_events_members_room_index" def __init__( self, @@ -764,6 +765,13 @@ class MainStateBackgroundUpdateStore(RoomMemberWorkerStore): self.DELETE_CURRENT_STATE_UPDATE_NAME, self._background_remove_left_rooms, ) + self.db_pool.updates.register_background_index_update( + self.MEMBERS_CURRENT_STATE_UPDATE_NAME, + index_name="current_state_events_members_room_index", + table="current_state_events", + columns=["room_id", "membership"], + where_clause="type='m.room.member'", + ) async def _background_remove_left_rooms( self, progress: JsonDict, batch_size: int diff --git a/synapse/storage/schema/main/delta/87/03_current_state_index.sql b/synapse/storage/schema/main/delta/87/03_current_state_index.sql new file mode 100644 index 000000000..76b974271 --- /dev/null +++ b/synapse/storage/schema/main/delta/87/03_current_state_index.sql @@ -0,0 +1,19 @@ +-- +-- This file is licensed under the Affero General Public License (AGPL) version 3. +-- +-- Copyright (C) 2024 New Vector, Ltd +-- +-- This program is free software: you can redistribute it and/or modify +-- it under the terms of the GNU Affero General Public License as +-- published by the Free Software Foundation, either version 3 of the +-- License, or (at your option) any later version. +-- +-- See the GNU Affero General Public License for more details: +-- . + + +-- Add a background updates to add a new index: +-- `current_state_events(room_id, membership) WHERE type = 'm.room.member' +-- This makes counting membership in rooms (for syncs) much faster +INSERT INTO background_updates (ordering, update_name, progress_json) VALUES + (8701, 'current_state_events_members_room_index', '{}'); diff --git a/tests/rest/client/sliding_sync/test_rooms_meta.py b/tests/rest/client/sliding_sync/test_rooms_meta.py index 6d2742e25..6dbce7126 100644 --- a/tests/rest/client/sliding_sync/test_rooms_meta.py +++ b/tests/rest/client/sliding_sync/test_rooms_meta.py @@ -371,14 +371,17 @@ class SlidingSyncRoomsMetaTestCase(SlidingSyncBase): "mxc://UPDATED_DUMMY_MEDIA_ID", response_body["rooms"][room_id1], ) - self.assertEqual( - response_body["rooms"][room_id1]["joined_count"], - 1, + + # We don't give extra room information to invitees + self.assertNotIn( + "joined_count", + response_body["rooms"][room_id1], ) - self.assertEqual( - response_body["rooms"][room_id1]["invited_count"], - 1, + self.assertNotIn( + "invited_count", + response_body["rooms"][room_id1], ) + self.assertIsNone( response_body["rooms"][room_id1].get("is_dm"), ) @@ -450,15 +453,16 @@ class SlidingSyncRoomsMetaTestCase(SlidingSyncBase): "mxc://DUMMY_MEDIA_ID", response_body["rooms"][room_id1], ) - self.assertEqual( - response_body["rooms"][room_id1]["joined_count"], - # FIXME: The actual number should be "1" (user2) but we currently don't - # support this for rooms where the user has left/been banned. - 0, + + # FIXME: We possibly want to return joined and invited counts for rooms + # you're banned form + self.assertNotIn( + "joined_count", + response_body["rooms"][room_id1], ) - self.assertEqual( - response_body["rooms"][room_id1]["invited_count"], - 0, + self.assertNotIn( + "invited_count", + response_body["rooms"][room_id1], ) self.assertIsNone( response_body["rooms"][room_id1].get("is_dm"), @@ -692,19 +696,15 @@ class SlidingSyncRoomsMetaTestCase(SlidingSyncBase): [], ) - self.assertEqual( - response_body["rooms"][room_id1]["joined_count"], - # FIXME: The actual number should be "1" (user2) but we currently don't - # support this for rooms where the user has left/been banned. - 0, + # FIXME: We possibly want to return joined and invited counts for rooms + # you're banned form + self.assertNotIn( + "joined_count", + response_body["rooms"][room_id1], ) - self.assertEqual( - response_body["rooms"][room_id1]["invited_count"], - # We shouldn't see user5 since they were invited after user1 was banned. - # - # FIXME: The actual number should be "1" (user3) but we currently don't - # support this for rooms where the user has left/been banned. - 0, + self.assertNotIn( + "invited_count", + response_body["rooms"][room_id1], ) def test_rooms_meta_heroes_incremental_sync_no_change(self) -> None: