Messages in this thread | | | Date | Tue, 16 Aug 2016 12:31:43 -0300 | From | Arnaldo Carvalho de Melo <> | Subject | Re: RFC: callchain showing same entry as hist_entry |
| |
Em Wed, Aug 17, 2016 at 12:23:55AM +0900, Namhyung Kim escreveu: > On Tue, Aug 16, 2016 at 12:08:03PM -0300, Arnaldo Carvalho de Melo wrote: > > Em Tue, Aug 16, 2016 at 11:55:11PM +0900, Namhyung Kim escreveu: > > > > > > On Tue, Aug 16, 2016 at 11:36:28AM -0300, Arnaldo Carvalho de Melo wrote: > > > > 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@redhat.com> > > 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. > > 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 >> 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
| |