Re: [PATCH 3/9] scripts/performance: Refactor dissect.py

From: Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>
To: Ahmed Karaman <ahmedkhaledkaraman@gmail.com>
Cc: "ldoktor@redhat.com" <ldoktor@redhat.com>,
	"ehabkost@redhat.com" <ehabkost@redhat.com>,
	"jsnow@redhat.com" <jsnow@redhat.com>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
	"philmd@redhat.com" <philmd@redhat.com>,
	"crosa@redhat.com" <crosa@redhat.com>,
	"alex.bennee@linaro.org" <alex.bennee@linaro.org>,
	"rth@twiddle.net" <rth@twiddle.net>
Subject: Re: [PATCH 3/9] scripts/performance: Refactor dissect.py
Date: Wed, 2 Sep 2020 10:48:54 +0200	[thread overview]
Message-ID: <CAHiYmc6YHNGrdx4vE9ng5Js5ndtVjADXy5TbLqjZFsGO1SiKdQ@mail.gmail.com> (raw)
In-Reply-To: <20200828104102.4490-4-ahmedkhaledkaraman@gmail.com>

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

On Friday, August 28, 2020, Ahmed Karaman <ahmedkhaledkaraman@gmail.com>
wrote:

> - Apply pylint and flake8 formatting rules to the script.
> - Move syntax and usage exmaple to main() docstring.
> - Update get_jit_line() to only detect the main jit call.
> - Use mypy for specifying parameters and return types in functions.
>
>

Reviewed-by: Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>

> Signed-off-by: Ahmed Karaman <ahmedkhaledkaraman@gmail.com>
> ---
>  scripts/performance/dissect.py | 123 ++++++++++++++++++---------------
>  1 file changed, 68 insertions(+), 55 deletions(-)
>
> diff --git a/scripts/performance/dissect.py b/scripts/performance/dissect.
> py
> index bf24f50922..d4df884b75 100755
> --- a/scripts/performance/dissect.py
> +++ b/scripts/performance/dissect.py
> @@ -1,34 +1,27 @@
>  #!/usr/bin/env python3
>
> -#  Print the percentage of instructions spent in each phase of QEMU
> -#  execution.
> -#
> -#  Syntax:
> -#  dissect.py [-h] -- <qemu executable> [<qemu executable options>] \
> -#                   <target executable> [<target executable options>]
> -#
> -#  [-h] - Print the script arguments help message.
> -#
> -#  Example of usage:
> -#  dissect.py -- 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 percentage of instructions spent in each phase of QEMU
> +execution.
> +
> +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
> @@ -36,23 +29,26 @@ import subprocess
>  import sys
>  import tempfile
>
> +from typing import List
> +
>
> -def get_JIT_line(callgrind_data):
> +def get_jit_line(callgrind_data: List[str]) -> int:
>      """
>      Search for the first instance of the JIT call in
>      the callgrind_annotate output when ran using --tree=caller
>      This is equivalent to the self number of instructions of JIT.
>
>      Parameters:
> -    callgrind_data (list): callgrind_annotate output
> +    callgrind_data (List[str]): callgrind_annotate output
>
>      Returns:
>      (int): Line number
>      """
>      line = -1
> -    for i in range(len(callgrind_data)):
> -        if callgrind_data[i].strip('\n') and \
> -                callgrind_data[i].split()[-1] == "[???]":
> +    for (i, callgrind_datum) in enumerate(callgrind_data):
> +        if callgrind_datum.strip('\n') and \
> +                callgrind_datum.split()[-1] == "[???]" and \
> +                callgrind_datum.split()[1] == "*":
>              line = i
>              break
>      if line == -1:
> @@ -61,6 +57,18 @@ def get_JIT_line(callgrind_data):
>
>
>  def main():
> +    """
> +    Parse the command line arguments then start the execution.
> +    Syntax:
> +    dissect.py [-h] -- <qemu executable> [<qemu executable options>] \
> +                 <target executable> [<target executable options>]
> +
> +    [-h] - Print the script arguments help message.
> +
> +    Example of usage:
> +    dissect.py -- qemu-arm coulomb_double-arm
> +    """
> +
>      # Parse the command line arguments
>      parser = argparse.ArgumentParser(
>          usage='dissect.py [-h] -- '
> @@ -76,7 +84,7 @@ def main():
>
>      # Insure that valgrind is installed
>      check_valgrind = subprocess.run(
> -        ["which", "valgrind"], stdout=subprocess.DEVNULL)
> +        ["which", "valgrind"], stdout=subprocess.DEVNULL, check=False)
>      if check_valgrind.returncode:
>          sys.exit("Please install valgrind before running the script.")
>
> @@ -93,7 +101,8 @@ def main():
>                                       "--callgrind-out-file=" + data_path]
>                                      + command),
>                                     stdout=subprocess.DEVNULL,
> -                                   stderr=subprocess.PIPE)
> +                                   stderr=subprocess.PIPE,
> +                                   check=False)
>          if callgrind.returncode:
>              sys.exit(callgrind.stderr.decode("utf-8"))
>
> @@ -102,7 +111,8 @@ def main():
>              callgrind_annotate = subprocess.run(
>                  ["callgrind_annotate", data_path, "--tree=caller"],
>                  stdout=output,
> -                stderr=subprocess.PIPE)
> +                stderr=subprocess.PIPE,
> +                check=False)
>              if callgrind_annotate.returncode:
>                  sys.exit(callgrind_annotate.stderr.decode("utf-8"))
>
> @@ -120,25 +130,28 @@ def main():
>          total_instructions = int(total_instructions.replace(',', ''))
>
>          # Line number with the JIT self number of instructions
> -        JIT_self_instructions_line_number = get_JIT_line(callgrind_data)
> +        jit_self_instructions_line_number = get_jit_line(callgrind_data)
>          # Get the JIT self number of instructions
> -        JIT_self_instructions_line_data = \
> -            callgrind_data[JIT_self_instructions_line_number]
> -        JIT_self_instructions = JIT_self_instructions_line_
> data.split()[0]
> -        JIT_self_instructions = int(JIT_self_instructions.replace(',',
> ''))
> +        jit_self_instructions_line_data = \
> +            callgrind_data[jit_self_instructions_line_number]
> +        jit_self_instructions = jit_self_instructions_line_
> data.split()[0]
> +        jit_self_instructions = int(jit_self_instructions.replace(',',
> ''))
>
>          # Line number with the JIT self + inclusive number of instructions
> -        # It's the line above the first JIT call when running with
> --tree=caller
> -        JIT_total_instructions_line_number = JIT_self_instructions_line_
> number-1
> +        # It's the line above the first JIT call when running with
> +        # --tree=caller
> +        jit_total_instructions_line_number = \
> +            jit_self_instructions_line_number-1
>          # Get the JIT self + inclusive number of instructions
> -        JIT_total_instructions_line_data = \
> -            callgrind_data[JIT_total_instructions_line_number]
> -        JIT_total_instructions = JIT_total_instructions_line_
> data.split()[0]
> -        JIT_total_instructions = int(JIT_total_instructions.replace(',',
> ''))
> +        jit_total_instructions_line_data = \
> +            callgrind_data[jit_total_instructions_line_number]
> +        jit_total_instructions = jit_total_instructions_line_
> data.split()[0]
> +        jit_total_instructions = int(jit_total_instructions.replace(',',
> ''))
>
>          # Calculate number of instructions in helpers and code generation
> -        helpers_instructions = JIT_total_instructions-JIT_
> self_instructions
> -        code_generation_instructions = total_instructions-JIT_total_
> instructions
> +        helpers_instructions = jit_total_instructions-jit_
> self_instructions
> +        code_generation_instructions = \
> +            total_instructions-jit_total_instructions
>
>          # Print results (Insert commas in large numbers)
>          # Print total number of instructions
> @@ -149,12 +162,12 @@ def main():
>          print('{:<20}{:>20}\t{:>6.3f}%'.
>                format("Code Generation:",
>                       format(code_generation_instructions, ","),
> -                     (code_generation_instructions / total_instructions)
> * 100))
> -        # Print JIT instructions and percentage
> +                     (code_generation_instructions/
> total_instructions)*100))
> +        # Print jit instructions and percentage
>          print('{:<20}{:>20}\t{:>6.3f}%'.
> -              format("JIT Execution:",
> -                     format(JIT_self_instructions, ","),
> -                     (JIT_self_instructions / total_instructions) * 100))
> +              format("jit Execution:",
> +                     format(jit_self_instructions, ","),
> +                     (jit_self_instructions / total_instructions) * 100))
>          # Print helpers instructions and percentage
>          print('{:<20}{:>20}\t{:>6.3f}%'.
>                format("Helpers:",
> --
> 2.17.1
>
>

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