Re: [PATCH v2 1/3] scripts/performance: Add topN_perf.py script

[Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 8806 bytes --]

петак, 19. јун 2020., Ahmed Karaman <ahmedkhaledkaraman@gmail.com> је
написао/ла:

> Python script that prints the top N most executed functions in QEMU
> using perf.
>
> Example Usage:


Don't use capitalization when not appropriate. This is better:

Example of usage:


> topN_perf.py -n 20 -- /path/to/qemu program -program -flags
>
> If '-n' is not specified, the default is 25.
>
>
The command line above is hardly example of usage. Furthermore, it is
unclear of what you meant with "program -program". Other things are unclear
too (what about qemu options?) The command line, as is now, is somewhere
between a syntax description and an example of usage, but is neither a
syntax description nor example of usage.

From the script, it looks that there is "-h" too. Not mentioned in commit
message ar all, and it shouldbe.

I would suggest that you rework this section inthis way, for the sake of
clarity:

Syntax: (

topN_perf.py [-h] [-n <number of displayed top functions >  -- <qemu
executable> [<qemu executable options>] <target executable> [<target
execurable options>]

-h - .........explain here
-n - .........explain here

Example of usage:

topN_perf.py -n 20 -- qemu-arm coulomb_double-arm

Example of output:


..... and here you continue with:



 No.  Percentage  Name                       Caller
> ----  ----------  -------------------------  -------------------------
>    1      16.25%  float64_mul                qemu-x86_64
>    2      12.01%  float64_sub                qemu-x86_64
>    3      11.99%  float64_add                qemu-x86_64
>    4       5.69%  helper_mulsd               qemu-x86_64
>    5       4.68%  helper_addsd               qemu-x86_64
>    6       4.43%  helper_lookup_tb_ptr       qemu-x86_64
>    7       4.28%  helper_subsd               qemu-x86_64
>    8       2.71%  f64_compare                qemu-x86_64
>    9       2.71%  helper_ucomisd             qemu-x86_64
>   10       1.04%  helper_pand_xmm            qemu-x86_64
>   11       0.71%  float64_div                qemu-x86_64
>   12       0.63%  helper_pxor_xmm            qemu-x86_64
>   13       0.50%  0x00007f7b7004ef95         [JIT] tid 491
>   14       0.50%  0x00007f7b70044e83         [JIT] tid 491
>   15       0.36%  helper_por_xmm             qemu-x86_64
>   16       0.32%  helper_cc_compute_all      qemu-x86_64
>   17       0.30%  0x00007f7b700433f0         [JIT] tid 491
>   18       0.30%  float64_compare_quiet      qemu-x86_64
>   19       0.27%  soft_f64_addsub            qemu-x86_64
>   20       0.26%  round_to_int               qemu-x86_64
>
> Signed-off-by: Ahmed Karaman <ahmedkhaledkaraman@gmail.com>
> ---
>  scripts/performance/topN_perf.py | 115 +++++++++++++++++++++++++++++++
>  1 file changed, 115 insertions(+)
>  create mode 100755 scripts/performance/topN_perf.py
>
> diff --git a/scripts/performance/topN_perf.py b/scripts/performance/topN_
> perf.py
> new file mode 100755
> index 0000000000..53fa503d8a
> --- /dev/null
> +++ b/scripts/performance/topN_perf.py
> @@ -0,0 +1,115 @@
> +#!/usr/bin/env python3
> +
> +#  Print the top N most executed functions in QEMU using perf.
> +#  Example Usage:
> +#  topN_perf.py -n 20 -- /path/to/qemu program -program -flags
> +#


The similat comments from commit message section apply here too.


> +#   If '-n' is not specified, the default is 25.
> +#
> +#  This file is a part of the project "TCG Continuous Benchmarking".
> +#
> +#  Copyright (C) 2020  Ahmed Karaman <ahmedkhaledkaraman@gmail.com>
> +#  Copyright (C) 2020  Aleksandar Markovic <aleksandar.qemu.devel@gmail.
> com>
> +#
> +#  This program is free software: you can redistribute it and/or modify
> +#  it under the terms of the GNU General Public License as published by
> +#  the Free Software Foundation, either version 2 of the License, or
> +#  (at your option) any later version.
> +#
> +#  This program is distributed in the hope that it will be useful,
> +#  but WITHOUT ANY WARRANTY; without even the implied warranty of
> +#  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +#  GNU General Public License for more details.
> +#
> +#  You should have received a copy of the GNU General Public License
> +#  along with this program. If not, see <https://www.gnu.org/licenses/>.
> +
> +import argparse
> +import os
> +import subprocess
> +import sys
> +
> +
> +# Parse the command line arguments
> +parser = argparse.ArgumentParser(usage='topN_perf.py [-h] [-n
> TOP_FUNCTIONS] --'
> +                                ' /path/to/qemu program -[flags
> PROGRAM_FLAGS]')
> +


Same.

+parser.add_argument('-n', dest='top', type=int, default=25,
> +                    help='Specify the number of top functions to print.')
> +
> +parser.add_argument('command', type=str, nargs='+',
> help=argparse.SUPPRESS)
> +
> +args = parser.parse_args()
> +
> +# Extract the needed variables from the args
> +command = args.command
> +top = args.top
> +
> +# Insure that perf is installed
> +check_perf = subprocess.run(["which", "perf"], stdout=subprocess.DEVNULL)
> +if check_perf.returncode:
> +    sys.exit("Please install perf before running the script!")
> +


OK. This is good.


> +# Insure user has previllage to run perf
> +check_previlage = subprocess.run(["perf", "stat", "ls", "/"],
> +                              stdout=subprocess.DEVNULL,
> stderr=subprocess.PIPE)
> +if check_previlage.returncode:
> +    sys.exit(check_previlage.stderr.decode("utf-8") +
> +             "\nOr alternatively, you can run the script with sudo
> privileges!")


I would avoid mixing stderr message and your message practically in one
sentence. Better:

error: <text from stderr>
You must run the script with sudo pivilages, or, alternatively, set kernel
Xxx option to....


> +
> +# Run perf record
> +perf_record = subprocess.run((["perf", "record"] + command),
> +                             stdout=subprocess.DEVNULL,
> stderr=subprocess.PIPE)
> +if perf_record.returncode:
> +    os.unlink('perf.data')
> +    sys.exit(perf_record.stderr.decode("utf-8"))
> +
> +# Save perf report output to tmp.perf.data
> +with open("tmp.perf.data", "w") as output:
> +    perf_report = subprocess.run(
> +        ["perf", "report", "--stdio"], stdout=output,
> stderr=subprocess.PIPE)
> +    if perf_report.returncode:
> +        os.unlink('perf.data')
> +        output.close()
> +        os.unlink('tmp.perf.data')
> +        sys.exit(perf_report.stderr.decode("utf-8"))
> +


I am really confused by using both perf.data and tmp.perf.data names. Why?
They are in entirely different format. "tmp.perf.data should be
perf.top-function.list, let's say, if I am not mistaken about its meaning
and usage.

+# Read the reported data to functions[]
> +functions = []
> +with open("tmp.perf.data", "r") as data:
> +    # Only read lines that are not comments (comments start with #)
> +    # Only read lines that are not empty
> +    functions = [line for line in data.readlines() if line and line[0]
> +                 != '#' and line[0] != "\n"]
> +
> +# Limit the number of top functions to "top"
> +number_of_top_functions = top if len(functions) > top else len(functions)
> +
> +# Store the data of the top functions in top_functions[]
> +top_functions = functions[:number_of_top_functions]
> +
> +# Print information headers


# Print table header


> +print('{:>4}  {:>10}  {:<30}  {}\n{}  {}  {}  {}'.format('No.',
> +                                                         'Percentage',
> +                                                         'Name',
> +                                                         'Caller',
> +                                                         '-' * 4,
> +                                                         '-' * 10,
> +                                                         '-' * 30,
> +                                                         '-' * 25))
> +
> +
> +# Print top N functions
> +for (index, function) in enumerate(top_functions, start=1):
> +    function_data = function.split()
> +    function_percentage = function_data[0]
> +    function_name = function_data[-1]
> +    function_caller = ' '.join(function_data[2:-2])
> +    print('{:>4}  {:>10}  {:<30}  {}'.format(index,
> +                                             function_percentage,
> +                                             function_name,
> +                                             function_caller))
> +
> +# Remove intermediate files
> +os.unlink('perf.data')
> +os.unlink('tmp.perf.data')
> --


Thanks,
Aleksandar.


> 2.17.1
>
>

[-- Attachment #2: Type: text/html, Size: 12410 bytes --]