Minor profiler refactor to avoid string construction#197
Minor profiler refactor to avoid string construction#197zwxxx wants to merge 4 commits intomicrosoft:masterfrom
Conversation
| void Profiler::EndTimeAndRecordEvent(EventCategory category, | ||
| const std::string& event_name, | ||
| TimePoint& start_time, | ||
| const std::initializer_list<std::pair<std::string, std::string>>& event_args, |
There was a problem hiding this comment.
Won't this take away the ability to record multiple key-val pairs in a profile? Isn't invoking this function conditionally (that is only when profiling is enabled) enough?
There was a problem hiding this comment.
I dont see why one EndTimeAndRecordEvent should take multiple events. At least existing code has no such cases. But if you insist, I can add it back.
There was a problem hiding this comment.
Thinking more about it, yeah recording just one event is fine. However we should've a name for the thing that you're recording. So in this case I don't know if the string that you're recording is an op_name or something else. Would be good to add event_name and event_value parameters to the function.
There was a problem hiding this comment.
ok it makes sense. added std::pair<std::string, std::string> back.
|
/AzurePipelines run all |
onnxruntime/core/common/profiler.cc
Outdated
| const std::string& event_name, | ||
| TimePoint& start_time, | ||
| const std::string& op_name, | ||
| const std::pair<std::string, std::string>& op_name, |
There was a problem hiding this comment.
Thanks for this change. This pair can contain stuff besides op_name too, right? what's a good name for this parameter? tag?
There was a problem hiding this comment.
These are probably meant to act as props/annotations/tags for the actual event being recorded. The event being recorded is already being captured in the event_name param. So may be we can use props.
|
Looks like some of the tests failed. Can you rebase? |
yufenglee
left a comment
yufenglee
left a comment
There was a problem hiding this comment.
Sorry, I didn't notice this PR. I made a similar PR that was merged.
|
I would like to close this PR as it has been inactive for a while and similar change was merged. |
3 participants
std string concatenation allocates. Check if profiler enabled then call EndTimeAndRecordEvent to avoid string construction and allocation.
@pranavsharma
@tracysh