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

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

Next message: Sai Gurrappadi: "Re: [RFC][PATCH v3 2/2] cpufreq: schedutil: Avoid reducing frequency of busy CPUs prematurely"
Previous message: Linus Torvalds: "Re: locking/atomic: Introduce atomic_try_cmpxchg()"
Next in thread: Arnaldo Carvalho de Melo: "Re: [PATCH] perf report: enable sorting by srcline as key"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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
> # 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: Sai Gurrappadi: "Re: [RFC][PATCH v3 2/2] cpufreq: schedutil: Avoid reducing frequency of busy CPUs prematurely"
Previous message: Linus Torvalds: "Re: locking/atomic: Introduce atomic_try_cmpxchg()"
Next in thread: Arnaldo Carvalho de Melo: "Re: [PATCH] perf report: enable sorting by srcline as key"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]