Implement profiler log writing at shutdown

Implement profiler log writing at shutdown

Add a flag to enable writing profile data during shutdown
(--profiling-output-file=$FILE).

Add TaskProfilerDataSerializer.  This class is responsible for
collecting and outputing profiler data.  Currently, the collection part
is very simple, since ThreadData::ToValue() is static.  It should become
more substantial when we add support for multi-process profiler dumps.
This class has to be located outside of base/ because it needs to access
Chrome-specific functions (ie. for fetching the userAgent).

Move AutoTracking class to the chrome/browser/task_profiler directory.
It needs to access the TaskProfilerDataSerializer, but components in
base/ should not have access to this Chrome-specific class.

BUG=107265, 109459
TEST=


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

[rlarocque, 2012-01-06 23:36:36]

Here is the C++ implementation of code to output the task profiler's state.  It
should match the format used for output from JS implemented in
http://codereview.chromium.org/8918018/.

The code to implement this is fairly simple.  The hardest part was trying to
figure out how things should be named and where to put them.  I'm not sure I've
got it right; let me know what you think.

[eroman, 2012-01-10 02:42:58]

When including multiple reviewers, please describe what you want each reviewer
to look at, otherwise things can get stalled ;)

I believe this review is best done by Jim, moving myself to the "cc" list.

[jar (doing other things), 2012-01-10 20:51:35]

http://codereview.chromium.org/9125015/diff/1/chrome/browser/task_profiler/ta...
File chrome/browser/task_profiler/task_profiler_data_serializer.cc (right):

http://codereview.chromium.org/9125015/diff/1/chrome/browser/task_profiler/ta...
chrome/browser/task_profiler/task_profiler_data_serializer.cc:32: // down, then
add that data to the 'per_process_data' array here.
This is a big challenge :-/.  Most processes shut down rather dramatically, with
very little warning.  Perhaps you can cause the shutdown hooks in the browser
process to "go slow" and first request profile info.

Historically, this has been a hassle, when we wanted to get histograms relating
to page load.  We gathered the data... but then (in our rush for speed) killed
teh process at shutdown before we had a chance to upload "the last page loaded"
data.

You may be able to ignore this "rush" constraint since the user has asked for
the dump... and may have to "pay" with a slightly slower shutdown.

[jar (doing other things), 2012-01-28 02:43:43]

+cc rtenneti

[ramant (doing other things), 2012-01-31 01:12:44]

http://codereview.chromium.org/9125015/diff/1/chrome/browser/task_profiler/ta...
File chrome/browser/task_profiler/task_profiler_data_serializer.cc (right):

http://codereview.chromium.org/9125015/diff/1/chrome/browser/task_profiler/ta...
chrome/browser/task_profiler/task_profiler_data_serializer.cc:32: // down, then
add that data to the 'per_process_data' array here.
Hi rlarocque,
  I had done collecting of data from child processes. Can work on getting data
from child processes when we are shutting down.

a small question: any updates on the status of this patch?

thanks,
raman

[rlarocque, 2012-01-31 19:26:03]

On 2012/01/31 01:12:44, ramant wrote:
>
http://codereview.chromium.org/9125015/diff/1/chrome/browser/task_profiler/ta...
> File chrome/browser/task_profiler/task_profiler_data_serializer.cc (right):
> 
>
http://codereview.chromium.org/9125015/diff/1/chrome/browser/task_profiler/ta...
> chrome/browser/task_profiler/task_profiler_data_serializer.cc:32: // down,
then
> add that data to the 'per_process_data' array here.
> Hi rlarocque,
>   I had done collecting of data from child processes. Can work on getting data
> from child processes when we are shutting down.
> 
> a small question: any updates on the status of this patch?
> 
> thanks,
> raman

Hi Raman,

I had hoped to get this committed as is, because I could see some value in the
information it does provide.  It would be nice if it could fetch data from child
processes, but the main process data is what I'm really after.  

I haven't had time to work on this lately, so there is no progress.  Also, I
don't know much about IPC or shutdown, so it sounds like you would be much more
qualified than me to carry on this work, if you're interested in doing so.  

Richard.

[ramant (doing other things), 2012-01-31 20:09:50]

On 2012/01/31 19:26:03, rlarocque wrote:
> On 2012/01/31 01:12:44, ramant wrote:
> >
>
http://codereview.chromium.org/9125015/diff/1/chrome/browser/task_profiler/ta...
> > File chrome/browser/task_profiler/task_profiler_data_serializer.cc (right):
> > 
> >
>
http://codereview.chromium.org/9125015/diff/1/chrome/browser/task_profiler/ta...
> > chrome/browser/task_profiler/task_profiler_data_serializer.cc:32: // down,
> then
> > add that data to the 'per_process_data' array here.
> > Hi rlarocque,
> >   I had done collecting of data from child processes. Can work on getting
data
> > from child processes when we are shutting down.
> > 
> > a small question: any updates on the status of this patch?
> > 
> > thanks,
> > raman
> 
> Hi Raman,
> 
> I had hoped to get this committed as is, because I could see some value in the
> information it does provide.  It would be nice if it could fetch data from
child
> processes, but the main process data is what I'm really after.  
> 
> I haven't had time to work on this lately, so there is no progress.  Also, I
> don't know much about IPC or shutdown, so it sounds like you would be much
more
> qualified than me to carry on this work, if you're interested in doing so.  
> 
> Richard.

Hi Richard,
  Will start the work on getting the data from child processes during shutdown.

  Would be great if the new files that were created in this CL as a separate CL
and get a review and land them (+1'ing eroman's statement).

thanks,
raman

[rlarocque, 2012-02-03 22:37:28]

On 2012/01/31 20:09:50, ramant wrote:
> On 2012/01/31 19:26:03, rlarocque wrote:
> > On 2012/01/31 01:12:44, ramant wrote:
> > >
> >
>
http://codereview.chromium.org/9125015/diff/1/chrome/browser/task_profiler/ta...
> > > File chrome/browser/task_profiler/task_profiler_data_serializer.cc
(right):
> > > 
> > >
> >
>
http://codereview.chromium.org/9125015/diff/1/chrome/browser/task_profiler/ta...
> > > chrome/browser/task_profiler/task_profiler_data_serializer.cc:32: // down,
> > then
> > > add that data to the 'per_process_data' array here.
> > > Hi rlarocque,
> > >   I had done collecting of data from child processes. Can work on getting
> data
> > > from child processes when we are shutting down.
> > > 
> > > a small question: any updates on the status of this patch?
> > > 
> > > thanks,
> > > raman
> > 
> > Hi Raman,
> > 
> > I had hoped to get this committed as is, because I could see some value in
the
> > information it does provide.  It would be nice if it could fetch data from
> child
> > processes, but the main process data is what I'm really after.  
> > 
> > I haven't had time to work on this lately, so there is no progress.  Also, I
> > don't know much about IPC or shutdown, so it sounds like you would be much
> more
> > qualified than me to carry on this work, if you're interested in doing so.  
> > 
> > Richard.
> 
> Hi Richard,
>   Will start the work on getting the data from child processes during
shutdown.
> 
>   Would be great if the new files that were created in this CL as a separate
CL
> and get a review and land them (+1'ing eroman's statement).
> 
> thanks,
> raman

I'm not sure I follow you.  Which parts of this patch should be dropped and
which should be carried over?

[ramant (doing other things), 2012-02-03 22:54:37]

On 2012/02/03 22:37:28, rlarocque wrote:
> On 2012/01/31 20:09:50, ramant wrote:
> > On 2012/01/31 19:26:03, rlarocque wrote:
> > > On 2012/01/31 01:12:44, ramant wrote:
> > > >
> > >
> >
>
http://codereview.chromium.org/9125015/diff/1/chrome/browser/task_profiler/ta...
> > > > File chrome/browser/task_profiler/task_profiler_data_serializer.cc
> (right):
> > > > 
> > > >
> > >
> >
>
http://codereview.chromium.org/9125015/diff/1/chrome/browser/task_profiler/ta...
> > > > chrome/browser/task_profiler/task_profiler_data_serializer.cc:32: //
down,
> > > then
> > > > add that data to the 'per_process_data' array here.
> > > > Hi rlarocque,
> > > >   I had done collecting of data from child processes. Can work on
getting
> > data
> > > > from child processes when we are shutting down.
> > > > 
> > > > a small question: any updates on the status of this patch?
> > > > 
> > > > thanks,
> > > > raman
> > > 
> > > Hi Raman,
> > > 
> > > I had hoped to get this committed as is, because I could see some value in
> the
> > > information it does provide.  It would be nice if it could fetch data from
> > child
> > > processes, but the main process data is what I'm really after.  
> > > 
> > > I haven't had time to work on this lately, so there is no progress.  Also,
I
> > > don't know much about IPC or shutdown, so it sounds like you would be much
> > more
> > > qualified than me to carry on this work, if you're interested in doing so.
 
> > > 
> > > Richard.
> > 
> > Hi Richard,
> >   Will start the work on getting the data from child processes during
> shutdown.
> > 
> >   Would be great if the new files that were created in this CL as a separate
> CL
> > and get a review and land them (+1'ing eroman's statement).
> > 
> > thanks,
> > raman
> 
> I'm not sure I follow you.  Which parts of this patch should be dropped and
> which should be carried over?

Would be great, if you could land this patch (with the TODO for me to get the
data from other processes). 

If not, I could reference this patch when we get all the data from all child
processes during shutdown. thanks.

[rlarocque, 2012-02-06 19:31:42]

> Would be great, if you could land this patch (with the TODO for me to get the
> data from other processes). 
> 
> If not, I could reference this patch when we get all the data from all child
> processes during shutdown. thanks.

I've rebased the patch and updated the TODO.  

Jim, could you PTAL?

[jar (doing other things), 2012-02-08 20:34:37]

A few nits... and then we should get this landed so that Raman can try to evolve
it.

Thanks!

http://codereview.chromium.org/9125015/diff/11001/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_main.h (right):

http://codereview.chromium.org/9125015/diff/11001/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main.h:164: task_profiler::AutoTracking
tracking_objects_;
This is as good as any for a first cut... but I'm sure we'll have to move this
around to get dumps before we're "too" shutdown (and unable to get data from
other processes).

http://codereview.chromium.org/9125015/diff/11001/chrome/browser/task_profile...
File chrome/browser/task_profiler/auto_tracking.h (right):

http://codereview.chromium.org/9125015/diff/11001/chrome/browser/task_profile...
chrome/browser/task_profiler/auto_tracking.h:10: #include "base/file_path.h"
nit: alphabetize

http://codereview.chromium.org/9125015/diff/11001/chrome/browser/task_profile...
chrome/browser/task_profiler/auto_tracking.h:30: output_file_path_ = path;
nit: move body of methods into auto_tracking.cc

http://codereview.chromium.org/9125015/diff/11001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/9125015/diff/11001/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:2466: 'browser/task_profiler/auto_tracking.cc',
nit: alphabetize

http://codereview.chromium.org/9125015/diff/11001/chrome/common/chrome_switch...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/9125015/diff/11001/chrome/common/chrome_switch...
chrome/common/chrome_switches.cc:986: // Specifies a path to output profiling
information at shutdown.
Please add to the comment to distinguish from the string on line 984.  Perhaps:

Specifies a path for the output of task-level-profiling, such as would can be
seen via about:profiler.

[rlarocque, 2012-02-08 23:49:44]

Thanks for the review.  Patch updated.

http://codereview.chromium.org/9125015/diff/11001/chrome/browser/task_profile...
File chrome/browser/task_profiler/auto_tracking.h (right):

http://codereview.chromium.org/9125015/diff/11001/chrome/browser/task_profile...
chrome/browser/task_profiler/auto_tracking.h:10: #include "base/file_path.h"
On 2012/02/08 20:34:37, jar wrote:
> nit: alphabetize

Done.

http://codereview.chromium.org/9125015/diff/11001/chrome/browser/task_profile...
chrome/browser/task_profiler/auto_tracking.h:30: output_file_path_ = path;
On 2012/02/08 20:34:37, jar wrote:
> nit: move body of methods into auto_tracking.cc

Done.

http://codereview.chromium.org/9125015/diff/11001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/9125015/diff/11001/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:2466: 'browser/task_profiler/auto_tracking.cc',
On 2012/02/08 20:34:37, jar wrote:
> nit: alphabetize

Done.

http://codereview.chromium.org/9125015/diff/11001/chrome/common/chrome_switch...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/9125015/diff/11001/chrome/common/chrome_switch...
chrome/common/chrome_switches.cc:986: // Specifies a path to output profiling
information at shutdown.
On 2012/02/08 20:34:37, jar wrote:
> Please add to the comment to distinguish from the string on line 984. 
Perhaps:
> 
> Specifies a path for the output of task-level-profiling, such as would can be
> seen via about:profiler.

Done, with slightly different text.  I wanted to make it clear the output file
can be loaded into about:profiler.

[jar (doing other things), 2012-02-09 18:07:48]

LGTM  (on the assumption that raman is picking this up.  We usually like to have
a feature in better or more final shape when we land... but I'm ok with a
hand-off via a landing of this, as it is going clearly in a desired direction).

[ramant (doing other things), 2012-02-09 18:29:34]

On 2012/02/09 18:07:48, jar wrote:
> LGTM  (on the assumption that raman is picking this up.  We usually like to
have
> a feature in better or more final shape when we land... but I'm ok with a
> hand-off via a landing of this, as it is going clearly in a desired
direction).

I will continue with this work and would like to make it reach final shape.
thanks.

[commit-bot: I haz the power, 2012-02-10 19:03:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/9125015/20001

[commit-bot: I haz the power, 2012-02-10 20:39:24]

Change committed as 121515