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

Retry Experimental Federation Speedup #9908

Conversation

ShadowJonathan
Copy link
Contributor

@ShadowJonathan ShadowJonathan commented Apr 29, 2021

Fixes #9863

Re-introduces #9702, but adds parallelisation per #9863 (comment)

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.
  • Pull request includes a sign off
  • Code style is correct (run the linters)

Signed-off-by: Jonathan de Jong <[email protected]>

@ShadowJonathan
Copy link
Contributor Author

9bedd51 contains the actual difference from #9702

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

looks good. a couple of suggestions.

for event in events:
events_by_room.setdefault(event.room_id, []).append(event)

await make_deferred_yieldable(
# concurrently process room events, to allow parallelization through db queries
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# concurrently process room events, to allow parallelization through db queries
# concurrently process rooms, to allow parallelization through db queries

Comment on lines +374 to 386
nested_events_and_dests: List[
List[Tuple[EventBase, Collection[str]]]
] = await make_deferred_yieldable(
defer.gatherResults(
[
run_in_background(handle_room_events, evs)
run_in_background(
get_federatable_events_and_destinations, evs
)
for evs in events_by_room.values()
],
consumeErrors=True,
)
)
Copy link
Member

Choose a reason for hiding this comment

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

there's a concurrently_execute in synapse.util.async_helpers which takes care of the logcontext boilerplate whilst also applying a limit to the number of concurrent operations (which would probably be a good thing here). I think you may as well use it while you're changing things.

Comment on lines +369 to +391
events_by_room: Dict[str, List[EventBase]] = {}
for event in events:
events_by_room.setdefault(event.room_id, []).append(event)

await make_deferred_yieldable(
# concurrently process room events, to allow parallelization through db queries
nested_events_and_dests: List[
List[Tuple[EventBase, Collection[str]]]
] = await make_deferred_yieldable(
defer.gatherResults(
[
run_in_background(handle_room_events, evs)
run_in_background(
get_federatable_events_and_destinations, evs
)
for evs in events_by_room.values()
],
consumeErrors=True,
)
)

# flatten list
events_and_dests = list(
itertools.chain.from_iterable(nested_events_and_dests)
)
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 it would be worth moving all this into a function with input events and output events_and_dests, so as to limit the scope of the intermediate variables events_by_room and nested_events_and_dests.

@erikjohnston erikjohnston added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label May 20, 2021
@anoadragon453
Copy link
Member

Hi @ShadowJonathan, are you able to continue working on this? 🙂

@ShadowJonathan
Copy link
Contributor Author

ShadowJonathan commented Aug 18, 2021

I need to rebase and think about what these changes exactly entail, but i'll put it on draft until i got more time for it (i dont expect much in the upcoming months)

I still want to work on it, though, just not right now

@ShadowJonathan ShadowJonathan marked this pull request as draft August 18, 2021 10:51
@ShadowJonathan
Copy link
Contributor Author

Putting this in closed state after further discussion, ill reopen it once i have more time

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate regression in matrix.org's send PDU lag.
4 participants