Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view (v2)

[Namhyung Kim]

Hi Arnaldo,

On Wed, Jan 20, 2016 at 10:32:04AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Jan 20, 2016 at 09:34:51AM +0900, Namhyung Kim escreveu:
> > On Tue, Jan 19, 2016 at 05:59:41PM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Sun, Jan 17, 2016 at 01:03:00AM +0900, Namhyung Kim escreveu:
> > > > Hello,
> > > > 
> > > > This is v2 attempt of my earlier patchset [1].  This patchset
> > > > implements a new feature that collects hist entries in a hierachical
> > > > manner.  That means lower-level entries belong to an upper-level
> > > > entry.  The entry hierachy is built on the sort keys given, so users
> > > > can set it whatever they want.  It only shows top-level entries first,
> > > > and user can expand/collapse it dynamically.
> > > > 
> > > > This time I implemented it for every output browser including TUI.
> > > > A screenshot on TUI looks like below:
> > > > 
> > > > For normal output:
> > > > 
> > > >   $ perf report --tui
> > > >   Samples: 3K of event 'cycles:pp', Event count (approx.): 1695979674
> > > >     Overhead  Command        Shared Object         Symbol
> > > >   ------------------------------------------------------------------------
> > > >   -    7.57%  swapper        [kernel.vmlinux]      [k] intel_idle
> > > >        intel_idle
> > > >        cpuidle_enter_state
> > > >        cpuidle_enter
> > > >        call_cpuidle
> > > >      + cpu_startup_entry
> > > >   +    1.16   firefox        firefox               [.] 0x00000000000019433
> > > >   +    0.97%  firefox        libpthread-2.22.so    [.] pthread_mutex_lock
> > > >   ...
> > > > 
> > > > 
> > > > With hierarchy view,
> > > 
> > > Ok, tested, this is really nice, I think it should be the default, from
> > > where to drill down, we could have a '--no-hierarchy', Ingo?
> > 
> > Yeah, we already have --no-hierarchy (as a side effect of having
> > --hierarchy) but I don't want to change the default now since existing
> > users will complain.  Now we have 'tips' in the perf report browser,
> > maybe it's enough to add a line to suggest to use it (and it's already
> > done by this patchset).  I remember the time we changed default for
> > '--children' and many people complained about it.
> 
> :-) Ok, what about an extra patch that, when alt+H is pressed (we're
> running out of appropriate hotkeys) we toggle to --hierarchy view, press
> it again, back to --no-hierarchy?

I guess it'd require non-trivial changes to the patchset and add some
amount of complexity.  I'll check whether it can be done easily later..


>  
> > We maybe change the default later but I think it's better to have some
> > time to people can play with it and find it useful. :)  And, as always,
> > we can have a config option to control the default.
> > 
> > Btw, do you think it's worth adding a short option (-H) for
> > --hierarchy (instead of making it default)?
> 
> I don't think this will remain for long as something we would want to
> use in the command line, i.e. that we would have about half the number
> of users wanting one hierarchy while the others wouldn't, for such cases
> probably the best is to keep it as just a long option?
> 
> I.e. either the user uses --hierarchy and finds it so useful that he
> will want to have it as his default or dislikes it and stops passing
> that option in the command line, end result is that it will be a one
> time use of such option, no need to have the one letter option used in
> this case.

Some people might find it useful sometimes (or only for specific
workloads), but don't want make it default.  If they want to use it
from time to time, maybe worth adding a short option.


> 
> One thing I just noticed was that right after I ran:
> 
>  $ perf report --hie
> 
> The tool suggests that I use the brand new '--hierarchy' option :-)
> 
> How hard would it be to provide a way to disable some of the suggestions
> when what is randomly suggested is already what the user is doing?

We could add callbacks for each tip to check it.

Thanks,
Namhyung