Skip to content
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.

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

Sliding Sync: Lazy-loading room members on incremental sync (remember memberships) #17809

Open
wants to merge 31 commits into
base: develop
Choose a base branch
from

Conversation

MadLittleMods
Copy link
Contributor

@MadLittleMods MadLittleMods commented Oct 9, 2024

Lazy-loading room members on incremental sync and remember which memberships we've sent down the connection before (up-to 100)

Fix #17804

Follow-up/alternative to #17806

Depends on #17785

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)

# sent it before and send the new state. (if we were tracking
# that we sent any other state, we should still keep track
# that).
{},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's possible that we could remember the specific state_keys that we have sent down before but this currently just acts the same as if a whole type was removed (same as simple_remove_type)

This is just a performance optimization though and the result would look like following:

Suggested change
{},
{
EventTypes.Member: {
"@user3:test",
}
},

Perhaps it's good that we "garbage collect" and forget what we've sent before for a given type when the client stops caring about a certain type 🤷.

Copy link
Member

@erikjohnston erikjohnston left a comment

Choose a reason for hiding this comment

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

I think this makes sense! I am slightly worried about the increase in the size of the tables if we keep adding more and more members in there, but I'll have a little think about how we might make that more efficient.

synapse/handlers/sliding_sync/__init__.py Show resolved Hide resolved
…c-lazy-load-members-on-incrental-sync3

Conflicts:
	synapse/handlers/sliding_sync/__init__.py
Base automatically changed from erikj/ss_required_state to develop October 14, 2024 12:31
…ers-on-incrental-sync3

Conflicts:
	synapse/handlers/sliding_sync/__init__.py
	tests/handlers/test_sliding_sync.py
@@ -0,0 +1 @@
Fix bug with sliding sync where `$LAZY`-loading room members would not return `required_state` membership in incremental syncs.
Copy link
Member

Choose a reason for hiding this comment

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

I am slightly worried about the increase in the size of the tables if we keep adding more and more members in there, but I'll have a little think about how we might make that more efficient.

More concretely: there's a few concerns I have:

  1. Over the lifetime of a connection the size of the store required state could grow very large (think Matrix HQ).
  2. Currently, we pull out everything when we get a request, so the size of the data matters
  3. Every time we change the state we end up copying all the rows from the previous connection position to the new position, so the amount of data again matters.

On point 3. I think we can change the DB tables up a bit so that we have one table that holds the base values that apply to all live positions in the connection (there will be max two), and then another table that are the delta from the base to that position. This means that we'd only need to copy over the deltas to the base and insert new deltas whenever we persist a new position (which should be a lot less data).

I don't really know what we do about not pulling lots of data from the DB. We could only pull out that data for the rooms we're sending down? Or maybe we we don't pull it out and instead query the DB for "which of these users have we previously sent membership down for?", though that feels like a bigger change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could only pull out that data for the rooms we're sending down?

This would probably be good to do in any case.

But it seems we're also concerned with how big an individual (room_id, user_id) could get.

I don't really know what we do about not pulling lots of data from the DB. We could only pull out that data for the rooms we're sending down? Or maybe we we don't pull it out and instead query the DB for "which of these users have we previously sent membership down for?", though that feels like a bigger change.

Are we set on wanting to track it? The dumb simple solution is to not track it at all which means returning a membership event for any timeline events.

Another alternative is that we could throw away a type when the state_key's grow too large. Or just throw the whole required_state_map when it grows too large. When I say throw-away, I just mean reset to whatever the requested required_state_map has. This is a decent middle ground between tracking nothing and everything. Ideally, we could kick out entries by recency but the complexity is probably not needed. It also doesn't require us to change our database schema.

Copy link
Member

Choose a reason for hiding this comment

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

I think we do want to track the sent memberships, otherwise you are often just doubling the number of events you have to pull out and send to the clients.

Another alternative is that we could throw away a type when the state_key's grow too large. Or just throw the whole required_state_map when it grows too large. When I say throw-away, I just mean reset to whatever the requested required_state_map has. This is a decent middle ground between tracking nothing and everything. Ideally, we could kick out entries by recency but the complexity is probably not needed. It also doesn't require us to change our database schema.

This is probably a decent idea that should be fairly easy to do? Even something like "if we have $LAZY and the set of members we've sent down is bigger than 100, reset", or something. It's not pretty but a) means for most rooms we'll never reset, and b) most of the time we won't send down redundant memberships.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to limit the state_keys we remember from previous requests to 100 for any type (also added tests).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sliding sync does not correctly do lazy loaded members for incremental sync
2 participants