[Telemetry] Add memory and IO stats to page cycler

[Telemetry] Add memory and IO stats to page cycler

This matches the existing page_cycler behavior exactly.

Also, only display by_url results when there are at least 2 pages in the
page_set. This prevents duplicate results for single page sets.

BUG=163680
TEST=tools/perf/run_multipage_benchmarks --browser=system page_cycler tools/perf/page_sets/page_cycler/moz.json
NOTRY=True

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

[tonyg, 2012-12-15 02:35:32]

ptal

[nduca, 2012-12-15 05:45:31]

https://codereview.chromium.org/11576050/diff/1/tools/telemetry/telemetry/pla...
File tools/telemetry/telemetry/platform.py (right):

https://codereview.chromium.org/11576050/diff/1/tools/telemetry/telemetry/pla...
tools/telemetry/telemetry/platform.py:23: 
I wonder if this should be exposed via devtools instead? We have a common ps
stats api, which even deals with cross plat stuff ("commit charge" is very
windows specific, no?) and I'd rather use that than support our own memory api
from python.

[tonyg, 2012-12-17 18:16:15]

On 2012/12/15 05:45:31, nduca wrote:
>
https://codereview.chromium.org/11576050/diff/1/tools/telemetry/telemetry/pla...
> File tools/telemetry/telemetry/platform.py (right):
> 
>
https://codereview.chromium.org/11576050/diff/1/tools/telemetry/telemetry/pla...
> tools/telemetry/telemetry/platform.py:23: 
> I wonder if this should be exposed via devtools instead? We have a common ps
> stats api, which even deals with cross plat stuff ("commit charge" is very
> windows specific, no?) and I'd rather use that than support our own memory api
> from python.

One thing I really like about the python approach is that it is cross browser.
Anything else we do would be chrome-specific.

Second, this stuff is all copied exactly from base/process_util and
chrome/test/perf/perf_test, so it is the set of metrics we need to replicate the
existing functionality. However, I can't see creating APIs for all of this junk.
There's a lot of platform specific differences, for instance on windows we won't
have the IO stats and we will have WSS instead of RSS. 

Help me understand the rationale for not wanting to do this in python.

[nduca, 2012-12-17 20:21:00]

Two things here....

Wrt py vs c++, Its not strictly copied. You're using something new to get the
data instead of using the base::ProcessMetrics ChromeProcessMetrics and a fair
bit of work and trickery goes into maintaining that.  We should just be exposing
that than creating a separate system that requires tweaking. Again, the more we
re-use existing stuff the simpler explanations are... think of the debate with
someone: "what's the memory stat telemetry reports?" "oh, its just base".

I also think that if we do a ff backend, the metrics reported for memory may
have some commonality with us, but they may also want some unique ones. So,
again, I think its browser specific or at least partly so.

A general piece of concern, not about py vs c++, perf tests should be able to
get all perf metrics as one object, e.g. GetMemoryStats(). A person should be
able to get memory stats, not have to do tons of dfferent calls per platform.

Finally, I'm concerned about our incremental accumulation of metrics that we're
getting via histogram, vs via this utility, vs say some otehr thing we use to
get memory from devtools, vs some yet other tool we have for getting gpu memory.
Can we have one clean solution? Or is this impossible?

[nduca, 2013-01-08 05:12:50]

Recapping, I changed my mind about using python to gather metrics being a bad
thing. It seems fine.

It would be cool if we could hide pids, and instead return memory stats by
process type, e.g.
   browser.platform.GetMemoryStats() -->
     {"browser": {dict of memory stats},
      "renderer": {dict of memory stats}}

Lets also make sure the docstrings explain that the stats are  completely system
specific. Or make each return an actual defined class, e.g IOStats. Something
that makes the helptext on the different platforms more understandable.

[tonyg, 2013-02-27 23:02:56]

All comments addressed.

@dtu, would you mind taking a look at this while nduca is gone?

[dtu, 2013-03-01 00:57:30]

I know nduca would have some things to say about this... For example, he was
pretty clear about "NO pids. :)"

https://chromiumcodereview.appspot.com/11576050/diff/23002/tools/telemetry/te...
File tools/telemetry/telemetry/core/chrome/platform.py (right):

https://chromiumcodereview.appspot.com/11576050/diff/23002/tools/telemetry/te...
tools/telemetry/telemetry/core/chrome/platform.py:76: return
NotImplementedError()
raise*

[dtu, 2013-03-01 01:07:26]

On 2013/03/01 00:57:30, Dave Tu wrote:
> I know nduca would have some things to say about this... For example, he was
> pretty clear about "NO pids. :)"
> 

One option for this might be to have Platform itself expose memory_stats and
io_stats and not the pid stuff.

>
https://chromiumcodereview.appspot.com/11576050/diff/23002/tools/telemetry/te...
> File tools/telemetry/telemetry/core/chrome/platform.py (right):
> 
>
https://chromiumcodereview.appspot.com/11576050/diff/23002/tools/telemetry/te...
> tools/telemetry/telemetry/core/chrome/platform.py:76: return
> NotImplementedError()
> raise*

[tonyg, 2013-03-01 01:20:24]

On 2013/03/01 01:07:26, Dave Tu wrote:
> On 2013/03/01 00:57:30, Dave Tu wrote:
> > I know nduca would have some things to say about this... For example, he was
> > pretty clear about "NO pids. :)"
> > 
> 
> One option for this might be to have Platform itself expose memory_stats and
> io_stats and not the pid stuff.
> 

So, Nat didn't say no pids, he said to hide the pids. We chatted about this in
person and the thing that bothered him about my original patch was exposing pid
in the browser interface. My rework pulled the pids out of the browser interface
and held it to the backend. Which I believe addresses his concern.

> >
>
https://chromiumcodereview.appspot.com/11576050/diff/23002/tools/telemetry/te...
> > File tools/telemetry/telemetry/core/chrome/platform.py (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/11576050/diff/23002/tools/telemetry/te...
> > tools/telemetry/telemetry/core/chrome/platform.py:76: return
> > NotImplementedError()
> > raise*

Good catch. That was a copy/paste error on my part. I fixed all included the
original.

I also added cros support so this works everywhere now.

[tonyg, 2013-03-01 01:21:32]

On 2013/03/01 01:20:24, tonyg wrote:
> On 2013/03/01 01:07:26, Dave Tu wrote:
> > On 2013/03/01 00:57:30, Dave Tu wrote:
> > > I know nduca would have some things to say about this... For example, he
was
> > > pretty clear about "NO pids. :)"
> > > 
> > 
> > One option for this might be to have Platform itself expose memory_stats and
> > io_stats and not the pid stuff.
> > 
> 
> So, Nat didn't say no pids, he said to hide the pids. We chatted about this in
> person and the thing that bothered him about my original patch was exposing
pid
> in the browser interface. My rework pulled the pids out of the browser
interface
> and held it to the backend. Which I believe addresses his concern.

Oh I meant to say, the problem with moving memory_stats / io_stats directly to
the platform is that it would push redundant code into the platform specific
files.

> 
> > >
> >
>
https://chromiumcodereview.appspot.com/11576050/diff/23002/tools/telemetry/te...
> > > File tools/telemetry/telemetry/core/chrome/platform.py (right):
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/11576050/diff/23002/tools/telemetry/te...
> > > tools/telemetry/telemetry/core/chrome/platform.py:76: return
> > > NotImplementedError()
> > > raise*
> 
> Good catch. That was a copy/paste error on my part. I fixed all included the
> original.
> 
> I also added cros support so this works everywhere now.

[dtu, 2013-03-01 02:35:14]

On 2013/03/01 01:21:32, tonyg wrote:
> On 2013/03/01 01:20:24, tonyg wrote:
> > On 2013/03/01 01:07:26, Dave Tu wrote:
> > > On 2013/03/01 00:57:30, Dave Tu wrote:
> > > > I know nduca would have some things to say about this... For example, he
> was
> > > > pretty clear about "NO pids. :)"
> > > > 
> > > 
> > > One option for this might be to have Platform itself expose memory_stats
and
> > > io_stats and not the pid stuff.
> > > 
> > 
> > So, Nat didn't say no pids, he said to hide the pids. We chatted about this
in
> > person and the thing that bothered him about my original patch was exposing
> pid
> > in the browser interface. My rework pulled the pids out of the browser
> interface
> > and held it to the backend. Which I believe addresses his concern.
> 
> Oh I meant to say, the problem with moving memory_stats / io_stats directly to
> the platform is that it would push redundant code into the platform specific
> files.

The problem is that the pids are still not hidden, because Platform is also
publicly exposed via browser.platform, so a user could still do
browser.platform.GetChildPids(). It's currently unclear whether Platform
is/should be public or backend. Another possibility is getting rid of
browser.platform, making Platform a backend or Chrome-only abstraction.

> 
> > 
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/11576050/diff/23002/tools/telemetry/te...
> > > > File tools/telemetry/telemetry/core/chrome/platform.py (right):
> > > > 
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/11576050/diff/23002/tools/telemetry/te...
> > > > tools/telemetry/telemetry/core/chrome/platform.py:76: return
> > > > NotImplementedError()
> > > > raise*
> > 
> > Good catch. That was a copy/paste error on my part. I fixed all included the
> > original.
> > 
> > I also added cros support so this works everywhere now.

[tonyg, 2013-03-01 19:44:59]

> The problem is that the pids are still not hidden, because Platform is also
> publicly exposed via browser.platform, so a user could still do
> browser.platform.GetChildPids(). It's currently unclear whether Platform
> is/should be public or backend. Another possibility is getting rid of
> browser.platform, making Platform a backend or Chrome-only abstraction.

Excellent point. I reworked things so that the new methods are not exposed to
the platform interface (only PlatformBackend). Let me know what you think.

[dtu, 2013-03-01 22:36:09]

On 2013/03/01 19:44:59, tonyg wrote:
> Excellent point. I reworked things so that the new methods are not exposed to
> the platform interface (only PlatformBackend). Let me know what you think.

This looks better, thanks! Now we need an abstract base class
platform_backend.py that ensures the methods are implemented. There's already
precedent for this with browser_backend.py

[tonyg, 2013-03-01 23:02:15]

> Now we need an abstract base class
> platform_backend.py that ensures the methods are implemented.

Good idea, that is cleaner. Done. ptal

[dtu, 2013-03-04 23:26:15]

Cool! Just nits. lgtm.

https://codereview.chromium.org/11576050/diff/34002/tools/perf/perf_tools/pag...
File tools/perf/perf_tools/page_cycler.py (right):

https://codereview.chromium.org/11576050/diff/34002/tools/perf/perf_tools/pag...
tools/perf/perf_tools/page_cycler.py:29: if memory['Browser'].has_key('VM'):
if 'VM' in memory['Browser']:
has_key is deprecated

https://codereview.chromium.org/11576050/diff/34002/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/chrome/cros_browser_backend.py (right):

https://codereview.chromium.org/11576050/diff/34002/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/chrome/cros_browser_backend.py:95: return 0
None

https://codereview.chromium.org/11576050/diff/34002/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/chrome/desktop_browser_backend.py (right):

https://codereview.chromium.org/11576050/diff/34002/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/chrome/desktop_browser_backend.py:80: return 0
None

https://codereview.chromium.org/11576050/diff/34002/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/chrome/desktop_browser_finder.py (right):

https://codereview.chromium.org/11576050/diff/34002/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/chrome/desktop_browser_finder.py:1: # Copyright
(c) 2012 The Chromium Authors. All rights reserved.
Note for the future: create linux/mac/win subclasses for this file.

https://codereview.chromium.org/11576050/diff/34002/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/chrome/linux_platform_backend.py (right):

https://codereview.chromium.org/11576050/diff/34002/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/chrome/linux_platform_backend.py:67: pid_ppid =
pid_ppid.split()
pid, ppid = pid_ppid.split()

[tonyg, 2013-03-04 23:40:56]

Thanks for the review! All comments addressed.

[commit-bot: I haz the power, 2013-03-04 23:41:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tonyg@chromium.org/11576050/28003

[commit-bot: I haz the power, 2013-03-05 02:43:14]

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

[commit-bot: I haz the power, 2013-03-05 17:27:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tonyg@chromium.org/11576050/28003

[commit-bot: I haz the power, 2013-03-05 18:53:59]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2013-03-05 19:07:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tonyg@chromium.org/11576050/28003

[commit-bot: I haz the power, 2013-03-05 19:08:47]

Change committed as 186219

[nduca, 2013-03-08 06:30:02]

Love this, btw.