[SYCL] Fix memory leak (queue_impl) due to #5901

[KseniyaTikhomirova]

Cross dependency event_impl vs queue_impl prevents objects release. Event_impl now has only weak pointer to queue.

Signed-off-by: Tikhomirova, Kseniya [email protected]

[al42and]

Not that anyone asked, but I can confirm that it fixes the problem I was having 👍

[KseniyaTikhomirova]

Not that anyone asked, but I can confirm that it fixes the problem I was having 👍

thank you, good to know

[steffenlarsen]

Is there any case where the queue may die before the event? In that case, are there any operations on the event that would try to lock the queue and fail?

[KseniyaTikhomirova]

Is there any case where the queue may die before the event? In that case, are there any operations on the event that would try to lock the queue and fail?

I was also thinking about this. In event_impl there is very similar approach in event_impl::flushIfNeeded.
I think it is possible when on application side we save event but destroy queue, device queue (host is stored in scheduler also and is destroyed with it). In this case operations with event like wait still should be ok because PIEvent is still alive and keeps reference to PiQueue. So earlier destruction is possible for queue_impl wrapper not backend objects.

[steffenlarsen]

I might worry that this could break if the scenario you mentioned happens and the event is then used as a dependency event in some command. Maybe the worker queue is only really used if the event is the result event of a command? If so, the command constructor should ensure it gets initialized. Is that the case?

[KseniyaTikhomirova]

I might worry that this could break if the scenario you mentioned happens and the event is then used as a dependency event in some command. Maybe the worker queue is only really used if the event is the result event of a command? If so, the command constructor should ensure it gets initialized. Is that the case?

Could you please describe a bit more what you meant here "Maybe the worker queue is only really used if the event is the result event of a command?" I am not sure that understand scenario in a right way

[steffenlarsen]

Could you please describe a bit more what you meant here "Maybe the worker queue is only really used if the event is the result event of a command?" I am not sure that understand scenario in a right way

From my reading of these changes, the worker queue is only set on the result event of a command and not any of the dependency events. I guess my main concern is if someone does something like

sycl::event Ev;
{
  sycl::queue Q1;
  Ev = Q.submit(...); // Ev will have Q1 as its worker queue.
}
{
  sycl::queue Q2;
  Q.submit([&](sycl::handler &CGH) {
   CGH.depends_on(Ev); // Ev.getWorkerQueue() will fail to lock its weak_ptr?
   ...
  }
}

it will fail. Is that a possible case? If so, maybe getWorkerQueue could be changed to return nullptr if the worker queue is dead? Would that break something?

[KseniyaTikhomirova]

it will fail. Is that a possible case? If so, maybe getWorkerQueue could be changed to return nullptr if the worker queue is dead? Would that break something?

What I expect in the scenario above:

1. std::weak_ptr::lock - "Creates a new std::shared_ptr that shares ownership of the managed object. If there is no managed object, i.e. *this is empty, then the returned shared_ptr also is empty.". So getWorkerQueue should already be able to return nullptr if weak_ptr is expired.
2. yes, in the scenario above I see the possibility for queue to be deleted after ev related work completion but PiEVent in ev is still alive and related PiQueue - too. On the second kernel submission in getPIEvent we will handle PiEVent of ev which is still absolutely valid (and completed BTW if Queue is deleted) and pass to backend.

[KseniyaTikhomirova]

it will fail. Is that a possible case? If so, maybe getWorkerQueue could be changed to return nullptr if the worker queue is dead? Would that break something?

I will try to add suggested code as unit test tomorrow.

[steffenlarsen]

std::weak_ptr::lock - "Creates a new std::shared_ptr that shares ownership of the managed object. If there is no managed object, i.e. *this is empty, then the returned shared_ptr also is empty.". So getWorkerQueue should already be able to return nullptr if weak_ptr is expired.

Admittedly I completely misremembered the semantics of std::weak_ptr::lock. A unittest would indeed put my mind at ease, thank you! 😄

[KseniyaTikhomirova]

Unfortunately scenario above involves host task execution because we connect two Queues via connection task which is always a host task.
It is not possible to implement a proper unit test there. In the scenario waiting for piEvent related to event for which Queue can be deleted is done via host task execution. That means that I need to involve DispatchHostTask execution that works with real scheduler and real plugins which we can not afford.
I can confirm that your scenario successfully passes locally and can suggest llvm-test-suite test only but not sure if it is reasonable enough.

[KseniyaTikhomirova]

failures are not related, already reported here #6760

[KseniyaTikhomirova]

@intel/llvm-gatekeepers hello, could you please merge it?