Asynchronous pruning for RubyThreadPoolExecutor #1082
Conversation
joshuay03
force-pushed
the
better-ruby-thread-pool-executor-pruning
branch
15 times, most recently
from
b6e5656 to
f90d46d
Compare
joshuay03
force-pushed
the
better-ruby-thread-pool-executor-pruning
branch
14 times, most recently
from
c5eca7d to
89b67f4
Compare
joshuay03
force-pushed
the
better-ruby-thread-pool-executor-pruning
branch
7 times, most recently
from
fd02056 to
6cedac3
Compare
eregon
left a comment
eregon
left a comment
There was a problem hiding this comment.
Finally taking a look at this, sorry for the delay
lib/concurrent-ruby/concurrent/collection/ruby_timeout_queue.rb
Outdated
Show resolved
Hide resolved
lib/concurrent-ruby/concurrent/collection/ruby_timeout_queue.rb
Outdated
Show resolved
Hide resolved
joshuay03
force-pushed
the
better-ruby-thread-pool-executor-pruning
branch
4 times, most recently
from
defd0f7 to
9739e9a
Compare
lib/concurrent-ruby/concurrent/collection/ruby_timeout_queue.rb
Outdated
Show resolved
Hide resolved
joshuay03
force-pushed
the
better-ruby-thread-pool-executor-pruning
branch
from
9739e9a to
6aa4ee5
Compare
eregon
left a comment
eregon
left a comment
There was a problem hiding this comment.
The Queue stuff looks ready to me, i.e. I reviewed and approve that part.
The pool changes it's hard for me to tell because I haven't had time to understand the implementation in details (and likely won't have time for that soon).
It would be great if another concurrent-ruby maintainer could help review that (and they should feel free to merge this PR after they approve)
lib/concurrent-ruby/concurrent/executor/ruby_thread_pool_executor.rb
Outdated
Show resolved
Hide resolved
lib/concurrent-ruby/concurrent/executor/ruby_thread_pool_executor.rb
Outdated
Show resolved
Hide resolved
lib/concurrent-ruby/concurrent/executor/ruby_thread_pool_executor.rb
Outdated
Show resolved
Hide resolved
joshuay03
force-pushed
the
better-ruby-thread-pool-executor-pruning
branch
3 times, most recently
from
e4f5794 to
4d088a0
Compare
joshuay03
force-pushed
the
better-ruby-thread-pool-executor-pruning
branch
from
4d088a0 to
cb37798
Compare
|
@bensheldon Could you review this? |
|
@eregon yes, I will review. |
bensheldon
left a comment
bensheldon
left a comment
There was a problem hiding this comment.
I went through this and I think it looks good. I'm very excited about this new behavior 🙇🏻
- The Ruby TimeoutQueue implementation looks solid and resilient to idle wakeups and matches the interface of the native Ruby Queue-with-timeout.
- The pruning behavior in RubyThreadPoolExecutor looks correct. I spent a lot of time trying to understand the
prunablebehavior, but I think I get it now; I wasn't able to think up a scenario where it wouldn't result in unintended behavior. - Test cleanup looks defensible 👍🏻
| def prune_worker(worker) | ||
| synchronize do | ||
| if ns_prunable_capacity > 0 | ||
| remove_worker worker |
There was a problem hiding this comment.
I think synchronize should be safe to nest, but noting that remove_worker also calls synchronize. Noting this because I've reviewed this twice and each time I've wanted to trace what was happening, which makes me think that having an ns_remove_worker might be clearer, but I don't feel strongly about it.
There was a problem hiding this comment.
Yes, MutexLockableObject#synchronize has been implemented to be re-entrant within the same thread:
I decided to reuse remove_worker because of this re-entrancy guarantee, since it's also called independently in the thread loop. I think I'll keep it this way.
|
@joshuay03 I'll let you merge this yourself now that you're a maintainer of concurrent-ruby :) |
|
Thank you both for the reviews! |
3 participants
Closes #1066
Closes #1075
Alternative to #1079
Implementation is based on the discussion in the linked issues.