[SYCL] Remove circular dependency between piEventRelease and cleanup() #6073
Conversation
Event is retained explicitely when created in createEventAndAssociateQueue() to ensure that the event is not destroyed before it is really signalled. Ref count is decremented in Event->cleanup() after event is singalled. This means that if reference count for an event is zero then cleanup was already called for this event and should not be called anymore. Interop events and user events don't have associated command list, queue, kernel and waitlist, so we don't need to call cleanup() for interop and user events.
| @@ -5544,9 +5544,6 @@ static pi_result EventRelease(pi_event Event, pi_queue LockedQueue) { | |||
| } | |||
|
|
|||
| if (--(Event->RefCount) == 0) { | |||
| if (!Event->CleanedUp) | |||
There was a problem hiding this comment.
What will call the "cleanup" of an event that wasn't explicitly waited with piEventsWait?
There was a problem hiding this comment.
Currently nothing will call cleanup in this case no matter with or without proposed change.
Per our current implementation if event is not waited then it's refcount is not zero, so we won't go through check on line 5546 in this case.
And per current implementation cleanup is supposed to be called for event which was waited. It looks like it was done intentionally to not release/reset resources like associated kernel/commandlist if event was not waited.
We can change this behavior obviously and cleanup everything even if associated work is not finished. But I am not sure if it is going to be correct,
As for me proposed change is needed anyway, this call to cleanup() under RefCount == 0 check is incorrect.
There was a problem hiding this comment.
We can change this behavior obviously and cleanup everything even if associated work is not finished.
That we cannot do, otherwise we might destroy some L0 resources that are still being used and result in a segfault or even worse. But we must cleanup everything after waiting for everything is completed. If we aren't cleaning up, this means we have memory leaks, right?
As for me proposed change is needed anyway, this call to cleanup() under RefCount == 0 check is incorrect.
I don't disagree to that, but want to understand better why this was needed before.
|
Merged in scope of #6179 |
Event is retained explicitely when created in createEventAndAssociateQueue()
to ensure that the event is not destroyed before it is really signalled.
Ref count is decremented in Event->cleanup() after event is singalled. This
means that if reference count for an event is zero then cleanup was already
called for this event and should not be called anymore.
Interop events and user events don't have associated command list, queue,
kernel and waitlist, so we don't need to call cleanup() for interop and user
events.