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

Avoid backfill when we already have messages to return #15737

Conversation

MadLittleMods
Copy link
Contributor

@MadLittleMods MadLittleMods commented Jun 7, 2023

Avoid backfill when we already have messages to return.

We now only block the client to backfill when we see a large gap in the events (more than 2 events missing in a row according to depth), more than 3 single-event holes, or not enough messages to fill the response. Otherwise, we return the messages directly to the client and backfill in the background for eventual consistency sake.

Fix #15696

Figure out if this change would skip backfill in a real-world scenario

For a real-world example with the trace linked from the issue (from viewing the Matrix Public Archive), it makes the following /messages request. If we run the JS snippet over the response, we can see that there are no gaps in response and we didn't really need to backfill at all. Which means this PR is a good change and would allow us not to block the client waiting for backfill which equates to 14s savings out of that 16s request!

GET https://matrix-client.matrix.org/_matrix/client/r0/rooms/!OGEhHVWSdvArJzumhm%3Amatrix.org/messages?dir=b&from=t535491-3610842373_0_0_0_0_0_0_0_0_0&limit=101&filter=%7B%22lazy_load_members%22%3Atrue%2C%22event_format%22%3A%22federation%22%7D

(() => {
    const rawEventDepthList = messagesResponse1.chunk.map((event) => {
        return event.depth;
    });
    // Unique sorted event depths
    const sortedEventDepths = Array.from(new Set(rawEventDepthList.sort()).values());

    let foundBigGap = false;
    let numberOfGaps = 0;
    let previousEventDepth = sortedEventDepths[0];
    for (const eventDepth of sortedEventDepths) {
        const depthGap = eventDepth - previousEventDepth;
        console.log('depthGap', depthGap);
        if (depthGap > 1) {
            numberOfGaps += 1;
        }

        if (depthGap > 2) {
            foundBigGap = true;
        }

        previousEventDepth = eventDepth;
    }

    console.log('numberOfGaps', numberOfGaps);
    console.log('foundBigGap', foundBigGap);
})();

Dev notes

Other PR where we did some backfill/processing in the background, #15585

MSC3871: Gappy timelines

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Pull request includes a sign off
  • Code style is correct
    (run the linters)

@MadLittleMods MadLittleMods added A-Federation A-Performance Performance, both client-facing and admin-facing T-Enhancement New features, changes in functionality, improvements in performance, or user-facing enhancements. A-Messages-Endpoint /messages client API endpoint (`RoomMessageListRestServlet`) (which also triggers /backfill) labels Jun 7, 2023
member_event_id
if leave_token.topological < curr_topo:
from_token = from_token.copy_and_replace(
StreamKeyType.ROOM, leave_token
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This logic is just pulled out from the existing condition to de-nest it further up here and have it ready when we initially fetch.

Comment on lines +597 to +598
# If we did backfill something, refetch the events from the database to
# catch anything new that might have been added since we last fetched.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the worst case now when we do need to backfill, it does mean that we call paginate_room_events() twice but I think the savings in the happy-path is well worth it.

Plus a slow backfill that is 10+ seconds for the federation request and processing doesn't hurt from an extra 250ms paginate_room_events() call again (it's not the slowest piece about this whole process to worry about here anyway).

@MadLittleMods MadLittleMods marked this pull request as ready for review June 9, 2023 21:29
@MadLittleMods MadLittleMods requested a review from a team as a code owner June 9, 2023 21:29
# We don't expect a negative depth but we'll just deal with it in
# any case by taking the absolute value to get the true gap between
# any two integers.
depth_gap = abs(event_depth - previous_event_depth)
Copy link
Member

Choose a reason for hiding this comment

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

Since the list is sorted won't this always be positive even if there are negative depths?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

ex. -5 - -6 = 1

🤷 Perhaps this gets across that we care about the absolute distance, not the difference in any case.

@MadLittleMods MadLittleMods merged commit 0757d59 into develop Jun 13, 2023
@MadLittleMods MadLittleMods deleted the madlittlemods/15696-avoid-backfill-if-we-already-have-enough-messages branch June 13, 2023 17:31
@MadLittleMods
Copy link
Contributor Author

Thanks for the review @erikjohnston 🐨

yingziwu added a commit to yingziwu/synapse that referenced this pull request Aug 21, 2023
Please note that this will be the last release of Synapse that is compatible with
Python 3.7 and earlier.
This is due to Python 3.7 now having reached End of Life; see our [deprecation policy](https://matrix-org.github.io/synapse/v1.87/deprecation_policy.html)
for more details.

- Pin `pydantic` to `^1.7.4` to avoid backwards-incompatible API changes from the 2.0.0 release.
  Resolves matrix-org#15858.
  Contributed by @PaarthShah. ([\matrix-org#15862](matrix-org#15862))

- Split out 2022 changes from the changelog so the rendered version in GitHub doesn't timeout as much. ([\matrix-org#15846](matrix-org#15846))

- Improve `/messages` response time by avoiding backfill when we already have messages to return. ([\matrix-org#15737](matrix-org#15737))
- Add spam checker module API for logins. ([\matrix-org#15838](matrix-org#15838))

- Fix a long-standing bug where media files were served in an unsafe manner. Contributed by @joshqou. ([\matrix-org#15680](matrix-org#15680))
- Avoid invalidating a cache that was just prefilled. ([\matrix-org#15758](matrix-org#15758))
- Fix requesting multiple keys at once over federation, related to [MSC3983](matrix-org/matrix-spec-proposals#3983). ([\matrix-org#15770](matrix-org#15770))
- Fix joining rooms through aliases where the alias server isn't a real homeserver. Contributed by @tulir @ Beeper. ([\matrix-org#15776](matrix-org#15776))
- Fix a bug in push rules handling leading to an invalid (per spec) `is_user_mention` rule sent to clients. Also fix wrong rule names for `is_user_mention` and `is_room_mention`. ([\matrix-org#15781](matrix-org#15781))
- Fix a bug introduced in 1.57.0 where the wrong table would be locked on updating database rows when using SQLite as the database backend. ([\matrix-org#15788](matrix-org#15788))
- Fix Sytest environmental variable evaluation in CI. ([\matrix-org#15804](matrix-org#15804))
- Fix forgotten rooms missing from initial sync after rejoining them. Contributed by Nico from Famedly. ([\matrix-org#15815](matrix-org#15815))
- Fix sqlite `user_filters` upgrade introduced in v1.86.0. ([\matrix-org#15817](matrix-org#15817))

- Document `looping_call()` functionality that will wait for the given function to finish before scheduling another. ([\matrix-org#15772](matrix-org#15772))
- Fix a typo in the [Admin API](https://matrix-org.github.io/synapse/latest/usage/administration/admin_api/index.html). ([\matrix-org#15805](matrix-org#15805))
- Fix typo in MSC number in faster remote room join architecture doc. ([\matrix-org#15812](matrix-org#15812))

- Remove experimental [MSC2716](matrix-org/matrix-spec-proposals#2716) implementation to incrementally import history into existing rooms. ([\matrix-org#15748](matrix-org#15748))

- Replace `EventContext` fields `prev_group` and `delta_ids` with field `state_group_deltas`. ([\matrix-org#15233](matrix-org#15233))
- Regularly try to send transactions to other servers after they failed instead of waiting for a new event to be available before trying. ([\matrix-org#15743](matrix-org#15743))
- Fix requesting multiple keys at once over federation, related to [MSC3983](matrix-org/matrix-spec-proposals#3983). ([\matrix-org#15755](matrix-org#15755))
- Allow for the configuration of max request retries and min/max retry delays in the matrix federation client. ([\matrix-org#15783](matrix-org#15783))
- Switch from `matrix://` to `matrix-federation://` scheme for internal Synapse routing of outbound federation traffic. ([\matrix-org#15806](matrix-org#15806))
- Fix harmless exceptions being printed when running the port DB script. ([\matrix-org#15814](matrix-org#15814))

* Bump attrs from 22.2.0 to 23.1.0. ([\matrix-org#15801](matrix-org#15801))
* Bump cryptography from 40.0.2 to 41.0.1. ([\matrix-org#15800](matrix-org#15800))
* Bump ijson from 3.2.0.post0 to 3.2.1. ([\matrix-org#15802](matrix-org#15802))
* Bump phonenumbers from 8.13.13 to 8.13.14. ([\matrix-org#15798](matrix-org#15798))
* Bump ruff from 0.0.265 to 0.0.272. ([\matrix-org#15799](matrix-org#15799))
* Bump ruff from 0.0.272 to 0.0.275. ([\matrix-org#15833](matrix-org#15833))
* Bump serde_json from 1.0.96 to 1.0.97. ([\matrix-org#15797](matrix-org#15797))
* Bump serde_json from 1.0.97 to 1.0.99. ([\matrix-org#15832](matrix-org#15832))
* Bump towncrier from 22.12.0 to 23.6.0. ([\matrix-org#15831](matrix-org#15831))
* Bump types-opentracing from 2.4.10.4 to 2.4.10.5. ([\matrix-org#15830](matrix-org#15830))
* Bump types-setuptools from 67.8.0.0 to 68.0.0.0. ([\matrix-org#15835](matrix-org#15835))
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Federation A-Messages-Endpoint /messages client API endpoint (`RoomMessageListRestServlet`) (which also triggers /backfill) A-Performance Performance, both client-facing and admin-facing T-Enhancement New features, changes in functionality, improvements in performance, or user-facing enhancements.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Avoid /backfill when we already have enough messages to return for a /messages response
2 participants