Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pfm parsing for perf stat codes per instruction #33

Merged
merged 5 commits into from
Dec 8, 2024
Merged

pfm parsing for perf stat codes per instruction #33

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9f7f103
pfm parsing for perf stat codes per instruction
PhilipDeegan Nov 9, 2024
e5a03fe
nvidia profiling
PhilipDeegan Dec 3, 2024
2176713
...
PhilipDeegan Dec 4, 2024
aa3b325
++
PhilipDeegan Dec 8, 2024
940c8ce
++
PhilipDeegan Dec 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,6 @@ bin
__pycache__
dist
phlop.egg-info/
scope_timer.txt

*scope_timer.txt
tpp
24 changes: 24 additions & 0 deletions mkn.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,28 @@ profile:
- name: base
inc: inc

- name: pfm
inc: tpp/pfm/include
sub: pfm&tpp/pfm(git://perfmon2.git.sourceforge.net/gitroot/perfmon2/libpfm4)
PhilipDeegan marked this conversation as resolved.
Show resolved Hide resolved

- name: pfm_lib
parent: pfm
mode: static
inc: |
tpp/pfm/include
tpp/pfm/lib, 0
src: tpp/pfm/lib

- name: pfm_events
self: pfm_lib
main: tpp/pfm/examples/check_events.c
out: check_events

- name: pfm_info
self: pfm_lib
main: tpp/pfm/examples/showevtinfo.c
out: showevtinfo

PhilipDeegan marked this conversation as resolved.
Show resolved Hide resolved
- name: scope_timer
parent: base
src: src/phlop/timing/scope_timer.cpp
Expand All @@ -18,3 +40,5 @@ profile:
src: src/phlop/timing/threaded_scope_timer.cpp
mode: shared
test: tests/timing/test_threaded_scope_timer.cpp


3 changes: 3 additions & 0 deletions phlop/app/perf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ def cli_args_parser(description="Perf tool"):
logging="0=off, 1=on non zero exit code, 2=always",
outfile="path for saved file if active",
tool="stat/record/etc",
extra="forward string to perf command",
)

parser = argparse.ArgumentParser(
Expand All @@ -133,6 +134,8 @@ def cli_args_parser(description="Perf tool"):
parser.add_argument("-o", "--outfile", default=None, help=_help.outfile)
parser.add_argument("-t", "--tool", default="stat", help=_help.tool)
parser.add_argument("--logging", type=int, default=1, help=_help.logging)
parser.add_argument("-e", "--extra", type=str, default="", help=_help.extra)

Comment on lines +134 to +135
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

⚠️ Potential issue

Extra argument needs input validation but shell injection is properly handled

The review comment's security concerns are partially valid, but the shell command execution is actually safe:

  1. The extra parameter is used in perf command construction through string formatting (as seen in stat_cmd and record_cmd functions)
  2. Command execution is handled safely through phlop/proc.py which uses subprocess.run with proper parameter handling
  3. The suggestion to move the argument up for better organization is valid

However, input validation is still recommended to ensure only valid perf options are passed. Here's a revised suggestion:

     parser.add_argument("-o", "--outfile", default=None, help=_help.outfile)
     parser.add_argument("-t", "--tool", default="stat", help=_help.tool)
+    parser.add_argument("-e", "--extra", type=str, default="",
+                        help=_help.extra,
+                        # Validate perf options
+                        choices=["", "-A", "-g", "--per-thread", "--per-core"])
     parser.add_argument("--logging", type=int, default=1, help=_help.logging)
-
-    parser.add_argument("-e", "--extra", type=str, default="", help=_help.extra)
🔗 Analysis chain

Security and validation concerns with the --extra argument.

Forwarding raw strings to shell commands without validation could pose security risks. Additionally, consider:

  1. Moving this argument up with the other related arguments for better code organization
  2. Adding input validation to prevent command injection

Let's check if the extra options are used in shell commands:

Consider applying this diff for better organization and basic validation:

     parser.add_argument("-o", "--outfile", default=None, help=_help.outfile)
     parser.add_argument("-t", "--tool", default="stat", help=_help.tool)
+    parser.add_argument("-e", "--extra", type=str, default="",
+                        help=_help.extra,
+                        # Basic validation to prevent command injection
+                        choices=["-A", "-g", "--per-thread"])
     parser.add_argument("--logging", type=int, default=1, help=_help.logging)

-    parser.add_argument("-e", "--extra", type=str, default="", help=_help.extra)
-
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for shell=True usage with the extra parameter
rg -A 5 "shell=True.*extra"

Length of output: 29

Script:

#!/bin/bash
# Let's search more broadly for usage of the 'extra' parameter
rg -A 5 '\bextra\b' phlop/app/perf.py

# Also check for subprocess calls that might use the extra parameter
ast-grep --pattern 'subprocess.run($$$)'

Length of output: 490

Script:

#!/bin/bash
# Let's check how the extra parameter is used in command construction
rg -B 10 -A 10 'cmd.*=.*extra' phlop/app/perf.py

# Also check the verify_cli_args function to see if there's any validation
ast-grep --pattern 'def verify_cli_args($_) {
  $$$
}'

Length of output: 107

Script:

#!/bin/bash
# Let's check the entire perf.py file to understand the command construction
rg -B 5 -A 5 'subprocess|cmd.*=' phlop/app/perf.py

# Also check for any imports of subprocess or command execution related modules
rg '^from.*import|^import.*' phlop/app/perf.py

Length of output: 998

Script:

#!/bin/bash
# Let's check how the extra parameter flows through the code
rg -B 5 -A 10 'def main' phlop/app/perf.py

# Also check the proc.py module since it handles command execution
rg -l 'run.*=' phlop/proc.py && cat phlop/proc.py

Length of output: 2030

return parser


Expand Down
5 changes: 5 additions & 0 deletions phlop/app/pfm/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
#
#
#
#
#
48 changes: 48 additions & 0 deletions phlop/app/pfm/check_events.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
#
#
#
#
#


import logging
from pathlib import Path

from phlop.os import pushd
from phlop.proc import run
from phlop.string import decode_bytes

FILE_DIR = Path(__file__).resolve().parent

logger = logging.getLogger(__name__)
check_events_start = "Total events:"


def parse_check_events_output(lines):
return lines[-1].split(":")[1].strip().replace("0x", "r")

Comment on lines +21 to +23
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add type hints and docstring.

The function lacks proper documentation and type hints. It also assumes the input will always have at least one line and the expected format.

-def parse_check_events_output(lines):
+def parse_check_events_output(lines: list[str]) -> str:
+    """Parse the output from check_events command.
+
+    Args:
+        lines: List of output lines from check_events command
+
+    Returns:
+        Parsed event code in perf stat compatible format
+
+    Raises:
+        IndexError: If lines list is empty
+        ValueError: If output format is unexpected
+    """
+    if not lines:
+        raise ValueError("Empty output from check_events")
+    try:
         return lines[-1].split(":")[1].strip().replace("0x", "r")
+    except (IndexError, KeyError) as e:
+        raise ValueError(f"Unexpected output format: {lines[-1]}") from e
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def parse_check_events_output(lines):
return lines[-1].split(":")[1].strip().replace("0x", "r")
def parse_check_events_output(lines: list[str]) -> str:
"""Parse the output from check_events command.
Args:
lines: List of output lines from check_events command
Returns:
Parsed event code in perf stat compatible format
Raises:
IndexError: If lines list is empty
ValueError: If output format is unexpected
"""
if not lines:
raise ValueError("Empty output from check_events")
try:
return lines[-1].split(":")[1].strip().replace("0x", "r")
except (IndexError, KeyError) as e:
raise ValueError(f"Unexpected output format: {lines[-1]}") from e


def run_check_events(code):
with pushd(FILE_DIR.parent.parent.parent):
return decode_bytes(
run(f"./tpp/pfm/examples/check_events {code}").stdout
).splitlines()
Comment on lines +25 to +29
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Improve path handling and add error handling.

The function uses a hardcoded path and lacks proper error handling for command execution failures.

-def run_check_events(code):
+def run_check_events(code: str) -> list[str]:
+    """Run check_events command with the given code.
+
+    Args:
+        code: Event code to check
+
+    Returns:
+        List of output lines from the command
+
+    Raises:
+        FileNotFoundError: If check_events executable is not found
+        subprocess.CalledProcessError: If command execution fails
+    """
+    check_events_path = FILE_DIR.parent.parent.parent / "tpp" / "pfm" / "examples" / "check_events"
+    if not check_events_path.exists():
+        raise FileNotFoundError(f"check_events not found at {check_events_path}")
+
     with pushd(FILE_DIR.parent.parent.parent):
         return decode_bytes(
-            run(f"./tpp/pfm/examples/check_events {code}").stdout
+            run(f"{check_events_path} {code}", check=True).stdout
         ).splitlines()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def run_check_events(code):
with pushd(FILE_DIR.parent.parent.parent):
return decode_bytes(
run(f"./tpp/pfm/examples/check_events {code}").stdout
).splitlines()
def run_check_events(code: str) -> list[str]:
"""Run check_events command with the given code.
Args:
code: Event code to check
Returns:
List of output lines from the command
Raises:
FileNotFoundError: If check_events executable is not found
subprocess.CalledProcessError: If command execution fails
"""
check_events_path = FILE_DIR.parent.parent.parent / "tpp" / "pfm" / "examples" / "check_events"
if not check_events_path.exists():
raise FileNotFoundError(f"check_events not found at {check_events_path}")
with pushd(FILE_DIR.parent.parent.parent):
return decode_bytes(
run(f"{check_events_path} {code}", check=True).stdout
).splitlines()



def get_evt_perf_code(code):
return parse_check_events_output(run_check_events(code))
Comment on lines +32 to +33
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add docstring and improve error handling.

The function should have proper documentation and propagate errors from its dependencies.

-def get_evt_perf_code(code):
+def get_evt_perf_code(code: str) -> str:
+    """Get performance event code in perf stat compatible format.
+
+    Args:
+        code: Raw event code
+
+    Returns:
+        Event code formatted for perf stat
+
+    Raises:
+        ValueError: If event code is invalid or empty
+    """
+    if not code:
+        raise ValueError("Event code cannot be empty")
     return parse_check_events_output(run_check_events(code))

Committable suggestion skipped: line range outside the PR's diff.



if __name__ == "__main__":
from phlop.app.pfm.showevtinfo import get_evt_info

key, code = "[MULT_FLOPS]", ""
for info in get_evt_info():
if key in info.umask:
code = f"{info.name}:{info.umask[key].code}"
break

assert code != ""

# print("get_evt_perf_code", get_evt_perf_code(code))
print(run(f"perf stat -e {get_evt_perf_code(code)} sleep 5"))
81 changes: 81 additions & 0 deletions phlop/app/pfm/showevtinfo.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
#
#
#
#
#


import logging
from dataclasses import dataclass, field
from pathlib import Path

from phlop.os import pushd
from phlop.proc import run
from phlop.string import decode_bytes

FILE_DIR = Path(__file__).resolve().parent

logger = logging.getLogger(__name__)
EVTINFO_delimiter = "#-----------------------------"


@dataclass
class EVTUMask:
id: str
desc: str
code: str


@dataclass
class EVTInfo:
idx: str
pmu: str
name: str
umask: dict = field(default_factory=lambda: {})
ect: dict = field(default_factory=lambda: {})


def _parse_evtinfo(bits_list):
assert len(bits_list) >= 7
info = EVTInfo(
idx=bits_list[0][1].strip(),
pmu=bits_list[1][1].strip(),
name=bits_list[2][1].strip(),
)
for bits in bits_list[7:]:
if bits[0].strip().startswith("Umask"):
info.umask[bits[3].strip()] = EVTUMask(
id=bits[3].strip(), desc=bits[5].strip(), code=bits[1].strip()
)
return info

Comment on lines +57 to +70
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Improve error handling and documentation in _parse_evtinfo.

The function has several areas for improvement:

  1. The assert statement should include an error message
  2. The magic number 7 should be explained or made into a constant
  3. Missing error handling for malformed input
+MIN_BITS_LENGTH = 7  # Minimum required fields: idx, pmu, name, and 4 other required fields

 def _parse_evtinfo(bits_list):
+    """Parse event information from a list of bit fields.
+    
+    Args:
+        bits_list: List of strings containing event information fields
+    
+    Returns:
+        EVTInfo: Parsed event information
+        
+    Raises:
+        ValueError: If input format is invalid
+    """
-    assert len(bits_list) >= 7
+    if len(bits_list) < MIN_BITS_LENGTH:
+        raise ValueError(f"Expected at least {MIN_BITS_LENGTH} fields, got {len(bits_list)}")
+
+    try:
         info = EVTInfo(
             idx=bits_list[0][1].strip(),
             pmu=bits_list[1][1].strip(),
             name=bits_list[2][1].strip(),
         )
+    except IndexError as e:
+        raise ValueError("Invalid field format in input") from e

     for bits in bits_list[7:]:
-        if bits[0].strip().startswith("Umask"):
-            info.umask[bits[3].strip()] = EVTUMask(
-                id=bits[3].strip(), desc=bits[5].strip(), code=bits[1].strip()
-            )
+        try:
+            if bits[0].strip().startswith("Umask"):
+                info.umask[bits[3].strip()] = EVTUMask(
+                    id=bits[3].strip(),
+                    desc=bits[5].strip(),
+                    code=bits[1].strip()
+                )
+        except IndexError as e:
+            logger.warning(f"Skipping malformed umask entry: {bits}")
     return info
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def _parse_evtinfo(bits_list):
assert len(bits_list) >= 7
info = EVTInfo(
idx=bits_list[0][1].strip(),
pmu=bits_list[1][1].strip(),
name=bits_list[2][1].strip(),
)
for bits in bits_list[7:]:
if bits[0].strip().startswith("Umask"):
info.umask[bits[3].strip()] = EVTUMask(
id=bits[3].strip(), desc=bits[5].strip(), code=bits[1].strip()
)
return info
MIN_BITS_LENGTH = 7 # Minimum required fields: idx, pmu, name, and 4 other required fields
def _parse_evtinfo(bits_list):
"""Parse event information from a list of bit fields.
Args:
bits_list: List of strings containing event information fields
Returns:
EVTInfo: Parsed event information
Raises:
ValueError: If input format is invalid
"""
if len(bits_list) < MIN_BITS_LENGTH:
raise ValueError(f"Expected at least {MIN_BITS_LENGTH} fields, got {len(bits_list)}")
try:
info = EVTInfo(
idx=bits_list[0][1].strip(),
pmu=bits_list[1][1].strip(),
name=bits_list[2][1].strip(),
)
except IndexError as e:
raise ValueError("Invalid field format in input") from e
for bits in bits_list[7:]:
try:
if bits[0].strip().startswith("Umask"):
info.umask[bits[3].strip()] = EVTUMask(
id=bits[3].strip(),
desc=bits[5].strip(),
code=bits[1].strip()
)
except IndexError as e:
logger.warning(f"Skipping malformed umask entry: {bits}")
return info


def parse_evtinfo_output(lines):
start_idx = 0
for line in lines:
start_idx += 1
if line.strip() == EVTINFO_delimiter:
break

bits_list, results = [], []
for line in lines[start_idx:]:
if line == EVTINFO_delimiter:
results.append(_parse_evtinfo(bits_list))
bits_list = []
continue
bits_list.append(line.strip().split(":"))

return results

PhilipDeegan marked this conversation as resolved.
Show resolved Hide resolved

def run_evtinfo():
with pushd(FILE_DIR.parent.parent.parent):
return decode_bytes(run("./tpp/pfm/examples/showevtinfo").stdout).splitlines()

Comment on lines +90 to +93
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Security concerns identified with external binary execution

The code attempts to execute a binary ./tpp/pfm/examples/showevtinfo that:

  • Is not present in the repository
  • Has no associated build or installation instructions
  • Uses relative path traversal with parent.parent.parent
  • Has no path validation or existence checks
  • Has no documented source or security considerations

Recommendations:

  • Add proper validation for binary existence before execution
  • Document the binary's source and installation process
  • Consider using absolute paths or environment variables instead of path traversal
  • Add error handling for cases when the binary is missing or has incorrect permissions
🔗 Analysis chain

Verify security implications of external command execution.

The code executes an external binary. Please ensure:

  1. The binary path is properly sanitized
  2. The binary's permissions are correctly set
  3. The binary's source is trusted

Let me gather more information about the binary execution and path handling.

Let me check the actual implementation and context of the binary execution.

Let me check one final aspect about the binary and its source.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check the permissions and ownership of the showevtinfo binary
fd showevtinfo --type f --exec ls -l {}

Length of output: 123

Script:

#!/bin/bash
# Check for the actual binary and its location
fd -t f "tpp/pfm/examples/showevtinfo"

# Look for related build or installation files
rg -l "showevtinfo" --type make --type cmake

# Check how the binary is used in the codebase
rg -l "run\(.*showevtinfo" -g '!*.pyc'

# Check the path handling
ast-grep --pattern 'pushd($$$)'

Length of output: 559

Script:

#!/bin/bash
# Check the implementation details of the run function
ast-grep --pattern 'def run($$$)'

# Check if there's any validation or security measures
rg -A 5 "run\(" phlop/app/pfm/showevtinfo.py

# Check the FILE_DIR definition
rg "FILE_DIR =" phlop/app/pfm/showevtinfo.py

# Look for any setup or installation instructions
fd "README" --type f --exec cat {}

Length of output: 2125

Script:

#!/bin/bash
# Check if the binary is part of the repository or expected to be built
rg -l "showevtinfo" -g "setup.*" -g "*.toml" -g "*.cmake" -g "CMakeLists.txt"

# Check for any documentation about the binary
rg -l "showevtinfo" -g "*.md" -g "*.rst"

# Check the import and dependencies
rg "^from|^import" phlop/app/pfm/showevtinfo.py

Length of output: 339


def get_evt_info():
return parse_evtinfo_output(run_evtinfo())

Comment on lines +90 to +97
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Improve error handling and configuration in execution functions.

The functions need better error handling and should avoid hard-coded paths.

+SHOWEVTINFO_PATH = "tpp/pfm/examples/showevtinfo"

 def run_evtinfo():
+    """Execute the showevtinfo command and return its output.
+    
+    Returns:
+        list[str]: Command output lines
+        
+    Raises:
+        RuntimeError: If command execution fails
+    """
     with pushd(FILE_DIR.parent.parent.parent):
-        return decode_bytes(run("./tpp/pfm/examples/showevtinfo").stdout).splitlines()
+        try:
+            result = run(f"./{SHOWEVTINFO_PATH}", check=True)
+            return decode_bytes(result.stdout).splitlines()
+        except Exception as e:
+            raise RuntimeError(f"Failed to execute {SHOWEVTINFO_PATH}: {e}") from e

 def get_evt_info():
+    """Get parsed event information.
+    
+    Returns:
+        list[EVTInfo]: List of parsed event information
+        
+    Raises:
+        RuntimeError: If event information cannot be retrieved
+    """
     return parse_evtinfo_output(run_evtinfo())
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def run_evtinfo():
with pushd(FILE_DIR.parent.parent.parent):
return decode_bytes(run("./tpp/pfm/examples/showevtinfo").stdout).splitlines()
def get_evt_info():
return parse_evtinfo_output(run_evtinfo())
SHOWEVTINFO_PATH = "tpp/pfm/examples/showevtinfo"
def run_evtinfo():
"""Execute the showevtinfo command and return its output.
Returns:
list[str]: Command output lines
Raises:
RuntimeError: If command execution fails
"""
with pushd(FILE_DIR.parent.parent.parent):
try:
result = run(f"./{SHOWEVTINFO_PATH}", check=True)
return decode_bytes(result.stdout).splitlines()
except Exception as e:
raise RuntimeError(f"Failed to execute {SHOWEVTINFO_PATH}: {e}") from e
def get_evt_info():
"""Get parsed event information.
Returns:
list[EVTInfo]: List of parsed event information
Raises:
RuntimeError: If event information cannot be retrieved
"""
return parse_evtinfo_output(run_evtinfo())


if __name__ == "__main__":
print(get_evt_info())
1 change: 0 additions & 1 deletion phlop/app/stats_man.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
from phlop.dict import ValDict
from phlop.proc import run_raw

logging.basicConfig(level=logging.DEBUG)
logger = logging.getLogger(__name__)

_default_interval = 2
Expand Down
1 change: 0 additions & 1 deletion phlop/run/mpirun_perf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

from phlop.app import perf as p

logging.basicConfig(level=logging.DEBUG)
logger = logging.getLogger(__name__)

MPI_RANK = os.environ.get("OMPI_COMM_WORLD_RANK")
Expand Down
1 change: 0 additions & 1 deletion phlop/run/mpirun_stats_man.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

from phlop.app import stats_man as sman

logging.basicConfig(level=logging.DEBUG)
logger = logging.getLogger(__name__)

MPI_RANK = os.environ.get("OMPI_COMM_WORLD_RANK")
Expand Down
10 changes: 7 additions & 3 deletions phlop/run/perf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,11 @@
"""


def perf_stat_cmd(cli_args, path, line):
def perf_stat_cmd(cli_args, path, line, options):
file = Path(line.split(" ")[-1]).stem
outpath = logpath / path.stem
outpath.mkdir(parents=True, exist_ok=True)
return p.stat_cmd(line, p.stat_events, outpath / f"{file}.json")
return p.stat_cmd(line, p.stat_events, outpath / f"{file}.json", options)


def get_from_files(cli_args):
Expand All @@ -50,7 +50,11 @@ def get_remaining(cli_args):
test_batches = {}
path = Path(cli_args.remaining[-1]).parent
test_case = tc.determine_cores_for_test_case(
tc.TestCase(cmd=perf_stat_cmd(cli_args, path, " ".join(cli_args.remaining)))
tc.TestCase(
cmd=perf_stat_cmd(
cli_args, path, " ".join(cli_args.remaining), cli_args.extra
)
)
)
test_batches[test_case.cores] = [test_case]
return [tc.TestBatch(v, k) for k, v in test_batches.items()]
Expand Down
3 changes: 1 addition & 2 deletions phlop/testing/parallel_processor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,7 @@
logger = getLogger(__name__)


class TestCaseFailure(Exception):
...
class TestCaseFailure(Exception): ...


class LoggingMode(Enum):
Expand Down
11 changes: 11 additions & 0 deletions sh/setup_pfm.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
#!/usr/bin/env bash

CWD="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" && cd "$CWD"/..

set -ex

[ ! -d "tpp/pfm" ] && (
git clone git://perfmon2.git.sourceforge.net/gitroot/perfmon2/libpfm4 tpp/pfm
cd tpp/pfm
make
)
PhilipDeegan marked this conversation as resolved.
Show resolved Hide resolved
8 changes: 5 additions & 3 deletions sh/test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
#!/usr/bin/env bash

CWD="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
cd "$CWD"/..
CWD="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" && cd "$CWD"/..

set -ex

Expand All @@ -19,4 +18,7 @@ py -O tests/timing/test_scope_timer.py test_scope_timer -f scope_timer.txt
py -Om phlop.run.valgrind echo yes
py -Om phlop.run.valgrind --tool=massif echo yes

py -Om phlop.run.perf echo yes || echo "perf failed, assumed CI"
py -Om phlop.run.perf -e="--all-user" echo yes || echo "perf failed, assumed CI"

# install via ./sh/setup_pfm.sh
[ -d "tpp/pfm" ] && py -O tests/_phlop/app/pfm/test_pfm.py || echo "pfm missing, skipped"
27 changes: 27 additions & 0 deletions tests/_phlop/app/pfm/test_pfm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
#
#
#


from phlop.proc import run
from phlop.string import decode_bytes

if __name__ == "__main__":
from phlop.app.pfm.check_events import get_evt_perf_code
from phlop.app.pfm.showevtinfo import get_evt_info

code = ""
key0, key1 = "[MULT_FLOPS]", "[ADD_SUB_FLOPS]"
for info in get_evt_info():
if key0 in info.umask:
for key, umask in info.umask.items():
code += f"{info.name}:{umask.code} "
break
# if key1 in info.umask:
# code += f"{info.name}:{info.umask[key1].code} "

code = code.strip()
assert code != ""
Comment on lines +13 to +24
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Multiple improvements needed in the event code gathering logic.

  1. The hardcoded keys make the script inflexible. Consider making them configurable.
  2. Remove commented code if it's no longer needed.
  3. The loop variable key is unused as flagged by static analysis.
-    code = ""
-    key0, key1 = "[MULT_FLOPS]", "[ADD_SUB_FLOPS]"
+    # Define events to monitor - consider moving to a config file
+    EVENTS_TO_MONITOR = ["[MULT_FLOPS]"]
+    code = ""
     for info in get_evt_info():
-        if key0 in info.umask:
-            for key, umask in info.umask.items():
+        if any(event in info.umask for event in EVENTS_TO_MONITOR):
+            for _key, umask in info.umask.items():
                 code += f"{info.name}:{umask.code} "
             break
-        # if key1 in info.umask:
-        #     code += f"{info.name}:{info.umask[key1].code} "
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
code = ""
key0, key1 = "[MULT_FLOPS]", "[ADD_SUB_FLOPS]"
for info in get_evt_info():
if key0 in info.umask:
for key, umask in info.umask.items():
code += f"{info.name}:{umask.code} "
break
# if key1 in info.umask:
# code += f"{info.name}:{info.umask[key1].code} "
code = code.strip()
assert code != ""
# Define events to monitor - consider moving to a config file
EVENTS_TO_MONITOR = ["[MULT_FLOPS]"]
code = ""
for info in get_evt_info():
if any(event in info.umask for event in EVENTS_TO_MONITOR):
for _key, umask in info.umask.items():
code += f"{info.name}:{umask.code} "
break
code = code.strip()
assert code != ""
🧰 Tools
🪛 Ruff

17-17: Loop control variable key not used within loop body

Rename unused key to _key

(B007)


events = " ".join([f"-e {get_evt_perf_code(ev)}" for ev in code.split(" ")])
print(decode_bytes(run(f"perf stat {events} sleep 5").stderr))
Comment on lines +26 to +27
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve command execution robustness and configurability.

The command execution could be improved in several ways:

  1. Make the sleep duration configurable
  2. Add error handling for the command execution
  3. Consider parsing and validating the command output instead of directly printing it
+    # Configuration
+    SLEEP_DURATION = 5  # seconds
+
     events = " ".join([f"-e {get_evt_perf_code(ev)}" for ev in code.split(" ")])
-    print(decode_bytes(run(f"perf stat {events} sleep 5").stderr))
+    try:
+        result = run(f"perf stat {events} sleep {SLEEP_DURATION}")
+        output = decode_bytes(result.stderr)
+        # TODO: Add output parsing and validation
+        print(output)
+    except Exception as e:
+        print(f"Error executing perf stat command: {e}")
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
events = " ".join([f"-e {get_evt_perf_code(ev)}" for ev in code.split(" ")])
print(decode_bytes(run(f"perf stat {events} sleep 5").stderr))
# Configuration
SLEEP_DURATION = 5 # seconds
events = " ".join([f"-e {get_evt_perf_code(ev)}" for ev in code.split(" ")])
try:
result = run(f"perf stat {events} sleep {SLEEP_DURATION}")
output = decode_bytes(result.stderr)
# TODO: Add output parsing and validation
print(output)
except Exception as e:
print(f"Error executing perf stat command: {e}")

Loading