Added frequency stats and c-state residency to CrOS power monitor.

Added frequency stats and c-state residency to CrOS power monitor.

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

[tmandel, 2014-06-27 02:03:06]

I've added c-state residency and frequency data to the power monitor for
ChromeOS. I also moved the battery data to be under 'Battery' in the results
instead of in the 'whole_package' dictionary. The only thing that differs from
the API is that the percentage of time spent in each frequency is recorded
instead of the average frequency of the CPU during the test, as the comments on
the API Doc suggest against averages.

[tonyg, 2014-06-27 02:13:37]

This is awesome! It looks like a lot of this would work on linux and android as
well. Should we factor out a sysfs_power_monitor.py that extends power_monitor
and pulls all the interesting stats from sysfs? It'd gracefully degrade if the
necessary files don't exist on the target system. Then we can make the various
android power monitors extend it and make a new linux_power_monitor which
extends it as well.

BTW, we should try to rectify this with
https://codereview.chromium.org/239083010/ . Perhaps this approach would
supersede that?

[dbasehore, 2014-06-27 18:18:38]

https://codereview.chromium.org/352583007/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/platform/power_monitor/cros_power_monitor.py
(right):

https://codereview.chromium.org/352583007/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/power_monitor/cros_power_monitor.py:34:
_initial_power: The result of 'power_supply_info' before the test.
Do we plan to collect CPU information outside of the power monitor? If so, we
might want to break this out into a separate class that the power monitor can
just have a reference to.

[tmandel, 2014-06-27 21:19:25]

On 2014/06/27 02:13:37, tonyg wrote:
> This is awesome! It looks like a lot of this would work on linux and android
as
> well. Should we factor out a sysfs_power_monitor.py that extends power_monitor
> and pulls all the interesting stats from sysfs? It'd gracefully degrade if the
> necessary files don't exist on the target system. Then we can make the various
> android power monitors extend it and make a new linux_power_monitor which
> extends it as well.
> 
> BTW, we should try to rectify this with
> https://codereview.chromium.org/239083010/ . Perhaps this approach would
> supersede that?

I think pretty much all of it except calculating the c-state residency times
would be the same on linux and android. It seems that android does have a
recorded time associated with C0 where Chrome OS does not, so each would have to
handle getting the proper numbers on their own. I'm not sure where we would put
that so that this new power monitor could call the right one. Also, the method
names, arguments, and return types for running a shell command differ among the
platforms, so we would have to figure out what to do to fix that. That being
said, all other CPU-related static methods for parsing and calculating averages
would be the same across all platforms. 

Do you have any suggestions?

[tmandel, 2014-07-01 20:11:10]

I've moved the CPU monitoring parts that can be shared across platforms into
sysfs_power_monitor.py and adjusted cros_power_monitor.py to extend it.

Each platform runs shell commands and parses c-state data differently, so I
created a class called SysfsPlatform that has an API for these functions. I
wasn't sure what convention is for where to put these classes or how to name
them of if they should even exist, so comments are welcome. Alternatively, these
functions could go in the <platform>_platform_backend.py file and
SysfsPowerMonitor could be given a reference to the backend at initialization,
but then the backend and power monitors would have references to each other. If
this is preferred I'll change it. Other suggestions are also welcome.

[dbasehore, 2014-07-08 23:47:31]

https://codereview.chromium.org/352583007/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/power_monitor/sysfs_platform.py
(right):

https://codereview.chromium.org/352583007/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/sysfs_platform.py:5: class
SysfsPlatform(object):
It might be better to put this functionality in the sysfs power monitor.
Separating out 2 static methods doesn't seem to be worth an additional class.

https://codereview.chromium.org/352583007/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/sysfs_platform.py:9: def
RunShellCommand(self, command):
There's already a _RunCommand method in posix_platform_backend and
cros_platform_backend. To use this without refactoring the code would require
the platform backend class to pass self to the power monitor though. I don't
have a a strong opinion either way.

[tbroch1, 2014-07-10 19:19:12]

https://codereview.chromium.org/352583007/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/power_monitor/sysfs_platform.py
(right):

https://codereview.chromium.org/352583007/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/sysfs_platform.py:25:
sample: A single sample of c-state residency times to be parsed.
nit: mention type of data (dict) and organization (set of cstate names,
corresponding times & latencies separated by cr)

[tmandel, 2014-07-14 22:29:39]

PTAL

Is there a preference by the owners to have the platform-specific functions in
the appropriate platform_backend file or in a separate class like I currently
have?

https://codereview.chromium.org/352583007/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/power_monitor/sysfs_platform.py
(right):

https://codereview.chromium.org/352583007/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/sysfs_platform.py:5: class
SysfsPlatform(object):
On 2014/07/08 23:47:31, dbasehore wrote:
> It might be better to put this functionality in the sysfs power monitor.
> Separating out 2 static methods doesn't seem to be worth an additional class.

I'm not sure how I could put it in sysfs_power_monitor without just implementing
it for all platforms that could possibly extend this and then choosing based on
which subclass calls it. The subclass should know how to run commands and parse
c-state data, so I'd think it'd be the subclass's job to provide the proper
implementation to the superclass. I'd prefer to put it in the platform_backend
files, but that requires the backend to send self to the power monitors.

https://codereview.chromium.org/352583007/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/sysfs_platform.py:9: def
RunShellCommand(self, command):
On 2014/07/08 23:47:31, dbasehore wrote:
> There's already a _RunCommand method in posix_platform_backend and
> cros_platform_backend. To use this without refactoring the code would require
> the platform backend class to pass self to the power monitor though. I don't
> have a a strong opinion either way.

If this is preferred I can change it to use the appropriate backends.

https://codereview.chromium.org/352583007/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/sysfs_platform.py:25:
sample: A single sample of c-state residency times to be parsed.
On 2014/07/10 19:19:12, tbroch1 wrote:
> nit: mention type of data (dict) and organization (set of cstate names,
> corresponding times & latencies separated by cr)

Done.

[tonyg, 2014-07-17 21:50:06]

lgtm

A couple of nits, but feel free to land after that.

https://codereview.chromium.org/352583007/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/power_monitor/sysfs_platform.py
(right):

https://codereview.chromium.org/352583007/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/power_monitor/sysfs_platform.py:37:
class CrosSysfsPlatform(SysfsPlatform):
Let's move this to a new file.

https://codereview.chromium.org/352583007/diff/100001/tools/telemetry/telemet...
File
tools/telemetry/telemetry/core/platform/power_monitor/sysfs_power_monitor.py
(right):

https://codereview.chromium.org/352583007/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/power_monitor/sysfs_power_monitor.py:37:
'ls %s | grep -w "^cpu[0-9]*"' % CPU_PATH).split()
Can we do the grep in python instead? Android, for example, has sysfs but not
grep.

[tmandel, 2014-07-17 22:59:25]

https://codereview.chromium.org/352583007/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/power_monitor/sysfs_platform.py
(right):

https://codereview.chromium.org/352583007/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/power_monitor/sysfs_platform.py:37:
class CrosSysfsPlatform(SysfsPlatform):
On 2014/07/17 21:50:06, tonyg wrote:
> Let's move this to a new file.

Done.

https://codereview.chromium.org/352583007/diff/100001/tools/telemetry/telemet...
File
tools/telemetry/telemetry/core/platform/power_monitor/sysfs_power_monitor.py
(right):

https://codereview.chromium.org/352583007/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/power_monitor/sysfs_power_monitor.py:37:
'ls %s | grep -w "^cpu[0-9]*"' % CPU_PATH).split()
On 2014/07/17 21:50:06, tonyg wrote:
> Can we do the grep in python instead? Android, for example, has sysfs but not
> grep.

Done.

[tmandel, 2014-07-17 23:00:06]

The CQ bit was checked by tmandel@chromium.org

[commit-bot: I haz the power, 2014-07-17 23:01:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tmandel@chromium.org/352583007/120001

[tmandel, 2014-07-18 01:39:55]

The CQ bit was unchecked by tmandel@chromium.org

[tmandel, 2014-07-18 01:53:13]

I forgot to move a unit test to a new file and update the imports, so the bots
were failing. Feel free to take another look if you'd like.

[tmandel, 2014-07-18 16:46:39]

The CQ bit was checked by tmandel@chromium.org

[commit-bot: I haz the power, 2014-07-18 16:47:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tmandel@chromium.org/352583007/140001

[commit-bot: I haz the power, 2014-07-18 19:32:22]

Change committed as 284183