Fix up SequencedWorkerPool in preparation for making it a TaskRunner

Fix up SequencedWorkerPool in preparation for making it a TaskRunner

Make SequencedWorkerPool ref-counted, merge it with ::Inner, and
make its destructor private.  Make users hold a scoped_refptr.

Fix bug where SequencedWorkerPool::Worker wasn't taking a reference
to the worker pool.

Rename SequencedWorkerPool::PostTask to PostTaskHelper.

Clean up includes and use forward declarations when possible.

Make SequencedWorkerPool::Shutdown completely thread-safe by merging
the terminating_ and shutdown_called_ flag. (Now that it's ref-counted,
it can be passed around multiple threads.)

Clean up includes and params in webkit/dom_storage a bit.

BUG=114329, 114330
TEST=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=123823

[akalin, 2012-02-15 22:54:21]

+brettw for review

[akalin, 2012-02-16 00:12:42]

Fix compile errors in dom_storage, +michaeln for reviewing that

[brettw, 2012-02-16 01:33:13]

Why would the TaskRunner classes need to access the inner object?

[akalin, 2012-02-16 01:35:15]

On 2012/02/16 01:33:13, brettw wrote:
> Why would the TaskRunner classes need to access the inner object?

Because it needs to hold a reference to it, as otherwise if the worker pool goes
away the TaskRunner will access freed memory.

It looks like Inner being ref-counted isn't really needed otherwise, but
conveniently enough TaskRunner provides a use case for it.

[brettw, 2012-02-16 01:48:07]

On Wed, Feb 15, 2012 at 5:35 PM,  <akalin@chromium.org> wrote:
> On 2012/02/16 01:33:13, brettw wrote:
>>
>> Why would the TaskRunner classes need to access the inner object?
>
>
> Because it needs to hold a reference to it, as otherwise if the worker pool
> goes
> away the TaskRunner will access freed memory.
>
> It looks like Inner being ref-counted isn't really needed otherwise, but
> conveniently enough TaskRunner provides a use case for it.

The inner class can outlive the outer class in the case where there
are continue on shutdown tasks. Exposing this seems backwards. If we
have a memory lifetime issue, then I think the outer class should be
refcounted.

Brett

[akalin, 2012-02-16 01:59:22]

On 2012/02/16 01:48:07, brettw wrote:
> On Wed, Feb 15, 2012 at 5:35 PM,  <mailto:akalin@chromium.org> wrote:
> > On 2012/02/16 01:33:13, brettw wrote:
> >>
> >> Why would the TaskRunner classes need to access the inner object?
> >
> >
> > Because it needs to hold a reference to it, as otherwise if the worker pool
> > goes
> > away the TaskRunner will access freed memory.
> >
> > It looks like Inner being ref-counted isn't really needed otherwise, but
> > conveniently enough TaskRunner provides a use case for it.
> 
> The inner class can outlive the outer class in the case where there
> are continue on shutdown tasks. Exposing this seems backwards. If we
> have a memory lifetime issue, then I think the outer class should be
> refcounted.

I don't see what else is holding a ref to the inner class.  I see that the
worker threads themselves do outlive the outer class, but those hold only raw
pointers to the inner class, which seems wrong.

Making the outer class seems like a worse abstraction leak to me.  Why do
instantiators SequencedWorkerPool have to suffer just because some other clients
may want to get TaskRunner wrappers around it?

A possible alternative would be to do what MessageLoop does and provide
facilities for notifying observers when the worker pool is shutting down.

In any case, I'd still like to split up that file as it's getting to be a big
ball of mud.  It also makes it easy for includers to not obey IWYU, as evidenced
by the dom_storage changes. :/

I think moving the inner/worker classes to the internal namespace is 'private'
enough.

[michaeln, 2012-02-17 04:32:19]

+1 what brett said, seems like a simpler solution

> have a memory lifetime issue, then I think the outer class should be
> refcounted.

[brettw, 2012-02-17 04:46:57]

Thanks for noticing the lack of use of the refcounted facility of the inner
class. My original patch used this but it seems like my current one (perhaps
incorrectly) doesn't.


There are two ways of looking at this. The first is that the sequenced worker
pool is a component, and the task runners are a facility built on top of that
API. This is how I think of it since I wrote the worker pool and now you're
adding task runners. In this way of thinking, the inner class should not be
exposed.

The other way of thinking is that the task runners are a facility exposed by the
worker pool to allow other code to access it.

It seems weird to me that we would have external objects that refer to some
inner part of the class, and if the creator releases the outer object, the inner
one stays alive and is usable by all these callers, but we can never get the
outer "official" API back.

And I don't think making the object refcounted would make callers more
complicated. The current one is just managed by the BrowserThread and we give
pointers when somebody asks for it. If we changed the BrowserThread class to
having a refptr, nothing would change for the current callers. The only
difference would be that you now have the option of hanging onto that pointer by
keeping a ref to it, which was an option you didn't have before.

[akalin, 2012-02-17 09:41:24]

On 2012/02/17 04:46:57, brettw wrote:
> Thanks for noticing the lack of use of the refcounted facility of the inner
> class. My original patch used this but it seems like my current one (perhaps
> incorrectly) doesn't.
> 
> 
> There are two ways of looking at this. The first is that the sequenced worker
> pool is a component, and the task runners are a facility built on top of that
> API. This is how I think of it since I wrote the worker pool and now you're
> adding task runners. In this way of thinking, the inner class should not be
> exposed.
> 
> The other way of thinking is that the task runners are a facility exposed by
the
> worker pool to allow other code to access it.
> 
> It seems weird to me that we would have external objects that refer to some
> inner part of the class, and if the creator releases the outer object, the
inner
> one stays alive and is usable by all these callers, but we can never get the
> outer "official" API back.
> 
> And I don't think making the object refcounted would make callers more
> complicated. The current one is just managed by the BrowserThread and we give
> pointers when somebody asks for it. If we changed the BrowserThread class to
> having a refptr, nothing would change for the current callers. The only
> difference would be that you now have the option of hanging onto that pointer
by
> keeping a ref to it, which was an option you didn't have before.

Okay.  My only concern was that making SWP ref-counted would blur the ownership.
 For example now, it's pretty clear what owns the SWP, since either it's a
member variable of some object or in a scoped_ptr or something.

But I guess a valid use case would be to pass around the SWP (or a refptr to it)
to multiple objects, threads, etc., and therefore it pretty much has to be
ref-counted.  I had in mind an analogy with MessageLoop (which isn't
ref-counted) but I guess that doesn't hold because MLs are much more restricted
(at most once per thread, etc.).

So the whole purpose of the ::Inner private class was so that the worker classes
can hold a refptr to them.  If the outer class will be ref-counted, then there's
no need for the ::Inner class, right?  The workers can hold refptrs to the SWP
object itself, and so can the TaskRunner wrappers.  Does that sound reasonable?

[brettw, 2012-02-17 20:18:14]

On Fri, Feb 17, 2012 at 1:41 AM,  <akalin@chromium.org> wrote:
> On 2012/02/17 04:46:57, brettw wrote:
>>
>> Thanks for noticing the lack of use of the refcounted facility of the
>> inner
>> class. My original patch used this but it seems like my current one
>> (perhaps
>> incorrectly) doesn't.
>
>
>
>> There are two ways of looking at this. The first is that the sequenced
>> worker
>> pool is a component, and the task runners are a facility built on top of
>> that
>> API. This is how I think of it since I wrote the worker pool and now
>> you're
>> adding task runners. In this way of thinking, the inner class should not
>> be
>> exposed.
>
>
>> The other way of thinking is that the task runners are a facility exposed
>> by
>
> the
>>
>> worker pool to allow other code to access it.
>
>
>> It seems weird to me that we would have external objects that refer to
>> some
>> inner part of the class, and if the creator releases the outer object, the
>
> inner
>>
>> one stays alive and is usable by all these callers, but we can never get
>> the
>> outer "official" API back.
>
>
>> And I don't think making the object refcounted would make callers more
>> complicated. The current one is just managed by the BrowserThread and we
>> give
>> pointers when somebody asks for it. If we changed the BrowserThread class
>> to
>> having a refptr, nothing would change for the current callers. The only
>> difference would be that you now have the option of hanging onto that
>> pointer
>
> by
>>
>> keeping a ref to it, which was an option you didn't have before.
>
>
> Okay.  My only concern was that making SWP ref-counted would blur the
> ownership.
>  For example now, it's pretty clear what owns the SWP, since either it's a
> member variable of some object or in a scoped_ptr or something.
>
> But I guess a valid use case would be to pass around the SWP (or a refptr to
> it)
> to multiple objects, threads, etc., and therefore it pretty much has to be
> ref-counted.  I had in mind an analogy with MessageLoop (which isn't
> ref-counted) but I guess that doesn't hold because MLs are much more
> restricted
> (at most once per thread, etc.).
>
> So the whole purpose of the ::Inner private class was so that the worker
> classes
> can hold a refptr to them.  If the outer class will be ref-counted, then
> there's
> no need for the ::Inner class, right?  The workers can hold refptrs to the
> SWP
> object itself, and so can the TaskRunner wrappers.  Does that sound
> reasonable?

The inner class solved this problem as well as the dependency problem.
It depends on a lot of obscure thread includes that I didn't really
want to expose to everybody that includes the pool header. I think we
can probably remove the scoping for the inner class, you're right.

Brett

[akalin, 2012-02-18 09:47:43]

> The inner class solved this problem as well as the dependency problem.
> It depends on a lot of obscure thread includes that I didn't really
> want to expose to everybody that includes the pool header. I think we
> can probably remove the scoping for the inner class, you're right.
> 
> Brett

Actually, can't we take it one step further and make SequencedWorkerPool
implement TaskRunner directly? Otherwise, the helper TaskRunner wrapper would
just be a scoped_refptr to the SequencedWorkerPool.

[brettw, 2012-02-20 21:28:44]

On Sat, Feb 18, 2012 at 1:47 AM,  <akalin@chromium.org> wrote:
> Actually, can't we take it one step further and make SequencedWorkerPool
> implement TaskRunner directly? Otherwise, the helper TaskRunner wrapper
> would
> just be a scoped_refptr to the SequencedWorkerPool.

It could implement the basic unordered kind, sure. But the ordered
ones would have to be separate objects since they need a unique
sequence token.

Brett

[akalin, 2012-02-22 22:33:10]

> It could implement the basic unordered kind, sure. But the ordered
> ones would have to be separate objects since they need a unique
> sequence token.

Yes, just a regular TaskRunner; that's what I meant.

Okay, I uploaded a new patch addressing all the comments and edited the CL
description/subject.  Basically, I made SequencedWorkerPool ref-counted, merged
it with the inner class, and made the worker class hold a refptr to it.

Please take another look!

[michaeln, 2012-02-23 03:55:25]

Thnx for cleaning up the iwyu things.

http://codereview.chromium.org/9347056/diff/13001/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_task_runner.h (right):

http://codereview.chromium.org/9347056/diff/13001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_task_runner.h:51: const
scoped_refptr<base::MessageLoopProxy>& delayed_task_loop);
Has our style been changed to require this technique of passing refs to
refcounted objects? Unless this is specificially required, i'd prefer the simple
ptr types to remain here.

[akalin, 2012-02-23 22:22:30]

PTAL.

ping, brett?

http://codereview.chromium.org/9347056/diff/13001/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_task_runner.h (right):

http://codereview.chromium.org/9347056/diff/13001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_task_runner.h:51: const
scoped_refptr<base::MessageLoopProxy>& delayed_task_loop);
On 2012/02/23 03:55:25, michaeln wrote:
> Has our style been changed to require this technique of passing refs to
> refcounted objects? Unless this is specificially required, i'd prefer the
simple
> ptr types to remain here.

Done.

[brettw, 2012-02-24 05:43:34]

http://codereview.chromium.org/9347056/diff/21001/base/threading/sequenced_wo...
File base/threading/sequenced_worker_pool.h (right):

http://codereview.chromium.org/9347056/diff/21001/base/threading/sequenced_wo...
base/threading/sequenced_worker_pool.h:22: #include
"base/synchronization/condition_variable.h"
One of the reasons I liked the inner class was that lots of code will use this,
and it keeps the dependencies under control. This is quite a lot of big
dependencies.

[michaeln, 2012-02-24 19:12:33]

/dom_storage lgtm

brett has a good point about hiding the details (the dependencies) in the
sequenced_worker_pool.cc, i'm not looking closely at that part, but maybe an
inner "State" class that the outer owns via scoped_ptr could trim down the
includes in the .h.

[akalin, 2012-02-24 20:22:18]

On 2012/02/24 05:43:34, brettw wrote:
>
http://codereview.chromium.org/9347056/diff/21001/base/threading/sequenced_wo...
> base/threading/sequenced_worker_pool.h:22: #include
> "base/synchronization/condition_variable.h"
> One of the reasons I liked the inner class was that lots of code will use
this,
> and it keeps the dependencies under control. This is quite a lot of big
> dependencies.

Yeah, okay.  Moved back into Inner class.  PTAL!

[brettw, 2012-02-27 17:08:40]

lgtm

[commit-bot: I haz the power, 2012-02-27 19:32:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/9347056/34009