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

Reduce thread usage #330

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open

Reduce thread usage #330

wants to merge 23 commits into from

Conversation

panchr
Copy link
Contributor

@panchr panchr commented Aug 14, 2020

Summary

  1. Remove extraneous sleep from ZookeeperWatcher thread
  2. Use TimerSet to execute scheduled tasks. This primarily allows for multiplexing of threads in order to avoid having many "polling" threads which are primarily idle.
  3. Remove on_expired_session handler for every Zookeeper(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

  • CI
  • Testing generated config in HAProxy
# haproxy.cfg contains the generated config from running on the `master` branch
# haproxy-refactored.cfg contains the generated config from this branch

$ egrep -o "server .* id" haproxy.cfg | sort > sorted-servers
$ egrep -o "server .* id" haproxy-refactored.cfg | sort > sorted-servers-refactored
$ diff -s sorted-servers sorted-servers-refactored                                                          
Files sorted-servers and sorted-servers-refactored are identical
  • Threads are reduced. Purple is a "control" host running master, whereas the other two are running this branch of Synapse.
    image

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):
image

Reviewers

@austin-zhu @gmcatsf @Jason-Jian

@panchr panchr changed the title [WIP] Reduce thread usage Reduce thread usage Aug 19, 2020
@panchr panchr marked this pull request as ready for review August 19, 2020 17:59
@panchr panchr requested a review from austin-zhu August 20, 2020 14:17
@@ -119,6 +122,8 @@ def run
raise e
end
end

@task_scheduler.kill
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@gmcatsf
Copy link
Contributor

gmcatsf commented Aug 21, 2020

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.

@panchr
Copy link
Contributor Author

panchr commented Aug 24, 2020

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

@gmcatsf
Copy link
Contributor

gmcatsf commented Sep 8, 2020

There are conflicts with current master please resolve them to have a clean CI.

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.

3 participants