Moved CPU Usage helper functions from Youtube class to BasePerfTest class in perf.py.

Moved CPU Usage helper functions from Youtube class to BasePerfTest class in perf.py.

I plan to use the CPU Usage functions in other non-Youtube related perf
tests, so moving these helper functions from the Youtube perf class to the
base perf class.  Also some minor style edits to the Youtube perf test
code.

BUG=None
TEST=None

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

[rohitbm, 2011-11-18 00:24:53]

LGTM

Thanks Dennis for making Youtube perf test code clear. You may still need an
LGTM from Nirnimesh or Kris to submit the code.

http://codereview.chromium.org/8585050/diff/1/chrome/test/functional/perf.py
File chrome/test/functional/perf.py (right):

http://codereview.chromium.org/8585050/diff/1/chrome/test/functional/perf.py#...
chrome/test/functional/perf.py:609: if sec_num < num_seconds - 1:  # Do not
sleep on the last iteration.
Should we use xrange(num_seconds-1) above instead of checking sec_num <
num_seconds-1 in each iteration?

[dennis_jeffrey, 2011-11-18 00:51:29]

Thank you for the review!  I made a slight change to the code based on your
comment.  I'll go ahead and try to submit now and see if things blow up (which
could indicate I might need another reviewer).

http://codereview.chromium.org/8585050/diff/1/chrome/test/functional/perf.py
File chrome/test/functional/perf.py (right):

http://codereview.chromium.org/8585050/diff/1/chrome/test/functional/perf.py#...
chrome/test/functional/perf.py:609: if sec_num < num_seconds - 1:  # Do not
sleep on the last iteration.
On 2011/11/18 00:24:53, rohitbm wrote:
> Should we use xrange(num_seconds-1) above instead of checking sec_num <
> num_seconds-1 in each iteration?

That's a good question.  I actually do want the loop to iterate 60 times like it
did before (that's why I'm using xrange(num_seconds).  However, on the very last
loop iteration, after we update total_shown_frames for the final time, I want to
avoid sleeping in that case.  The reason is because the length of time that we
sleep affects the overall results, since we're computing the difference in CPU
usage between |cpu_usage_start| and |cpu_usage_end|.  If we let the video play
for 1 more second in the last loop iteration, then exit the loop, then the CPU
has been utilized for 1 more second, which we have not accounted for in the
|total_shown_frames| variable.  Does that make sense?

But you do have a point that it's undesirable to have an extra check on each
loop iteration.  I think that if we reverse the order of the statements inside
the loop (i.e., sleep first, then update |total_shown_frames|, I think that
would be better.  I went ahead and made that change ;-)

[rohitbm, 2011-11-18 01:39:48]

On 2011/11/18 00:51:29, dennis_jeffrey wrote:
> Thank you for the review!  I made a slight change to the code based on your
> comment.  I'll go ahead and try to submit now and see if things blow up (which
> could indicate I might need another reviewer).
> 
> http://codereview.chromium.org/8585050/diff/1/chrome/test/functional/perf.py
> File chrome/test/functional/perf.py (right):
> 
>
http://codereview.chromium.org/8585050/diff/1/chrome/test/functional/perf.py#...
> chrome/test/functional/perf.py:609: if sec_num < num_seconds - 1:  # Do not
> sleep on the last iteration.
> On 2011/11/18 00:24:53, rohitbm wrote:
> > Should we use xrange(num_seconds-1) above instead of checking sec_num <
> > num_seconds-1 in each iteration?
> 
> That's a good question.  I actually do want the loop to iterate 60 times like
it
> did before (that's why I'm using xrange(num_seconds).  However, on the very
last
> loop iteration, after we update total_shown_frames for the final time, I want
to
> avoid sleeping in that case.  The reason is because the length of time that we
> sleep affects the overall results, since we're computing the difference in CPU
> usage between |cpu_usage_start| and |cpu_usage_end|.  If we let the video play
> for 1 more second in the last loop iteration, then exit the loop, then the CPU
> has been utilized for 1 more second, which we have not accounted for in the
> |total_shown_frames| variable.  Does that make sense?
Thanks for the explanation! That makes sense.
> 
> But you do have a point that it's undesirable to have an extra check on each
> loop iteration.  I think that if we reverse the order of the statements inside
> the loop (i.e., sleep first, then update |total_shown_frames|, I think that
> would be better.  I went ahead and made that change ;-)