-
Notifications
You must be signed in to change notification settings - Fork 472
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
Support standard deviation(STDDEV) in report output #1913
Conversation
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 your work! In general, they look good. I've added some comments for some cleanups.
But I'd like to update the the commit message to have a "report:" prefix and remove the unrelated changes. Also it'd be nice if you would add some example output of the new user-facing changes. You can use the fibonacci example to show the stddev in the report. Ideally we can add it to the test suite to verify the behavior. Finally please update the uftrace-report and uftrace-tui documentation to add the new fields.
83bcd87
to
b826ed5
Compare
@namhyung Thank you for the feedback! I've made the requested changes:
Please review the changes and let me know if there are any further suggestions or modifications needed. |
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 add an example output in the commit message. I think uftrace report -f total-avg,total-stdv
would be fine.
f27c145
to
080a1cf
Compare
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.
Can you please break the long lines? I think the convention is 72 characters per line in the commit message. Also it'd be nice if you indent the examples by 4 spaces so that it can be displayed properly in markdown.
Hi @namhyung, I've changed everything to Coefficient of Variance (CV) now. Please check whether the current version works. Thank you! BTW, our instructor suggests that we should complete our contribution by the end of the semester (April 25th). Could you please kindly do me a favor and merge the PR before the deadline? I can make some in-time revision if you still find there could be some room for improvement. Much thanks for your help! |
Change looks good now. While it's technically correct to use the term "CV" it can be confusing with Curriculum Vitae (resume), so I think it's better to keep "STDV" and explain it in the documentation that it's actually "relative" standard deviation (RSD). |
@namhyung Thanks for the comment. I've switched back to the |
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.
Mostly looks good. Can you please use the full name in the Signed-off-by line?
66fefbc
to
2bce96f
Compare
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.
A few nitpicks on the commit message:
- Add a blank line between the subject line and the message body
- Break long lines in the message around 72 characters per line
- Merge the example usage and the output into a single block
- Use the actual output (verbatim) in the example
- Indent the example block with 4 spaces
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.
LGTM, thanks!
Hmm.. I found that some report diff tests are failing.
|
That is strange. I think I don't change any of the features related to diff here |
I've tried to directly running the test on the master branch, it also fails the same test. It seems that the test fail derives from previous commits. |
Really? I don't see the failures in the master branch. |
I can reproduce it easily like this:
With gdb and build with
It seems to access the NULL pointer for Here's the backtrace.
|
Ok, I think it's because you didn't implement stdv for diff mode. The diff field table doesn't have total_stdv and self_stdv. But the default field table has 'call' column which is not out of range in the field table due to the new stdv fields. It's questionable that comparing standard deviation though. You can either implement the diff mode or move stdv fields after |
I see. Thanks for the guidance. I think I can choose the later approach. Comparing stdv in diff mode seems not very much necessary? |
Well.. I don't think it's in the high priority at least. :) |
This commit introduces the ability to display the relative standard deviation (RSD) for function execution times in the uftrace report output. This feature helps in understanding the variability in function performance across different runs. Example usage: $ uftrace report -f total-avg,total-stdv Total avg Total stdv Function ========== ========== ==================== 10.263 ms 0.00% main 10.209 ms 0.00% bar 10.191 ms 0.00% usleep 8.125 us 5.43% foo 2.509 us 3.05% loop This enhancement adds the 'total-stdv' field, which can be specified alongside other fields like 'total-avg'. Resolves: namhyung#1897 Signed-off-by: Ziming Zhou <[email protected]>
Hi @namhyung, I've tested the current version. Should succeed all |
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.
LGTM!
Hello @namhyung,
I've tried to incorporate STDDEV calculation in the report output. It can now be displayed from
report -f all
command. In order to minimize the calculation overhead, I employ efficient running STDDEV calculation following the method according this post:stdev = sqrt((sum_x2 / n) - (mean * mean))
, wheremean = sum_x / n
.This method involves sqrt calculation, which is the main calculation overhead, the sqrt result should be rounded to uint_64 (ns) in consistency with the format of other statistics. I've tried optimization targeted on (int) -> (int) sqrt calculation. However, it turns out that the fastest way is still using the std C math library <math.h>, and cast the result to an int value. Relevant discussion could be found in this post
result = (int) sqrt(i)
(Note: In that case, the compilation tag -lm should be added to the makefile)
I've run on the tests and see that the pass/fail condition would be the same before / after the modification. I've also run defect detection tool Infer to guarantee that no more new defects have been introduced.
Subsequently, I've run the uftrace w/ & w/o STDDEV calculation on the same
mnist.py
code (adopted frompytorch/example
). Despite the two new columns added to the report, there seems no significant difference of the avg/min/max execution time in other columns, which means that the added STDDEV feature would not evidently interfere the original tracer functionality.I've also run Facebook's Infer tool to check for potential defection in the current code base. I've made three adjustment to the file handling (no close for open) and memory deallocation (no free after xalloc) in
utils/dwarfs.c
. I've attached the complete infer report in bugs.txt and report.csv, please see if you need help to check and correct the remaining of them.Finally, I must say this is such a good projects with very well-done documentation and superb functionality. Thank you for your effort! Please let me know for further revision of this PR.