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

From: Arnaldo Carvalho de Melo
Date: Thu Jun 18 2015 - 15:15:53 EST

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@xxxxxxx> 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@xxxxxxx>
> 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@xxxxxxx>
> ---
> 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
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/