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

Small perf improvement to limited incremental sync #17149

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

Conversation

erikjohnston
Copy link
Member

@erikjohnston erikjohnston commented May 3, 2024

The idea here is that we often end up fetching enough events from the DB to fill the gap, but we still end up with a gap due to the timeline limit. However, we can still apply the same optimisation as before to the list of fetched events, rather than just the truncated list we return.

@erikjohnston
Copy link
Member Author

cc @richvdh, as this is adding another optimisation on top of the sync state delta work you/we did

@erikjohnston erikjohnston marked this pull request as ready for review May 3, 2024 11:59
@erikjohnston erikjohnston requested a review from a team as a code owner May 3, 2024 11:59
Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

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

A few questions, but on the whole looks sound.

synapse/handlers/sync.py Show resolved Hide resolved
synapse/handlers/sync.py Outdated Show resolved Hide resolved
synapse/handlers/sync.py Outdated Show resolved Hide resolved
Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@erikjohnston
Copy link
Member Author

Thinking about it, I'm slightly worried that we don't have tests for this case. I'll need to have a think about how we go about doing that 🤔

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

Successfully merging this pull request may close these issues.

None yet

2 participants