[SYCL] Do not lock unconditionally while access queue_iml members #17575
Conversation
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
GitHub Actions
Inactive
Alexandr-Konovalov
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
Alexandr-Konovalov
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
There was a problem hiding this comment.
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.
| @@ -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; | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| @@ -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); | |||
There was a problem hiding this comment.
Why is the following not an issue:
Thread A Thread B
acquire mtx
check atomic without mutex, empty
push_back
atomic_store_true
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@sergey-semenov , @KseniyaTikhomirova would be much more knowledgeable about this than I am.
There was a problem hiding this comment.
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.
Sure, will do, after (or if) the correctness issue would be resolved. |
queue_impl::MMissedCleanupRequests and queue_impl::MInOrderExternalEvent are empty on hot path, check a flag instead or before the locking.