-
Notifications
You must be signed in to change notification settings - Fork 11
Optimizing the server into an async server model #68
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
base: development
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This kind of got badly mangled by a linter, I'm not seeing anything inherently wrong with it and the code seems to still work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Requests is a synchronous library, it usually is for scripting, not servers
As a whole, it slows things down a lot)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to clarify, my review wasn’t about switching requests -> httpx. I was only pointing out potential issues I noticed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you that's helpful still
|
I'm implementing Lazy Evaluation for now, but I'll try to come with something better for the users like a callback system eventually |
|
I added asyncio tasks to run things in "parallel" (still single-threaded) Basically that way, while one coroutine (basically a task for the program specified to do a single thing at that moment while letting other things proceeed then resume later to finish) for an user is waiting for something to complete, another can take the job, etc |
|
@spotlightishere whenever you have time it'll be really nice if you check this all up. I'm sure this PR could speed up the project a lot so this project could finally get a greater reputation and popularity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there, thank you so much for the changes :) I unfortunately don't have time to run this and verify functionality at the moment, but wanted to leave two passing comments so that it's easier to going forward.
Unfortunately, our main throttle right now is with the Pretendo network backend, as we have to individually add and remove all friends - bulk adding (syncing) friends like we do with Nintendo unfortunately causes their backend to struggle. Additionally, a lack of reactive updating for the Discord RPC half of things definitely causes a lot of issues :(
I really appreciate you taking the time to make these changes, thank you! Syncing the title DB is only done on server start, and any changes there would be a good step forward.
| import requests, io, math, json, os | ||
| import asyncio, io, math, json, os | ||
| from PIL import Image | ||
| from server import client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little wary of this change, as api (and thus love3) is indirectly required for things such as backend.py and discord.py.
It'd be nice to ideally have them all merged together, but in the meantime, would it be possible to pass client as a parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try this out tonight
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wholly agree with this change, but unfortunately converting from tabs to spaces is a lot of noise for this pull request! It'd be wonderful to make the entire codebase be PEP 8 compliant, however. Just for this PR, would it be possible to reformat with tabs again so that it's easier to review?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry! I'll try to fix that I might have accidentally converted tabs to space within pycharm
|
Considering this about pretendo, I'll make sure not to accidentally send too much requests especially if I wrote some in parallel ^^' For what is for 3ds-rpc server and all though, I'll make sure things dont get too heavy on my current laptop (8gb ram and 8 1ghz cores) to be sure it doesn't cause issues I was considering creating background threads though but that's probably not something I'll do right now |
You probably want to implement some kind of throttling still for pretendo, since their servers are easy for us to overwhelm and why pretendo is "intentionally" slow |
|
I don't believe any additional throttling changes would be necessary as we still individually add friends, etc outside of this code: Lines 120 to 143 in e8188ea
The overwhelming occurs when using |
True, I'm just worried about async deciding to run the func multiple times since its no longer syncronous |
So I noticed 3DS-RPC relies a LOT on synchronous libraries, so I took the time to implement some minimal changes for now, like replacing those
requestscalls to one of aHttpxclient.I wasn't able to setup the project on my environment (Fedora workstation, x86_64) so it might need some testings before to make sure things work fine, but it should definitely make a difference, hopefully even make a lot of problems the project currently has disappear. (Like on the endless issues: #67 #42 etc )
You're welcome!
Please contact me on Discord if you need any more help! I'll be very glad to optimize this project a lot further!
Also maybe I'd need some help setting up the project or at least next time as it was way too messy when I tried ^^"
Remember this needs testing!!
Wouldn't like for the users to encounter weird race conditions bugs too so it'd be preferable to make sure things work well before