namhyung / uftrace

Function graph tracer for C/C++/Rust/Python
https://uftrace.github.io/slide/
GNU General Public License v2.0
2.92k stars 419 forks source link

Support standard deviation(STDDEV) in report output #1913

Closed ziming-zh closed 2 months ago

ziming-zh commented 2 months ago

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)), where mean = 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 , 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 from pytorch/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.

ziming-zh commented 2 months ago

@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.

ziming-zh commented 2 months ago

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!

namhyung commented 2 months ago

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).

ziming-zh commented 2 months ago

@namhyung Thanks for the comment. I've switched back to the stdv notation. Please review the revision. Thanks!

namhyung commented 2 months ago

Hmm.. I found that some report diff tests are failing.

$ make -j8 runtest TESTARG=report_diff
  TEST     test_run
Start 10 tests with 8 worker

Compiler                  gcc                                           clang                                       
Runtime test case         pg             finstrument-fu fpatchable-fun  pg             finstrument-fu fpatchable-fun
------------------------: O0 O1 O2 O3 Os O0 O1 O2 O3 Os O0 O1 O2 O3 Os  O0 O1 O2 O3 Os O0 O1 O2 O3 Os O0 O1 O2 O3 Os
067 report_diff         : SG SG SG SG SG SG SG SG SG SG SG SG SG SG SG  SG SG SG SG SG SG SG SG SG SG SG SG SG SG SG
158 report_diff_policy1 : SG SG SG SG SG SG SG SG SG SG SG SG SG SG SG  SG SG SG SG SG SG SG SG SG SG SG SG SG SG SG
159 report_diff_policy2 : SG SG SG SG SG SG SG SG SG SG SG SG SG SG SG  SG SG SG SG SG SG SG SG SG SG SG SG SG SG SG
160 report_diff_policy3 : SG SG SG SG SG SG SG SG SG SG SG SG SG SG SG  SG SG SG SG SG SG SG SG SG SG SG SG SG SG SG
177 report_diff_policy4 : SG SG SG SG SG SG SG SG SG SG SG SG SG SG SG  SG SG SG SG SG SG SG SG SG SG SG SG SG SG SG
239 report_diff_field1  : OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK  OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK
240 report_diff_field2  : OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK  OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK
241 report_diff_field3  : OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK  OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK
242 report_diff_field4  : OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK  OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK
243 report_diff_field5  : OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK  OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK

runtime test stats
====================
total   300  Tests executed (success: 50.00%)
  OK:   150  Test succeeded
  OK:     0  Test succeeded (with some fixup)
  NG:     0  Different test result
  NZ:     0  Non-zero return value
  SG:   150  Abnormal exit by signal
  TM:     0  Test ran too long
  BI:     0  Build failed
  LA:     0  Unsupported Language
  SK:     0  Skipped
ziming-zh commented 2 months ago

That is strange. I think I don't change any of the features related to diff here

ziming-zh commented 2 months ago

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.

namhyung commented 2 months ago

Really? I don't see the failures in the master branch.

namhyung commented 2 months ago

I can reproduce it easily like this:

$ uftrace record --force pwd
/home/namhyung/tmp

$ uftrace record --force pwd
/home/namhyung/tmp

$ uftrace report --diff=uftrace.data.old
Segmentation fault.

With gdb and build with DEBUG=1, I can see this.

Program received signal SIGSEGV, Segmentation fault.
0x000055555559a79b in add_field (output_fields=0x5555555e9030 <output_fields>, field=0x0) at /home/namhyung/project/uftrace/utils/field.c:80
80      if (field->used)

It seems to access the NULL pointer for field.

Here's the backtrace.

(gdb) bt
#0  0x000055555559a79b in add_field (output_fields=0x5555555e9030 <output_fields>, field=0x0) at /home/namhyung/project/uftrace/utils/field.c:80
#1  0x00005555555b276f in setup_default_field (fields=0x5555555e9030 <output_fields>, opts=0x7fffffffd9d0, 
    p_field_table=0x5555555ea920 <field_diff_table>) at /home/namhyung/project/uftrace/utils/report.c:959
#2  0x000055555559a918 in setup_field (output_fields=0x5555555e9030 <output_fields>, opts=0x7fffffffd9d0, 
    setup_default_field=0x5555555b2711 <setup_default_field>, field_table=0x5555555ea920 <field_diff_table>, field_table_size=10)
    at /home/namhyung/project/uftrace/utils/field.c:125
#3  0x00005555555b2a00 in setup_report_field (output_fields=0x5555555e9030 <output_fields>, opts=0x7fffffffd9d0, avg_mode=AVG_NONE)
    at /home/namhyung/project/uftrace/utils/report.c:1024
#4  0x0000555555580467 in report_diff (handle=0x7fffffffd7b0, opts=0x7fffffffd9d0) at /home/namhyung/project/uftrace/cmds/report.c:471
#5  0x00005555555808df in command_report (argc=0, argv=0x7fffffffdca0, opts=0x7fffffffd9d0) at /home/namhyung/project/uftrace/cmds/report.c:546
#6  0x00005555555650aa in main (argc=0, argv=0x7fffffffdca0) at /home/namhyung/project/uftrace/uftrace.c:1543
namhyung commented 2 months ago

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 REPORT_F_SIZE in utils/field.h.

ziming-zh commented 2 months ago

I see. Thanks for the guidance. I think I can choose the later approach. Comparing stdv in diff mode seems not very much necessary?

namhyung commented 2 months ago

Comparing stdv in diff mode seems not very much necessary?

Well.. I don't think it's in the high priority at least. :)

ziming-zh commented 2 months ago

Hi @namhyung, I've tested the current version. Should succeed all report test case for now