[PATCHSET 0/7] perf tools: Support hierarchy report with event group (v1)

[PATCHSET 0/7] perf tools: Support hierarchy report with event group (v1)

[Namhyung Kim]

Hello,

This patchset implements hierarchy mode with event group.  It was
disabled due to complexity when I wrote the hierarchy code, but
there's no fundamental reason to do it.

It's also available on 'perf/hierarchy-group-v1' branch in my tree:

  git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git

Thanks,
Namhyung


Namhyung Kim (7):
  perf hists browser: Fix event group display
  perf hist: Introduce hists__match_hierarchy()
  perf hist: Introduce hists__link_hierarchy()
  perf hist: Initialize hierachy tree explicitly
  perf ui/stdio: Reset output width for hierarchy
  perf ui/tui: Reset output width for hierarchy
  perf report: Enable group view with hierarchy

 tools/perf/builtin-report.c    |   1 -
 tools/perf/ui/browsers/hists.c |   7 +-
 tools/perf/ui/stdio/hist.c     |   6 ++
 tools/perf/util/hist.c         | 148 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 160 insertions(+), 2 deletions(-)

-- 
2.9.3

[PATCH 1/7] perf hists browser: Fix event group display

[Namhyung Kim]

Milian reported that the event group on TUI shows duplicated overhead.
This was due to a bug on calculating hpp->buf position.  The
hpp_advance() was called from __hpp__slsmg_color_printf() on TUI but
it's already called from the hpp__call_print_fn macro in __hpp__fmt().
The end result is that the print function returns number of bytes it
printed but the buffer advanced twice of the length.

This is generally not a problem since it doesn't need to access the
buffer again.  But with event group, overhead needs to be printed
multiple times and hist_entry__snprintf_alignment() tries to fill the
space with buffer after it printed.  So it (brokenly) showed the last
overhead again.

The bug was there from the beginning, but I think it's only revealed
when the alignment function was added.

Reported-by: Milian Wolff <milian.wolff@kdab.com>
Fixes: 89fee7094323 ("perf hists: Do column alignment on the format iterator")
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/browsers/hists.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index f0611c937d4b..35e44b1879e3 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1097,7 +1097,6 @@ static int __hpp__slsmg_color_printf(struct perf_hpp *hpp, const char *fmt, ...)
 	ret = scnprintf(hpp->buf, hpp->size, fmt, len, percent);
 	ui_browser__printf(arg->b, "%s", hpp->buf);
 
-	advance_hpp(hpp, ret);
 	return ret;
 }
 
-- 
2.9.3

[PATCH 2/7] perf hist: Introduce hists__match_hierarchy()

[Namhyung Kim]

The hists__match_hierarchy() is to find matching hist entries in a
group.  It needs to search all matching children in the hierarchy.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/hist.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index de15dbcdcecf..be3f5ce31303 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -2174,6 +2174,51 @@ static struct hist_entry *hists__find_entry(struct hists *hists,
 	return NULL;
 }
 
+static struct hist_entry *hists__find_hierarchy_entry(struct rb_root *root,
+						      struct hist_entry *he)
+{
+	struct rb_node *n = root->rb_node;
+
+	while (n) {
+		struct hist_entry *iter;
+		struct perf_hpp_fmt *fmt;
+		int64_t cmp = 0;
+
+		iter = rb_entry(n, struct hist_entry, rb_node_in);
+		perf_hpp_list__for_each_sort_list(he->hpp_list, fmt) {
+			cmp = fmt->collapse(fmt, iter, he);
+			if (cmp)
+				break;
+		}
+
+		if (cmp < 0)
+			n = n->rb_left;
+		else if (cmp > 0)
+			n = n->rb_right;
+		else
+			return iter;
+	}
+
+	return NULL;
+}
+
+static void hists__match_hierarchy(struct rb_root *leader_root,
+				   struct rb_root *other_root)
+{
+	struct rb_node *nd;
+	struct hist_entry *pos, *pair;
+
+	for (nd = rb_first(leader_root); nd; nd = rb_next(nd)) {
+		pos  = rb_entry(nd, struct hist_entry, rb_node_in);
+		pair = hists__find_hierarchy_entry(other_root, pos);
+
+		if (pair) {
+			hist_entry__add_pair(pair, pos);
+			hists__match_hierarchy(&pos->hroot_in, &pair->hroot_in);
+		}
+	}
+}
+
 /*
  * Look for pairs to link to the leader buckets (hist_entries):
  */
@@ -2183,6 +2228,12 @@ void hists__match(struct hists *leader, struct hists *other)
 	struct rb_node *nd;
 	struct hist_entry *pos, *pair;
 
+	if (symbol_conf.report_hierarchy) {
+		/* hierarchy report always collapses entries */
+		return hists__match_hierarchy(&leader->entries_collapsed,
+					      &other->entries_collapsed);
+	}
+
 	if (hists__has(leader, need_collapse))
 		root = &leader->entries_collapsed;
 	else
-- 
2.9.3

[PATCH 3/7] perf hist: Introduce hists__link_hierarchy()

[Namhyung Kim]

The hists__link_hierarchy() is to support hierarchy report with event
group.  When it matches leader event and other members (with the
hists__match_hierarchy), it also needs to link unmatched member entries
with a dummy leader event so that it can show up in the output.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/hist.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 95 insertions(+)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index be3f5ce31303..702ba3a8ead6 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -2149,6 +2149,50 @@ static struct hist_entry *hists__add_dummy_entry(struct hists *hists,
 	return he;
 }
 
+static struct hist_entry *add_dummy_hierarchy_entry(struct hists *hists,
+						    struct rb_root *root,
+						    struct hist_entry *pair)
+{
+	struct rb_node **p;
+	struct rb_node *parent = NULL;
+	struct hist_entry *he;
+	struct perf_hpp_fmt *fmt;
+
+	p = &root->rb_node;
+	while (*p != NULL) {
+		int64_t cmp = 0;
+
+		parent = *p;
+		he = rb_entry(parent, struct hist_entry, rb_node_in);
+
+		perf_hpp_list__for_each_sort_list(he->hpp_list, fmt) {
+			cmp = fmt->collapse(fmt, he, pair);
+			if (cmp)
+				break;
+		}
+		if (!cmp)
+			goto out;
+
+		if (cmp < 0)
+			p = &parent->rb_left;
+		else
+			p = &parent->rb_right;
+	}
+
+	he = hist_entry__new(pair, true);
+	if (he) {
+		rb_link_node(&he->rb_node_in, parent, p);
+		rb_insert_color(&he->rb_node_in, root);
+
+		he->dummy = true;
+		he->hists = hists;
+		memset(&he->stat, 0, sizeof(he->stat));
+		hists__inc_stats(hists, he);
+	}
+out:
+	return he;
+}
+
 static struct hist_entry *hists__find_entry(struct hists *hists,
 					    struct hist_entry *he)
 {
@@ -2248,6 +2292,50 @@ void hists__match(struct hists *leader, struct hists *other)
 	}
 }
 
+static int hists__link_hierarchy(struct hists *leader_hists,
+				 struct hist_entry *parent,
+				 struct rb_root *leader_root,
+				 struct rb_root *other_root)
+{
+	struct rb_node *nd;
+	struct hist_entry *pos, *leader;
+
+	for (nd = rb_first(other_root); nd; nd = rb_next(nd)) {
+		pos = rb_entry(nd, struct hist_entry, rb_node_in);
+
+		if (hist_entry__has_pairs(pos)) {
+			bool found = false;
+
+			list_for_each_entry(leader, &pos->pairs.head, pairs.node) {
+				if (leader->hists == leader_hists) {
+					found = true;
+					break;
+				}
+			}
+			if (!found)
+				return -1;
+		} else {
+			leader = add_dummy_hierarchy_entry(leader_hists,
+							   leader_root, pos);
+			if (leader == NULL)
+				return -1;
+
+			/* do not point parent in the pos */
+			leader->parent_he = parent;
+
+			hist_entry__add_pair(pos, leader);
+		}
+
+		if (!pos->leaf) {
+			if (hists__link_hierarchy(leader_hists, leader,
+						  &leader->hroot_in,
+						  &pos->hroot_in) < 0)
+				return -1;
+		}
+	}
+	return 0;
+}
+
 /*
  * Look for entries in the other hists that are not present in the leader, if
  * we find them, just add a dummy entry on the leader hists, with period=0,
@@ -2259,6 +2347,13 @@ int hists__link(struct hists *leader, struct hists *other)
 	struct rb_node *nd;
 	struct hist_entry *pos, *pair;
 
+	if (symbol_conf.report_hierarchy) {
+		/* hierarchy report always collapses entries */
+		return hists__link_hierarchy(leader, NULL,
+					     &leader->entries_collapsed,
+					     &other->entries_collapsed);
+	}
+
 	if (hists__has(other, need_collapse))
 		root = &other->entries_collapsed;
 	else
-- 
2.9.3

[PATCH 4/7] perf hist: Initialize hierachy tree explicitly

[Namhyung Kim]

The hroot_in and hroot_out are root of hiearchy tree of hist entry.  But
as hist entry is initialized by copying existing template entry, it
sometimes has non-empty tree and copied it incorrectly.  This is a
problem especially when event group is used since it creates dummy
entries from already-processed entries in other event members.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/hist.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 702ba3a8ead6..37a08f20730a 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -417,6 +417,8 @@ static int hist_entry__init(struct hist_entry *he,
 	}
 	INIT_LIST_HEAD(&he->pairs.node);
 	thread__get(he->thread);
+	he->hroot_in  = RB_ROOT;
+	he->hroot_out = RB_ROOT;
 
 	if (!symbol_conf.report_hierarchy)
 		he->leaf = true;
-- 
2.9.3

[PATCH 5/7] perf ui/stdio: Reset output width for hierarchy

[Namhyung Kim]

When --hierarchy option is used, each entry has its own hpp_list to show
the result.  But it missed to update width of each column.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/stdio/hist.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/perf/ui/stdio/hist.c b/tools/perf/ui/stdio/hist.c
index 9b65f4a6b35a..83ca728b6f61 100644
--- a/tools/perf/ui/stdio/hist.c
+++ b/tools/perf/ui/stdio/hist.c
@@ -733,6 +733,7 @@ size_t hists__fprintf(struct hists *hists, bool show_header, int max_rows,
 		      bool use_callchain)
 {
 	struct perf_hpp_fmt *fmt;
+	struct perf_hpp_list_node *node;
 	struct rb_node *nd;
 	size_t ret = 0;
 	const char *sep = symbol_conf.field_sep;
@@ -745,6 +746,11 @@ size_t hists__fprintf(struct hists *hists, bool show_header, int max_rows,
 
 	hists__for_each_format(hists, fmt)
 		perf_hpp__reset_width(fmt, hists);
+	/* hierarchy entries have their own hpp list */
+	list_for_each_entry(node, &hists->hpp_formats, list) {
+		perf_hpp_list__for_each_format(&node->hpp, fmt)
+			perf_hpp__reset_width(fmt, hists);
+	}
 
 	if (symbol_conf.col_width_list_str)
 		perf_hpp__set_user_width(symbol_conf.col_width_list_str);
-- 
2.9.3

[PATCH 6/7] perf ui/tui: Reset output width for hierarchy

[Namhyung Kim]

When --hierarchy option is used, each entry has its own hpp_list to show
the result.  But it missed to update width of each column.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/browsers/hists.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 35e44b1879e3..49db16334814 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -2067,6 +2067,7 @@ void hist_browser__init(struct hist_browser *browser,
 			struct hists *hists)
 {
 	struct perf_hpp_fmt *fmt;
+	struct perf_hpp_list_node *node;
 
 	browser->hists			= hists;
 	browser->b.refresh		= hist_browser__refresh;
@@ -2079,6 +2080,11 @@ void hist_browser__init(struct hist_browser *browser,
 		perf_hpp__reset_width(fmt, hists);
 		++browser->b.columns;
 	}
+	/* hierarchy entries have their own hpp list */
+	list_for_each_entry(node, &hists->hpp_formats, list) {
+		perf_hpp_list__for_each_format(&node->hpp, fmt)
+			perf_hpp__reset_width(fmt, hists);
+	}
 }
 
 struct hist_browser *hist_browser__new(struct hists *hists)
-- 
2.9.3

[PATCH 7/7] perf report: Enable group view with hierarchy

[Namhyung Kim]

Now all missing pieces are implemented, let's enable it.  An example
output below:

  $ perf report --hierarchy --stdio
  ...
  #               Overhead  Command / Shared Object / Symbol
  # ......................  ..................................
  #
      25.74%  27.18%        sh
         19.96%  24.14%        libc-2.24.so
            9.55%  14.64%        [.] __strcmp_sse2
            1.54%   0.00%        [.] __tfind
            1.07%   1.13%        [.] _int_malloc
            0.95%   0.00%        [.] __strchr_sse2
            0.89%   1.39%        [.] __tsearch
            0.76%   0.00%        [.] strlen
  ...

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-report.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 1a07c4cdf6ed..6e88460cd13d 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -935,7 +935,6 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
 
 	if (symbol_conf.report_hierarchy) {
 		/* disable incompatible options */
-		symbol_conf.event_group = false;
 		symbol_conf.cumulate_callchain = false;
 
 		if (field_order) {
-- 
2.9.3

Re: [PATCH 4/7] perf hist: Initialize hierachy tree explicitly

[Arnaldo Carvalho de Melo]

Em Mon, Sep 12, 2016 at 03:19:55PM +0900, Namhyung Kim escreveu:
> The hroot_in and hroot_out are root of hiearchy tree of hist entry.  But
> as hist entry is initialized by copying existing template entry, it

Which makes default_ops->new() pointing to a zalloc based constructor
totally useless, right? I.e. we malloc() + bzero() to then imediatelly
memcpy from the template :-)

> sometimes has non-empty tree and copied it incorrectly.  This is a
> problem especially when event group is used since it creates dummy
> entries from already-processed entries in other event members.

I guess this needs a "Fixes:" probably for the changeset introducing
those members :-\

- Arnaldo
 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/hist.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index 702ba3a8ead6..37a08f20730a 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -417,6 +417,8 @@ static int hist_entry__init(struct hist_entry *he,
>  	}
>  	INIT_LIST_HEAD(&he->pairs.node);
>  	thread__get(he->thread);
> +	he->hroot_in  = RB_ROOT;
> +	he->hroot_out = RB_ROOT;
>  
>  	if (!symbol_conf.report_hierarchy)
>  		he->leaf = true;
> -- 
> 2.9.3

Re: [PATCH 1/7] perf hists browser: Fix event group display

[Arnaldo Carvalho de Melo]

Em Mon, Sep 12, 2016 at 03:19:52PM +0900, Namhyung Kim escreveu:
> Milian reported that the event group on TUI shows duplicated overhead.
> This was due to a bug on calculating hpp->buf position.  The
> hpp_advance() was called from __hpp__slsmg_color_printf() on TUI but
> it's already called from the hpp__call_print_fn macro in __hpp__fmt().
> The end result is that the print function returns number of bytes it
> printed but the buffer advanced twice of the length.
> 
> This is generally not a problem since it doesn't need to access the
> buffer again.  But with event group, overhead needs to be printed
> multiple times and hist_entry__snprintf_alignment() tries to fill the
> space with buffer after it printed.  So it (brokenly) showed the last
> overhead again.
> 
> The bug was there from the beginning, but I think it's only revealed
> when the alignment function was added.

Thanks, applied.

Milian, if you could, please test this, I would then change the line
below to Reported-and-Tested-by: you, if I get it before my next pull
req to Ingo.

- Arnaldo
 
> Reported-by: Milian Wolff <milian.wolff@kdab.com>
> Fixes: 89fee7094323 ("perf hists: Do column alignment on the format iterator")
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/ui/browsers/hists.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index f0611c937d4b..35e44b1879e3 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -1097,7 +1097,6 @@ static int __hpp__slsmg_color_printf(struct perf_hpp *hpp, const char *fmt, ...)
>  	ret = scnprintf(hpp->buf, hpp->size, fmt, len, percent);
>  	ui_browser__printf(arg->b, "%s", hpp->buf);
>  
> -	advance_hpp(hpp, ret);
>  	return ret;
>  }
>  
> -- 
> 2.9.3

Re: [PATCH 2/7] perf hist: Introduce hists__match_hierarchy()

[Arnaldo Carvalho de Melo]

Em Mon, Sep 12, 2016 at 03:19:53PM +0900, Namhyung Kim escreveu:
> The hists__match_hierarchy() is to find matching hist entries in a
> group.  It needs to search all matching children in the hierarchy.

Can you please enrich this changeset log? Define "matching", an example
of how the output of a hierarchy + group would help people trying to
understand the changes you're making, for instance.

- Arnaldo
 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/hist.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
> 
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index de15dbcdcecf..be3f5ce31303 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -2174,6 +2174,51 @@ static struct hist_entry *hists__find_entry(struct hists *hists,
>  	return NULL;
>  }
>  
> +static struct hist_entry *hists__find_hierarchy_entry(struct rb_root *root,
> +						      struct hist_entry *he)
> +{
> +	struct rb_node *n = root->rb_node;
> +
> +	while (n) {
> +		struct hist_entry *iter;
> +		struct perf_hpp_fmt *fmt;
> +		int64_t cmp = 0;
> +
> +		iter = rb_entry(n, struct hist_entry, rb_node_in);
> +		perf_hpp_list__for_each_sort_list(he->hpp_list, fmt) {
> +			cmp = fmt->collapse(fmt, iter, he);
> +			if (cmp)
> +				break;
> +		}
> +
> +		if (cmp < 0)
> +			n = n->rb_left;
> +		else if (cmp > 0)
> +			n = n->rb_right;
> +		else
> +			return iter;
> +	}
> +
> +	return NULL;
> +}
> +
> +static void hists__match_hierarchy(struct rb_root *leader_root,
> +				   struct rb_root *other_root)
> +{
> +	struct rb_node *nd;
> +	struct hist_entry *pos, *pair;
> +
> +	for (nd = rb_first(leader_root); nd; nd = rb_next(nd)) {
> +		pos  = rb_entry(nd, struct hist_entry, rb_node_in);
> +		pair = hists__find_hierarchy_entry(other_root, pos);
> +
> +		if (pair) {
> +			hist_entry__add_pair(pair, pos);
> +			hists__match_hierarchy(&pos->hroot_in, &pair->hroot_in);
> +		}
> +	}
> +}
> +
>  /*
>   * Look for pairs to link to the leader buckets (hist_entries):
>   */
> @@ -2183,6 +2228,12 @@ void hists__match(struct hists *leader, struct hists *other)
>  	struct rb_node *nd;
>  	struct hist_entry *pos, *pair;
>  
> +	if (symbol_conf.report_hierarchy) {
> +		/* hierarchy report always collapses entries */
> +		return hists__match_hierarchy(&leader->entries_collapsed,
> +					      &other->entries_collapsed);
> +	}
> +
>  	if (hists__has(leader, need_collapse))
>  		root = &leader->entries_collapsed;
>  	else
> -- 
> 2.9.3

Re: [PATCH 5/7] perf ui/stdio: Reset output width for hierarchy

[Arnaldo Carvalho de Melo]

Em Mon, Sep 12, 2016 at 03:19:56PM +0900, Namhyung Kim escreveu:
> When --hierarchy option is used, each entry has its own hpp_list to show
> the result.  But it missed to update width of each column.

Missing "Fixes:" tag? This is important for the stable series, that IIRC
uses those tags to semi-automatically figure out what should be picked.

- Arnaldo
 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/ui/stdio/hist.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/tools/perf/ui/stdio/hist.c b/tools/perf/ui/stdio/hist.c
> index 9b65f4a6b35a..83ca728b6f61 100644
> --- a/tools/perf/ui/stdio/hist.c
> +++ b/tools/perf/ui/stdio/hist.c
> @@ -733,6 +733,7 @@ size_t hists__fprintf(struct hists *hists, bool show_header, int max_rows,
>  		      bool use_callchain)
>  {
>  	struct perf_hpp_fmt *fmt;
> +	struct perf_hpp_list_node *node;
>  	struct rb_node *nd;
>  	size_t ret = 0;
>  	const char *sep = symbol_conf.field_sep;
> @@ -745,6 +746,11 @@ size_t hists__fprintf(struct hists *hists, bool show_header, int max_rows,
>  
>  	hists__for_each_format(hists, fmt)
>  		perf_hpp__reset_width(fmt, hists);
> +	/* hierarchy entries have their own hpp list */
> +	list_for_each_entry(node, &hists->hpp_formats, list) {
> +		perf_hpp_list__for_each_format(&node->hpp, fmt)
> +			perf_hpp__reset_width(fmt, hists);
> +	}
>  
>  	if (symbol_conf.col_width_list_str)
>  		perf_hpp__set_user_width(symbol_conf.col_width_list_str);
> -- 
> 2.9.3

Re: [PATCH 6/7] perf ui/tui: Reset output width for hierarchy

[Arnaldo Carvalho de Melo]

Em Mon, Sep 12, 2016 at 03:19:57PM +0900, Namhyung Kim escreveu:
> When --hierarchy option is used, each entry has its own hpp_list to show
> the result.  But it missed to update width of each column.

Ditto, missing "Fixes:" tag.

- Arnaldo
 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/ui/browsers/hists.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 35e44b1879e3..49db16334814 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -2067,6 +2067,7 @@ void hist_browser__init(struct hist_browser *browser,
>  			struct hists *hists)
>  {
>  	struct perf_hpp_fmt *fmt;
> +	struct perf_hpp_list_node *node;
>  
>  	browser->hists			= hists;
>  	browser->b.refresh		= hist_browser__refresh;
> @@ -2079,6 +2080,11 @@ void hist_browser__init(struct hist_browser *browser,
>  		perf_hpp__reset_width(fmt, hists);
>  		++browser->b.columns;
>  	}
> +	/* hierarchy entries have their own hpp list */
> +	list_for_each_entry(node, &hists->hpp_formats, list) {
> +		perf_hpp_list__for_each_format(&node->hpp, fmt)
> +			perf_hpp__reset_width(fmt, hists);
> +	}
>  }
>  
>  struct hist_browser *hist_browser__new(struct hists *hists)
> -- 
> 2.9.3

Re: [PATCH 7/7] perf report: Enable group view with hierarchy

[Arnaldo Carvalho de Melo]

Em Mon, Sep 12, 2016 at 03:19:58PM +0900, Namhyung Kim escreveu:
> Now all missing pieces are implemented, let's enable it.  An example
> output below:

So. what are those events? What was the command line that produced the
'perf.data' file that was used?

- Arnaldo
 
>   $ perf report --hierarchy --stdio
>   ...
>   #               Overhead  Command / Shared Object / Symbol
>   # ......................  ..................................
>   #
>       25.74%  27.18%        sh
>          19.96%  24.14%        libc-2.24.so
>             9.55%  14.64%        [.] __strcmp_sse2
>             1.54%   0.00%        [.] __tfind
>             1.07%   1.13%        [.] _int_malloc
>             0.95%   0.00%        [.] __strchr_sse2
>             0.89%   1.39%        [.] __tsearch
>             0.76%   0.00%        [.] strlen
>   ...
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/builtin-report.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index 1a07c4cdf6ed..6e88460cd13d 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -935,7 +935,6 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
>  
>  	if (symbol_conf.report_hierarchy) {
>  		/* disable incompatible options */
> -		symbol_conf.event_group = false;
>  		symbol_conf.cumulate_callchain = false;
>  
>  		if (field_order) {
> -- 
> 2.9.3

Re: [PATCH 2/7] perf hist: Introduce hists__match_hierarchy()

[Namhyung Kim]

Hi Arnaldo,

On Mon, Sep 12, 2016 at 11:25:30AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Sep 12, 2016 at 03:19:53PM +0900, Namhyung Kim escreveu:
> > The hists__match_hierarchy() is to find matching hist entries in a
> > group.  It needs to search all matching children in the hierarchy.
> 
> Can you please enrich this changeset log? Define "matching", an example
> of how the output of a hierarchy + group would help people trying to
> understand the changes you're making, for instance.

Ok.  How about this?


The hists__match_hierarchy() is to find matching hist entries in a
group.  A matching entry has same values for all sort keys given.
With event group, a leader event should show other members in a
group.  So each entry in the leader should be able to find its pair
entries which have same values.  With hierarchy mode, it needs to
search all matching children in a hierarchy.

An example output below:

  #               Overhead  Command / Shared Object / Symbol
  # ......................  ..................................
  #
      25.74%  27.18%        sh
         19.96%  24.14%        libc-2.24.so
            9.55%  14.64%        [.] __strcmp_sse2
            1.54%   0.00%        [.] __tfind
            1.07%   1.13%        [.] _int_malloc
  ...

In the above example, two overhead were shown - one for leader and
another for member.  They were matched since their command, dso and
symbol have same values.


Thanks,
Namhyung


>  
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/util/hist.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 51 insertions(+)
> > 
> > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> > index de15dbcdcecf..be3f5ce31303 100644
> > --- a/tools/perf/util/hist.c
> > +++ b/tools/perf/util/hist.c
> > @@ -2174,6 +2174,51 @@ static struct hist_entry *hists__find_entry(struct hists *hists,
> >  	return NULL;
> >  }
> >  
> > +static struct hist_entry *hists__find_hierarchy_entry(struct rb_root *root,
> > +						      struct hist_entry *he)
> > +{
> > +	struct rb_node *n = root->rb_node;
> > +
> > +	while (n) {
> > +		struct hist_entry *iter;
> > +		struct perf_hpp_fmt *fmt;
> > +		int64_t cmp = 0;
> > +
> > +		iter = rb_entry(n, struct hist_entry, rb_node_in);
> > +		perf_hpp_list__for_each_sort_list(he->hpp_list, fmt) {
> > +			cmp = fmt->collapse(fmt, iter, he);
> > +			if (cmp)
> > +				break;
> > +		}
> > +
> > +		if (cmp < 0)
> > +			n = n->rb_left;
> > +		else if (cmp > 0)
> > +			n = n->rb_right;
> > +		else
> > +			return iter;
> > +	}
> > +
> > +	return NULL;
> > +}
> > +
> > +static void hists__match_hierarchy(struct rb_root *leader_root,
> > +				   struct rb_root *other_root)
> > +{
> > +	struct rb_node *nd;
> > +	struct hist_entry *pos, *pair;
> > +
> > +	for (nd = rb_first(leader_root); nd; nd = rb_next(nd)) {
> > +		pos  = rb_entry(nd, struct hist_entry, rb_node_in);
> > +		pair = hists__find_hierarchy_entry(other_root, pos);
> > +
> > +		if (pair) {
> > +			hist_entry__add_pair(pair, pos);
> > +			hists__match_hierarchy(&pos->hroot_in, &pair->hroot_in);
> > +		}
> > +	}
> > +}
> > +
> >  /*
> >   * Look for pairs to link to the leader buckets (hist_entries):
> >   */
> > @@ -2183,6 +2228,12 @@ void hists__match(struct hists *leader, struct hists *other)
> >  	struct rb_node *nd;
> >  	struct hist_entry *pos, *pair;
> >  
> > +	if (symbol_conf.report_hierarchy) {
> > +		/* hierarchy report always collapses entries */
> > +		return hists__match_hierarchy(&leader->entries_collapsed,
> > +					      &other->entries_collapsed);
> > +	}
> > +
> >  	if (hists__has(leader, need_collapse))
> >  		root = &leader->entries_collapsed;
> >  	else
> > -- 
> > 2.9.3

Re: [PATCH 4/7] perf hist: Initialize hierachy tree explicitly

[Namhyung Kim]

On Mon, Sep 12, 2016 at 11:22:28AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Sep 12, 2016 at 03:19:55PM +0900, Namhyung Kim escreveu:
> > The hroot_in and hroot_out are root of hiearchy tree of hist entry.  But
> > as hist entry is initialized by copying existing template entry, it
> 
> Which makes default_ops->new() pointing to a zalloc based constructor
> totally useless, right? I.e. we malloc() + bzero() to then imediatelly
> memcpy from the template :-)

Right.

> 
> > sometimes has non-empty tree and copied it incorrectly.  This is a
> > problem especially when event group is used since it creates dummy
> > entries from already-processed entries in other event members.
> 
> I guess this needs a "Fixes:" probably for the changeset introducing
> those members :-\

This problem can occur only if event group is used with hierarchy
which will be enabled by this patchset.  In the normal hierarchy path,
hist entries are created before processing and have empty hierarchy
tree.  So current code has no problem.

Thanks,
Namhyung

> 
> - Arnaldo
>  
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/util/hist.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> > index 702ba3a8ead6..37a08f20730a 100644
> > --- a/tools/perf/util/hist.c
> > +++ b/tools/perf/util/hist.c
> > @@ -417,6 +417,8 @@ static int hist_entry__init(struct hist_entry *he,
> >  	}
> >  	INIT_LIST_HEAD(&he->pairs.node);
> >  	thread__get(he->thread);
> > +	he->hroot_in  = RB_ROOT;
> > +	he->hroot_out = RB_ROOT;
> >  
> >  	if (!symbol_conf.report_hierarchy)
> >  		he->leaf = true;
> > -- 
> > 2.9.3

Re: [PATCH 5/7] perf ui/stdio: Reset output width for hierarchy

[Namhyung Kim]

On Mon, Sep 12, 2016 at 11:27:28AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Sep 12, 2016 at 03:19:56PM +0900, Namhyung Kim escreveu:
> > When --hierarchy option is used, each entry has its own hpp_list to show
> > the result.  But it missed to update width of each column.
> 
> Missing "Fixes:" tag? This is important for the stable series, that IIRC
> uses those tags to semi-automatically figure out what should be picked.

Looking at the code again, it already reset the width in the
hists__fprintf_hiearachy_headers().  But it's only called when the
show_header is true, so perf-top broke alignment.


  $ sudo perf top --hierarchy --stdio
  
   PerfTop:     160 irqs/sec  kernel:38.8%  exact: 100.0% [4000Hz cycles:pp],  (all, 12 CPUs)
  -------------------------------------------------------------------------------------------             

   52.32%     perf              
      24.74%     [.] __symbols__insert
      5.62%     [.] rb_next
      5.14%     [.] dso__load_sym

It's from the commit 1b2dbbf41a0f ("perf hists: Use own hpp_list for
hierarchy mode").  I'll fix the code and add the "Fixes:" tag in v2.

Thanks,
Namhyung


> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/ui/stdio/hist.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/tools/perf/ui/stdio/hist.c b/tools/perf/ui/stdio/hist.c
> > index 9b65f4a6b35a..83ca728b6f61 100644
> > --- a/tools/perf/ui/stdio/hist.c
> > +++ b/tools/perf/ui/stdio/hist.c
> > @@ -733,6 +733,7 @@ size_t hists__fprintf(struct hists *hists, bool show_header, int max_rows,
> >  		      bool use_callchain)
> >  {
> >  	struct perf_hpp_fmt *fmt;
> > +	struct perf_hpp_list_node *node;
> >  	struct rb_node *nd;
> >  	size_t ret = 0;
> >  	const char *sep = symbol_conf.field_sep;
> > @@ -745,6 +746,11 @@ size_t hists__fprintf(struct hists *hists, bool show_header, int max_rows,
> >  
> >  	hists__for_each_format(hists, fmt)
> >  		perf_hpp__reset_width(fmt, hists);
> > +	/* hierarchy entries have their own hpp list */
> > +	list_for_each_entry(node, &hists->hpp_formats, list) {
> > +		perf_hpp_list__for_each_format(&node->hpp, fmt)
> > +			perf_hpp__reset_width(fmt, hists);
> > +	}
> >  
> >  	if (symbol_conf.col_width_list_str)
> >  		perf_hpp__set_user_width(symbol_conf.col_width_list_str);
> > -- 
> > 2.9.3

Re: [PATCH 6/7] perf ui/tui: Reset output width for hierarchy

[Namhyung Kim]

On Mon, Sep 12, 2016 at 11:27:47AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Sep 12, 2016 at 03:19:57PM +0900, Namhyung Kim escreveu:
> > When --hierarchy option is used, each entry has its own hpp_list to show
> > the result.  But it missed to update width of each column.
> 
> Ditto, missing "Fixes:" tag.

It's the same commit.  Will add the tag.

Thanks,
Namhyung


>  
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/ui/browsers/hists.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> > index 35e44b1879e3..49db16334814 100644
> > --- a/tools/perf/ui/browsers/hists.c
> > +++ b/tools/perf/ui/browsers/hists.c
> > @@ -2067,6 +2067,7 @@ void hist_browser__init(struct hist_browser *browser,
> >  			struct hists *hists)
> >  {
> >  	struct perf_hpp_fmt *fmt;
> > +	struct perf_hpp_list_node *node;
> >  
> >  	browser->hists			= hists;
> >  	browser->b.refresh		= hist_browser__refresh;
> > @@ -2079,6 +2080,11 @@ void hist_browser__init(struct hist_browser *browser,
> >  		perf_hpp__reset_width(fmt, hists);
> >  		++browser->b.columns;
> >  	}
> > +	/* hierarchy entries have their own hpp list */
> > +	list_for_each_entry(node, &hists->hpp_formats, list) {
> > +		perf_hpp_list__for_each_format(&node->hpp, fmt)
> > +			perf_hpp__reset_width(fmt, hists);
> > +	}
> >  }
> >  
> >  struct hist_browser *hist_browser__new(struct hists *hists)
> > -- 
> > 2.9.3

Re: [PATCH 7/7] perf report: Enable group view with hierarchy

[Namhyung Kim]

On Mon, Sep 12, 2016 at 11:28:44AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Sep 12, 2016 at 03:19:58PM +0900, Namhyung Kim escreveu:
> > Now all missing pieces are implemented, let's enable it.  An example
> > output below:
> 
> So. what are those events? What was the command line that produced the
> 'perf.data' file that was used?

Oh, the command line was:

  $ perf record -e '{cycles,instructions}' make 

I'll add it to the changelog.

Thanks,
Namhyung


p.s. Actually the most overhead goes to the compiler (cc1) in this
data.  But I omitted it since it has so many unresolved symbol
addresses.  I think it's not a problem to demonstrate the hierarchy +
group feature.


> 
> - Arnaldo
>  
> >   $ perf report --hierarchy --stdio
> >   ...
> >   #               Overhead  Command / Shared Object / Symbol
> >   # ......................  ..................................
> >   #
> >       25.74%  27.18%        sh
> >          19.96%  24.14%        libc-2.24.so
> >             9.55%  14.64%        [.] __strcmp_sse2
> >             1.54%   0.00%        [.] __tfind
> >             1.07%   1.13%        [.] _int_malloc
> >             0.95%   0.00%        [.] __strchr_sse2
> >             0.89%   1.39%        [.] __tsearch
> >             0.76%   0.00%        [.] strlen
> >   ...
> > 
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/builtin-report.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> > index 1a07c4cdf6ed..6e88460cd13d 100644
> > --- a/tools/perf/builtin-report.c
> > +++ b/tools/perf/builtin-report.c
> > @@ -935,7 +935,6 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
> >  
> >  	if (symbol_conf.report_hierarchy) {
> >  		/* disable incompatible options */
> > -		symbol_conf.event_group = false;
> >  		symbol_conf.cumulate_callchain = false;
> >  
> >  		if (field_order) {
> > -- 
> > 2.9.3

[tip:perf/core] perf hists browser: Fix event group display

[tip-bot for Namhyung Kim]

Commit-ID:  d9ea48bc4e7cc297ca1073fa3f90ed80d964b7b4
Gitweb:     http://git.kernel.org/tip/d9ea48bc4e7cc297ca1073fa3f90ed80d964b7b4
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Mon, 12 Sep 2016 15:19:52 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 12 Sep 2016 11:10:26 -0300

perf hists browser: Fix event group display

Milian reported that the event group on TUI shows duplicated overhead.
This was due to a bug on calculating hpp->buf position.  The
hpp_advance() was called from __hpp__slsmg_color_printf() on TUI but
it's already called from the hpp__call_print_fn macro in __hpp__fmt().
The end result is that the print function returns number of bytes it
printed but the buffer advanced twice of the length.

This is generally not a problem since it doesn't need to access the
buffer again.  But with event group, overhead needs to be printed
multiple times and hist_entry__snprintf_alignment() tries to fill the
space with buffer after it printed.  So it (brokenly) showed the last
overhead again.

The bug was there from the beginning, but I think it's only revealed
when the alignment function was added.

Reported-by: Milian Wolff <milian.wolff@kdab.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Fixes: 89fee7094323 ("perf hists: Do column alignment on the format iterator")
Link: http://lkml.kernel.org/r/20160912061958.16656-2-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/ui/browsers/hists.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index f0611c9..35e44b1 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1097,7 +1097,6 @@ static int __hpp__slsmg_color_printf(struct perf_hpp *hpp, const char *fmt, ...)
 	ret = scnprintf(hpp->buf, hpp->size, fmt, len, percent);
 	ui_browser__printf(arg->b, "%s", hpp->buf);
 
-	advance_hpp(hpp, ret);
 	return ret;
 }