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
| @@ -114,9 +120,9 @@ def worker_task_completed | |||
| synchronize { @completed_task_count += 1 } | |||
| end | |||
|
|
|||
| # @!macro thread_pool_executor_method_prune_pool | |||
| def prune_pool | |||
There was a problem hiding this comment.
This public API no longer makes sense now that the pruning is automatic. It seems it was only added because of the Ruby thread pool's synchronous pruning, which the Java version didn't suffer from, hence why it's a no-op there.
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
2 times, most recently
from
57ae7ae to
9164d29
Compare
joshuay03
force-pushed
the
better-ruby-thread-pool-executor-pruning
branch
2 times, most recently
from
10f5833 to
76cd713
Compare
joshuay03
changed the title
joshuay03
force-pushed
the
better-ruby-thread-pool-executor-pruning
branch
13 times, most recently
from
547428b to
a675844
Compare
|
|
||
| if non_block | ||
| super(true) | ||
| elsif @mutex.synchronize { empty? && timed_out?(timeout) { @cond_var.wait(@mutex, timeout) } } |
There was a problem hiding this comment.
Just flagging that there could be a spurious wake up signal on the Condition Variable that prematurely invokes this condition. If that happened, I think you'd accidentally end up in a blocking pop.
I think the recommended design is to set an additional variable flag in addition to the signal (in push) to disambiguate between the meaningful signal and the spurious ones. The primitive for this is a Concurrent::Event.
There was a problem hiding this comment.
Ah good catch, I'll address, thanks!
There was a problem hiding this comment.
I think the recommended design is to set an additional variable flag in addition to the signal (in push) to disambiguate between the meaningful signal and the spurious ones. The primitive for this is a Concurrent::Event.
I think we can rely on #empty? instead? - it can only be true if there was a spurious wakeup.
There was a problem hiding this comment.
I've addressed, let me know what you think.
There was a problem hiding this comment.
I imagine that's sufficient 👍🏻 Thank you!
I think the only thing is there could be a race condition on empty? between a non-timeout pop and a timeout pop on the same Queue instance, because the non-timeout call to super is not synchronized with the same mutex. ...but we're building this for a very specific use case where that wouldn't happen.
There was a problem hiding this comment.
Good point! Although I think it’s best to ensure the implementation is robust in case it gets relied on more heavily in future work.
I've pushed a slightly different approach where we rely on the non-blocking super call instead.
joshuay03
force-pushed
the
better-ruby-thread-pool-executor-pruning
branch
6 times, most recently
from
1c208c9 to
fd02056
Compare
joshuay03
force-pushed
the
better-ruby-thread-pool-executor-pruning
branch
from
fd02056 to
6cedac3
Compare
Closes #1066
Closes #1075
Alternative to #1079
Implementation is based on the discussion in the linked issues.