Adding support for non joinable threads to PlatformThread

Added a new method CreateNonJoinable() to PlatformThread.  Added the windows and posix implementations.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=11053

[willchan no longer on Chromium, 2009-03-04 22:17:34]

It'd be nice to have non-joinable thread support for implementing a threadpool
that dynamically allocates threads as needed.  Hence the changelist.  This is a
quick and dirty implementation of it.  For windows, we just close the handle
immediately.  For posix, we set the detachable attribute for the thread before
creation.  The implementation is mostly a copy/paste of Create().

I think Dean wanted to generalize this further and create a CreateWithOptions()
method of some sort, which I'm cool with.  My understanding is Darin is opposed
to adding Options since people aren't supposed to be using PlatformThread much
anyway.  Anyway, I'm mailing this out so people can discuss how best to do this.

[darin (slow to review), 2009-03-04 22:58:18]

normally, we use joinable threads to help make shutdown sane.  threads that
create other threads usually wait for the threads they created to exit before
they themselves exit.

however, sometimes you have a system call like getaddrinfo that could block for
a very long time.  it would be unfortunate if browser shutdown blocked waiting
for getaddrinfo to return.

so, in chrome we let the threads that call getaddrinfo leak.  we do that on
windows by leveraging the builtin windows thread pool.

is your plan to only use non-joinable threads for the thread pool used to invoke
getaddrinfo?  or do you also plan to use this for the filestream implementation?
 i ask because non-joinable threads make it harder to deal with threading issues
owing to shutdown related grief.

[willchan no longer on Chromium, 2009-03-04 23:20:47]

On 2009/03/04 22:58:18, darin wrote:
> normally, we use joinable threads to help make shutdown sane.  threads that
> create other threads usually wait for the threads they created to exit before
> they themselves exit.
> 
> however, sometimes you have a system call like getaddrinfo that could block
for
> a very long time.  it would be unfortunate if browser shutdown blocked waiting
> for getaddrinfo to return.
> 
> so, in chrome we let the threads that call getaddrinfo leak.  we do that on
> windows by leveraging the builtin windows thread pool.
> 
> is your plan to only use non-joinable threads for the thread pool used to
invoke
> getaddrinfo?  or do you also plan to use this for the filestream
implementation?
>  i ask because non-joinable threads make it harder to deal with threading
issues
> owing to shutdown related grief.

Ah, these are interesting points.  I was planning on using this for the
threadpool for filestreams.  I have not looked at the different requirements (if
any) for getaddrinfo.  Are you implying that for the filestream implementation,
we would not want to simply leak these threads?  I'm not familiar with shutdown
issues, what do you mean by leveraging the builtin windows thread pool?  Are you
saying that leaking threads normally is bad?  Is that because of OS issues with
not cleaning up the descriptors properly, or is it because process exit might
kill the threads in the middle of execution, which could leave things in an
inconsistent state?

Following up on this, does this imply I should be using a pool of non-joinable
threads for getaddrinfo and a separate pool of joinable threads for file IO? 
What are your thoughts here?

[darin (slow to review), 2009-03-04 23:51:25]

> Ah, these are interesting points.  I was planning on using this for the
> threadpool for filestreams.  I have not looked at the different requirements
(if
> any) for getaddrinfo.  Are you implying that for the filestream
implementation,
> we would not want to simply leak these threads?

Right, maybe we would not.


> I'm not familiar with shutdown
> issues, what do you mean by leveraging the builtin windows thread pool?  Are
you
> saying that leaking threads normally is bad?  Is that because of OS issues
with
> not cleaning up the descriptors properly, or is it because process exit might
> kill the threads in the middle of execution, which could leave things in an
> inconsistent state?

main issue:

often we put objects onto background threads that need to be posted back to a
consumer thread where they then need to be destroyed.  at shutdown, normally
the consumer will tell the guy responsible for the background thread to stop.
that usually results in a thread join so that the consumer can be sure that
the background thread is done touching his objects, meaning that it is now
safe for him to destroy those objects.

a secondary concern is supporting memory leak detection tools.  if we have
threads that leak a lot of objects, then our MLK detection tools have trouble.
we end up having to maintain a lot of exceptions.

so, in the end it just works out a lot nicer if we clean up threads
systematically using join to synchronize shutdown.


> Following up on this, does this imply I should be using a pool of non-joinable
> threads for getaddrinfo and a separate pool of joinable threads for file IO? 
> What are your thoughts here?

That is what I was wondering.  Maybe it would be good if the thread pool could
be
used in either way.  Maybe when the thread pool gets deleted it could join with
its
worker threads if it is in the joinable mode.  It may also be the case that this
is
not worth the effort.

I think we avoid the whole MLK issue on Windows since the thread pool is managed
by windows entirely.  None of our objects leak in other words.

[darin (slow to review), 2009-03-05 19:22:32]

the interface lgtm

http://codereview.chromium.org/40133/diff/1/3
File base/platform_thread_posix.cc (right):

http://codereview.chromium.org/40133/diff/1/3#newcode95
Line 95: bool PlatformThread::CreateNonJoinable(size_t stack_size, Delegate*
delegate) {
since this function duplicates almost all of the code in Create, perhaps we
should just create a local subroutine that takes a detached boolean parameter.

[willchan no longer on Chromium, 2009-03-05 20:06:55]

http://codereview.chromium.org/40133/diff/1/3
File base/platform_thread_posix.cc (right):

http://codereview.chromium.org/40133/diff/1/3#newcode95
Line 95: bool PlatformThread::CreateNonJoinable(size_t stack_size, Delegate*
delegate) {
On 2009/03/05 19:22:32, darin wrote:
> since this function duplicates almost all of the code in Create, perhaps we
> should just create a local subroutine that takes a detached boolean parameter.

Done.

[darin (slow to review), 2009-03-05 21:11:02]

LGTM

http://codereview.chromium.org/40133/diff/13/1007
File base/platform_thread_posix.cc (right):

http://codereview.chromium.org/40133/diff/13/1007#newcode111
Line 111: PlatformThreadHandle thread_handle;
in cases like this, i often just name the local variable "unused" to make it
self documenting.

[willchan no longer on Chromium, 2009-03-05 22:05:03]

http://codereview.chromium.org/40133/diff/13/1007
File base/platform_thread_posix.cc (right):

http://codereview.chromium.org/40133/diff/13/1007#newcode111
Line 111: PlatformThreadHandle thread_handle;
On 2009/03/05 21:11:02, darin wrote:
> in cases like this, i often just name the local variable "unused" to make it
> self documenting.

Done.

[darin (slow to review), 2009-03-05 22:47:09]

LGTM