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

Investigate if uvloop with asyncioreactor gives some speedboosts #10366

Open
ShadowJonathan opened this issue Jul 10, 2021 · 6 comments
Open
Labels
T-Enhancement New features, changes in functionality, improvements in performance, or user-facing enhancements.

Comments

@ShadowJonathan
Copy link
Contributor

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
Copy link
Member

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?

@anoadragon453 anoadragon453 added the T-Enhancement New features, changes in functionality, improvements in performance, or user-facing enhancements. label Jul 21, 2021
@ShadowJonathan
Copy link
Contributor Author

ShadowJonathan commented Jul 21, 2021

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
Copy link
Member

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

@ShadowJonathan
Copy link
Contributor Author

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
Copy link
Member

@ShadowJonathan is this no longer blocked?

@ShadowJonathan
Copy link
Contributor Author

ShadowJonathan commented Jul 30, 2021

#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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Enhancement New features, changes in functionality, improvements in performance, or user-facing enhancements.
Projects
None yet
Development

No branches or pull requests

2 participants