Fully enable about:tracking by default

Fully enable about:tracking by default

This is a re-land of:
http://codereview.chromium.org/8391019/
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107793

Original landing had trouble with message_loop_x.h, due to header
include ordering.  I pulled out the structure that was really needed by
tracked_objects.h into a new file tracked_info.*.  This allows tracked_objects
to inlude this tracked_info, but not have to include the message_loop.h
totality.  I also removed a DCHECK that that was triggering on a test,
and added yet one more file (browser_main.cc) where I removed a #ifdef
for TRACKING_ALL_OBJECTS.  The changes were minor, and I'm hoping to get
clear perf runs with tihs landing, so I'm going to TBR it and reland
early in the morning.



Comments from original landing:

Support is now controlled by the flag:
--enable-tracking
and the default is always on. To turn it off, use:
--enable-tracking=0

All profiler code is compiled now in release and official
builds (in addition to debug, where it was already active),
but most entry points can be disabled (turned into no-ops) 
by a single const bool setting atop tracked_objects.cc (in 
case folks want to revert the perf-impact of this change).

Transition to faster Now() service on Windows for the 
profiler use only.
The TimeTicks::Now() function on Window uses locking
to get a 64 bit time value. This CL transitions 
times used for profiling to more directly use a
32 bit Time interface, which is actually what drives the
64 bit TimeTicks. By using the smaller value, we avoid 
the need for locks, or even atomic operations for the most
part in the tracking system. On linux, we just down-sample
the standard TimeTicks to 32 bits for consistency (clean
ability to snapshot asyncronously without atomics...
but I should verify that such is helpful to performance).

I've also put in yet more cleanup and refactoring.

tbr=rtenneti
bug=101856
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107895

[ramant (doing other things), 2011-10-29 17:26:35]

LGTM.

nit: would be great if we have unit tests to activate/deactivate tracking.

http://codereview.chromium.org/8414036/diff/4036/base/tracked_objects.h
File base/tracked_objects.h (right):

http://codereview.chromium.org/8414036/diff/4036/base/tracked_objects.h#newco...
base/tracked_objects.h:180: // TimeTicks maintains a wasteful 64 bits of data
(we need need less than 32),
nit: need need -> need

http://codereview.chromium.org/8414036/diff/4036/base/tracked_objects.h#newco...
base/tracked_objects.h:421: // uses locks at the lowest level (when accessing
the underlying maps which
nit: This function is uses ... -> This function uses...

http://codereview.chromium.org/8414036/diff/4036/base/tracked_objects.h#newco...
base/tracked_objects.h:457: // We generally provide a aggregate total for the
entire list, as well as
nit: a aggregate -> an aggregate

http://codereview.chromium.org/8414036/diff/4036/base/tracked_objects.h#newco...
base/tracked_objects.h:648: // Records the end of a timed run of an object.  The
|birth| is the record for
nit: |birth| I am guessing it is hidden in TrackingInfo. They used to be
arguments, but because they are part of TrackingInfo, would be great if we could
adjust the comments.

[jar (doing other things), 2011-10-29 21:02:49]

Changes per comments by rtenneti.

Also included new tests for the transition from active to deactivated at various
points in the lifecycle of a tracked object.

http://codereview.chromium.org/8414036/diff/4036/base/tracked_objects.h
File base/tracked_objects.h (right):

http://codereview.chromium.org/8414036/diff/4036/base/tracked_objects.h#newco...
base/tracked_objects.h:180: // TimeTicks maintains a wasteful 64 bits of data
(we need need less than 32),
On 2011/10/29 17:26:35, ramant wrote:
> nit: need need -> need

Done.

http://codereview.chromium.org/8414036/diff/4036/base/tracked_objects.h#newco...
base/tracked_objects.h:421: // uses locks at the lowest level (when accessing
the underlying maps which
On 2011/10/29 17:26:35, ramant wrote:
> nit: This function is uses ... -> This function uses...

Done.

http://codereview.chromium.org/8414036/diff/4036/base/tracked_objects.h#newco...
base/tracked_objects.h:457: // We generally provide a aggregate total for the
entire list, as well as
On 2011/10/29 17:26:35, ramant wrote:
> nit: a aggregate -> an aggregate

Done.

http://codereview.chromium.org/8414036/diff/4036/base/tracked_objects.h#newco...
base/tracked_objects.h:648: // Records the end of a timed run of an object.  The
|birth| is the record for
On 2011/10/29 17:26:35, ramant wrote:
> nit: |birth| I am guessing it is hidden in TrackingInfo. They used to be
> arguments, but because they are part of TrackingInfo, would be great if we
could
> adjust the comments.

Done.

[jar (doing other things), 2011-10-30 00:46:00]

Darin/Avi/ben/brettw/jam/sky:

I'm in search of a rubber stamp LGTM from an owner of Content/Browser for the
the removal of the ifdef in
browser_main_loop.cc.  This has been running in debug with no problem... I'm
just enabling in release.

I'd really like to land this over the weekend, to assure no perf regression
results.

Thanks in advance,

Jim

[brettw, 2011-10-30 03:36:54]

LGTM for content/browser

[Timur Iskhodzhanov, 2011-10-30 18:48:25]

Hi Jim,

Mind taking a look at the TSan bots? They're unhappy with this change.

Thanks,
Timur

[jar (doing other things), 2011-10-30 22:21:56]

So far I've seen only a benign race report, where to methods look at status
(enabled vs disabled tracking).  I have to file a suppression for that, as
I carefully handle this async transiition in the underlying code (so it has
no negative impact).

If you have pointers to anything else that I missed, that would be great.

I hadn't written up a suppression CL yet... but will.

Jim

On Sun, Oct 30, 2011 at 11:48 AM, <timurrrr@chromium.org> wrote:

> Hi Jim,
>
> Mind taking a look at the TSan bots? They're unhappy with this change.
>
> Thanks,
> Timur
>
>
http://codereview.chromium.**org/8414036/<http://codereview.chromium.org/8414...
>

[jar (doing other things), 2011-10-31 16:49:45]

I found another potential race in the list... where changes in earlier base
unit tests could leave threads running... and interact with my tracked
object unit test.  I crafted a leak to avoid any chance of a conflict.

I landed this stuff overnight... but due to instability in Mac, things were
aggressively/blindly reverted.  I'll reland, and try to clean up any tsan
issues.

thanks for your help,

Jim

On Sun, Oct 30, 2011 at 3:21 PM, Jim Roskind <jar@chromium.org> wrote:

> So far I've seen only a benign race report, where to methods look at
> status (enabled vs disabled tracking).  I have to file a suppression for
> that, as I carefully handle this async transiition in the underlying code
> (so it has no negative impact).
>
> If you have pointers to anything else that I missed, that would be great.
>
> I hadn't written up a suppression CL yet... but will.
>
> Jim
>
>
> On Sun, Oct 30, 2011 at 11:48 AM, <timurrrr@chromium.org> wrote:
>
>> Hi Jim,
>>
>> Mind taking a look at the TSan bots? They're unhappy with this change.
>>
>> Thanks,
>> Timur
>>
>>
http://codereview.chromium.**org/8414036/<http://codereview.chromium.org/8414...
>>
>
>