Re: [PATCH V4 3/3] perf tool: Add sort key symoff for perf diff

[Namhyung Kim <namhyung@kernel.org>]

Hi Kan,

On Wed, 19 Nov 2014 14:17:33 +0000, Kan Liang wrote:
>> 
>> On Tue, 18 Nov 2014 11:38:20 -0500, kan liang wrote:
>> > From: Kan Liang <kan.liang@intel.com>
>> >
>> > Sometime, especially debugging scaling issue, the function level diff
>> > may be high granularity. The user may want to do deeper diff analysis
>> > for some cache or lock issue. The "symoff" key can let the user sort
>> > differential profile by ips. This feature should only work when the
>> > perf.data comes from same binary.
>> 
>> I think the symoff sort key now works well for different (i.e. modified)
>> binaries too.
>
> For different binaries, the function may be changed. So the offset may
> point to different code. 
> What about this?
> "This feature should work when the perf.data comes from
> either same binary or same function of different binary."
> Or just simply remove this sentence.

I'd like to remove it. :)

>
>> 
>> >
>> >  -s::
>> >  --sort=::
>> > -	Sort by key(s): pid, comm, dso, symbol, cpu, parent, srcline.
>> > -	Please see description of --sort in the perf-report man page.
>> > +	Sort by key(s): pid, comm, dso, symbol, cpu, parent, srcline, symoff.
>> > +
>> > +	- symoff: exact symbol +  offset address executed at the time of
>> sample.
>> > +	(for same binary compare)
>> 
>> Ditto.  And symoff is not only for perf diff, but it should work for normal
>> perf report also.  So you'd better move the description to perf report man
>> page IMHO.
>
> OK, I will do it, and also remove "(for same binary compare)"

Thanks!

>
>> 
>> 
>> > +
>> > +	For other keys, please see description of --sort in the perf-report
>> man page.
>> >
>> >  -t::
>> >  --field-separator=::
>> > diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
>> > index 1ce425d..03a4001 100644
>> > --- a/tools/perf/builtin-diff.c
>> > +++ b/tools/perf/builtin-diff.c
>> > @@ -744,7 +744,7 @@ static const struct option options[] = {
>> >  	OPT_STRING('S', "symbols", &symbol_conf.sym_list_str,
>> "symbol[,symbol...]",
>> >  		   "only consider these symbols"),
>> >  	OPT_STRING('s', "sort", &sort_order, "key[,key2...]",
>> > -		   "sort by key(s): pid, comm, dso, symbol, parent, cpu,
>> srcline, ..."
>> > +		   "sort by key(s): pid, comm, dso, symbol, parent, cpu,
>> srcline, symoff, ..."
>> >  		   " Please refer the man page for the complete list."),
>> >  	OPT_STRING('t', "field-separator", &symbol_conf.field_sep,
>> "separator",
>> >  		   "separator for columns, no spaces will be added between
>> "
>> > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c index
>> > 6e88b9e..3d8a71b 100644
>> > --- a/tools/perf/util/hist.c
>> > +++ b/tools/perf/util/hist.c
>> > @@ -67,13 +67,14 @@ void hists__calc_col_len(struct hists *hists, struct
>> hist_entry *h)
>> >  		symlen = h->ms.sym->namelen + 4;
>> >  		if (verbose)
>> >  			symlen += BITS_PER_LONG / 4 + 2 + 3;
>> > -		hists__new_col_len(hists, HISTC_SYMBOL, symlen);
>> >  	} else {
>> >  		symlen = unresolved_col_width + 4 + 2;
>> > -		hists__new_col_len(hists, HISTC_SYMBOL, symlen);
>> >  		hists__set_unres_dso_col_len(hists, HISTC_DSO);
>> >  	}
>> >
>> > +	hists__new_col_len(hists, HISTC_SYMBOL, symlen);
>> > +	hists__new_col_len(hists, HISTC_SYMOFF, symlen);
>> 
>> SYMOFF will need larger length at least 6 (for "+0xYYY").  Idealy 3 + symbol
>> size in hexdigit?
>> 
>
> We also need to handle the case which doesn't have symbol available.
> What about this?

I'm fine with it (but I believe you'll fix the indentation).

Thanks,
Namhyung

>         /*
> +      * +6 accounts for '"+0xYYY ' symoff info
>          * +4 accounts for '[x] ' priv level info
>          * +2 accounts for 0x prefix on raw addresses
>          * +3 accounts for ' y ' symtab origin info
>          */	
>                  if (h->ms.sym) {
> 		symlen = h->ms.sym->namelen + 4;
> 		if (verbose)
> 			symlen += BITS_PER_LONG / 4 + 2 + 3;
> 		hists__new_col_len(hists, HISTC_SYMBOL, symlen);
>
> +                              symlen = h->ms.sym->namelen + 6
> +	                hists__new_col_len(hists, HISTC_SYMOFF, symlen);
> 	} else {
> 		symlen = unresolved_col_width + 4 + 2;
> 		hists__new_col_len(hists, HISTC_SYMBOL, symlen);
> 		hists__set_unres_dso_col_len(hists, HISTC_DSO);
> +                             symlen = unresolved_col_width + 2
> +                             hists__new_col_len(hists, HISTC_SYMOFF, symlen);
> 	}