Skip to content

[SYCL] Attach auxiliary resources to memory objects #5588

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

steffenlarsen
Copy link
Contributor

@steffenlarsen steffenlarsen commented Feb 16, 2022

Some reductions may require additional resources. These resources can be buffers, so to avoid introducing unintended synchronization points these resources must be stored until later. Previously however, storing these resources were done as part of the associated command, which could mean that there would be a circular reference keeping both alive, consequently causing a memory leak.

These changes introduce a new strategy for these auxiliary resources as being "attached" to memory objects (USM and SYCLMemObjI). Attachments are tracked by the scheduler and when a memory object is being freed it will detach the resources.

Some reductions may require additional resources. These resources can
be buffers, so to avoid introducing unintended synchronization points
these resources must be stored until later. Previously however, storing
these resources were done as part of the associated command, which
could mean that there would be a circular reference keeping both alive,
consequently causing a memory leak.

These changes introduce a new strategy for these auxiliary resources as
being "attached" to memory objects (USM and SYCLMemObjI). Attachments
are tracked by the scheduler and when a memory object is being freed it
will detach - and for USM defer the deletion of - the resources.

Signed-off-by: Steffen Larsen <[email protected]>
@steffenlarsen steffenlarsen requested a review from a team as a code owner February 16, 2022 10:43
@steffenlarsen
Copy link
Contributor Author

/summary:run

Signed-off-by: Steffen Larsen <[email protected]>
@steffenlarsen
Copy link
Contributor Author

There was an issue with deferring the release of resources attached to USM due to the delayed release happening after context destruction. It has now been changed to release on detachment, which may cause USM free to sync because it has a resource buffer applied.

@romanovvlad @sergey-semenov @s-kanaev - Would this be a problem? Alternative is to transfer the attached resource to the associated context and have it release it when being destroyed.

Signed-off-by: Steffen Larsen <[email protected]>
@steffenlarsen
Copy link
Contributor Author

Based on previous testing, the issues do appear to have been fixed now. Added the missing windows symbols.

@steffenlarsen
Copy link
Contributor Author

/summary:run

@steffenlarsen
Copy link
Contributor Author

/verify with intel/llvm-test-suite#624

@romanovvlad
Copy link
Contributor

There was an issue with deferring the release of resources attached to USM due to the delayed release happening after context destruction. It has now been changed to release on detachment, which may cause USM free to sync because it has a resource buffer applied.

@romanovvlad @sergey-semenov @s-kanaev - Would this be a problem? Alternative is to transfer the attached resource to the associated context and have it release it when being destroyed.

It should not be a problem since the operation these buffer would wait during destruction is supposed to be completed in sycl::free call.

@steffenlarsen
Copy link
Contributor Author

After offline discussion with @romanovvlad we agreed that there is no good way to consistently release memory object (not USM) resources managed by contexts. When we can break ABI we should have the memory objects hold the resources, but until then we will let the global handler manage a map for these.

Signed-off-by: Steffen Larsen <[email protected]>
@steffenlarsen
Copy link
Contributor Author

/verify with intel/llvm-test-suite#624

Copy link
Contributor

@romanovvlad romanovvlad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@steffenlarsen
Copy link
Contributor Author

Closing this in favor of a stricter solution: #5653
This PR was insufficient for another upcoming patch, so the other solution was preferred.

@steffenlarsen steffenlarsen deleted the steffen/attach_aux_resources branch December 6, 2023 11:38
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.

2 participants