[SYCL] Do not lock unconditionally while access queue_iml::MInOrderExternalEvent #17575

Alexandr-Konovalov · 2025-03-21T15:03:06Z

queue_impl::MInOrderExternalEvent is empty on hot path, use a flag to avoid locking when empty.

queue_impl::MMissedCleanupRequests and queue_impl::MInOrderExternalEvent are empty on hot path, check a flag instead or before the locking.

aelovikov-intel

Honestly, I'd prefer it to be two different PRs (in case something would regress, easier bisecting/reverts).

Also, I'd rather see a bit extra boilerplate code via abstracting
class Helper{ atomic flag; other_data b; methods... };

so that we could ensure that whenever b is set/unset the flag is always updated properly.

aelovikov-intel · 2025-03-21T20:28:37Z

sycl/source/detail/queue_impl.hpp

@@ -1040,6 +1047,8 @@ class queue_impl {
  // the fallback implementation of profiling info
  bool MFallbackProfiling = false;

+  // Is value presented in MInOrderExternalEvent?
+  std::atomic_bool MHasValueInOrderExternalEvent = false;


I think the name is confusing, maybe MInOrderExteranEventIsSet would be better?

Second, is it correct to assume that we still want the optional because default-constructed/empty event is heavy?

Second, is it correct to assume that we still want the optional because default-constructed/empty event is heavy?

Yep, one is heavy. I hope one day we able to implement an empty sycl::event with just empty std::shared_ptr<detail::event_impl> impl, then optional became unneeded.

aelovikov-intel · 2025-03-21T20:36:22Z

sycl/source/detail/queue_impl.cpp

@@ -801,6 +802,7 @@ void queue_impl::revisitUnenqueuedCommandsState(
  else {
    std::lock_guard<std::mutex> RequestLock(MMissedCleanupRequestsMtx);
    MMissedCleanupRequests.push_back(CompletedHostTask->getCommandGraph());
+    MAreCleanupRequestsMissed.store(true, std::memory_order_release);


Why is the following not an issue:

Thread A Thread B acquire mtx check atomic without mutex, empty push_back atomic_store_true

?

Thank you, this is exactly the question I would like to discuss with the fellow sycl experts!

According to my understanding, the change is correct, because the situation you describing is undistinguishing from a situation when Thread A just staying, say, at the beginning of queue_impl::revisitUnenqueuedCommandsState(), i.e. there is no reason for Thread B to expect something in MMissedCleanupRequests at this point.

@sergey-semenov , @KseniyaTikhomirova would be much more knowledgeable about this than I am.

Chatted with @KseniyaTikhomirova about this today. Like Alexandr said, the case is indistinguishable from the first thread being earlier in its execution path.
With the synchronization being made less strict, it will be slightly more likely that we'll exit queue::wait with some dependency related data still present in the queue. But that's not a functional problem since that data will be handled during the next cleanup call.

Alexandr-Konovalov · 2025-03-24T08:52:45Z

Honestly, I'd prefer it to be two different PRs (in case something would regress, easier bisecting/reverts).

Sure, will do, after (or if) the correctness issue would be resolved.

Alexandr-Konovalov · 2025-04-01T11:00:40Z

Honestly, I'd prefer it to be two different PRs (in case something would regress, easier bisecting/reverts).

Sure, will do, after (or if) the correctness issue would be resolved.

If you are ok with the change, it's turn out 3 PR: with the template addition and 2 with enabling it for in-order event and for missed requests. Is it sound good?

aelovikov-intel · 2025-04-01T15:56:00Z

If you are ok with the change, it's turn out 3 PR: with the template addition and 2 with enabling it for in-order event and for missed requests. Is it sound good?

Adding yet unused helper in a separate PR will mean a unittest needs to be introduced together with it :) I think if you'd have two different PRs that introduce the same helper in otherwise unmodified area of the code, they won't actually cause any merge conflicts, regardless of the order in which they'll go in.

Alexandr-Konovalov · 2025-04-02T13:17:03Z

If you are ok with the change, it's turn out 3 PR: with the template addition and 2 with enabling it for in-order event and for missed requests. Is it sound good?

Adding yet unused helper in a separate PR will mean a unittest needs to be introduced together with it :) I think if you'd have two different PRs that introduce the same helper in otherwise unmodified area of the code, they won't actually cause any merge conflicts, regardless of the order in which they'll go in.

Interesting. To be honest, I have a different model here. I thought about single PR with 3 commits inside (adding CheckLockCheck and enabling for MMissedCleanupRequests and for MInOrderExternalEvent in separate commits). Then, merging to sycl should be done without squash.

But your solution with 2 PRs is also possible.

Are you ok with the code, so I can start splitting it to two PRs?

aelovikov-intel · 2025-04-02T14:38:38Z

Then, merging to sycl should be done without squash.

We don't do that in this project (I'm not even sure if that's enabled).

aelovikov-intel · 2025-04-02T14:41:03Z

sycl/source/detail/queue_impl.hpp

+      MDataPresent.store(true, std::memory_order_release);
+      func(MData);
+    }
+    template <typename F> void get(F &&func) {


I think modifications on get might be unexpected. Would pop work as a name?

Done. Then, it might be reasonable to change put to pop.

Did you mean push?

Sorry, typo. Yes, push/pop instead of put/get.

aelovikov-intel · 2025-04-02T14:43:34Z

sycl/source/detail/queue_impl.hpp

@@ -850,12 +855,12 @@ class queue_impl {
    auto EventRet = Handler.finalize();
    EventImplPtr EventRetImpl = getSyclObjImpl(EventRet);
    if (Type == CGType::CodeplayHostTask)
-      Deps.UnenqueuedCmdEvents.push_back(EventRetImpl);
+      Deps.UnenqueuedCmdEvents.push_back(std::move(EventRetImpl));


getSyclObjImpl(EventRet) vs std::move(EventRetImpl) is almost equal, I don't think the temporary variable is justified anymore. IMO, inline it and maybe add a comment for the future as well.

It's turn out this was already modified on sycl branch (reference used instead). So, dropped from the patch.

aelovikov-intel · 2025-04-02T14:45:30Z

sycl/source/detail/queue_impl.hpp

-    std::lock_guard<std::mutex> Lock(MInOrderExternalEventMtx);
-    MInOrderExternalEvent = Event;
+    MInOrderExternalEvent.put(
+        [&Event](std::optional<event> &InOrderExternalEvent) {


I wouldn't mind [&](auto &InOrderExternalEvent) { ... } here and similarly elsewhere, but won't insist.

aelovikov-intel · 2025-04-03T15:56:29Z

Are you ok with the code, so I can start splitting it to two PRs?

I am. @sergey-semenov , any objections?

Alexandr-Konovalov · 2025-04-07T09:55:04Z

Are you ok with the code, so I can start splitting it to two PRs?

I am. @sergey-semenov , any objections?

@sergey-semenov, what do you think?

sergey-semenov · 2025-04-07T11:00:04Z

No objections from me.

Alexandr-Konovalov · 2025-04-09T17:36:47Z

Colleagues @aelovikov-intel , @sergey-semenov , what do you think?

aelovikov-intel · 2025-04-09T18:22:30Z

check a flag instead or before

This might be hard to read. Maybe "use a flag to avoid locking when empty"?

Alexandr-Konovalov · 2025-04-10T10:02:56Z

check a flag instead or before

This might be hard to read. Maybe "use a flag to avoid locking when empty"?

Sure, done.

[SYCL] Do not lock unconditionally while access queue_iml members

5dfc457

queue_impl::MMissedCleanupRequests and queue_impl::MInOrderExternalEvent are empty on hot path, check a flag instead or before the locking.

Alexandr-Konovalov temporarily deployed to WindowsCILock March 21, 2025 15:03 — with GitHub Actions Inactive

Alexandr-Konovalov temporarily deployed to WindowsCILock March 21, 2025 15:19 — with GitHub Actions Inactive

Alexandr-Konovalov marked this pull request as ready for review March 21, 2025 19:10

Alexandr-Konovalov requested a review from a team as a code owner March 21, 2025 19:10

Alexandr-Konovalov requested a review from aelovikov-intel March 21, 2025 19:10

aelovikov-intel reviewed Mar 21, 2025

View reviewed changes

Move check-lock-check logic to a separate template.

2af3dec

Alexandr-Konovalov had a problem deploying to WindowsCILock April 1, 2025 10:56 — with GitHub Actions Error

Code formatting.

32a8487

Alexandr-Konovalov temporarily deployed to WindowsCILock April 1, 2025 11:12 — with GitHub Actions Inactive

Alexandr-Konovalov had a problem deploying to WindowsCILock April 1, 2025 11:27 — with GitHub Actions Error

Code formatting.

4bd575c

Alexandr-Konovalov temporarily deployed to WindowsCILock April 1, 2025 12:05 — with GitHub Actions Inactive

Alexandr-Konovalov temporarily deployed to WindowsCILock April 1, 2025 12:28 — with GitHub Actions Inactive

aelovikov-intel reviewed Apr 2, 2025

View reviewed changes

Alexandr-Konovalov added 2 commits April 2, 2025 18:43

Rename put/get to push/pop. Change args passing in lambda functions.

2b1b35f

Merge branch 'sycl' into Alexandr-Konovalov/check-external-event

1cdfa2b

Alexandr-Konovalov had a problem deploying to WindowsCILock April 2, 2025 16:59 — with GitHub Actions Error

Code formatting.

ed1c0c4

Alexandr-Konovalov had a problem deploying to WindowsCILock April 3, 2025 14:49 — with GitHub Actions Error

Code formatting.

5b3a159

Alexandr-Konovalov temporarily deployed to WindowsCILock April 3, 2025 14:50 — with GitHub Actions Inactive

Alexandr-Konovalov temporarily deployed to WindowsCILock April 3, 2025 15:25 — with GitHub Actions Inactive

Merge branch 'sycl' into Alexandr-Konovalov/check-external-event

9332d0e

Alexandr-Konovalov temporarily deployed to WindowsCILock April 7, 2025 11:53 — with GitHub Actions Inactive

Alexandr-Konovalov had a problem deploying to WindowsCILock April 7, 2025 12:11 — with GitHub Actions Error

Alexandr-Konovalov added 2 commits April 7, 2025 14:12

Revert changes related to MMissedCleanupRequests.

ec7cce0

Fix comment.

3b24720

Alexandr-Konovalov temporarily deployed to WindowsCILock April 7, 2025 12:30 — with GitHub Actions Inactive

Alexandr-Konovalov changed the title ~~[SYCL] Do not lock unconditionally while access queue_iml members~~ [SYCL] Do not lock unconditionally while access queue_iml::MInOrderExternalEvent Apr 7, 2025

Alexandr-Konovalov temporarily deployed to WindowsCILock April 7, 2025 12:45 — with GitHub Actions Inactive

This was referenced Apr 7, 2025

[SYCL] Do not lock unconditionally while access queue_iml::MMissedCleanupRequests #17883

Merged

[SYCL] Optimize handler::StoreLambda implementation #17669

Merged

Merge branch 'sycl' into Alexandr-Konovalov/check-external-event

e3afeba

Alexandr-Konovalov temporarily deployed to WindowsCILock April 8, 2025 09:40 — with GitHub Actions Inactive

Alexandr-Konovalov temporarily deployed to WindowsCILock April 8, 2025 10:15 — with GitHub Actions Inactive

aelovikov-intel approved these changes Apr 9, 2025

View reviewed changes

aelovikov-intel merged commit 10da83a into intel:sycl Apr 10, 2025
24 checks passed

[SYCL] Do not lock unconditionally while access queue_iml::MInOrderExternalEvent #17575

[SYCL] Do not lock unconditionally while access queue_iml::MInOrderExternalEvent #17575

Uh oh!

Conversation

Alexandr-Konovalov commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aelovikov-intel left a comment

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!

Alexandr-Konovalov commented Mar 24, 2025

Uh oh!

Alexandr-Konovalov commented Apr 1, 2025

Uh oh!

aelovikov-intel commented Apr 1, 2025

Uh oh!

Alexandr-Konovalov commented Apr 2, 2025

Uh oh!

aelovikov-intel commented Apr 2, 2025

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!

aelovikov-intel commented Apr 3, 2025

Uh oh!

Alexandr-Konovalov commented Apr 7, 2025

Uh oh!

sergey-semenov commented Apr 7, 2025

Uh oh!

Alexandr-Konovalov commented Apr 9, 2025

Uh oh!

aelovikov-intel commented Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Alexandr-Konovalov commented Apr 10, 2025

Uh oh!

Uh oh!

Uh oh!

Alexandr-Konovalov commented Mar 21, 2025 •

edited

Loading

aelovikov-intel commented Apr 9, 2025 •

edited

Loading