From 6f7bed22d501e889a03bf6e569487685a1ee99db Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 28 Apr 2021 17:51:07 +0100 Subject: [PATCH 1/5] Don't set the external cache if its been done recently This may shave some time off event sending and make Redis less busy. --- synapse/handlers/federation.py | 4 +++- synapse/handlers/message.py | 33 +++++++++++++++++++++++++++++---- 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 9d867aaf4d55..e8330a2b50db 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -2446,7 +2446,9 @@ async def _check_event_auth( # If we are going to send this event over federation we precaclculate # the joined hosts. if event.internal_metadata.get_send_on_behalf_of(): - await self.event_creation_handler.cache_joined_hosts_for_event(event) + await self.event_creation_handler.cache_joined_hosts_for_event( + event, context + ) return context diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index ec8eb2167463..5332d33cf1fa 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -51,6 +51,7 @@ from synapse.types import Requester, RoomAlias, StreamToken, UserID, create_requester from synapse.util import json_decoder, json_encoder from synapse.util.async_helpers import Linearizer +from synapse.util.caches.expiringcache import ExpiringCache from synapse.util.metrics import measure_func from synapse.visibility import filter_events_for_client @@ -447,6 +448,19 @@ def __init__(self, hs: "HomeServer"): self._external_cache = hs.get_external_cache() + # Stores the state groups we've recently added to the joined hosts + # external cache. Note that the timeout must be significantly less than + # the TTL on the external cache. + self._external_cache_joined_hosts_updates = ( + None + ) # type: Optional[ExpiringCache] + if self._external_cache.is_enabled(): + self._external_cache_joined_hosts_updates = ExpiringCache( + "_external_cache_joined_hosts_updates", + self.clock, + expiry_ms=30 * 60 * 1000, + ) + async def create_event( self, requester: Requester, @@ -957,7 +971,7 @@ async def handle_new_client_event( await self.action_generator.handle_push_actions_for_event(event, context) - await self.cache_joined_hosts_for_event(event) + await self.cache_joined_hosts_for_event(event, context) try: # If we're a worker we need to hit out to the master. @@ -998,7 +1012,9 @@ async def handle_new_client_event( await self.store.remove_push_actions_from_staging(event.event_id) raise - async def cache_joined_hosts_for_event(self, event: EventBase) -> None: + async def cache_joined_hosts_for_event( + self, event: EventBase, context: EventContext + ) -> None: """Precalculate the joined hosts at the event, when using Redis, so that external federation senders don't have to recalculate it themselves. """ @@ -1013,16 +1029,22 @@ async def cache_joined_hosts_for_event(self, event: EventBase) -> None: # Note: We have to cache event ID -> prev state group, as we don't # store that in the DB. # - # Note: We always set the state group -> joined hosts cache, even if - # we already set it, so that the expiry time is reset. + # Note: We set the state group -> joined hosts cache if it hasn't been + # set for a while, so that the expiry time is reset. state_entry = await self.state.resolve_state_groups_for_events( event.room_id, event_ids=event.prev_event_ids() ) if state_entry.state_group: + if self._external_cache_joined_hosts_updates: + if state_entry.state_group in self._external_cache_joined_hosts_updates: + return + joined_hosts = await self.store.get_joined_hosts(event.room_id, state_entry) + # Note that the expiry times must be larger than the expiry time in + # _external_cache_joined_hosts_updates. await self._external_cache.set( "event_to_prev_state_group", event.event_id, @@ -1036,6 +1058,9 @@ async def cache_joined_hosts_for_event(self, event: EventBase) -> None: expiry_ms=60 * 60 * 1000, ) + if self._external_cache_joined_hosts_updates: + self._external_cache_joined_hosts_updates[context.prev_group] = None + async def _validate_canonical_alias( self, directory_handler, room_alias_str: str, expected_room_id: str ) -> None: From a64038e370f3981a4a8ac017f82701e53a05c674 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 28 Apr 2021 17:52:26 +0100 Subject: [PATCH 2/5] Newsfile --- changelog.d/9905.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/9905.misc diff --git a/changelog.d/9905.misc b/changelog.d/9905.misc new file mode 100644 index 000000000000..95c331922b7e --- /dev/null +++ b/changelog.d/9905.misc @@ -0,0 +1 @@ +Don't set the external cache for "joined hosts" if its been done recently. From c3d7b3a6f9eef73be92de257814793343a3aaca5 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 4 May 2021 13:36:10 +0100 Subject: [PATCH 3/5] Code review --- synapse/handlers/message.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 5332d33cf1fa..45659863a6f0 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -1022,6 +1022,9 @@ async def cache_joined_hosts_for_event( if not self._external_cache.is_enabled(): return + # If external cache is enabled we should always have this. + assert self._external_cache_joined_hosts_updates + # We actually store two mappings, event ID -> prev state group, # state group -> joined hosts, which is much more space efficient # than event ID -> joined hosts. @@ -1037,9 +1040,8 @@ async def cache_joined_hosts_for_event( ) if state_entry.state_group: - if self._external_cache_joined_hosts_updates: - if state_entry.state_group in self._external_cache_joined_hosts_updates: - return + if state_entry.state_group in self._external_cache_joined_hosts_updates: + return joined_hosts = await self.store.get_joined_hosts(event.room_id, state_entry) @@ -1058,8 +1060,7 @@ async def cache_joined_hosts_for_event( expiry_ms=60 * 60 * 1000, ) - if self._external_cache_joined_hosts_updates: - self._external_cache_joined_hosts_updates[context.prev_group] = None + self._external_cache_joined_hosts_updates[state_entry.state_group] = None async def _validate_canonical_alias( self, directory_handler, room_alias_str: str, expected_room_id: str From 47e7558ae50f61f4ff61e2e74a0a5cf534833150 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 4 May 2021 14:12:36 +0100 Subject: [PATCH 4/5] Fix missign 'is not None' --- synapse/handlers/message.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 45659863a6f0..714ee77b205c 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -1023,7 +1023,7 @@ async def cache_joined_hosts_for_event( return # If external cache is enabled we should always have this. - assert self._external_cache_joined_hosts_updates + assert self._external_cache_joined_hosts_updates is not None # We actually store two mappings, event ID -> prev state group, # state group -> joined hosts, which is much more space efficient From 7e88d003a72f4dac7b92979f9b5b3eb59b6b51fc Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 5 May 2021 16:53:10 +0100 Subject: [PATCH 5/5] Newsfile --- changelog.d/9905.feature | 1 + changelog.d/9905.misc | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) create mode 100644 changelog.d/9905.feature delete mode 100644 changelog.d/9905.misc diff --git a/changelog.d/9905.feature b/changelog.d/9905.feature new file mode 100644 index 000000000000..96a0e7f09fbc --- /dev/null +++ b/changelog.d/9905.feature @@ -0,0 +1 @@ +Improve performance of sending events for worker-based deployments using Redis. diff --git a/changelog.d/9905.misc b/changelog.d/9905.misc deleted file mode 100644 index 95c331922b7e..000000000000 --- a/changelog.d/9905.misc +++ /dev/null @@ -1 +0,0 @@ -Don't set the external cache for "joined hosts" if its been done recently.