element-hq / element-hq/synapse

Synapse synchrotron performance for timeline entries can be improved

Open
#13,327 0 comments 0 reactions 0 assignees View on GitHub
A-Performance A-Sync T-Enhancement
Dominant language
Python
Stars
4.6k
Forks
600
Avg merge
5d 22h
Merged PRs (30d)
51

Description

This issue has been migrated from [#13327](https://github.com/matrix-org/synapse/issues/13327).

---

When Synapse works out a /sync response, it should:
- Use a more conservative [load limit](https://github.com/matrix-org/synapse/blob/5099b5ecc735b98ac9d559ef6191554bafff964b/synapse/handlers/sync.py#L540) when the timeline limit is set to low numbers like 1. Otherwise, Synapse will load up way more events than it needs to (10 instead of 1), which then has knock-on implications as each event then has state-before-the-event fetched to do history visibility checks. Ideally this would be set to the `timeline_limit`, but Erik has mentioned good reasons why maybe it shouldn't be:
> I think all of that code is to make sure we correctly handle the case where we filter out events that we received from the DB, so we always load at least 10 to make sure we don't pointlessly pull out things one by one. We can filter events out for other reasons too, e.g. they don't have permission to see the message (as it was before they'd joined or whatever).

Perhaps as a compromise the `load_limit` could eagerly try being the `timeline_limit` and then progressively increase on repeated queries if it cannot be satisfied. This means in the 'happy path' we can cut down on pulling out literally an order of magnitude more events (when timeline limit is 1).

- Bail early when the timeline events have been satisfied. `filter_events_for_client` is expensive as it loads the state-before-the-event, and it's called for every [loaded event](https://github.com/matrix-org/synapse/blob/5099b5ecc735b98ac9d559ef6191554bafff964b/synapse/handlers/sync.py#L588) (which is compounded when this includes events the client didn't ask for). This hard work is then [discarded](https://github.com/matrix-org/synapse/blob/5099b5ecc735b98ac9d559ef6191554bafff964b/synapse/handlers/sync.py#L607) - instead it would be good if `filter_events_for_client` bailed early the moment it had the required number of events. This could just be an optional kwarg on the function to bail once it has a certain number of events?

### Background

I have been profiling sync v2 performance in an effort to try to identify areas where performance can be improved, mostly as a stop-gap until sliding sync can land. The key part to mention is this image:

Screenshot 2022-07-18 at 17 49 33

The red lines are the time spent to calculate the `timeline` section of the response, specifically [this code](https://github.com/matrix-org/synapse/blob/5099b5ecc735b98ac9d559ef6191554bafff964b/synapse/handlers/sync.py#L1999-L2018). In an effort to reduce this time, clients can send up a `/sync` request with a timeline limit of 1. However, this doesn't result in much improvement because of the two points mentioned above. If those issues were resolved, setting a low timeline limit could significantly speed up sync v2.

Contributor guide

Open the contributing guide

Research direction

Start in synapse/handlers/sync.py at the load-limit logic around the /sync timeline handling and the timeline response code referenced in the issue. Examine filter_events_for_client and its use for loaded events; done means low timeline limits avoid unnecessary event and state-before-event work while still returning the requested visible timeline events correctly.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
api, backend, performance
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
42/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.