-
Notifications
You must be signed in to change notification settings - Fork 251
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
Reduce thread usage #330
base: master
Are you sure you want to change the base?
Reduce thread usage #330
Conversation
@@ -119,6 +122,8 @@ def run | |||
raise e | |||
end | |||
end | |||
|
|||
@task_scheduler.kill |
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.
is this kill
async or not?
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 believe it is async from the docs
Begin an immediate shutdown. In-progress tasks will be allowed to complete but enqueued tasks will be dismissed and no new tasks will be accepted. Has no additional effect if the thread pool is not running.
LGTM, can we add a test to verify the behavior when on_session_expired is actually invoked? I do not seem to find one in current rspec. |
I added those tests now @gmcatsf |
There are conflicts with current master please resolve them to have a clean CI. |
Summary
TimerSet
to execute scheduled tasks. This primarily allows for multiplexing of threads in order to avoid having many "polling" threads which are primarily idle.on_expired_session
handler for everyZookeeper(Poll)?Watcher
and instead, only use one per ZK client (which is shared). This requires a slightly different callback, but works the same.Note: I am not sure if this complexity is worth the gain. While this reduces the number of threads, the large portion of main memory usage comes from the additional dual-read watchers, which still exist. Perhaps just doing 1 is enough because it's simpler and less risky.
Testing
master
, whereas the other two are running this branch of Synapse.Misc
One clear benefit is that the CPU usage is reduced to somewhere between 33% and 50% of the original (again, purple is running
master
and other two are running this branch):Reviewers
@austin-zhu @gmcatsf @Jason-Jian