qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: John Snow <jsnow@redhat.com>
To: Ahmed Karaman <ahmedkhaledkaraman@gmail.com>,
	qemu-devel@nongnu.org, aleksandar.qemu.devel@gmail.com,
	philmd@redhat.com, alex.bennee@linaro.org, eblake@redhat.com,
	ldoktor@redhat.com, rth@twiddle.net, ehabkost@redhat.com,
	crosa@redhat.com
Subject: Re: [PATCH 1/9] scripts/performance: Refactor topN_perf.py
Date: Thu, 1 Oct 2020 16:41:38 -0400	[thread overview]
Message-ID: <ec6e7528-0281-9bdc-5afc-4b9c8a541f13@redhat.com> (raw)
In-Reply-To: <20200828104102.4490-2-ahmedkhaledkaraman@gmail.com>

I realize this review comes well after you are no longer being paid to 
work on this, so I am offering my time to help polish your patches if 
you would like.

On 8/28/20 6:40 AM, Ahmed Karaman wrote:
> - Apply pylint and flake8 formatting rules to the script.
> - Use 'tempfile' instead of '/tmp' for creating temporary files.
> 

I had meant to maybe consider using some helper functions so that you 
didn't need to rename the globals, for instance:

> Signed-off-by: Ahmed Karaman <ahmedkhaledkaraman@gmail.com>
> ---
>   scripts/performance/topN_perf.py | 174 +++++++++++++++----------------
>   1 file changed, 87 insertions(+), 87 deletions(-)
> 
> diff --git a/scripts/performance/topN_perf.py b/scripts/performance/topN_perf.py
> index 07be195fc8..56b100da87 100755
> --- a/scripts/performance/topN_perf.py
> +++ b/scripts/performance/topN_perf.py
> @@ -1,72 +1,77 @@
>   #!/usr/bin/env python3
>   
> -#  Print the top N most executed functions in QEMU using perf.
> -#  Syntax:
> -#  topN_perf.py [-h] [-n] <number of displayed top functions>  -- \
> -#           <qemu executable> [<qemu executable options>] \
> -#           <target executable> [<target execurable options>]
> -#
> -#  [-h] - Print the script arguments help message.
> -#  [-n] - Specify the number of top functions to print.
> -#       - If this flag is not specified, the tool defaults to 25.
> -#
> -#  Example of usage:
> -#  topN_perf.py -n 20 -- qemu-arm coulomb_double-arm
> -# > -#  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/>.
> +"""
> +Print the top N most executed functions in QEMU using perf.
> +
> +Syntax:
> +topN_perf.py [-h] [-n <number of displayed top functions>] -- \
> +         <qemu executable> [<qemu executable options>] \
> +         <target executable> [<target execurable options>]
> +
> +[-h] - Print the script arguments help message.
> +[-n] - Specify the number of top functions to print.
> +     - If this flag is not specified, the tool defaults to 25.
> +
> +Example of usage:
> +topN_perf.py -n 20 -- qemu-arm coulomb_double-arm
> +

Based on discussion we've had upstream since you sent this, I think we 
will be keeping license and authorship information out of the 
docstrings, so this part can stay a comment.

> +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
> +import tempfile
>   
>   
>   # Parse the command line arguments
> -parser = argparse.ArgumentParser(
> -    usage='topN_perf.py [-h] [-n] <number of displayed top functions >  -- '
> +PARSER = argparse.ArgumentParser(
> +    usage='topN_perf.py [-h] [-n <number of displayed top functions>] -- '
>             '<qemu executable> [<qemu executable options>] '
>             '<target executable> [<target executable options>]')
>   

This is a little odd; generally we can avoid having such globals by 
making a main() function that defines a parser as a local instead.

e.g.,

def main():
     parser = ...
     parser.add_argument(...)

     args = parser.parse_args()

     ...

     return 0


if __name__ == '__main__':
     sys.exit(main())


> -parser.add_argument('-n', dest='top', type=int, default=25,
> +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)
> +PARSER.add_argument('command', type=str, nargs='+', help=argparse.SUPPRESS)
>   
> -args = parser.parse_args()
> +ARGS = PARSER.parse_args()
>   
>   # Extract the needed variables from the args
> -command = args.command
> -top = args.top
> +COMMAND = ARGS.command
> +TOP = ARGS.top
>   
>   # Insure that perf is installed
> -check_perf_presence = subprocess.run(["which", "perf"],
> -                                     stdout=subprocess.DEVNULL)
> -if check_perf_presence.returncode:
> +CHECK_PERF_PRESENCE = subprocess.run(["which", "perf"],
> +                                     stdout=subprocess.DEVNULL,
> +                                     check=False)
> +if CHECK_PERF_PRESENCE.returncode:
>       sys.exit("Please install perf before running the script!")
>   
>   # Insure user has previllage to run perf
> -check_perf_executability = subprocess.run(["perf", "stat", "ls", "/"],
> +CHECK_PERF_EXECUTABILITY = subprocess.run(["perf", "stat", "ls", "/"],
>                                             stdout=subprocess.DEVNULL,
> -                                          stderr=subprocess.DEVNULL)
> -if check_perf_executability.returncode:
> -    sys.exit(
> -"""
> +                                          stderr=subprocess.DEVNULL,
> +                                          check=False)
> +if CHECK_PERF_EXECUTABILITY.returncode:
> +    sys.exit("""
>   Error:
>   You may not have permission to collect stats.
>   
> @@ -85,43 +90,42 @@ To make this setting permanent, edit /etc/sysctl.conf too, e.g.:
>      kernel.perf_event_paranoid = -1
>   
>   * Alternatively, you can run this script under sudo privileges.
> -"""
> -)
> -
> -# Run perf record
> -perf_record = subprocess.run((["perf", "record", "--output=/tmp/perf.data"] +
> -                              command),
> -                             stdout=subprocess.DEVNULL,
> -                             stderr=subprocess.PIPE)
> -if perf_record.returncode:
> -    os.unlink('/tmp/perf.data')
> -    sys.exit(perf_record.stderr.decode("utf-8"))
> -
> -# Save perf report output to /tmp/perf_report.out
> -with open("/tmp/perf_report.out", "w") as output:
> -    perf_report = subprocess.run(
> -        ["perf", "report", "--input=/tmp/perf.data", "--stdio"],
> -        stdout=output,
> -        stderr=subprocess.PIPE)
> -    if perf_report.returncode:
> -        os.unlink('/tmp/perf.data')
> -        output.close()
> -        os.unlink('/tmp/perf_report.out')
> -        sys.exit(perf_report.stderr.decode("utf-8"))
> -
> -# Read the reported data to functions[]
> -functions = []
> -with open("/tmp/perf_report.out", "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]
> +""")
> +
> +# Run perf and save all intermediate files in a temporary directory
> +with tempfile.TemporaryDirectory() as tmpdir:
> +    RECORD_PATH = os.path.join(tmpdir, "record.data")
> +    REPORT_PATH = os.path.join(tmpdir, "report.txt")
> +
> +    PERF_RECORD = subprocess.run((["perf", "record", "--output="+RECORD_PATH] +
> +                                  COMMAND),
> +                                 stdout=subprocess.DEVNULL,
> +                                 stderr=subprocess.PIPE,
> +                                 check=False)
> +    if PERF_RECORD.returncode:
> +        sys.exit(PERF_RECORD.stderr.decode("utf-8"))
> +
> +    with open(REPORT_PATH, "w") as output:
> +        PERF_REPORT = subprocess.run(
> +            ["perf", "report", "--input="+RECORD_PATH, "--stdio"],
> +            stdout=output,
> +            stderr=subprocess.PIPE,
> +            check=False)
> +        if PERF_REPORT.returncode:
> +            sys.exit(PERF_REPORT.stderr.decode("utf-8"))
> +
> +    # Save the reported data to FUNCTIONS[]
> +    with open(REPORT_PATH, "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"
> +NO_TOP_FUNCTIONS = TOP if len(FUNCTIONS) > TOP else len(FUNCTIONS)
> +
> +# Store the data of the top functions in TOP_FUNCTIONS[]
> +TOP_FUNCTIONS = FUNCTIONS[:NO_TOP_FUNCTIONS]
>   
>   # Print table header
>   print('{:>4}  {:>10}  {:<30}  {}\n{}  {}  {}  {}'.format('No.',
> @@ -134,7 +138,7 @@ print('{:>4}  {:>10}  {:<30}  {}\n{}  {}  {}  {}'.format('No.',
>                                                            '-' * 25))
>   
>   # Print top N functions
> -for (index, function) in enumerate(top_functions, start=1):
> +for (index, function) in enumerate(TOP_FUNCTIONS, start=1):
>       function_data = function.split()
>       function_percentage = function_data[0]
>       function_name = function_data[-1]
> @@ -143,7 +147,3 @@ for (index, function) in enumerate(top_functions, start=1):
>                                                function_percentage,
>                                                function_name,
>                                                function_invoker))
> -
> -# Remove intermediate files
> -os.unlink('/tmp/perf.data')
> -os.unlink('/tmp/perf_report.out')
> 



  parent reply	other threads:[~2020-10-01 20:43 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-28 10:40 [PATCH 0/9] GSoC 2020 - TCG Continuous Benchmarking scripts and tools Ahmed Karaman
2020-08-28 10:40 ` [PATCH 1/9] scripts/performance: Refactor topN_perf.py Ahmed Karaman
2020-09-07 20:52   ` Aleksandar Markovic
2020-09-18 20:33   ` Aleksandar Markovic
2020-09-19 11:17     ` Bottleneck problem to merge Python patches Philippe Mathieu-Daudé
2020-09-21 14:49       ` John Snow
2020-09-21 15:54       ` Eduardo Habkost
2020-09-21 17:57       ` Cleber Rosa
2020-10-01 20:41   ` John Snow [this message]
2020-10-01 21:59     ` [PATCH 1/9] scripts/performance: Refactor topN_perf.py John Snow
2020-08-28 10:40 ` [PATCH 2/9] scripts/performance: Refactor topN_callgrind.py Ahmed Karaman
2020-09-07 20:53   ` Aleksandar Markovic
2020-08-28 10:40 ` [PATCH 3/9] scripts/performance: Refactor dissect.py Ahmed Karaman
2020-09-02  8:48   ` Aleksandar Markovic
2020-08-28 10:40 ` [PATCH 4/9] scripts/performance: Add list_fn_callees.py script Ahmed Karaman
2020-08-28 10:40 ` [PATCH 5/9] scripts/performance: Add list_helpers.py script Ahmed Karaman
2020-08-28 10:40 ` [PATCH 6/9] scripts/performance: Add bisect.py script Ahmed Karaman
2020-08-28 10:41 ` [PATCH 7/9] tests/performance: Add nightly tests Ahmed Karaman
2020-09-02  8:36   ` Aleksandar Markovic
2020-09-02 13:26   ` Alex Bennée
2020-09-02 17:29     ` Ahmed Karaman
2020-09-15 16:39     ` Aleksandar Markovic
2020-09-16  8:31       ` Alex Bennée
2020-08-28 10:41 ` [PATCH 8/9] MAINTAINERS: Add 'tests/performance' to 'Performance Tools and Tests' subsection Ahmed Karaman
2020-09-02  8:37   ` Aleksandar Markovic
2020-08-28 10:41 ` [PATCH 9/9] scripts/performance: Add topN_system.py script Ahmed Karaman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ec6e7528-0281-9bdc-5afc-4b9c8a541f13@redhat.com \
    --to=jsnow@redhat.com \
    --cc=ahmedkhaledkaraman@gmail.com \
    --cc=aleksandar.qemu.devel@gmail.com \
    --cc=alex.bennee@linaro.org \
    --cc=crosa@redhat.com \
    --cc=eblake@redhat.com \
    --cc=ehabkost@redhat.com \
    --cc=ldoktor@redhat.com \
    --cc=philmd@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=rth@twiddle.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).