Re: [PATCH 2/3] scripts/performance: Add callgrind_top_25.py script

[Ahmed Karaman]

Thanks Mr. Alex for your suggestions. I will send a v2 of this series
with the updates.

On Wed, Jun 17, 2020 at 2:16 PM Alex Bennée <alex.bennee@linaro.org> wrote:

> You will want the script to be +x if the user is to execute it.

Thanks for the reminder. Forgot to do this before sending the patch.

> > +#  Print the top 25 most executed functions in QEMU using callgrind.
> > +#  Example Usage:
> > +#  callgrind_top_25.py <qemu-build>/x86_64-linux-user/qemu-x86_64
> > executable
>
> Why limit to 25, make the name generic and maybe just default to 25
> unless the user specifies a different option.

Very valid suggestion. Thanks!

>
> I would recommend using:
>
>   from argparse import ArgumentParser
>
> from the start as adding options with hand parsing will be a pain. I
> would suggest a specific option for the qemu binary and then using a
> positional argument that can be read after -- so you don't confuse
> options.
>

Great, what do you think of the format below:
topN_callgrind.py -n20 -- /path/to/qemu executable -executable - arguments

> Direct os.system calls are discouraged, you tend to get weird effects
> like:
>
>   ../../scripts/performance/callgrind_top_25.py 
> ./aarch64-linux-user/qemu-aarch64 ./tests/tcg/aarch64-linux-user/fcvt
>   sh: 1: Syntax error: "&" unexpected
>   Traceback (most recent call last):
>     File "../../scripts/performance/callgrind_top_25.py", line 52, in <module>
>       with open('tmp.callgrind.data', 'r') as data:
>   FileNotFoundError: [Errno 2] No such file or directory: 'tmp.callgrind.data'
>
> I would:
>
>   - check for valgrind in path and fail gracefully if not found
>   - use os.subprocess API for launching (with or without the shell)
>

This weird error was because of the space between "&&" and "2>/dev/null"
These were inserted by the autopep8 python formatter before committing.
When this is fixed, everything works fine, but I believe your
suggestion of using the os.subprocess is valid so I will implement it
and also check for valgrind as you've said.

> > +
> > +# Line number with the total number of instructions
> > +number_of_instructions_line = 20
> > +
> > +# Line number with the top function
> > +first_func_line = 25
>
> for example
>
>     parser.add_argument('-n', dest="top", type=int, default=25,
>                         help="Hottest n functions")

Will also use:
    parser.add_argument('command',  type=str, nargs='+',
                help="QEMU invocation to report the top functions for")
To parse all remaining arguments after "--".

> > +# Get the total number of instructions
> > +total_number_of_instructions = int(
> > +    callgrind_data[number_of_instructions_line].split('
> > ')[0].replace(',', ''))
>
> There is no harm in having your steps split out a little.

Noted!

> > +# Remove intermediate files
> > +os.system('rm callgrind.data tmp.callgrind.data')
>
> os.unlink()
>

Noted!