Adds cleanup functions for resources - #224
Conversation
|
Will we need a patch release ? We use the wait_for functions in my application, |
|
No, we probably don't need a patch release for these changes. With these changes, applications will realistically be calling the Regarding the |
rhornung67
left a comment
rhornung67
left a comment
There was a problem hiding this comment.
Seems reasonable to me. However, someone else should review and approve to make sure.
|
|
||
| static stream_state& get_stream_state() | ||
| { | ||
| static stream_state state; |
There was a problem hiding this comment.
Given that we're manually handling the thread safety now, we should probably make this a class static member variable so it doesn't get another layer of thread safe initialization that comes from being a function local static. Given that we have a library we should probably put the variable definition in the camp sources, as opposed to an inline static member var.
| * The caller must ensure no other thread is using CUDA resources while | ||
| * cleanup runs. | ||
| */ | ||
| static void cleanup() |
There was a problem hiding this comment.
Does it make more sense to put most of this code in a member function of the state?
There was a problem hiding this comment.
I can move most of the cleanup logic to be a state member function. However, I think we will still want a static cleanup function on the resource to be consistent across resources.
| if (num < 0) { | ||
| std::lock_guard<std::mutex> lock(s_mtx); | ||
| s_previous = (s_previous + 1) % num_streams; | ||
| return s_streams[s_previous]; |
There was a problem hiding this comment.
This still needs to be locked.
There was a problem hiding this comment.
In fact does it make sense to make this whole function a member function of the state now?
There was a problem hiding this comment.
It probably makes sense to make getting a stream and getting the default stream member functions at this point.
4 participants
This is a bugfix to add cleanup functions for resources.
The Hip and Cuda resources are non-owning and never destroy streams, which show up as leaks in memory checkers/sanitizers. In these cases, the streams would be destroyed in the new cleanup function. If there is no cleanup needed for a resource (such as Host, Omp_target, Sycl), then the cleanup function will be a no-op.
An open question for this PR is whether the cleanup functions should be thread-safe? Currently, these are not thread safe to be consistent with
RAJA::release_unused_internal_memory().Note: These changes were created by AI. I have reviewed the changes and they seemed reasonable.
Related to: #223