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

Faster remote room joins (worker mode): do not populate external hosts-in-room cache when sending events as this requires blocking for full state. #14749

Merged
merged 4 commits into from
Jan 11, 2023
Merged

Faster remote room joins (worker mode): do not populate external hosts-in-room cache when sending events as this requires blocking for full state. #14749

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0fbc1aa
Don't block for full state to cache hosts in room when sending events
reivilibre Dec 29, 2022
4ca8e45
Newsfile
reivilibre Dec 29, 2022
a9f3e28
Drive-by hoisting
reivilibre Dec 29, 2022
1e4e430
fixup: reword comment
Jan 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/14749.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Faster remote room joins (worker mode): do not populate external hosts-in-room cache when sending events as this requires blocking for full state.
16 changes: 11 additions & 5 deletions synapse/handlers/message.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1531,12 +1531,18 @@ async def cache_joined_hosts_for_events(
external federation senders don't have to recalculate it themselves.
"""

for event, _ in events_and_context:
if not self._external_cache.is_enabled():
return
if not self._external_cache.is_enabled():
return

# If external cache is enabled we should always have this.
assert self._external_cache_joined_hosts_updates is not None

# If external cache is enabled we should always have this.
assert self._external_cache_joined_hosts_updates is not None
for event, event_context in events_and_context:
if event_context.partial_state:
# We can't precalculate joined hosts for a partial-state event,
# (at least not without blocking until full state).
# So skip this calculation entirely.
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 this is slightly confusing wording. You're right that we can't calculate the hosts from the state, but we do know the hosts we'll send the event to (albeit we'd have to query the DB) and so could add that to the external cache.

There's probably not much point doing so though.

Copy link
Contributor

Choose a reason for hiding this comment

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

How does the following sound?

-                 # We can't precalculate joined hosts for a partial-state event,
-                 # (at least not without blocking until full state).
-                 # So skip this calculation entirely.
+                # The federation senders don't use the external cache when sending
+                # events in partial-state rooms, so there's not much point in
+                # precalculating the hosts to send to.
+                #
+                # Note that the precalculation logic below will block for partial-state
+                # events and so can't be used for such events.

Copy link
Member

Choose a reason for hiding this comment

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

wfm!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fair — but note this cache is entirely about calculating hosts from the state, which as you say isn't necessary, but it is impossible in a partial-stated room without changing the definition of what we mean by what this cache stores. The distinction is not really noteworthy though; just skipping it ought to be fine.

Copy link
Contributor

@squahtx squahtx Jan 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@reivilibre I've reworded it again, to

-                # We can't precalculate joined hosts for a partial-state event,
-                # (at least not without blocking until full state).
-                # So skip this calculation entirely.
+                # To populate the cache for a partial-state event, we either have to
+                # block until full state, which the code below does, or change the
+                # meaning of cache values to be the list of hosts to which we plan to
+                # send events and calculate that instead.
+                #
+                # The federation senders don't use the external cache when sending
+                # events in partial-state rooms anyway, so let's not bother populating
+                # the cache.

Thoughts?

continue

# We actually store two mappings, event ID -> prev state group,
# state group -> joined hosts, which is much more space efficient
Expand Down