Fix performance regression related to delayed events processing #18926
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is preferable to pulling out entire events and only fetching the `sender` field.
We also only call it once for all events in the batch, instead of per state delta.
There's no need to do this for every event in the batch. Just find the min time for all delayed events.
There are zero delayed events on matrix.org's database. We can save a lot of time pulling out state deltas by first just checking if there are any delayed events to actually deal with.
anoadragon453
changed the title
|
Initial theory is that the |
anoadragon453
force-pushed
the
anoa/delayed_event_perf_regression
branch
from
13fd4bd to
4ff8184
Compare
This reverts commit 9f1c7e3. This change caused delayed events to not be scheduled, thus in turn causing the `TestDelayedEvents/delayed_.*_events_are_sent_on_timeout` tests to fail. I don't fully understand why my change didn't work, but have spent enough time looking at the code. The optimisation was very slight anyhow, so I don't think it's worth persuing for longer.
anoadragon453
force-pushed
the
anoa/delayed_event_perf_regression
branch
from
1500d26 to
1eb25f1
Compare
Apply the same fix to a similar loop in the presence handling code.
If a room has been purged, then the associated event would be gone as well. Thus we wouldn't be able to pull out the sender. This is different from the `sender` field being null on an event.
anoadragon453
commented
Sep 22, 2025
synapse/handlers/delayed_events.py
Outdated
Comment on lines
263
to
270
| if sender_str is sentinel: | ||
| # This can happen if a room is purged. State deltas related to | ||
| # the room are left behind, but the event/room no longer exist. | ||
| logger.warning( | ||
| "Skipping state delta with event ID '%s' as it is an unknown event - the room may have been purged", | ||
| delta.event_id, | ||
| ) | ||
| continue |
There was a problem hiding this comment.
This was added after an exception was found when running an earlier draft of this PR on element.io.
MadLittleMods
approved these changes
Sep 22, 2025
|
CI failures are due to flakes. |
3 tasks
anoadragon453
added a commit
that referenced
this pull request
Sep 24, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hopefully fixes #18925.
Carries out a number of optimisations to this bit of the codebase (note that on element.io we have about 7mil state deltas to process):
get_eventDB query per state delta with a single, smaller query for all state deltas.Set the next "scheduled at" time to the minimum of all delayed events once, instead of doing so for every delta.this ended up causing tests to fail, and has been reverted.Highly recommended to review commit-by-commit.
Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.