Telemetry: Linux-based backends should return CPU results in seconds, not jiffies.

Telemetry: Linux-based backends should return CPU results in seconds, not jiffies.

Committed: https://crrev.com/8da2981fc70a7524c4516d0c5f7340c2c7a94da5
Cr-Commit-Position: refs/heads/master@{#302004}

[Lei Zhang, 2014-10-28 07:47:02]

thestig@chromium.org changed reviewers:
+ jeremy@chromium.org, tonyg@chromium.org

[jeremy, 2014-10-28 11:33:53]

https://codereview.chromium.org/684713003/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/platform/linux_based_platform_backend.py
(right):

https://codereview.chromium.org/684713003/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/linux_based_platform_backend.py:95: def
GetClockTicks(self):
Can you add a comment for what this does?
Any chance that os.sysconf() returns 0 ?

[Lei Zhang, 2014-10-28 20:56:58]

https://codereview.chromium.org/684713003/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/platform/linux_based_platform_backend.py
(right):

https://codereview.chromium.org/684713003/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/linux_based_platform_backend.py:95: def
GetClockTicks(self):
On 2014/10/28 11:33:53, jeremy wrote:
> Can you add a comment for what this does?

Done.

> Any chance that os.sysconf() returns 0 ?

I don't think so. Noted in the comment.

[jeremy, 2014-10-29 06:11:32]

LGTM

https://codereview.chromium.org/684713003/diff/20001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/core/platform/linux_based_platform_backend_unittest.py
(right):

https://codereview.chromium.org/684713003/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/linux_based_platform_backend_unittest.py:38:
backend.SetMockFile('/proc/1/stat', f.read())
IMHO it might be a bit cleaner if you split these 2 lines into a function?

[Lei Zhang, 2014-10-29 07:03:08]

https://codereview.chromium.org/684713003/diff/20001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/core/platform/linux_based_platform_backend_unittest.py
(right):

https://codereview.chromium.org/684713003/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/linux_based_platform_backend_unittest.py:38:
backend.SetMockFile('/proc/1/stat', f.read())
On 2014/10/29 06:11:32, jeremy wrote:
> IMHO it might be a bit cleaner if you split these 2 lines into a function?

Done. Can you take a look again to make sure I did what I think you asked?

[jeremy, 2014-10-29 07:05:32]

Yes, that's exactly what I had in mind, thanks!

[Lei Zhang, 2014-10-29 07:17:46]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-10-29 07:18:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/684713003/40001

[commit-bot: I haz the power, 2014-10-29 07:26:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-29 07:26:03]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[tonyg, 2014-10-29 16:47:17]

https://codereview.chromium.org/684713003/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/linux_based_platform_backend.py
(right):

https://codereview.chromium.org/684713003/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/linux_based_platform_backend.py:96:
"""Returns the number of clock ticks per second. It is never 0."""
Should we assert that it is never 0?

https://codereview.chromium.org/684713003/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/linux_based_platform_backend.py:97:
return os.sysconf('SC_CLK_TCK')
This won't work on Android+CrOS because it is running on the host, not the
device. In order to get it from the device, we'd have to use one of the wrappers
like GetFileContents(SOME_FILE_IN_PROC_OR_SOMETHING)

[Lei Zhang, 2014-10-29 21:35:53]

https://codereview.chromium.org/684713003/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/linux_based_platform_backend.py
(right):

https://codereview.chromium.org/684713003/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/linux_based_platform_backend.py:97:
return os.sysconf('SC_CLK_TCK')
On 2014/10/29 16:47:16, tonyg wrote:
> This won't work on Android+CrOS because it is running on the host, not the
> device. In order to get it from the device, we'd have to use one of the
wrappers
> like GetFileContents(SOME_FILE_IN_PROC_OR_SOMETHING)

It's not exposed through proc. Someone tried to add a patch to do so to the
kernel but got rejected. From the command line, one would call getconf, except
Android and CrOS do not have getconf, so we are kind of screwed.

Fortunately, the return value here will always be 100 on most Linux platforms.
http://lxr.free-electrons.com/source/include/asm-generic/param.h#L8

The kernel has an internal HZ constant and a USER_HZ constant for things exposed
to userland. The Ubuntu Linux kernel has CONFIG_HZ set to 250, yet the return
value here is 100. I tested with a program to burn some CPU cycles and noted
every second, the jiffies increment by 100. So, I'm hard coding 100 here and
calling it a day.

[tonyg, 2014-10-29 21:44:09]

no better ideas, so lgtm :/

[Lei Zhang, 2014-10-29 21:53:49]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-10-29 21:55:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/684713003/60001

[commit-bot: I haz the power, 2014-10-29 23:47:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-29 23:47:39]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tonyg, 2014-10-29 23:49:09]

On 2014/10/29 23:47:39, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   win_chromium_x64_rel on tryserver.chromium.win
>
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

Failures look legit to me

[Lei Zhang, 2014-10-29 23:58:52]

On 2014/10/29 23:49:09, tonyg wrote:
> On 2014/10/29 23:47:39, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   win_chromium_x64_rel on tryserver.chromium.win
> >
>
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
> 
> Failures look legit to me

Fixed. (1 + 4) jiffies = 0.05 seconds.

[Lei Zhang, 2014-10-29 23:58:56]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-10-30 00:01:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/684713003/80001

[commit-bot: I haz the power, 2014-10-30 01:12:58]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-10-30 01:13:39]

Patchset 5 (id:??) landed as
https://crrev.com/8da2981fc70a7524c4516d0c5f7340c2c7a94da5
Cr-Commit-Position: refs/heads/master@{#302004}