Telemetry: makes profilers a bit more generic.

Telemetry: makes profilers a bit more generic.

Rather than making assumptions at the "browser" level about pid and
output file name, push those as utilities and let each profiler decide those.
This will make it easy to implement TCMalloc dump fetcher, etc. in the future.
There are no functional changes in this patch.

BUG=

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

[bulach, 2013-05-13 13:49:10]

tonyg: this is NOT ready, but would appreciate your feedback if it's worth
trying... :)

in a nutshell:
- this slightly decouples "browser" from profiler, so they're more generic and
don't know about pid, file names, etc..

- however, what I'd like to do with the android jank meter isn't yet quite
possible: it needs to fill in "results", but "profiler.CollectProfile" is called
later too late, after PageMeasurementResults.DidMeasurePage() has been called...

so two questions:
- is worth it to make couple Profiler with PageMeasurementsResults somehow?
- if not, are the other parts of this patch (moving out of browser.py and into
profiler.py) worth it?

[tonyg, 2013-05-14 02:47:53]

I like it. Some suggestions and answers to your questions inline.

https://codereview.chromium.org/14978006/diff/2001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/core/browser.py (right):

https://codereview.chromium.org/14978006/diff/2001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/browser.py:171: profiler.CollectProfile(results)
Rather than give the profiler the results object, what about changing this
method to return a dict of arbitrary key-value results. Then have
smoothness_measurement.py create the profiler and call browser.StartProfiling()
in WillNavigateToPage() and get the results from StopProfiling() in
MeasurePage().

https://codereview.chromium.org/14978006/diff/2001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/core/platform/profiler/__init__.py (right):

https://codereview.chromium.org/14978006/diff/2001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/platform/profiler/__init__.py:25: def
_GetPIDInfo(self):
Can this be: _GetProcessOutputFileMap() and just return {'pid': pid,
'output_file': output_file}? Or better yet, return a list of NamedTuples. It
looks like cmd_line and process_name are never read from pid_info.

https://codereview.chromium.org/14978006/diff/2001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/core/platform/profiler/perf_profiler.py (right):

https://codereview.chromium.org/14978006/diff/2001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/platform/profiler/perf_profiler.py:15: def
__init__(self, browser_backend, platform_backend, output_path):
FYI, I added 2 new profilers to merge up with this patch.

https://codereview.chromium.org/14978006/diff/2001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/platform/profiler/perf_profiler.py:20:
self._perf_items = []
It might be better to factor out a SingleProcessPerfProfiler class in this file
with __init__() and CollectProfile() methods. Then keep an array of them rather
than this hard-to-read tuple.

https://codereview.chromium.org/14978006/diff/2001/tools/telemetry/telemetry/...
File
tools/telemetry/telemetry/core/platform/profiler/raw_display_profiler_android.py
(right):

https://codereview.chromium.org/14978006/diff/2001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/platform/profiler/raw_display_profiler_android.py:17:
return 'raw-display-profiler-android'
Should this be call surfaceflinger?

[bulach, 2013-05-14 10:45:37]

thanks tony! I addressed all the comments, but most important, removed the
android surface flinger so this patch now only focus on decoupling profilers and
pid.

https://codereview.chromium.org/14978006/diff/2001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/core/browser.py (right):

https://codereview.chromium.org/14978006/diff/2001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/browser.py:171: profiler.CollectProfile(results)
On 2013/05/14 02:47:53, tonyg wrote:
> Rather than give the profiler the results object, what about changing this
> method to return a dict of arbitrary key-value results. Then have
> smoothness_measurement.py create the profiler and call
browser.StartProfiling()
> in WillNavigateToPage() and get the results from StopProfiling() in
> MeasurePage().

that's a good idea.. I wanted to make the surface flinger orthogonal to a
particular measurment like these other profilers, that is, ideally we could just
pass --profiler=android-surface-flinger to any measurement and be done with it..

however :)
the first problem I want to solve is the profiler-per-pid, as required by
TCMalloc.. I'll focus on that for this patch, and will get back to the surface
flinger later on (it's not an immediate problem..)

https://codereview.chromium.org/14978006/diff/2001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/core/platform/profiler/__init__.py (right):

https://codereview.chromium.org/14978006/diff/2001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/platform/profiler/__init__.py:25: def
_GetPIDInfo(self):
On 2013/05/14 02:47:53, tonyg wrote:
> Can this be: _GetProcessOutputFileMap() and just return {'pid': pid,
> 'output_file': output_file}? Or better yet, return a list of NamedTuples. It
> looks like cmd_line and process_name are never read from pid_info.

Done.

https://codereview.chromium.org/14978006/diff/2001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/core/platform/profiler/perf_profiler.py (right):

https://codereview.chromium.org/14978006/diff/2001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/platform/profiler/perf_profiler.py:15: def
__init__(self, browser_backend, platform_backend, output_path):
On 2013/05/14 02:47:53, tonyg wrote:
> FYI, I added 2 new profilers to merge up with this patch.

Done.

https://codereview.chromium.org/14978006/diff/2001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/platform/profiler/perf_profiler.py:20:
self._perf_items = []
On 2013/05/14 02:47:53, tonyg wrote:
> It might be better to factor out a SingleProcessPerfProfiler class in this
file
> with __init__() and CollectProfile() methods. Then keep an array of them
rather
> than this hard-to-read tuple.

good idea! done.

https://codereview.chromium.org/14978006/diff/2001/tools/telemetry/telemetry/...
File
tools/telemetry/telemetry/core/platform/profiler/raw_display_profiler_android.py
(right):

https://codereview.chromium.org/14978006/diff/2001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/platform/profiler/raw_display_profiler_android.py:17:
return 'raw-display-profiler-android'
On 2013/05/14 02:47:53, tonyg wrote:
> Should this be call surfaceflinger?

as above, removed this class from this patch (now focusing on only on the
profiler-per-pid issue).

[tonyg, 2013-05-14 16:27:33]

Thanks for simplifying. Just a couple of more comments, everything else lg.

https://codereview.chromium.org/14978006/diff/11001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/platform/profiler/__init__.py (right):

https://codereview.chromium.org/14978006/diff/11001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/platform/profiler/__init__.py:36: 'pid_info',
['pid', 'output_file'])
I just realized, this would be even simpler as a dict. {pid1: output_file1,
pid2: output_file2}. Goes with the method name of OutputFileMap.

https://codereview.chromium.org/14978006/diff/11001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/platform/profiler/iprofiler_profiler.py
(right):

https://codereview.chromium.org/14978006/diff/11001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/platform/profiler/iprofiler_profiler.py:63:
self._single_process = []
This name isn't very clear. Maybe _process_profilers or
_single_process_profilers?

https://codereview.chromium.org/14978006/diff/11001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/platform/profiler/perf_profiler.py (right):

https://codereview.chromium.org/14978006/diff/11001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/platform/profiler/perf_profiler.py:51:
self._single_process = []
ditto

https://codereview.chromium.org/14978006/diff/11001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/platform/profiler/sample_profiler.py
(right):

https://codereview.chromium.org/14978006/diff/11001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/platform/profiler/sample_profiler.py:56:
self._single_process = []
ditto

[bulach, 2013-05-14 16:55:57]

thanks tony! all addressed, ptal

https://codereview.chromium.org/14978006/diff/11001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/platform/profiler/__init__.py (right):

https://codereview.chromium.org/14978006/diff/11001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/platform/profiler/__init__.py:36: 'pid_info',
['pid', 'output_file'])
On 2013/05/14 16:27:33, tonyg wrote:
> I just realized, this would be even simpler as a dict. {pid1: output_file1,
> pid2: output_file2}. Goes with the method name of OutputFileMap.

Done.

https://codereview.chromium.org/14978006/diff/11001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/platform/profiler/iprofiler_profiler.py
(right):

https://codereview.chromium.org/14978006/diff/11001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/platform/profiler/iprofiler_profiler.py:63:
self._single_process = []
On 2013/05/14 16:27:33, tonyg wrote:
> This name isn't very clear. Maybe _process_profilers or
> _single_process_profilers?

Done.

https://codereview.chromium.org/14978006/diff/11001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/platform/profiler/perf_profiler.py (right):

https://codereview.chromium.org/14978006/diff/11001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/platform/profiler/perf_profiler.py:51:
self._single_process = []
On 2013/05/14 16:27:33, tonyg wrote:
> ditto

Done.

https://codereview.chromium.org/14978006/diff/11001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/platform/profiler/sample_profiler.py
(right):

https://codereview.chromium.org/14978006/diff/11001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/platform/profiler/sample_profiler.py:56:
self._single_process = []
On 2013/05/14 16:27:33, tonyg wrote:
> ditto

Done.

[tonyg, 2013-05-14 17:21:12]

lgtm

Really clean, thanks!

[commit-bot: I haz the power, 2013-05-15 09:10:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/14978006/18001

[commit-bot: I haz the power, 2013-05-15 09:17:04]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-05-16 08:58:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/14978006/18001

[commit-bot: I haz the power, 2013-05-16 09:38:48]

Retried try job too often on linux_chromeos for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-05-16 12:42:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/14978006/18001

[commit-bot: I haz the power, 2013-05-16 13:49:16]

Retried try job too often on linux_chromeos for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-05-16 15:34:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/14978006/18001

[commit-bot: I haz the power, 2013-05-16 16:05:38]

Retried try job too often on linux_chromeos for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-05-17 08:08:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/14978006/18001

[commit-bot: I haz the power, 2013-05-17 09:27:12]

Change committed as 200773