Trace PostTasks from post to run

Trace PostTasks from post to run.

To do this properly, we need a new type of trace event "FLOW" which will be drawn as lines in about:tracing from BEGIN to END.

Also instruments SequencedWorkerPool and ChannelReader::DispatchInputData, which were not currently traced.

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

[jbates, 2012-09-13 01:36:32]

jar: ptal

https://chromiumcodereview.appspot.com/10913242/diff/1/base/message_loop.cc
File base/message_loop.cc (left):

https://chromiumcodereview.appspot.com/10913242/diff/1/base/message_loop.cc#o...
base/message_loop.cc:489: PendingTask new_pending_task(pending_task);
Bonus: one less copy of the PendingTask struct.

[jar (doing other things), 2012-09-13 02:14:48]

https://chromiumcodereview.appspot.com/10913242/diff/1/base/message_loop.cc
File base/message_loop.cc (right):

https://chromiumcodereview.appspot.com/10913242/diff/1/base/message_loop.cc#n...
base/message_loop.cc:97: static_cast<uint64>(reinterpret_cast<intptr_t>(loop));
Have you considered that message loops might generally be allocated in the same
place in multiple processes?  You can hope for randomness... but most threads
start up very sequentially (while nearly single threaded), and with TCMalloc,
certain sized objects are allocated in a span for a specific size class... and
they all get allocated from the  thread local cache of the spawning thread (UI
thread?)... so... they might all be very similar, or nearly const (for a
specfiic thread, across all processes).

YMMV :-/

https://chromiumcodereview.appspot.com/10913242/diff/1/base/message_loop.cc#n...
base/message_loop.cc:444: TRACE_ID_MANGLE(GetTaskTraceID(pending_task, this)));
... and just checking... when tracing is off, this GetTaskTraceID() is *not*
called. Correct?

https://chromiumcodereview.appspot.com/10913242/diff/1/base/message_loop.cc#n...
base/message_loop.cc:595: pending_task->sequence_num = next_sequence_num_++;
:-) 
Cute!  You're using the incoming_queue_lock_, rather than the apartment_model
which used to restrict access to this next_sequence_num_ to take place only on
the owning thread. 

Please change the header to indicate that this is now protected by the lock.

[jbates, 2012-09-13 17:07:01]

https://chromiumcodereview.appspot.com/10913242/diff/1/base/message_loop.cc
File base/message_loop.cc (right):

https://chromiumcodereview.appspot.com/10913242/diff/1/base/message_loop.cc#n...
base/message_loop.cc:97: static_cast<uint64>(reinterpret_cast<intptr_t>(loop));
On 2012/09/13 02:14:49, jar wrote:
> Have you considered that message loops might generally be allocated in the
same
> place in multiple processes?  You can hope for randomness... but most threads
> start up very sequentially (while nearly single threaded), and with TCMalloc,
> certain sized objects are allocated in a span for a specific size class... and
> they all get allocated from the  thread local cache of the spawning thread (UI
> thread?)... so... they might all be very similar, or nearly const (for a
> specfiic thread, across all processes).
> 
> YMMV :-/

Yep, that's definitely a concern. To deal with pointer collisions, the process
ID is xor'd with the return value of this function (see TRACE_ID_MANGLE below).
I updated the comment on this function to make that clear.

https://chromiumcodereview.appspot.com/10913242/diff/1/base/message_loop.cc#n...
base/message_loop.cc:444: TRACE_ID_MANGLE(GetTaskTraceID(pending_task, this)));
On 2012/09/13 02:14:49, jar wrote:
> ... and just checking... when tracing is off, this GetTaskTraceID() is *not*
> called. Correct?

Exactly.

https://chromiumcodereview.appspot.com/10913242/diff/1/base/message_loop.cc#n...
base/message_loop.cc:595: pending_task->sequence_num = next_sequence_num_++;
On 2012/09/13 02:14:49, jar wrote:
> :-) 
> Cute!  You're using the incoming_queue_lock_, rather than the apartment_model
> which used to restrict access to this next_sequence_num_ to take place only on
> the owning thread. 
> 
> Please change the header to indicate that this is now protected by the lock.

Done.

[jar (doing other things), 2012-09-14 02:10:44]

Darin,

Do you have any idea why all the slots in MessageLoop were made "protected?" 
Was it just a bulk move, despite the fact that many items could really stay
private?


If darin does not complain... LGTM with nits below.

https://chromiumcodereview.appspot.com/10913242/diff/9001/base/debug/trace_ev...
File base/debug/trace_event.h (right):

https://chromiumcodereview.appspot.com/10913242/diff/9001/base/debug/trace_ev...
base/debug/trace_event.h:751: explicit ForceMangle(long id) :
nit: Colon on next line, indented 4.

https://chromiumcodereview.appspot.com/10913242/diff/9001/base/message_loop.h
File base/message_loop.h (right):

https://chromiumcodereview.appspot.com/10913242/diff/9001/base/message_loop.h...
base/message_loop.h:510: int next_sequence_num_;
Can we move this to private as well?  I searched, and don't think it is accessed
in any derived class... 

In fact, I *think* the oncoming_queue_lock_ can also move, along with
incoming_queue_.

[darin (slow to review), 2012-09-14 03:36:34]

On Thu, Sep 13, 2012 at 7:10 PM, <jar@chromium.org> wrote:

> Darin,
>
> Do you have any idea why all the slots in MessageLoop were made
> "protected?"
> Was it just a bulk move, despite the fact that many items could really stay
> private?
>
>
MessageLoop has subclasses:  e.g., MessageLoopForIO

I haven't confirmed that all of the protected members actually need to be
protected instead of private.

-Darin


>
> If darin does not complain... LGTM with nits below.
>
>
> https://chromiumcodereview.**appspot.com/10913242/diff/**
>
9001/base/debug/trace_event.h<https://chromiumcodereview.appspot.com/10913242/diff/9001/base/debug/trace_event.h>
> File base/debug/trace_event.h (right):
>
> https://chromiumcodereview.**appspot.com/10913242/diff/**
>
9001/base/debug/trace_event.h#**newcode751<https://chromiumcodereview.appspot.com/10913242/diff/9001/base/debug/trace_event.h#newcode751>
> base/debug/trace_event.h:751: explicit ForceMangle(long id) :
> nit: Colon on next line, indented 4.
>
> https://chromiumcodereview.**appspot.com/10913242/diff/**
>
9001/base/message_loop.h<https://chromiumcodereview.appspot.com/10913242/diff/9001/base/message_loop.h>
> File base/message_loop.h (right):
>
> https://chromiumcodereview.**appspot.com/10913242/diff/**
>
9001/base/message_loop.h#**newcode510<https://chromiumcodereview.appspot.com/10913242/diff/9001/base/message_loop.h#newcode510>
> base/message_loop.h:510: int next_sequence_num_;
> Can we move this to private as well?  I searched, and don't think it is
> accessed in any derived class...
>
> In fact, I *think* the oncoming_queue_lock_ can also move, along with
> incoming_queue_.
>
>
https://chromiumcodereview.**appspot.com/10913242/<https://chromiumcodereview...
>

[jbates, 2012-09-14 18:00:49]

jam: ipc/OWNERS

https://chromiumcodereview.appspot.com/10913242/diff/9001/base/debug/trace_ev...
File base/debug/trace_event.h (right):

https://chromiumcodereview.appspot.com/10913242/diff/9001/base/debug/trace_ev...
base/debug/trace_event.h:751: explicit ForceMangle(long id) :
On 2012/09/14 02:10:44, jar wrote:
> nit: Colon on next line, indented 4.

Done.

https://chromiumcodereview.appspot.com/10913242/diff/9001/base/message_loop.h
File base/message_loop.h (right):

https://chromiumcodereview.appspot.com/10913242/diff/9001/base/message_loop.h...
base/message_loop.h:510: int next_sequence_num_;
On 2012/09/14 02:10:44, jar wrote:
> Can we move this to private as well?  I searched, and don't think it is
accessed
> in any derived class... 
> 
> In fact, I *think* the oncoming_queue_lock_ can also move, along with
> incoming_queue_.

It looks like you're right that many of these members could be private. I filed
https://code.google.com/p/chromium/issues/detail?id=149691 to track this, since
it might make this patch a bit more complicated than necessary.

[jam, 2012-09-17 22:16:29]

On 2012/09/14 18:00:49, jbates wrote:
> jam: ipc/OWNERS

I'm too overloaded with reviews and ipc/ has a bunch of owners, please redirect.

[jbates, 2012-09-17 22:19:24]

cpu: ipc/OWNERS

[darin (slow to review), 2012-09-17 23:18:52]

LGTM for src/ipc/

[commit-bot: I haz the power, 2012-09-17 23:44:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbates@chromium.org/10913242/11001

[commit-bot: I haz the power, 2012-09-18 00:01:00]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-09-18 00:33:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbates@chromium.org/10913242/14005

[commit-bot: I haz the power, 2012-09-18 00:35:05]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-09-18 00:37:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbates@chromium.org/10913242/14005

[commit-bot: I haz the power, 2012-09-18 00:41:14]

Step "update" is always a major failure.
Look at the try server FAQ for more details.