-
Notifications
You must be signed in to change notification settings - Fork 41
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
[WIP] Flamegraph generation for rootbench.git #94
Conversation
Co-Authored-By: Vassil Vassilev <[email protected]>
It could be invocated from Cmake or as a standalone.
root/roofit/roofit/.rootrc
Outdated
@@ -0,0 +1 @@ | |||
RooFit.Banner: no |
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.
Could you add a comment, explaining why we need this?
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.
Anything that had been even linked with Roofit will automatically have a Roofit banner as a part of output. Only way to suppress it is to put a .rootrc with suppression setting in a place where binary will be executed (by default in ROOT master suppression is OFF and I didn't manage to convince to remove it, since it is a part of ownership notification).
rootbench-scripts/flamegraph.sh
Outdated
#!/bin/bash | ||
|
||
BENCHMARKPATTERN="*benchmark*" | ||
MKDIR=/bin/mkdir |
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.
We should use MKDIR=$(which mkdir)
. Likewise for the rest.
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.
okay! thanks
rootbench-scripts/flamegraph.sh
Outdated
echo "Can't create directory $1. Exiting..." | ||
exit 1 | ||
fi | ||
for bm in $bm_sub_list |
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.
Could you run this script through a bash code formatter tool? Indentations and new lines seem non-standard.
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.
yes sure.
Closing because I moved development in #192 (hopefully we can merge it soon!) |
No description provided.