Change the thread interface in runtime/platform and use it starting all threads

Change the thread interface in runtime/platform and use it starting all threads

The platform thread interface (dart::thread) is now refactored to an
all static interface as suggested by iposva@ and asiva@. Use this
interface for running all threads in the VM.

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

BUG=
TEST=

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

[Mads Ager (google), 2012-01-20 12:35:34]

http://codereview.chromium.org/9141005/diff/2002/runtime/bin/thread_pool.cc
File runtime/bin/thread_pool.cc (right):

http://codereview.chromium.org/9141005/diff/2002/runtime/bin/thread_pool.cc#n...
runtime/bin/thread_pool.cc:69: calloc(size_, sizeof(dart::ThreadHandle)));
I think we usually use 4-space indent we we have to move arguments to a separate
line?

http://codereview.chromium.org/9141005/diff/2002/runtime/bin/thread_pool.h
File runtime/bin/thread_pool.h (right):

http://codereview.chromium.org/9141005/diff/2002/runtime/bin/thread_pool.h#ne...
runtime/bin/thread_pool.h:68: static void Main(uword);
uword args?

http://codereview.chromium.org/9141005/diff/2002/runtime/platform/thread.h
File runtime/platform/thread.h (right):

http://codereview.chromium.org/9141005/diff/2002/runtime/platform/thread.h#ne...
runtime/platform/thread.h:33: static bool Join(ThreadHandle thread, int64_t
millis = kNoTimeout);
Do you need the timeout version of this. I don't see anything in this change
that needs it. Since it is unimplemented on Mac in this change I think we should
leave it out.

http://codereview.chromium.org/9141005/diff/2002/runtime/platform/thread_linu...
File runtime/platform/thread_linux.cc (right):

http://codereview.chromium.org/9141005/diff/2002/runtime/platform/thread_linu...
runtime/platform/thread_linux.cc:51: reinterpret_cast<void*
(*)(void*)>(function),  // NOLINT
Can we typedef the 'void* (*)(void*)' function type and do the casting on the
line before? I find this hard to read.

http://codereview.chromium.org/9141005/diff/2002/runtime/platform/thread_maco...
File runtime/platform/thread_macos.cc (right):

http://codereview.chromium.org/9141005/diff/2002/runtime/platform/thread_maco...
runtime/platform/thread_macos.cc:35: reinterpret_cast<void*
(*)(void*)>(function),  // NOLINT
Ditto.

[Søren Gjesse, 2012-01-20 13:25:45]

http://codereview.chromium.org/9141005/diff/2002/runtime/bin/thread_pool.cc
File runtime/bin/thread_pool.cc (right):

http://codereview.chromium.org/9141005/diff/2002/runtime/bin/thread_pool.cc#n...
runtime/bin/thread_pool.cc:69: calloc(size_, sizeof(dart::ThreadHandle)));
On 2012/01/20 12:35:34, Mads Ager wrote:
> I think we usually use 4-space indent we we have to move arguments to a
separate
> line?

Done.

http://codereview.chromium.org/9141005/diff/2002/runtime/bin/thread_pool.h
File runtime/bin/thread_pool.h (right):

http://codereview.chromium.org/9141005/diff/2002/runtime/bin/thread_pool.h#ne...
runtime/bin/thread_pool.h:68: static void Main(uword);
On 2012/01/20 12:35:34, Mads Ager wrote:
> uword args?

Done.

http://codereview.chromium.org/9141005/diff/2002/runtime/platform/thread.h
File runtime/platform/thread.h (right):

http://codereview.chromium.org/9141005/diff/2002/runtime/platform/thread.h#ne...
runtime/platform/thread.h:33: static bool Join(ThreadHandle thread, int64_t
millis = kNoTimeout);
On 2012/01/20 12:35:34, Mads Ager wrote:
> Do you need the timeout version of this. I don't see anything in this change
> that needs it. Since it is unimplemented on Mac in this change I think we
should
> leave it out.

Good point, removed.

http://codereview.chromium.org/9141005/diff/2002/runtime/platform/thread_linu...
File runtime/platform/thread_linux.cc (right):

http://codereview.chromium.org/9141005/diff/2002/runtime/platform/thread_linu...
runtime/platform/thread_linux.cc:51: reinterpret_cast<void*
(*)(void*)>(function),  // NOLINT
On 2012/01/20 12:35:34, Mads Ager wrote:
> Can we typedef the 'void* (*)(void*)' function type and do the casting on the
> line before? I find this hard to read.

typedefed as PThreadStartFunction (in Mac OS file as well).

http://codereview.chromium.org/9141005/diff/2002/runtime/platform/thread_maco...
File runtime/platform/thread_macos.cc (right):

http://codereview.chromium.org/9141005/diff/2002/runtime/platform/thread_maco...
runtime/platform/thread_macos.cc:35: reinterpret_cast<void*
(*)(void*)>(function),  // NOLINT
On 2012/01/20 12:35:34, Mads Ager wrote:
> Ditto.

Done.

[Ivan Posva, 2012-01-20 16:45:55]

https://chromiumcodereview.appspot.com/9141005/diff/5027/runtime/bin/thread_p...
File runtime/bin/thread_pool.cc (right):

https://chromiumcodereview.appspot.com/9141005/diff/5027/runtime/bin/thread_p...
runtime/bin/thread_pool.cc:83: dart::Thread::Join(threads_[i]);
This will not work, please see comment in thread.h. Instead you need to wait
until the count of active threads reaches 0.

https://chromiumcodereview.appspot.com/9141005/diff/5027/runtime/platform/thr...
File runtime/platform/thread.h (right):

https://chromiumcodereview.appspot.com/9141005/diff/5027/runtime/platform/thr...
runtime/platform/thread.h:33: static void Join(ThreadHandle thread);
Please remove the Join functionality it is not really useful. That will also
remove the need for defining and returning a ThreadHandle.

https://chromiumcodereview.appspot.com/9141005/diff/5027/runtime/platform/thr...
File runtime/platform/thread_linux.cc (left):

https://chromiumcodereview.appspot.com/9141005/diff/5027/runtime/platform/thr...
runtime/platform/thread_linux.cc:58: static void* ThreadStart(void* data_ptr) {
In the past I have found this extra trampoline helpful to provide a hook for
extra logging, both when starting or exiting a thread.

https://chromiumcodereview.appspot.com/9141005/diff/5027/runtime/platform/thr...
File runtime/platform/thread_macos.cc (right):

https://chromiumcodereview.appspot.com/9141005/diff/5027/runtime/platform/thr...
runtime/platform/thread_macos.cc:50: int result = pthread_join(thread, NULL);
The threads are created detached (as they should be), so this function will
always fail with EINVAL.

[Søren Gjesse, 2012-01-23 13:14:57]

I am not sure why you don't like Thread::Join.

However I have now two different Patch Sets:

In Patch Set #6 I added the flag kJoinable to specify whether a thread should be
detached or not.

In Patch Set #7 I have removed Thread::Join altogether and is using a Monitor
together with some state to wait for all threads in the thread pool to stop.

https://chromiumcodereview.appspot.com/9141005/diff/5027/runtime/bin/thread_p...
File runtime/bin/thread_pool.cc (right):

https://chromiumcodereview.appspot.com/9141005/diff/5027/runtime/bin/thread_p...
runtime/bin/thread_pool.cc:83: dart::Thread::Join(threads_[i]);
On 2012/01/20 16:45:55, Ivan Posva wrote:
> This will not work, please see comment in thread.h. Instead you need to wait
> until the count of active threads reaches 0.

See reply in thread.h

https://chromiumcodereview.appspot.com/9141005/diff/5027/runtime/platform/thr...
File runtime/platform/thread.h (right):

https://chromiumcodereview.appspot.com/9141005/diff/5027/runtime/platform/thr...
runtime/platform/thread.h:33: static void Join(ThreadHandle thread);
On 2012/01/20 16:45:55, Ivan Posva wrote:
> Please remove the Join functionality it is not really useful. That will also
> remove the need for defining and returning a ThreadHandle.

Why is Join not useful? Especially for testing I have found that if a test uses
a number of threads to do something then just joining with all the threads on
the main thread works nicely. Without Join some other synchronization mechanism
will be needed for that.

I missed that the pthreads created by to VM where detached by default whereas
the ones used by the eventhandler and thread pool here joinable (not using this
shared code before).

I have added a flag to Start for specified joinable - that might be an option.

https://chromiumcodereview.appspot.com/9141005/diff/5027/runtime/platform/thr...
File runtime/platform/thread_linux.cc (left):

https://chromiumcodereview.appspot.com/9141005/diff/5027/runtime/platform/thr...
runtime/platform/thread_linux.cc:58: static void* ThreadStart(void* data_ptr) {
On 2012/01/20 16:45:55, Ivan Posva wrote:
> In the past I have found this extra trampoline helpful to provide a hook for
> extra logging, both when starting or exiting a thread.

OK, I added it back. It was also still there on Windows to be able to close the
thread handle.

https://chromiumcodereview.appspot.com/9141005/diff/5027/runtime/platform/thr...
File runtime/platform/thread_macos.cc (right):

https://chromiumcodereview.appspot.com/9141005/diff/5027/runtime/platform/thr...
runtime/platform/thread_macos.cc:50: int result = pthread_join(thread, NULL);
On 2012/01/20 16:45:55, Ivan Posva wrote:
> The threads are created detached (as they should be), so this function will
> always fail with EINVAL.

Obviously I did not run the VM tests.

How about the joinable flag when starting a thread?

[Ivan Posva, 2012-01-24 02:04:01]

Pulling the implementation of the TaskQueue into thread_pool.cc can be done in a
different CL, but the locks should be cleaned up.

-Ivan

http://codereview.chromium.org/9141005/diff/12001/runtime/bin/thread_pool.cc
File runtime/bin/thread_pool.cc (right):

http://codereview.chromium.org/9141005/diff/12001/runtime/bin/thread_pool.cc#...
runtime/bin/thread_pool.cc:76: terminate_ = true;
Please do not access shared state outside the lock protecting it. The lock
protecting this should be the lock protecting access to the queue. In essence
you need to protections: one for the size of the thread pool and one for the
queue and termination.

http://codereview.chromium.org/9141005/diff/12001/runtime/bin/thread_pool.cc#...
runtime/bin/thread_pool.cc:78: MonitorLocker monitor(&monitor_);
The TaskQueue should be an implementation detail of ThreadPool and not be
exposed.

http://codereview.chromium.org/9141005/diff/12001/runtime/platform/thread_win.cc
File runtime/platform/thread_win.cc (right):

http://codereview.chromium.org/9141005/diff/12001/runtime/platform/thread_win...
runtime/platform/thread_win.cc:17: bool joinable)
joinable is a thing of the past.

[siva, 2012-01-24 02:26:05]

DBC

http://codereview.chromium.org/9141005/diff/12001/runtime/bin/thread_pool.cc
File runtime/bin/thread_pool.cc (right):

http://codereview.chromium.org/9141005/diff/12001/runtime/bin/thread_pool.cc#...
runtime/bin/thread_pool.cc:70: reinterpret_cast<uword>(this));
If you choose to have an error return value for Thread::Start you should decide
here whether you want to abort the start and shutdown the threadpool or continue
with whatever number of threads we created (e.g we wanted to create a thread
pool with 100 threads but we are only able to create 70)

http://codereview.chromium.org/9141005/diff/12001/runtime/bin/thread_pool.cc#...
runtime/bin/thread_pool.cc:102: if (pool->terminate_) continue;
Why continue and not a break?

http://codereview.chromium.org/9141005/diff/12001/runtime/platform/thread.h
File runtime/platform/thread.h (right):

http://codereview.chromium.org/9141005/diff/12001/runtime/platform/thread.h#n...
runtime/platform/thread.h:28: static void Start(ThreadStartFunction function,
uword parameters);
Now that you are using threads formally in a thread pool and a server process
setting does it make sense to return an error from this function instead of the
Fatal death on error?

[Søren Gjesse, 2012-01-24 12:07:55]

https://chromiumcodereview.appspot.com/9141005/diff/12001/runtime/bin/thread_...
File runtime/bin/thread_pool.cc (right):

https://chromiumcodereview.appspot.com/9141005/diff/12001/runtime/bin/thread_...
runtime/bin/thread_pool.cc:70: reinterpret_cast<uword>(this));
On 2012/01/24 02:26:05, asiva wrote:
> If you choose to have an error return value for Thread::Start you should
decide
> here whether you want to abort the start and shutdown the threadpool or
continue
> with whatever number of threads we created (e.g we wanted to create a thread
> pool with 100 threads but we are only able to create 70)

Added return code to Thread::Start. For now just abort here on failure.

https://chromiumcodereview.appspot.com/9141005/diff/12001/runtime/bin/thread_...
runtime/bin/thread_pool.cc:76: terminate_ = true;
On 2012/01/24 02:04:01, Ivan Posva wrote:
> Please do not access shared state outside the lock protecting it. The lock
> protecting this should be the lock protecting access to the queue. In essence
> you need to protections: one for the size of the thread pool and one for the
> queue and termination.

Agree, moved thread_pool cleanup to separate CL,
https://chromiumcodereview.appspot.com/9212043.

https://chromiumcodereview.appspot.com/9141005/diff/12001/runtime/bin/thread_...
runtime/bin/thread_pool.cc:78: MonitorLocker monitor(&monitor_);
On 2012/01/24 02:04:01, Ivan Posva wrote:
> The TaskQueue should be an implementation detail of ThreadPool and not be
> exposed.

Agree, moved thread_pool cleanup to separate CL,
https://chromiumcodereview.appspot.com/9212043.

https://chromiumcodereview.appspot.com/9141005/diff/12001/runtime/bin/thread_...
runtime/bin/thread_pool.cc:102: if (pool->terminate_) continue;
On 2012/01/24 02:26:05, asiva wrote:
> Why continue and not a break?

No particular reason, changed to break.

https://chromiumcodereview.appspot.com/9141005/diff/12001/runtime/platform/th...
File runtime/platform/thread.h (right):

https://chromiumcodereview.appspot.com/9141005/diff/12001/runtime/platform/th...
runtime/platform/thread.h:28: static void Start(ThreadStartFunction function,
uword parameters);
On 2012/01/24 02:26:05, asiva wrote:
> Now that you are using threads formally in a thread pool and a server process
> setting does it make sense to return an error from this function instead of
the
> Fatal death on error?

Added a return code here. It's an int which holds the system specific error
code. Zero is success. For now all callers of Thread::Start has the fatal death
on error.

https://chromiumcodereview.appspot.com/9141005/diff/12001/runtime/platform/th...
File runtime/platform/thread_win.cc (right):

https://chromiumcodereview.appspot.com/9141005/diff/12001/runtime/platform/th...
runtime/platform/thread_win.cc:17: bool joinable)
On 2012/01/24 02:04:01, Ivan Posva wrote:
> joinable is a thing of the past.

Sorry, did not test this change on Windows.

[Søren Gjesse, 2012-01-26 16:23:10]

PTAL

[siva, 2012-01-27 18:53:17]

http://codereview.chromium.org/9141005/diff/19004/runtime/bin/eventhandler_li...
File runtime/bin/eventhandler_linux.cc (right):

http://codereview.chromium.org/9141005/diff/19004/runtime/bin/eventhandler_li...
runtime/bin/eventhandler_linux.cc:344:
reinterpret_cast<EventHandlerImplementation*>(args);
ASSERT(handler != NULL);

http://codereview.chromium.org/9141005/diff/19004/runtime/bin/eventhandler_ma...
File runtime/bin/eventhandler_macos.cc (right):

http://codereview.chromium.org/9141005/diff/19004/runtime/bin/eventhandler_ma...
runtime/bin/eventhandler_macos.cc:342:
reinterpret_cast<EventHandlerImplementation*>(args);
ASSERT(handler != NULL);

http://codereview.chromium.org/9141005/diff/19004/runtime/bin/eventhandler_wi...
File runtime/bin/eventhandler_win.cc (right):

http://codereview.chromium.org/9141005/diff/19004/runtime/bin/eventhandler_wi...
runtime/bin/eventhandler_win.cc:829:
reinterpret_cast<EventHandlerImplementation*>(args);
ASSERT(handler != NULL);

http://codereview.chromium.org/9141005/diff/19004/runtime/bin/thread_pool.cc
File runtime/bin/thread_pool.cc (right):

http://codereview.chromium.org/9141005/diff/19004/runtime/bin/thread_pool.cc#...
runtime/bin/thread_pool.cc:84: }
Threadpools always have the problem of a worker thread ending up with a wayward
task the handling of which could result in it being permanantly blocked.
In that case do we want to wait forever here for that thread to die or bail out?

http://codereview.chromium.org/9141005/diff/19004/runtime/bin/thread_pool.h
File runtime/bin/thread_pool.h (right):

http://codereview.chromium.org/9141005/diff/19004/runtime/bin/thread_pool.h#n...
runtime/bin/thread_pool.h:48: };
There was a comment by Ivan earlier about hiding the implementation of TaskQueue
and TaskQueueEntry inside of ThreadPool. Are you considering that for this CL?

[Søren Gjesse, 2012-01-30 14:42:57]

http://codereview.chromium.org/9141005/diff/19004/runtime/bin/eventhandler_li...
File runtime/bin/eventhandler_linux.cc (right):

http://codereview.chromium.org/9141005/diff/19004/runtime/bin/eventhandler_li...
runtime/bin/eventhandler_linux.cc:344:
reinterpret_cast<EventHandlerImplementation*>(args);
On 2012/01/27 18:53:17, asiva wrote:
> ASSERT(handler != NULL);

Done.

http://codereview.chromium.org/9141005/diff/19004/runtime/bin/eventhandler_ma...
File runtime/bin/eventhandler_macos.cc (right):

http://codereview.chromium.org/9141005/diff/19004/runtime/bin/eventhandler_ma...
runtime/bin/eventhandler_macos.cc:342:
reinterpret_cast<EventHandlerImplementation*>(args);
On 2012/01/27 18:53:17, asiva wrote:
> ASSERT(handler != NULL);

Done.

http://codereview.chromium.org/9141005/diff/19004/runtime/bin/eventhandler_wi...
File runtime/bin/eventhandler_win.cc (right):

http://codereview.chromium.org/9141005/diff/19004/runtime/bin/eventhandler_wi...
runtime/bin/eventhandler_win.cc:829:
reinterpret_cast<EventHandlerImplementation*>(args);
On 2012/01/27 18:53:17, asiva wrote:
> ASSERT(handler != NULL);

Done.

http://codereview.chromium.org/9141005/diff/19004/runtime/bin/thread_pool.cc
File runtime/bin/thread_pool.cc (right):

http://codereview.chromium.org/9141005/diff/19004/runtime/bin/thread_pool.cc#...
runtime/bin/thread_pool.cc:84: }
On 2012/01/27 18:53:17, asiva wrote:
> Threadpools always have the problem of a worker thread ending up with a
wayward
> task the handling of which could result in it being permanantly blocked.
> In that case do we want to wait forever here for that thread to die or bail
out?

Good point. I will add a timeout and some error message to
https://chromiumcodereview.appspot.com/9212043.

http://codereview.chromium.org/9141005/diff/19004/runtime/bin/thread_pool.h
File runtime/bin/thread_pool.h (right):

http://codereview.chromium.org/9141005/diff/19004/runtime/bin/thread_pool.h#n...
runtime/bin/thread_pool.h:48: };
On 2012/01/27 18:53:17, asiva wrote:
> There was a comment by Ivan earlier about hiding the implementation of
TaskQueue
> and TaskQueueEntry inside of ThreadPool. Are you considering that for this CL?

I moved thread_pool cleanup to separate CL,
https://chromiumcodereview.appspot.com/9212043 (which is based off this).

[siva, 2012-02-02 05:53:07]

LGTM 

The TaskQueue abstraction change will be addressed in another CL.

http://codereview.chromium.org/9141005/diff/19004/runtime/bin/thread_pool.h
File runtime/bin/thread_pool.h (right):

http://codereview.chromium.org/9141005/diff/19004/runtime/bin/thread_pool.h#n...
runtime/bin/thread_pool.h:48: };
On 2012/01/30 14:42:57, Søren Gjesse wrote:
> On 2012/01/27 18:53:17, asiva wrote:
> > There was a comment by Ivan earlier about hiding the implementation of
> TaskQueue
> > and TaskQueueEntry inside of ThreadPool. Are you considering that for this
CL?
> 
> I moved thread_pool cleanup to separate CL,
> https://chromiumcodereview.appspot.com/9212043 (which is based off this).


Ok, thanks for clarifying that.

[Ivan Posva, 2012-02-02 07:53:24]

LGTM, but the duplicate class name needs to be resolved.

Thanks
-Ivan

https://chromiumcodereview.appspot.com/9141005/diff/27001/runtime/bin/thread_...
File runtime/bin/thread_pool.cc (right):

https://chromiumcodereview.appspot.com/9141005/diff/27001/runtime/bin/thread_...
runtime/bin/thread_pool.cc:67: MonitorLocker monitor(&monitor_);
It is very, very confusing to have two different MonitorLocker classes!

[Søren Gjesse, 2012-02-02 08:46:08]

https://chromiumcodereview.appspot.com/9141005/diff/27001/runtime/bin/thread_...
File runtime/bin/thread_pool.cc (right):

https://chromiumcodereview.appspot.com/9141005/diff/27001/runtime/bin/thread_...
runtime/bin/thread_pool.cc:67: MonitorLocker monitor(&monitor_);
On 2012/02/02 07:53:24, Ivan Posva wrote:
> It is very, very confusing to have two different MonitorLocker classes!

They are in different namespaces though. I will try to come up with two
different names though.