Add support for base::Closure in the MessageLoop.

Add support for base::Closure in the MessageLoop, and reimplement the whole sucker on top of base::Closure.  After this, all Task objects that are posted will be wrapped in a closure prior to dispatch.

BUG=35223
TEST=unittests.

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

[awong, 2011-02-09 02:00:08]

Does this look about right?  If so, I'll go mess with the unittests.

[willchan no longer on Chromium, 2011-02-09 05:51:48]

Looks about right.

http://codereview.chromium.org/6463013/diff/1/base/message_loop.cc
File base/message_loop.cc (right):

http://codereview.chromium.org/6463013/diff/1/base/message_loop.cc#newcode242
base/message_loop.cc:242: // TODO(ajwong): Record |from_here| correctly for
Closures.
IIRC, when we discussed this before, I suggested ripping
tracked_objects::Location out of Task (which actually inherits from some sort of
tracked object thingy) and embedding it in the PendingTask itself. This should
hopefully allow a way to fix the TaskObserver too.

[awong, 2011-02-12 09:56:01]

[ +darin ]

I'm thinking of doing the TrackedObjects change in a second CL.  I'm not quite
sure how that'd let us fix TaskObserver quite yet...on the otherhand, according
to codesearch, gpu_watchdog_thread.cc uses TaskObserver and it doesn't even
expect the Task so we could happily send NULL for now.

As for unittests...I just started looking at them.  There are a *lot* of tests. 
Is it worth forking each of them to make it work with Closure at this point, or
should I do that later in the migration?

[awong, 2011-02-13 07:39:53]

I ended up giving a shot at creating a "Closure" version of a lot of these.  I
made it through the majority, but I'm still dubious at whether I should even try
to update the unittests yet -- for the time being, I'm essentially forking
PostTask based tests.  On the otherhand, the code in this file is super lower
churn. Most of it has not been touched since 2008.

Thoughts?

[willchan no longer on Chromium, 2011-02-15 01:41:11]

Have not read all the tests yet. I support forking them now. It doesn't change
often.

http://codereview.chromium.org/6463013/diff/16001/base/message_loop.cc
File base/message_loop.cc (right):

http://codereview.chromium.org/6463013/diff/16001/base/message_loop.cc#newcod...
base/message_loop.cc:240: void MessageLoop::PostClosure(
For all these Post*Closure() functions, you need to assert that the Closure
isn't empty.

http://codereview.chromium.org/6463013/diff/16001/base/message_loop.cc#newcod...
base/message_loop.cc:419: // TODO(ajwong): Figure out how to handle
FOR_EACH_OBSERVER.
Probably should add a ClosureObserver.

http://codereview.chromium.org/6463013/diff/16001/base/message_loop.cc#newcod...
base/message_loop.cc:556: void MessageLoop::AddToIncomingQueue(const
PendingTask& pending_task) {
You should sanity check PendingTask here. task XOR closure.

http://codereview.chromium.org/6463013/diff/16001/base/message_loop_unittest.cc
File base/message_loop_unittest.cc (right):

http://codereview.chromium.org/6463013/diff/16001/base/message_loop_unittest....
base/message_loop_unittest.cc:11: #include "base/message_loop.h"
This should go first.

[darin (slow to review), 2011-02-15 05:05:47]

I believe TaskObserver was modified to pass a Task just so that people could get
access to the tracked_object fields.

http://codereview.chromium.org/6463013/diff/16001/base/message_loop.h
File base/message_loop.h (right):

http://codereview.chromium.org/6463013/diff/16001/base/message_loop.h#newcode378
base/message_loop.h:378: Task* task;                        // The task to run.
It would be nice to implement the old Task-based methods in terms of Closure! :)

[awong, 2011-02-15 08:24:42]

Okay, hold off on reviewing for a bit. 

Per Darin's comment, will redo this CL by completely ripping Task* out and
making Callback the authoritative delayed-execution carrier.  This will require
a few other CLs first.

http://codereview.chromium.org/6463013/diff/16001/base/message_loop.cc
File base/message_loop.cc (right):

http://codereview.chromium.org/6463013/diff/16001/base/message_loop.cc#newcod...
base/message_loop.cc:419: // TODO(ajwong): Figure out how to handle
FOR_EACH_OBSERVER.
On 2011/02/15 01:41:11, willchan wrote:
> Probably should add a ClosureObserver.

Sounds like maybe we should modify the observer to only get the Tracked*, not
Task*.

http://codereview.chromium.org/6463013/diff/16001/base/message_loop.cc#newcod...
base/message_loop.cc:556: void MessageLoop::AddToIncomingQueue(const
PendingTask& pending_task) {
On 2011/02/15 01:41:11, willchan wrote:
> You should sanity check PendingTask here. task XOR closure.

Sounds like Darin is in for cowboying it up and just going wholesale Closure
now.  That will obviate this issue. 

Good times.

http://codereview.chromium.org/6463013/diff/16001/base/message_loop.h
File base/message_loop.h (right):

http://codereview.chromium.org/6463013/diff/16001/base/message_loop.h#newcode378
base/message_loop.h:378: Task* task;                        // The task to run.
On 2011/02/15 05:05:47, darin wrote:
> It would be nice to implement the old Task-based methods in terms of Closure!
:)

Hah...yes it would.  I was initially thinking of a less intrusive first-step.
But, you're right...being conservative here doesn't gain us much.

To confirm, we will be throwing the whole world on top of the Callback<>
implementation now.

[darin (slow to review), 2011-02-15 18:13:19]

On Tue, Feb 15, 2011 at 12:24 AM, <ajwong@chromium.org> wrote:

> Okay, hold off on reviewing for a bit.
>
> Per Darin's comment, will redo this CL by completely ripping Task* out and
> making Callback the authoritative delayed-execution carrier.  This will
> require
> a few other CLs first.
>
>
>
> http://codereview.chromium.org/6463013/diff/16001/base/message_loop.cc
> File base/message_loop.cc (right):
>
>
>
>
http://codereview.chromium.org/6463013/diff/16001/base/message_loop.cc#newcod...
> base/message_loop.cc:419: // TODO(ajwong): Figure out how to handle
> FOR_EACH_OBSERVER.
> On 2011/02/15 01:41:11, willchan wrote:
>
>> Probably should add a ClosureObserver.
>>
>
> Sounds like maybe we should modify the observer to only get the
> Tracked*, not Task*.
>
>
>
>
http://codereview.chromium.org/6463013/diff/16001/base/message_loop.cc#newcod...
> base/message_loop.cc:556: void MessageLoop::AddToIncomingQueue(const
> PendingTask& pending_task) {
> On 2011/02/15 01:41:11, willchan wrote:
>
>> You should sanity check PendingTask here. task XOR closure.
>>
>
> Sounds like Darin is in for cowboying it up and just going wholesale
> Closure now.  That will obviate this issue.
>
> Good times.
>
>
> http://codereview.chromium.org/6463013/diff/16001/base/message_loop.h
> File base/message_loop.h (right):
>
>
>
http://codereview.chromium.org/6463013/diff/16001/base/message_loop.h#newcode378
> base/message_loop.h:378: Task* task;                        // The task
> to run.
> On 2011/02/15 05:05:47, darin wrote:
>
>> It would be nice to implement the old Task-based methods in terms of
>>
> Closure! :)
>
> Hah...yes it would.  I was initially thinking of a less intrusive
> first-step. But, you're right...being conservative here doesn't gain us
> much.
>
> To confirm, we will be throwing the whole world on top of the Callback<>
> implementation now.
>
>
Yes... why not? :-)


>
> http://codereview.chromium.org/6463013/
>

[awong, 2011-03-21 19:34:31]

I think this is ready for another look finally.

There are some valgrind issues that I need to track down.  I think they might
asll be due to having renamed PendingTask to PendingClosure, but need to verify.

[awong, 2011-03-21 23:54:27]

PTAL

Verified that all valgrind issues from the last run were either present in
unrelated try runs, or due to an expectation getting invalidated due to the name
change.

I've rebased and added new test expectations.

Most of this CL is just name switches, and turning classes -> functions in
unittests.  The two possibly contentious changes are:

  1) Changing TaskObserver -> ClosureObserver, making the API take in the post
time rather than the Closure itself.
  2) Replicating the legacy Task leak behavior in TaskClosureAdapter.

For #2 in particular, if there was any guidance on why we needed to leak these,
I'd love to beef up the comments.

-Albert


On 2011/03/21 19:34:31, awong wrote:
> I think this is ready for another look finally.
> 
> There are some valgrind issues that I need to track down.  I think they might
> asll be due to having renamed PendingTask to PendingClosure, but need to
verify.

[willchan no longer on Chromium, 2011-03-22 23:30:45]

http://codereview.chromium.org/6463013/diff/41002/base/message_loop.cc
File base/message_loop.cc (right):

http://codereview.chromium.org/6463013/diff/41002/base/message_loop.cc#newcode7
base/message_loop.cc:7: // This needs to be above the gdk includes because gdk
#defines Status.
We should instead move the platform specific includes lower. I see there's
already a OS_POSIX && !defined(OS_MACOSX) section below.

http://codereview.chromium.org/6463013/diff/41002/base/message_loop.cc#newcode94
base/message_loop.cc:94: // |should_leak_task| points to a flag variable that
can be used to determine
Why do we need to use a pointer? Can't we determine this on startup? Then just
copy the bool into this. A bool may be smaller than a pointer.

http://codereview.chromium.org/6463013/diff/41002/base/message_loop.cc#newcod...
base/message_loop.cc:118: Task* task_;
I sorta feel like this should be a scoped_ptr to indicate ownership. And in the
destructor, check the bool and call .release() if needed.

http://codereview.chromium.org/6463013/diff/41002/base/message_loop.cc#newcod...
base/message_loop.cc:466: #if defined(TRACK_ALL_TASK_OBJECTS)
Where are the corresponding changes to remove this code from base::Tracked?

[awong, 2011-03-23 04:05:45]

http://codereview.chromium.org/6463013/diff/41002/base/message_loop.cc
File base/message_loop.cc (right):

http://codereview.chromium.org/6463013/diff/41002/base/message_loop.cc#newcode7
base/message_loop.cc:7: // This needs to be above the gdk includes because gdk
#defines Status.
On 2011/03/22 23:30:45, willchan wrote:
> We should instead move the platform specific includes lower. I see there's
> already a OS_POSIX && !defined(OS_MACOSX) section below.

Done.

http://codereview.chromium.org/6463013/diff/41002/base/message_loop.cc#newcode94
base/message_loop.cc:94: // |should_leak_task| points to a flag variable that
can be used to determine
On 2011/03/22 23:30:45, willchan wrote:
> Why do we need to use a pointer? Can't we determine this on startup? Then just
> copy the bool into this. A bool may be smaller than a pointer.

It needs to be a pointer because whether or not the task needs to be leaked
depends on the phase at which a closure is "dequeued" in the code below.

If you look in  the DeletePendingClosures function, and compare with the code
was previously there, you'll see that if the closure is in the work_queue, but
has no runtime, then it is leaked.  Is it also leaked if it is in the
deferred_non_nestable_work_queue_.  However, otherwise, it is actually deleted
regardless of the purify/heapcheck settings.

I would love to clean this up, but would like more background on the previous
behavior first.  Also, I am afraid to change this behavior in the same CL as
this one.  Too many things changing at once in subtle code.

http://codereview.chromium.org/6463013/diff/41002/base/message_loop.cc#newcod...
base/message_loop.cc:118: Task* task_;
On 2011/03/22 23:30:45, willchan wrote:
> I sorta feel like this should be a scoped_ptr to indicate ownership. And in
the
> destructor, check the bool and call .release() if needed.

I bounced between the two.  In the end, it seemed like the scoped_ptr was
overkill for this setup -- especially since the lifetime of this pointer is all
sorts of weird.

http://codereview.chromium.org/6463013/diff/41002/base/message_loop.cc#newcod...
base/message_loop.cc:466: #if defined(TRACK_ALL_TASK_OBJECTS)
On 2011/03/22 23:30:45, willchan wrote:
> Where are the corresponding changes to remove this code from base::Tracked?

WorkerPool is not migrated yet so I can't break this out yet.

Can I do in a subsequent CL?

[willchan no longer on Chromium, 2011-03-23 18:01:09]

http://codereview.chromium.org/6463013/diff/41002/base/message_loop.cc
File base/message_loop.cc (right):

http://codereview.chromium.org/6463013/diff/41002/base/message_loop.cc#newcode94
base/message_loop.cc:94: // |should_leak_task| points to a flag variable that
can be used to determine
On 2011/03/23 04:05:45, awong wrote:
> On 2011/03/22 23:30:45, willchan wrote:
> > Why do we need to use a pointer? Can't we determine this on startup? Then
just
> > copy the bool into this. A bool may be smaller than a pointer.
> 
> It needs to be a pointer because whether or not the task needs to be leaked
> depends on the phase at which a closure is "dequeued" in the code below.
> 
> If you look in  the DeletePendingClosures function, and compare with the code
> was previously there, you'll see that if the closure is in the work_queue, but
> has no runtime, then it is leaked.  Is it also leaked if it is in the
> deferred_non_nestable_work_queue_.  However, otherwise, it is actually deleted
> regardless of the purify/heapcheck settings.

I'm not sure if I completely understand. If a Task is run normally, then in
TaskClosureAdapter::Run(), we'll delete it and set |task_| to NULL.

TaskClosureAdapater::~TaskClosureAdapter() will only encounter |task_ != NULL|
if we're in DeletePendingClosures(), right? And we know if we're running under
valgrind/purify/heapchecker on startup, so we can set a bool instead of using a
pointer, right?

> 
> I would love to clean this up, but would like more background on the previous
> behavior first.  Also, I am afraid to change this behavior in the same CL as
> this one.  Too many things changing at once in subtle code.

http://codereview.chromium.org/6463013/diff/41002/base/message_loop.cc#newcod...
base/message_loop.cc:466: #if defined(TRACK_ALL_TASK_OBJECTS)
On 2011/03/23 04:05:45, awong wrote:
> On 2011/03/22 23:30:45, willchan wrote:
> > Where are the corresponding changes to remove this code from base::Tracked?
> 
> WorkerPool is not migrated yet so I can't break this out yet.
> 
> Can I do in a subsequent CL?

My preference would be to do it in the same CL, but I don't feel strongly. You
don't seem like you're going to flake out on doing it, so it's not too big a
deal. It just might make someone wonder where this code came from. It'd look
more obvious in a diff afterward if you were deleting it from somewhere else.

Btw, this won't result in any doublecounting or anything right? Just checking
since you've left the code in base::Tracked.

[awong, 2011-03-23 19:33:01]

http://codereview.chromium.org/6463013/diff/41002/base/message_loop.cc
File base/message_loop.cc (right):

http://codereview.chromium.org/6463013/diff/41002/base/message_loop.cc#newcode94
base/message_loop.cc:94: // |should_leak_task| points to a flag variable that
can be used to determine
On 2011/03/23 18:01:10, willchan wrote:
> On 2011/03/23 04:05:45, awong wrote:
> > On 2011/03/22 23:30:45, willchan wrote:
> > > Why do we need to use a pointer? Can't we determine this on startup? Then
> just
> > > copy the bool into this. A bool may be smaller than a pointer.
> > 
> > It needs to be a pointer because whether or not the task needs to be leaked
> > depends on the phase at which a closure is "dequeued" in the code below.
> > 
> > If you look in  the DeletePendingClosures function, and compare with the
code
> > was previously there, you'll see that if the closure is in the work_queue,
but
> > has no runtime, then it is leaked.  Is it also leaked if it is in the
> > deferred_non_nestable_work_queue_.  However, otherwise, it is actually
deleted
> > regardless of the purify/heapcheck settings.
> 
> I'm not sure if I completely understand. If a Task is run normally, then in
> TaskClosureAdapter::Run(), we'll delete it and set |task_| to NULL.
> 
> TaskClosureAdapater::~TaskClosureAdapter() will only encounter |task_ != NULL|
> if we're in DeletePendingClosures(), right? And we know if we're running under
> valgrind/purify/heapchecker on startup, so we can set a bool instead of using
a
> pointer, right?

Not quite.  In DeletePendingClosure(), there seems to be a careful dance to
shuffle tasks from the work_queue to the delayed_work_queue.  During this, they
actually leak the tasks where tasks.delayed_run_time() == NULL.

For the other tasks though, they don't delete things.  Also, the comment below
makes it look like the order of the deletes matter.

> > 
> > I would love to clean this up, but would like more background on the
previous
> > behavior first.  Also, I am afraid to change this behavior in the same CL as
> > this one.  Too many things changing at once in subtle code.
>

http://codereview.chromium.org/6463013/diff/41002/base/message_loop.cc#newcod...
base/message_loop.cc:466: #if defined(TRACK_ALL_TASK_OBJECTS)
On 2011/03/23 18:01:10, willchan wrote:
> On 2011/03/23 04:05:45, awong wrote:
> > On 2011/03/22 23:30:45, willchan wrote:
> > > Where are the corresponding changes to remove this code from
base::Tracked?
> > 
> > WorkerPool is not migrated yet so I can't break this out yet.
> > 
> > Can I do in a subsequent CL?
> 
> My preference would be to do it in the same CL, but I don't feel strongly. You
> don't seem like you're going to flake out on doing it, so it's not too big a
> deal. It just might make someone wonder where this code came from. It'd look
> more obvious in a diff afterward if you were deleting it from somewhere else.
> 
> Btw, this won't result in any doublecounting or anything right? Just checking
> since you've left the code in base::Tracked.

Fixing WorkerPool will take a bit of works since I'll need to create the
equivalent of PendingClosure to own the tracked object.  How's this sound...I'll
try to fix it in parallel with waiting for Darin's review.  If he's not back
before I migrate WorkerPool, then I'll add it to this CL. Otherwise I'll do it
separately.

[willchan no longer on Chromium, 2011-03-23 19:41:56]

LGTM

http://codereview.chromium.org/6463013/diff/41002/base/message_loop.cc
File base/message_loop.cc (right):

http://codereview.chromium.org/6463013/diff/41002/base/message_loop.cc#newcode94
base/message_loop.cc:94: // |should_leak_task| points to a flag variable that
can be used to determine
Hah! I missed that little subtlety. I don't see why we treat delayed tasks
differently. What you have now is fine, although I don't think we should do that
in general, but that doesn't belong in this changelist.

On 2011/03/23 19:33:01, awong wrote:
> On 2011/03/23 18:01:10, willchan wrote:
> > On 2011/03/23 04:05:45, awong wrote:
> > > On 2011/03/22 23:30:45, willchan wrote:
> > > > Why do we need to use a pointer? Can't we determine this on startup?
Then
> > just
> > > > copy the bool into this. A bool may be smaller than a pointer.
> > > 
> > > It needs to be a pointer because whether or not the task needs to be
leaked
> > > depends on the phase at which a closure is "dequeued" in the code below.
> > > 
> > > If you look in  the DeletePendingClosures function, and compare with the
> code
> > > was previously there, you'll see that if the closure is in the work_queue,
> but
> > > has no runtime, then it is leaked.  Is it also leaked if it is in the
> > > deferred_non_nestable_work_queue_.  However, otherwise, it is actually
> deleted
> > > regardless of the purify/heapcheck settings.
> > 
> > I'm not sure if I completely understand. If a Task is run normally, then in
> > TaskClosureAdapter::Run(), we'll delete it and set |task_| to NULL.
> > 
> > TaskClosureAdapater::~TaskClosureAdapter() will only encounter |task_ !=
NULL|
> > if we're in DeletePendingClosures(), right? And we know if we're running
under
> > valgrind/purify/heapchecker on startup, so we can set a bool instead of
using
> a
> > pointer, right?
> 
> Not quite.  In DeletePendingClosure(), there seems to be a careful dance to
> shuffle tasks from the work_queue to the delayed_work_queue.  During this,
they
> actually leak the tasks where tasks.delayed_run_time() == NULL.
> 
> For the other tasks though, they don't delete things.  Also, the comment below
> makes it look like the order of the deletes matter.
> 
> > > 
> > > I would love to clean this up, but would like more background on the
> previous
> > > behavior first.  Also, I am afraid to change this behavior in the same CL
as
> > > this one.  Too many things changing at once in subtle code.
> > 
>

http://codereview.chromium.org/6463013/diff/41002/base/message_loop.cc#newcod...
base/message_loop.cc:466: #if defined(TRACK_ALL_TASK_OBJECTS)
On 2011/03/23 19:33:01, awong wrote:
> On 2011/03/23 18:01:10, willchan wrote:
> > On 2011/03/23 04:05:45, awong wrote:
> > > On 2011/03/22 23:30:45, willchan wrote:
> > > > Where are the corresponding changes to remove this code from
> base::Tracked?
> > > 
> > > WorkerPool is not migrated yet so I can't break this out yet.
> > > 
> > > Can I do in a subsequent CL?
> > 
> > My preference would be to do it in the same CL, but I don't feel strongly.
You
> > don't seem like you're going to flake out on doing it, so it's not too big a
> > deal. It just might make someone wonder where this code came from. It'd look
> > more obvious in a diff afterward if you were deleting it from somewhere
else.
> > 
> > Btw, this won't result in any doublecounting or anything right? Just
checking
> > since you've left the code in base::Tracked.
> 
> Fixing WorkerPool will take a bit of works since I'll need to create the
> equivalent of PendingClosure to own the tracked object.  How's this
sound...I'll
> try to fix it in parallel with waiting for Darin's review.  If he's not back
> before I migrate WorkerPool, then I'll add it to this CL. Otherwise I'll do it
> separately.

Fine with me.

[awong, 2011-04-08 20:33:08]

Darin-san...review おねがいします... 

＾_＾

[darin (slow to review), 2011-04-09 04:21:58]

LGTM

http://codereview.chromium.org/6463013/diff/46002/base/message_loop.cc
File base/message_loop.cc (right):

http://codereview.chromium.org/6463013/diff/46002/base/message_loop.cc#newcod...
base/message_loop.cc:748: delayed_run_time(delayed_run_time), sequence_num(0),
nit: new line before sequence_num(0)

[awong, 2011-04-10 15:58:12]

addressed nit. Added one question, and running through try bots.

http://codereview.chromium.org/6463013/diff/46002/base/message_loop.cc
File base/message_loop.cc (right):

http://codereview.chromium.org/6463013/diff/46002/base/message_loop.cc#newcod...
base/message_loop.cc:547: // absolutely "correct" behavior.
Hey Darin, do you know the rationale behind why we do this funky dance in
DeletePendingClosure() where we leak

  1) tasks in the work_queue_ w/o a delayed_run_time
  2) all tasks in the deferred_non_nestable_work_queue_

but not the others?

I would have expected that you would either leaked all tasks in all queues or
deleted all tasks.

I think we're leaking the tasks out of work_queue_ because we can't reschedule
them w/o a delayed_run_time.  I don't understand why we're leaking tasks from
the deferred_non_nestable_work_queue_.

If you have any insight into this algorithm, I'd like to update this comment
with the rationale for why we have this behavior.

http://codereview.chromium.org/6463013/diff/46002/base/message_loop.cc#newcod...
base/message_loop.cc:748: delayed_run_time(delayed_run_time), sequence_num(0),
On 2011/04/09 04:21:58, darin wrote:
> nit: new line before sequence_num(0)

Done.

[darin (slow to review), 2011-04-10 22:26:46]

On Sun, Apr 10, 2011 at 8:58 AM, <ajwong@chromium.org> wrote:

> addressed nit. Added one question, and running through try bots.
>
>
>
> http://codereview.chromium.org/6463013/diff/46002/base/message_loop.cc
> File base/message_loop.cc (right):
>
>
>
http://codereview.chromium.org/6463013/diff/46002/base/message_loop.cc#newcod...
> base/message_loop.cc:547: // absolutely "correct" behavior.
> Hey Darin, do you know the rationale behind why we do this funky dance
> in DeletePendingClosure() where we leak
>
>  1) tasks in the work_queue_ w/o a delayed_run_time
>  2) all tasks in the deferred_non_nestable_work_queue_
>

I tried (years ago now!) deleting all tasks when the ML is destroyed, but
that caused
regressions on chrome-bot.  The stack traces were unhelpful, and I was never
able to
get at the root cause of the issue.  It may well have just been tickling
race conditions
that were fixed subsequently (e.g., unsafe reference counting in the history
system).

It is worth a shot to try destroying all pending tasks when the ML is
destroyed.  It
may just work now.

-Darin




>
> but not the others?
>
> I would have expected that you would either leaked all tasks in all
> queues or deleted all tasks.
>
> I think we're leaking the tasks out of work_queue_ because we can't
> reschedule them w/o a delayed_run_time.  I don't understand why we're
> leaking tasks from the deferred_non_nestable_work_queue_.
>
> If you have any insight into this algorithm, I'd like to update this
> comment with the rationale for why we have this behavior.
>
>
>
>
http://codereview.chromium.org/6463013/diff/46002/base/message_loop.cc#newcod...
> base/message_loop.cc:748: delayed_run_time(delayed_run_time),
> sequence_num(0),
> On 2011/04/09 04:21:58, darin wrote:
>
>> nit: new line before sequence_num(0)
>>
>
> Done.
>
>
> http://codereview.chromium.org/6463013/
>

[darin (slow to review), 2011-04-10 22:27:48]

See also:
http://code.google.com/p/chromium/issues/detail?id=61131

On Sun, Apr 10, 2011 at 3:26 PM, Darin Fisher <darin@chromium.org> wrote:

>
>
> On Sun, Apr 10, 2011 at 8:58 AM, <ajwong@chromium.org> wrote:
>
>> addressed nit. Added one question, and running through try bots.
>>
>>
>>
>> http://codereview.chromium.org/6463013/diff/46002/base/message_loop.cc
>> File base/message_loop.cc (right):
>>
>>
>>
http://codereview.chromium.org/6463013/diff/46002/base/message_loop.cc#newcod...
>> base/message_loop.cc:547: // absolutely "correct" behavior.
>> Hey Darin, do you know the rationale behind why we do this funky dance
>> in DeletePendingClosure() where we leak
>>
>>  1) tasks in the work_queue_ w/o a delayed_run_time
>>  2) all tasks in the deferred_non_nestable_work_queue_
>>
>
> I tried (years ago now!) deleting all tasks when the ML is destroyed, but
> that caused
> regressions on chrome-bot.  The stack traces were unhelpful, and I was
> never able to
> get at the root cause of the issue.  It may well have just been tickling
> race conditions
> that were fixed subsequently (e.g., unsafe reference counting in the
> history system).
>
> It is worth a shot to try destroying all pending tasks when the ML is
> destroyed.  It
> may just work now.
>
> -Darin
>
>
>
>
>>
>> but not the others?
>>
>> I would have expected that you would either leaked all tasks in all
>> queues or deleted all tasks.
>>
>> I think we're leaking the tasks out of work_queue_ because we can't
>> reschedule them w/o a delayed_run_time.  I don't understand why we're
>> leaking tasks from the deferred_non_nestable_work_queue_.
>>
>> If you have any insight into this algorithm, I'd like to update this
>> comment with the rationale for why we have this behavior.
>>
>>
>>
>>
http://codereview.chromium.org/6463013/diff/46002/base/message_loop.cc#newcod...
>> base/message_loop.cc:748: delayed_run_time(delayed_run_time),
>> sequence_num(0),
>> On 2011/04/09 04:21:58, darin wrote:
>>
>>> nit: new line before sequence_num(0)
>>>
>>
>> Done.
>>
>>
>> http://codereview.chromium.org/6463013/
>>
>
>

[Mike Belshe, 2011-04-13 03:44:12]

Sorry to be negative, but I think this change is a step in the wrong direction. 
The code probably works, and I'm certain that it is nothing short of genius, but
its not readable.  It may well be that I'm not a good enough programmer, but I
think others will react similarly.  As an analogy, if someone offered to
implement the MessageLoop in assembly, we'd reject it due to un-maintainability.
 I argue that this code is similar, and should not be checked in.

The template code in callback.h has always been bad.  And when you hit it with
the debugger, its hard to step through.  But we have managed through it, and it
has been relatively simple templates compared to this new code.  The Closure
code is an order of magnitude more complicated (so complicated that it needs a
python script to generate it!), and as I look at it, I have no clue what it
does.  I simply can't read it.

The notion of a "Closure" also seems like a bad name to me.  Logically,
"PostTask" makes sense, but "PostClosure" does not, as C++ doesn't  have
Closures.  I realize you're simulating Closure behavior with a series of very
complicated templates (I did see your description in callback.h - thanks for
writing that), and I guess if you Bind() up everything, this all works.  But I
doubt that I am the only one that can't read this code to determine if it
actually works.  And I am scared of the day I run into that code in the
debugger.  New programmers to the code base are going to have a much higher
barrier to entry on Chromium due to this unobvious naming scheme. 

So, sorry to rain on what is clearly a lot of very complicated work.  I think
there is some amazing coding going into this, and you're super geniuses for
figuring it out - simulated C++ Closures?  Wow!

Am I the odd man out that can't read this stuff?  If so, I'll stand down.  But
otherwise, I really think this is a negative CL.

Lastly, is there some huge benefit to this code?  I can't tell if it has a
performance impact or a memory impact, since I can't even tell what the code
does.  I'm guessing you think it will make it less likely to have lifecycle
issues with callbacks.  Simpletons like me can't even use this code because I
have no idea what it does, so it doesn't really matter if it is better or not.






On 2011/04/10 22:27:48, darin wrote:
> See also:
> http://code.google.com/p/chromium/issues/detail?id=61131
> 
> On Sun, Apr 10, 2011 at 3:26 PM, Darin Fisher <mailto:darin@chromium.org>
wrote:
> 
> >
> >
> > On Sun, Apr 10, 2011 at 8:58 AM, <mailto:ajwong@chromium.org> wrote:
> >
> >> addressed nit. Added one question, and running through try bots.
> >>
> >>
> >>
> >> http://codereview.chromium.org/6463013/diff/46002/base/message_loop.cc
> >> File base/message_loop.cc (right):
> >>
> >>
> >>
>
http://codereview.chromium.org/6463013/diff/46002/base/message_loop.cc#newcod...
> >> base/message_loop.cc:547: // absolutely "correct" behavior.
> >> Hey Darin, do you know the rationale behind why we do this funky dance
> >> in DeletePendingClosure() where we leak
> >>
> >>  1) tasks in the work_queue_ w/o a delayed_run_time
> >>  2) all tasks in the deferred_non_nestable_work_queue_
> >>
> >
> > I tried (years ago now!) deleting all tasks when the ML is destroyed, but
> > that caused
> > regressions on chrome-bot.  The stack traces were unhelpful, and I was
> > never able to
> > get at the root cause of the issue.  It may well have just been tickling
> > race conditions
> > that were fixed subsequently (e.g., unsafe reference counting in the
> > history system).
> >
> > It is worth a shot to try destroying all pending tasks when the ML is
> > destroyed.  It
> > may just work now.
> >
> > -Darin
> >
> >
> >
> >
> >>
> >> but not the others?
> >>
> >> I would have expected that you would either leaked all tasks in all
> >> queues or deleted all tasks.
> >>
> >> I think we're leaking the tasks out of work_queue_ because we can't
> >> reschedule them w/o a delayed_run_time.  I don't understand why we're
> >> leaking tasks from the deferred_non_nestable_work_queue_.
> >>
> >> If you have any insight into this algorithm, I'd like to update this
> >> comment with the rationale for why we have this behavior.
> >>
> >>
> >>
> >>
>
http://codereview.chromium.org/6463013/diff/46002/base/message_loop.cc#newcod...
> >> base/message_loop.cc:748: delayed_run_time(delayed_run_time),
> >> sequence_num(0),
> >> On 2011/04/09 04:21:58, darin wrote:
> >>
> >>> nit: new line before sequence_num(0)
> >>>
> >>
> >> Done.
> >>
> >>
> >> http://codereview.chromium.org/6463013/
> >>
> >
> >

[Mike Belshe, 2011-04-13 04:16:57]

http://codereview.chromium.org/6463013/diff/57023/base/message_loop.h
File base/message_loop.h (right):

http://codereview.chromium.org/6463013/diff/57023/base/message_loop.h#newcode312
base/message_loop.h:312: class BASE_API ClosureObserver {
Even if "ClosureObserver" is more accurate than "TaskObserver", it is far more
complexifying - especially when you consider "WillProcessClosure()", what does
that mean?  It sounds like its creating a Closure or binding things together. 
The old name "WillProcessTask" is actually much closer to what is really going
on here, can we keep that API, even if you change the data structure used
internally?

http://codereview.chromium.org/6463013/diff/57023/base/message_loop.h#newcode383
base/message_loop.h:383: struct PendingClosure {
Similar comment here.  The name "PendingClosure" sounds like it means that we're
now binding a bunch of things together and processing something that is pending
Closure.

http://codereview.chromium.org/6463013/diff/57023/base/message_loop.h#newcode413
base/message_loop.h:413: class TaskQueue : public std::queue<PendingClosure> {
Ah - precedent - thank you for not changing this to ClosureQueue. :-)

Similar comments throughout.  I think the MessageLoop is mechanism for
processing a set of "Tasks" not a set of "Closures".  Bury an references to
Closures as deep as you can.

[awong, 2011-04-13 19:20:37]

Hi Mike,

Thanks for the feedback!  I'm actually glad someone is willing to call a sanity
check even given the late stage of implementation.  Means the review process is
working. :)

For background on the rationale behind this change, please see the bug:  

   http://code.google.com/p/chromium/issues/detail?id=35223

It links to all the old discussion threads, and summarizes the decisions that
have occurred since Oct 2010.  Will Chan also posted the high level motivations
on the announcement thread.

If I'm correct, the main objection is that this is un-maintainable because it is
too complex to read.  I don't think anyone here is going to say that you could
understand this w/o having a deep knowledge of templates.  However, I don't
believe this is an order of magnitude more complicated than what is already
there.  Here are the analogues to the current code:

  (new) -> (equivalent old classes)
  base::Callback -> Task, Callback, Mutant
  base::Bind -> NewRunnableMethod, NewRunnableFunction, NewCallback,
CreateFunctor
  base::InvokerStorageN -> TupleN
  base::InvokerN -> DispatchToMethod, DispatchToFunction
  base::Unretained -> RunnableMethodTraits (kinda)

All, except the last, are very direct analogues.  What makes the implementation
look more complicated is mostly the extra COMPILE_ASSERTS, which are arguably
missing from the current implementation.  If you remove the COMPILE_ASSERTS, the
code starts looking quite similar.

The additional complexity comes from attempting to support:
   1) Currying (Adds more InvokerNs)
   2) base::Unretained (replaces RunnableMethodTraits)

The fact that we use a python script to generate is a red herring.  Tuple.h is a
similar to our expanded code. In fact, because tuple.h is not script generated
makes it hard to modify behavior to add things like COMPILE_ASSERTS for
functions.

For debuggability, we our function stack should be pretty similar to the status
quo on both Bind() and Run() to NRM() and NRM's Run().  Is there something
specific you are worried about?

As for the name "Closure," we actually had a few rounds of discussion on this
topic and eventually settled on Closure.  How strongly do you feel about this? 
We could reopen this discussion, but given that there was already a number of
rounds of talking, I'm reluctant to rediscuss.  Closure isn't completely
invalid.  The object is a function with all its free arguments bound, which in
C++ is roughly equivalent to having its surrounding environment.  Though there's
no precedent in Chrome for this term, in Google, the analogous concept to task
is actually called Closure.

I admit the code complex.  And I'm am sympathetic to the readability issues
(heck, Will, Brett, and I are all C++ readability reviewers).   However, I don't
think it's so much worse than the status quo, and the benefits of allowing
people to remove all their adhoc helper classes/functions, getting rid of
RunnableMethodTraits (with often get abused), etc., seems pretty big to me.

My bigger concern than the implementation complexity is whether or not the
resulting API is easily usable, and safe.  For that, I think in addition to this
CL is we need to update the external usage documentation.

Thoughts?

-Albert	

On 2011/04/13 03:44:12, Mike Belshe wrote:
> Sorry to be negative, but I think this change is a step in the wrong
direction. 
> The code probably works, and I'm certain that it is nothing short of genius,
but
> its not readable.  It may well be that I'm not a good enough programmer, but I
> think others will react similarly.  As an analogy, if someone offered to
> implement the MessageLoop in assembly, we'd reject it due to
un-maintainability.
>  I argue that this code is similar, and should not be checked in.
> 
> The template code in callback.h has always been bad.  And when you hit it with
> the debugger, its hard to step through.  But we have managed through it, and
it
> has been relatively simple templates compared to this new code.  The Closure
> code is an order of magnitude more complicated (so complicated that it needs a
> python script to generate it!), and as I look at it, I have no clue what it
> does.  I simply can't read it.
> 
> The notion of a "Closure" also seems like a bad name to me.  Logically,
> "PostTask" makes sense, but "PostClosure" does not, as C++ doesn't  have
> Closures.  I realize you're simulating Closure behavior with a series of very
> complicated templates (I did see your description in callback.h - thanks for
> writing that), and I guess if you Bind() up everything, this all works.  But I
> doubt that I am the only one that can't read this code to determine if it
> actually works.  And I am scared of the day I run into that code in the
> debugger.  New programmers to the code base are going to have a much higher
> barrier to entry on Chromium due to this unobvious naming scheme. 
> 
> So, sorry to rain on what is clearly a lot of very complicated work.  I think
> there is some amazing coding going into this, and you're super geniuses for
> figuring it out - simulated C++ Closures?  Wow!
> 
> Am I the odd man out that can't read this stuff?  If so, I'll stand down.  But
> otherwise, I really think this is a negative CL.
> 
> Lastly, is there some huge benefit to this code?  I can't tell if it has a
> performance impact or a memory impact, since I can't even tell what the code
> does.  I'm guessing you think it will make it less likely to have lifecycle
> issues with callbacks.  Simpletons like me can't even use this code because I
> have no idea what it does, so it doesn't really matter if it is better or not.
> 
> 
> 
> 
> 
> 
> On 2011/04/10 22:27:48, darin wrote:
> > See also:
> > http://code.google.com/p/chromium/issues/detail?id=61131
> > 
> > On Sun, Apr 10, 2011 at 3:26 PM, Darin Fisher <mailto:darin@chromium.org>
> wrote:
> > 
> > >
> > >
> > > On Sun, Apr 10, 2011 at 8:58 AM, <mailto:ajwong@chromium.org> wrote:
> > >
> > >> addressed nit. Added one question, and running through try bots.
> > >>
> > >>
> > >>
> > >> http://codereview.chromium.org/6463013/diff/46002/base/message_loop.cc
> > >> File base/message_loop.cc (right):
> > >>
> > >>
> > >>
> >
>
http://codereview.chromium.org/6463013/diff/46002/base/message_loop.cc#newcod...
> > >> base/message_loop.cc:547: // absolutely "correct" behavior.
> > >> Hey Darin, do you know the rationale behind why we do this funky dance
> > >> in DeletePendingClosure() where we leak
> > >>
> > >>  1) tasks in the work_queue_ w/o a delayed_run_time
> > >>  2) all tasks in the deferred_non_nestable_work_queue_
> > >>
> > >
> > > I tried (years ago now!) deleting all tasks when the ML is destroyed, but
> > > that caused
> > > regressions on chrome-bot.  The stack traces were unhelpful, and I was
> > > never able to
> > > get at the root cause of the issue.  It may well have just been tickling
> > > race conditions
> > > that were fixed subsequently (e.g., unsafe reference counting in the
> > > history system).
> > >
> > > It is worth a shot to try destroying all pending tasks when the ML is
> > > destroyed.  It
> > > may just work now.
> > >
> > > -Darin
> > >
> > >
> > >
> > >
> > >>
> > >> but not the others?
> > >>
> > >> I would have expected that you would either leaked all tasks in all
> > >> queues or deleted all tasks.
> > >>
> > >> I think we're leaking the tasks out of work_queue_ because we can't
> > >> reschedule them w/o a delayed_run_time.  I don't understand why we're
> > >> leaking tasks from the deferred_non_nestable_work_queue_.
> > >>
> > >> If you have any insight into this algorithm, I'd like to update this
> > >> comment with the rationale for why we have this behavior.
> > >>
> > >>
> > >>
> > >>
> >
>
http://codereview.chromium.org/6463013/diff/46002/base/message_loop.cc#newcod...
> > >> base/message_loop.cc:748: delayed_run_time(delayed_run_time),
> > >> sequence_num(0),
> > >> On 2011/04/09 04:21:58, darin wrote:
> > >>
> > >>> nit: new line before sequence_num(0)
> > >>>
> > >>
> > >> Done.
> > >>
> > >>
> > >> http://codereview.chromium.org/6463013/
> > >>
> > >
> > >

[awong, 2011-04-15 18:08:55]

PTAL

Renamed PostClosure to PostTask.  PostTask() is now an overloaded function.

Similarly shifted [Cc]losure to [Ttask] everywhere, except the type name.

base::Calback<void(void)> is still Closure.

[darin (slow to review), 2011-04-15 18:35:36]

I'm happy with these renaming changes.  I think it makes message_loop.cc a
little bit confusing due to the overloaded terminology, but that's OK.

http://codereview.chromium.org/6463013/diff/71014/base/message_loop.h
File base/message_loop.h (right):

http://codereview.chromium.org/6463013/diff/71014/base/message_loop.h#newcode174
base/message_loop.h:174: 
I think you should add some kind of TODO comment here explaining your intention
to deprecate the above functions in favor of these, yadda yadda.

[willchan no longer on Chromium, 2011-04-15 18:37:29]

LGTM, mod darin's comments.

[awong, 2011-04-15 18:45:56]

http://codereview.chromium.org/6463013/diff/71014/base/message_loop.h
File base/message_loop.h (right):

http://codereview.chromium.org/6463013/diff/71014/base/message_loop.h#newcode174
base/message_loop.h:174: 
On 2011/04/15 18:35:36, darin wrote:
> I think you should add some kind of TODO comment here explaining your
intention
> to deprecate the above functions in favor of these, yadda yadda.

Done.

[darin (slow to review), 2011-04-15 19:50:09]

Thanks, LGTM

On Fri, Apr 15, 2011 at 11:45 AM, <ajwong@chromium.org> wrote:

>
> http://codereview.chromium.org/6463013/diff/71014/base/message_loop.h
> File base/message_loop.h (right):
>
>
>
http://codereview.chromium.org/6463013/diff/71014/base/message_loop.h#newcode174
> base/message_loop.h:174:
> On 2011/04/15 18:35:36, darin wrote:
>
>> I think you should add some kind of TODO comment here explaining your
>>
> intention
>
>> to deprecate the above functions in favor of these, yadda yadda.
>>
>
> Done.
>
>
> http://codereview.chromium.org/6463013/
>

[mbelshe, 2011-04-15 22:30:56]

Thanks for taking all the time and bearing with me.

LGTM

On Fri, Apr 15, 2011 at 12:50 PM, Darin Fisher <darin@chromium.org> wrote:

> Thanks, LGTM
>
>
> On Fri, Apr 15, 2011 at 11:45 AM, <ajwong@chromium.org> wrote:
>
>>
>> http://codereview.chromium.org/6463013/diff/71014/base/message_loop.h
>> File base/message_loop.h (right):
>>
>>
>>
http://codereview.chromium.org/6463013/diff/71014/base/message_loop.h#newcode174
>> base/message_loop.h:174:
>> On 2011/04/15 18:35:36, darin wrote:
>>
>>> I think you should add some kind of TODO comment here explaining your
>>>
>> intention
>>
>>> to deprecate the above functions in favor of these, yadda yadda.
>>>
>>
>> Done.
>>
>>
>> http://codereview.chromium.org/6463013/
>>
>
>

[joth, 2011-04-16 15:11:57]

Minor drive-by comments - all fine to keep for another CL if you don't want to
churn this one any more.
Thanks so much for doing all this!

http://codereview.chromium.org/6463013/diff/70014/base/message_loop.h
File base/message_loop.h (right):

http://codereview.chromium.org/6463013/diff/70014/base/message_loop.h#newcode178
base/message_loop.h:178: // There are 2 sets of Post*Task functions, one which
tasks the older Task*
nit: one which _takes_

http://codereview.chromium.org/6463013/diff/70014/base/message_loop.h#newcode179
base/message_loop.h:179: // function object representation, and one that task
the newer base::Closure.
again, one that _takes_

http://codereview.chromium.org/6463013/diff/70014/base/message_loop.h#newcode543
base/message_loop.h:543: bool should_leak_tasks_;
maybe worth putting a comment on this member, if just directing to the subtle
code that uses it

http://codereview.chromium.org/6463013/diff/70014/base/message_loop_unittest.cc
File base/message_loop_unittest.cc (right):

http://codereview.chromium.org/6463013/diff/70014/base/message_loop_unittest....
base/message_loop_unittest.cc:166: &Foo::Test2Mixed,foo.get(), a, &d));
nit: missing space after "&Foo::Test2Mixed," x5

http://codereview.chromium.org/6463013/diff/70014/chrome/renderer/renderer_ma...
File chrome/renderer/renderer_main.cc (right):

http://codereview.chromium.org/6463013/diff/70014/chrome/renderer/renderer_ma...
chrome/renderer/renderer_main.cc:226: if (begin_process_message_ !=
base::TimeTicks())
suggest
if (begin_process_message_.is_null())

(also, it looks odd to me passing class TimeTicks by value rather than const
ref, but it seems to be standard practice)

[awong, 2011-04-17 00:34:57]

Good catches!

Addressed comments.

http://codereview.chromium.org/6463013/diff/70014/base/message_loop.h
File base/message_loop.h (right):

http://codereview.chromium.org/6463013/diff/70014/base/message_loop.h#newcode178
base/message_loop.h:178: // There are 2 sets of Post*Task functions, one which
tasks the older Task*
On 2011/04/16 15:11:57, joth wrote:
> nit: one which _takes_

Done.

http://codereview.chromium.org/6463013/diff/70014/base/message_loop.h#newcode179
base/message_loop.h:179: // function object representation, and one that task
the newer base::Closure.
On 2011/04/16 15:11:57, joth wrote:
> again, one that _takes_

Done.

http://codereview.chromium.org/6463013/diff/70014/base/message_loop.h#newcode543
base/message_loop.h:543: bool should_leak_tasks_;
On 2011/04/16 15:11:57, joth wrote:
> maybe worth putting a comment on this member, if just directing to the subtle
> code that uses it

Good idea! Comment added.

http://codereview.chromium.org/6463013/diff/70014/base/message_loop_unittest.cc
File base/message_loop_unittest.cc (right):

http://codereview.chromium.org/6463013/diff/70014/base/message_loop_unittest....
base/message_loop_unittest.cc:166: &Foo::Test2Mixed,foo.get(), a, &d));
On 2011/04/16 15:11:57, joth wrote:
> nit: missing space after "&Foo::Test2Mixed," x5

Done.

http://codereview.chromium.org/6463013/diff/70014/chrome/renderer/renderer_ma...
File chrome/renderer/renderer_main.cc (right):

http://codereview.chromium.org/6463013/diff/70014/chrome/renderer/renderer_ma...
chrome/renderer/renderer_main.cc:226: if (begin_process_message_ !=
base::TimeTicks())
Ooh...good catch.

As for TimeTicks being passed by value, it's designed to be done that way. The
data member is just one int64, so pass by value ends up more efficient than by
const ref.

On 2011/04/16 15:11:57, joth wrote:
> suggest
> if (begin_process_message_.is_null())
> 
> (also, it looks odd to me passing class TimeTicks by value rather than const
> ref, but it seems to be standard practice)

[commit-bot: I haz the power, 2011-04-20 14:02:51]

Can't apply patch for file base/base/bind_internal_win.h.
A         base/base
patching file base/base/bind_internal_win.h
Hunk #1 FAILED at 15.
Hunk #2 FAILED at 180.
2 out of 2 hunks FAILED -- saving rejects to file
base/base/bind_internal_win.h.rej