Re: [PATCH 4/7] perf script: Add --inline option

[Ingo Molnar]

* Milian Wolff <milian.wolff@xxxxxxxx> wrote:

> On Wednesday, May 24, 2017 9:21:42 AM CEST Ingo Molnar wrote:
> > * Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
> > > On Wed, May 24, 2017 at 08:38:11AM +0200, Ingo Molnar wrote:
> > > > * Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
> > > > > The --inline option is to show inlined functions in callchains.
> > > > > 
> > > > > For example,
> > > > > 
> > > > >   $ perf script
> > > > >   
> > > > >   a.out  5644 11611.467597:     309961 cycles:u:
> > > > >                      790 main (/home/namhyung/tmp/perf/a.out)
> > > > >                    
> > > > >                    20511 __libc_start_main (/usr/lib/libc-2.25.so)
> > > > >                    
> > > > >                      8ba _start (/home/namhyung/tmp/perf/a.out)
> > > > >   
> > > > >   ...
> > > > >   
> > > > >   $ perf script --inline
> > > > >   
> > > > >   a.out  5644 11611.467597:     309961 cycles:u:
> > > > >                      790 main (/home/namhyung/tmp/perf/a.out)
> > > > >                      
> > > > >                          std::__detail::_Adaptor<std::linear_congruent
> > > > >                          ial_engine<unsigned long, 16807ul, 0ul,
> > > > >                          2147483647ul>, double>::operator()
> > > > >                          std::uniform_real_distribution<double>::oper
> > > > >                          ator()<std::linear_congruential_engine<unsign
> > > > >                          ed long, 16807ul, 0ul, 2147483647ul> >
> > > > >                          std::uniform_real_distribution<double>::oper
> > > > >                          ator()<std::linear_congruential_engine<unsign
> > > > >                          ed long, 16807ul, 0ul, 2147483647ul> > main
> > > > >                    
> > > > >                    20511 __libc_start_main (/usr/lib/libc-2.25.so)
> > > > >                    
> > > > >                      8ba _start (/home/namhyung/tmp/perf/a.out)
> > > > >   
> > > > >   ...
> > > > 
> > > > Shouldn't this be the default behavior, to make call chains more
> > > > readable?
> > > 
> > > AFAIK perf report didn't make it default due to a performance impact,
> > > but I didn't know how much it is.  Especially if perf was not built
> > > with libbfd it'll run external addr2line to get inlined functions for
> > > each callchain entry..
> > 
> > So then at least let's make it the default when all libraries are present.
> > Not enabling something when the build is not 'complete' is fair game -
> > distros will typically have all the libraries available.
> > 
> > We need to remember that roughly 99% of all our users will use as few perf
> > command line options as they can get away with - myself included. Adding a
> > non-debugging feature as a non-default command line option is really as if
> > we didn't do anything: very few if any people will use it, and it might
> > bitrot in the future without people noticing.
> > 
> > So we need apply some thought into making it available to two orders of
> > magnitude more people! If someone types 'perf report' we should give the
> > best selection of all the features we have available.
> 
> Just a suggestion: My larger patch set that is in review now adds some caching 
> features which already speeds up the whole process considerably. As such, my 
> suggestion is to wait for this patch set to be integrated. Then we could 
> enable --inline unconditionally, or at least only when libbfd is available.

I'm fine with that - and please make the default-enabling part of your patch 
series, so it does not get forgotten.

Thanks,

	Ingo