Update task tracking to not depend on message_loop_ singleton

Update task tracking to not depend on message_loop_ singleton

I also did a bunch of cleanup, and transitioned to tracking
the duration of a run, rather than the time from posting
(construction of a task) to completion of the run.  It is
less interesting for now to track queueing delay, and we
need a focus on task execution time.  I left in the hook
(API) with the expectation that I'll be extending the
tracked_objects code to include this as well.

I also landed changes to run in Linux/Mac.  The fact that
I've punted on shutdown made this landing easy (all
code support was previously lost during migration to
some flavor of bind support).

r=willchan,jam,viettrungluu,ajwong
BUG=62728
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=105694

[jar (doing other things), 2011-10-14 02:29:52]

@willchan: This relates to the bug you filed about message loop.  I basically
stopped saving the message loop, and should have resolved the bug. Please look
at tracked_objects.cc

@jam: I have some code in content, and I wanted to be sure you were fine with
it.

@ajwong: I think I may have done the linux integration TODO() you had.  Please
see if you agree.

@vietttrungluu: I moved something in content in keeping with your todo.  Please
tell me if you think there is a reason not to have done so.

IMO, a lot of this code needs mostly a rubber stamp review, since it was
reviewed (for the fancy(?) parts) long ago.  I do still need to do some careful
subtle stuff with atomics to make it more bullet proof, but that will wait for
another CL.

I'm pretty bothered about Chrome performance, and have been hacking away at this
for a few weeks (and getting nice results).  I'd like to advertise it to the
team... but it needed at least this initial cleaning.  I'll soon transition (per
eroman's guidance) to sending out a JSON file, and leaving JS to really improve
data visualization... but that is for another CL.

http://codereview.chromium.org/8233037/diff/19006/base/message_loop.cc
File base/message_loop.cc (left):

http://codereview.chromium.org/8233037/diff/19006/base/message_loop.cc#oldcod...
base/message_loop.cc:486: tracked_objects::ThreadData::TallyADeathIfActive(
I changed this code so that I don't need to ifdef it here.  In production, the
function will be a no-op, and will be compiled out.

http://codereview.chromium.org/8233037/diff/19006/base/threading/worker_pool_...
File base/threading/worker_pool_win.cc (left):

http://codereview.chromium.org/8233037/diff/19006/base/threading/worker_pool_...
base/threading/worker_pool_win.cc:25: time_posted = TimeTicks::Now();
I switched to a "higher performance" Now(), which can be disabled into almost a
no-op when we are not profiling.

http://codereview.chromium.org/8233037/diff/19006/base/threading/worker_pool_...
base/threading/worker_pool_win.cc:51:
tracked_objects::ThreadData::TallyADeathIfActive(
This function will be a no-op when we don't define TRACK_ALL_TASK_OBJECTS, so I
don't need to ifdef it here.

http://codereview.chromium.org/8233037/diff/19006/base/tracked_objects.cc
File base/tracked_objects.cc (left):

http://codereview.chromium.org/8233037/diff/19006/base/tracked_objects.cc#old...
base/tracked_objects.cc:240: // http://crbug.com/62728
This was the primary motivation for this cleanup CL.

http://codereview.chromium.org/8233037/diff/19006/base/tracked_objects.cc#old...
base/tracked_objects.cc:263: // http://crbug.com/62728
Again... the motivation for the CL.

http://codereview.chromium.org/8233037/diff/19006/base/tracked_objects.cc#old...
base/tracked_objects.cc:307: return message_loop_->thread_name();
We now store the thread name, and don't need to keep looking it up.

http://codereview.chromium.org/8233037/diff/19006/base/tracked_objects.cc#old...
base/tracked_objects.cc:348: // A class used to count down which is accessed by
several threads.  This is
We no longer do any terminaition cleanup... so all this code goes away.

http://codereview.chromium.org/8233037/diff/19006/base/tracked_objects.cc#old...
base/tracked_objects.cc:422: message_loops.push_back(it->message_loop());
Since we don't do this racy cleanup attempt, we don't need these message
loops... and they are gone, as well as this code.

http://codereview.chromium.org/8233037/diff/19006/base/tracked_objects.cc#old...
base/tracked_objects.cc:840: if (left.count() != right.count())
I decided it was stupid to aggregate groups of results with similar durations or
counts (similar names, yeah; similar files, yeah; similar durations, nah).  We
just act as though all pairs of durations are equivalent (unworthy of
speparating into individual sub-groups.
distinct groups in HTML), and pass.

http://codereview.chromium.org/8233037/diff/19006/content/browser/browser_mai...
File content/browser/browser_main.cc (left):

http://codereview.chromium.org/8233037/diff/19006/content/browser/browser_mai...
content/browser/browser_main.cc:248: InitializeMainThread();
I had to move this so that we wouldn't create task instances before we told the
ThreadData class what the thread name was.  I agreed with the above TODO()...
and just did it.

[jar (doing other things), 2011-10-14 15:48:42]

http://codereview.chromium.org/8233037/diff/19006/base/tracked_objects.cc
File base/tracked_objects.cc (right):

http://codereview.chromium.org/8233037/diff/19006/base/tracked_objects.cc#new...
base/tracked_objects.cc:380: NOTREACHED();
This has to be removed. 
This code is used is single-threaded testing to cleanup.  I won't upload this
change until I have code reviews from folks (since I put a lot of comments in
the left pane when I sent out for reviews)
Jim

[jam, 2011-10-14 16:03:15]

content lgtm

[awong, 2011-10-14 16:54:49]

So lgtm for mac TODO.

For my TODO, which I think was mac only, the problem was readily apparent on the
try bots in the form of a large number of CHECK fail.  Since you're passing the
Mac try bot, I think it's fixed.

http://codereview.chromium.org/8233037/diff/19006/base/threading/worker_pool_...
File base/threading/worker_pool_posix.cc (right):

http://codereview.chromium.org/8233037/diff/19006/base/threading/worker_pool_...
base/threading/worker_pool_posix.cc:93:
tracked_objects::ThreadData::TallyADeathIfActive(pending_task.post_births,
if the #define is false, won't pending_task.post_births not exist?

[jar (doing other things), 2011-10-14 22:15:53]

Response to comment by ajwong.

http://codereview.chromium.org/8233037/diff/19006/base/threading/worker_pool_...
File base/threading/worker_pool_posix.cc (right):

http://codereview.chromium.org/8233037/diff/19006/base/threading/worker_pool_...
base/threading/worker_pool_posix.cc:93:
tracked_objects::ThreadData::TallyADeathIfActive(pending_task.post_births,
Look in this CL... in tracked_objects.cc .... you'll see that I have caused the
function to return instantly (when TRACK_ALL_OBJECTS is undefined).  As a
result, this will turn into a do-nothing call, which the global optimizer will
discard.

On 2011/10/14 16:54:50, awong wrote:
> if the #define is false, won't pending_task.post_births not exist?

[jar (doing other things), 2011-10-14 22:18:23]

http://codereview.chromium.org/8233037/diff/19006/base/threading/worker_pool_...
File base/threading/worker_pool_posix.cc (right):

http://codereview.chromium.org/8233037/diff/19006/base/threading/worker_pool_...
base/threading/worker_pool_posix.cc:93:
tracked_objects::ThreadData::TallyADeathIfActive(pending_task.post_births,
Doh.... I' misread.... your question is about the slot in the struct not
existing :-/.

Let me figure out why I've built successufully on both Release and Debug.


On 2011/10/14 16:54:50, awong wrote:
> if the #define is false, won't pending_task.post_births not exist?

[jar (doing other things), 2011-10-14 22:59:57]

Uploaded changes.

http://codereview.chromium.org/8233037/diff/19006/base/threading/worker_pool_...
File base/threading/worker_pool_posix.cc (right):

http://codereview.chromium.org/8233037/diff/19006/base/threading/worker_pool_...
base/threading/worker_pool_posix.cc:93:
tracked_objects::ThreadData::TallyADeathIfActive(pending_task.post_births,
Sorry about that... until I modify the PendingTask struct to always have the
slot, I'll just leave this code ifdef'ed out.

I could add the extra slot... but I'll leave that for another CL.

On 2011/10/14 22:18:23, jar wrote:
> Doh.... I' misread.... your question is about the slot in the struct not
> existing :-/.
> 
> Let me figure out why I've built successufully on both Release and Debug.
> 
> 
> On 2011/10/14 16:54:50, awong wrote:
> > if the #define is false, won't pending_task.post_births not exist?
>

[awong, 2011-10-14 23:29:01]

I got a chance to look at more than just my TODO and had a few comments (mostly
API and style nits).

As I'll be out for the next 2 weeks, feel free to proceed without me.

http://codereview.chromium.org/8233037/diff/24001/base/tracked_objects.cc
File base/tracked_objects.cc (right):

http://codereview.chromium.org/8233037/diff/24001/base/tracked_objects.cc#new...
base/tracked_objects.cc:53: static_cast<int>(life_duration_.InMilliseconds()),
Would it be worth using the 64-bit compatibility format macros and avoid the
cast?

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#64-bit_Portabi...

http://codereview.chromium.org/8233037/diff/24001/base/tracked_objects.cc#new...
base/tracked_objects.cc:273: #endif
It'd be nice to just #else the rest of the function rather than relying on
compiler deadbranch elimination.

http://codereview.chromium.org/8233037/diff/24001/base/tracked_objects.cc#new...
base/tracked_objects.cc:289: #endif
Same as above.

http://codereview.chromium.org/8233037/diff/24001/base/tracked_objects.cc#new...
base/tracked_objects.cc:298: // we discard time_posted.
Should this be a TODO with a bug?

http://codereview.chromium.org/8233037/diff/24001/base/tracked_objects.h
File base/tracked_objects.h (right):

http://codereview.chromium.org/8233037/diff/24001/base/tracked_objects.h#newc...
base/tracked_objects.h:69: // currently over-write the birth time with the time
at which a Run() of the
over-write -> overwrite

http://codereview.chromium.org/8233037/diff/24001/base/tracked_objects.h#newc...
base/tracked_objects.h:485: static ThreadData* FactoryGet(const char*
suggested_name);
I thought the general pattern for singleton-ish getters was just to call this
ThreadData* Get().

FactoryGet() makes me think you're getting a factory.

If we use the MessageLoopProxy style convention, we'd just leave this as
"current()"

BTW, what happens if you call it with different suggested_names twice in one
thread?  Would it be better to just have a InitForThread() function separate
from the current() call?

[jar (doing other things), 2011-10-15 15:11:32]

Response to ajwong's comments.

I did more name cleaning beyond the requested changes, and implemented support
for queue duration display as well as run-time duration display.

http://codereview.chromium.org/8233037/diff/24001/base/tracked_objects.cc
File base/tracked_objects.cc (right):

http://codereview.chromium.org/8233037/diff/24001/base/tracked_objects.cc#new...
base/tracked_objects.cc:53: static_cast<int>(life_duration_.InMilliseconds()),
Done.

I'll be surprised when we have more than 24days of milliseconds.... but I guess
this way I don't need to worry about it.


On 2011/10/14 23:29:02, awong wrote:
> Would it be worth using the 64-bit compatibility format macros and avoid the
> cast?
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#64-bit_Portabi...

http://codereview.chromium.org/8233037/diff/24001/base/tracked_objects.cc#new...
base/tracked_objects.cc:273: #endif
On 2011/10/14 23:29:02, awong wrote:
> It'd be nice to just #else the rest of the function rather than relying on
> compiler deadbranch elimination.

Done.

http://codereview.chromium.org/8233037/diff/24001/base/tracked_objects.cc#new...
base/tracked_objects.cc:289: #endif
On 2011/10/14 23:29:02, awong wrote:
> Same as above.

Done.

http://codereview.chromium.org/8233037/diff/24001/base/tracked_objects.cc#new...
base/tracked_objects.cc:298: // we discard time_posted.
On 2011/10/14 23:29:02, awong wrote:
> Should this be a TODO with a bug?

I went ahead and implemented queue duration tallying as well.

http://codereview.chromium.org/8233037/diff/24001/base/tracked_objects.h
File base/tracked_objects.h (right):

http://codereview.chromium.org/8233037/diff/24001/base/tracked_objects.h#newc...
base/tracked_objects.h:69: // currently over-write the birth time with the time
at which a Run() of the
On 2011/10/14 23:29:02, awong wrote:
> over-write -> overwrite

Done.

http://codereview.chromium.org/8233037/diff/24001/base/tracked_objects.h#newc...
base/tracked_objects.h:485: static ThreadData* FactoryGet(const char*
suggested_name);
Done.

On 2011/10/14 23:29:02, awong wrote:
> I thought the general pattern for singleton-ish getters was just to call this
> ThreadData* Get().
> 
> FactoryGet() makes me think you're getting a factory.
> 
> If we use the MessageLoopProxy style convention, we'd just leave this as
> "current()"
> 
> BTW, what happens if you call it with different suggested_names twice in one
> thread?  Would it be better to just have a InitForThread() function separate
> from the current() call?