Re: RFC: callchain showing same entry as hist_entry

[Arnaldo Carvalho de Melo]

Em Tue, Aug 16, 2016 at 11:55:11PM +0900, Namhyung Kim escreveu:
> Hi Arnaldo,
> 
> On Tue, Aug 16, 2016 at 11:36:28AM -0300, Arnaldo Carvalho de Melo wrote:
> > Hi Namhyung, Jiri,
> > 
> > 	please take a look at the patch below, and Ack if possible, it
> > is a problem introduced in:
> > 
> >   cfaa154b2335 ("perf tools: Get rid of obsolete hist_entry__sort_list")
> > 
> > That is not equivalent to the code that was there and results in having
> > the same entry as the first entry for the callchain as in the
> > hist_entry, which is annoying and doesn't match the original intent of
> > that 'continue' branch, as described in the comment right above it.
> 
> AFAIK the intent was to skip first callchain entry iff the first sort
> key is 'symbol'.  The sort_order being NULL means it'd use the default
> sort key which is 'comm,dso,sym' so it should not skip the first
> callchain entry.
> 
> The original code (before cfaa154b2335) was like below:
> 
> -                       if (!i++ && sort__first_dimension == SORT_SYM)
> +                       if (!i++ && field_order == NULL &&
> +                           sort_order && !prefixcmp(sort_order, "sym"))
> 
> I think the current code works as intended, no?

Well, see below the cset comment (local one, will change as we discuss),
may clarify further, but yeah, we can't take for granted that the
default sort order has "sym" in it :-\

So the original intent description was ambiguous, what we want to avoid
is what is in the cset comment below, so the logic for that should be a
bit different, tho.

For starters we need to have a callchain_something__first() that will
skip the first one using this logic and then use it in all places in
--stdio, --tui, --gtk where we need this.

commit c066eaa8d06b87c77b82d55bbc59be308aca4684
Author: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
Date:   Tue Aug 16 11:36:50 2016 -0300

    perf callchain: Do not repeat the hist_entry symbol as the 1st callchain entry
    
    We were getting:
    
         2.62%  [k] __d_lookup_rcu
                |
                ---__d_lookup_rcu
                   |
                    --2.52%--lookup_fast
                              |
                               --2.50%--walk_component
    
    Noticed the __d_lookup_rcu dup, when the original coder intent, and the saner
    output is:
    
         2.62%  [k] __d_lookup_rcu
                |
                 --2.52%--lookup_fast
                           |
                            --2.50%--walk_component
    
    This is for --stdio, TUI being investigated.
    
    Cc: Adrian Hunter <adrian.hunter@xxxxxxxxx>
    Cc: David Ahern <dsahern@xxxxxxxxx>
    Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
    Cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
    Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
    Cc: Wang Nan <wangnan0@xxxxxxxxxx>
    Fixes: cfaa154b2335 ("perf tools: Get rid of obsolete hist_entry__sort_list")
    Link: http://lkml.kernel.org/r/20160816143628.GG20972@xxxxxxxxxx
    Signed-off-by: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>

diff --git a/tools/perf/ui/stdio/hist.c b/tools/perf/ui/stdio/hist.c
index 9b65f4a6b35a..46a083e59ce9 100644
--- a/tools/perf/ui/stdio/hist.c
+++ b/tools/perf/ui/stdio/hist.c
@@ -207,7 +207,7 @@ static size_t callchain__fprintf_graph(FILE *fp, struct rb_root *root,
 			 * displayed twice.
 			 */
 			if (!i++ && field_order == NULL &&
-			    sort_order && !prefixcmp(sort_order, "sym"))
+			    (sort_order == NULL || !prefixcmp(sort_order, "sym")))
 				continue;
 			if (!printed) {
 				ret += callchain__fprintf_left_margin(fp, left_margin);