From d68bda30d5b9f20582446d7824e92791d330a255 Mon Sep 17 00:00:00 2001 From: Nick Barrett Date: Fri, 19 Aug 2022 13:42:33 +0100 Subject: [PATCH 01/12] Remove cached wrap on `_get_joined_users_from_context` method The method doesn't actually do any data fetching and the method that does, `_get_joined_profile_from_event_id`, has it's own cache. --- synapse/storage/databases/main/roommember.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/roommember.py b/synapse/storage/databases/main/roommember.py index 827c1f1efd3b..356c751e444a 100644 --- a/synapse/storage/databases/main/roommember.py +++ b/synapse/storage/databases/main/roommember.py @@ -852,7 +852,6 @@ async def get_joined_users_from_state( room_id, state_group, state, context=state_entry ) - @cached(num_args=2, iterable=True, max_entries=100000) async def _get_joined_users_from_context( self, room_id: str, @@ -861,6 +860,15 @@ async def _get_joined_users_from_context( event: Optional[EventBase] = None, context: Optional["_StateCacheEntry"] = None, ) -> Dict[str, ProfileInfo]: + """ + For a given (room_id, state_group), get a map of user ID to profile information + for user in the room. + + This method doesn't do any fetching of data itself and instead checks various + caches for the relevant data before passing any remaining missing event IDs to + `_get_joined_profiles_from_event_ids` which does the actual data fetching. + """ + # We don't use `state_group`, it's there so that we can cache based # on it. However, it's important that it's never None, since two current_states # with a state_group of None are likely to be different. From 4346425e7d58963791348c957d28c3905d857e8b Mon Sep 17 00:00:00 2001 From: Nick Barrett Date: Fri, 19 Aug 2022 13:45:51 +0100 Subject: [PATCH 02/12] Changelog file --- changelog.d/13569.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/13569.misc diff --git a/changelog.d/13569.misc b/changelog.d/13569.misc new file mode 100644 index 000000000000..ec23f8a30613 --- /dev/null +++ b/changelog.d/13569.misc @@ -0,0 +1 @@ +Remove cache on `_get_joined_users_from_context` calls that are already cached elsewhere. Contributed by Nick @ Beeper (@fizzadar). From caab2e1a0caeccec47716107eec3a677dfcf39d1 Mon Sep 17 00:00:00 2001 From: Nick Barrett Date: Fri, 19 Aug 2022 13:55:13 +0100 Subject: [PATCH 03/12] Remove context argument from `_get_joined_users_from_context` method If the cache hits are indeed next to zero for this method, the context optimisation provides little benefit and thus can be removed. --- synapse/storage/databases/main/roommember.py | 23 +------------------- 1 file changed, 1 insertion(+), 22 deletions(-) diff --git a/synapse/storage/databases/main/roommember.py b/synapse/storage/databases/main/roommember.py index 356c751e444a..94c18c44bf61 100644 --- a/synapse/storage/databases/main/roommember.py +++ b/synapse/storage/databases/main/roommember.py @@ -849,7 +849,7 @@ async def get_joined_users_from_state( assert state_group is not None with Measure(self._clock, "get_joined_users_from_state"): return await self._get_joined_users_from_context( - room_id, state_group, state, context=state_entry + room_id, state_group, state, ) async def _get_joined_users_from_context( @@ -858,7 +858,6 @@ async def _get_joined_users_from_context( state_group: Union[object, int], current_state_ids: StateMap[str], event: Optional[EventBase] = None, - context: Optional["_StateCacheEntry"] = None, ) -> Dict[str, ProfileInfo]: """ For a given (room_id, state_group), get a map of user ID to profile information @@ -881,26 +880,6 @@ async def _get_joined_users_from_context( if key[0] == EventTypes.Member ] - if context is not None: - # If we have a context with a delta from a previous state group, - # check if we also have the result from the previous group in cache. - # If we do then we can reuse that result and simply update it with - # any membership changes in `delta_ids` - if context.prev_group and context.delta_ids: - prev_res = self._get_joined_users_from_context.cache.get_immediate( - (room_id, context.prev_group), None - ) - if prev_res and isinstance(prev_res, dict): - users_in_room = dict(prev_res) - member_event_ids = [ - e_id - for key, e_id in context.delta_ids.items() - if key[0] == EventTypes.Member - ] - for etype, state_key in context.delta_ids: - if etype == EventTypes.Member: - users_in_room.pop(state_key, None) - # We check if we have any of the member event ids in the event cache # before we ask the DB From c4b32ba1682767e4b0380b98753dc277f16a141b Mon Sep 17 00:00:00 2001 From: Nick Barrett Date: Fri, 19 Aug 2022 13:55:25 +0100 Subject: [PATCH 04/12] Remove unused event argument from `_get_joined_users_from_context` --- synapse/storage/databases/main/roommember.py | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/synapse/storage/databases/main/roommember.py b/synapse/storage/databases/main/roommember.py index 94c18c44bf61..b96fbfd9dd27 100644 --- a/synapse/storage/databases/main/roommember.py +++ b/synapse/storage/databases/main/roommember.py @@ -31,7 +31,6 @@ import attr from synapse.api.constants import EventTypes, Membership -from synapse.events import EventBase from synapse.metrics import LaterGauge from synapse.metrics.background_process_metrics import ( run_as_background_process, @@ -857,7 +856,6 @@ async def _get_joined_users_from_context( room_id: str, state_group: Union[object, int], current_state_ids: StateMap[str], - event: Optional[EventBase] = None, ) -> Dict[str, ProfileInfo]: """ For a given (room_id, state_group), get a map of user ID to profile information @@ -908,14 +906,6 @@ async def _get_joined_users_from_context( ) users_in_room.update(row for row in event_to_memberships.values() if row) - if event is not None and event.type == EventTypes.Member: - if event.membership == Membership.JOIN: - if event.event_id in member_event_ids: - users_in_room[event.state_key] = ProfileInfo( - display_name=event.content.get("displayname", None), - avatar_url=event.content.get("avatar_url", None), - ) - return users_in_room @cached(max_entries=10000) From b63692d3e711758753535aa5b0ae9ce1400141ac Mon Sep 17 00:00:00 2001 From: Nick Barrett Date: Fri, 19 Aug 2022 14:56:20 +0100 Subject: [PATCH 05/12] Linting --- synapse/storage/databases/main/roommember.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/roommember.py b/synapse/storage/databases/main/roommember.py index b96fbfd9dd27..2f7e24213026 100644 --- a/synapse/storage/databases/main/roommember.py +++ b/synapse/storage/databases/main/roommember.py @@ -848,7 +848,9 @@ async def get_joined_users_from_state( assert state_group is not None with Measure(self._clock, "get_joined_users_from_state"): return await self._get_joined_users_from_context( - room_id, state_group, state, + room_id, + state_group, + state, ) async def _get_joined_users_from_context( From f992ddf44eb8cb27ed67e9118f8f0533ae525842 Mon Sep 17 00:00:00 2001 From: Nick Barrett Date: Fri, 19 Aug 2022 15:10:03 +0100 Subject: [PATCH 06/12] Remove unused room_id argument --- synapse/storage/databases/main/roommember.py | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/synapse/storage/databases/main/roommember.py b/synapse/storage/databases/main/roommember.py index 2f7e24213026..e18746fcc31d 100644 --- a/synapse/storage/databases/main/roommember.py +++ b/synapse/storage/databases/main/roommember.py @@ -847,21 +847,15 @@ async def get_joined_users_from_state( assert state_group is not None with Measure(self._clock, "get_joined_users_from_state"): - return await self._get_joined_users_from_context( - room_id, - state_group, - state, - ) + return await self._get_joined_users_from_context(state_group, state) async def _get_joined_users_from_context( self, - room_id: str, state_group: Union[object, int], current_state_ids: StateMap[str], ) -> Dict[str, ProfileInfo]: """ - For a given (room_id, state_group), get a map of user ID to profile information - for user in the room. + For a given state_group, get a map of user ID to profile information for users in the room. This method doesn't do any fetching of data itself and instead checks various caches for the relevant data before passing any remaining missing event IDs to From 5f99fe89ce41e1268f44a2740e57d21ff2c33b77 Mon Sep 17 00:00:00 2001 From: Nick Barrett Date: Thu, 25 Aug 2022 09:42:26 +0100 Subject: [PATCH 07/12] Remove `state_group` and `entry` arguments from `get_joined_users_from_state` methods --- synapse/state/__init__.py | 2 +- synapse/storage/databases/main/roommember.py | 26 +++----------------- 2 files changed, 5 insertions(+), 23 deletions(-) diff --git a/synapse/state/__init__.py b/synapse/state/__init__.py index c355e4f98ab3..1769786eee7a 100644 --- a/synapse/state/__init__.py +++ b/synapse/state/__init__.py @@ -233,7 +233,7 @@ async def get_current_users_in_room( logger.debug("calling resolve_state_groups from get_current_users_in_room") entry = await self.resolve_state_groups_for_events(room_id, latest_event_ids) state = await entry.get_state(self._state_storage_controller, StateFilter.all()) - return await self.store.get_joined_users_from_state(room_id, state, entry) + return await self.store.get_joined_users_from_state(room_id, state) async def get_hosts_in_room_at_events( self, room_id: str, event_ids: Collection[str] diff --git a/synapse/storage/databases/main/roommember.py b/synapse/storage/databases/main/roommember.py index e18746fcc31d..d26f0020b81f 100644 --- a/synapse/storage/databases/main/roommember.py +++ b/synapse/storage/databases/main/roommember.py @@ -835,24 +835,13 @@ async def get_mutual_rooms_between_users( return shared_room_ids or frozenset() async def get_joined_users_from_state( - self, room_id: str, state: StateMap[str], state_entry: "_StateCacheEntry" + self, room_id: str, state: StateMap[str] ) -> Dict[str, ProfileInfo]: - state_group: Union[object, int] = state_entry.state_group - if not state_group: - # If state_group is None it means it has yet to be assigned a - # state group, i.e. we need to make sure that calls with a state_group - # of None don't hit previous cached calls with a None state_group. - # To do this we set the state_group to a new object as object() != object() - state_group = object() - - assert state_group is not None with Measure(self._clock, "get_joined_users_from_state"): - return await self._get_joined_users_from_context(state_group, state) + return await self._get_joined_users_from_context(state) async def _get_joined_users_from_context( - self, - state_group: Union[object, int], - current_state_ids: StateMap[str], + self, current_state_ids: StateMap[str] ) -> Dict[str, ProfileInfo]: """ For a given state_group, get a map of user ID to profile information for users in the room. @@ -862,11 +851,6 @@ async def _get_joined_users_from_context( `_get_joined_profiles_from_event_ids` which does the actual data fetching. """ - # We don't use `state_group`, it's there so that we can cache based - # on it. However, it's important that it's never None, since two current_states - # with a state_group of None are likely to be different. - assert state_group is not None - users_in_room = {} member_event_ids = [ e_id @@ -1104,9 +1088,7 @@ async def _get_joined_hosts( else: # The cache doesn't match the state group or prev state group, # so we calculate the result from first principles. - joined_users = await self.get_joined_users_from_state( - room_id, state, state_entry - ) + joined_users = await self.get_joined_users_from_state(room_id, state) cache.hosts_to_joined_users = {} for user_id in joined_users: From 63967da1d790ceb3d4702b91614858b7950b0781 Mon Sep 17 00:00:00 2001 From: Nick Barrett Date: Thu, 25 Aug 2022 09:45:36 +0100 Subject: [PATCH 08/12] Inline `_get_joined_users_from_context` method --- synapse/storage/databases/main/roommember.py | 80 ++++++++++---------- 1 file changed, 39 insertions(+), 41 deletions(-) diff --git a/synapse/storage/databases/main/roommember.py b/synapse/storage/databases/main/roommember.py index d26f0020b81f..242176d1aeca 100644 --- a/synapse/storage/databases/main/roommember.py +++ b/synapse/storage/databases/main/roommember.py @@ -836,57 +836,55 @@ async def get_mutual_rooms_between_users( async def get_joined_users_from_state( self, room_id: str, state: StateMap[str] - ) -> Dict[str, ProfileInfo]: - with Measure(self._clock, "get_joined_users_from_state"): - return await self._get_joined_users_from_context(state) - - async def _get_joined_users_from_context( - self, current_state_ids: StateMap[str] ) -> Dict[str, ProfileInfo]: """ - For a given state_group, get a map of user ID to profile information for users in the room. + For a given set of state IDs, get a map of user ID to profile information + for users in the room. This method doesn't do any fetching of data itself and instead checks various caches for the relevant data before passing any remaining missing event IDs to `_get_joined_profiles_from_event_ids` which does the actual data fetching. """ - users_in_room = {} - member_event_ids = [ - e_id - for key, e_id in current_state_ids.items() - if key[0] == EventTypes.Member - ] - - # We check if we have any of the member event ids in the event cache - # before we ask the DB - - # We don't update the event cache hit ratio as it completely throws off - # the hit ratio counts. After all, we don't populate the cache if we - # miss it here - event_map = self._get_events_from_local_cache( - member_event_ids, update_metrics=False - ) - - missing_member_event_ids = [] - for event_id in member_event_ids: - ev_entry = event_map.get(event_id) - if ev_entry and not ev_entry.event.rejected_reason: - if ev_entry.event.membership == Membership.JOIN: - users_in_room[ev_entry.event.state_key] = ProfileInfo( - display_name=ev_entry.event.content.get("displayname", None), - avatar_url=ev_entry.event.content.get("avatar_url", None), - ) - else: - missing_member_event_ids.append(event_id) - - if missing_member_event_ids: - event_to_memberships = await self._get_joined_profiles_from_event_ids( - missing_member_event_ids + with Measure(self._clock, "get_joined_users_from_state"): + users_in_room = {} + member_event_ids = [ + e_id for key, e_id in state.items() if key[0] == EventTypes.Member + ] + + # We check if we have any of the member event ids in the event cache + # before we ask the DB + + # We don't update the event cache hit ratio as it completely throws off + # the hit ratio counts. After all, we don't populate the cache if we + # miss it here + event_map = self._get_events_from_local_cache( + member_event_ids, update_metrics=False ) - users_in_room.update(row for row in event_to_memberships.values() if row) - return users_in_room + missing_member_event_ids = [] + for event_id in member_event_ids: + ev_entry = event_map.get(event_id) + if ev_entry and not ev_entry.event.rejected_reason: + if ev_entry.event.membership == Membership.JOIN: + users_in_room[ev_entry.event.state_key] = ProfileInfo( + display_name=ev_entry.event.content.get( + "displayname", None + ), + avatar_url=ev_entry.event.content.get("avatar_url", None), + ) + else: + missing_member_event_ids.append(event_id) + + if missing_member_event_ids: + event_to_memberships = await self._get_joined_profiles_from_event_ids( + missing_member_event_ids + ) + users_in_room.update( + row for row in event_to_memberships.values() if row + ) + + return users_in_room @cached(max_entries=10000) def _get_joined_profile_from_event_id( From d5a5f7f49547b8fdb5a44bc7e22b19cb4b505210 Mon Sep 17 00:00:00 2001 From: Nick Barrett Date: Thu, 25 Aug 2022 09:46:37 +0100 Subject: [PATCH 09/12] Update changelog --- changelog.d/13569.misc | 1 - changelog.d/13569.removal | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) delete mode 100644 changelog.d/13569.misc create mode 100644 changelog.d/13569.removal diff --git a/changelog.d/13569.misc b/changelog.d/13569.misc deleted file mode 100644 index ec23f8a30613..000000000000 --- a/changelog.d/13569.misc +++ /dev/null @@ -1 +0,0 @@ -Remove cache on `_get_joined_users_from_context` calls that are already cached elsewhere. Contributed by Nick @ Beeper (@fizzadar). diff --git a/changelog.d/13569.removal b/changelog.d/13569.removal new file mode 100644 index 000000000000..af9d407671b4 --- /dev/null +++ b/changelog.d/13569.removal @@ -0,0 +1 @@ +Remove redundant `_get_joined_users_from_context` cache. Contributed by Nick @ Beeper (@fizzadar). From 64044817b4d968a74fdd6e42f4372336c921eb4a Mon Sep 17 00:00:00 2001 From: Nick Barrett Date: Thu, 25 Aug 2022 15:28:27 +0100 Subject: [PATCH 10/12] Fix mergem missing method rename --- synapse/storage/databases/main/roommember.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/synapse/storage/databases/main/roommember.py b/synapse/storage/databases/main/roommember.py index 1abed09a07c3..7edd2f7f02fa 100644 --- a/synapse/storage/databases/main/roommember.py +++ b/synapse/storage/databases/main/roommember.py @@ -860,9 +860,9 @@ async def get_mutual_rooms_between_users( return shared_room_ids or frozenset() - async def get_joined_users_from_state( + async def get_joined_user_ids_from_state( self, room_id: str, state: StateMap[str] - ) -> Dict[str, ProfileInfo]: + ) -> Set[str]: """ For a given set of state IDs, get a map of user ID to profile information for users in the room. @@ -872,7 +872,7 @@ async def get_joined_users_from_state( `_get_joined_profiles_from_event_ids` which does the actual data fetching. """ - with Measure(self._clock, "get_joined_users_from_state"): + with Measure(self._clock, "get_joined_user_ids_from_state"): users_in_room = set() member_event_ids = [ e_id From 4e62c4632baabda62e21ffff32bfb088407a80f4 Mon Sep 17 00:00:00 2001 From: Nick Barrett Date: Thu, 25 Aug 2022 15:28:36 +0100 Subject: [PATCH 11/12] Fix formatting --- synapse/storage/databases/main/roommember.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/synapse/storage/databases/main/roommember.py b/synapse/storage/databases/main/roommember.py index 7edd2f7f02fa..2b8a18e3c918 100644 --- a/synapse/storage/databases/main/roommember.py +++ b/synapse/storage/databases/main/roommember.py @@ -875,9 +875,7 @@ async def get_joined_user_ids_from_state( with Measure(self._clock, "get_joined_user_ids_from_state"): users_in_room = set() member_event_ids = [ - e_id - for key, e_id in current_state_ids.items() - if key[0] == EventTypes.Member + e_id for key, e_id in state.items() if key[0] == EventTypes.Member ] # We check if we have any of the member event ids in the event cache @@ -900,8 +898,10 @@ async def get_joined_user_ids_from_state( missing_member_event_ids.append(event_id) if missing_member_event_ids: - event_to_memberships = await self._get_user_ids_from_membership_event_ids( - missing_member_event_ids + event_to_memberships = ( + await self._get_user_ids_from_membership_event_ids( + missing_member_event_ids + ) ) users_in_room.update( user_id for user_id in event_to_memberships.values() if user_id @@ -1106,7 +1106,9 @@ async def _get_joined_hosts( else: # The cache doesn't match the state group or prev state group, # so we calculate the result from first principles. - joined_user_ids = await self.get_joined_user_ids_from_state(room_id, state) + joined_user_ids = await self.get_joined_user_ids_from_state( + room_id, state + ) cache.hosts_to_joined_users = {} for user_id in joined_user_ids: From d630c0cdbb129b9cd5bd2a45da5ce759705a1697 Mon Sep 17 00:00:00 2001 From: Nick Barrett Date: Fri, 26 Aug 2022 16:28:47 +0100 Subject: [PATCH 12/12] Tidy up bad docstring --- synapse/storage/databases/main/roommember.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/synapse/storage/databases/main/roommember.py b/synapse/storage/databases/main/roommember.py index 2b8a18e3c918..4f012d0341ca 100644 --- a/synapse/storage/databases/main/roommember.py +++ b/synapse/storage/databases/main/roommember.py @@ -864,12 +864,10 @@ async def get_joined_user_ids_from_state( self, room_id: str, state: StateMap[str] ) -> Set[str]: """ - For a given set of state IDs, get a map of user ID to profile information - for users in the room. + For a given set of state IDs, get a set of user IDs in the room. - This method doesn't do any fetching of data itself and instead checks various - caches for the relevant data before passing any remaining missing event IDs to - `_get_joined_profiles_from_event_ids` which does the actual data fetching. + This method checks the local event cache, before calling + `_get_user_ids_from_membership_event_ids` for any uncached events. """ with Measure(self._clock, "get_joined_user_ids_from_state"):