New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Sliding Sync: Speed up incremental sync by avoiding extra work #17665

Merged

erikjohnston merged 9 commits into develop from madlittlemods/sliding-sync-do-less-on-incremental-sync

Sep 9, 2024

Contributor

MadLittleMods commented Sep 4, 2024 •

edited

Loading

Speed up incremental sync by avoiding extra work. We first look at the state delta changes and only fetch and calculate further derived things if they have changed.

This isn't a very good trace comparison because these are just from some synthetic local tests that have too little data and run so fast that there is a lot of variation in the span durations. The main thing to notice is that the get_current_state_ids_at(...) chunk isn't happening the after trace.

Before	After

Trace before: aded9b5d1d517d83.json

Trace after: 48dee2667089b630.json

The real-world trace before (0672eb7bbbc4d50e.json) illustrates the problem a little better.

Dev notes

Enable Jaeger tracing in Twisted Trial tests:

Run Jaeger locally with Docker: https://www.jaegertracing.io/docs/1.6/getting-started/#all-in-one-docker-image

Add the following to tests/unittest.py in setUp(...):

        from synapse.logging.opentracing import init_tracer

        # Start the tracer
        init_tracer(self.hs)  # noqa

Add the following to tests/rest/client/sliding_sync/test_sliding_sync.py in default_config(...)

        config["opentracing"] = {
            "enabled": True,
            "jaeger_config": {
                "sampler": {"type": "const", "param": 1},
                "logging": False,
            },
        }

Add a sleep to the end of the test you're running so the traces have a chance to flow to Jaeger before the process exits.

import time
time.sleep(6)

Run your tests.

Visit http://localhost:16686/ and choose the test master service and find the traces you're interesting in.

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.
Code style is correct
(run the linters)

MadLittleMods added 6 commits

September 3, 2024 20:00


          Add test to make sure we're not sending room name if it didn't change

27ba276


          Re-arrange to do less on incremental sync


          Tweak test to test avatar

51b51d1


          Fix tests (also start of heroes tests)

562b486


          Add hero test

798aecf


          Avoid fetching more state than necessary

2de6f00

MadLittleMods added the A-Sync label

MadLittleMods added 2 commits

September 4, 2024 16:07


          Trace get_room_summary(...)

6b901a0


          Add changelog

c4530d3

MadLittleMods commented

View reviewed changes

synapse/handlers/sliding_sync/__init__.py

@@ @@ -744,26 +745,75 @@ async def get_room_sync_data( @@
                       # indicate to the client that a state reset happened. Perhaps we should indicate
                       # this by setting `initial: True` and empty `required_state`.
-                      # Check whether the room has a name set
-                      name_state_ids = await self.get_current_state_ids_at(

Contributor Author

MadLittleMods Sep 4, 2024 •

edited

Loading

We now get to avoid this costly get_current_state_ids_at(...) on incremental sync and instead look through the current state deltas to see if the room name changed (get_current_state_deltas_for_room(...)).

Since we need to get_current_state_deltas_for_room(...) anyway, doing it here first instead is just free lunch.

Before:

Screenshot 2024-09-04 at 4 04 34 PM

MadLittleMods commented

View reviewed changes

synapse/handlers/sliding_sync/__init__.py

Comment on lines +794 to +819

+                      room_membership_summary: Optional[Mapping[str, MemberSummary]] = None
                       empty_membership_summary = MemberSummary([], 0)
-                      if room_membership_for_user_at_to_token.membership in (
-                          Membership.LEAVE,
-                          Membership.BAN,
-                      ):
-                          # TODO: Figure out how to get the membership summary for left/banned rooms
-                          room_membership_summary = {}
-                      else:
-                          room_membership_summary = await self.store.get_room_summary(room_id)
-                          # TODO: Reverse/rewind back to the `to_token`
+                      # We need the room summary for:
+                      #  - Always for initial syncs (or the first time we send down the room)
+                      #  - When the room has no name, we need `heroes`
+                      #  - When the membership has changed so we need to give updated `heroes` and
+                      #    `joined_count`/`invited_count`.
+                      #
+                      # Ideally, instead of just looking at `name_changed`, we'd check if the room
+                      # name is not set but this is a good enough approximation that saves us from
+                      # having to pull out the full event. This just means, we're generating the
+                      # summary whenever the room name changes instead of only when it changes to
+                      # `None`.
+                      if initial or name_changed or membership_changed:
+                          # We can't trace the function directly because it's cached and the `@cached`
+                          # decorator doesn't mix with `@trace` yet.
+                          with start_active_span("get_room_summary"):
+                              if room_membership_for_user_at_to_token.membership in (
+                                  Membership.LEAVE,
+                                  Membership.BAN,
+                              ):
+                                  # TODO: Figure out how to get the membership summary for left/banned rooms
+                                  room_membership_summary = {}
+                              else:
+                                  room_membership_summary = await self.store.get_room_summary(room_id)
+                                  # TODO: Reverse/rewind back to the `to_token`

Contributor Author

MadLittleMods Sep 4, 2024

We now also conditionally generate the room_membership_summary based on when it's needed.

This wasn't being measured before in a trace but probably some savings since that was it's own database transaction involving multiple queries.

MadLittleMods commented

View reviewed changes

synapse/handlers/sliding_sync/__init__.py

                           events = await self.store.get_events(
-                              [d.event_id for d in deltas if d.event_id]
+                              state_filter.filter_state(room_state_delta_id_map).values()

Contributor Author

MadLittleMods Sep 4, 2024

Extra savings here because we now filter the room state to what we need before fetching the full events.

MadLittleMods commented

View reviewed changes

synapse/handlers/sliding_sync/__init__.py

Comment on lines +1107 to +1119

+                      joined_count: Optional[int] = None
+                      if initial or membership_changed:
+                          assert room_membership_summary is not None
+                          joined_count = room_membership_summary.get(
+                              Membership.JOIN, empty_membership_summary
+                          ).count
+                      invited_count: Optional[int] = None
+                      if initial or membership_changed:
+                          assert room_membership_summary is not None
+                          invited_count = room_membership_summary.get(
+                              Membership.INVITE, empty_membership_summary
+                          ).count

Contributor Author

MadLittleMods Sep 4, 2024

No need to return the joined_count/invited_count if they haven't changed

MadLittleMods commented

View reviewed changes

synapse/handlers/sliding_sync/__init__.py

+                              hero_membership_state = await self.get_current_state_at(
+                                  room_id=room_id,
+                                  room_membership_for_user_at_to_token=room_membership_for_user_at_to_token,
+                                  state_filter=StateFilter.from_types(hero_room_state),

Contributor Author

MadLittleMods Sep 4, 2024 •

edited

Loading

We could subtract the membership events we already have from the delta changes but this is bound to only be one event and makes the logic more complex. Most likely, since the heroes are the first of the memberships, the new membership won't be involved anyway.

MadLittleMods commented

View reviewed changes

synapse/handlers/sliding_sync/__init__.py

Comment on lines +960 to +963

+                      if initial or name_changed:
+                          meta_room_state.append((EventTypes.Name, ""))
+                      if initial or avatar_changed:
+                          meta_room_state.append((EventTypes.RoomAvatar, ""))

Contributor Author

MadLittleMods Sep 4, 2024 •

edited

Loading

We get to avoid fetching extra state if the name/avatar hasn't changed.

MadLittleMods commented

View reviewed changes

synapse/handlers/sliding_sync/__init__.py

Comment on lines +993 to +1001

+                          # heroes from the state
+                          if hero_user_ids:
+                              hero_membership_state = await self.get_current_state_at(
+                                  room_id=room_id,
+                                  room_membership_for_user_at_to_token=room_membership_for_user_at_to_token,
+                                  state_filter=StateFilter.from_types(hero_room_state),
+                                  to_token=to_token,
+                              )
+                              room_state.update(hero_membership_state)

Contributor Author

MadLittleMods Sep 4, 2024 •

edited

Loading

Previously, on incremental sync, heroes was bugged because it only showed the heroes that were in the membership changes.

Now we get the full hero list out but only when necessary.


          Merge branch 'develop' into madlittlemods/sliding-sync-do-less-on-inc…

40ca1e8

…remental-sync

MadLittleMods marked this pull request as ready for review

September 4, 2024 22:34

MadLittleMods requested a review from a team as a code owner

September 4, 2024 22:34

erikjohnston approved these changes

View reviewed changes

Member

erikjohnston left a comment

Looks great!

erikjohnston merged commit 5389374 into develop

39 checks passed

erikjohnston deleted the madlittlemods/sliding-sync-do-less-on-incremental-sync branch

September 9, 2024 09:36

Contributor Author

MadLittleMods commented Sep 9, 2024

Thanks for the review and merge @erikjohnston 🐦

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels