Skip to content

Global thread pool is now a thread pool executor. #42

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

Merged
merged 18 commits into from
Apr 2, 2014

Conversation

jdantonio
Copy link
Member

@mighe @chrisseaton @lucasallan

In this PR I've made the global thread pool a ThreadPoolExecutor instance. Before this commit it wasn't a real thread pool. It was a PerThreadExecutor (formerly NullThreadPool) which spawned a new thread for every request. I'd like to discuss if this is the best direction for this gem. There are two issues to discuss:

  1. Is this a reasonable configuration? The links below are some sites I found while randomly searching Google for "global thread pool configuration". They provided some guidance.
  2. The Java thread pool doesn't seem to shut down on exit. During experimentation I've had to explicitly shutdown the pool otherwise the program just hangs and I need to kill it. Is there a way to prevent this? If not, we may have to default to a Ruby thread pool even when running under JRuby.

I've added a demo program at demos/global_thread_pool-demo.rb that creates 500 Future objects each of which makes a call to a RESTful API. It outputs the time it takes to perform the task both sequentially and concurrently.

@jdantonio
Copy link
Member Author

Perhaps an at_exit hook can solve the JRuby issue. I'll have to experiment with that.

@jdantonio
Copy link
Member Author

This latest update seems to solve the shutdown problem I was experiencing on JRuby.

@mighe
Copy link
Contributor

mighe commented Apr 1, 2014

There's one thing about architecture I'd like to talk about: the global thread pool.
I think it is a perfect sensible default, but sometimes we would choose the executor; for example in an application we could have two different pools, one for long jobs and one for short jobs.
A the moment is not possible/easy saying something like operation.execute(executor)

Do you think this could be a good improvement?

@jdantonio
Copy link
Member Author

@mighe I think that's a great idea. I think it makes sense for some of the abstractions, but I think some would need the executor set on construction, not task execution. For Future I think it makes sense to pass an optional executor on execution, perhaps like this:

Concurrent::Future.execute(executor: executor) do
  # long running operation
end

Some of the other abstractions (like Agent and Async) have a strict order of operations. For those I think it makes more sense to set an optional executor on construction, perhaps like this:

agent = Concurrent::Agent.new(executor: executor)

If we take this approach then we can get rid of the UsesGlobalThreadPool module (which didn't work out as well as I had hoped it would).

For now I've added a second global thread pool. One for short-running tasks and another for long-running operations. And I've exposed those thread pools through module functions so they can be post to directly:

Concurrent::task do
  # short running task
end

Concurrent::operation do
  # long running operation
end

I've also updated Future with :task/:operation options which will allow any given Future to be post to the long-running operation global thread pool, like so:

short = Concurrent::Future.execute{ :foo }
short = Concurrent::Future.execute(task: true){ :foo }
short = Concurrent::Future.execute(operation: false){ :foo }

long = Concurrent::Future(task: false){ :bar }
long = Concurrent::Future(operation: true){ :bar }

This should allow for easy distinction between short-running and long-running tasks without the individual programmer needing to create their own thread pools.

Thoughts?

@jdantonio
Copy link
Member Author

@mighe The latest commit on this PR adds the :executor option to Future (which no longer includes the UsesGlobalThreadPool module). It does what I believe you are suggesting. This commit also updates Async to allow it to use an explicitly assigned executor.

Does this work for what you suggested?

@chrisseaton The latest commit makes a small change to Concurrent::dataflow to support the updated Future. Can you please take a look and let me know if this is a reasonable update?

@mighe
Copy link
Contributor

mighe commented Apr 2, 2014

Yes, this is exactly what I thought!
I like very much this architecture

jdantonio added a commit that referenced this pull request Apr 2, 2014
Global thread pool is now a thread pool executor.
@jdantonio jdantonio merged commit 297758e into master Apr 2, 2014
@jdantonio jdantonio deleted the global-thread-pool branch April 2, 2014 15:27
@jdantonio jdantonio mentioned this pull request Jun 18, 2014
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.

2 participants