Refactored thread pool

Refactored thread pool

The thread pool has been refactored to hold all implementation state
in the ThreadPool class and have all internal state protected by a
single monitor.

Added support for starting and stopping the same pool several
times. When stopping the queue can be drained or not.

Added two tests which handles messages.

This change is based on https://chromiumcodereview.appspot.com/9141005/.

R=iposva@google.com, ager@google.com

BUG=
TEST=

Committed: https://code.google.com/p/dart/source/detail?r=4181

[Mads Ager (google), 2012-01-24 12:29:38]

https://chromiumcodereview.appspot.com/9212043/diff/1/runtime/bin/thread_pool.cc
File runtime/bin/thread_pool.cc (right):

https://chromiumcodereview.appspot.com/9212043/diff/1/runtime/bin/thread_pool...
runtime/bin/thread_pool.cc:12: MonitorLocker monitor(&monitor_);
MonitorLocker locker(&monitor_); ?

Similarly for the other ones?

https://chromiumcodereview.appspot.com/9212043/diff/1/runtime/bin/thread_pool...
runtime/bin/thread_pool.cc:31: if (terminate_ && (!drain_ || result == NULL)) {
I don't understand this condition. Shouldn't this be

if (terminate_ && !drain_)

If you have drain_ set to true and you get a spurious wakeup from the monitor
you can get here with: terminate_ == true, drain_ == true and result == NULL. In
that case you will terminate without draining the queue?

https://chromiumcodereview.appspot.com/9212043/diff/1/runtime/bin/thread_pool.h
File runtime/bin/thread_pool.h (right):

https://chromiumcodereview.appspot.com/9212043/diff/1/runtime/bin/thread_pool...
runtime/bin/thread_pool.h:26: // Start the thread pool running.
Remove ' running'

https://chromiumcodereview.appspot.com/9212043/diff/1/runtime/bin/thread_pool...
runtime/bin/thread_pool.h:28: // Shutdown the thread pool. If drain is set to
true all tasks
I would put a blank line here.

https://chromiumcodereview.appspot.com/9212043/diff/1/runtime/bin/thread_pool...
runtime/bin/thread_pool.h:31: void Shutdown(bool drain = false);
How about an enum instead of the bool: kDrain, kDoNotDrain?

https://chromiumcodereview.appspot.com/9212043/diff/1/runtime/bin/thread_pool...
runtime/bin/thread_pool.h:66: // to head. New entried are inserted at the tail
and entries are
entries

https://chromiumcodereview.appspot.com/9212043/diff/1/runtime/bin/thread_pool...
File runtime/bin/thread_pool_test.cc (right):

https://chromiumcodereview.appspot.com/9212043/diff/1/runtime/bin/thread_pool...
runtime/bin/thread_pool_test.cc:22: void TestTaskHandler(ThreadPool::Task args)
{
Blank line before the function?

[Søren Gjesse, 2012-01-24 13:04:34]

https://chromiumcodereview.appspot.com/9212043/diff/1/runtime/bin/thread_pool.cc
File runtime/bin/thread_pool.cc (right):

https://chromiumcodereview.appspot.com/9212043/diff/1/runtime/bin/thread_pool...
runtime/bin/thread_pool.cc:12: MonitorLocker monitor(&monitor_);
On 2012/01/24 12:29:39, Mads Ager wrote:
> MonitorLocker locker(&monitor_); ?
> 
> Similarly for the other ones?

Done.

https://chromiumcodereview.appspot.com/9212043/diff/1/runtime/bin/thread_pool...
runtime/bin/thread_pool.cc:31: if (terminate_ && (!drain_ || result == NULL)) {
On 2012/01/24 12:29:39, Mads Ager wrote:
> I don't understand this condition. Shouldn't this be
> 
> if (terminate_ && !drain_)
> 
> If you have drain_ set to true and you get a spurious wakeup from the monitor
> you can get here with: terminate_ == true, drain_ == true and result == NULL.
In
> that case you will terminate without draining the queue?

Good catch! result would always be NULL here.

Moved the condition to be just after Wait an changed it to:

if (terminate_ && (drain_flag_ == kDoNotDrain || head_ == NULL))

This should mean "if terminating return NULL if either not draining or queue is
empty".

https://chromiumcodereview.appspot.com/9212043/diff/1/runtime/bin/thread_pool.h
File runtime/bin/thread_pool.h (right):

https://chromiumcodereview.appspot.com/9212043/diff/1/runtime/bin/thread_pool...
runtime/bin/thread_pool.h:26: // Start the thread pool running.
On 2012/01/24 12:29:39, Mads Ager wrote:
> Remove ' running'

Done.

https://chromiumcodereview.appspot.com/9212043/diff/1/runtime/bin/thread_pool...
runtime/bin/thread_pool.h:28: // Shutdown the thread pool. If drain is set to
true all tasks
On 2012/01/24 12:29:39, Mads Ager wrote:
> I would put a blank line here.

Done.

https://chromiumcodereview.appspot.com/9212043/diff/1/runtime/bin/thread_pool...
runtime/bin/thread_pool.h:31: void Shutdown(bool drain = false);
On 2012/01/24 12:29:39, Mads Ager wrote:
> How about an enum instead of the bool: kDrain, kDoNotDrain?

Done.

https://chromiumcodereview.appspot.com/9212043/diff/1/runtime/bin/thread_pool...
runtime/bin/thread_pool.h:66: // to head. New entried are inserted at the tail
and entries are
On 2012/01/24 12:29:39, Mads Ager wrote:
> entries

Done.

https://chromiumcodereview.appspot.com/9212043/diff/1/runtime/bin/thread_pool...
File runtime/bin/thread_pool_test.cc (right):

https://chromiumcodereview.appspot.com/9212043/diff/1/runtime/bin/thread_pool...
runtime/bin/thread_pool_test.cc:22: void TestTaskHandler(ThreadPool::Task args)
{
On 2012/01/24 12:29:39, Mads Ager wrote:
> Blank line before the function?

Done.

[Mads Ager (google), 2012-01-24 13:44:27]

lgtm

https://chromiumcodereview.appspot.com/9212043/diff/7/runtime/bin/thread_pool.h
File runtime/bin/thread_pool.h (right):

https://chromiumcodereview.appspot.com/9212043/diff/7/runtime/bin/thread_pool...
runtime/bin/thread_pool.h:34: // Shutdown the thread pool. If drain flags
specifies whether all
If drain flags -> The drain flag

https://chromiumcodereview.appspot.com/9212043/diff/7/runtime/bin/thread_pool...
runtime/bin/thread_pool.h:69: int size_;  // Current number of threads.
size_ -> number_of_threads_?

https://chromiumcodereview.appspot.com/9212043/diff/7/runtime/bin/thread_pool...
File runtime/bin/thread_pool_test.cc (right):

https://chromiumcodereview.appspot.com/9212043/diff/7/runtime/bin/thread_pool...
runtime/bin/thread_pool_test.cc:74: printf("%d %d\n", task_count, task_sum);
Remove debug printing?

[Søren Gjesse, 2012-01-24 14:38:50]

https://chromiumcodereview.appspot.com/9212043/diff/7/runtime/bin/thread_pool.h
File runtime/bin/thread_pool.h (right):

https://chromiumcodereview.appspot.com/9212043/diff/7/runtime/bin/thread_pool...
runtime/bin/thread_pool.h:34: // Shutdown the thread pool. If drain flags
specifies whether all
On 2012/01/24 13:44:27, Mads Ager wrote:
> If drain flags -> The drain flag

Done.

https://chromiumcodereview.appspot.com/9212043/diff/7/runtime/bin/thread_pool...
runtime/bin/thread_pool.h:69: int size_;  // Current number of threads.
On 2012/01/24 13:44:27, Mads Ager wrote:
> size_ -> number_of_threads_?

Done.

https://chromiumcodereview.appspot.com/9212043/diff/7/runtime/bin/thread_pool...
File runtime/bin/thread_pool_test.cc (right):

https://chromiumcodereview.appspot.com/9212043/diff/7/runtime/bin/thread_pool...
runtime/bin/thread_pool_test.cc:74: printf("%d %d\n", task_count, task_sum);
On 2012/01/24 13:44:27, Mads Ager wrote:
> Remove debug printing?

Done.