Re: [PATCHSET 0/3] perf diff: Introduce delta-abs compute method

[Namhyung Kim <namhyung@kernel.org>]

Hi Arnaldo,

On Mon, Feb 06, 2017 at 09:51:49AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Feb 06, 2017 at 04:20:34PM +0900, Namhyung Kim escreveu:
> > Hello,
> > 
> > This patchset adds 'delta-abs' compute method to -c/--compute option.
> > The 'delta-abs' is same as 'delta' but shows entries with bigger
> > absolute delta first instead of sorting numerically.  This is only
> > useful together with -o option.
> > 
> > Below is default output (-c delta):
> > 
> >   $ perf diff -o 1 -c delta | grep -v ^# | head
> >     42.22%   +4.97%  [kernel.kallsyms]  [k] cfb_imageblit
> >      0.62%   +1.23%  [kernel.kallsyms]  [k] mutex_lock
> >              +1.15%  [kernel.kallsyms]  [k] copy_user_generic_string
> >      2.40%   +0.95%  [kernel.kallsyms]  [k] bit_putcs
> >      0.31%   +0.79%  [kernel.kallsyms]  [k] link_path_walk
> >              +0.64%  [kernel.kallsyms]  [k] kmem_cache_alloc
> >      0.00%   +0.57%  [kernel.kallsyms]  [k] __rcu_read_unlock
> >              +0.45%  [kernel.kallsyms]  [k] alloc_set_pte
> >      0.16%   +0.45%  [kernel.kallsyms]  [k] menu_select
> >              +0.41%  ld-2.24.so         [.] do_lookup_x
> > 
> > Now with 'delta-abs' it shows entries have bigger delta value either
> > positive or negative.
> > 
> >   $ perf diff -o 1 -c delta-abs | grep -v ^# | head
> >     42.22%   +4.97%  [kernel.kallsyms]  [k] cfb_imageblit
> >     12.72%   -3.01%  [kernel.kallsyms]  [k] intel_idle
> >      9.72%   -1.31%  [unknown]          [.] 0x0000000000411343
> >      0.62%   +1.23%  [kernel.kallsyms]  [k] mutex_lock
> >              +1.15%  [kernel.kallsyms]  [k] copy_user_generic_string
> >      2.40%   +0.95%  [kernel.kallsyms]  [k] bit_putcs
> >      0.31%   +0.79%  [kernel.kallsyms]  [k] link_path_walk
> >      1.35%   -0.71%  [kernel.kallsyms]  [k] smp_call_function_single
> >              +0.64%  [kernel.kallsyms]  [k] kmem_cache_alloc
> >      0.00%   +0.57%  [kernel.kallsyms]  [k] __rcu_read_unlock
> > 
> > The patch 2 and 3 are to add config options to control the default
> > behavior of perf diff command.  I think that it's worth consider
> > changing the default to use 'delta-abs' method since users want to see
> > where the difference occurs actually (either positive or negative) IMHO.
> 
> I agree on having the default changed to 'delta-abs', Ingo?

Good.  Also, as I said in the changelog, it needs to change default
value of -o option to 1 in order to make the 'delta-abs' effective.

> 
> Namhyung, and perhaps we should have a single letter option to do that
> '| grep -v ^#' bit :-) and perhaps we also should have, for all tools
> the equivalent of that "| head", that git log has:
> 
> [acme@jouet linux]$ git log --oneline -5
> d7cb3a507d23 Merge tag 'perf-core-for-mingo-4.11-20170201' of git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux into perf/core
> 5443624bedd0 perf/x86/intel/pt: Add format strings for PTWRITE and power event tracing
> b05d1093987a perf ftrace: Add ftrace.tracer config option
> 43d41deb71fe perf tools: Create for_each_event macro for tracepoints iteration
> a26305363d4b perf test: Add libbpf pinning test
> [acme@jouet linux]$
> 
> That '-5' to show just the first 5 lines worth of output.
> 
> With all that we would have:
> 
>   perf diff -o 1 -q10
> 
> As the equivalent to "perf diff -o 1 -c delta-abs | grep -v ^# | head".

The -q/--quiet looks ok since it corresponds to -v/--verbose option.
But I'm not sure about the number option.

In case of git, it'll stop processing commits after the given number
of them, so it will reduce significant processing time IMHO.  However,
in perf, we need to process whole data anyway and sort at the final
stage, and then stop displaying entries after the given number.

Maybe it's just a shortcut of piping to the head command.  Then I
don't feel the strong desire to have it as we have pager, TUI and GUI
already.

Thanks,
Namhyung


> 
> Ah, adding Paul McKenney to the CC list, he may have something to add
> here.
> 
> - Arnaldo
>  
> > The code is avaiable at 'perf/diff-delta-abs-v1' branch in
> > 
> >   git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
> > 
> > Thanks,
> > Namhyung
> > 
> > 
> > Namhyung Kim (3):
> >   perf diff: Add 'delta-abs' compute method
> >   perf diff: Add diff.order config option
> >   perf diff: Add diff.compute config option
> > 
> >  tools/perf/Documentation/perf-config.txt | 12 +++++
> >  tools/perf/Documentation/perf-diff.txt   | 15 +++++--
> >  tools/perf/builtin-diff.c                | 76 ++++++++++++++++++++++++++++++--
> >  3 files changed, 97 insertions(+), 6 deletions(-)
> > 
> > -- 
> > 2.11.0