Lazy initialization of queue inside dpcpp_default policy #1154

adamfidel · 2023-09-06T15:01:07Z

There are a few potential issues with oneDPL's global sycl::queue, including:

Requirement that a SYCL device is available whenever oneDPL is included as mentioned in Using oneDPL forces client to always run with "SYCL device" present #1060
Static initialization order fiasco with regard to queues being defined in other translation units

To address the issue, this PR lazily initializes the sycl::queue inside the dpcpp_default execution policy on first use.

include/oneapi/dpl/pstl/hetero/dpcpp/execution_sycl_defs.h

julianmi · 2023-09-06T16:19:18Z

include/oneapi/dpl/pstl/hetero/dpcpp/execution_sycl_defs.h

Could we also add a test that fails before this change?

We might be able to add a test that just includes oneDPL but doesn't call anything, and then invoke it with a device filter that disables all devices (e.g., ONEAPI_DEVICE_SELECTOR='!*:*'). In this case, the old code should throw an exception whereas this PR should not.

I have manually confirmed that this test works in principle. I created the following test:

#include <oneapi/dpl/execution> int main() { return 0; }

Running it as ONEAPI_DEVICE_SELECTOR='!*:*' ./build/test/header_inclusion_only.pass crashes on main but does not crash with this branch.

I feel that there is benefit in adding this test, but it will require some special changes to the CMake to modify the environment specifically for this test. It is further complicated by the fact that the CMake files currently already do something similar with the variable $ONEAPI_DEVICE_SELECTOR, which is either set to ONEAPI_DEVICE_SELECTOR=gpu/acc/fpga or SYCL_DEVICE_FILTER=gpu/acc/fpga depending on the compiler version and device type.

The dynamic selection branch also makes changes to this variable. In all, I believe that there is value for adding this test but the risks are high to unintentionally break something with this being so close to the code freeze.

include/oneapi/dpl/pstl/hetero/dpcpp/execution_sycl_defs.h

SergeyKopienko · 2023-09-06T17:06:28Z

include/oneapi/dpl/pstl/hetero/dpcpp/execution_sycl_defs.h

+    device_policy&
+    operator=(const device_policy& other)
+    {
+        q = other.queue();


Should we really need to create sycl::queue instance here in other.queue() call ?
May be good enough simple copy q state?

It should be sufficient to just copy q directly as you suggest.

include/oneapi/dpl/pstl/hetero/dpcpp/execution_sycl_defs.h

akukanov

I do not see any serious problems in the current version of the patch. However, the implementations of the policy classes could use some more uglification.

akukanov · 2023-09-06T22:13:24Z

include/oneapi/dpl/pstl/hetero/dpcpp/execution_sycl_defs.h

+
    explicit device_policy(sycl::queue q_) : q(q_) {}
-    explicit device_policy(sycl::device d_) : q(d_) {}
-    operator sycl::queue() const { return q; }
+    explicit device_policy(sycl::device d_) { q.emplace(d_); }


Could you reorder the code so that these existing constructors come before the new copy & move ones, perhaps right after the line 54?

include/oneapi/dpl/pstl/hetero/dpcpp/execution_sycl_defs.h

SergeyKopienko · 2023-09-07T07:59:08Z

include/oneapi/dpl/pstl/hetero/dpcpp/execution_sycl_defs.h

    {
+        ::std::scoped_lock lock{other.mtx};
+        q.swap(other.q);


I believe better to make simple move assignment to not extend life of our instance sycl::queue if it's already created earlier:

q = ::std::move(other.q);

Move assignment supported in std::optional : https://en.cppreference.com/w/cpp/utility/optional/operator%3D

Also header file for std::swap not included.

I also don't understand why we do use swap member function for move and don't use it for copy. I feel like reusing copy/move-assignment from optional is the right way to go. But if you have arguments why you did swap please clarify.

My other question is can we make member-init-list there instead of default constructing and then assigning? For example, astatic function that that locks and returns optional with the correct value category?

Also header file for std::swap not included.

That's something I don't understand at all. we don't use std::swap (at least in this line). For member function including <optional> is enough

I mean if we take a loot into https://en.cppreference.com/w/cpp/algorithm/swap,
there are 3 headers in which std::swap declared:
Defined in header <algorithm> (until C++11)
Defined in header <utility> (since C++11)
Defined in header <string_view> (since C++17)
And when I see introduced std::swap call I'm waiting for the inclusion of one from these headers.

I switched the swap with the move constructor and rewrote the copy/move constructor as suggested by @rarutyun.

https://github.com/oneapi-src/oneDPL/pull/1154/files/9d6b47a9ae2a0a22c750082719a53afee7031b3d#r1318514858

it was my mistake: sure, you have used swap member function, not ::std::swap and no additional headers required in this case.
I am sorry.

SergeyKopienko · 2023-09-07T08:01:03Z

include/oneapi/dpl/pstl/hetero/dpcpp/execution_sycl_defs.h

+    operator=(device_policy&& other)
+    {
+        ::std::scoped_lock lock{mtx, other.mtx};
+        q.swap(other.q);


The same:

q = ::std::move(other.q);

SergeyKopienko · 2023-09-07T08:11:17Z

include/oneapi/dpl/pstl/hetero/dpcpp/execution_sycl_defs.h

+
+    operator sycl::queue() const { return queue(); }
+    sycl::queue
+    queue() const


Am I correct that this function sycl::queue queue() should be declared as virtual now in this and base class?

Let's assume we have created the policy instance of fpga_policy
After this PR this instance doesn't have created sycl::queue instance inside as it's lazy initialized.

In some code we save result of static_cast<device_policy&>(policy) into auto& d_policy:

fpga_policy policy; auto& d_policy = static_cast<device_policy&>(policy);

and after that we call queue() function from reference to the base class:

fpga_policy policy; auto& d_policy = static_cast<device_policy&>(policy); auto q = d_policy.queue(); // call device_policy::queue() !?

Looks like we will call queue() method of the base class and creation of sycl::queue instance will be executed incorrectly.

Or we need some another approach to resolve this issue: may be we are able to pack sycl::queue creation into someone predicates: one predicate for the base class and one for derived class and so on.

May be able to apply something like #1158
but this PR doesn't resolve the problem of broken lazy initialization in constructor of

template <unsigned int other_factor, typename OtherName> fpga_policy(const fpga_policy<other_factor, OtherName>& other) : base(other.queue()){};

See my comment https://github.com/oneapi-src/oneDPL/pull/1154/files#r1318324154
Need to come up with something more.

I feel like that:

If we do that, this is not the design intent we want

If we don't do that - this is the API breaking change

include/oneapi/dpl/pstl/hetero/dpcpp/execution_sycl_defs.h

SergeyKopienko · 2023-09-07T09:13:27Z

include/oneapi/dpl/pstl/hetero/dpcpp/execution_sycl_defs.h

-    {
-    }
-
+    fpga_policy() = default;
    template <unsigned int other_factor, typename OtherName>
    fpga_policy(const fpga_policy<other_factor, OtherName>& other) : base(other.queue()){};


Lazy initialization concept would be broken at this place: queue() call will create sycl::queue instance

May be we should pass static_cast<const base&>(other) into constructor of base class to avoid this broken case?

Also looks like move constructor of this format and copy/move operaor= required in this class too.

Lazy initialization concept would be broken at this place: queue() call will create sycl::queue instance

That's exactly the question what behavior we want for non-default constructors and assignments. I think I want them being initialized (I can change my mind, though), but I also can see the issues with that since we have inheritance

include/oneapi/dpl/pstl/hetero/dpcpp/execution_sycl_defs.h

rarutyun · 2023-09-07T11:01:31Z

include/oneapi/dpl/pstl/hetero/dpcpp/execution_sycl_defs.h

    {
+        ::std::scoped_lock lock{other.mtx};
+        q.swap(other.q);


I also don't understand why we do use swap member function for move and don't use it for copy. I feel like reusing copy/move-assignment from optional is the right way to go. But if you have arguments why you did swap please clarify.

My other question is can we make member-init-list there instead of default constructing and then assigning? For example, astatic function that that locks and returns optional with the correct value category?

Also header file for std::swap not included.

That's something I don't understand at all. we don't use std::swap (at least in this line). For member function including <optional> is enough

rarutyun · 2023-09-07T11:05:27Z

include/oneapi/dpl/pstl/hetero/dpcpp/execution_sycl_defs.h

+    device_policy&
+    operator=(const device_policy& other)
+    {
+        ::std::scoped_lock lock{mtx, other.mtx};


I believe it's not done. What if it's self assignment? How does scoped_lock behave for the same mutex passed twice? My bet it's a deadlock (better to say, lifelock)

Yes, @rarutyun, you are right. Agreed.

I modified the code to now check for self assignment.

rarutyun · 2023-09-07T11:22:41Z

include/oneapi/dpl/pstl/hetero/dpcpp/execution_sycl_defs.h

+    device_policy(const device_policy<OtherName>& other)
+    {
+        ::std::scoped_lock lock{other.mtx};
+        q = other.q;


I am wondering what is the correct design? should we call other.queue() or other.q? This is relevant for all places. I understand the difference in the behavior and I am trying to understand what is the right one.

rarutyun · 2023-09-07T11:27:48Z

Just to state it explicitly. Those are not all comments that I have. I am thinking about Sergey's comment about virtuality and also about additional things for copy/move semantics

…, remove swap

include/oneapi/dpl/pstl/hetero/dpcpp/execution_sycl_defs.h

akukanov · 2024-07-10T12:45:55Z

After merging #1652, this patch is outdated.

adamfidel added 2 commits September 6, 2023 07:53

Lazy initialization of queue inside dpcpp_default policy

3c95aca

Fix clang-format

a3aea24

adamfidel added this to the 2022.3.0 milestone Sep 6, 2023

Update fpga_policy to use lazy initialization as well

5699334

adamfidel marked this pull request as ready for review September 6, 2023 16:01

adamfidel requested review from julianmi, akukanov, rarutyun, MikeDvorskiy, SergeyKopienko and mmichel11 September 6, 2023 16:02

adamfidel changed the title ~~Draft: Lazy initialization of queue inside dpcpp_default policy~~ Lazy initialization of queue inside dpcpp_default policy Sep 6, 2023

Clang-format

8b6bd6d

mmichel11 reviewed Sep 6, 2023

View reviewed changes

include/oneapi/dpl/pstl/hetero/dpcpp/execution_sycl_defs.h Show resolved Hide resolved

julianmi reviewed Sep 6, 2023

View reviewed changes

adamfidel added 2 commits September 6, 2023 09:42

Make device_policy thread-safe

888e0cb

clang-format

2c3096d

SergeyKopienko reviewed Sep 6, 2023

View reviewed changes

include/oneapi/dpl/pstl/hetero/dpcpp/execution_sycl_defs.h Show resolved Hide resolved

SergeyKopienko reviewed Sep 6, 2023

View reviewed changes

include/oneapi/dpl/pstl/hetero/dpcpp/execution_sycl_defs.h Outdated Show resolved Hide resolved

SergeyKopienko reviewed Sep 6, 2023

View reviewed changes

include/oneapi/dpl/pstl/hetero/dpcpp/execution_sycl_defs.h Show resolved Hide resolved

Changes to device_policy ctors for thread-safety

9d6b47a

akukanov reviewed Sep 6, 2023

View reviewed changes

SergeyKopienko requested changes Sep 7, 2023

View reviewed changes

SergeyKopienko reviewed Sep 7, 2023

View reviewed changes

include/oneapi/dpl/pstl/hetero/dpcpp/execution_sycl_defs.h Show resolved Hide resolved

SergeyKopienko reviewed Sep 7, 2023

View reviewed changes

include/oneapi/dpl/pstl/hetero/dpcpp/execution_sycl_defs.h Outdated Show resolved Hide resolved

rarutyun reviewed Sep 7, 2023

View reviewed changes

akukanov removed this from the 2022.3.0 milestone Sep 7, 2023

Check for self-assignment, use static lock method for copy/move ctors…

fee82e0

…, remove swap

SergeyKopienko reviewed Sep 8, 2023

View reviewed changes

include/oneapi/dpl/pstl/hetero/dpcpp/execution_sycl_defs.h Show resolved Hide resolved

rarutyun mentioned this pull request Sep 13, 2023

Using oneDPL forces client to always run with "SYCL device" present #1060

Closed

akukanov added the follow through PRs/issues that should be completed/resolved label Nov 29, 2023

SergeyKopienko mentioned this pull request Feb 27, 2024

Lazy sycl::queue creation in oneDPL hetero policies #1422

Closed

akukanov removed the follow through PRs/issues that should be completed/resolved label Jul 10, 2024

SergeyKopienko closed this Jul 10, 2024

SergeyKopienko deleted the dev/adamfidel/lazy_init_dpcpp_default branch July 10, 2024 12:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lazy initialization of queue inside dpcpp_default policy #1154

Lazy initialization of queue inside dpcpp_default policy #1154

adamfidel commented Sep 6, 2023

julianmi Sep 6, 2023

adamfidel Sep 6, 2023

adamfidel Sep 6, 2023

SergeyKopienko Sep 6, 2023

adamfidel Sep 6, 2023

adamfidel Sep 6, 2023

akukanov left a comment

akukanov Sep 6, 2023

adamfidel Sep 7, 2023

SergeyKopienko Sep 7, 2023

SergeyKopienko Sep 7, 2023

rarutyun Sep 7, 2023

SergeyKopienko Sep 7, 2023 •

edited

Loading

adamfidel Sep 7, 2023

SergeyKopienko Sep 8, 2023

SergeyKopienko Sep 7, 2023

SergeyKopienko Sep 7, 2023

SergeyKopienko Sep 7, 2023 •

edited

Loading

rarutyun Sep 7, 2023

SergeyKopienko Sep 7, 2023

SergeyKopienko Sep 7, 2023

SergeyKopienko Sep 7, 2023 •

edited

Loading

rarutyun Sep 7, 2023

rarutyun Sep 7, 2023

rarutyun Sep 7, 2023

SergeyKopienko Sep 7, 2023

adamfidel Sep 7, 2023

rarutyun Sep 7, 2023

rarutyun commented Sep 7, 2023

akukanov commented Jul 10, 2024

Lazy initialization of queue inside dpcpp_default policy #1154

Lazy initialization of queue inside dpcpp_default policy #1154

Conversation

adamfidel commented Sep 6, 2023

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

akukanov 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

SergeyKopienko Sep 7, 2023 • edited Loading

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

SergeyKopienko Sep 7, 2023 • edited Loading

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

SergeyKopienko Sep 7, 2023 • edited Loading

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

rarutyun commented Sep 7, 2023

akukanov commented Jul 10, 2024

SergeyKopienko Sep 7, 2023 •

edited

Loading

SergeyKopienko Sep 7, 2023 •

edited

Loading

SergeyKopienko Sep 7, 2023 •

edited

Loading