Re: [PATCH] perf report: enable sorting by srcline as key

From: Arnaldo Carvalho de Melo
Date: Fri Mar 24 2017 - 15:12:24 EST

Next message: Mark Brown: "Applied "ASoC: codec: wm8960: Refactor sysclk freq search" to the asoc tree"
Previous message: Moritz Fischer: "Re: [PATCH 0/5] fpga: enhancements to support non-dt code"
In reply to: Arnaldo Carvalho de Melo: "Re: [PATCH] perf report: enable sorting by srcline as key"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Em Fri, Mar 24, 2017 at 04:09:38PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Sat, Mar 18, 2017 at 10:49:28PM +0100, Milian Wolff escreveu:
> > Often it is interesting to know how costly a given source line is in
> > total. Previously, one had to build these sums manually based on all
> > addresses that pointed to the same source line. This patch introduces
> > srcline as a sort key, which will do the aggregation for us.
> >
> > Paired with the recent addition of showing inline frames, this makes
> > perf report much more useful for many C++ work loads.
> >
> > The following shows the new feature in action. First, let's show the
> > status quo output when we sort by address. The result contains many
> > hist entries that generate the same output:
>
> Looks ok, one pet peeve below
>
> > ~~~~~~~~~~~~~~~~
> > $ perf report --stdio --inline -g address

Another minor issue: please right justify examples, as there are markers
that will be interpreted by git and associated scripts, like starting a
line with '#', which will be suppressed from the commit message. I fixed
it up.

> > # Children Self Command Shared Object Symbol
> > # ........ ........ ............ ................... .........................................
> > #
> > 99.89% 35.34% cpp-inlining cpp-inlining [.] main
> > |
> > |--64.55%--main complex:655
> > | /home/milian/projects/kdab/rnd/hotspot/tests/test-clients/cpp-inlining/main.cpp:39 (inline)
> > | /usr/include/c++/6.3.1/complex:664 (inline)
> > | |
> > | |--60.31%--hypot +20
> > | | |
> > | | |--8.52%--__hypot_finite +273
> > | | |
> > | | |--7.32%--__hypot_finite +411
> > ...
> > --35.34%--_start +4194346
> > __libc_start_main +241
> > |
> > |--6.65%--main random.tcc:3326
> > | /home/milian/projects/kdab/rnd/hotspot/tests/test-clients/cpp-inlining/main.cpp:39 (inline)
> > | /usr/include/c++/6.3.1/bits/random.h:1809 (inline)
> > | /usr/include/c++/6.3.1/bits/random.h:1818 (inline)
> > | /usr/include/c++/6.3.1/bits/random.h:185 (inline)
> > |
> > |--2.70%--main random.tcc:3326
> > | /home/milian/projects/kdab/rnd/hotspot/tests/test-clients/cpp-inlining/main.cpp:39 (inline)
> > | /usr/include/c++/6.3.1/bits/random.h:1809 (inline)
> > | /usr/include/c++/6.3.1/bits/random.h:1818 (inline)
> > | /usr/include/c++/6.3.1/bits/random.h:185 (inline)
> > |
> > |--1.69%--main random.tcc:3326
> > | /home/milian/projects/kdab/rnd/hotspot/tests/test-clients/cpp-inlining/main.cpp:39 (inline)
> > | /usr/include/c++/6.3.1/bits/random.h:1809 (inline)
> > | /usr/include/c++/6.3.1/bits/random.h:1818 (inline)
> > | /usr/include/c++/6.3.1/bits/random.h:185 (inline)
> > ...
> > ~~~~~~~~~~~~~~~~
> >
> > With this patch and `-g srcline` we instead get the following output:
> >
> > ~~~~~~~~~~~~~~~~
> > $ perf report --stdio --inline -g srcline
> > # Children Self Command Shared Object Symbol
> > # ........ ........ ............ ................... .........................................
> > #
> > 99.89% 35.34% cpp-inlining cpp-inlining [.] main
> > |
> > |--64.55%--main complex:655
> > | /home/milian/projects/kdab/rnd/hotspot/tests/test-clients/cpp-inlining/main.cpp:39 (inline)
> > | /usr/include/c++/6.3.1/complex:664 (inline)
> > | |
> > | |--64.02%--hypot
> > | | |
> > | | --59.81%--__hypot_finite
> > | |
> > | --0.53%--cabs
> > |
> > --35.34%--_start
> > __libc_start_main
> > |
> > |--12.48%--main random.tcc:3326
> > | /home/milian/projects/kdab/rnd/hotspot/tests/test-clients/cpp-inlining/main.cpp:39 (inline)
> > | /usr/include/c++/6.3.1/bits/random.h:1809 (inline)
> > | /usr/include/c++/6.3.1/bits/random.h:1818 (inline)
> > | /usr/include/c++/6.3.1/bits/random.h:185 (inline)
> > ...
> > ~~~~~~~~~~~~~~~~
> >
> > Signed-off-by: Milian Wolff <milian.wolff@xxxxxxxx>
> > ---
> > tools/perf/Documentation/perf-report.txt | 1 +
> > tools/perf/ui/browsers/hists.c | 3 +-
> > tools/perf/ui/stdio/hist.c | 3 +-
> > tools/perf/util/annotate.c | 3 +-
> > tools/perf/util/callchain.c | 52 +++++++++++++++++++++++++++++---
> > tools/perf/util/callchain.h | 3 +-
> > tools/perf/util/map.c | 3 +-
> > tools/perf/util/sort.c | 16 ++++++----
> > tools/perf/util/srcline.c | 11 +++++--
> > tools/perf/util/util.h | 4 +--
> > 10 files changed, 78 insertions(+), 21 deletions(-)
> >
> > diff --git a/tools/perf/Documentation/perf-report.txt b/tools/perf/Documentation/perf-report.txt
> > index 248bba434b53..37a175914157 100644
> > --- a/tools/perf/Documentation/perf-report.txt
> > +++ b/tools/perf/Documentation/perf-report.txt
> > @@ -235,6 +235,7 @@ OPTIONS
> > sort_key can be:
> > - function: compare on functions (default)
> > - address: compare on individual code addresses
> > + - srcline: compare on source filename and line number
> >
> > branch can be:
> > - branch: include last branch information in callgraph when available.
> > diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> > index 757222bc9dad..bca08be9fd78 100644
> > --- a/tools/perf/ui/browsers/hists.c
> > +++ b/tools/perf/ui/browsers/hists.c
> > @@ -851,7 +851,8 @@ static int hist_browser__show_inline(struct hist_browser *browser,
> > if (ui_browser__is_current_entry(&browser->b, row))
> > color = HE_COLORSET_SELECTED;
> >
> > - if (callchain_param.key == CCKEY_ADDRESS) {
> > + if (callchain_param.key == CCKEY_ADDRESS ||
> > + callchain_param.key == CCKEY_SRCLINE) {
> > if (ilist->filename != NULL)
> > scnprintf(buf, sizeof(buf),
> > "%s:%d (inline)",
> > diff --git a/tools/perf/ui/stdio/hist.c b/tools/perf/ui/stdio/hist.c
> > index 183470f25100..bf95b4a9a592 100644
> > --- a/tools/perf/ui/stdio/hist.c
> > +++ b/tools/perf/ui/stdio/hist.c
> > @@ -53,7 +53,8 @@ static size_t inline__fprintf(struct map *map, u64 ip, int left_margin,
> > ret += fprintf(fp, " ");
> > }
> >
> > - if (callchain_param.key == CCKEY_ADDRESS) {
> > + if (callchain_param.key == CCKEY_ADDRESS ||
> > + callchain_param.key == CCKEY_SRCLINE) {
> > if (ilist->filename != NULL)
> > ret += fprintf(fp, "%s:%d (inline)",
> > ilist->filename,
> > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> > index 273f21fa32b5..e7194ff88c4f 100644
> > --- a/tools/perf/util/annotate.c
> > +++ b/tools/perf/util/annotate.c
> > @@ -1668,7 +1668,8 @@ static int symbol__get_source_line(struct symbol *sym, struct map *map,
> > goto next;
> >
> > offset = start + i;
> > - src_line->path = get_srcline(map->dso, offset, NULL, false);
> > + src_line->path = get_srcline(map->dso, offset, NULL,
> > + false, true);
> > insert_source_line(&tmp_root, src_line);
> >
> > next:
> > diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
> > index aba953421a03..d78776a20e80 100644
> > --- a/tools/perf/util/callchain.c
> > +++ b/tools/perf/util/callchain.c
> > @@ -80,6 +80,10 @@ static int parse_callchain_sort_key(const char *value)
> > callchain_param.key = CCKEY_ADDRESS;
> > return 0;
> > }
> > + if (!strncmp(value, "srcline", strlen(value))) {
> > + callchain_param.key = CCKEY_SRCLINE;
> > + return 0;
> > + }
> > if (!strncmp(value, "branch", strlen(value))) {
> > callchain_param.branch_callstack = 1;
> > return 0;
> > @@ -510,14 +514,51 @@ enum match_result {
> > MATCH_GT,
> > };
> >
> > +static enum match_result match_chain_srcline(struct callchain_cursor_node *node,
> > + struct callchain_list *cnode)
> > +{
> > + char *left = get_srcline(cnode->ms.map->dso,
> > + map__rip_2objdump(cnode->ms.map, cnode->ip),
> > + cnode->ms.sym, true, false);
> > + char *right = get_srcline(node->map->dso,
> > + map__rip_2objdump(node->map, node->ip),
> > + node->sym, true, false);
> > + enum match_result ret = MATCH_EQ;
> > + int cmp;
> > +
> > + if (left && right)
> > + cmp = strcmp(left, right);
> > + else if (!left && right)
> > + cmp = 1;
> > + else if (left && !right)
> > + cmp = -1;
> > + else if (cnode->ip == node->ip)
> > + cmp = 0;
> > + else
> > + cmp = (cnode->ip < node->ip) ? -1 : 1;
> > +
> > + if (cmp != 0)
> > + ret = cmp < 0 ? MATCH_LT : MATCH_GT;
> > +
> > + free_srcline(left);
> > + free_srcline(right);
> > + return ret;
> > +}
> > +
> > static enum match_result match_chain(struct callchain_cursor_node *node,
> > struct callchain_list *cnode)
> > {
> > struct symbol *sym = node->sym;
> > u64 left, right;
> >
> > - if (cnode->ms.sym && sym &&
> > - callchain_param.key == CCKEY_FUNCTION) {
> > + if (callchain_param.key == CCKEY_SRCLINE) {
> > + enum match_result match = match_chain_srcline(node, cnode);
> > +
> > + if (match != MATCH_ERROR)
> > + return match;
> > + }
> > +
> > + if (cnode->ms.sym && sym && callchain_param.key == CCKEY_FUNCTION) {
>
> The above line is the same as it was in those first two removed, its
> just churn :-\ I.e. this part:
>
> - if (cnode->ms.sym && sym &&
> - callchain_param.key == CCKEY_FUNCTION) {
> + if (cnode->ms.sym && sym && callchain_param.key == CCKEY_FUNCTION) {
>
> Please avoid doing that in the future.
>
> > left = cnode->ms.sym->start;
> > right = sym->start;
> > } else {
> > @@ -911,15 +952,16 @@ int fill_callchain_info(struct addr_location *al, struct callchain_cursor_node *
> > char *callchain_list__sym_name(struct callchain_list *cl,
> > char *bf, size_t bfsize, bool show_dso)
> > {
> > + bool show_addr = callchain_param.key == CCKEY_ADDRESS;
> > + bool show_srcline = show_addr || callchain_param.key == CCKEY_SRCLINE;
> > int printed;
> >
> > if (cl->ms.sym) {
> > - if (callchain_param.key == CCKEY_ADDRESS &&
> > - cl->ms.map && !cl->srcline)
> > + if (show_srcline && cl->ms.map && !cl->srcline)
> > cl->srcline = get_srcline(cl->ms.map->dso,
> > map__rip_2objdump(cl->ms.map,
> > cl->ip),
> > - cl->ms.sym, false);
> > + cl->ms.sym, false, show_addr);
> > if (cl->srcline)
> > printed = scnprintf(bf, bfsize, "%s %s",
> > cl->ms.sym->name, cl->srcline);
> > diff --git a/tools/perf/util/callchain.h b/tools/perf/util/callchain.h
> > index 4f4b60f1558a..c56c23dbbf72 100644
> > --- a/tools/perf/util/callchain.h
> > +++ b/tools/perf/util/callchain.h
> > @@ -77,7 +77,8 @@ typedef void (*sort_chain_func_t)(struct rb_root *, struct callchain_root *,
> >
> > enum chain_key {
> > CCKEY_FUNCTION,
> > - CCKEY_ADDRESS
> > + CCKEY_ADDRESS,
> > + CCKEY_SRCLINE
> > };
> >
> > enum chain_value {
> > diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
> > index 1d9ebcf9e38e..c1870ac365a3 100644
> > --- a/tools/perf/util/map.c
> > +++ b/tools/perf/util/map.c
> > @@ -405,7 +405,8 @@ int map__fprintf_srcline(struct map *map, u64 addr, const char *prefix,
> >
> > if (map && map->dso) {
> > srcline = get_srcline(map->dso,
> > - map__rip_2objdump(map, addr), NULL, true);
> > + map__rip_2objdump(map, addr), NULL,
> > + true, true);
> > if (srcline != SRCLINE_UNKNOWN)
> > ret = fprintf(fp, "%s%s", prefix, srcline);
> > free_srcline(srcline);
> > diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> > index 8b0d4e39f640..73f3ec1cf2a0 100644
> > --- a/tools/perf/util/sort.c
> > +++ b/tools/perf/util/sort.c
> > @@ -323,7 +323,7 @@ char *hist_entry__get_srcline(struct hist_entry *he)
> > return SRCLINE_UNKNOWN;
> >
> > return get_srcline(map->dso, map__rip_2objdump(map, he->ip),
> > - he->ms.sym, true);
> > + he->ms.sym, true, true);
> > }
> >
> > static int64_t
> > @@ -366,7 +366,8 @@ sort__srcline_from_cmp(struct hist_entry *left, struct hist_entry *right)
> > left->branch_info->srcline_from = get_srcline(map->dso,
> > map__rip_2objdump(map,
> > left->branch_info->from.al_addr),
> > - left->branch_info->from.sym, true);
> > + left->branch_info->from.sym,
> > + true, true);
> > }
> > if (!right->branch_info->srcline_from) {
> > struct map *map = right->branch_info->from.map;
> > @@ -376,7 +377,8 @@ sort__srcline_from_cmp(struct hist_entry *left, struct hist_entry *right)
> > right->branch_info->srcline_from = get_srcline(map->dso,
> > map__rip_2objdump(map,
> > right->branch_info->from.al_addr),
> > - right->branch_info->from.sym, true);
> > + right->branch_info->from.sym,
> > + true, true);
> > }
> > return strcmp(right->branch_info->srcline_from, left->branch_info->srcline_from);
> > }
> > @@ -407,7 +409,8 @@ sort__srcline_to_cmp(struct hist_entry *left, struct hist_entry *right)
> > left->branch_info->srcline_to = get_srcline(map->dso,
> > map__rip_2objdump(map,
> > left->branch_info->to.al_addr),
> > - left->branch_info->from.sym, true);
> > + left->branch_info->from.sym,
> > + true, true);
> > }
> > if (!right->branch_info->srcline_to) {
> > struct map *map = right->branch_info->to.map;
> > @@ -417,7 +420,8 @@ sort__srcline_to_cmp(struct hist_entry *left, struct hist_entry *right)
> > right->branch_info->srcline_to = get_srcline(map->dso,
> > map__rip_2objdump(map,
> > right->branch_info->to.al_addr),
> > - right->branch_info->to.sym, true);
> > + right->branch_info->to.sym,
> > + true, true);
> > }
> > return strcmp(right->branch_info->srcline_to, left->branch_info->srcline_to);
> > }
> > @@ -448,7 +452,7 @@ static char *hist_entry__get_srcfile(struct hist_entry *e)
> > return no_srcfile;
> >
> > sf = __get_srcline(map->dso, map__rip_2objdump(map, e->ip),
> > - e->ms.sym, false, true);
> > + e->ms.sym, false, true, true);
> > if (!strcmp(sf, SRCLINE_UNKNOWN))
> > return no_srcfile;
> > p = strchr(sf, ':');
> > diff --git a/tools/perf/util/srcline.c b/tools/perf/util/srcline.c
> > index f9d4b47d1fb5..6f8651104990 100644
> > --- a/tools/perf/util/srcline.c
> > +++ b/tools/perf/util/srcline.c
> > @@ -427,7 +427,7 @@ static struct inline_node *addr2inlines(const char *dso_name, u64 addr,
> > #define A2L_FAIL_LIMIT 123
> >
> > char *__get_srcline(struct dso *dso, u64 addr, struct symbol *sym,
> > - bool show_sym, bool unwind_inlines)
> > + bool show_sym, bool show_addr, bool unwind_inlines)
> > {
> > char *file = NULL;
> > unsigned line = 0;
> > @@ -461,6 +461,11 @@ char *__get_srcline(struct dso *dso, u64 addr, struct symbol *sym,
> > dso->has_srcline = 0;
> > dso__free_a2l(dso);
> > }
> > +
> > + if (!show_addr)
> > + return (show_sym && sym) ?
> > + strndup(sym->name, sym->namelen) : NULL;
> > +
> > if (sym) {
> > if (asprintf(&srcline, "%s+%" PRIu64, show_sym ? sym->name : "",
> > addr - sym->start) < 0)
> > @@ -477,9 +482,9 @@ void free_srcline(char *srcline)
> > }
> >
> > char *get_srcline(struct dso *dso, u64 addr, struct symbol *sym,
> > - bool show_sym)
> > + bool show_sym, bool show_addr)
> > {
> > - return __get_srcline(dso, addr, sym, show_sym, false);
> > + return __get_srcline(dso, addr, sym, show_sym, show_addr, false);
> > }
> >
> > struct inline_node *dso__parse_addr_inlines(struct dso *dso, u64 addr)
> > diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
> > index cc0700d6fef0..7cf5752b38fd 100644
> > --- a/tools/perf/util/util.h
> > +++ b/tools/perf/util/util.h
> > @@ -287,9 +287,9 @@ struct symbol;
> >
> > extern bool srcline_full_filename;
> > char *get_srcline(struct dso *dso, u64 addr, struct symbol *sym,
> > - bool show_sym);
> > + bool show_sym, bool show_addr);
> > char *__get_srcline(struct dso *dso, u64 addr, struct symbol *sym,
> > - bool show_sym, bool unwind_inlines);
> > + bool show_sym, bool show_addr, bool unwind_inlines);
> > void free_srcline(char *srcline);
> >
> > int perf_event_paranoid(void);
> > --
> > 2.12.0

Next message: Mark Brown: "Applied "ASoC: codec: wm8960: Refactor sysclk freq search" to the asoc tree"
Previous message: Moritz Fischer: "Re: [PATCH 0/5] fpga: enhancements to support non-dt code"
In reply to: Arnaldo Carvalho de Melo: "Re: [PATCH] perf report: enable sorting by srcline as key"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]