RFC: callchain showing same entry as hist_entry

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.

Now looking at doing the same for the TUI...

- Arnaldo

tools/perf/ui/stdio/hist.c +1 -1

index 9b65f4a6b35a..46a083e59ce9 100644
@@ -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);

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.

Now looking at doing the same for the TUI...

- Arnaldo

tools/perf/ui/stdio/hist.c +1 -1

index 9b65f4a6b35a..46a083e59ce9 100644
@@ -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);

Hi Arnaldo,

Quoted 11 lines · Arnaldo Carvalho de Melo

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:

(quoted diff) +2 -1

-                       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?

Em Tue, Aug 16, 2016 at 11:55:11PM +0900, Namhyung Kim escreveu:

Quoted 26 lines

Hi Arnaldo,

Quoted 11 lines · Arnaldo Carvalho de Melo

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:

(quoted diff) +2 -1

-                       if (!i++ && sort__first_dimension == SORT_SYM)
+                       if (!i++ && field_order == NULL &&
+                           sort_order && !prefixcmp(sort_order, "sym"))

Quoted 1 line

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 <[email protected]>
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 <[email protected]>

Cc David Ahern <[email protected]>

Cc Jiri Olsa <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Wang Nan <[email protected]>

Fixes cfaa154b2335 ("perf tools: Get rid of obsolete hist_entry__sort_list")

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

tools/perf/ui/stdio/hist.c +1 -1

index 9b65f4a6b35a..46a083e59ce9 100644
@@ -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);

Quoted 63 lines

Em Tue, Aug 16, 2016 at 11:55:11PM +0900, Namhyung Kim escreveu:

Quoted 23 lines · Arnaldo Carvalho de Melo

Quoted 9 lines

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:

(quoted diff) +2 -1

-                       if (!i++ && sort__first_dimension == SORT_SYM)
+                       if (!i++ && field_order == NULL &&
+                           sort_order && !prefixcmp(sort_order, "sym"))

Quoted 1 line

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 <[email protected]>
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

Quoted 1 line

This is for --stdio, TUI being investigated.

Did you run 'perf report -s sym --stdio'? It seems that current code already works as sane.. Do you want to make it work for the default sort

Quoted 1 line

key too?

Em Wed, Aug 17, 2016 at 12:23:55AM +0900, Namhyung Kim escreveu:

Quoted 3 lines

Did you run 'perf report -s sym --stdio'? It seems that current code already works as sane.. Do you want to make it work for the default sort

Quoted 1 line

key too?

What I want is not to repeat the hist_entry line as the first entry in the callchain, which is confusing.

I.e. repeat the duplication that is happening with the default sort

entry, that contains "sym".

- Arnaldo

Quoted 11 lines

Em Tue, Aug 16, 2016 at 12:31:43PM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 9 lines

Em Wed, Aug 17, 2016 at 12:23:55AM +0900, Namhyung Kim escreveu:

Quoted 3 lines

Did you run 'perf report -s sym --stdio'? It seems that current code already works as sane.. Do you want to make it work for the default sort

Quoted 1 line

key too?

What I want is not to repeat the hist_entry line as the first entry in the callchain, which is confusing.

I.e. repeat the duplication that is happening with the default sort

Quoted 3 lines

s/repeat/remove/g

Quoted 1 line

entry, that contains "sym".

Hmm.. if so, wouldn't it be better skipping the first callchain entry when the user-given sort key contains "sym" too (not only when it starts with 'sym')?

Em Tue, Aug 16, 2016 at 12:31:43PM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 3 lines

Em Wed, Aug 17, 2016 at 12:23:55AM +0900, Namhyung Kim escreveu:

Quoted 2 lines

Did you run 'perf report -s sym --stdio'? It seems that current code already works as sane.. Do you want to make it work for the default sort

Quoted 6 lines

Quoted 1 line

Quoted 1 line

key too?

What I want is not to repeat the hist_entry line as the first entry in the callchain, which is confusing.

I.e. repeat the duplication that is happening with the default sort

s/repeat/remove/g

Quoted 1 line

entry, that contains "sym".

Em Wed, Aug 17, 2016 at 12:41:18AM +0900, Namhyung Kim escreveu:

Quoted 3 lines

Hmm.. if so, wouldn't it be better skipping the first callchain entry when the user-given sort key contains "sym" too (not only when it starts with 'sym')?

Probably, I think, whatever causes the mentioned duplication. And do that at all the UIs, hence the suggestion for a callchain__first_node() or more suitably named routine where such logic would live, to be used for all the callchain rendering interfaces.

Em Tue, Aug 16, 2016 at 12:46:57PM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 9 lines

Em Wed, Aug 17, 2016 at 12:41:18AM +0900, Namhyung Kim escreveu:

Quoted 3 lines

Hmm.. if so, wouldn't it be better skipping the first callchain entry when the user-given sort key contains "sym" too (not only when it starts with 'sym')?

Probably, I think, whatever causes the mentioned duplication. And do that at all the UIs, hence the suggestion for a callchain__first_node() or more suitably named routine where such logic would live, to be used for all the callchain rendering interfaces.

So, it is not possible to have that callchain__first_node() one, since we're dealing with one of the "list" entries, that are in each "node", etc, we need to do that test when printing the first entry, how about this one instead?