Adding thread_pool to the GCL namespace.

Adds in a very basic thread pool (no ability to add new events). Contains basic functionality for pausing/starting the pool and counting the number of active threads. Support for delayed execution is offloaded to the alarm service. A more complex form which allows enqueue of new tasks will be added later.

[ChrisM, 2010-08-09 07:07:48]

Hey guys, ping since the thread pool is now working and passing all of its
tests.

[Alasdair, 2010-08-17 01:41:49]

Just a couple of quick comments. Will take a closer look tomorrow.

http://codereview.appspot.com/1678062/diff/2001/3004
File include/thread_pool.h (right):

http://codereview.appspot.com/1678062/diff/2001/3004#newcode67
include/thread_pool.h:67: vector<atomic_int> thread_states;
Would it be better to use atomic<ThreadState>?

http://codereview.appspot.com/1678062/diff/2001/3004#newcode74
include/thread_pool.h:74: PoolState pool_state;
I believe we're using Google style member variables, with trailing underscores?

http://codereview.appspot.com/1678062/diff/2001/3005
File src/thread_pool.cc (right):

http://codereview.appspot.com/1678062/diff/2001/3005#newcode20
src/thread_pool.cc:20: thread_states(pool_size, atomic_int()),
I think the thread_states are going to have an undefined value until they
actually start running. Do we need to initialise the thread_states here?

http://codereview.appspot.com/1678062/diff/2001/3005#newcode55
src/thread_pool.cc:55: pool_state_mu.unlock();
Don't think we need this if we're using a unique lock.

http://codereview.appspot.com/1678062/diff/2001/3005#newcode90
src/thread_pool.cc:90: // Wait for there something to be in the queue. Even if
the queue is closed
"to be something"

http://codereview.appspot.com/1678062/diff/2001/3005#newcode100
src/thread_pool.cc:100: // If the pool is paused
Something missing on the end of this sentence?

[ChrisM, 2010-08-27 04:01:40]

http://codereview.appspot.com/1678062/diff/2001/3004
File include/thread_pool.h (right):

http://codereview.appspot.com/1678062/diff/2001/3004#newcode67
include/thread_pool.h:67: vector<atomic_int> thread_states;
On 2010/08/17 01:41:50, Alasdair wrote:
> Would it be better to use atomic<ThreadState>?

I've removed the thread states anyway.

http://codereview.appspot.com/1678062/diff/2001/3004#newcode74
include/thread_pool.h:74: PoolState pool_state;
On 2010/08/17 01:41:50, Alasdair wrote:
> I believe we're using Google style member variables, with trailing
underscores?

Done.

http://codereview.appspot.com/1678062/diff/2001/3005
File src/thread_pool.cc (right):

http://codereview.appspot.com/1678062/diff/2001/3005#newcode20
src/thread_pool.cc:20: thread_states(pool_size, atomic_int()),
On 2010/08/17 01:41:50, Alasdair wrote:
> I think the thread_states are going to have an undefined value until they
> actually start running. Do we need to initialise the thread_states here?

Thread states are gone for now.

http://codereview.appspot.com/1678062/diff/2001/3005#newcode55
src/thread_pool.cc:55: pool_state_mu.unlock();
True
On 2010/08/17 01:41:50, Alasdair wrote:
> Don't think we need this if we're using a unique lock.

http://codereview.appspot.com/1678062/diff/2001/3005#newcode90
src/thread_pool.cc:90: // Wait for there something to be in the queue. Even if
the queue is closed
On 2010/08/17 01:41:50, Alasdair wrote:
> "to be something"

Done.

http://codereview.appspot.com/1678062/diff/2001/3005#newcode100
src/thread_pool.cc:100: // If the pool is paused
On 2010/08/17 01:41:50, Alasdair wrote:
> Something missing on the end of this sentence?

Done.

[Lawrence_Crowl.org, 2010-09-24 17:15:11]

Why does the thread pool need to be a friend of the source
source and the thread class?

What is the user-side interface to the thread pool?

On 8/26/10, ccmysen@gmail.com <ccmysen@gmail.com> wrote:
>
>  http://codereview.appspot.com/1678062/diff/2001/3004
>  File include/thread_pool.h (right):
>
> http://codereview.appspot.com/1678062/diff/2001/3004#newcode67
>  include/thread_pool.h:67: vector<atomic_int> thread_states;
>  On 2010/08/17 01:41:50, Alasdair wrote:
>
> > Would it be better to use atomic<ThreadState>?
> >
>
>  I've removed the thread states anyway.
>
> http://codereview.appspot.com/1678062/diff/2001/3004#newcode74
>  include/thread_pool.h:74: PoolState pool_state;
>  On 2010/08/17 01:41:50, Alasdair wrote:
>
> > I believe we're using Google style member variables, with trailing
> >
>  underscores?
>
>  Done.
>
>  http://codereview.appspot.com/1678062/diff/2001/3005
>  File src/thread_pool.cc (right):
>
> http://codereview.appspot.com/1678062/diff/2001/3005#newcode20
>  src/thread_pool.cc:20: thread_states(pool_size, atomic_int()),
>  On 2010/08/17 01:41:50, Alasdair wrote:
>
> > I think the thread_states are going to have an undefined value until
> >
>  they
>
> > actually start running. Do we need to initialise the thread_states
> >
>  here?
>
>  Thread states are gone for now.
>
> http://codereview.appspot.com/1678062/diff/2001/3005#newcode55
>  src/thread_pool.cc:55: pool_state_mu.unlock();
>  True
>  On 2010/08/17 01:41:50, Alasdair wrote:
>
> > Don't think we need this if we're using a unique lock.
> >
>
> http://codereview.appspot.com/1678062/diff/2001/3005#newcode90
>  src/thread_pool.cc:90: // Wait for there something to be in the queue.
>  Even if the queue is closed
>  On 2010/08/17 01:41:50, Alasdair wrote:
>
> > "to be something"
> >
>
>  Done.
>
> http://codereview.appspot.com/1678062/diff/2001/3005#newcode100
>  src/thread_pool.cc:100: // If the pool is paused
>  On 2010/08/17 01:41:50, Alasdair wrote:
>
> > Something missing on the end of this sentence?
> >
>
>  Done.
>
>  http://codereview.appspot.com/1678062/
>


-- 
Lawrence Crowl

[ChrisM, 2010-09-24 17:23:21]

This is currently being re-written (i've got it half implemented at the
moment, though not fully).
I'll ping (hopefully next week) when i've gotten it mostly done.

In terms of the interface, though, there basically isn't any interface at
the moment. You construct the pool and hand it a source and it just goes
until the source closes or you stop the pool.

In the new form with pipelines and growable thread pools, there will be a
slightly wider interface which will let you to call execute() which will
basically just add to the sink and there will probably (i'm leaving it out
for now) be an interface for providing hints as to when to grow/collapse the
pool. I'm currently using the java thread pool design, though, of growing
the thread pool only when work is backed up and reducing the size when there
is no work to do for multiple threads in the pool.

-c

On Fri, Sep 24, 2010 at 10:14 AM, Lawrence Crowl <Lawrence@crowl.org> wrote:

> Why does the thread pool need to be a friend of the source
> source and the thread class?
>
> What is the user-side interface to the thread pool?
>
> On 8/26/10, ccmysen@gmail.com <ccmysen@gmail.com> wrote:
> >
> >  http://codereview.appspot.com/1678062/diff/2001/3004
> >  File include/thread_pool.h (right):
> >
> > http://codereview.appspot.com/1678062/diff/2001/3004#newcode67
> >  include/thread_pool.h:67: vector<atomic_int> thread_states;
> >  On 2010/08/17 01:41:50, Alasdair wrote:
> >
> > > Would it be better to use atomic<ThreadState>?
> > >
> >
> >  I've removed the thread states anyway.
> >
> > http://codereview.appspot.com/1678062/diff/2001/3004#newcode74
> >  include/thread_pool.h:74: PoolState pool_state;
> >  On 2010/08/17 01:41:50, Alasdair wrote:
> >
> > > I believe we're using Google style member variables, with trailing
> > >
> >  underscores?
> >
> >  Done.
> >
> >  http://codereview.appspot.com/1678062/diff/2001/3005
> >  File src/thread_pool.cc (right):
> >
> > http://codereview.appspot.com/1678062/diff/2001/3005#newcode20
> >  src/thread_pool.cc:20: thread_states(pool_size, atomic_int()),
> >  On 2010/08/17 01:41:50, Alasdair wrote:
> >
> > > I think the thread_states are going to have an undefined value until
> > >
> >  they
> >
> > > actually start running. Do we need to initialise the thread_states
> > >
> >  here?
> >
> >  Thread states are gone for now.
> >
> > http://codereview.appspot.com/1678062/diff/2001/3005#newcode55
> >  src/thread_pool.cc:55: pool_state_mu.unlock();
> >  True
> >  On 2010/08/17 01:41:50, Alasdair wrote:
> >
> > > Don't think we need this if we're using a unique lock.
> > >
> >
> > http://codereview.appspot.com/1678062/diff/2001/3005#newcode90
> >  src/thread_pool.cc:90: // Wait for there something to be in the queue.
> >  Even if the queue is closed
> >  On 2010/08/17 01:41:50, Alasdair wrote:
> >
> > > "to be something"
> > >
> >
> >  Done.
> >
> > http://codereview.appspot.com/1678062/diff/2001/3005#newcode100
> >  src/thread_pool.cc:100: // If the pool is paused
> >  On 2010/08/17 01:41:50, Alasdair wrote:
> >
> > > Something missing on the end of this sentence?
> > >
> >
> >  Done.
> >
> >  http://codereview.appspot.com/1678062/
> >
>
>
> --
> Lawrence Crowl
>

[Alasdair, 2011-01-14 19:49:51]

http://codereview.appspot.com/1678062/diff/9001/src/thread_pool.cc
File src/thread_pool.cc (right):

http://codereview.appspot.com/1678062/diff/9001/src/thread_pool.cc#newcode83
src/thread_pool.cc:83: 
Note from meeting, 14/1/11. When we get here, we've popped a value off the
queue. We shouldn't break (even if shutting down) without somehow consuming the
value. (Or putting it back on the queue???)