Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add peak memory usage tracking to cuIO benchmarks #7770
Add peak memory usage tracking to cuIO benchmarks #7770
Changes from 3 commits
c34b32b
b9147f4
c49d60b
4664f9a
d10265c
5c90e76
7f94765
e2fff21
30a921c
8d85fa9
804cb80
71c5d32
4662f6e
baa4150
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you getting consistent numbers between runs? @kaatish mentioned that the peak usage varies when running the ORC writer benchmark.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tested, it reports the same number for corresponding benchmarks across runs for ORC. @kaatish, what method did you use to test peak memory usage? If you're using nsys, you need to turn off the pool allocator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used the script referenced in issue #7661 which calls pynvml methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to make changes to all benchmarks. Do you want to add this in a separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No I just wanted to get reviews on the method first, before going ahead and changing all the benchmarks. In the first commit, I made the tracking resource part of the base fixture and that automatically enabled it for every benchmark. But a problem with that was that it tracked memory usage for the setup as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in 4664f9a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use this to catch memory leaks in tests (just query current instead of max on exit)?
Edit: leaks are pretty unlikely given libcudf's coding style, we probably don't need to invest time into this check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be pretty easy. But since this is more of a test (curr value should always be 0 at the end) rather than a benchmark, should we make this opt-in? If anyone suspects a leak, they can add a check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of adding this check to the unit tests, rather than benchmarks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well sure. I can do that in a subsequent PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized that any leak we have would probably be due to memory allocated without the rmm resource. Anything with rmm is likely using some RAII object to hold the memory. And this doesn't track non-rmm allocated memory.