Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Thu, Jul 20, 2017 at 06:36:55AM +0900, Taeung Song escreveu:
> Currently the --show-total-period option of perf-annotate
> is different from perf-report's.
> 
> For example, perf-report ordinarily shows period and number of samples.
> 
>  # Overhead       Samples        Period  Command  Shared Object   Symbol
>  # ........  ............  ............  .......  ..............  ............
>  #
>       9.83%           102      84813704  test     test            [.] knapsack

But this is not what this patch does, it is still doing too many things,
it should first add sample to those function signatures, leaving the
"meat" to a followup patch, where we will not be distracted with
infrastructure needed to do what you describe in the changelog.

I'm doing it here this time, please look at the result, later.

- Arnaldo
 
> But --show-total-period of perf-annotate has the problem that show
> number of samples, not period.
> So fix this option to show period instead of number of samples.
> 
> Before:
> 
>   Percent |      Source code & Disassembly of old for cycles:ppp (102 samples)
>  -----------------------------------------------------------------------------
>           :
>           :
>           :
>           :      Disassembly of section .text:
>           :
>           :      0000000000400816 <knapsack>:
>           :      knapsack():
>         1 :        400816:       push   %rbp
> 
> After:
> 
>   Percent |  Source code & Disassembly of old for cycles:ppp (84813704 event count)
>  ----------------------------------------------------------------------------------
>           :
>           :
>           :
>           :  Disassembly of section .text:
>           :
>           :  0000000000400816 <knapsack>:
>           :  knapsack():
>    743737 :    400816:       push   %rbp
> 
> Reported-by: Namhyung Kim <namhyung@kernel.org>
> Cc: Milian Wolff <milian.wolff@kdab.com>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
> ---
>  tools/perf/builtin-annotate.c |  4 +---
>  tools/perf/builtin-report.c   | 13 ++++++----
>  tools/perf/builtin-top.c      |  6 +++--
>  tools/perf/util/annotate.c    | 55 ++++++++++++++++++++++++++++++++++++-------
>  tools/perf/util/annotate.h    |  4 +++-
>  5 files changed, 63 insertions(+), 19 deletions(-)
> 
> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> index 5205408..0381408 100644
> --- a/tools/perf/builtin-annotate.c
> +++ b/tools/perf/builtin-annotate.c
> @@ -177,14 +177,12 @@ static int perf_evsel__add_sample(struct perf_evsel *evsel,
>  	 */
>  	process_branch_stack(sample->branch_stack, al, sample);
>  
> -	sample->period = 1;
>  	sample->weight = 1;
> -
>  	he = hists__add_entry(hists, al, NULL, NULL, NULL, sample, true);
>  	if (he == NULL)
>  		return -ENOMEM;
>  
> -	ret = hist_entry__inc_addr_samples(he, evsel->idx, al->addr);
> +	ret = hist_entry__inc_addr_samples(he, sample, evsel->idx, al->addr);
>  	hists__inc_nr_samples(hists, true);
>  	return ret;
>  }
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index cea25d0..a9bd011 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -115,13 +115,14 @@ static int hist_iter__report_callback(struct hist_entry_iter *iter,
>  	struct report *rep = arg;
>  	struct hist_entry *he = iter->he;
>  	struct perf_evsel *evsel = iter->evsel;
> +	struct perf_sample *sample = iter->sample;
>  	struct mem_info *mi;
>  	struct branch_info *bi;
>  
>  	if (!ui__has_annotation())
>  		return 0;
>  
> -	hist__account_cycles(iter->sample->branch_stack, al, iter->sample,
> +	hist__account_cycles(sample->branch_stack, al, sample,
>  			     rep->nonany_branch_mode);
>  
>  	if (sort__mode == SORT_MODE__BRANCH) {
> @@ -138,14 +139,16 @@ static int hist_iter__report_callback(struct hist_entry_iter *iter,
>  		if (err)
>  			goto out;
>  
> -		err = hist_entry__inc_addr_samples(he, evsel->idx, al->addr);
> +		err = hist_entry__inc_addr_samples(he, sample,
> +						   evsel->idx, al->addr);
>  
>  	} else if (symbol_conf.cumulate_callchain) {
>  		if (single)
> -			err = hist_entry__inc_addr_samples(he, evsel->idx,
> -							   al->addr);
> +			err = hist_entry__inc_addr_samples(he, sample,
> +							   evsel->idx, al->addr);
>  	} else {
> -		err = hist_entry__inc_addr_samples(he, evsel->idx, al->addr);
> +		err = hist_entry__inc_addr_samples(he, sample,
> +						   evsel->idx, al->addr);
>  	}
>  
>  out:
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 022486d..09885a4 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -183,6 +183,7 @@ static void ui__warn_map_erange(struct map *map, struct symbol *sym, u64 ip)
>  
>  static void perf_top__record_precise_ip(struct perf_top *top,
>  					struct hist_entry *he,
> +					struct perf_sample *sample,
>  					int counter, u64 ip)
>  {
>  	struct annotation *notes;
> @@ -199,7 +200,7 @@ static void perf_top__record_precise_ip(struct perf_top *top,
>  	if (pthread_mutex_trylock(&notes->lock))
>  		return;
>  
> -	err = hist_entry__inc_addr_samples(he, counter, ip);
> +	err = hist_entry__inc_addr_samples(he, sample, counter, ip);
>  
>  	pthread_mutex_unlock(&notes->lock);
>  
> @@ -671,7 +672,8 @@ static int hist_iter__top_callback(struct hist_entry_iter *iter,
>  	struct perf_evsel *evsel = iter->evsel;
>  
>  	if (perf_hpp_list.sym && single)
> -		perf_top__record_precise_ip(top, he, evsel->idx, al->addr);
> +		perf_top__record_precise_ip(top, he, iter->sample,
> +					    evsel->idx, al->addr);
>  
>  	hist__account_cycles(iter->sample->branch_stack, al, iter->sample,
>  		     !(top->record_opts.branch_stack & PERF_SAMPLE_BRANCH_ANY));
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index a62067a..d4f1a0a 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -816,9 +816,33 @@ int addr_map_symbol__inc_samples(struct addr_map_symbol *ams, int evidx)
>  	return symbol__inc_addr_samples(ams->sym, ams->map, evidx, ams->al_addr);
>  }
>  
> -int hist_entry__inc_addr_samples(struct hist_entry *he, int evidx, u64 ip)
> +int hist_entry__inc_addr_samples(struct hist_entry *he, struct perf_sample *sample,
> +				 int evidx, u64 addr)
>  {
> -	return symbol__inc_addr_samples(he->ms.sym, he->ms.map, evidx, ip);
> +	struct symbol *sym = he->ms.sym;
> +	struct annotation *notes;
> +	struct sym_hist *h;
> +	s64 offset;
> +
> +	if (sym == NULL)
> +		return 0;
> +
> +	notes = symbol__get_annotation(sym, false);
> +	if (notes == NULL)
> +		return -ENOMEM;
> +
> +	if ((addr < sym->start || addr >= sym->end) &&
> +	    (addr != sym->end || sym->start != sym->end))
> +		return -ERANGE;
> +
> +	offset = addr - sym->start;
> +	h = annotation__histogram(notes, evidx);
> +	h->total_samples++;
> +	h->addr[offset].nr_samples++;
> +	h->total_period += sample->period;
> +	h->addr[offset].period += sample->period;
> +
> +	return 0;
>  }
>  
>  static void disasm_line__init_ins(struct disasm_line *dl, struct arch *arch, struct map *map)
> @@ -934,6 +958,7 @@ double disasm__calc_percent(struct annotation *notes, int evidx, s64 offset,
>  	double percent = 0.0;
>  
>  	sample->nr_samples = 0;
> +	sample->period = 0;
>  
>  	if (src_line) {
>  		size_t sizeof_src_line = sizeof(*src_line) +
> @@ -953,12 +978,16 @@ double disasm__calc_percent(struct annotation *notes, int evidx, s64 offset,
>  	} else {
>  		struct sym_hist *h = annotation__histogram(notes, evidx);
>  		unsigned int hits = 0;
> +		unsigned int p = 0;
>  
> -		while (offset < end)
> -			hits += h->addr[offset++].nr_samples;
> +		while (offset < end) {
> +			hits += h->addr[offset].nr_samples;
> +			p += h->addr[offset++].period;
> +		}
>  
>  		if (h->total_samples) {
>  			sample->nr_samples = hits;
> +			sample->period = p;
>  			percent = 100.0 * hits / h->total_samples;
>  		}
>  	}
> @@ -1131,8 +1160,8 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st
>  			color = get_percent_color(percent);
>  
>  			if (symbol_conf.show_total_period)
> -				color_fprintf(stdout, color, " %7" PRIu64,
> -					      sample.nr_samples);
> +				color_fprintf(stdout, color, " %11" PRIu64,
> +					      sample.period);
>  			else
>  				color_fprintf(stdout, color, " %7.2f", percent);
>  		}
> @@ -1162,6 +1191,10 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st
>  		if (perf_evsel__is_group_event(evsel))
>  			width *= evsel->nr_members;
>  
> +		if (symbol_conf.show_total_period)
> +			width += perf_evsel__is_group_event(evsel) ?
> +				4 * evsel->nr_members : 4;
> +
>  		if (!*dl->line)
>  			printf(" %*s:\n", width, " ");
>  		else
> @@ -1812,8 +1845,14 @@ int symbol__annotate_printf(struct symbol *sym, struct map *map,
>  	if (perf_evsel__is_group_event(evsel))
>  		width *= evsel->nr_members;
>  
> -	graph_dotted_len = printf(" %-*.*s|	Source code & Disassembly of %s for %s (%" PRIu64 " samples)\n",
> -	       width, width, "Percent", d_filename, evsel_name, h->total_samples);
> +	if (symbol_conf.show_total_period)
> +		width += perf_evsel__is_group_event(evsel) ?
> +			4 * evsel->nr_members : 4;
> +
> +	graph_dotted_len = printf(" %-*.*s|	Source code & Disassembly of %s for %s (%" PRIu64 " %s)\n",
> +				  width, width, "Percent", d_filename, evsel_name,
> +				  symbol_conf.show_total_period ? h->total_period : h->total_samples,
> +				  symbol_conf.show_total_period ? "event count" : "samples");
>  
>  	printf("%-*.*s----\n",
>  	       graph_dotted_len, graph_dotted_len, graph_dotted_line);
> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> index bef1d02..4406352 100644
> --- a/tools/perf/util/annotate.h
> +++ b/tools/perf/util/annotate.h
> @@ -88,6 +88,7 @@ double disasm__calc_percent(struct annotation *notes, int evidx, s64 offset,
>  
>  struct sym_hist {
>  	u64		total_samples;
> +	u64		total_period;
>  	struct sym_hist_entry addr[0];
>  };
>  
> @@ -160,7 +161,8 @@ int addr_map_symbol__account_cycles(struct addr_map_symbol *ams,
>  				    struct addr_map_symbol *start,
>  				    unsigned cycles);
>  
> -int hist_entry__inc_addr_samples(struct hist_entry *he, int evidx, u64 addr);
> +int hist_entry__inc_addr_samples(struct hist_entry *he, struct perf_sample *sample,
> +				 int evidx, u64 addr);
>  
>  int symbol__alloc_hist(struct symbol *sym);
>  void symbol__annotate_zero_histograms(struct symbol *sym);
> -- 
> 2.7.4