[NFCI][SYCL] Refactor event_impl creation
#18907
Conversation
aelovikov-intel
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
* Inherit from `std::enable_shared_for_this` as part of a bigger scope ongoing refactoring * Unlike other `*_impl` classes, `event_impl` ctors before this change weren't immediately obvious for an unfamiliar reader. Because of that, instead of a single variaded `event_impl::create(Ts&&...)` I opted to have multiple named version of it, each delegating to its own ctor. Also, some ctor code was reshuffled between different overloads.
aelovikov-intel
force-pushed
the
event_impl
branch
from
fa59bd9 to
7315e73
Compare
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
| break; | ||
| } | ||
| case HES_NotComplete: { | ||
| MIsProfilingEnabled = true; |
There was a problem hiding this comment.
It looks like MIsHostEvent wasn't set for this scenario before, but this is weird to me. If anyone has any idea, I'd like to hear it.
There was a problem hiding this comment.
I'm not sure if it is relevant to this, but maybe @steffenlarsen will help me remember the details.
The default event() gets created in lots of places in our codebase but then is promptly thrown away. Because it is often occurring on the critical path, its default construction needs to be super fast and cannot waste time. One of the issues we dealt with was the context, deferring its assignment unless and until needed. But that introduced a complication with the "is this a host event" question, which before our change, simply checked the context. We resolved it, but there was a bit of legerdemain where we had to avoid prematurely setting the MIsHostEvent flag. I'm probably misremembering. Also, I don't think we touched this routine.
There was a problem hiding this comment.
Sadly I don't remember the details either, but it smells like events are due for some refactoring in general. The idea of "host" seems very overloaded.
| : MQueue{Queue.weak_from_this()}, | ||
| MIsProfilingEnabled{Queue.MIsProfilingEnabled} { | ||
| this->setContextImpl(Queue.getContextImplPtr()); | ||
| MState.store(HES_Complete); |
There was a problem hiding this comment.
That's how it was before my changes, but I have no idea why. If anyone has any idea, I'd like to hear it.
|
@slawekptak , @sergey-semenov , ping. |
sycl/source/detail/event_impl.hpp
Outdated
| } | ||
|
|
||
| static std::shared_ptr<event_impl> | ||
| create_from_underlying(ur_event_handle_t Event, const context &SyclContext) { |
There was a problem hiding this comment.
Maybe "create_from_handle" would be more aligned with the current naming? We have getHandleRef() functions returning the UR handles.
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aelovikov-intel
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aelovikov-intel
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
aelovikov-intel
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
aelovikov-intel
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
aelovikov-intel
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
aelovikov-intel
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
Continuation of the refactoring efforts in intel#18715 intel#18748 intel#18830 intel#18907 intel#18983
Continuation of the refactoring efforts in intel#18715 intel#18748 intel#18830 intel#18907 intel#18983
Continuation of the refactoring efforts in intel#18715 intel#18748 intel#18830 intel#18907 intel#18983
Continuation of the refactoring efforts in intel#18715 intel#18748 intel#18830 intel#18907 intel#18983
Continuation of the refactoring efforts in intel#18715 intel#18748 intel#18830 intel#18907 intel#18983 intel#19004 intel#19006
Continuation of the refactoring efforts in intel#18715 intel#18748 intel#18830 intel#18907 intel#18983 intel#19006
Continuation of the refactoring efforts in intel#18715 intel#18748 intel#18830 intel#18907 intel#18983 intel#19006
std::enable_shared_for_thisas part of a bigger scope ongoing refactoring*_implclasses,event_implctors before this change weren't immediately obvious for an unfamiliar reader. Because of that, instead of a single variadedevent_impl::create(Ts&&...)I opted to have multiple named versions of it, each delegating to its own ctor. Also, some ctor code was reshuffled between different overloads.