Re: [PATCH 1/4] perf annotate: Fix wrong --show-total-period option showing number of samples
From: Taeung Song
Date: Thu Jul 13 2017 - 03:23:17 EST
Hi Arnaldo :)
On 07/13/2017 05:09 AM, Arnaldo Carvalho de Melo wrote:
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".
Okey, I'll separate this patch.
Then, you need a patch to introduce that "struct sym_sample" and use it,
but please rename it to "struct sym_hist_entry".
I got it.
You can do it and just update the sym_hist_entry->period field, before
the change to pass 'struct sample' around.
Understood.
That disasm__calc_percent() function could receive a pointer to a struct
sym_hist_entry instead of two pointers.
Okey.
Small, self contained patches that do one thing make reviewing easier,
by yourself and others.
Yep.
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
I understood what you said! I'll send v2 in accordance with your comment.
Thanks,
Taeung
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 <namhyung@xxxxxxxxxx>
Cc: Milian Wolff <milian.wolff@xxxxxxxx>
Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
Signed-off-by: Taeung Song <treeze.taeung@xxxxxxxxx>
---
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