Re: Feedback on: [RESEND PATCH v2 0/5] perf report: Show inline stack

[Milian Wolff <milian.wolff@kdab.com>]

[-- Attachment #1: Type: text/plain, Size: 6725 bytes --]

On Monday, January 9, 2017 11:07:26 AM CET Jin, Yao wrote:
> Hi Wolff,
> 
> Thanks a lot for your feedback.
> 
> Have you tested with the browser mode? I mean without the "--stdio" option.

Yes, I have done that:

~~~~~~~~~~~~
milian@milian-kdab2:/tmp$ g++ -O2 -g inline.cpp -o inline
milian@milian-kdab2:/tmp$ perf record --call-graph dwarf ./inline
6.66016e+15
[ perf record: Woken up 5 times to write data ]
[ perf record: Captured and wrote 1.215 MB perf.data (149 samples) ]
~~~~~~~~~~~~

Both with and without your patch (and passing --inline) I get:

~~~~~~~~~~~~
milian@milian-kdab2:/tmp$ perf report --no-children --inline
Samples: 149  of event 'cycles:ppp', Event count (approx.): 141334499
  Overhead  Command  Shared Object     Symbol                                                                                                                                                                   
-   62.24%  inline   inline            [.] main                                                                                                                                                                 
     main                                                                                                                                                                                                       
     __libc_start_main                                                                                                                                                                                          
     _start                                                                                                                                                                                                     
-   26.24%  inline   libm-2.24.so      [.] __hypot_finite                                                                                                                                                       
   - __hypot_finite                                                                                                                                                                                             
      - 25.56% hypot                                                                                                                                                                                            
           main                                                                                                                                                                                                 
           __libc_start_main                                                                                                                                                                                    
           _start                                                                                                                                                                                               
        0.68% 0x40ead238ed1a2cf4                                                                                                                                                                                
-    6.85%  inline   libm-2.24.so      [.] hypot
     hypot
     main
     __libc_start_main                                                                                                                                                                                          
     _start
~~~~~~~~~~~~

> Could you give your test binary then I can try to reproduce the issue? I
> have tried with my tests but I could not reproduce.

The following code should reproduce the issue:

~~~~~~~~~~~~
#include <complex>
#include <cmath>
#include <random>
#include <iostream>

using namespace std;

int main()
{
    uniform_real_distribution<double> uniform(-1E5, 1E5);
    default_random_engine engine;
    double s = 0;
    for (int i = 0; i < 1000000; ++i) {
        s += norm(complex<double>(uniform(engine), uniform(engine)));
    }
    cout << s << '\n';
    return 0;
}
~~~~~~~~~~~~

Hope this helps

> On 1/8/2017 4:13 AM, Milian Wolff wrote:
> > Hey Jin,
> > 
> > I have finally had the time to test your patch series. Sadly, I hit a
> > fundamental issue with it early on in my testing:
> > 
> > In my test case, I get with `perf report --no-children` e.g. the 
following:
> >      42.50%  lab_mandelbrot   libm-2.24.so                [.]
> >      __hypot_finite
> >      
> >              ---__hypot_finite
> >              
> >                 hypot
> >                 main
> >                 __libc_start_main
> >                 _start
> > 
> > Enabling --inline does not change the output, even though hypot is inlined
> > into main.
> > 
> > Looking at your code, it looks to me as if you are only trying to find
> > inliners for root entries, cf. hist_entry__fprintf (stdio/hist.c), and
> > only
> > when unwinding failed (?, cf. (callchain_ret == 0)).
> > 
> > Is this understanding correct, or should your patch also add inliners for
> > nodes in the middle of the callstack?
> > 
> > This approach makes the whole feature unusable for my use case. In my
> > (really hacky) approach to adding inliners to the output of perf
> > (https://github.com/
> > milianw/linux/commit/71d031c9d679bfb4a4044226e8903dd80ea601b3) I decided
> > to lookup inliners for _every_ callchain entry. The result then there is
> > with my> 
> > branch and a `perf report -g srcline --stdio`:
> >      42.50%  lab_mandelbrot   libm-2.24.so                [.]
> >      __hypot_finite
> >      
> >              ---__hypot_finite +387
> >              
> >                 hypot +20
> >                 std::__complex_abs complex:589
> >                 std::abs<double> complex:597
> >                 drawMandelbrot mandelbrot.h:39
> >                 main main.cpp:55
> >                 __libc_start_main +241
> >                 _start +42
> > 
> > Note the three inliner entries in the middle of the callstack. The way I
> > did it, one will also see "proper" entries for inlined functions in a
> > top-down `perf report --children` output.
> > 
> > In general, your patch series looks much more polished than mine, so I
> > would love to get it integrated. But with your approach, we will need
> > lots of changes to the various browsers to get all features in. My hacky
> > approach has the nice property that it simply adds "normal" callchain
> > entries for inlined nodes - maybe you should do the same?
> > 
> > Thanks a lot for your work, much appreciated.


-- 
Milian Wolff | milian.wolff@kdab.com | Software Engineer
KDAB (Deutschland) GmbH&Co KG, a KDAB Group company
Tel: +49-30-521325470
KDAB - The Qt Experts

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5903 bytes --]