Support Closure in ALL the loops!

Support Closure in ALL the loops!

Add an overload for PostTask into MessageLoopProxy, and WorkerPool.

BUG=35223
TEST=unittests.


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

[awong, 2011-07-07 21:20:10]

Add in PostTask to MessageLoopProxy and WorkerPool.

[darin (slow to review), 2011-07-07 21:54:01]

http://codereview.chromium.org/7316015/diff/7001/base/message_loop_proxy_impl.cc
File base/message_loop_proxy_impl.cc (right):

http://codereview.chromium.org/7316015/diff/7001/base/message_loop_proxy_impl...
base/message_loop_proxy_impl.cc:133: {
the extra nesting here can be removed.  it only existed above so that we would
avoid deleting the task while holding the message loop lock.

http://codereview.chromium.org/7316015/diff/7001/base/task.cc
File base/task.cc (right):

http://codereview.chromium.org/7316015/diff/7001/base/task.cc#newcode37
base/task.cc:37: namespace subtle {
nit: we usually have a new line of padding at the opening and close of a
namespace, except when forward declaring namespace types.

http://codereview.chromium.org/7316015/diff/7001/base/task.h
File base/task.h (right):

http://codereview.chromium.org/7316015/diff/7001/base/task.h#newcode567
base/task.h:567: namespace subtle {
nit: new line after namespace

http://codereview.chromium.org/7316015/diff/7001/base/threading/worker_pool_p...
File base/threading/worker_pool_posix.cc (right):

http://codereview.chromium.org/7316015/diff/7001/base/threading/worker_pool_p...
base/threading/worker_pool_posix.cc:83: PosixDynamicThreadPool::PendingTask
pending_task = pool_->WaitForTask();
I have not reviewed this file very carefully.

http://codereview.chromium.org/7316015/diff/7001/base/threading/worker_pool_w...
File base/threading/worker_pool_win.cc (right):

http://codereview.chromium.org/7316015/diff/7001/base/threading/worker_pool_w...
base/threading/worker_pool_win.cc:43:
tracked_objects::ThreadData::TallyADeathIfActive(
nit: indentation is off by 2

[awong, 2011-07-08 00:38:39]

Comments addressed.

FYI, Mac tests seems to be consistently failing on this change due to the
AtExitManager's lock being busy on destruction. Not sure how that might be
happening.  If anyone has ideas, much appreciated.  Otherwise, going to start
binary searching the code through the try bots.

http://codereview.chromium.org/7316015/diff/7001/base/message_loop_proxy_impl.cc
File base/message_loop_proxy_impl.cc (right):

http://codereview.chromium.org/7316015/diff/7001/base/message_loop_proxy_impl...
base/message_loop_proxy_impl.cc:133: {
On 2011/07/07 21:54:01, darin wrote:
> the extra nesting here can be removed.  it only existed above so that we would
> avoid deleting the task while holding the message loop lock.

Done.

http://codereview.chromium.org/7316015/diff/7001/base/task.cc
File base/task.cc (right):

http://codereview.chromium.org/7316015/diff/7001/base/task.cc#newcode37
base/task.cc:37: namespace subtle {
On 2011/07/07 21:54:01, darin wrote:
> nit: we usually have a new line of padding at the opening and close of a
> namespace, except when forward declaring namespace types.

Done.

http://codereview.chromium.org/7316015/diff/7001/base/task.h
File base/task.h (right):

http://codereview.chromium.org/7316015/diff/7001/base/task.h#newcode567
base/task.h:567: namespace subtle {
On 2011/07/07 21:54:01, darin wrote:
> nit: new line after namespace

Done.

http://codereview.chromium.org/7316015/diff/7001/base/threading/worker_pool_p...
File base/threading/worker_pool_posix.cc (right):

http://codereview.chromium.org/7316015/diff/7001/base/threading/worker_pool_p...
base/threading/worker_pool_posix.cc:83: PosixDynamicThreadPool::PendingTask
pending_task = pool_->WaitForTask();
On 2011/07/07 21:54:01, darin wrote:
> I have not reviewed this file very carefully.

Okay. Will wait for Will.

http://codereview.chromium.org/7316015/diff/7001/base/threading/worker_pool_w...
File base/threading/worker_pool_win.cc (right):

http://codereview.chromium.org/7316015/diff/7001/base/threading/worker_pool_w...
base/threading/worker_pool_win.cc:43:
tracked_objects::ThreadData::TallyADeathIfActive(
On 2011/07/07 21:54:01, darin wrote:
> nit: indentation is off by 2

Done.

[willchan no longer on Chromium, 2011-07-08 12:04:21]

I'm not sure if my analysis of the LazyInstance bug is 100% spot on, but I think
it's at least 80% correct.

http://codereview.chromium.org/7316015/diff/6022/base/message_loop_proxy.h
File base/message_loop_proxy.h (right):

http://codereview.chromium.org/7316015/diff/6022/base/message_loop_proxy.h#ne...
base/message_loop_proxy.h:56: const base::Closure& task, int64 delay_ms) = 0;
Move |delay_ms| to the next line a la Chromium style guide.

http://codereview.chromium.org/7316015/diff/6022/base/message_loop_proxy_impl.h
File base/message_loop_proxy_impl.h (right):

http://codereview.chromium.org/7316015/diff/6022/base/message_loop_proxy_impl...
base/message_loop_proxy_impl.h:38: const base::Closure& task, int64 delay_ms);
|delay_ms| to next line.

http://codereview.chromium.org/7316015/diff/6022/base/message_loop_proxy_impl...
base/message_loop_proxy_impl.h:61: const base::Closure& task, int64 delay_ms,
|delay_ms| to next line

http://codereview.chromium.org/7316015/diff/6022/base/message_loop_proxy_impl...
File base/message_loop_proxy_impl_unittest.cc (right):

http://codereview.chromium.org/7316015/diff/6022/base/message_loop_proxy_impl...
base/message_loop_proxy_impl_unittest.cc:8: #include
"base/message_loop_proxy_impl.h"
This header should be first.

http://codereview.chromium.org/7316015/diff/6022/base/task.h
File base/task.h (right):

http://codereview.chromium.org/7316015/diff/6022/base/task.h#newcode576
base/task.h:576: // In particular, notice that the this is RefCounted instead of
Your comment is totally off. This is RefCountedThreadSafe.

http://codereview.chromium.org/7316015/diff/6022/base/task.h#newcode577
base/task.h:577: // RefCountedThreadSafe.  We rely on the fact thas users of
this class are
s/thas/that/

http://codereview.chromium.org/7316015/diff/6022/base/threading/worker_pool_p...
File base/threading/worker_pool_posix.cc (right):

http://codereview.chromium.org/7316015/diff/6022/base/threading/worker_pool_p...
base/threading/worker_pool_posix.cc:106: bool WorkerPool::PostTask(const
tracked_objects::Location& from_here,
Minor nit, perhaps not even worth fixing. So don't bother unless you care.

I'm a bit surprised we don't do the Task/Closure=>PendingTask conversion at the
outermost API, which is the static WorkerPool::PostTask() methods. I would have
thought we would want to eliminate the two paths at the earliest point possible.

http://codereview.chromium.org/7316015/diff/6022/base/threading/worker_pool_p...
base/threading/worker_pool_posix.cc:136:
tracked_objects::ThreadData::TallyADeathIfActive(
These calls are probably what's causing your shutdown crashes. They rely on
MessageLoop::current(), which uses LazyInstance. The LazyInstance probably
already got destroyed by the AtExitManager. I suspect ~AtExitManager's body
destroys the LazyInstance, but in between the window where the Lock is released
at the end of the body and before ~AtExitManager begins deleting the member
variables, this pool recreates MessageLoop::current()'s LazyInstance and
acquires the lock, so then when it gets destroyed, it aborts since it's illegal
to delete a lock while it's held.

http://codereview.chromium.org/7316015/diff/6022/base/tracked_objects.h
File base/tracked_objects.h (right):

http://codereview.chromium.org/7316015/diff/6022/base/tracked_objects.h#newco...
base/tracked_objects.h:495: 
Spurious?

[awong, 2011-07-08 18:36:51]

Doing test on try servers with death tally code removed now.

http://codereview.chromium.org/7316015/diff/6022/base/message_loop_proxy.h
File base/message_loop_proxy.h (right):

http://codereview.chromium.org/7316015/diff/6022/base/message_loop_proxy.h#ne...
base/message_loop_proxy.h:56: const base::Closure& task, int64 delay_ms) = 0;
On 2011/07/08 12:04:21, willchan wrote:
> Move |delay_ms| to the next line a la Chromium style guide.

Done.

http://codereview.chromium.org/7316015/diff/6022/base/message_loop_proxy_impl.h
File base/message_loop_proxy_impl.h (right):

http://codereview.chromium.org/7316015/diff/6022/base/message_loop_proxy_impl...
base/message_loop_proxy_impl.h:38: const base::Closure& task, int64 delay_ms);
On 2011/07/08 12:04:21, willchan wrote:
> |delay_ms| to next line.

Done.

http://codereview.chromium.org/7316015/diff/6022/base/message_loop_proxy_impl...
base/message_loop_proxy_impl.h:61: const base::Closure& task, int64 delay_ms,
On 2011/07/08 12:04:21, willchan wrote:
> |delay_ms| to next line

Done.

http://codereview.chromium.org/7316015/diff/6022/base/message_loop_proxy_impl...
File base/message_loop_proxy_impl_unittest.cc (right):

http://codereview.chromium.org/7316015/diff/6022/base/message_loop_proxy_impl...
base/message_loop_proxy_impl_unittest.cc:8: #include
"base/message_loop_proxy_impl.h"
On 2011/07/08 12:04:21, willchan wrote:
> This header should be first.

I don't think this rule applies to unittests does it?

http://codereview.chromium.org/7316015/diff/6022/base/task.h
File base/task.h (right):

http://codereview.chromium.org/7316015/diff/6022/base/task.h#newcode576
base/task.h:576: // In particular, notice that the this is RefCounted instead of
On 2011/07/08 12:04:21, willchan wrote:
> Your comment is totally off. This is RefCountedThreadSafe.

Sorry...it's actually supposed to just be RefCounted.  I switched it temporarily
in the last patch as a stab in the dark for the try servers.  Reverting to just
RefCounted.

A different question is whether or not this is a sane thing to do.  IIRC, people
were OCD about extra refcounts inside message loop code which lead this to being
just RefCounted instead of RefCountedThreadSafe when it was first added.

http://codereview.chromium.org/7316015/diff/6022/base/task.h#newcode577
base/task.h:577: // RefCountedThreadSafe.  We rely on the fact thas users of
this class are
On 2011/07/08 12:04:21, willchan wrote:
> s/thas/that/

Done.

http://codereview.chromium.org/7316015/diff/6022/base/threading/worker_pool_p...
File base/threading/worker_pool_posix.cc (right):

http://codereview.chromium.org/7316015/diff/6022/base/threading/worker_pool_p...
base/threading/worker_pool_posix.cc:106: bool WorkerPool::PostTask(const
tracked_objects::Location& from_here,
On 2011/07/08 12:04:21, willchan wrote:
> Minor nit, perhaps not even worth fixing. So don't bother unless you care.
> 
> I'm a bit surprised we don't do the Task/Closure=>PendingTask conversion at
the
> outermost API, which is the static WorkerPool::PostTask() methods. I would
have
> thought we would want to eliminate the two paths at the earliest point
possible.

Yeah. That was my first attempt.  Because Closure is refcounted, you can't just
create a Closure of the task and pass it in on the spot since then you don't
know who will delete it.

Thus, we have to create a PendingTask carrier that can do a destructive pass off
of ownership for "task" safely.

Since the PendingTask handoff has strange semantics (eg., you construct it in
one spot, and then it gets destructively passed into the pool), it felt cleaner
to keep all the ugly stuff in PosixDynamicThreadPool.  That way WorkerPoolImpl
and WorkerPool are nice and thin.

Another question I was wondering about is whether or not we really needed
WorkerPoolImpl...but I didn't want to tackle that in the current change.

http://codereview.chromium.org/7316015/diff/6022/base/threading/worker_pool_p...
base/threading/worker_pool_posix.cc:136:
tracked_objects::ThreadData::TallyADeathIfActive(
On 2011/07/08 12:04:21, willchan wrote:
> These calls are probably what's causing your shutdown crashes. They rely on
> MessageLoop::current(), which uses LazyInstance. The LazyInstance probably
> already got destroyed by the AtExitManager. I suspect ~AtExitManager's body
> destroys the LazyInstance, but in between the window where the Lock is
released
> at the end of the body and before ~AtExitManager begins deleting the member
> variables, this pool recreates MessageLoop::current()'s LazyInstance and
> acquires the lock, so then when it gets destroyed, it aborts since it's
illegal
> to delete a lock while it's held.

Ah.  Interesting...  I can see how that might do that, but the new code should
just be replicating existing behavior since "delete task;" previously should
have invoked the same code path on the Task's destructor.  I'll test by just
removing this and seeing what happens.

http://codereview.chromium.org/7316015/diff/6022/base/tracked_objects.h
File base/tracked_objects.h (right):

http://codereview.chromium.org/7316015/diff/6022/base/tracked_objects.h#newco...
base/tracked_objects.h:495: 
On 2011/07/08 12:04:21, willchan wrote:
> Spurious?

Done.

[willchan no longer on Chromium, 2011-07-10 16:47:32]

http://codereview.chromium.org/7316015/diff/6022/base/message_loop_proxy_impl...
File base/message_loop_proxy_impl_unittest.cc (right):

http://codereview.chromium.org/7316015/diff/6022/base/message_loop_proxy_impl...
base/message_loop_proxy_impl_unittest.cc:8: #include
"base/message_loop_proxy_impl.h"
On 2011/07/08 18:36:51, awong wrote:
> On 2011/07/08 12:04:21, willchan wrote:
> > This header should be first.
> 
> I don't think this rule applies to unittests does it?

Common misconception for old fartz who first read the style guide before the
change happened maybe 2 or 3 years ago:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_...
says: "In dir/foo.cc or dir/foo_test.cc, whose main purpose is to implement or
test the stuff in dir2/foo2.h, order your includes as follows:"

http://codereview.chromium.org/7316015/diff/6022/base/threading/worker_pool_p...
File base/threading/worker_pool_posix.cc (right):

http://codereview.chromium.org/7316015/diff/6022/base/threading/worker_pool_p...
base/threading/worker_pool_posix.cc:106: bool WorkerPool::PostTask(const
tracked_objects::Location& from_here,
On 2011/07/08 18:36:51, awong wrote:
> On 2011/07/08 12:04:21, willchan wrote:
> > Minor nit, perhaps not even worth fixing. So don't bother unless you care.
> > 
> > I'm a bit surprised we don't do the Task/Closure=>PendingTask conversion at
> the
> > outermost API, which is the static WorkerPool::PostTask() methods. I would
> have
> > thought we would want to eliminate the two paths at the earliest point
> possible.
> 
> Yeah. That was my first attempt.  Because Closure is refcounted, you can't
just
> create a Closure of the task and pass it in on the spot since then you don't
> know who will delete it.
> 
> Thus, we have to create a PendingTask carrier that can do a destructive pass
off
> of ownership for "task" safely.
> 
> Since the PendingTask handoff has strange semantics (eg., you construct it in
> one spot, and then it gets destructively passed into the pool), it felt
cleaner
> to keep all the ugly stuff in PosixDynamicThreadPool.  That way WorkerPoolImpl
> and WorkerPool are nice and thin.

OK, I see. That makes sense. Comments explaining the destructive handoff would
be nice.

> 
> Another question I was wondering about is whether or not we really needed
> WorkerPoolImpl...but I didn't want to tackle that in the current change.

It's not clear to me what the alternative is...make each member variable be
global itself?

http://codereview.chromium.org/7316015/diff/6022/base/threading/worker_pool_p...
base/threading/worker_pool_posix.cc:136:
tracked_objects::ThreadData::TallyADeathIfActive(
On 2011/07/08 18:36:51, awong wrote:
> On 2011/07/08 12:04:21, willchan wrote:
> > These calls are probably what's causing your shutdown crashes. They rely on
> > MessageLoop::current(), which uses LazyInstance. The LazyInstance probably
> > already got destroyed by the AtExitManager. I suspect ~AtExitManager's body
> > destroys the LazyInstance, but in between the window where the Lock is
> released
> > at the end of the body and before ~AtExitManager begins deleting the member
> > variables, this pool recreates MessageLoop::current()'s LazyInstance and
> > acquires the lock, so then when it gets destroyed, it aborts since it's
> illegal
> > to delete a lock while it's held.
> 
> Ah.  Interesting...  I can see how that might do that, but the new code should
> just be replicating existing behavior since "delete task;" previously should
> have invoked the same code path on the Task's destructor.  I'll test by just
> removing this and seeing what happens.

Looks like the Mac trybot run passed. Try again to see if it's flaky?

http://codereview.chromium.org/7316015/diff/12023/base/message_loop.cc
File base/message_loop.cc (right):

http://codereview.chromium.org/7316015/diff/12023/base/message_loop.cc#newcod...
base/message_loop.cc:242: new base::subtle::TaskClosureAdapter(task,
&should_leak_tasks_)),
I was looking at the refcounted nature of TaskClosureAdapter and was wondering
about whether or not we should revisit this. It looks like it's refcounted so we
can just copy PendingTask. Copying PendingTask saved a heap allocation before,
but now that we have a TaskClosureAdapter, I don't know if it does. Perhaps
rather than making TaskClosureAdapter copyable, we should just making
PendingTask heap allocated. We wouldn't have to heap allocate TaskClosureAdapter
then.

I haven't completely vetted the code to see if this is possible. But offhand, it
looks possible.

http://codereview.chromium.org/7316015/diff/12023/base/task.h
File base/task.h (right):

http://codereview.chromium.org/7316015/diff/12023/base/task.h#newcode576
base/task.h:576: // In particular, notice that the this is RefCounted instead of
"notice that the this is"? s/the //?

http://codereview.chromium.org/7316015/diff/12023/base/threading/worker_pool_...
File base/threading/worker_pool_win.cc (right):

http://codereview.chromium.org/7316015/diff/12023/base/threading/worker_pool_...
base/threading/worker_pool_win.cc:60: ULONG flags = 0;
Maybe rather than copy/pasting we should use a function?

[akalin, 2011-07-14 00:50:56]

You may want to fix the clang errors:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
(try job for my patch which is based on this)

[awong, 2011-07-14 23:03:30]

Okay, I gave up and just removed the object tracking code for right now.

Any objections to that?

http://codereview.chromium.org/7316015/diff/6022/base/message_loop_proxy_impl...
File base/message_loop_proxy_impl_unittest.cc (right):

http://codereview.chromium.org/7316015/diff/6022/base/message_loop_proxy_impl...
base/message_loop_proxy_impl_unittest.cc:8: #include
"base/message_loop_proxy_impl.h"
On 2011/07/10 16:47:33, willchan wrote:
> On 2011/07/08 18:36:51, awong wrote:
> > On 2011/07/08 12:04:21, willchan wrote:
> > > This header should be first.
> > 
> > I don't think this rule applies to unittests does it?
> 
> Common misconception for old fartz who first read the style guide before the
> change happened maybe 2 or 3 years ago:
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_...
> says: "In dir/foo.cc or dir/foo_test.cc, whose main purpose is to implement or
> test the stuff in dir2/foo2.h, order your includes as follows:"

Done.

(And I'm not old!)

http://codereview.chromium.org/7316015/diff/6022/base/threading/worker_pool_p...
File base/threading/worker_pool_posix.cc (right):

http://codereview.chromium.org/7316015/diff/6022/base/threading/worker_pool_p...
base/threading/worker_pool_posix.cc:106: bool WorkerPool::PostTask(const
tracked_objects::Location& from_here,
On 2011/07/10 16:47:33, willchan wrote:
> On 2011/07/08 18:36:51, awong wrote:
> > On 2011/07/08 12:04:21, willchan wrote:
> > > Minor nit, perhaps not even worth fixing. So don't bother unless you care.
> > > 
> > > I'm a bit surprised we don't do the Task/Closure=>PendingTask conversion
at
> > the
> > > outermost API, which is the static WorkerPool::PostTask() methods. I would
> > have
> > > thought we would want to eliminate the two paths at the earliest point
> > possible.
> > 
> > Yeah. That was my first attempt.  Because Closure is refcounted, you can't
> just
> > create a Closure of the task and pass it in on the spot since then you don't
> > know who will delete it.
> > 
> > Thus, we have to create a PendingTask carrier that can do a destructive pass
> off
> > of ownership for "task" safely.
> > 
> > Since the PendingTask handoff has strange semantics (eg., you construct it
in
> > one spot, and then it gets destructively passed into the pool), it felt
> cleaner
> > to keep all the ugly stuff in PosixDynamicThreadPool.  That way
WorkerPoolImpl
> > and WorkerPool are nice and thin.
> 
> OK, I see. That makes sense. Comments explaining the destructive handoff would
> be nice.

Added a comment into one of the PostTasks.

> 
> > 
> > Another question I was wondering about is whether or not we really needed
> > WorkerPoolImpl...but I didn't want to tackle that in the current change.
> 
> It's not clear to me what the alternative is...make each member variable be
> global itself?

Make  g_lazy_worker_pool be base::LazyInstance<PosixDynamicThreadPool>
g_lazy_worker_pool(base::LINKER_INITIALIZED);

Let PosixDynamicThreadPool's default constructor hard-code "WorkerPool" and
kIdleSecondsBeforeExit.  Then have WorkerPool::PostTask call
PosixDynamicThreadPool::PostTask directly (dropping the task_is_slow flag).

http://codereview.chromium.org/7316015/diff/6022/base/threading/worker_pool_p...
base/threading/worker_pool_posix.cc:136:
tracked_objects::ThreadData::TallyADeathIfActive(
On 2011/07/10 16:47:33, willchan wrote:
> On 2011/07/08 18:36:51, awong wrote:
> > On 2011/07/08 12:04:21, willchan wrote:
> > > These calls are probably what's causing your shutdown crashes. They rely
on
> > > MessageLoop::current(), which uses LazyInstance. The LazyInstance probably
> > > already got destroyed by the AtExitManager. I suspect ~AtExitManager's
body
> > > destroys the LazyInstance, but in between the window where the Lock is
> > released
> > > at the end of the body and before ~AtExitManager begins deleting the
member
> > > variables, this pool recreates MessageLoop::current()'s LazyInstance and
> > > acquires the lock, so then when it gets destroyed, it aborts since it's
> > illegal
> > > to delete a lock while it's held.
> > 
> > Ah.  Interesting...  I can see how that might do that, but the new code
should
> > just be replicating existing behavior since "delete task;" previously should
> > have invoked the same code path on the Task's destructor.  I'll test by just
> > removing this and seeing what happens.
> 
> Looks like the Mac trybot run passed. Try again to see if it's flaky?

I'm pretty consistently getting an error when the object tracking code is
enabled. However, I can't repro locally, and I can't see any code path by which
there should be a problem.  Maybe I should just remove the tracking from
workerpool for right now...does that seem okay to you?

http://codereview.chromium.org/7316015/diff/12023/base/message_loop.cc
File base/message_loop.cc (right):

http://codereview.chromium.org/7316015/diff/12023/base/message_loop.cc#newcod...
base/message_loop.cc:242: new base::subtle::TaskClosureAdapter(task,
&should_leak_tasks_)),
On 2011/07/10 16:47:33, willchan wrote:
> I was looking at the refcounted nature of TaskClosureAdapter and was wondering
> about whether or not we should revisit this. It looks like it's refcounted so
we
> can just copy PendingTask. Copying PendingTask saved a heap allocation before,
> but now that we have a TaskClosureAdapter, I don't know if it does. Perhaps
> rather than making TaskClosureAdapter copyable, we should just making
> PendingTask heap allocated. We wouldn't have to heap allocate
TaskClosureAdapter
> then.
> 
> I haven't completely vetted the code to see if this is possible. But offhand,
it
> looks possible.

I'm not quite sure how this would work.  The reason we need it refcounted is to
allow Bind() to create something that PendingTask can hold in a base::Closure. 
If we made PendingClosure heap allocated, that might help in avoiding the need
to do the weird destructive handoff, but I don't think it actually saves us from
refcounting TaskClosureAdapter.

http://codereview.chromium.org/7316015/diff/12023/base/task.h
File base/task.h (right):

http://codereview.chromium.org/7316015/diff/12023/base/task.h#newcode576
base/task.h:576: // In particular, notice that the this is RefCounted instead of
On 2011/07/10 16:47:33, willchan wrote:
> "notice that the this is"? s/the //?

Done.

http://codereview.chromium.org/7316015/diff/12023/base/threading/worker_pool_...
File base/threading/worker_pool_win.cc (right):

http://codereview.chromium.org/7316015/diff/12023/base/threading/worker_pool_...
base/threading/worker_pool_win.cc:60: ULONG flags = 0;
On 2011/07/10 16:47:33, willchan wrote:
> Maybe rather than copy/pasting we should use a function?

You mean make PendingTask more global?  I'm not sure it's worth it...

[awong, 2011-07-18 19:49:54]

ping...ready for another review.

[willchan no longer on Chromium, 2011-07-19 12:18:26]

LGTM, actually I defer the controversial point to Darin :)

http://codereview.chromium.org/7316015/diff/6022/base/message_loop_proxy_impl...
File base/message_loop_proxy_impl_unittest.cc (right):

http://codereview.chromium.org/7316015/diff/6022/base/message_loop_proxy_impl...
base/message_loop_proxy_impl_unittest.cc:8: #include
"base/message_loop_proxy_impl.h"
On 2011/07/14 23:03:30, awong wrote:
> On 2011/07/10 16:47:33, willchan wrote:
> > On 2011/07/08 18:36:51, awong wrote:
> > > On 2011/07/08 12:04:21, willchan wrote:
> > > > This header should be first.
> > > 
> > > I don't think this rule applies to unittests does it?
> > 
> > Common misconception for old fartz who first read the style guide before the
> > change happened maybe 2 or 3 years ago:
> > 
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_...
> > says: "In dir/foo.cc or dir/foo_test.cc, whose main purpose is to implement
or
> > test the stuff in dir2/foo2.h, order your includes as follows:"
> 
> Done.
> 
> (And I'm not old!)

In Google years you are pretty old now :)

http://codereview.chromium.org/7316015/diff/6022/base/threading/worker_pool_p...
File base/threading/worker_pool_posix.cc (right):

http://codereview.chromium.org/7316015/diff/6022/base/threading/worker_pool_p...
base/threading/worker_pool_posix.cc:136:
tracked_objects::ThreadData::TallyADeathIfActive(
On 2011/07/14 23:03:30, awong wrote:
> On 2011/07/10 16:47:33, willchan wrote:
> > On 2011/07/08 18:36:51, awong wrote:
> > > On 2011/07/08 12:04:21, willchan wrote:
> > > > These calls are probably what's causing your shutdown crashes. They rely
> on
> > > > MessageLoop::current(), which uses LazyInstance. The LazyInstance
probably
> > > > already got destroyed by the AtExitManager. I suspect ~AtExitManager's
> body
> > > > destroys the LazyInstance, but in between the window where the Lock is
> > > released
> > > > at the end of the body and before ~AtExitManager begins deleting the
> member
> > > > variables, this pool recreates MessageLoop::current()'s LazyInstance and
> > > > acquires the lock, so then when it gets destroyed, it aborts since it's
> > > illegal
> > > > to delete a lock while it's held.
> > > 
> > > Ah.  Interesting...  I can see how that might do that, but the new code
> should
> > > just be replicating existing behavior since "delete task;" previously
should
> > > have invoked the same code path on the Task's destructor.  I'll test by
just
> > > removing this and seeing what happens.
> > 
> > Looks like the Mac trybot run passed. Try again to see if it's flaky?
> 
> I'm pretty consistently getting an error when the object tracking code is
> enabled. However, I can't repro locally, and I can't see any code path by
which
> there should be a problem.  Maybe I should just remove the tracking from
> workerpool for right now...does that seem okay to you?

Suboptimal, but I'm OK with it. I don't use it. I defer to Darin on whether this
is OK. I bet he defers to jar@.

http://codereview.chromium.org/7316015/diff/12023/base/message_loop.cc
File base/message_loop.cc (right):

http://codereview.chromium.org/7316015/diff/12023/base/message_loop.cc#newcod...
base/message_loop.cc:242: new base::subtle::TaskClosureAdapter(task,
&should_leak_tasks_)),
On 2011/07/14 23:03:30, awong wrote:
> On 2011/07/10 16:47:33, willchan wrote:
> > I was looking at the refcounted nature of TaskClosureAdapter and was
wondering
> > about whether or not we should revisit this. It looks like it's refcounted
so
> we
> > can just copy PendingTask. Copying PendingTask saved a heap allocation
before,
> > but now that we have a TaskClosureAdapter, I don't know if it does. Perhaps
> > rather than making TaskClosureAdapter copyable, we should just making
> > PendingTask heap allocated. We wouldn't have to heap allocate
> TaskClosureAdapter
> > then.
> > 
> > I haven't completely vetted the code to see if this is possible. But
offhand,
> it
> > looks possible.
> 
> I'm not quite sure how this would work.  The reason we need it refcounted is
to
> allow Bind() to create something that PendingTask can hold in a base::Closure.

> If we made PendingClosure heap allocated, that might help in avoiding the need
> to do the weird destructive handoff, but I don't think it actually saves us
from
> refcounting TaskClosureAdapter.

I started writing out how this would work, but the benefits aren't a clear win,
and this is code that hopefully will die in Not Too Long (TM). So I'm fine with
not changing things.

http://codereview.chromium.org/7316015/diff/12023/base/threading/worker_pool_...
File base/threading/worker_pool_win.cc (right):

http://codereview.chromium.org/7316015/diff/12023/base/threading/worker_pool_...
base/threading/worker_pool_win.cc:60: ULONG flags = 0;
On 2011/07/14 23:03:30, awong wrote:
> On 2011/07/10 16:47:33, willchan wrote:
> > Maybe rather than copy/pasting we should use a function?
> 
> You mean make PendingTask more global?  I'm not sure it's worth it...

I mean taking lines 61-67 and pulling it out into a function in the anonymous
namespace, so the other PostTask() overload can use it. This is minor, so if you
don't want to, no biggie.

http://codereview.chromium.org/7316015/diff/26001/base/task.h
File base/task.h (right):

http://codereview.chromium.org/7316015/diff/26001/base/task.h#newcode567
base/task.h:567: namespace subtle {
<bikeshed>I think internal is more appropriate</bikeshed>

http://codereview.chromium.org/7316015/diff/26001/base/task.h#newcode571
base/task.h:571: // implement the compatibiltiy APIs while we are transitioning
from Task to
s/compatibiltiy/compatibility/

[awong, 2011-07-19 21:08:20]

Addressed Will's comments.

Darin, do you want to chime in on me killing object tracking in workerpool for
now?

http://codereview.chromium.org/7316015/diff/12023/base/threading/worker_pool_...
File base/threading/worker_pool_win.cc (right):

http://codereview.chromium.org/7316015/diff/12023/base/threading/worker_pool_...
base/threading/worker_pool_win.cc:60: ULONG flags = 0;
On 2011/07/19 12:18:26, willchan wrote:
> On 2011/07/14 23:03:30, awong wrote:
> > On 2011/07/10 16:47:33, willchan wrote:
> > > Maybe rather than copy/pasting we should use a function?
> > 
> > You mean make PendingTask more global?  I'm not sure it's worth it...
> 
> I mean taking lines 61-67 and pulling it out into a function in the anonymous
> namespace, so the other PostTask() overload can use it. This is minor, so if
you
> don't want to, no biggie.

Ah.  That makes more sense.

Done.

http://codereview.chromium.org/7316015/diff/26001/base/task.h
File base/task.h (right):

http://codereview.chromium.org/7316015/diff/26001/base/task.h#newcode567
base/task.h:567: namespace subtle {
On 2011/07/19 12:18:26, willchan wrote:
> <bikeshed>I think internal is more appropriate</bikeshed>

I feel like subtle scares me people more. And this thing has all sorts of weird
semantics.

[akalin, 2011-07-20 07:52:03]

http://codereview.chromium.org/7316015/diff/35001/base/message_loop_proxy.h
File base/message_loop_proxy.h (right):

http://codereview.chromium.org/7316015/diff/35001/base/message_loop_proxy.h#n...
base/message_loop_proxy.h:11: #include "base/callback.h"
Can you remove this and instead forward-declare Callback and Closure?

You can do:

template <typename Sig> Callback<Sig>;
typedef Callback<void()> Closure;

Otherwise, bind.h will end up pulling callback.h with my WeakHandle change.

bind.h needs to #include weak_handle.h for support for the special syntax. 
weak_handle.h needs to #include message_loop_proxy.h because it needs to delete
the weak ptr on the correct thread, so it needs access to the (templated)
DeleteSoon method.  And so if message_loop_proxy.h needs callback.h, then bind.h
will end up including callback.h.

An alternate solution would be for me to remove the #include for weak_ptr.h and
weak_handle.h.  I'm not familiar with the details of template expansion, but it
seems like the definitions of WeakPtr/WeakHandle, etc. would need to be present
only at template instantation time, i.e. when you actually Bind() a WeakHandle,
aand if you're binding a WeakHandle, then you must have already included
weak_handle.h.  Unless you've forward-declared WeakHandle and are binding a
const ref, I guess?  I dunno.

I'll temporarily proceed with replacing this #include with a forward-declare in
my CL, but what do you think?

[akalin, 2011-07-20 07:52:43]

That should be:

template <typename Sig> class Callback<Sig>;
typedef Callback<void()> Closure;

[akalin, 2011-07-20 07:53:31]

Or more correctly:

template <typename Sig> class Callback<Sig>;
typedef Callback<void(void)> Closure;

[akalin, 2011-07-20 07:54:06]

On 2011/07/20 07:53:31, akalin wrote:
> Or more correctly:
> 
> template <typename Sig> class Callback<Sig>;
> typedef Callback<void(void)> Closure;

You also don't need to base::-qualify the references to Closure in
message_loop_callback.*, since you're already in base.

[awong, 2011-07-20 21:17:41]

Darin, soft ping.

Note that this will block the WeakHandle CL.

http://codereview.chromium.org/7316015/diff/35001/base/message_loop_proxy.h
File base/message_loop_proxy.h (right):

http://codereview.chromium.org/7316015/diff/35001/base/message_loop_proxy.h#n...
base/message_loop_proxy.h:11: #include "base/callback.h"
On 2011/07/20 07:52:03, akalin wrote:
> Can you remove this and instead forward-declare Callback and Closure?
> 
> You can do:
> 
> template <typename Sig> Callback<Sig>;
> typedef Callback<void()> Closure;
> 
> Otherwise, bind.h will end up pulling callback.h with my WeakHandle change.
> 
> bind.h needs to #include weak_handle.h for support for the special syntax. 
> weak_handle.h needs to #include message_loop_proxy.h because it needs to
delete
> the weak ptr on the correct thread, so it needs access to the (templated)
> DeleteSoon method.  And so if message_loop_proxy.h needs callback.h, then
bind.h
> will end up including callback.h.
> 
> An alternate solution would be for me to remove the #include for weak_ptr.h
and
> weak_handle.h.  I'm not familiar with the details of template expansion, but
it
> seems like the definitions of WeakPtr/WeakHandle, etc. would need to be
present
> only at template instantation time, i.e. when you actually Bind() a
WeakHandle,
> aand if you're binding a WeakHandle, then you must have already included
> weak_handle.h.  Unless you've forward-declared WeakHandle and are binding a
> const ref, I guess?  I dunno.
> 
> I'll temporarily proceed with replacing this #include with a forward-declare
in
> my CL, but what do you think?


Talked with Fred offline.  We realized that the main point of keeping callback.h
out of bind.h was so that bind.h wouldn't provide the Callback<> and Closure
types, thus deterring people from using bind.h just to get the correct types.
So, forward declaring is equivalent to including.

I'm going to leave this as is for now.  We'll try to solve the WeakHandle issue
separately.

[akalin, 2011-07-22 17:11:33]

On 2011/07/20 21:17:41, awong wrote:
> Darin, soft ping.

LGTM

Ping, Darin?

[darin (slow to review), 2011-07-25 19:47:37]

LGTM

http://codereview.chromium.org/7316015/diff/35001/base/threading/worker_pool_...
File base/threading/worker_pool_posix.h (right):

http://codereview.chromium.org/7316015/diff/35001/base/threading/worker_pool_...
base/threading/worker_pool_posix.h:44: class BASE_API PosixDynamicThreadPool
note: this class name doesn't match the file name.  it seems like this should
live in files named posix_dynamic_thread_pool.{h,cc}

http://codereview.chromium.org/7316015/diff/35001/base/threading/worker_pool_...
base/threading/worker_pool_posix.h:82: // Worker thread method to wait for up to
|idle_seconds_before_exit| for more
nit: add a new line above this comment.

[awong, 2011-07-26 01:38:13]

thanks everyone!

Going to run through try bots and then try to commit.

http://codereview.chromium.org/7316015/diff/35001/base/threading/worker_pool_...
File base/threading/worker_pool_posix.h (right):

http://codereview.chromium.org/7316015/diff/35001/base/threading/worker_pool_...
base/threading/worker_pool_posix.h:44: class BASE_API PosixDynamicThreadPool
On 2011/07/25 19:47:37, darin wrote:
> note: this class name doesn't match the file name.  it seems like this should
> live in files named posix_dynamic_thread_pool.{h,cc}

Yeah...will touch that in another CL.

http://codereview.chromium.org/7316015/diff/35001/base/threading/worker_pool_...
base/threading/worker_pool_posix.h:82: // Worker thread method to wait for up to
|idle_seconds_before_exit| for more
On 2011/07/25 19:47:37, darin wrote:
> nit: add a new line above this comment.

Done.

[commit-bot: I haz the power, 2011-07-26 18:25:19]

Change committed as 94129