Em Wed, Jul 12, 2017 at 07:14:04AM +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 --show-total-period of perf-annotate has two problem like below: > > 1) Wrong column i.e. 'Percent' > 2) Show number of samples, not period > > So fix this option to show period instead of number of samples.
so you have multiple bugs here, please fix one per patch, i.e. if using --show-total-period the header should not be "Percent". Then, you need a patch to introduce that "struct sym_sample" and use it, but please rename it to "struct sym_hist_entry". You can do it and just update the sym_hist_entry->period field, before the change to pass 'struct sample' around. That disasm__calc_percent() function could receive a pointer to a struct sym_hist_entry instead of two pointers. Small, self contained patches that do one thing make reviewing easier, by yourself and others. Please give this a try, if you didn't understand something here I can do it for you, as this needs doing to fix real bugs. Thanks, - Arnaldo > Before: > > Percent | Source code & Disassembly of old for cycles:ppp (102 samples) > ----------------------------------------------------------------------------- > : > : > : > : Disassembly of section .text: > : > : 0000000000400816 <knapsack>: > : knapsack(): > 1 : 400816: push %rbp > > After: > > Event count | Source code & Disassembly of old for cycles:ppp (84813704 > event count) > > -------------------------------------------------------------------------------------- > : > : > : > : Disassembly of section .text: > : > : 0000000000400816 <knapsack>: > : knapsack(): > 743737 : 400816: push %rbp > > Cc: Namhyung Kim <[email protected]> > Cc: Milian Wolff <[email protected]> > Cc: Jiri Olsa <[email protected]> > Signed-off-by: Taeung Song <[email protected]> > --- > tools/perf/builtin-annotate.c | 4 +- > tools/perf/builtin-report.c | 13 +++--- > tools/perf/builtin-top.c | 6 ++- > tools/perf/ui/browsers/annotate.c | 4 +- > tools/perf/ui/gtk/annotate.c | 4 +- > tools/perf/util/annotate.c | 92 > +++++++++++++++++++++++++++++---------- > tools/perf/util/annotate.h | 12 +++-- > 7 files changed, 96 insertions(+), 39 deletions(-) > > diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c > index 7a5dc7e..f314661 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; > - Please do not remove this line. > 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 79a33eb..5f1894c 100644 > --- a/tools/perf/builtin-report.c > +++ b/tools/perf/builtin-report.c > @@ -113,13 +113,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) { > @@ -136,14 +137,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); why break this into two lines? > > } 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(¬es->lock)) > return; > > - err = hist_entry__inc_addr_samples(he, counter, ip); > + err = hist_entry__inc_addr_samples(he, sample, counter, ip); > > pthread_mutex_unlock(¬es->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/ui/browsers/annotate.c > b/tools/perf/ui/browsers/annotate.c > index b376637..73e5ae2 100644 > --- a/tools/perf/ui/browsers/annotate.c > +++ b/tools/perf/ui/browsers/annotate.c > @@ -449,13 +449,13 @@ 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++) { > - u64 nr_samples; > + u64 nr_samples, period; > > bpos->samples[i].percent = disasm__calc_percent(notes, > evsel->idx + i, > pos->offset, > next ? next->offset : len, > - &path, &nr_samples); > + &path, &nr_samples, &period); > bpos->samples[i].nr = nr_samples; > > if (max_percent < bpos->samples[i].percent) > diff --git a/tools/perf/ui/gtk/annotate.c b/tools/perf/ui/gtk/annotate.c > index 87e3760..d736fd5 100644 > --- a/tools/perf/ui/gtk/annotate.c > +++ b/tools/perf/ui/gtk/annotate.c > @@ -34,10 +34,10 @@ static int perf_gtk__get_percent(char *buf, size_t size, > struct symbol *sym, > return 0; > > symhist = annotation__histogram(symbol__annotation(sym), evidx); > - if (!symbol_conf.event_group && !symhist->addr[dl->offset]) > + if (!symbol_conf.event_group && !symhist->addr[dl->offset].nr_samples) > return 0; > > - percent = 100.0 * symhist->addr[dl->offset] / symhist->sum; > + percent = 100.0 * symhist->addr[dl->offset].nr_samples / symhist->sum; > > markup = perf_gtk__get_percent_color(percent); > if (markup) > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c > index ef434b5..f7aeb5f 100644 > --- a/tools/perf/util/annotate.c > +++ b/tools/perf/util/annotate.c > @@ -610,10 +610,10 @@ int symbol__alloc_hist(struct symbol *sym) > size_t sizeof_sym_hist; > > /* Check for overflow when calculating sizeof_sym_hist */ > - if (size > (SIZE_MAX - sizeof(struct sym_hist)) / sizeof(u64)) > + if (size > (SIZE_MAX - sizeof(struct sym_hist)) / sizeof(struct > sym_sample)) > return -1; > > - sizeof_sym_hist = (sizeof(struct sym_hist) + size * sizeof(u64)); > + sizeof_sym_hist = (sizeof(struct sym_hist) + size * sizeof(struct > sym_sample)); > > /* Check for overflow in zalloc argument */ > if (sizeof_sym_hist > (SIZE_MAX - sizeof(*notes->src)) > @@ -714,11 +714,11 @@ static int __symbol__inc_addr_samples(struct symbol > *sym, struct map *map, > offset = addr - sym->start; > h = annotation__histogram(notes, evidx); > h->sum++; > - h->addr[offset]++; > + h->addr[offset].nr_samples++; > > pr_debug3("%#" PRIx64 " %s: period++ [addr: %#" PRIx64 ", %#" PRIx64 > ", evidx=%d] => %" PRIu64 "\n", sym->start, sym->name, > - addr, addr - sym->start, evidx, h->addr[offset]); > + addr, addr - sym->start, evidx, h->addr[offset].nr_samples); > return 0; > } > > @@ -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->sum++; > + 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) > @@ -928,11 +952,13 @@ 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, u64 *nr_samples) > + s64 end, const char **path, u64 *nr_samples, u64 > *period) > { > struct source_line *src_line = notes->src->lines; > double percent = 0.0; > + > *nr_samples = 0; > + *period = 0; > > if (src_line) { > size_t sizeof_src_line = sizeof(*src_line) + > @@ -952,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++]; > + while (offset < end) { > + hits += h->addr[offset].nr_samples; > + p += h->addr[offset++].period; > + } > > if (h->sum) { > *nr_samples = hits; > + *period = p; > percent = 100.0 * hits / h->sum; > } > } > @@ -1057,10 +1087,11 @@ static int disasm_line__print(struct disasm_line *dl, > struct symbol *sym, u64 st > > if (dl->offset != -1) { > const char *path = NULL; > - u64 nr_samples; > + u64 nr_samples, period; > double percent, max_percent = 0.0; > double *ppercents = &percent; > u64 *psamples = &nr_samples; > + u64 *pperiod = . > int i, nr_percent = 1; > const char *color; > struct annotation *notes = symbol__annotation(sym); > @@ -1075,9 +1106,9 @@ static int disasm_line__print(struct disasm_line *dl, > struct symbol *sym, u64 st > nr_percent = evsel->nr_members; > ppercents = calloc(nr_percent, sizeof(double)); > psamples = calloc(nr_percent, sizeof(u64)); > - if (ppercents == NULL || psamples == NULL) { > + pperiod = calloc(nr_percent, sizeof(u64)); > + if (ppercents == NULL || psamples == NULL || pperiod == > NULL) > return -1; > - } > } > > for (i = 0; i < nr_percent; i++) { > @@ -1085,10 +1116,11 @@ 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, &nr_samples); > + &path, &nr_samples, &period); > > ppercents[i] = percent; > psamples[i] = nr_samples; > + pperiod[i] = period; > if (percent > max_percent) > max_percent = percent; > } > @@ -1127,11 +1159,12 @@ static int disasm_line__print(struct disasm_line *dl, > struct symbol *sym, u64 st > for (i = 0; i < nr_percent; i++) { > percent = ppercents[i]; > nr_samples = psamples[i]; > + period = pperiod[i]; > color = get_percent_color(percent); > > if (symbol_conf.show_total_period) > - color_fprintf(stdout, color, " %7" PRIu64, > - nr_samples); > + color_fprintf(stdout, color, " %11" PRIu64, > + period); > else > color_fprintf(stdout, color, " %7.2f", percent); > } > @@ -1150,6 +1183,9 @@ static int disasm_line__print(struct disasm_line *dl, > struct symbol *sym, u64 st > if (psamples != &nr_samples) > free(psamples); > > + if (pperiod != &period) > + free(pperiod); > + > } else if (max_lines && printed >= max_lines) > return 1; > else { > @@ -1161,6 +1197,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 > @@ -1702,7 +1742,7 @@ static int symbol__get_source_line(struct symbol *sym, > struct map *map, > double percent = 0.0; > > h = annotation__histogram(notes, evidx + k); > - nr_samples = h->addr[i]; > + nr_samples = h->addr[i].nr_samples; > if (h->sum) > percent = 100.0 * nr_samples / h->sum; > > @@ -1773,9 +1813,9 @@ static void symbol__annotate_hits(struct symbol *sym, > struct perf_evsel *evsel) > u64 len = symbol__size(sym), offset; > > for (offset = 0; offset < len; ++offset) > - if (h->addr[offset] != 0) > + if (h->addr[offset].nr_samples != 0) > printf("%*" PRIx64 ": %" PRIu64 "\n", BITS_PER_LONG / 2, > - sym->start + offset, h->addr[offset]); > + sym->start + offset, h->addr[offset].nr_samples); > printf("%*s: %" PRIu64 "\n", BITS_PER_LONG / 2, "h->sum", h->sum); > } > > @@ -1811,8 +1851,16 @@ 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->sum); > + 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, > + symbol_conf.show_total_period ? "Event count" > : "Percent", > + d_filename, evsel_name, > + symbol_conf.show_total_period ? > h->total_period : h->sum, > + symbol_conf.show_total_period ? "event count" > : "samples"); > > printf("%-*.*s----\n", > graph_dotted_len, graph_dotted_len, graph_dotted_line); > @@ -1878,8 +1926,8 @@ void symbol__annotate_decay_histogram(struct symbol > *sym, int evidx) > > h->sum = 0; > for (offset = 0; offset < len; ++offset) { > - h->addr[offset] = h->addr[offset] * 7 / 8; > - h->sum += h->addr[offset]; > + h->addr[offset].nr_samples = h->addr[offset].nr_samples * 7 / 8; > + h->sum += h->addr[offset].nr_samples; > } > } > > diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h > index bac698d..6b2e645 100644 > --- a/tools/perf/util/annotate.h > +++ b/tools/perf/util/annotate.h > @@ -79,11 +79,16 @@ 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, u64 *nr_samples); > + s64 end, const char **path, u64 *nr_samples, u64 > *period); > +struct sym_sample { > + u64 nr_samples; > + u64 period; > +}; > > struct sym_hist { > u64 sum; > - u64 addr[0]; > + u64 total_period; > + struct sym_sample addr[0]; > }; > > struct cyc_hist { > @@ -155,7 +160,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
