-
Notifications
You must be signed in to change notification settings - Fork 174
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 CUPTI/RoCM versions to traces #985
Conversation
This pull request was exported from Phabricator. Differential Revision: D62538511 |
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.
Overall LGTM, just had some suggestions for handling ownership of the mutex, things may get tricky if callers are expected to handle it.
void addMetadata(const std::string& key, const std::string& value) { | ||
std::lock_guard<std::mutex> guard(mutex_); | ||
void addMetadata(const std::string& key, const std::string& value, bool lock_mutex=true) { | ||
if (lock_mutex){ |
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'm not certain about the branching logic with mutex controls. I think it can lead to over-complicated scenarios of lock ownership.
Would it make sense to use a recursive_mutex, so that we allow C++ to handle the ownership, and double-locking on the same lock is fine?
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.
Good point for sure. Let me try a recursive mutex!
Summary: Pull Request resolved: pytorch#985 Because of the differences that are emerging between different versions, it would be useful in the metadata we could see which third-party library version we are using. We add them to our kineto traces in this diff. Reviewed By: aaronenyeshi Differential Revision: D62538511
This pull request was exported from Phabricator. Differential Revision: D62538511 |
b7f637b
to
2bceb21
Compare
This pull request has been merged in ca1eedb. |
Summary: Because of the differences that are emerging between different versions, it would be useful in the metadata we could see which third-party library version we are using. We add them to our kineto traces in this diff.
Differential Revision: D62538511