Re: [PATCH] perf annotate: With --show-total-period, display total # of samples.

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

Em Thu, Jun 18, 2015 at 08:06:25PM +0200, Martin Liška escreveu:
> On 06/18/2015 06:26 PM, Ingo Molnar wrote:
> > * Martin Liška <mliska@suse.cz> wrote:
> >> +++ b/tools/perf/builtin-annotate.c
> >> +	OPT_BOOLEAN(0, "show-total-period", &symbol_conf.show_total_period,
> >> +		    "Show a column with the sum of periods"),
> >>  	OPT_END()

> > What is the toggle for this in the 'perf report' TUI? (which displays annotated 
> > output too)

> Thanks for note, I fixed that by introducing a new 't' hot key.

> >From 98e1998fcd7481c7e1756e643d66eb85559849ac Mon Sep 17 00:00:00 2001
> From: mliska <mliska@suse.cz>
> Date: Wed, 27 May 2015 10:54:42 +0200
> Subject: [PATCH] perf annotate: With --show-total-period, display total # of
>  samples.
> 
> To compare two records on an instruction base, with --show-total-period
> option provided, display total number of samples that belong to a line
> in assembly language.
> 
> New hot key 't' is introduced for 'perf annotate' TUI.

> Signed-off-by: Martin Liska <mliska@suse.cz>
> ---
>  tools/perf/builtin-annotate.c     |  5 +++-
>  tools/perf/ui/browsers/annotate.c | 60 +++++++++++++++++++++++++++------------
>  tools/perf/util/annotate.c        | 28 ++++++++++++++----
>  tools/perf/util/annotate.h        |  3 +-
>  tools/perf/util/hist.h            |  3 +-
>  5 files changed, 72 insertions(+), 27 deletions(-)
> 
> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> index 4e08c2d..b0c442e 100644
> --- a/tools/perf/builtin-annotate.c
> +++ b/tools/perf/builtin-annotate.c
> @@ -161,7 +161,8 @@ find_next:
>  			/* skip missing symbols */
>  			nd = rb_next(nd);
>  		} else if (use_browser == 1) {
> -			key = hist_entry__tui_annotate(he, evsel, NULL);
> +			key = hist_entry__tui_annotate(he, evsel, NULL,
> +				symbol_conf.show_total_period);

No need to pass this global variable around, since we already have it
like that, why not simply read it in hist_entry__tui_annotate()?

>  			switch (key) {
>  			case -1:
>  				if (!ann->skip_missing)
> @@ -329,6 +330,8 @@ int cmd_annotate(int argc, const char **argv, const char *prefix __maybe_unused)
>  		   "objdump binary to use for disassembly and annotations"),
>  	OPT_BOOLEAN(0, "group", &symbol_conf.event_group,
>  		    "Show event group information together"),
> +	OPT_BOOLEAN(0, "show-total-period", &symbol_conf.show_total_period,
> +		    "Show a column with the sum of periods"),
>  	OPT_END()
>  	};
>  	int ret = hists__init();
> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> index acb0e23..e7cd596 100644
> --- a/tools/perf/ui/browsers/annotate.c
> +++ b/tools/perf/ui/browsers/annotate.c
> @@ -11,16 +11,21 @@
>  #include "../../util/evsel.h"
>  #include <pthread.h>
>  
> +struct disasm_tuple {
> +	double		percent;
> +	double		samples;

Why use double for 'samples'? We use u64 elsewhere for this.

And 'disasm_tuple' is way too vague, how about:

struct disasm_line_samples {
	double		percent;
	u64		nr;
};

and then, below...

> +};
> +
>  struct browser_disasm_line {
> -	struct rb_node	rb_node;
> -	u32		idx;
> -	int		idx_asm;
> -	int		jump_sources;
> +	struct rb_node		rb_node;
> +	u32			idx;
> +	int			idx_asm;
> +	int			jump_sources;
>  	/*
>  	 * actual length of this array is saved on the nr_events field
>  	 * of the struct annotate_browser
>  	 */
> -	double		percent[1];
> +	struct disasm_tuple    tuples[1];

here have:

	struct disasm_line_samples samples;o

Then use bdl->samples[i].nr and bdl->samples[i].percent.

>  };
>  
>  static struct annotate_browser_opt {
> @@ -28,7 +33,8 @@ static struct annotate_browser_opt {
>  	     use_offset,
>  	     jump_arrows,
>  	     show_linenr,
> -	     show_nr_jumps;
> +	     show_nr_jumps,
> +	     show_total_period;
>  } annotate_browser__opts = {
>  	.use_offset	= true,
>  	.jump_arrows	= true,
> @@ -105,15 +111,19 @@ static void annotate_browser__write(struct ui_browser *browser, void *entry, int
>  	char bf[256];
>  
>  	for (i = 0; i < ab->nr_events; i++) {
> -		if (bdl->percent[i] > percent_max)
> -			percent_max = bdl->percent[i];
> +		if (bdl->tuples[i].percent > percent_max)
> +			percent_max = bdl->tuples[i].percent;

When reading this:
`
		if (bdl->samples[i].percent > percent_max)
			percent_max = bdl->samples[i].percent;

It gets clearer what this is about.

>  	}
>  
>  	if (dl->offset != -1 && percent_max != 0.0) {
>  		for (i = 0; i < ab->nr_events; i++) {
> -			ui_browser__set_percent_color(browser, bdl->percent[i],
> +			ui_browser__set_percent_color(browser,
> +						      bdl->tuples[i].percent,
>  						      current_entry);
> -			slsmg_printf("%6.2f ", bdl->percent[i]);
> +			if (annotate_browser__opts.show_total_period)
> +				slsmg_printf("%6.0f ", bdl->tuples[i].samples);
> +			else
> +				slsmg_printf("%6.2f ", bdl->tuples[i].percent);
>  		}
>  	} else {
>  		ui_browser__set_percent_color(browser, 0, current_entry);
> @@ -273,9 +283,9 @@ static int disasm__cmp(struct browser_disasm_line *a,
>  	int i;
>  
>  	for (i = 0; i < nr_pcnt; i++) {
> -		if (a->percent[i] == b->percent[i])
> +		if (a->tuples[i].percent == b->tuples[i].percent)
>  			continue;
> -		return a->percent[i] < b->percent[i];
> +		return a->tuples[i].percent < b->tuples[i].percent;
>  	}
>  	return 0;
>  }
> @@ -366,14 +376,17 @@ static void annotate_browser__calc_percent(struct annotate_browser *browser,
>  		next = disasm__get_next_ip_line(&notes->src->source, pos);
>  
>  		for (i = 0; i < browser->nr_events; i++) {
> -			bpos->percent[i] = disasm__calc_percent(notes,
> +			double samples;
> +
> +			bpos->tuples[i].percent = disasm__calc_percent(notes,
>  						evsel->idx + i,
>  						pos->offset,
>  						next ? next->offset : len,
> -					        &path);
> +						&path, &samples);
> +			bpos->tuples[i].samples = samples;
>  
> -			if (max_percent < bpos->percent[i])
> -				max_percent = bpos->percent[i];
> +			if (max_percent < bpos->tuples[i].percent)
> +				max_percent = bpos->tuples[i].percent;
>  		}
>  
>  		if (max_percent < 0.01) {
> @@ -737,6 +750,7 @@ static int annotate_browser__run(struct annotate_browser *browser,
>  		"n             Search next string\n"
>  		"o             Toggle disassembler output/simplified view\n"
>  		"s             Toggle source code view\n"
> +		"t             Toggle total period view\n"
>  		"/             Search string\n"
>  		"k             Toggle line numbers\n"
>  		"r             Run available scripts\n"
> @@ -812,6 +826,11 @@ show_sup_ins:
>  				ui_helpline__puts("Actions are only available for 'callq', 'retq' & jump instructions.");
>  			}
>  			continue;
> +		case 't':
> +			annotate_browser__opts.show_total_period =
> +			  !annotate_browser__opts.show_total_period;
> +			annotate_browser__update_addr_width(browser);
> +			continue;
>  		case K_LEFT:
>  		case K_ESC:
>  		case 'q':
> @@ -836,12 +855,16 @@ int map_symbol__tui_annotate(struct map_symbol *ms, struct perf_evsel *evsel,
>  }
>  
>  int hist_entry__tui_annotate(struct hist_entry *he, struct perf_evsel *evsel,
> -			     struct hist_browser_timer *hbt)
> +			     struct hist_browser_timer *hbt,
> +			     bool show_total_period)
>  {
>  	/* reset abort key so that it can get Ctrl-C as a key */
>  	SLang_reset_tty();
>  	SLang_init_tty(0, 0, 0);
>  
> +	/* Set default value for show_total_period.  */
> +	annotate_browser__opts.show_total_period = show_total_period;
> +
>  	return map_symbol__tui_annotate(&he->ms, evsel, hbt);
>  }
>  
> @@ -929,7 +952,7 @@ int symbol__tui_annotate(struct symbol *sym, struct map *map,
>  
>  	if (perf_evsel__is_group_event(evsel)) {
>  		nr_pcnt = evsel->nr_members;
> -		sizeof_bdl += sizeof(double) * (nr_pcnt - 1);
> +		sizeof_bdl += sizeof(struct disasm_tuple) * (nr_pcnt - 1);
>  	}
>  
>  	if (symbol__annotate(sym, map, sizeof_bdl) < 0) {
> @@ -1006,6 +1029,7 @@ static struct annotate_config {
>  	ANNOTATE_CFG(show_linenr),
>  	ANNOTATE_CFG(show_nr_jumps),
>  	ANNOTATE_CFG(use_offset),
> +	ANNOTATE_CFG(show_total_period),
>  };
>  
>  #undef ANNOTATE_CFG
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index bf80430..8b94603 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -654,10 +654,11 @@ struct disasm_line *disasm__get_next_ip_line(struct list_head *head, struct disa
>  }
>  
>  double disasm__calc_percent(struct annotation *notes, int evidx, s64 offset,
> -			    s64 end, const char **path)
> +			    s64 end, const char **path, double *samples)
>  {
>  	struct source_line *src_line = notes->src->lines;
>  	double percent = 0.0;
> +	*samples = 0.0;
>  
>  	if (src_line) {
>  		size_t sizeof_src_line = sizeof(*src_line) +
> @@ -671,6 +672,7 @@ double disasm__calc_percent(struct annotation *notes, int evidx, s64 offset,
>  				*path = src_line->path;
>  
>  			percent += src_line->p[evidx].percent;
> +			*samples += src_line->p[evidx].samples;
>  			offset++;
>  		}
>  	} else {
> @@ -680,8 +682,10 @@ double disasm__calc_percent(struct annotation *notes, int evidx, s64 offset,
>  		while (offset < end)
>  			hits += h->addr[offset++];
>  
> -		if (h->sum)
> +		if (h->sum) {
> +			*samples = hits;
>  			percent = 100.0 * hits / h->sum;
> +		}
>  	}
>  
>  	return percent;
> @@ -696,8 +700,9 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st
>  
>  	if (dl->offset != -1) {
>  		const char *path = NULL;
> -		double percent, max_percent = 0.0;
> +		double percent, samples, max_percent = 0.0;
>  		double *ppercents = &percent;
> +		double *psamples = &samples;
>  		int i, nr_percent = 1;
>  		const char *color;
>  		struct annotation *notes = symbol__annotation(sym);
> @@ -710,8 +715,10 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st
>  		if (perf_evsel__is_group_event(evsel)) {
>  			nr_percent = evsel->nr_members;
>  			ppercents = calloc(nr_percent, sizeof(double));
> -			if (ppercents == NULL)
> +			psamples = calloc(nr_percent, sizeof(double));
> +			if (ppercents == NULL || psamples == NULL) {
>  				return -1;
> +			}
>  		}
>  
>  		for (i = 0; i < nr_percent; i++) {
> @@ -719,9 +726,10 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st
>  					notes->src->lines ? i : evsel->idx + i,
>  					offset,
>  					next ? next->offset : (s64) len,
> -					&path);
> +					&path, &samples);
>  
>  			ppercents[i] = percent;
> +			psamples[i] = samples;
>  			if (percent > max_percent)
>  				max_percent = percent;
>  		}
> @@ -759,8 +767,13 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st
>  
>  		for (i = 0; i < nr_percent; i++) {
>  			percent = ppercents[i];
> +			samples = psamples[i];
>  			color = get_percent_color(percent);
> -			color_fprintf(stdout, color, " %7.2f", percent);
> +
> +			if (symbol_conf.show_total_period)
> +				color_fprintf(stdout, color, " %7.0f", samples);
> +			else
> +				color_fprintf(stdout, color, " %7.2f", percent);
>  		}
>  
>  		printf(" :	");
> @@ -770,6 +783,9 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st
>  		if (ppercents != &percent)
>  			free(ppercents);
>  
> +		if (psamples != &samples)
> +			free(psamples);
> +
>  	} else if (max_lines && printed >= max_lines)
>  		return 1;
>  	else {
> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> index cadbdc9..752d1bd 100644
> --- a/tools/perf/util/annotate.h
> +++ b/tools/perf/util/annotate.h
> @@ -72,7 +72,7 @@ struct disasm_line *disasm__get_next_ip_line(struct list_head *head, struct disa
>  int disasm_line__scnprintf(struct disasm_line *dl, char *bf, size_t size, bool raw);
>  size_t disasm__fprintf(struct list_head *head, FILE *fp);
>  double disasm__calc_percent(struct annotation *notes, int evidx, s64 offset,
> -			    s64 end, const char **path);
> +			    s64 end, const char **path, double *samples);

Change samples to u64

>  
>  struct sym_hist {
>  	u64		sum;
> @@ -82,6 +82,7 @@ struct sym_hist {
>  struct source_line_percent {

Please rename source_line_percent to source_line_samples, for
consistency with 'struct disasm_line_samples'

>  	double		percent;
>  	double		percent_sum;
> +	double          samples;

Also change to u64 and rename it to 'nr'.

struct source_line_samples {
	double	percent;
	double	percent_sum;
	u64	nr;
};

>  };
>  
>  struct source_line {
> diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
> index 5ed8d9c..65f953f 100644
> --- a/tools/perf/util/hist.h
> +++ b/tools/perf/util/hist.h
> @@ -306,7 +306,8 @@ int map_symbol__tui_annotate(struct map_symbol *ms, struct perf_evsel *evsel,
>  			     struct hist_browser_timer *hbt);
>  
>  int hist_entry__tui_annotate(struct hist_entry *he, struct perf_evsel *evsel,
> -			     struct hist_browser_timer *hbt);
> +			     struct hist_browser_timer *hbt,
> +			     bool show_total_period);

Why not use symbol_conf.show_total_period, since it already is there for
this?

>  
>  int perf_evlist__tui_browse_hists(struct perf_evlist *evlist, const char *help,
>  				  struct hist_browser_timer *hbt,
> -- 
> 2.1.4
>