Create bootperf-bin tools for running boot time tests and reporting results

Create bootperf-bin tools for running boot time tests and reporting results

BUG=chromium-os:8655
TEST=run bootperf as described by 'bootperf --help'
TEST=run showbootstats as described by 'showbootstats --help'

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=2c00282

[truty, 2010-11-02 00:45:46]

lgtm - looks very nice. Only shortcoming is if it's not uncommon for a single
data point to be missed in one of the keys it might be prudent to accept it
instead of rejecting the whole set after a very long run. Also, the Python code
is much nicer to review than the bash code.

http://codereview.chromium.org/4148011/diff/14001/12007
File bootperf-bin/bootperf (right):

http://codereview.chromium.org/4148011/diff/14001/12007#newcode70
bootperf-bin/bootperf:70: <ip-address>.  By default, the test is run once; if
<count> is
I think you should indicate that one run is 10 iterations here.

http://codereview.chromium.org/4148011/diff/14001/12009
File bootperf-bin/resultset.py (right):

http://codereview.chromium.org/4148011/diff/14001/12009#newcode71
bootperf-bin/resultset.py:71: return check[0]
I'm curious to know how frequently this occurs.

http://codereview.chromium.org/4148011/diff/14001/12009#newcode176
bootperf-bin/resultset.py:176: keylist = self._keyvals.setdefault(shortkey, [])
Do you think the Python interpreter is smart enough to hoist the calculation of
shortkey out of the loop? This stands out to me.

http://codereview.chromium.org/4148011/diff/14001/12010
File bootperf-bin/showbootdata (right):

http://codereview.chromium.org/4148011/diff/14001/12010#newcode53
bootperf-bin/showbootdata:53: "-d", "--diskstats", action="store_false",
It's nitty, but since this is used so closely with bootperf I think it's
confusing that you use -d for directory in bootperf and positional directory
here (with -d meaning something completely different). I would unify the
interface to the two tools for new users.

http://codereview.chromium.org/4148011/diff/14001/12010#newcode54
bootperf-bin/showbootdata:54: dest="use_timestats",
So, what happens if I specify -d and -t.  Should the UI check for this instead
of just defaulting to time stats?

http://codereview.chromium.org/4148011/diff/14001/12010#newcode79
bootperf-bin/showbootdata:79: optparser.add_option_group(optgroup)
Same comment as before with regard to handling both -a and -r.

[jrbarnette, 2010-11-02 21:22:11]

http://codereview.chromium.org/4148011/diff/14001/12009
File bootperf-bin/resultset.py (right):

http://codereview.chromium.org/4148011/diff/14001/12009#newcode71
bootperf-bin/resultset.py:71: return check[0]
On 2010/11/02 00:45:46, truty wrote:
> I'm curious to know how frequently this occurs.

This code can be expected to execute the 'return check[0]'
code path *every* time.  Realistically, the only way it could fail would
be if the user ran bootperf multiple times on the same results directory,
and the different runs used different versions of platform_BootPerf that
produced different keyval sets.

The check here is as much documentation as safety:  The dependency
on the keyval sets being the same for every iteration is baked in to the
code with things like the 'num_iterations' field.  The check here makes the
dependency more obvious.

[jrbarnette, 2010-11-02 21:25:10]

On 2010/11/02 00:45:46, truty wrote:
> lgtm - looks very nice. Only shortcoming is if it's not uncommon for a single
> data point to be missed in one of the keys it might be prudent to accept it
> instead of rejecting the whole set after a very long run. Also, the Python
code
> is much nicer to review than the bash code.
> 
See the response regarding _docheck():  It should never happen that
we wind up missing a data point.  The only ways I know of for this to
happen imply a corrupt or bogus data set, so failing to produce output
is better than silently working around the condition.

[jrbarnette, 2010-11-02 21:28:05]

http://codereview.chromium.org/4148011/diff/14001/12009
File bootperf-bin/resultset.py (right):

http://codereview.chromium.org/4148011/diff/14001/12009#newcode176
bootperf-bin/resultset.py:176: keylist = self._keyvals.setdefault(shortkey, [])
On 2010/11/02 00:45:46, truty wrote:
> Do you think the Python interpreter is smart enough to hoist the calculation
of
> shortkey out of the loop? This stands out to me.

The calculation can't be hoisted out of the loop here; shortkey has a
different value every time through the loop.

[jrbarnette, 2010-11-02 21:42:31]

On 2010/11/02 00:45:46, truty wrote:
[ ... ]
> http://codereview.chromium.org/4148011/diff/14001/12007#newcode70
> bootperf-bin/bootperf:70: <ip-address>.  By default, the test is run once; if
> <count> is
> I think you should indicate that one run is 10 iterations here.
> 
Good point.  I'll update the text.

> http://codereview.chromium.org/4148011/diff/14001/12010#newcode53
> bootperf-bin/showbootdata:53: "-d", "--diskstats", action="store_false",
> It's nitty, but since this is used so closely with bootperf I think it's
> confusing that you use -d for directory in bootperf and positional directory
> here (with -d meaning something completely different). I would unify the
> interface to the two tools for new users.
> 
This thought had crossed my mind as well.  My biggest problem was that I
don't have a convenient alternative letter to use:  The -d option for either
'bootperf' or 'showbootdata' would have to change, but I don't know what
other letter to use for either case...

Complete unification of the command line isn't useful:  'bootperf' can't
reasonably operate on multiple results directories, and 'showbootdata'
shouldn't require multiple -d options for the (very) common case of
comparing multiple data sets.

> http://codereview.chromium.org/4148011/diff/14001/12010#newcode54
> bootperf-bin/showbootdata:54: dest="use_timestats",
> So, what happens if I specify -d and -t.  Should the UI check for this instead
> of just defaulting to time stats?
> 
Actually, the default is the last option used.  However, I reckon the behavior
could be more user friendly.  I'll see what I can do.



> http://codereview.chromium.org/4148011/diff/14001/12010#newcode79
> bootperf-bin/showbootdata:79: optparser.add_option_group(optgroup)
> Same comment as before with regard to handling both -a and -r.

Whatever I come up with for -t/-d, I'll apply to -a/-r as well.

[jrbarnette, 2010-11-03 18:42:01]

PTAL.

I've updated the scripts.  Highlights of the changes since patch 4:
  * Changed bootstat --results_dir/-d option to --output_dir/-o
  * Fixed a bug in bootstat --help
  * Updated bootstat help output to reflect that platform_BootPerf
    reboots the target 10 times for each user specified count
  * Added checks to showbootdata to enforce that -a/-r and -t/-d
    are mutually exclusive, and updated the help text to reflect this.

[jrbarnette, 2010-11-05 20:02:42]

On 2010/11/03 18:42:01, jrbarnette wrote:
> PTAL.
> 
> I've updated the scripts.  Highlights of the changes since patch 4:
>   * Changed bootstat --results_dir/-d option to --output_dir/-o
>   * Fixed a bug in bootstat --help
>   * Updated bootstat help output to reflect that platform_BootPerf
>     reboots the target 10 times for each user specified count
>   * Added checks to showbootdata to enforce that -a/-r and -t/-d
>     are mutually exclusive, and updated the help text to reflect this.

Two bug fixes since the update above:
  * Fixed the output from --rawdata to properly sort the data points
    in the order in which they were produced.
  * Fixed a rounding error that could cause some time statistics to be
    off by 1 ms.

[truty1, 2010-11-05 20:46:50]

I can't see how the new rounding fix works.  Otherwise, the rest are nits.  This
would not pass Google Python style guidelines (you should consider running Lint
sometime).

http://codereview.chromium.org/4148011/diff/35001/36003
File bootperf-bin/perfprinter.py (right):

http://codereview.chromium.org/4148011/diff/35001/36003#newcode56
bootperf-bin/perfprinter.py:56: for run in dirlist:
This will sort 100 after 1 and before 2 unless you supply a key to extract and
convert to int, but that's just a nit.

http://codereview.chromium.org/4148011/diff/35001/36004
File bootperf-bin/resultset.py (right):

http://codereview.chromium.org/4148011/diff/35001/36004#newcode233
bootperf-bin/resultset.py:233: return int(self.TIME_SCALE * float(value) + 0.5)
I may be confused here. Can you please supply an example for why you round up
like this.

http://codereview.chromium.org/4148011/diff/35001/36005
File bootperf-bin/showbootdata (right):

http://codereview.chromium.org/4148011/diff/35001/36005#newcode62
bootperf-bin/showbootdata:62: optparser.set_defaults(use_timestats=False)
Just curious why you don't use add_option's default= parameter? I think it's
cleaner than this.

[jrbarnette, 2010-11-05 21:06:57]

http://codereview.chromium.org/4148011/diff/35001/36003
File bootperf-bin/perfprinter.py (right):

http://codereview.chromium.org/4148011/diff/35001/36003#newcode56
bootperf-bin/perfprinter.py:56: for run in dirlist:
On 2010/11/05 20:46:50, truty1 wrote:
> This will sort 100 after 1 and before 2 unless you supply a key to extract and
> convert to int, but that's just a nit.

This code relies on bootperf's implementation, which names the 'run'
directories run.000, run.001, etc.  'bootperf' does this to guarantee that
lexical sort will work, including the lexical sort done here.

http://codereview.chromium.org/4148011/diff/35001/36004
File bootperf-bin/resultset.py (right):

http://codereview.chromium.org/4148011/diff/35001/36004#newcode233
bootperf-bin/resultset.py:233: return int(self.TIME_SCALE * float(value) + 0.5)
On 2010/11/05 20:46:50, truty1 wrote:
> I may be confused here. Can you please supply an example for why you round up
> like this.

int() truncates FP numbers.  In some cases a number like "2.01" comes out
as 2009.9999... after scaling.  Thus, truncation produces a values that's too
small by 1 ms.  The formula int(x + 0.5) produces x rounded to the nearest
integer (at least when x is positive...).

http://codereview.chromium.org/4148011/diff/35001/36005
File bootperf-bin/showbootdata (right):

http://codereview.chromium.org/4148011/diff/35001/36005#newcode62
bootperf-bin/showbootdata:62: optparser.set_defaults(use_timestats=False)
On 2010/11/05 20:46:50, truty1 wrote:
> Just curious why you don't use add_option's default= parameter? I think it's
> cleaner than this.

The documentation for optparse discourages using add_option(default=...):
   http://docs.python.org/release/2.5.2/lib/optparse-default-values.html

[jrbarnette, 2010-11-05 21:09:08]

On Nov 5, 2010, at 1:46 PM, truty@google.com wrote:

> I can't see how the new rounding fix works.  Otherwise, the rest are  
> nits.  This
> would not pass Google Python style guidelines (you should consider  
> running Lint
> sometime).
>
I want to pass the style guidelines:  can you tell me where to find
"Lint"?  Also, let me know of the specific problems you see.

Thanks!

>
> http://codereview.chromium.org/4148011/diff/35001/36003
> File bootperf-bin/perfprinter.py (right):
>
> http://codereview.chromium.org/4148011/diff/35001/36003#newcode56
> bootperf-bin/perfprinter.py:56: for run in dirlist:
> This will sort 100 after 1 and before 2 unless you supply a key to
> extract and convert to int, but that's just a nit.
>
> http://codereview.chromium.org/4148011/diff/35001/36004
> File bootperf-bin/resultset.py (right):
>
> http://codereview.chromium.org/4148011/diff/35001/36004#newcode233
> bootperf-bin/resultset.py:233: return int(self.TIME_SCALE *  
> float(value)
> + 0.5)
> I may be confused here. Can you please supply an example for why you
> round up like this.
>
> http://codereview.chromium.org/4148011/diff/35001/36005
> File bootperf-bin/showbootdata (right):
>
> http://codereview.chromium.org/4148011/diff/35001/36005#newcode62
> bootperf-bin/showbootdata:62:
> optparser.set_defaults(use_timestats=False)
> Just curious why you don't use add_option's default= parameter? I  
> think
> it's cleaner than this.
>
> http://codereview.chromium.org/4148011/show

-- jrb

[truty1, 2010-11-05 21:24:43]

ok, lgtm
I use: /home/build/nonconf/devtools/gpylint/gpylint.par