[Dynamic Selection] Adding sycl profiling for auto tune policy #1464

AnuyaWelling2801 · 2024-03-22T16:15:02Z

Changes made :

Adding an async_waiter_base to ensure type erasure for the selection type and enable lazy reporting
Adding a choice to use use_event_profiling = q.template has_propertysycl::property::queue::enable_profiling()
Using lazy report to report time instead of host task for auto_tune policy
Added more tests in auto_tune_policy_sycl for devices with event profiling enabled.
Changing the report method for auto_tune policy

vossmjp

You need to take a closer look at thread safety and also find a non-trait-based approach to checking for lazy_report.

vossmjp · 2024-03-22T21:01:46Z

include/oneapi/dpl/internal/dynamic_selection_impl/auto_tune_policy.h

@@ -229,6 +229,9 @@ class auto_tune_policy
            }
            else
            {
+                if constexpr(lazy_report_v<Backend>){
+                        backend_->lazy_report();


You are holding a lock on state_->m_ when calling the lazy_report function. Seems unnecessary and perhaps dangerous as a general pattern.

vossmjp · 2024-03-22T21:23:20Z

include/oneapi/dpl/internal/dynamic_selection_impl/sycl_backend.h

@@ -142,6 +195,15 @@ class sycl_backend
        return global_rank_;
    }

+    void lazy_report(){


This function is not thread safe. You might have concurrent accesses to the async_waiter array that change its size -- calls to lazy_report and submit (which calls add_waiter).

Fixed. Created a new structure async_wait_arr.

vossmjp · 2024-03-22T21:24:10Z

include/oneapi/dpl/internal/dynamic_selection_impl/sycl_backend.h

+    std::vector<async_waiter_base*> async_waiter_arr;
+
+    template<typename T>
+    void add_waiter(T *t){


Are you protecting this? Is it thread safe?

vossmjp · 2024-03-22T21:30:22Z

include/oneapi/dpl/internal/dynamic_selection_impl/sycl_backend.h

                });
-            });
-            return async_waiter{e2};
+                return async_waiter{e2, new SelectionHandle(s)};


I know we don't currently have a policy that requires reporting of both task completion and time, but this implementation would not support it because of this return. If we assumed they were mutually exclusive, the next condition would be an "else if", but its just an "if" so that both might be supported.

vossmjp · 2024-03-22T21:34:22Z

include/oneapi/dpl/internal/dynamic_selection_traits.h

@@ -299,6 +311,14 @@ struct report_value
 template <typename S, typename Info>
 inline constexpr bool report_value_v = report_value<S, Info>::value;

+template <typename S>
+struct lazy_report_value


I believe this is the first trait for a backend -- i.e. that a backend requires a call to lazy report. Backends are not as visible as policies so likely don't want a trait in this header. End users cannot access the backend, only the policy implementation can.

Not sure how to check if the backend has certain functions without traits. Created a new namespace called backend_traits for checking presence of lazy_report and has_enable_profiling in the backend from the policy

include/oneapi/dpl/internal/dynamic_selection_impl/sycl_backend.h

* test/parallel_api/dynamic_selection/sycl/test_auto_tune_policy_sycl.pass.cpp - fix USM shared memory leaks Signed-off-by: Sergey Kopienko <[email protected]> * test/parallel_api/dynamic_selection/sycl/test_auto_tune_policy_sycl.pass.cpp - create unique task names for h.single_task([](){}); Signed-off-by: Sergey Kopienko <[email protected]> --------- Signed-off-by: Sergey Kopienko <[email protected]>

@AnuyaWelling2801

Already fixed by @AnuyaWelling2801

…aram (#1539) * include/oneapi/dpl/internal/dynamic_selection_traits.h - fix traits to specify report method second parameter type Signed-off-by: Sergey Kopienko <[email protected]> * Change second parameter type of auto_tune_selection_type::report method - to report_duration (std::chrono::milliseconds) Signed-off-by: Sergey Kopienko <[email protected]> --------- Signed-off-by: Sergey Kopienko <[email protected]>

vossmjp · 2024-04-26T19:37:32Z

include/oneapi/dpl/internal/dynamic_selection_impl/dynamic_load_policy.h

@@ -150,6 +150,10 @@ struct dynamic_load_policy
    selection_type
    select(Args&&...)
    {
+        if constexpr (backend_traits::lazy_report_v<Backend>)


@AnuyaWelling2801 Shouldn't this file need to include the backend_traits.h? Is it somehow including indirectly?

The backend_traits.h file is being included indirectly since the tests include the file oneapi/dpl/dynamic_selection. This file includes auto_tune_policy,h (which contains the file backend_traits.h) before the file dynamic_load_policy.h

include/oneapi/dpl/internal/dynamic_selection_impl/backend_traits.h

include/oneapi/dpl/internal/dynamic_selection_impl/sycl_backend.h

The critical issues have been addressed

… function private

include/oneapi/dpl/internal/dynamic_selection_impl/sycl_backend.h

include/oneapi/dpl/internal/dynamic_selection_impl/auto_tune_policy.h

…... args) function (#1547) * include/oneapi/dpl/internal/dynamic_selection_impl/sycl_backend.h - simplify sycl_backend::submit(SelectionHandle s, Function&& f, Args&&... args) function Signed-off-by: Sergey Kopienko <[email protected]> * include/oneapi/dpl/internal/dynamic_selection_impl/sycl_backend.h - remove not required type name Signed-off-by: Sergey Kopienko <[email protected]> --------- Signed-off-by: Sergey Kopienko <[email protected]>

akukanov · 2024-04-30T17:27:08Z

include/oneapi/dpl/internal/dynamic_selection_impl/sycl_backend.h

        {
-            std::chrono::steady_clock::time_point t0;
-            if constexpr (report_value_v<SelectionHandle, execution_info::task_time_t>)
+            const auto t0 = report_clock_type::now();


I am not sure I agree with the change Sergey proposed to remove if constexpr around taking the initial timing snapshot. It makes the code concise, but is that better in this case? Performance-wise it might be OK if we assume the overhead of std::chrono::steady_clock::now() is negligible, but it provokes questions like "why are the conditions to set and to use the variable different, including even constexpr checks; is it by purpose?".

Maybe using the ternary operator ?: is a good compromise here; it is not constexpr but hopefully the compiler can optimize it anyway. Though if I am the only one concerned and others are not, then it's fine to keep the code as now.

I think it's ok as is. Let's keep as is. We can always revisit it if we see some performance impact later.

vossmjp

All issues are resolved, So looks good to me now.

vossmjp · 2024-04-30T19:06:50Z

include/oneapi/dpl/internal/dynamic_selection_impl/sycl_backend.h

        {
-            std::chrono::steady_clock::time_point t0;
-            if constexpr (report_value_v<SelectionHandle, execution_info::task_time_t>)
+            const auto t0 = report_clock_type::now();


I think it's ok as is. Let's keep as is. We can always revisit it if we see some performance impact later.

AnuyaWelling2801 added 6 commits January 9, 2024 15:15

Added static assert for keyArgs==Args

9417ddb

WIP for jit compilation

7e54e28

Fixing Autotune bug

268f0d5

Added sycl profiling without host task

0dda712

Corrections in dynamic traits

a38327a

Remove comments in dynamic_selection_traits

70f993c

AnuyaWelling2801 requested review from reble, vossmjp and egfefey March 22, 2024 16:15

Merged main

59fa9a8

AnuyaWelling2801 marked this pull request as draft March 22, 2024 17:22

Adding back jit compolation restrictions

69f5014

vossmjp reviewed Mar 22, 2024

View reviewed changes

AnuyaWelling2801 added this to the 2022.6.0 milestone Mar 25, 2024

AnuyaWelling2801 mentioned this pull request Mar 25, 2024

[Dynamic Selection] Use sycl profiling for auto tune policy #1255

Closed

AnuyaWelling2801 changed the title ~~Adding sycl profiling for auto tune policy~~ [Dynamic Selection] Adding sycl profiling for auto tune policy Mar 26, 2024

Adressing comments to add thread safety, better traits

757c470

AnuyaWelling2801 marked this pull request as ready for review March 27, 2024 16:25

AnuyaWelling2801 added 2 commits March 27, 2024 15:12

Added backend trait to check if profiling is enabled

37a895a

Backend traits, renaming and adding a lock to lazy report

d9fe635

SergeyKopienko reviewed Apr 8, 2024

View reviewed changes

include/oneapi/dpl/internal/dynamic_selection_impl/sycl_backend.h Outdated Show resolved Hide resolved

SergeyKopienko reviewed Apr 8, 2024

View reviewed changes

include/oneapi/dpl/internal/dynamic_selection_impl/sycl_backend.h Outdated Show resolved Hide resolved

SergeyKopienko reviewed Apr 8, 2024

View reviewed changes

include/oneapi/dpl/internal/dynamic_selection_impl/sycl_backend.h Outdated Show resolved Hide resolved

SergeyKopienko reviewed Apr 8, 2024

View reviewed changes

include/oneapi/dpl/internal/dynamic_selection_impl/sycl_backend.h Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

Fixed memory leaks

95ee3ac

SergeyKopienko reviewed Apr 10, 2024

View reviewed changes

include/oneapi/dpl/internal/dynamic_selection_impl/sycl_backend.h Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

No nullptr for selection handle in async waiter constructor

cd5b318

This comment was marked as outdated.

Sign in to view

SergeyKopienko reviewed Apr 26, 2024

View reviewed changes

include/oneapi/dpl/internal/dynamic_selection_impl/sycl_backend.h Outdated Show resolved Hide resolved

AnuyaWelling2801 and others added 2 commits April 26, 2024 10:29

Addressed comments to sycl backend

b70aeec

This comment was marked as resolved.

Sign in to view

SergeyKopienko self-requested a review April 26, 2024 16:19

SergeyKopienko and others added 2 commits April 26, 2024 11:36

Fixing clang format

c217e1a

vossmjp reviewed Apr 26, 2024

View reviewed changes

AnuyaWelling2801 added 3 commits April 26, 2024 15:11

Adding header file for backend_traits

75dd226

Changed structure for sycl_backend submit

bc95cd2

Fixed clang format

19e78e7

akukanov reviewed Apr 29, 2024

View reviewed changes

AnuyaWelling2801 added 4 commits April 29, 2024 11:28

Changes to make variables is_profiling_enabled and aliases for report…

3d3dab2

… function private

Moved is_profiling_enabled outside the async_waiter class

8009669

Making sycl backend submit function more readable

ae46b12

Fixed clang format

0cf0daa

SergeyKopienko reviewed Apr 30, 2024

View reviewed changes

include/oneapi/dpl/internal/dynamic_selection_impl/sycl_backend.h Outdated Show resolved Hide resolved

SergeyKopienko reviewed Apr 30, 2024

View reviewed changes

include/oneapi/dpl/internal/dynamic_selection_impl/sycl_backend.h Show resolved Hide resolved

SergeyKopienko reviewed Apr 30, 2024

View reviewed changes

include/oneapi/dpl/internal/dynamic_selection_impl/sycl_backend.h Outdated Show resolved Hide resolved

SergeyKopienko reviewed Apr 30, 2024

View reviewed changes

include/oneapi/dpl/internal/dynamic_selection_impl/sycl_backend.h Outdated Show resolved Hide resolved

SergeyKopienko reviewed Apr 30, 2024

View reviewed changes

include/oneapi/dpl/internal/dynamic_selection_impl/auto_tune_policy.h Show resolved Hide resolved

SergeyKopienko and others added 3 commits April 30, 2024 10:30

Minor changes to auto_tune_policy.h and sycl_backend.h

aa07b29

Improving readability

797ea84

akukanov reviewed Apr 30, 2024

View reviewed changes

vossmjp approved these changes Apr 30, 2024

View reviewed changes

AnuyaWelling2801 merged commit 5ea755c into main Apr 30, 2024
19 of 20 checks passed

AnuyaWelling2801 deleted the dev/awelling/auto_tune_sycl_profiling branch April 30, 2024 19:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Dynamic Selection] Adding sycl profiling for auto tune policy #1464

[Dynamic Selection] Adding sycl profiling for auto tune policy #1464

AnuyaWelling2801 commented Mar 22, 2024 •

edited

Loading

vossmjp left a comment

vossmjp Mar 22, 2024

AnuyaWelling2801 Mar 28, 2024

vossmjp Mar 22, 2024

AnuyaWelling2801 Mar 28, 2024

vossmjp Mar 22, 2024

AnuyaWelling2801 Mar 28, 2024

vossmjp Mar 22, 2024

AnuyaWelling2801 Mar 28, 2024

vossmjp Mar 22, 2024

AnuyaWelling2801 Mar 28, 2024

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as outdated.

This comment was marked as resolved.

vossmjp Apr 26, 2024

AnuyaWelling2801 Apr 26, 2024

AnuyaWelling2801 Apr 29, 2024

vossmjp Apr 30, 2024

akukanov Apr 30, 2024 •

edited

Loading

vossmjp Apr 30, 2024

vossmjp left a comment

vossmjp Apr 30, 2024

[Dynamic Selection] Adding sycl profiling for auto tune policy #1464

[Dynamic Selection] Adding sycl profiling for auto tune policy #1464

Conversation

AnuyaWelling2801 commented Mar 22, 2024 • edited Loading

vossmjp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as outdated.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akukanov Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vossmjp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnuyaWelling2801 commented Mar 22, 2024 •

edited

Loading

akukanov Apr 30, 2024 •

edited

Loading