search

matrix-org / synapse

Synapse: Matrix homeserver written in Python/Twisted.
https://matrix-org.github.io/synapse
Apache License 2.0
11.81k stars 2.13k forks source link

Investigate if `uvloop` with `asyncioreactor` gives some speedboosts #10366

Open ShadowJonathan opened 3 years ago

ShadowJonathan commented 3 years ago

From the twisted 21.7.0.rc1 release:

twisted.internet.asyncioreactor.AsyncioSelectorReactor will no longer raise a TypeError like "SelectorEventLoop required, instead got: <uvloop.Loop ...>" (broken since 21.2.0).

uvloop is pretty fast, and twisted can utilize asyncio event loops with asyncioreactor, so i wonder if some speed boosts could be gained by switching to asyncioreactor + uvloop, even if optionally.

anoadragon453 commented 3 years ago

This is interesting, and compatible with Python 3.5+. A conditional would be necessary for the old versions of Twisted we support.

While useful, I don't know whether the event loop is currently Synapse's main bottleneck -- though I imagine it would have a bump in performance across the system.

@ShadowJonathan how difficult would it be to write a PoC to use this lib across Synapse so that we could get an idea of potential performance gains?

ShadowJonathan commented 3 years ago

I can write a branch that will utilize uvloop, but I have no instruments to test it with, is the intention that someone from EMS or y'all do some benchmarks regarding that? If so, I'll make something in a quick afternoon.

anoadragon453 commented 3 years ago

One solution is just to run it on a personal homeserver and see what breaks/if it produces a notable change in metrics.

ShadowJonathan commented 3 years ago

We did just that, got it to run on @anoadragon453's homeserver, and I'm thinking of making an PR where this is the default behaviour (after having discussed that a little).

The tests on anoa's homeserver returned inconclusive, with some slight positive variations, and I think testing on a larger homeserver is required before the gains really show themselves, thus the decision to PR this.

The branch we tested is based on #10402, thus things are blocked until that merges.

anoadragon453 commented 3 years ago

@ShadowJonathan is this no longer blocked?

ShadowJonathan commented 3 years ago

10446 and #10450 are merged now, so this is no longer blocked, no.

Once i have the time, i'll conjure a proper PR for this, though considering i'm on vacation that may take a second or two.