Skip to content

[Bindless][UR][LevelZero] Fix memory leak caused by not destroying sampler handles #18240

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

GeorgeWeb
Copy link
Contributor

@GeorgeWeb GeorgeWeb commented Apr 29, 2025

This PR fixes memory leaks where both UR and LevelZero sampler handles weren't being released and memory wasn't cleaned up appropriately when the bindless images object gets destroyed. The old SYCL 2020 images' sampler implementation is different as it is a self-managed RAII object which is not true for bindless images, hence we need to ensure, depending on the requirements of the backend/adapter API, to manage the lifetime of the sampler associated to the bindless image accordingly.

@GeorgeWeb GeorgeWeb force-pushed the georgi/fix-ze-sampler-create-leak branch from ff210d6 to 7f7474d Compare April 29, 2025 13:54
@GeorgeWeb GeorgeWeb marked this pull request as ready for review April 29, 2025 14:54
@GeorgeWeb GeorgeWeb requested a review from a team as a code owner April 29, 2025 14:54
@igchor
Copy link
Member

igchor commented Apr 29, 2025

@GeorgeWeb How do other backends handle the lifetime of the sampler? The specification says nothing about urBindlessImagesSampledImageCreateEx taking ownership of the sampler. Isn't it possible for user to use the same sampler in multiple calls?

@GeorgeWeb
Copy link
Contributor Author

GeorgeWeb commented Apr 30, 2025

@GeorgeWeb How do other backends handle the lifetime of the sampler? The specification says nothing about urBindlessImagesSampledImageCreateEx taking ownership of the sampler. Isn't it possible for user to use the same sampler in multiple calls?

Hey @igchor, you raise a good point and I think I didn't think this through fully. There is no precedent to take ownership of the sampler there. Actually this is not a Unified Runtime issue at the moment but rather it's just a bindless images issue definition, hence this PR currently may not be the right approach. It is, at the moment, absolutely plausible for the user to use the same sampler in multiple calls. However, I am not sure if this is the right direction or bindless images should just tie the sampler with the image altogether - that is a new point of discussion. Hence, I may close this PR altogether as we explore into defining and handling properly. Thanks.

@GeorgeWeb GeorgeWeb closed this Apr 30, 2025
@ProGTX
Copy link
Contributor

ProGTX commented Apr 30, 2025

Note this relates to oneapi-src/unified-runtime#1463

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants