[SYCL][UR][L0 v2] Fix issues with event lifetime

[igchor]

When releasing executionEvent in the commandBuffer we need to also retain the queue. Otherwise event->release() in commandBuffer's destructor might attempt to release the event to an event pool that has already been destroyed.

Also, make sure eventPool is destroyed after commandListManager to avoid any issues with events owned by the commandListManager and move context retain/release from commandListManager to queue/command_buffer.

The implementation of commandListManager marked move ctor and assignment operator as defaulted which was wrong - the implementation would have to account for context and device retain/release calls. To avoid this, move the logic to queue/commandBuffer which can have move ctors/assignments operators removed.

[pbalcer]

When releasing executionEvent in the commandBuffer we need to also retain the queue. Otherwise event->release() in commandBuffer's destructor might attempt to release the event to an event pool that has already been destroyed.

Event pools are released to the context when the queue is released. So this would mean that the command buffer outlives the context itself, not just the queue.

[pbalcer]

lgtm, but please add a test that releases a command buffer and queue before using the command buffer for the last time.

[igchor]

When releasing executionEvent in the commandBuffer we need to also retain the queue. Otherwise event->release() in commandBuffer's destructor might attempt to release the event to an event pool that has already been destroyed.

Event pools are released to the context when the queue is released. So this would mean that the command buffer outlives the context itself, not just the queue.

Yes, and we could actually solve this problem by retaining the context in the command buffer, rather than the queue. However, there is a potential problem with the execution event: if the queue associated with that event is destroyed and we want to later synchronize on the execution event we might crash because (as far as I know) counter-based events rely on the associated command-list for keeping some of it's state (and command list is potentially destroyed/re-used as soon as queue is released). I did solve this problem by retaining the queue in command buffer but now as I think about it, it is a more general problem. What if someone wants to pass an event as a dependency to some operation after the associated queue is destroyed?

I think we might actually need to start retaining the queue when allocating events (which we have tried to avoid until now). One alternative I'm considering is to do some logic in the event pool. For example, we could create another raii wrapper that would internally store cache_borrowed_event_pool* and if eventPool->events.size() != eventPool->freelist.size() delay returning the event pool to the context and destroying the queue until all events are released (add a callback to eventPool::free)

[pbalcer]

:(

I think we might actually need to start retaining the queue when allocating events (which we have tried to avoid until now). One alternative I'm considering is to do some logic in the event pool. For example, we could create another raii wrapper that would internally store cache_borrowed_event_pool* and if eventPool->events.size() != eventPool->freelist.size() delay returning the event pool to the context and destroying the queue until all events are released (add a callback to eventPool::free)

I think this might be the best option. I'll think on it.

But let's do that as a separate PR.

[igchor]

:(

I think we might actually need to start retaining the queue when allocating events (which we have tried to avoid until now). One alternative I'm considering is to do some logic in the event pool. For example, we could create another raii wrapper that would internally store cache_borrowed_event_pool* and if eventPool->events.size() != eventPool->freelist.size() delay returning the event pool to the context and destroying the queue until all events are released (add a callback to eventPool::free)

I think this might be the best option. I'll think on it.

But let's do that as a separate PR.

Yes, for now, I think we can just merge the fix with context retain/release - this is enough for all the current tests to pass (with ooo queue).

[igchor]

@intel/sycl-graphs-reviewers could you please take a look?