Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[V3] use uvloop if available #1935

Merged
merged 3 commits into from
Jul 25, 2018
Merged

[V3] use uvloop if available #1935

merged 3 commits into from
Jul 25, 2018

Conversation

mikeshardmind
Copy link
Contributor

Type

  • Bugfix
  • Enhancement
  • New feature

Description of the changes

uvloop is a significantly faster event loop, and we should be able to just use it where available. Being a drop in replacement, simply checking availability, and using it if is available seems safe.

Note:
I've been using this particular modification on my own bots without issue prior to making this PR

@mikeshardmind
Copy link
Contributor Author

Checks are failing on docs here for something not modified by this PR

https://travis-ci.org/Cog-Creators/Red-DiscordBot/jobs/403994298#L771

Warning, treated as error:
/home/travis/build/Cog-Creators/Red-DiscordBot/docs/install_windows.rst.rst:20:broken link: https://java.com/en/download/manual.jsp (403 Client Error: Forbidden for url: https://java.com/en/download/manual.jsp)

@Kowlin Kowlin added the V3 label Jul 21, 2018
@calebj
Copy link
Member

calebj commented Jul 24, 2018

I recommend only doing this automatically when the cython interpreter is detected, since uvloop is slow and sometimes problematic on pypy. Granted, pypy hasn't caught up with 3.6 yet, but it's only a matter of time.

@mikeshardmind
Copy link
Contributor Author

mikeshardmind commented Jul 24, 2018

Red doesn't run on pypy as you've already pointed out due to compatibility issues. I also don't feel this is a needed step as this will only work if the uvloop package is installed. If someone is specifically opting for pypy despite our docs making no mention of it, I generally assume they also have enough knowledge to not install uvloop on it, and uvloop is not being provided in any extras.

If even with this consideration, you still think this is an issue, I can add the check, but I'd prefer not to. There have been discussions of uvloop being a CFFI extension instead of C specifically for cases like pypy (while retaining the C one for CPython if it were faster) over at magicstack's issue tracker. While nothing has come of this yet, it would become one more thing for the development team to continue tracking to see if the check should be modified.

@calebj
Copy link
Member

calebj commented Jul 24, 2018

True. I've subbed the issue in question, but I'm still leaning towards putting in the check. My reasoning is that, because pypy shares the site package directories with cpython (on my system's build, anyway), there would be no way to exclude uvloop from being picked up if/when pypy 3.6 is released. It's better to err on the side of vanilla speed than to risk a potential slowdown--at least, until a CFFI implementation is released.

@mikeshardmind
Copy link
Contributor Author

Fair enough. I'll alter the PR to include this.

Michael H and others added 2 commits July 24, 2018 22:30
@Kowlin Kowlin merged commit 4b19421 into Cog-Creators:V3/develop Jul 25, 2018
@mikeshardmind mikeshardmind deleted the uvloop branch December 26, 2019 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants