PytatoPyOpenCLArrayContext: add support for kernel profiling by matthiasdiener · Pull Request #311 · inducer/arraycontext

matthiasdiener · 2025-04-04T19:52:24Z

TODOs:

needs a test

Please squash

Copilot

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

arraycontext/impl/pytato/compile.py:643

Add tests that verify kernel profiling support works as expected when profile_kernels is enabled.

if self.actx.profile_kernels:

arraycontext/impl/pytato/init.py:299

Add tests for the new profiling methods (e.g., _wait_and_transfer_profile_events, get_profiling_data_for_kernel, reset_profiling_data_for_kernel, tabulate_profiling_data) to ensure that profiling data is collected and summarized correctly.

self.profile_kernels = profile_kernels

alexfikl

Got excited about the profiling so I skimmed a bit and left some comments, hope that's ok!

arraycontext/impl/pytato/__init__.py

alexfikl · 2025-04-04T20:18:41Z

arraycontext/impl/pytato/__init__.py

+        """Reset profiling data for kernel `kernel_name`."""
+        self.profile_results.pop(kernel_name, None)
+
+    def tabulate_profiling_data(self) -> pytools.Table:


This looks more like a helper function than a class method?

Dunno - this is currently the main way of "getting" the complete profiling data.

Isn't the main way of getting it actx.profile_results and/or actx.get_profile_for_kernel? This is just a handy way to display it (probably one of many).

Sure, it's just a convenience function for the user. We have the same method for eager profiling: https://github.com/illinois-ceesd/mirgecom/blob/41c6e567d44258557d95ef8453c3b82c4c89ab55/mirgecom/profiling.py#L216

I agree with @alexfikl that this should be a function, not a method.

What do you guys think of 1123c16?

Looks good to me!

arraycontext/impl/pytato/__init__.py

arraycontext/impl/pytato/compile.py

Co-authored-by: Alexandru Fikl <alexfikl@gmail.com>

arraycontext/impl/pytato/__init__.py

matthiasdiener · 2025-04-04T20:45:22Z

Thanks for taking a look @alexfikl ! This is ready for a first look @inducer .

inducer · 2025-04-07T18:09:43Z

arraycontext/impl/pytato/__init__.py

+        """Reset profiling data for kernel `kernel_name`."""
+        self.profile_results.pop(kernel_name, None)
+
+    def tabulate_profiling_data(self) -> pytools.Table:


I agree with @alexfikl that this should be a function, not a method.

arraycontext/impl/pytato/__init__.py

inducer · 2025-04-07T18:13:16Z

arraycontext/impl/pytato/__init__.py

+    """Holds a profile event that has not been collected by the profiler yet."""
+
+    cl_event: cl._cl.Event
+    translation_unit: Any


Do we want the translation_unit in here? I'm not sold.

Changed it to just the name of the translation unit in 7216ccf

inducer · 2025-04-07T18:14:04Z

arraycontext/impl/pytato/__init__.py

                allocator=self.allocator,
                **bound_arguments)
+
+        self.add_profiling_event(evt, pt_prg)


Only call the function if profiling is enabled, to avoid incurring the function call penalty. (Also below.)

Done in 7216ccf

inducer · 2025-04-07T18:15:13Z

arraycontext/impl/pytato/__init__.py

                allocator=self.allocator,
                **bound_arguments)
+
+        self.add_profiling_event(evt, pt_prg)


This is inaccurate: one loopy TranslationUnit will enqueue multiple kernels, so you'll need to either

collect multiple events, or

enqueue a marker before and after.

(Also below.)

I implemented the "marker" strategy in 7216ccf

arraycontext/impl/pytato/__init__.py

inducer · 2025-04-09T21:32:36Z

arraycontext/impl/pytato/__init__.py

+    def get_profiling_data_for_kernel(self, kernel_name: str) \
+          -> MultiCallKernelProfile:
+        """Return profiling data for kernel *kernel_name*."""
+        self._wait_and_transfer_profile_events()
+
+        time = 0
+        num_calls = 0
+
+        if kernel_name in self.profile_results:
+            knl_results = self.profile_results[kernel_name]
+
+            num_calls = len(knl_results)
+
+            for r in knl_results:
+                time += r
+
+        return MultiCallKernelProfile(num_calls, time)
+


What I'd like from the user interface here is just minimal access to the raw data and a reset primitive, without aggregation, and without exposing the before-wait/after-wait distinction. Everything else belongs in external utility routines, IMO.

I tried to minimize the API in 6bdb85b. What do you think?

arraycontext/impl/pytato/__init__.py

inducer · 2025-04-09T21:34:15Z

arraycontext/impl/pytato/utils.py

 # }}}

+
+def tabulate_profiling_data(profile_results: dict[str, MultiCallKernelProfile])\


Add to docs.

Added the (so far only) interface to the doc in 6bdb85b.

inducer · 2025-04-09T21:34:37Z

arraycontext/impl/pytato/__init__.py

+                time += r
+
+        return MultiCallKernelProfile(num_calls, time)
+


Make sure to add the final interface to the class documentation.

Added the (so far only) interface to the doc in 6bdb85b.

Co-authored-by: Andreas Klöckner <inform@tiker.net>

arraycontext/impl/pytato/__init__.py

inducer · 2025-04-16T22:00:27Z

Thx!

matthiasdiener self-assigned this Apr 4, 2025

matthiasdiener force-pushed the profile-pytato-actx branch from 5f2d45d to 1ce6dce Compare April 4, 2025 19:54

matthiasdiener added 3 commits April 4, 2025 15:01

Fix ruff 0.11.4 error

ac5f974

PytatoPyOpenCLArrayContext: add support for kernel profiling

676d4bc

Merge branch 'ruff-0.11.4' into profile-pytato-actx

2c2e431

matthiasdiener force-pushed the profile-pytato-actx branch from 1ce6dce to 2c2e431 Compare April 4, 2025 20:06

matthiasdiener requested a review from Copilot April 4, 2025 20:07

Copilot AI reviewed Apr 4, 2025

View reviewed changes

alexfikl reviewed Apr 4, 2025

View reviewed changes

matthiasdiener and others added 3 commits April 4, 2025 15:27

add a simple test

040f739

remove global pyopencl import

b0c00b4

replace argument indicator

8ade259

Co-authored-by: Alexandru Fikl <alexfikl@gmail.com>

matthiasdiener commented Apr 4, 2025

View reviewed changes

arraycontext/impl/pytato/__init__.py Outdated Show resolved Hide resolved

add add_profiling_event

55a0c2c

matthiasdiener requested a review from inducer April 4, 2025 20:57

Merge branch 'main' into profile-pytato-actx

afd88c2

matthiasdiener marked this pull request as ready for review April 7, 2025 17:45

inducer reviewed Apr 7, 2025

View reviewed changes

matthiasdiener marked this pull request as draft April 7, 2025 21:33

matthiasdiener added 2 commits April 7, 2025 16:39

outline multi-kernel per t_unit implementation

7216ccf

small fixes

55537cc

matthiasdiener force-pushed the profile-pytato-actx branch from 99cc47a to 55537cc Compare April 7, 2025 23:04

refactor tabulate_profiling_data

1123c16

matthiasdiener force-pushed the profile-pytato-actx branch from 7f24976 to 1123c16 Compare April 8, 2025 18:24

add test to disable

03dfa33

matthiasdiener force-pushed the profile-pytato-actx branch from 1fbf1ad to 03dfa33 Compare April 8, 2025 18:47

alexfikl reviewed Apr 8, 2025

View reviewed changes

arraycontext/impl/pytato/__init__.py Outdated Show resolved Hide resolved

matthiasdiener marked this pull request as ready for review April 8, 2025 19:42

matthiasdiener requested a review from inducer April 8, 2025 19:42

inducer reviewed Apr 9, 2025

View reviewed changes

rename to private fields/method

94d9d16

Co-authored-by: Andreas Klöckner <inform@tiker.net>

matthiasdiener force-pushed the profile-pytato-actx branch from 6a878e2 to 94d9d16 Compare April 9, 2025 22:26

refactor to simplify API

6bdb85b

matthiasdiener force-pushed the profile-pytato-actx branch from e986f53 to ea691a4 Compare April 10, 2025 00:07

matthiasdiener requested a review from inducer April 10, 2025 00:08

bit more doc

fedb836

matthiasdiener force-pushed the profile-pytato-actx branch from ea691a4 to fedb836 Compare April 10, 2025 00:10

factor out profile enable/disable

6ae30d6

inducer approved these changes Apr 16, 2025

View reviewed changes

arraycontext/impl/pytato/__init__.py Show resolved Hide resolved

inducer merged commit 1013ecb into main Apr 16, 2025
12 checks passed

inducer deleted the profile-pytato-actx branch April 16, 2025 22:00

		# }}}


		def tabulate_profiling_data(profile_results: dict[str, MultiCallKernelProfile])\

Comments

Conversation

matthiasdiener commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

alexfikl left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

matthiasdiener commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

inducer commented Apr 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

matthiasdiener commented Apr 4, 2025 •

edited

Loading

matthiasdiener commented Apr 4, 2025 •

edited

Loading