-
Notifications
You must be signed in to change notification settings - Fork 838
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
Profiler improvements #2724
base: main
Are you sure you want to change the base?
Profiler improvements #2724
Conversation
// sections. This can be used in conjunction with running the relevant micro | ||
// benchmark to evaluate end-to-end performance. | ||
template <int MAX_EVENTS> |
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.
This should be template <int MAX_EVENTS = 4096>
so that all existing applications using the profiler do not need to be changed.
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.
Done
case MicroProfilerLogFormat::Csv: | ||
MicroPrintf("\"Event\",\"Tag\",\"Ms\",\"Ticks\""); | ||
for (int i = 0; i < num_events_; ++i) { | ||
#if defined(HEXAGON) || defined(CMSIS_NN) | ||
int ticks = end_ticks_[i] - start_ticks_[i]; | ||
MicroPrintf("%d,%s,%u,%d", i, tags_[i], TicksToMs(ticks), ticks); | ||
#else | ||
uint32_t ticks = end_ticks_[i] - start_ticks_[i]; | ||
uint64_t us = TicksToUs(ticks); | ||
MicroPrintf("%d,%s,%" PRIu64 ".%" PRIu64 ",%" PRIu32, i, tags_[i], | ||
us / 1000, us % 1000, ticks); | ||
#endif |
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.
Changing the CSV format probably won't pass review, as it most likely breaks countless internal Google, and external third-party scripts.
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.
Removed the Ms
column
const char* tag; | ||
uint32_t ticks; | ||
uint32_t tag_count; |
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.
Only used by the "human readable" log format.
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.
What should I do about that? #1290 introduced
struct TicksPerTag {
const char* tag;
uint32_t ticks;
};
// In practice, the number of tags will be much lower than the number of
// events. But it is theoretically possible that each event to be unique and
// hence we allow total_ticks_per_tag to have kMaxEvents entries.
TicksPerTag total_ticks_per_tag[kMaxEvents];
which was "only" used by the CSV log format.
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.
The comment is really just there for @suleshahid to see.
void SortTagGroups() { | ||
for (int i = 0; i < num_tag_groups_ - 1; ++i) { | ||
for (int j = i + 1; j < num_tag_groups_; ++j) { | ||
if (tag_groups_[j].ticks > tag_groups_[i].ticks) { | ||
TagGroup temp_tag_group = tag_groups_[i]; | ||
tag_groups_[i] = tag_groups_[j]; | ||
tag_groups_[j] = temp_tag_group; | ||
} | ||
} | ||
} | ||
} |
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.
Please use standard C++ library algorithms (as long as they don't require memory allocation). Here std::sort
should be used.
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.
Done
virtual uint32_t BeginEvent(const char* tag) override; | ||
uint32_t BeginEvent(const char* tag) { |
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.
This is an override of the abstract base class, and should remain as such.
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.
Done
virtual void EndEvent(uint32_t event_handle) override; | ||
void EndEvent(uint32_t event_handle) { |
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.
This is an override of the abstract base class, and should remain as such.
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.
Done
|
||
// Prints the profiling information of each of the events in human readable | ||
// form. | ||
void Log() const; | ||
void Log(MicroProfilerLogFormat format) const { |
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.
Should have a default argument of MicroProfilerLogFormt::Csv
so that all existing applications continue to compile without modification.
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.
Why CSV though? This method logs in human readable by default?
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 added HumanReadable
as default argument since that's what would keep the existing behavior while continuing to compile
|
||
// Prints the profiling information of each of the events in CSV (Comma | ||
// Separated Value) form. | ||
void LogCsv() const; |
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.
LogCsv
should be retained, and should call Log(MicroProfilerFormat::Csv)
, so that existing applications do not require modification.
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.
Done
case MicroProfilerLogFormat::Csv: { | ||
MicroPrintf("\"Tag\",\"Total ms\",\"Total ticks\""); | ||
for (int i = 0; i < num_tag_groups_; ++i) { | ||
uint64_t us = TicksToUs(tag_groups_[i].ticks); | ||
MicroPrintf("%s, %" PRIu64 ".%" PRIu64 ", %u", tag_groups_[i].tag, | ||
us / 1000, us % 1000, tag_groups_[i].ticks); | ||
} |
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.
Changing the CSV format probably won't pass review, as it most likely breaks countless internal Google, and external third-party scripts.
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.
Removed the Total ms
column
Thanks for your PR submission (and all the spelling corrections!). This will require some discussion with @suleshahid. I like the template change for sizing the timing data. |
// Prints total ticks for each unique tag in CSV format. | ||
// Output will have one row for each unique tag along with the | ||
// total ticks summed across all events with that particular tag. | ||
void LogTicksPerTagCsv(); |
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.
This method cannot be removed as it breaks existing applications.
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 it back
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.
Is there a policy to never break stuff like this? IMO duplicating APIs like this is also not great.
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.
@andresovela Let's just say, that at the present time, no one is allowed to break anything. 🙂
Oops, closed by mistake. |
"This PR is being marked as stale due to inactivity. Remove label or comment to prevent closure in 5 days." |
Waiting for review |
This PR adds a few improvements to the
MicroProfiler
class.bug=#2725
First, I made it a template so that it's possible to adjust the maximum number of events the profiler can handle via a template parameter.
The motivation for this change is to save memory. #1835 increased the maximum number of events from 1024 to 4096, which is a huge waste of RAM. The
MicroProfiler
class has these buffers:We need 20 bytes per event, so an instance of
MicroProfiler
uses 80 KB of RAM, even if the model you're profiling has only a few nodes.This PR also adds the following enum to specify the logging format and remove some duplicated code and clean the API.
I replaced
LogTicksPerTagCsv()
withLogGrouped()
, which does essentially the same thing, but also prints in a human readable format which looks like this:Finally I also added
TicksToUs()
to micro_time.h for convenience.