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

Allow to disable both connection pooling and pruning idependently #327

Merged
merged 2 commits into from
Feb 3, 2016

Conversation

byroot
Copy link
Contributor

@byroot byroot commented Feb 3, 2016

Ref: #326

By profiling capistrano, it occurred that connection pruning was a significant performance hotspot.

See #326 (comment)

After investigation it's clear that the ConnectionPool put way to much zeal into pruning expired connection.
The prune_expired method that iterates over all the connections is called both on checkin and on checkout, so it's executed twice per host and per command.
e.g. if you execute 5 commands, on 10 servers, it will be executed 2 * 5 * 10, 100 times! Also note that it synchronize a mutex, so it's very probable that it kills limits the concurrency.

So much that 12% of the execution time (on a micro benchmark but still) was spent checking expirations.

I think that for many cases just checking is the connection is closed on checkout is enough.

I ran the same benchmark a saw some interesting performance improvements (even though it's always hard to be put an exact number because of the network).
capture d ecran 2016-02-03 a 00 12 37

NB: This is somewhat backwards incompatible. idle_timeout = 0 used to disable connection pooling entirely by always reaping connections, while now it actually let connections live indefinitely until they are closed.

If a version bump is not an option, I can make idle_timeout = 0 disable pooling as well, but then we need something like idle_timeout = false to disable connection pruning.

@byroot
Copy link
Contributor Author

byroot commented Feb 3, 2016

WALL profile of existing connections being used before the patch:

==================================
  Mode: wall(1000)
  Samples: 3860 (1.40% miss rate)
  GC: 442 (11.45%)
==================================
     TOTAL    (pct)     SAMPLES    (pct)     FRAME
       567  (14.7%)         567  (14.7%)     SSHKit::Backend::ConnectionPool::Entry#expired?
       347   (9.0%)         347   (9.0%)     Net::SSH::Compat.io_select
       341   (8.8%)         341   (8.8%)     block (2 levels) in Net::SSH::Connection::Channel#forward_local_env
       681  (17.6%)         106   (2.7%)     block (2 levels) in SSHKit::Backend::ConnectionPool#prune_expired
        90   (2.3%)          90   (2.3%)     Net::SSH::Transport::State#update_next_iv
        89   (2.3%)          89   (2.3%)     Net::SSH::Transport::HMAC::Abstract.digest_class

After the patch:

==================================
  Mode: wall(1000)
  Samples: 3033 (2.38% miss rate)
  GC: 359 (11.84%)
==================================
     TOTAL    (pct)     SAMPLES    (pct)     FRAME
       347  (11.4%)         347  (11.4%)     Net::SSH::Compat.io_select
       345  (11.4%)         345  (11.4%)     block (2 levels) in Net::SSH::Connection::Channel#forward_local_env
        98   (3.2%)          91   (3.0%)     SSHKit::Runner::Parallel#execute
        87   (2.9%)          87   (2.9%)     Net::SSH::Transport::HMAC::Abstract.mac_length
        86   (2.8%)          86   (2.8%)     Net::SSH::Transport::HMAC::Abstract.digest_class
        91   (3.0%)          75   (2.5%)     Net::SSH::Buffer#read
       119   (3.9%)          72   (2.4%)     Net::SSH::BufferedIo#fill
        70   (2.3%)          70   (2.3%)     Net::SSH::Transport::State#update_next_iv
       116   (3.8%)          62   (2.0%)     Net::SSH::BufferedIo#send_pending
        56   (1.8%)          56   (1.8%)     block in Net::SSH::Transport::PacketStream#enqueue_packet

So as expected the next bottleneck is IO.select, but it's already a big win.

@byroot
Copy link
Contributor Author

byroot commented Feb 3, 2016

I pushed a new commit using my SSHKit fix. Please re-review.

@kirs
Copy link
Member

kirs commented Feb 3, 2016

Looks good for me.

@leehambley @mattbrictson

@mattbrictson
Copy link
Member

This looks good to me as well.

@byroot Can you add a CHANGELOG entry explaining the "somewhat backwards incompatible" behavior so that users are not caught off guard?

Once that's in I would like to merge this.

Thanks!

@byroot
Copy link
Contributor Author

byroot commented Feb 3, 2016

Sure!

@byroot
Copy link
Contributor Author

byroot commented Feb 3, 2016

@mattbrictson done!

@mattbrictson
Copy link
Member

🎉

mattbrictson added a commit that referenced this pull request Feb 3, 2016
Allow to disable both connection pooling and pruning idependently
@mattbrictson mattbrictson merged commit 6de1e10 into capistrano:master Feb 3, 2016
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