Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Optimize filter_events_for_client for faster /messages - v1 #14494

Closed
wants to merge 10 commits into from
Closed

Optimize filter_events_for_client for faster /messages - v1 #14494

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ede07ca
Add more tracing to filter_events_for_client
MadLittleMods Nov 19, 2022
8340906
Add suspicion
MadLittleMods Nov 19, 2022
3ee285f
Slight changes
MadLittleMods Nov 22, 2022
2e86455
Add alternative lookup
MadLittleMods Nov 22, 2022
92a1aaf
Clean up logging for fair comparison
MadLittleMods Nov 22, 2022
0459a9c
Compare old and new
MadLittleMods Nov 22, 2022
2939ead
Fix lints
MadLittleMods Nov 22, 2022
65a5d8f
SQLite compatible cast
MadLittleMods Nov 22, 2022
d4b647b
No need to order by stuff that isn't different
MadLittleMods Nov 22, 2022
e257e9f
Merge branch 'develop' into madlittlemods/14108-optimize-filter_event…
MadLittleMods Nov 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions synapse/storage/databases/state/bg_updates.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,9 +101,11 @@ def _get_state_groups_from_groups_txn(
where_clause = " AND (%s)" % (where_clause,)

if isinstance(self.database_engine, PostgresEngine):
# Suspicion start
# Temporarily disable sequential scans in this transaction. This is
# a temporary hack until we can add the right indices in
txn.execute("SET LOCAL enable_seqscan=off")
# Suspicion end
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved

# The below query walks the state_group tree so that the "state"
# table includes all state_groups in the tree. It then joins
Expand Down
8 changes: 8 additions & 0 deletions synapse/storage/databases/state/store.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import attr

from synapse.api.constants import EventTypes
from synapse.logging.tracing import SynapseTags, set_tag, tag_args, trace
from synapse.storage._base import SQLBaseStore
from synapse.storage.database import (
DatabasePool,
Expand Down Expand Up @@ -158,6 +159,8 @@ def _get_state_group_delta_txn(txn: LoggingTransaction) -> _GetStateGroupDelta:
)

@cancellable
@trace
@tag_args
Comment on lines 173 to +175
Copy link
Contributor Author

@MadLittleMods MadLittleMods Nov 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should @cancellable be the innermost or outermost decorator?

I am guessing outermost but perhaps it doesn't matter

async def _get_state_groups_from_groups(
self, groups: List[int], state_filter: StateFilter
) -> Dict[int, StateMap[str]]:
Expand All @@ -171,6 +174,11 @@ async def _get_state_groups_from_groups(
Returns:
Dict of state group to state map.
"""
set_tag(
SynapseTags.FUNC_ARG_PREFIX + "groups.length",
str(len(groups)),
)

results: Dict[int, StateMap[str]] = {}

chunks = [groups[i : i + 100] for i in range(0, len(groups), 100)]
Expand Down
21 changes: 17 additions & 4 deletions synapse/visibility.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,13 @@
from synapse.events import EventBase
from synapse.events.snapshot import EventContext
from synapse.events.utils import prune_event
from synapse.logging.opentracing import trace
from synapse.logging.opentracing import (
start_active_span,
SynapseTags,
set_tag,
tag_args,
trace,
)
from synapse.storage.controllers import StorageControllers
from synapse.storage.databases.main import DataStore
from synapse.storage.state import StateFilter
Expand Down Expand Up @@ -53,6 +59,7 @@


@trace
@tag_args
async def filter_events_for_client(
storage: StorageControllers,
user_id: str,
Expand Down Expand Up @@ -82,6 +89,11 @@ async def filter_events_for_client(
Returns:
The filtered events.
"""
set_tag(
SynapseTags.FUNC_ARG_PREFIX + "events.length",
str(len(events)),
)

# Filter out events that have been soft failed so that we don't relay them
# to clients.
events_before_filtering = events
Expand Down Expand Up @@ -130,9 +142,10 @@ def allowed(event: EventBase) -> Optional[EventBase]:
sender_erased=erased_senders.get(event.sender, False),
)

MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor Author

@MadLittleMods MadLittleMods Nov 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does the state_groups table exist?

$ psql synapse
# \d+ state_groups
                                Table "public.state_groups"
  Column  |  Type  | Collation | Nullable | Default | Storage  | Stats target | Description
----------+--------+-----------+----------+---------+----------+--------------+-------------
 id       | bigint |           | not null |         | plain    |              |
 room_id  | text   |           | not null |         | extended |              |
 event_id | text   |           | not null |         | extended |              |

It seems like a subset of state_groups_state

$ psql synapse
# \d+ state_groups_state
                               Table "public.state_groups_state"
   Column    |  Type  | Collation | Nullable | Default | Storage  | Stats target | Description
-------------+--------+-----------+----------+---------+----------+--------------+-------------
 state_group | bigint |           | not null |         | plain    |              |
 room_id     | text   |           | not null |         | extended |              |
 type        | text   |           | not null |         | extended |              |
 state_key   | text   |           | not null |         | extended |              |
 event_id    | text   |           | not null |         | extended |              |

MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved
# Check each event: gives an iterable of None or (a potentially modified)
# EventBase.
filtered_events = map(allowed, events)
with start_active_span("filtering events against allowed function"):
# Check each event: gives an iterable of None or (a potentially modified)
# EventBase.
filtered_events = map(allowed, events)

# Turn it into a list and remove None entries before returning.
return [ev for ev in filtered_events if ev]
Expand Down