[UR][Benchmarks] Support for Unitrace for all benchmarks #19320

mateuszpn · 2025-07-07T09:03:57Z

Run main.py script with --unitrace to get unitrace logs for every benchmark.
Use --unitrace-inclusive to get benchmark results and unitrace logs.

Signed-off-by: Mateusz P. Nowak <mateusz.p.nowak@intel.com>

Copilot

Pull Request Overview

Adds support for Unitrace tracing in all benchmarks by introducing new CLI flags, integrating Unitrace setup/teardown logic, and extending benchmark run methods.

Introduce --unitrace and --unitrace-inclusive flags and related options
Add unitrace.py utilities (prepare, cleanup, handle, prune)
Update run_bench and individual benchmarks to accept unitrace_timestamp and skip exclusions

Reviewed Changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
devops/scripts/benchmarks/utils/unitrace.py	New module for Unitrace log handling
devops/scripts/benchmarks/options.py	Added CLI options for Unitrace and `save_name`
devops/scripts/benchmarks/main.py	Integrated Unitrace flags into workflow and CLI
devops/scripts/benchmarks/html/data.js	Removed trailing whitespace
devops/scripts/benchmarks/history.py	Updated `save` signature to include `timestamp`
devops/scripts/benchmarks/benches/velocity.py	Extended `run` to forward `unitrace_timestamp`
devops/scripts/benchmarks/benches/umf.py	Extended `run` to forward `unitrace_timestamp`
devops/scripts/benchmarks/benches/test.py	Extended `run` to forward `unitrace_timestamp`
devops/scripts/benchmarks/benches/syclbench.py	Extended `run` to forward `unitrace_timestamp`
devops/scripts/benchmarks/benches/llamacpp.py	Extended `run` to forward `unitrace_timestamp`
devops/scripts/benchmarks/benches/gromacs.py	Extended `run` to forward `unitrace_timestamp`
devops/scripts/benchmarks/benches/compute.py	Added Unitrace exclusion list and timestamp logic
devops/scripts/benchmarks/benches/benchdnn_list.py	Defined `unitrace_exclusion_list` for oneDNN bench
devops/scripts/benchmarks/benches/benchdnn.py	Integrated Unitrace options for oneDNN bench
devops/scripts/benchmarks/benches/base.py	Updated `run_bench` signature to add Unitrace hooks

Comments suppressed due to low confidence (2)

devops/scripts/benchmarks/utils/unitrace.py:26

[nitpick] Consider adding unit tests for prune_unitrace_dirs (and other helper functions) to verify that old directories are removed correctly and edge cases are handled.

def prune_unitrace_dirs(base_dir, FILECNT=10):

devops/scripts/benchmarks/benches/compute.py:229

[nitpick] This exclusion list is duplicated in benchdnn_list.py; consider centralizing it in one module to avoid inconsistencies.

    unitrace_exclusion_list = [

Copilot · 2025-07-07T09:20:02Z

devops/scripts/benchmarks/main.py

 from utils.validate import Validate
 from utils.detect_versions import DetectVersion
 from presets import enabled_suites, presets
+from utils.oneapi import get_oneapi


The import get_oneapi is not used in this file; consider removing it to avoid unused imports.

Suggested change

from utils.oneapi import get_oneapi

Copilot · 2025-07-07T09:20:03Z

devops/scripts/benchmarks/utils/unitrace.py

+    bench_dir = f"{options.unitrace_res_dir}/{options.save_name}_{unitrace_timestamp}"
+    os.makedirs(bench_dir, exist_ok=True)
+
+    unitrace_output = f"{bench_dir}/{name}_{unitrace_timestamp}"


[nitpick] Constructing file paths with hardcoded / can break on some platforms; consider using os.path.join(options.unitrace_res_dir, f"{options.save_name}_{unitrace_timestamp}") for portability.

Suggested change

bench_dir = f"{options.unitrace_res_dir}/{options.save_name}_{unitrace_timestamp}"

os.makedirs(bench_dir, exist_ok=True)

unitrace_output = f"{bench_dir}/{name}_{unitrace_timestamp}"

bench_dir = os.path.join(options.unitrace_res_dir, f"{options.save_name}_{unitrace_timestamp}")

os.makedirs(bench_dir, exist_ok=True)

unitrace_output = os.path.join(bench_dir, f"{name}_{unitrace_timestamp}")

Copilot · 2025-07-07T09:20:03Z

devops/scripts/benchmarks/main.py

+    parser.add_argument(
+        "--unitrace",
+        action="store_true",
+        help="Unitrace tracing for sigle iteration of benchmarks",


Typo in help text: "sigle" should be "single".

Suggested change

help="Unitrace tracing for sigle iteration of benchmarks",

help="Unitrace tracing for single iteration of benchmarks",

Copilot · 2025-07-07T09:20:04Z

devops/scripts/benchmarks/benches/base.py

@@ -86,7 +87,14 @@ def get_adapter_full_path():
        ), f"could not find adapter file {adapter_path} (and in similar lib paths)"

    def run_bench(


[nitpick] Avoid mutable default arguments like extra_unitrace_opt=[]; use None and set to an empty list inside the function to prevent unintended sharing.

pbalcer · 2025-07-07T09:07:11Z

devops/scripts/benchmarks/utils/unitrace.py

+                "-DBUILD_WITH_XPTI=1",
+                "-DBUILD_WITH_MPI=0",
+            ],
+            ld_library=get_oneapi().ld_libraries() + [f"{options.sycl}/lib"],


pbalcer · 2025-07-07T09:08:11Z

devops/scripts/benchmarks/utils/unitrace.py

@@ -0,0 +1,182 @@
+# Copyright (C) 2024-2025 Intel Corporation


pbalcer · 2025-07-07T09:09:15Z

devops/scripts/benchmarks/utils/unitrace.py

+        print(f"Moved {pid_json_files[0]} to {dst}")
+
+    # Prune old unitrace directories
+    prune_unitrace_dirs(options.unitrace_res_dir, FILECNT=5)


You use this only here. Why bother with defining a default value of an argument that you are going to override anyway?

pbalcer · 2025-07-07T09:12:34Z

devops/scripts/benchmarks/utils/unitrace.py

+
+    shutil.move(os.path.join(options.benchmark_cwd, pid_json_files[0]), dst)
+    if options.verbose:
+        print(f"Moved {pid_json_files[0]} to {dst}")


all the logs will need to be changed after #19158 is merged.

pbalcer · 2025-07-07T09:35:44Z

devops/scripts/benchmarks/utils/unitrace.py

+    return bench_dir, unitrace_output, unitrace_command
+
+
+def handle_unitrace_output(bench_dir, unitrace_output, timestamp):


I feel like this is all very complicated. Maybe we can use directory structure to help us?

Let's say we have two benchmarks: "SubmitKernel" and "Hashtable". We can organize the unitrace output such that each trace file is in its own directory:

.../results/traces/SubmitKernel/YYYYMMDD_HHMMSS_[name].PID.out .../results/traces/Hashtable/YYYYMMDD_HHMMSS_[name].PID.out

Removing old results would become simply removing all but 5 newest files in a directory. We wouldn't have to find, rename and move files, since we could make unitrace output them to the destination place upfront.

pbalcer · 2025-07-07T09:37:30Z

devops/scripts/benchmarks/main.py

+    parser.add_argument(
+        "--unitrace",
+        action="store_true",
+        help="Unitrace tracing for sigle iteration of benchmarks",
+    )
+
+    parser.add_argument(
+        "--unitrace-inclusive",
+        action="store_true",
+        help="Regular run of benchmarks iterations and unitrace tracing in single additional run",
+    )


please do --unitrace inclusive and --unitrace as a single option, not two separate ones. See how --output-html is done.

pbalcer · 2025-07-07T09:43:25Z

devops/scripts/benchmarks/benches/compute.py

@@ -224,6 +224,71 @@ def parse_unit_type(compute_unit):


 class ComputeBenchmark(Benchmark):
+
+    # list of benchmarks to exclude from unitrace due to SIGSEGV, SIGABRT or timeouts
+    unitrace_exclusion_list = [


We should just let things fail. If unitrace is failing, it's an urgent bug that needs to be fixed.

Signed-off-by: Mateusz P. Nowak <mateusz.p.nowak@intel.com>

PatKamin · 2025-07-07T11:31:28Z

devops/scripts/benchmarks/utils/unitrace.py

+from utils.oneapi import get_oneapi
+
+
+def extract_save_name_and_timestamp(dirname):


This is used only in this module, please rename this method with an underscore prefix to make it clearer that it's for an internal use of this module's methods

PatKamin · 2025-07-07T11:31:34Z

devops/scripts/benchmarks/utils/unitrace.py

+    return None, None
+
+
+def prune_unitrace_dirs(base_dir, FILECNT=10):


This is used only in this module, please rename this method with an underscore prefix to make it clearer that it's for an internal use of this module's methods

PatKamin · 2025-07-07T11:35:25Z

devops/scripts/benchmarks/main.py


    # It's important we don't save the current results into history before
    # we calculate historical averages or get latest results for compare.
    # Otherwise we might be comparing the results to themselves.
    if not options.dry_run:
-        history.save(saved_name, results, save_name is not None)
+        history.save(saved_name, timestamp, results, options.save_name is not None)


Now history.save can use options.save_name directly and the last argument can be removed

PatKamin · 2025-07-07T11:39:37Z

devops/scripts/benchmarks/history.py

-            if options.timestamp_override is None
-            else options.timestamp_override
-        )
+        if timestamp is None:


It seems like the timestamp is unconditionally created in the main() method, so this one is not needed?

PatKamin · 2025-07-07T11:43:08Z

devops/scripts/benchmarks/benches/base.py

+                cwd=options.benchmark_cwd,
+                ld_library=ld_libraries,
+            )
+        except subprocess.CalledProcessError as e:


e is unused, can be omitted

PatKamin · 2025-07-07T11:54:12Z

devops/scripts/benchmarks/benches/base.py

-            cwd=options.benchmark_cwd,
-            ld_library=ld_libraries,
-        )
+        if unitrace_timestamp is not None:


It's unintuitive that passing a unitrace timestamp triggers unitracing. Please, consider making a unitrace class and passing a Unitrace object or None. This would also allow encapsulating unitrace download and setup inside Unitrace's constructor.

PatKamin · 2025-07-07T11:57:10Z

devops/scripts/benchmarks/benches/benchdnn.py

@@ -129,7 +129,7 @@ def setup(self):
        if not self.bench_bin.exists():
            raise FileNotFoundError(f"Benchmark binary not found: {self.bench_bin}")

-    def run(self, env_vars):
+    def run(self, env_vars, unitrace_timestamp: str = None) -> list[Result]:


nit: add a dict type as a type hint to env_vars

PatKamin · 2025-07-07T12:02:54Z

devops/scripts/benchmarks/utils/unitrace.py

+    prune_unitrace_dirs(options.unitrace_res_dir, FILECNT=5)
+
+
+def download_and_build_unitrace(workdir):


options.workdir can be used directly, no need to pass an argument

PatKamin · 2025-07-07T12:17:31Z

devops/scripts/benchmarks/utils/unitrace.py

+            add_sycl=True,
+        )
+        run(
+            ["cmake", "--build", build_dir, "-j"],


I think we should use options.build_jobs as much as we can. Also, there is a potential bug in this option's default value. os.process_cpu_count() shall be used as a default to respect the available cpu count for a given process, not multiprocessing.cpu_count() which returns ALL cpus in the system. This leads to builds using more parallel jobs than available cpus, ie. when limiting cpu count for a benchmark framework with taskset.

PatKamin · 2025-07-07T12:18:50Z

devops/scripts/benchmarks/utils/unitrace.py

+            ld_library=get_oneapi().ld_libraries() + [f"{options.sycl}/lib"],
+            add_sycl=True,
+        )
+    print("Unitrace built successfully.")


What if the build fails?

PatKamin · 2025-07-07T12:21:54Z

devops/scripts/benchmarks/utils/unitrace.py

+                    print(f"Renamed {src} to {dst}")
+                break
+
+    # Handle {name}.{pid}.json files in cwd: move and rename to {self.name()}_{timestamp}.json


Doesn't unitrace already have an option for changing the default name of jsons and .pid logs?

PatKamin · 2025-07-07T12:25:34Z

devops/scripts/benchmarks/utils/unitrace.py

+        raise FileNotFoundError(f"Unitrace binary not found: {unitrace_bin}. ")
+    os.makedirs(options.unitrace_res_dir, exist_ok=True)
+    if not options.save_name:
+        raise ValueError(


I think this is raised too late. It should be raised before we set up all the benchmarks and try to run them. We already know whether unitrace and save_name has been set right after the arguments parsing.

PatKamin · 2025-07-07T12:27:29Z

devops/scripts/benchmarks/utils/unitrace.py

+                shutil.rmtree(full_path)
+
+
+def unitrace_cleanup(bench_cwd, unitrace_output):


Please, use type hints in all the methods

pbalcer

One general comment - when under tracing, we should reduce the number of iterations etc, to shorten the duration of the benchmark.
If we really need to disable a benchmark (because it's difficult to make it sufficiently short for example), the Benchmark class should have something like traceable() to filter out which benchmarks should / shouldn't be traced.

mateuszpn added 4 commits June 30, 2025 11:04

Add Unitrace support

4d32fe5

Signed-off-by: Mateusz P. Nowak <mateusz.p.nowak@intel.com>

Merge branch 'intel:sycl' into unitrace

72d8108

Add Unitrace support

d653883

Signed-off-by: Mateusz P. Nowak <mateusz.p.nowak@intel.com>

Merge branch 'unitrace' of github.com:mateuszpn/llvm into unitrace

5834aa9

mateuszpn requested a review from a team as a code owner July 7, 2025 09:03

mateuszpn temporarily deployed to WindowsCILock July 7, 2025 09:04 — with GitHub Actions Inactive

mateuszpn added 2 commits July 7, 2025 09:12

Add list of Unitrace exclusions

d8f6b2b

Signed-off-by: Mateusz P. Nowak <mateusz.p.nowak@intel.com>

Merge remote-tracking branch 'upstream/sycl' into unitrace

01c1c1a

mateuszpn requested a review from Copilot July 7, 2025 09:17

Copilot AI reviewed Jul 7, 2025

View reviewed changes

mateuszpn temporarily deployed to WindowsCILock July 7, 2025 09:25 — with GitHub Actions Inactive

pbalcer reviewed Jul 7, 2025

View reviewed changes

Final fixes

be36447

Signed-off-by: Mateusz P. Nowak <mateusz.p.nowak@intel.com>

PatKamin reviewed Jul 7, 2025

View reviewed changes

pbalcer reviewed Jul 7, 2025

View reviewed changes

	help="Unitrace tracing for sigle iteration of benchmarks",
	help="Unitrace tracing for single iteration of benchmarks",

		@@ -86,7 +87,14 @@ def get_adapter_full_path():
		), f"could not find adapter file {adapter_path} (and in similar lib paths)"

		def run_bench(

		@@ -0,0 +1,182 @@
		# Copyright (C) 2024-2025 Intel Corporation

		return bench_dir, unitrace_output, unitrace_command


		def handle_unitrace_output(bench_dir, unitrace_output, timestamp):

		from utils.oneapi import get_oneapi


		def extract_save_name_and_timestamp(dirname):

		return None, None


		def prune_unitrace_dirs(base_dir, FILECNT=10):

		prune_unitrace_dirs(options.unitrace_res_dir, FILECNT=5)


		def download_and_build_unitrace(workdir):

		shutil.rmtree(full_path)


		def unitrace_cleanup(bench_cwd, unitrace_output):

[UR][Benchmarks] Support for Unitrace for all benchmarks #19320

Are you sure you want to change the base?

[UR][Benchmarks] Support for Unitrace for all benchmarks #19320

Conversation

mateuszpn commented Jul 7, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pbalcer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!