[SYCL] Fix memory leak for interop events created from native handle #6180
Conversation
|
I expect pre-commit testing to fail at least because of intel/llvm-test-suite#1032 I'm planning to wait for results and possibly make more changes in in the test suite based on the testing results. |
| // a task exists. Indeed, the user might have created ZeEvent in non-signaled | ||
| // state and never used it to run anything. As such, even in the scenario with | ||
| // ownership transfer, it's user's responsibility to ensure that lifetime of | ||
| // the sycl::even they are creating covers the point where anything signals | ||
| // it. |
There was a problem hiding this comment.
I agree that we should not expect to see a "signal" of this event, or "wait" on it without user's explicit ask for that (e.g. using this event as a dependency of another event). But we don't also require the user to signal event. Instead, disregarding of whether it was signaled or not, we should delete PI event when SYCL event goes out of its scope (but only destroy L0 native event if the ownership was transferred to SYCL RT). I think at least the comment needs to be refined.
| @@ -5664,6 +5664,20 @@ pi_result piextEventCreateWithNativeHandle(pi_native_handle NativeHandle, | |||
| // need to create a host-visible proxy for it. | |||
| (*Event)->HostVisibleEvent = *Event; | |||
|
|
|||
| // Regular events managed by SYCL RT are created with RefCnt == 2 so that | |||
| // their lifetime is extended beyond that of of the associated pi_event as we | |||
There was a problem hiding this comment.
beyond that of of the associated pi_event
The lifetime of L0 event and PI event (pi_event) are equal for them. We don't destroy PI event while L0 event is still running, and we do destroy both when event is completed and released.
There was a problem hiding this comment.
Can you suggest the change in wording that would be accurate here?
Co-authored-by: smaslov-intel <sergey.v.maslov@intel.com>
intel#6180 moved the retain logic for events to the make_event interop API. However, this means that the old OpenCL interop constructor for event does not retain the native event. These changes add the retain logic directly to the old OpenCL interop constructor for OpenCL. Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
#6180 moved the retain logic for events to the make_event interop API. However, this means that the old OpenCL interop constructor for event does not retain the native event. These changes add the retain logic directly to the old OpenCL interop constructor for OpenCL. Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
No description provided.