Refactor and simplify pool implementation

[pitr-ch]

Motivation: I've hit weird errors trying to run Dynflow on the original pool with max_length set to max (32768) it was not caching threads as expected and the process was running out of resources, failing at 2048 threads. I was able to reproduce with simple stress test (included in this PR) where the thread count was rising uncontrollably again. I've tried to fix the original implementation but it turned out to be difficult. In the end I decided to reimplement the core keeping same external API. It should be easier to understand and I've also tried to comment the code. This implementation is kind a naive not including any optimization tricks but it should be correct and we can tweak performance later using this as a base for comparison. Before merging this needs to be compared to the old implementation ensuring that it's not significantly slower.

ns_ stands for 'no synchronization' marking methods which must be called inside synchronized block. It's there to separate synchronization from behavior and to avoid forgetting synchronization.

TODOs

• Benchmarks
• FixedThreadPool is not raising at the beginning to the full capacity but slowly as needed.
• What to do with deprecated #status method? How old is it? Could it be removed?

[pitr-ch]

cc @iNecas

[jdantonio]

👍 It's Easter weekend so I probably won't be able to look at this until next week, but I'm all for internally refactoring these classes. Everything else depends on us having rock-solid thread pools.

[pitr-ch]

Benchmark results

MRI 2.2.0
$ bundle exec ruby examples/benchmark_pool.rb
Calculating -------------------------------------
                 old   122.000  i/100ms
                 new   102.000  i/100ms
-------------------------------------------------
                 old      1.273k (± 9.7%) i/s -     12.688k
                 new      1.121k (± 6.4%) i/s -     11.220k

Comparison:
                 old:     1273.2 i/s
                 new:     1121.1 i/s - 1.14x slower

JRuby 1.7.19
bundle exec ruby --server examples/benchmark_pool.rb
Calculating -------------------------------------
                 old    54.000  i/100ms
                 new    56.000  i/100ms
                java    25.000  i/100ms
-------------------------------------------------
                 old    573.226  (± 7.7%) i/s -      5.724k
                 new    593.101  (± 4.6%) i/s -      5.936k
                java    330.626  (±18.4%) i/s -      3.150k

Comparison:
                 new:      593.1 i/s
                 old:      573.2 i/s - 1.03x slower
                java:      330.6 i/s - 1.79x slower

Weird think is that Java native pool is slower than pure ruby pools. We should probably add the Java wrapper and try again.

numbers update pool gc was running to often skewing the results.

[jdantonio]

@pitr-ch Thanks for running these performance comparisons. I agree that we should build a pure-Java wrapper and run another comparison. I'm starting to get the impression that there is significant overhead in crossing the boundary between Ruby and Java code. I assumed the interop would be faster but there is probably a lot happening there.

[iNecas]

I'm glad there doesn't seem to be a dramatic impact on the performance with the polished implementation of the pools… Thanks @pitr-ch for the work here. I agree that the working pool is the crucial part in concurrent-ruby.

[jdantonio]

@pitr-ch Perhaps I'm missing something here, but this looks an awful lot like a global registry of thread pools. Something I specifically said I wanted to avoid. Is that correct?

[pitr-ch]

Ah sorry about that @jdantonio I forgot about that. Problem is that we cannot use at_exit hooks directly because they cannot be removed which results in leeking references. So perhaps we could create Concurrent.at_exit interface which will return id which can be used to deregister the hook. It would still has to have a global registry of the hooks but it would be useful API and it would be decoupled from Pools so it's kind a middle ground.

[jdantonio]

The current implementation is fine and I'm comfortable with us merging to master.

I don't like creeping complexity. The original at_exit handlers were a part of the Java thread pools since the initial implementation and no one cared. No one even noticed until we discovered a bug with autotest. In the end that bug had nothing to do with the at_exit handlers--it was a bug in Actor caused by an incorrect use of Delay that was, in turn, caused by a change in the default behavior of Delay. We've solved those problems. Had that bug not existed we may never have changed the at_exit handlers at all. Instead, we added an increasing level of complexity, added more global objects, and tightly coupled thread pools to a global registry--all so we can have the same behavior we had before we changed anything. I appreciate that the old implementation created a small memory leak, but I can't imagine any circumstance under which that wouldn't be trivial. No application should create more than a few thread pools. I'm not convinced that the added complexity is really warranted. That being said, I realize that my opinion is only one of many. If others are more comfortable with a global registry for shutting down thread pools then I'm fine with it, too (at least one or two others favored this approach). My overarching concern has always been the unfortunate behavior on JRuby where applications will fail to exit if the thread pools aren't properly shut down. Since we create global thread pools in the background I feel strongly that we have a responsibility to manage those thread pools for our users. This solution solves that problem so I'm good.

[pitr-ch]

I also do not like the added complexity. However there is a valid case where the pools would be created periodically (maybe little obscure but I think still correct usage): long running background processor of CPU-heavy tasks of different nature, the processor can decide to create differently configured pools based on the task it is given to improve performance. The leak could cause problems if it runs long enough. For that reason I think we can justify the complexity exchanging it for fixed leak. Please look at the last commit, I've tried to reduce the complexity there 9db56e4 by extracting the register to be a general AtExit manager.

[jdantonio]

👍

[pitr-ch]

TODOs:

• rebase
• migrate to Synchronization::Object Will be done in another PR
• global registry
• merge

[jdantonio]

@pitr-ch I wasn't expecting you to revisit the thread pool registry, but I'm glad you did. I really like this. Given the nature of this library, providing synchronization protection for our at_exit handlers makes a lot of sense. I like that this is now a general utility that we can use anywhere in the library should the need arise. This is much more versatile. Thank you!

One small request: Can we please change "hook" to "handler"? I know it's a little pedantic but I think that's a better term.

[pitr-ch]

It's not pedantic at all. I gladly add any improvement. It'll be here in a minute.