Conversation
src/profiler/profiler.h
Outdated
| /*! \brief id of thread which operation run on */ | ||
| std::thread::id thread_id_ = std::this_thread::get_id(); // Not yet seen a | ||
| // case where this isn't valid | ||
| size_t thread_id_ = std::hash<std::thread::id>{}(std::this_thread::get_id()); |
There was a problem hiding this comment.
why do it here instead of emit time?
There was a problem hiding this comment.
Does each ProfileStat get used only once? I thought this would be better, because we can hash once only.
There was a problem hiding this comment.
it's no longer thread_id_ if you do this...
src/profiler/profiler.h
Outdated
| std::thread::id thread_id_ = std::this_thread::get_id(); // Not yet seen a | ||
| // case where this isn't valid | ||
| size_t thread_id_ = std::hash<std::thread::id>{}(std::this_thread::get_id()); | ||
| // thread getid returns a hex, hashing it to get a size_t |
There was a problem hiding this comment.
nit: usually comment goes above the code?
src/profiler/profiler.cc
Outdated
| } else { | ||
| file << ",\n"; | ||
| } | ||
| file << ",\n"; |
There was a problem hiding this comment.
Do we need this ? Also is there a reason not to use std::endl ?
There was a problem hiding this comment.
There were two new lines here earlier. Using one \n is slightly better for performance since we are anyway flushing the buffer with the second new line
|
@marcoabreu Looks like this test |
|
Yes it's flaky unfortunately. Please just push a new commit to trigger CI |
src/profiler/profiler.h
Outdated
| /*! \brief id of thread which operation run on */ | ||
| std::thread::id thread_id_ = std::this_thread::get_id(); // Not yet seen a | ||
| // case where this isn't valid | ||
| size_t thread_id_ = std::hash<std::thread::id>{}(std::this_thread::get_id()); |
src/profiler/profiler.h
Outdated
| /*! \brief id of thread which operation run on */ | ||
| std::thread::id thread_id_ = std::this_thread::get_id(); // Not yet seen a | ||
| // case where this isn't valid | ||
| size_t thread_id_ = std::hash<std::thread::id>{}(std::this_thread::get_id()); |
There was a problem hiding this comment.
it's no longer thread_id_ if you do this...
|
Hi, the community has passed to vote about associating the code changes with JIRA (https://lists.apache.org/thread.html/ab22cf0e35f1bce2c3bf3bec2bc5b85a9583a3fe7fd56ba1bbade55f@%3Cdev.mxnet.apache.org%3E) We have updated the guidelines for contributors in https://cwiki.apache.org/confluence/display/MXNET/Development+Process, please ensure that you have created a JIRA at https://issues.apache.org/jira/projects/MXNET/issues/ to describe your work in this pull request and include the JIRA title in your PR as [MXNET-xxxx] your title where MXNET-xxxx is the JIRA id Thanks! |
|
@cjolivier01 ping |
* hash tid * remove first flag conditions to fix missing commas * hash tid * remove first flag conditions to fix missing commas * update comment * CI restart * address review comments
* hash tid * remove first flag conditions to fix missing commas * hash tid * remove first flag conditions to fix missing commas * update comment * CI restart * address review comments
* hash tid * remove first flag conditions to fix missing commas * hash tid * remove first flag conditions to fix missing commas * update comment * CI restart * address review comments
Description
Fixes issue where thread id can be hex, and the hex symbols are not recognized by chrome tracing tool. Using hash to convert it to an integer
The json file has some missing commas because of the first_flag condition. The reason for this seems lost at this point. Removing it bring back the comma necessary for a valid json. Verified that it does generate valid jsons by training a network.
Checklist
Essentials
make lint)Changes