From cd4d26fcb254da18e3b8bf20546578b0c12ebcc5 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 22 Apr 2022 13:32:00 +0100 Subject: [PATCH 01/16] skip a dict construction we may as well just chain together the two inputs --- synapse/handlers/federation.py | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 78d149905f52..896f66a9f792 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -1,4 +1,4 @@ -# Copyright 2014-2021 The Matrix.org Foundation C.I.C. +# Copyright 2014-2022 The Matrix.org Foundation C.I.C. # Copyright 2020 Sorunome # # Licensed under the Apache License, Version 2.0 (the "License"); @@ -15,6 +15,7 @@ """Contains handlers for federation events.""" +import itertools import logging from http import HTTPStatus from typing import TYPE_CHECKING, Dict, Iterable, List, Optional, Tuple, Union @@ -230,14 +231,18 @@ async def _maybe_backfill_inner( if not filtered_extremities and not insertion_events_to_be_backfilled: return False - extremities = { - **oldest_events_with_depth, - # TODO: insertion_events_to_be_backfilled is currently skipping the filtered_extremities checks - **insertion_events_to_be_backfilled, - } + # TODO: insertion_events_to_be_backfilled is currently skipping the filtered_extremities checks + + # we now have a list of potential places to backpaginate from. We prefer to + # start with the most recent (ie, max depth), so let's sort the list. + sorted_extremeties_tuple: List[Tuple[str, int]] = sorted( + itertools.chain( + oldest_events_with_depth.items(), + insertion_events_to_be_backfilled.items(), + ), + key=lambda e: -int(e[1]), + ) - # Check if we reached a point where we should start backfilling. - sorted_extremeties_tuple = sorted(extremities.items(), key=lambda e: -int(e[1])) max_depth = sorted_extremeties_tuple[0][1] # If we're approaching an extremity we trigger a backfill, otherwise we From 9a68a56e34cd04c134d02218cd95279cbb5fafe3 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 22 Apr 2022 13:34:27 +0100 Subject: [PATCH 02/16] Move some filtering and sorting logic earlier We can potentially skip some expensive db work by moving this synchronous code earlier. The `sorted` might be expensive, but nowhere near as expensive as the db lookups. --- synapse/handlers/federation.py | 108 ++++++++++++++++----------------- 1 file changed, 54 insertions(+), 54 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 896f66a9f792..940e25d7acdd 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -179,60 +179,6 @@ async def _maybe_backfill_inner( logger.debug("Not backfilling as no extremeties found.") return False - # We only want to paginate if we can actually see the events we'll get, - # as otherwise we'll just spend a lot of resources to get redacted - # events. - # - # We do this by filtering all the backwards extremities and seeing if - # any remain. Given we don't have the extremity events themselves, we - # need to actually check the events that reference them. - # - # *Note*: the spec wants us to keep backfilling until we reach the start - # of the room in case we are allowed to see some of the history. However - # in practice that causes more issues than its worth, as a) its - # relatively rare for there to be any visible history and b) even when - # there is its often sufficiently long ago that clients would stop - # attempting to paginate before backfill reached the visible history. - # - # TODO: If we do do a backfill then we should filter the backwards - # extremities to only include those that point to visible portions of - # history. - # - # TODO: Correctly handle the case where we are allowed to see the - # forward event but not the backward extremity, e.g. in the case of - # initial join of the server where we are allowed to see the join - # event but not anything before it. This would require looking at the - # state *before* the event, ignoring the special casing certain event - # types have. - - forward_event_ids = await self.store.get_successor_events( - list(oldest_events_with_depth) - ) - - extremities_events = await self.store.get_events( - forward_event_ids, - redact_behaviour=EventRedactBehaviour.AS_IS, - get_prev_content=False, - ) - - # We set `check_history_visibility_only` as we might otherwise get false - # positives from users having been erased. - filtered_extremities = await filter_events_for_server( - self.storage, - self.server_name, - list(extremities_events.values()), - redact=False, - check_history_visibility_only=True, - ) - logger.debug( - "_maybe_backfill_inner: filtered_extremities %s", filtered_extremities - ) - - if not filtered_extremities and not insertion_events_to_be_backfilled: - return False - - # TODO: insertion_events_to_be_backfilled is currently skipping the filtered_extremities checks - # we now have a list of potential places to backpaginate from. We prefer to # start with the most recent (ie, max depth), so let's sort the list. sorted_extremeties_tuple: List[Tuple[str, int]] = sorted( @@ -292,6 +238,60 @@ async def _maybe_backfill_inner( if filtered_sorted_extremeties_tuple: sorted_extremeties_tuple = filtered_sorted_extremeties_tuple + # We only want to paginate if we can actually see the events we'll get, + # as otherwise we'll just spend a lot of resources to get redacted + # events. + # + # We do this by filtering all the backwards extremities and seeing if + # any remain. Given we don't have the extremity events themselves, we + # need to actually check the events that reference them. + # + # *Note*: the spec wants us to keep backfilling until we reach the start + # of the room in case we are allowed to see some of the history. However + # in practice that causes more issues than its worth, as a) its + # relatively rare for there to be any visible history and b) even when + # there is its often sufficiently long ago that clients would stop + # attempting to paginate before backfill reached the visible history. + # + # TODO: If we do do a backfill then we should filter the backwards + # extremities to only include those that point to visible portions of + # history. + # + # TODO: Correctly handle the case where we are allowed to see the + # forward event but not the backward extremity, e.g. in the case of + # initial join of the server where we are allowed to see the join + # event but not anything before it. This would require looking at the + # state *before* the event, ignoring the special casing certain event + # types have. + + forward_event_ids = await self.store.get_successor_events( + list(oldest_events_with_depth) + ) + + extremities_events = await self.store.get_events( + forward_event_ids, + redact_behaviour=EventRedactBehaviour.AS_IS, + get_prev_content=False, + ) + + # We set `check_history_visibility_only` as we might otherwise get false + # positives from users having been erased. + filtered_extremities = await filter_events_for_server( + self.storage, + self.server_name, + list(extremities_events.values()), + redact=False, + check_history_visibility_only=True, + ) + logger.debug( + "_maybe_backfill_inner: filtered_extremities %s", filtered_extremities + ) + + if not filtered_extremities and not insertion_events_to_be_backfilled: + return False + + # TODO: insertion_events_to_be_backfilled is currently skipping the filtered_extremities checks + # We don't want to specify too many extremities as it causes the backfill # request URI to be too long. extremities = dict(sorted_extremeties_tuple[:5]) From 8843069d6e5736c544cdd66a245f904186f18814 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 22 Apr 2022 13:38:41 +0100 Subject: [PATCH 03/16] update some comments and logs --- synapse/handlers/federation.py | 42 +++++++++++++++++++++------------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 940e25d7acdd..1b30169391bc 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -189,7 +189,14 @@ async def _maybe_backfill_inner( key=lambda e: -int(e[1]), ) - max_depth = sorted_extremeties_tuple[0][1] + logger.debug( + "_maybe_backfill_inner: room_id: %s: current_depth: %s, limit: %s, extrems (%d): %s", + room_id, + current_depth, + limit, + len(sorted_extremeties_tuple), + sorted_extremeties_tuple, + ) # If we're approaching an extremity we trigger a backfill, otherwise we # no-op. @@ -200,6 +207,11 @@ async def _maybe_backfill_inner( # chose more than one times the limit in case of failure, but choosing a # much larger factor will result in triggering a backfill request much # earlier than necessary. + # + # XXX: shouldn't we do this *after* the filter by depth below? Again, we don't + # care about events that have happened after our current position. + # + max_depth = sorted_extremeties_tuple[0][1] if current_depth - 2 * limit > max_depth: logger.debug( "Not backfilling as we don't need to. %d < %d - 2 * %d", @@ -216,27 +228,25 @@ async def _maybe_backfill_inner( # 2. we have likely previously tried and failed to backfill from that # extremity, so to avoid getting "stuck" requesting the same # backfill repeatedly we drop those extremities. - filtered_sorted_extremeties_tuple = [ - t for t in sorted_extremeties_tuple if int(t[1]) <= current_depth - ] - - logger.debug( - "room_id: %s, backfill: current_depth: %s, limit: %s, max_depth: %s, extrems (%d): %s filtered_sorted_extremeties_tuple: %s", - room_id, - current_depth, - limit, - max_depth, - len(sorted_extremeties_tuple), - sorted_extremeties_tuple, - filtered_sorted_extremeties_tuple, - ) - + # # However, we need to check that the filtered extremities are non-empty. # If they are empty then either we can a) bail or b) still attempt to # backfill. We opt to try backfilling anyway just in case we do get # relevant events. + # + filtered_sorted_extremeties_tuple = [ + t for t in sorted_extremeties_tuple if int(t[1]) <= current_depth + ] if filtered_sorted_extremeties_tuple: + logger.debug( + "_maybe_backfill_inner: extrems before current depth: %s", + filtered_sorted_extremeties_tuple, + ) sorted_extremeties_tuple = filtered_sorted_extremeties_tuple + else: + logger.debug( + "_maybe_backfill_inner: all extrems are *after* current depth. Backfilling anyway." + ) # We only want to paginate if we can actually see the events we'll get, # as otherwise we'll just spend a lot of resources to get redacted From 8feaeceb95d468bd2577cba2322c3000ceab896a Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 22 Apr 2022 18:41:06 +0100 Subject: [PATCH 04/16] Use lists of tuples instead of Dicts Both of these queries `GROUP BY b.event_id`, so there's no deduplication being done, and the dict has no value for us. --- synapse/handlers/federation.py | 6 +++--- .../storage/databases/main/event_federation.py | 15 ++++++++------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 1b30169391bc..60fc74effd51 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -162,7 +162,7 @@ async def _maybe_backfill_inner( await self.store.get_oldest_event_ids_with_depth_in_room(room_id) ) - insertion_events_to_be_backfilled: Dict[str, int] = {} + insertion_events_to_be_backfilled: List[Tuple[str, int]] = [] if self.hs.config.experimental.msc2716_enabled: insertion_events_to_be_backfilled = ( await self.store.get_insertion_event_backward_extremities_in_room( @@ -183,8 +183,8 @@ async def _maybe_backfill_inner( # start with the most recent (ie, max depth), so let's sort the list. sorted_extremeties_tuple: List[Tuple[str, int]] = sorted( itertools.chain( - oldest_events_with_depth.items(), - insertion_events_to_be_backfilled.items(), + oldest_events_with_depth, + insertion_events_to_be_backfilled, ), key=lambda e: -int(e[1]), ) diff --git a/synapse/storage/databases/main/event_federation.py b/synapse/storage/databases/main/event_federation.py index 634e19e035d2..d97c4593ab8a 100644 --- a/synapse/storage/databases/main/event_federation.py +++ b/synapse/storage/databases/main/event_federation.py @@ -695,7 +695,9 @@ def _get_auth_chain_difference_txn( # Return all events where not all sets can reach them. return {eid for eid, n in event_to_missing_sets.items() if n} - async def get_oldest_event_ids_with_depth_in_room(self, room_id) -> Dict[str, int]: + async def get_oldest_event_ids_with_depth_in_room( + self, room_id + ) -> List[Tuple[str, int]]: """Gets the oldest events(backwards extremities) in the room along with the aproximate depth. @@ -708,7 +710,7 @@ async def get_oldest_event_ids_with_depth_in_room(self, room_id) -> Dict[str, in room_id: Room where we want to find the oldest events Returns: - Map from event_id to depth + List of (event_id, depth) tuples """ def get_oldest_event_ids_with_depth_in_room_txn(txn, room_id): @@ -741,7 +743,7 @@ def get_oldest_event_ids_with_depth_in_room_txn(txn, room_id): txn.execute(sql, (room_id, False)) - return dict(txn) + return txn.fetchall() return await self.db_pool.runInteraction( "get_oldest_event_ids_with_depth_in_room", @@ -751,7 +753,7 @@ def get_oldest_event_ids_with_depth_in_room_txn(txn, room_id): async def get_insertion_event_backward_extremities_in_room( self, room_id - ) -> Dict[str, int]: + ) -> List[Tuple[str, int]]: """Get the insertion events we know about that we haven't backfilled yet. We use this function so that we can compare and see if someones current @@ -763,7 +765,7 @@ async def get_insertion_event_backward_extremities_in_room( room_id: Room where we want to find the oldest events Returns: - Map from event_id to depth + List of (event_id, depth) tuples """ def get_insertion_event_backward_extremities_in_room_txn(txn, room_id): @@ -778,8 +780,7 @@ def get_insertion_event_backward_extremities_in_room_txn(txn, room_id): """ txn.execute(sql, (room_id,)) - - return dict(txn) + return txn.fetchall() return await self.db_pool.runInteraction( "get_insertion_event_backward_extremities_in_room", From b945de6a1845f6884097eb712b2f8b6aa0737fa5 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 25 Apr 2022 11:38:29 +0100 Subject: [PATCH 05/16] Use an `attrs` to track backfill points Tuples are annoying. --- synapse/handlers/federation.py | 78 ++++++++++++++++++++++------------ 1 file changed, 52 insertions(+), 26 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 60fc74effd51..c4fe044c5078 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -15,11 +15,14 @@ """Contains handlers for federation events.""" +import enum import itertools import logging +from enum import Enum from http import HTTPStatus from typing import TYPE_CHECKING, Dict, Iterable, List, Optional, Tuple, Union +import attr from signedjson.key import decode_verify_key_bytes from signedjson.sign import verify_signed_json from unpaddedbase64 import decode_base64 @@ -93,6 +96,24 @@ def get_domains_from_state(state: StateMap[EventBase]) -> List[Tuple[str, int]]: return sorted(joined_domains.items(), key=lambda d: d[1]) +class _BackfillPointType(Enum): + # a regular backwards extremity (ie, an event which we don't yet have, but which + # is referred to by other events in the DAG) + BACKWARDS_EXTREMITY = enum.auto() + + # an MSC2716 "insertion event" + INSERTION_PONT = enum.auto() + + +@attr.s(slots=True, auto_attribs=True, frozen=True) +class _BackfillPoint: + """A potential point we might backfill from""" + + event_id: str + depth: int + type: _BackfillPointType + + class FederationHandler: """Handles general incoming federation requests @@ -158,44 +179,49 @@ async def maybe_backfill( async def _maybe_backfill_inner( self, room_id: str, current_depth: int, limit: int ) -> bool: - oldest_events_with_depth = ( - await self.store.get_oldest_event_ids_with_depth_in_room(room_id) - ) + backwards_extremities = [ + _BackfillPoint(event_id, depth, _BackfillPointType.BACKWARDS_EXTREMITY) + for event_id, depth in await self.store.get_oldest_event_ids_with_depth_in_room( + room_id + ) + ] - insertion_events_to_be_backfilled: List[Tuple[str, int]] = [] + insertion_events_to_be_backfilled: List[_BackfillPoint] = [] if self.hs.config.experimental.msc2716_enabled: - insertion_events_to_be_backfilled = ( - await self.store.get_insertion_event_backward_extremities_in_room( + insertion_events_to_be_backfilled = [ + _BackfillPoint(event_id, depth, _BackfillPointType.INSERTION_PONT) + for event_id, depth in await self.store.get_insertion_event_backward_extremities_in_room( room_id ) - ) + ] logger.debug( - "_maybe_backfill_inner: extremities oldest_events_with_depth=%s insertion_events_to_be_backfilled=%s", - oldest_events_with_depth, + "_maybe_backfill_inner: backwards_extremities=%s insertion_events_to_be_backfilled=%s", + backwards_extremities, insertion_events_to_be_backfilled, ) - if not oldest_events_with_depth and not insertion_events_to_be_backfilled: + if not backwards_extremities and not insertion_events_to_be_backfilled: logger.debug("Not backfilling as no extremeties found.") return False # we now have a list of potential places to backpaginate from. We prefer to # start with the most recent (ie, max depth), so let's sort the list. - sorted_extremeties_tuple: List[Tuple[str, int]] = sorted( + sorted_backfill_points: List[_BackfillPoint] = sorted( itertools.chain( - oldest_events_with_depth, + backwards_extremities, insertion_events_to_be_backfilled, ), - key=lambda e: -int(e[1]), + key=lambda e: -int(e.depth), ) logger.debug( - "_maybe_backfill_inner: room_id: %s: current_depth: %s, limit: %s, extrems (%d): %s", + "_maybe_backfill_inner: room_id: %s: current_depth: %s, limit: %s, " + "backfill points (%d): %s", room_id, current_depth, limit, - len(sorted_extremeties_tuple), - sorted_extremeties_tuple, + len(sorted_backfill_points), + sorted_backfill_points, ) # If we're approaching an extremity we trigger a backfill, otherwise we @@ -211,7 +237,7 @@ async def _maybe_backfill_inner( # XXX: shouldn't we do this *after* the filter by depth below? Again, we don't # care about events that have happened after our current position. # - max_depth = sorted_extremeties_tuple[0][1] + max_depth = sorted_backfill_points[0].depth if current_depth - 2 * limit > max_depth: logger.debug( "Not backfilling as we don't need to. %d < %d - 2 * %d", @@ -234,18 +260,18 @@ async def _maybe_backfill_inner( # backfill. We opt to try backfilling anyway just in case we do get # relevant events. # - filtered_sorted_extremeties_tuple = [ - t for t in sorted_extremeties_tuple if int(t[1]) <= current_depth + filtered_sorted_backfill_points = [ + t for t in sorted_backfill_points if t.depth <= current_depth ] - if filtered_sorted_extremeties_tuple: + if filtered_sorted_backfill_points: logger.debug( - "_maybe_backfill_inner: extrems before current depth: %s", - filtered_sorted_extremeties_tuple, + "_maybe_backfill_inner: backfill points before current depth: %s", + filtered_sorted_backfill_points, ) - sorted_extremeties_tuple = filtered_sorted_extremeties_tuple + sorted_backfill_points = filtered_sorted_backfill_points else: logger.debug( - "_maybe_backfill_inner: all extrems are *after* current depth. Backfilling anyway." + "_maybe_backfill_inner: all backfill points are *after* current depth. Backfilling anyway." ) # We only want to paginate if we can actually see the events we'll get, @@ -275,7 +301,7 @@ async def _maybe_backfill_inner( # types have. forward_event_ids = await self.store.get_successor_events( - list(oldest_events_with_depth) + (e.event_id for e in backwards_extremities) ) extremities_events = await self.store.get_events( @@ -304,7 +330,7 @@ async def _maybe_backfill_inner( # We don't want to specify too many extremities as it causes the backfill # request URI to be too long. - extremities = dict(sorted_extremeties_tuple[:5]) + extremities = [e.event_id for e in sorted_backfill_points[:5]] # Now we need to decide which hosts to hit first. From a1bd48f77e05f09ce38765073d0700cffbea357b Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 25 Apr 2022 12:09:35 +0100 Subject: [PATCH 06/16] Bail out when we find a visible extremity Rather than checking all of the backwards extremities (which may be legion), we check one at a time and bail out as soon as we find a visible one --- synapse/handlers/federation.py | 69 +++++++++++++++++++--------------- 1 file changed, 38 insertions(+), 31 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index c4fe044c5078..6053208007d7 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -279,8 +279,11 @@ async def _maybe_backfill_inner( # events. # # We do this by filtering all the backwards extremities and seeing if - # any remain. Given we don't have the extremity events themselves, we - # need to actually check the events that reference them. + # any remain. + # + # Doing this filtering can be expensive (we load the full state for the room + # at each of the sucessor events), so we check them one at a time until we find + # one that is visible, and then stop. # # *Note*: the spec wants us to keep backfilling until we reach the start # of the room in case we are allowed to see some of the history. However @@ -292,38 +295,42 @@ async def _maybe_backfill_inner( # TODO: If we do do a backfill then we should filter the backwards # extremities to only include those that point to visible portions of # history. - # - # TODO: Correctly handle the case where we are allowed to see the - # forward event but not the backward extremity, e.g. in the case of - # initial join of the server where we are allowed to see the join - # event but not anything before it. This would require looking at the - # state *before* the event, ignoring the special casing certain event - # types have. - - forward_event_ids = await self.store.get_successor_events( - (e.event_id for e in backwards_extremities) - ) - extremities_events = await self.store.get_events( - forward_event_ids, - redact_behaviour=EventRedactBehaviour.AS_IS, - get_prev_content=False, - ) + found_filtered_extremity = False + for bp in backwards_extremities: + # Given we don't have the extremity events themselves, we + # need to actually check the events that reference them - their "successor" + # events. + # + # TODO: Correctly handle the case where we are allowed to see the + # successor event but not the backward extremity, e.g. in the case of + # initial join of the server where we are allowed to see the join + # event but not anything before it. This would require looking at the + # state *before* the event, ignoring the special casing certain event + # types have. + + forward_event_ids = await self.store.get_successor_events([bp.event_id]) + + extremities_events = await self.store.get_events( + forward_event_ids, + redact_behaviour=EventRedactBehaviour.AS_IS, + get_prev_content=False, + ) - # We set `check_history_visibility_only` as we might otherwise get false - # positives from users having been erased. - filtered_extremities = await filter_events_for_server( - self.storage, - self.server_name, - list(extremities_events.values()), - redact=False, - check_history_visibility_only=True, - ) - logger.debug( - "_maybe_backfill_inner: filtered_extremities %s", filtered_extremities - ) + # We set `check_history_visibility_only` as we might otherwise get false + # positives from users having been erased. + filtered_extremities = await filter_events_for_server( + self.storage, + self.server_name, + list(extremities_events.values()), + redact=False, + check_history_visibility_only=True, + ) + if filtered_extremities: + found_filtered_extremity = True + break - if not filtered_extremities and not insertion_events_to_be_backfilled: + if not found_filtered_extremity and not insertion_events_to_be_backfilled: return False # TODO: insertion_events_to_be_backfilled is currently skipping the filtered_extremities checks From cb880815a9cebafa37c3ef1aca0a17bdae6c03c7 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 25 Apr 2022 12:15:45 +0100 Subject: [PATCH 07/16] Use `get_events_as_list` We don't need the dict here. --- synapse/handlers/federation.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 6053208007d7..670a5994aff8 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -311,8 +311,8 @@ async def _maybe_backfill_inner( forward_event_ids = await self.store.get_successor_events([bp.event_id]) - extremities_events = await self.store.get_events( - forward_event_ids, + extremities_events = await self.store.get_events_as_list( + successor_event_ids, redact_behaviour=EventRedactBehaviour.AS_IS, get_prev_content=False, ) @@ -322,7 +322,7 @@ async def _maybe_backfill_inner( filtered_extremities = await filter_events_for_server( self.storage, self.server_name, - list(extremities_events.values()), + extremities_events, redact=False, check_history_visibility_only=True, ) From 1f0a63e6b57ca94775408c3b3ad81ff0df3dbceb Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 25 Apr 2022 12:17:16 +0100 Subject: [PATCH 08/16] Include insertion events in the `filtered_extremities` checks --- synapse/handlers/federation.py | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 670a5994aff8..158493e7aa69 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -297,10 +297,10 @@ async def _maybe_backfill_inner( # history. found_filtered_extremity = False - for bp in backwards_extremities: - # Given we don't have the extremity events themselves, we - # need to actually check the events that reference them - their "successor" - # events. + for bp in sorted_backfill_points: + # For regular backwards extremities, we don't have the extremity events + # themselves, so we need to actually check the events that reference them - + # their "successor" events. # # TODO: Correctly handle the case where we are allowed to see the # successor event but not the backward extremity, e.g. in the case of @@ -308,11 +308,15 @@ async def _maybe_backfill_inner( # event but not anything before it. This would require looking at the # state *before* the event, ignoring the special casing certain event # types have. + if bp.type == _BackfillPointType.INSERTION_PONT: + event_ids_to_check = [bp.event_id] + else: + event_ids_to_check = await self.store.get_successor_events( + [bp.event_id] + ) - forward_event_ids = await self.store.get_successor_events([bp.event_id]) - - extremities_events = await self.store.get_events_as_list( - successor_event_ids, + events_to_check = await self.store.get_events_as_list( + event_ids_to_check, redact_behaviour=EventRedactBehaviour.AS_IS, get_prev_content=False, ) @@ -322,7 +326,7 @@ async def _maybe_backfill_inner( filtered_extremities = await filter_events_for_server( self.storage, self.server_name, - extremities_events, + events_to_check, redact=False, check_history_visibility_only=True, ) @@ -330,11 +334,9 @@ async def _maybe_backfill_inner( found_filtered_extremity = True break - if not found_filtered_extremity and not insertion_events_to_be_backfilled: + if not found_filtered_extremity: return False - # TODO: insertion_events_to_be_backfilled is currently skipping the filtered_extremities checks - # We don't want to specify too many extremities as it causes the backfill # request URI to be too long. extremities = [e.event_id for e in sorted_backfill_points[:5]] From d285af299aef9d54cbbeedecc99eba6a05966303 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 25 Apr 2022 12:24:52 +0100 Subject: [PATCH 09/16] Only request extremities which we think will be visible --- synapse/handlers/federation.py | 43 ++++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 17 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 158493e7aa69..e09779ddf382 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -20,7 +20,7 @@ import logging from enum import Enum from http import HTTPStatus -from typing import TYPE_CHECKING, Dict, Iterable, List, Optional, Tuple, Union +from typing import TYPE_CHECKING, Dict, Iterable, List, Optional, Set, Tuple, Union import attr from signedjson.key import decode_verify_key_bytes @@ -274,16 +274,19 @@ async def _maybe_backfill_inner( "_maybe_backfill_inner: all backfill points are *after* current depth. Backfilling anyway." ) - # We only want to paginate if we can actually see the events we'll get, - # as otherwise we'll just spend a lot of resources to get redacted - # events. + # We still need to narrow down the list of extremities we pass to the remote + # server. We limit to 5 of them, to avoid the request URI becoming too long. + # + # However, we only want to paginate from a particular extremity if we can + # actually see the events we'll get, as otherwise we'll just spend a lot of + # resources to get redacted events. # # We do this by filtering all the backwards extremities and seeing if # any remain. # # Doing this filtering can be expensive (we load the full state for the room # at each of the sucessor events), so we check them one at a time until we find - # one that is visible, and then stop. + # enough good ones, and then stop. # # *Note*: the spec wants us to keep backfilling until we reach the start # of the room in case we are allowed to see some of the history. However @@ -291,13 +294,12 @@ async def _maybe_backfill_inner( # relatively rare for there to be any visible history and b) even when # there is its often sufficiently long ago that clients would stop # attempting to paginate before backfill reached the visible history. - # - # TODO: If we do do a backfill then we should filter the backwards - # extremities to only include those that point to visible portions of - # history. - found_filtered_extremity = False + extremities_to_request: Set[str] = set() for bp in sorted_backfill_points: + if len(extremities_to_request) >= 5: + break + # For regular backwards extremities, we don't have the extremity events # themselves, so we need to actually check the events that reference them - # their "successor" events. @@ -331,15 +333,22 @@ async def _maybe_backfill_inner( check_history_visibility_only=True, ) if filtered_extremities: - found_filtered_extremity = True - break + extremities_to_request.add(bp.event_id) + else: + logger.debug( + "_maybe_backfill_inner: skipping extremity %s as it would not be visible", + bp, + ) - if not found_filtered_extremity: + if not extremities_to_request: + logger.debug( + "_maybe_backfill_inner: found no extremities which would be visible" + ) return False - # We don't want to specify too many extremities as it causes the backfill - # request URI to be too long. - extremities = [e.event_id for e in sorted_backfill_points[:5]] + logger.debug( + "_maybe_backfill_inner: extremities_to_request %s", extremities_to_request + ) # Now we need to decide which hosts to hit first. @@ -359,7 +368,7 @@ async def try_backfill(domains: List[str]) -> bool: for dom in domains: try: await self._federation_event_handler.backfill( - dom, room_id, limit=100, extremities=extremities + dom, room_id, limit=100, extremities=extremities_to_request ) # If this succeeded then we probably already have the # appropriate stuff. From 4ac692f3548d5633a80ac81e2ca2af7e1a53454a Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 22 Apr 2022 14:26:59 +0100 Subject: [PATCH 10/16] add a TODO comment --- synapse/visibility.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/synapse/visibility.py b/synapse/visibility.py index 250f0735975b..de6d2ffc526a 100644 --- a/synapse/visibility.py +++ b/synapse/visibility.py @@ -419,6 +419,13 @@ async def _event_to_memberships( return {} # for each event, get the event_ids of the membership state at those events. + # + # TODO: this means that we request the entire membership list. If there are only + # one or two users on this server, and the room is huge, this is very wasteful + # (it means more db work, and churns the *stateGroupMembersCache*). + # It might be that we could extend StateFilter to specify "give me keys matching + # *:", to avoid this. + event_to_state_ids = await storage.state.get_state_ids_for_events( frozenset(e.event_id for e in events), state_filter=StateFilter.from_types(types=((EventTypes.Member, None),)), From bdfa5a700352ac90893edddf3604a8f830649d53 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 25 Apr 2022 12:29:43 +0100 Subject: [PATCH 11/16] Make `get_successor_events` take a single event ... since all its callers now only pass a single event --- synapse/handlers/federation.py | 4 +--- synapse/handlers/room_batch.py | 2 +- .../storage/databases/main/event_federation.py | 15 ++++++--------- 3 files changed, 8 insertions(+), 13 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index e09779ddf382..58f15a7962d8 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -313,9 +313,7 @@ async def _maybe_backfill_inner( if bp.type == _BackfillPointType.INSERTION_PONT: event_ids_to_check = [bp.event_id] else: - event_ids_to_check = await self.store.get_successor_events( - [bp.event_id] - ) + event_ids_to_check = await self.store.get_successor_events(bp.event_id) events_to_check = await self.store.get_events_as_list( event_ids_to_check, diff --git a/synapse/handlers/room_batch.py b/synapse/handlers/room_batch.py index 78e299d3a5c5..29de7e5bed10 100644 --- a/synapse/handlers/room_batch.py +++ b/synapse/handlers/room_batch.py @@ -54,7 +54,7 @@ async def inherit_depth_from_prev_ids(self, prev_event_ids: List[str]) -> int: # it has a larger `depth` but before the successor event because the `stream_ordering` # is negative before the successor event. successor_event_ids = await self.store.get_successor_events( - [most_recent_prev_event_id] + most_recent_prev_event_id ) # If we can't find any successor events, then it's a forward extremity of diff --git a/synapse/storage/databases/main/event_federation.py b/synapse/storage/databases/main/event_federation.py index d97c4593ab8a..471022470843 100644 --- a/synapse/storage/databases/main/event_federation.py +++ b/synapse/storage/databases/main/event_federation.py @@ -1296,22 +1296,19 @@ def _get_missing_events(self, txn, room_id, earliest_events, latest_events, limi event_results.reverse() return event_results - async def get_successor_events(self, event_ids: Iterable[str]) -> List[str]: - """Fetch all events that have the given events as a prev event + async def get_successor_events(self, event_id: str) -> List[str]: + """Fetch all events that have the given event as a prev event Args: - event_ids: The events to use as the previous events. + event_id: The event to search for as a prev_event. """ - rows = await self.db_pool.simple_select_many_batch( + return await self.db_pool.simple_select_onecol( table="event_edges", - column="prev_event_id", - iterable=event_ids, - retcols=("event_id",), + keyvalues={"prev_event_id": event_id}, + retcol="event_id", desc="get_successor_events", ) - return [row["event_id"] for row in rows] - @wrap_as_background_process("delete_old_forward_extrem_cache") async def _delete_old_forward_extrem_cache(self) -> None: def _delete_old_forward_extrem_cache_txn(txn): From a23e5a1c061eaab675cbce0d17e49235fae24e2a Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 22 Apr 2022 14:40:57 +0100 Subject: [PATCH 12/16] changelog --- changelog.d/12522.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/12522.misc diff --git a/changelog.d/12522.misc b/changelog.d/12522.misc new file mode 100644 index 000000000000..e696f59d9b67 --- /dev/null +++ b/changelog.d/12522.misc @@ -0,0 +1 @@ +Optimise room backfill, to reduce memory usage. From 881d79406838edd664dc1e3899d799d58115dfe9 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Mon, 25 Apr 2022 18:12:12 +0100 Subject: [PATCH 13/16] Update synapse/handlers/federation.py Co-authored-by: David Robertson --- synapse/handlers/federation.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 58f15a7962d8..ed611669f66a 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -285,8 +285,9 @@ async def _maybe_backfill_inner( # any remain. # # Doing this filtering can be expensive (we load the full state for the room - # at each of the sucessor events), so we check them one at a time until we find - # enough good ones, and then stop. + # at each of the backfill points, or (worse) their successors), so we check + # the backfill points one at a time until we find enough good ones, and then + # stop. # # *Note*: the spec wants us to keep backfilling until we reach the start # of the room in case we are allowed to see some of the history. However From 84879633516be303f5b795cb5aac3468da953d88 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 25 Apr 2022 18:15:13 +0100 Subject: [PATCH 14/16] More comment updates, per review --- synapse/handlers/federation.py | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index ed611669f66a..033ac7c05d21 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -274,26 +274,24 @@ async def _maybe_backfill_inner( "_maybe_backfill_inner: all backfill points are *after* current depth. Backfilling anyway." ) - # We still need to narrow down the list of extremities we pass to the remote - # server. We limit to 5 of them, to avoid the request URI becoming too long. + # For performance's sake, we only want to paginate from a particular extremity + # if we can actually see the events we'll get. Otherwise, we'd just spend a lot + # of resources to get redacted events. We check each extremity in turn and + # ignore those which users on our server wouldn't be able to see. # - # However, we only want to paginate from a particular extremity if we can - # actually see the events we'll get, as otherwise we'll just spend a lot of - # resources to get redacted events. + # Additionally, we limit ourselves to backfilling from at most 5 extremities, + # for two reasons: # - # We do this by filtering all the backwards extremities and seeing if - # any remain. - # - # Doing this filtering can be expensive (we load the full state for the room - # at each of the backfill points, or (worse) their successors), so we check - # the backfill points one at a time until we find enough good ones, and then - # stop. + # - The check which determines if we can see an extremity's events can be + # expensive (we load the full state for the room at each of the backfill + # points, or (worse) their successors) + # - We want to avoid the server-server API request URI becoming too long. # # *Note*: the spec wants us to keep backfilling until we reach the start - # of the room in case we are allowed to see some of the history. However - # in practice that causes more issues than its worth, as a) its - # relatively rare for there to be any visible history and b) even when - # there is its often sufficiently long ago that clients would stop + # of the room in case we are allowed to see some of the history. However, + # in practice that causes more issues than its worth, as (a) it's + # relatively rare for there to be any visible history and (b) even when + # there is it's often sufficiently long ago that clients would stop # attempting to paginate before backfill reached the visible history. extremities_to_request: Set[str] = set() From b75b3f5e9f42eeede0f328e29ca2cfcdb41b805a Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 25 Apr 2022 18:15:53 +0100 Subject: [PATCH 15/16] Use a list rather than a set It's impossible for us to have duplicates here, so we may as well use a boring list. --- synapse/handlers/federation.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 033ac7c05d21..dc3d6b22044e 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -20,7 +20,7 @@ import logging from enum import Enum from http import HTTPStatus -from typing import TYPE_CHECKING, Dict, Iterable, List, Optional, Set, Tuple, Union +from typing import TYPE_CHECKING, Dict, Iterable, List, Optional, Tuple, Union import attr from signedjson.key import decode_verify_key_bytes @@ -294,7 +294,7 @@ async def _maybe_backfill_inner( # there is it's often sufficiently long ago that clients would stop # attempting to paginate before backfill reached the visible history. - extremities_to_request: Set[str] = set() + extremities_to_request: List[str] = [] for bp in sorted_backfill_points: if len(extremities_to_request) >= 5: break @@ -330,7 +330,7 @@ async def _maybe_backfill_inner( check_history_visibility_only=True, ) if filtered_extremities: - extremities_to_request.add(bp.event_id) + extremities_to_request.append(bp.event_id) else: logger.debug( "_maybe_backfill_inner: skipping extremity %s as it would not be visible", From 1dfe084bb372989db6ff08efe6f6e6a9907d420d Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Mon, 25 Apr 2022 18:28:43 +0100 Subject: [PATCH 16/16] Update and rename 12522.misc to 12522.bugfix --- changelog.d/12522.bugfix | 1 + changelog.d/12522.misc | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) create mode 100644 changelog.d/12522.bugfix delete mode 100644 changelog.d/12522.misc diff --git a/changelog.d/12522.bugfix b/changelog.d/12522.bugfix new file mode 100644 index 000000000000..2220f05ceb75 --- /dev/null +++ b/changelog.d/12522.bugfix @@ -0,0 +1 @@ +Fix a bug introduced in Synapse 0.99.3 which could cause Synapse to consume large amounts of RAM when back-paginating in a large room. diff --git a/changelog.d/12522.misc b/changelog.d/12522.misc deleted file mode 100644 index e696f59d9b67..000000000000 --- a/changelog.d/12522.misc +++ /dev/null @@ -1 +0,0 @@ -Optimise room backfill, to reduce memory usage.