On 06/18/2015 09:15 PM, Arnaldo Carvalho de Melo wrote: > 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 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 >> 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 >> --- >> 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 >> >> +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(¬es->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 >> > Hello. Thank you for the review, all remarks were reasonable. Feel free to comment next version (v3) of the patch. Thanks, Martin