From 034934ac77986073ab855020d9a01fb99f2ce920 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 2 Feb 2023 15:28:32 +0000 Subject: [PATCH 1/3] Only notify the target of a membership event Naughty, but should be a big speedup in large rooms Changelog --- changelog.d/14971.misc | 1 + synapse/push/bulk_push_rule_evaluator.py | 29 ++++++++++++++++-------- 2 files changed, 21 insertions(+), 9 deletions(-) create mode 100644 changelog.d/14971.misc diff --git a/changelog.d/14971.misc b/changelog.d/14971.misc new file mode 100644 index 000000000000..130045a1237f --- /dev/null +++ b/changelog.d/14971.misc @@ -0,0 +1 @@ +Improve performance of joining and leaving large rooms with many local users. diff --git a/synapse/push/bulk_push_rule_evaluator.py b/synapse/push/bulk_push_rule_evaluator.py index 88cfc05d0552..51c6fad6e0e4 100644 --- a/synapse/push/bulk_push_rule_evaluator.py +++ b/synapse/push/bulk_push_rule_evaluator.py @@ -139,15 +139,26 @@ async def _get_rules_for_event( Returns: Mapping of user ID to their push rules. """ - # We get the users who may need to be notified by first fetching the - # local users currently in the room, finding those that have push rules, - # and *then* checking which users are actually allowed to see the event. - # - # The alternative is to first fetch all users that were joined at the - # event, but that requires fetching the full state at the event, which - # may be expensive for large rooms with few local users. - - local_users = await self.store.get_local_users_in_room(event.room_id) + # If this is a membership event, only calculate push rules for the target. + # While it's possible for users to configure push rules to respond to such an + # event, in practise nobody does this. At the cost of violating the spec a + # little, we can skip fetching a huge number of push rules in large rooms. + # This helps make joins and leaves faster. + if event.type == EventTypes.Member: + if self.hs.is_mine_id(event.state_key): + local_users = [event.state_key] + else: + return {} + else: + # We get the users who may need to be notified by first fetching the + # local users currently in the room, finding those that have push rules, + # and *then* checking which users are actually allowed to see the event. + # + # The alternative is to first fetch all users that were joined at the + # event, but that requires fetching the full state at the event, which + # may be expensive for large rooms with few local users. + + local_users = await self.store.get_local_users_in_room(event.room_id) # Filter out appservice users. local_users = [ From 02fde449e4f183ef6c3d76489abdc4215a325298 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 2 Feb 2023 16:24:38 +0000 Subject: [PATCH 2/3] Don't notify a target who's not in the room Also avoid unncessary DB queries when creating a room --- synapse/push/bulk_push_rule_evaluator.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/synapse/push/bulk_push_rule_evaluator.py b/synapse/push/bulk_push_rule_evaluator.py index 51c6fad6e0e4..ba91b6938528 100644 --- a/synapse/push/bulk_push_rule_evaluator.py +++ b/synapse/push/bulk_push_rule_evaluator.py @@ -145,8 +145,17 @@ async def _get_rules_for_event( # little, we can skip fetching a huge number of push rules in large rooms. # This helps make joins and leaves faster. if event.type == EventTypes.Member: - if self.hs.is_mine_id(event.state_key): - local_users = [event.state_key] + # We never notify a user about their own actions. This is enforced in + # `_action_for_event_by_user` in the loop over `rules_by_user`, but we + # do the same check here to avoid unnecessary DB queries. + if event.sender != event.state_key and self.hs.is_mine_id(event.state_key): + # Check the target is in the room, to avoid notifying them of + # e.g. a pre-emptive ban. + target_already_in_room = await self.store.check_local_user_in_room( + event.state_key, event.room_id + ) + if target_already_in_room: + local_users = [event.state_key] else: return {} else: From 135c9ec9681bf78b59ec02bbcbb2b8819301505b Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 2 Feb 2023 16:47:39 +0000 Subject: [PATCH 3/3] Fix stupid bug --- synapse/push/bulk_push_rule_evaluator.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/synapse/push/bulk_push_rule_evaluator.py b/synapse/push/bulk_push_rule_evaluator.py index ba91b6938528..95851c9a144b 100644 --- a/synapse/push/bulk_push_rule_evaluator.py +++ b/synapse/push/bulk_push_rule_evaluator.py @@ -145,6 +145,7 @@ async def _get_rules_for_event( # little, we can skip fetching a huge number of push rules in large rooms. # This helps make joins and leaves faster. if event.type == EventTypes.Member: + local_users = [] # We never notify a user about their own actions. This is enforced in # `_action_for_event_by_user` in the loop over `rules_by_user`, but we # do the same check here to avoid unnecessary DB queries. @@ -156,8 +157,6 @@ async def _get_rules_for_event( ) if target_already_in_room: local_users = [event.state_key] - else: - return {} else: # We get the users who may need to be notified by first fetching the # local users currently in the room, finding those that have push rules, @@ -184,6 +183,9 @@ async def _get_rules_for_event( local_users = list(local_users) local_users.append(invited) + if not local_users: + return {} + rules_by_user = await self.store.bulk_get_push_rules(local_users) logger.debug("Users in room: %s", local_users)