Skip to content
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

Fixed memory leaks in rocrand_tests #557

Merged

Conversation

NguyenNhuDi
Copy link
Collaborator

Added missing hipFree calls to test_rocrand_config_dispatch and test_rocrand_cpp_basic. Fixed an issue where test_utils was calling an extra hipCreateGraph call causing a graph to be overwritten which led to a memory leak.

Copy link
Collaborator

@umfranzw umfranzw left a comment

Choose a reason for hiding this comment

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

I took a look through the change - it's really nice to have the helper as a class. I just have a few suggestions for some more changes to the way a few of the new helper member functions work.

test/test_rocrand_hipgraphs.cpp Show resolved Hide resolved
test/test_rocrand_hipgraphs.cpp Outdated Show resolved Hide resolved
test/test_utils_hipgraphs.hpp Outdated Show resolved Hide resolved
test/test_utils_hipgraphs.hpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@umfranzw umfranzw left a comment

Choose a reason for hiding this comment

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

I think this looks good - I've just mentioned two more small things I noticed below.

test/test_utils_hipgraphs.hpp Outdated Show resolved Hide resolved
test/test_utils_hipgraphs.hpp Outdated Show resolved Hide resolved
@umfranzw umfranzw merged commit c1640d9 into ROCm:release-staging/rocm-rel-6.3 Oct 17, 2024
23 of 24 checks passed
stanleytsang-amd added a commit that referenced this pull request Nov 20, 2024
* Remove website URL from comments (#542)

Referencing or using code from some websites is prohibited in this repository.
This change removes an informational reference in the comments.

* Add gfx1151 target (#543) (#546)

Co-authored-by: Stanley Tsang <[email protected]>
Co-authored-by: Eiden Yoshida <[email protected]>

* Move data type support file along with index and ToC changes (#548) (#549)

* Convert change log to new format (#564)

* Fixed memory leaks in rocrand_tests (#557)

* added hipFree to test_rocrand_cpp_basic

* fixed memory leak for test_rocrand_config_dispatch

* fixed a memory leak in test_utils

* changed createGraph to createAndLaunchGraph, as well as fixed stream capture order

* changed default boolean (kaunchGraph, sync)  to be true in createAndLaunchGraph

* added back missing end stream capture

* reformated curlys for consistency

* removed createAndLaunchGraph inside resetGraphHelper

* Update changelog release headers (#568) (#575)

* Update changelog release headers

* Small correction

(cherry picked from commit c7da202)

* Add readme and metadata info and clarify CUDA references (#567) (#576)

(cherry picked from commit 81d9e58)

* added gfx12 and gfx1151 to default gpu list

* updated changelog

* Update CHANGELOG.md

Co-authored-by: Jeffrey Novotny <[email protected]>

* Update CHANGELOG.md

Co-authored-by: Jeffrey Novotny <[email protected]>

* Remove gfx940,gfx941 targets (#580)

* Remove unreleased

---------

Co-authored-by: Wayne Franz <[email protected]>
Co-authored-by: amd-garydeng <[email protected]>
Co-authored-by: Eiden Yoshida <[email protected]>
Co-authored-by: Jeffrey Novotny <[email protected]>
Co-authored-by: Di Nguyen <[email protected]>
Co-authored-by: Val Movsik <[email protected]>
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.

3 participants