-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Improved profiler result printing. #7709
Conversation
src/runtime/profiler_common.cpp
Outdated
while (sstr.size() < cursor) { | ||
sstr << " "; | ||
} | ||
|
||
float ft = fs->time / (p->runs * 1000000.0f); | ||
if (ft < 10000) sstr << " "; |
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.
All "if" statements in Halide should be in {braces}, even trivial single-line ones like these
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.
Oh, hmm. Then why doesn't the clang-format CI-step complain?
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.
clang-format didn't, but clang-tidy did
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.
Ah, what, clang-tidy checks this separately. Clang-format has an option for this too, I believe.
Dang this clang-tidy fix makes the code really ugly... |
Yeah, clang-tidy and clang-format make things better on average. There are definitely cases where it makes things worse, but it's worth it in expectation. |
I can surround it with |
We should fix our clang-format setings to enforce this as well (if possible). In the meantime, please just manually format in a way that makes clang-tidy happy; we prefer to reserve |
Okay, then this is it. 😄 |
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.
Thanks for the fix!
* Fixed the regularization for BGU. * Improved profiler result printing. * Clang-format ain't liking pretty code. * Clang-tidy ain't liking pretty code. --------- Co-authored-by: Steven Johnson <srj@google.com>
This 50-cents improvement makes my profiling reports more readable, as I have long function names. Additionally, I cheaply aligned the timing by printing spacing depending on how many order of magnitude the number spans. Example output below: