Re: [PATCH 1/1] scripts/performance: Add bisect.py script

[John Snow <jsnow@redhat.com>]

On 7/25/20 8:31 AM, Aleksandar Markovic wrote:
> 
> 
> On Wednesday, July 22, 2020, Ahmed Karaman <ahmedkhaledkaraman@gmail.com 
> <mailto:ahmedkhaledkaraman@gmail.com>> wrote:
> 
>     Python script that locates the commit that caused a performance
>     degradation or improvement in QEMU using the git bisect command
>     (binary search).
> 
>     Syntax:
>     bisect.py [-h] -s,--start START [-e,--end END] [-q,--qemu QEMU] \
>     --target TARGET --tool {perf,callgrind} -- \
>     <target executable> [<target executable options>]
> 
>     [-h] - Print the script arguments help message
>     -s,--start START - First commit hash in the search range
>     [-e,--end END] - Last commit hash in the search range
>                      (default: Latest commit)
>     [-q,--qemu QEMU] - QEMU path.
>                      (default: Path to a GitHub QEMU clone)
>     --target TARGET - QEMU target name
>     --tool {perf,callgrind} - Underlying tool used for measurements
> 
>     Example of usage:
>     bisect.py --start=fdd76fecdd --qemu=/path/to/qemu --target=ppc \
>     --tool=perf -- coulomb_double-ppc -n 1000
> 
>     Example output:
>     Start Commit Instructions:     12,710,790,060
>     End Commit Instructions:       13,031,083,512
>     Performance Change:            -2.458%
> 
>     Estimated Number of Steps:     10
> 
>     *****************BISECT STEP 1*****************
>     Instructions:        13,031,097,790
>     Status:              slow commit
>     *****************BISECT STEP 2*****************
>     Instructions:        12,710,805,265
>     Status:              fast commit
>     *****************BISECT STEP 3*****************
>     Instructions:        13,031,028,053
>     Status:              slow commit
>     *****************BISECT STEP 4*****************
>     Instructions:        12,711,763,211
>     Status:              fast commit
>     *****************BISECT STEP 5*****************
>     Instructions:        13,031,027,292
>     Status:              slow commit
>     *****************BISECT STEP 6*****************
>     Instructions:        12,711,748,738
>     Status:              fast commit
>     *****************BISECT STEP 7*****************
>     Instructions:        12,711,748,788
>     Status:              fast commit
>     *****************BISECT STEP 8*****************
>     Instructions:        13,031,100,493
>     Status:              slow commit
>     *****************BISECT STEP 9*****************
>     Instructions:        12,714,472,954
>     Status:              fast commit
>     ****************BISECT STEP 10*****************
>     Instructions:        12,715,409,153
>     Status:              fast commit
>     ****************BISECT STEP 11*****************
>     Instructions:        12,715,394,739
>     Status:              fast commit
> 
>     *****************BISECT RESULT*****************
>     commit 0673ecdf6cb2b1445a85283db8cbacb251c46516
>     Author: Richard Henderson <richard.henderson@linaro.org
>     <mailto:richard.henderson@linaro.org>>
>     Date:   Tue May 5 10:40:23 2020 -0700
> 
>          softfloat: Inline float64 compare specializations
> 
>          Replace the float64 compare specializations with inline functions
>          that call the standard float64_compare{,_quiet} functions.
>          Use bool as the return type.
>     ***********************************************
> 
>     Signed-off-by: Ahmed Karaman <ahmedkhaledkaraman@gmail.com
>     <mailto:ahmedkhaledkaraman@gmail.com>>
>     ---
>       scripts/performance/bisect.py | 374 ++++++++++++++++++++++++++++++++++
>       1 file changed, 374 insertions(+)
>       create mode 100755 scripts/performance/bisect.py
> 
>     diff --git a/scripts/performance/bisect.py
>     b/scripts/performance/bisect.py
>     new file mode 100755
>     index 0000000000..869cc69ef4
>     --- /dev/null
>     +++ b/scripts/performance/bisect.py
>     @@ -0,0 +1,374 @@
>     +#!/usr/bin/env python3
>     +
>     +#  Locate the commit that caused a performance degradation or
>     improvement in
>     +#  QEMU using the git bisect command (binary search).
>     +#
>     +#  Syntax:
>     +#  bisect.py [-h] -s,--start START [-e,--end END] [-q,--qemu QEMU] \
>     +#  --target TARGET --tool {perf,callgrind} -- \
>     +#  <target executable> [<target executable options>]
>     +#
>     +#  [-h] - Print the script arguments help message
>     +#  -s,--start START - First commit hash in the search range
>     +#  [-e,--end END] - Last commit hash in the search range
>     +#             (default: Latest commit)
>     +#  [-q,--qemu QEMU] - QEMU path.
>     +#              (default: Path to a GitHub QEMU clone)
>     +#  --target TARGET - QEMU target name
>     +#  --tool {perf,callgrind} - Underlying tool used for measurements
>     +
>     +#  Example of usage:
>     +#  bisect.py --start=fdd76fecdd --qemu=/path/to/qemu --target=ppc
>     --tool=perf \
>     +#  -- coulomb_double-ppc -n 1000
>     +#
>     +#  This file is a part of the project "TCG Continuous Benchmarking".
>     +#
>     +#  Copyright (C) 2020  Ahmed Karaman <ahmedkhaledkaraman@gmail.com
>     <mailto:ahmedkhaledkaraman@gmail.com>>
>     +#  Copyright (C) 2020  Aleksandar Markovic
>     <aleksandar.qemu.devel@gmail.com
>     <mailto:aleksandar.qemu.devel@gmail.com>>
>     +#
> 
> 
> Hi, Ahmed.
> 
> Yes, somewhat related to John's hints on these comments, it is customary 
> to have just a brief description before "Copyright" lines. This means 
> one sentence, or a short paragraph (3-4 sentences max). The lenghty 
> syntax commemt should be, in my opinion, moved after the license 
> preamble, just before the start of real Python code.
> 

I think it's fine in the module-level docstring. Those are sometimes 
pretty long and generally refer you to other functions/classes/etc of 
interest.

In this case, this is intended to be an executable script / entrypoint, 
so having the syntax up top isn't really such a cumbersome thing.

That said, it might be prone to rot up here. Moving it to a docstring 
for the main() function, near where the parser is actually constructed, 
might help preserve accuracy for longer, at the expense of burying it 
deeper into the file.

It's an extremely minor point, and I'm afraid of getting lost too deeply 
in bikeshedding for a GSoC submission. I will be happy to see 
pylint/flake8 pass. (In pylint's case: some minimum exceptions to turn 
off warnings for too many lines / too many variables is good.)

--js