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

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

[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.

 * changes in v2)
  - the first bug fix patch applied to acme/perf/core
  - update changelog, add Fixes: tags  (Arnaldo)
  - rename print_hierarchy_header
  
For example, following command line used to show each event
separately.  As event group view is enabled by default, now hierarchy
mode shows the two event together like below:

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

  $ perf report --hierarchy --stdio
  ...
  #               Overhead  Command / Shared Object / Symbol
  # ......................  ..................................
  #
      89.22%   87.73%       cc1
         80.69%  79.46%        cc1
             1.41%   2.00%        [.] _cpp_lex_direct
             1.26%   1.70%        [.] bitmap_set_bit
             1.11%   1.13%        [.] ggc_internal_alloc
	     ...
          0.25%   0.01%        [kernel.vmlinux]
             0.23%   0.01%        [k] page_fault
             0.01%   0.00%        [k] apic_timer_interrupt
             0.01%   0.00%        [k] entry_SYSCALL_64
	  ...
       5.03%   5.97%        as
          3.10%   3.84%        as
             0.14%   0.21%        [.] 0x00000000000114f4
             0.12%   0.01%        [.] 0x0000000000011510
             0.04%   0.00%        [.] 0x000000000001c639
             0.04%   0.00%        [.] 0x000000000000cff6
	     ...
       3.32%   3.19%        ld
          2.34%   2.13%        libbfd-2.27.so
             0.50%   0.25%        [.] bfd_link_hash_traverse
             0.42%   0.30%        [.] bfd_elf_link_add_symbols
             0.41%   0.59%        [.] bfd_hash_lookup
             0.09%   0.04%        [.] bfd_hash_insert
        ...

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

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

Thanks,
Namhyung


Namhyung Kim (7):
  perf hist: Introduce hists__match_hierarchy()
  perf hist: Introduce hists__link_hierarchy()
  perf hist: Initialize hierachy tree explicitly
  perf ui/stdio: Always reset output width for hierarchy
  perf ui/stdio: Rename print_hierarchy_header()
  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 |   6 ++
 tools/perf/ui/stdio/hist.c     |  27 +++-----
 tools/perf/util/hist.c         | 148 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 163 insertions(+), 19 deletions(-)

-- 
2.9.3

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

[Namhyung Kim]

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 would look like 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.

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 2/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 3/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 4/7] perf ui/stdio: Always 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 for perf-top.
The perf-report has no problem since it resets during header display.

  $ 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

Move the code into hists__fprintf() so that it can be called always.
Also it'd be better to put similar code together.

Fixes: 1b2dbbf41a0f ("perf hists: Use own hpp_list for hierarchy mode")
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/stdio/hist.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/tools/perf/ui/stdio/hist.c b/tools/perf/ui/stdio/hist.c
index 9b65f4a6b35a..18b4fd9342cd 100644
--- a/tools/perf/ui/stdio/hist.c
+++ b/tools/perf/ui/stdio/hist.c
@@ -628,14 +628,6 @@ hists__fprintf_hierarchy_headers(struct hists *hists,
 				 struct perf_hpp *hpp,
 				 FILE *fp)
 {
-	struct perf_hpp_list_node *fmt_node;
-	struct perf_hpp_fmt *fmt;
-
-	list_for_each_entry(fmt_node, &hists->hpp_formats, list) {
-		perf_hpp_list__for_each_format(&fmt_node->hpp, fmt)
-			perf_hpp__reset_width(fmt, hists);
-	}
-
 	return print_hierarchy_header(hists, hpp, symbol_conf.field_sep, fp);
 }
 
@@ -733,6 +725,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 +738,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 5/7] perf ui/stdio: Rename print_hierarchy_header()

[Namhyung Kim]

Now the hists__fprintf_hierarchy_headers() is a simple wrapper passing
field separator.  Let's do it directly.

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

diff --git a/tools/perf/ui/stdio/hist.c b/tools/perf/ui/stdio/hist.c
index 18b4fd9342cd..a57131e61fe3 100644
--- a/tools/perf/ui/stdio/hist.c
+++ b/tools/perf/ui/stdio/hist.c
@@ -528,8 +528,8 @@ static int print_hierarchy_indent(const char *sep, int indent,
 	return fprintf(fp, "%-.*s", (indent - 2) * HIERARCHY_INDENT, line);
 }
 
-static int print_hierarchy_header(struct hists *hists, struct perf_hpp *hpp,
-				  const char *sep, FILE *fp)
+static int hists__fprintf_hierarchy_headers(struct hists *hists,
+					    struct perf_hpp *hpp, FILE *fp)
 {
 	bool first_node, first_col;
 	int indent;
@@ -538,6 +538,7 @@ static int print_hierarchy_header(struct hists *hists, struct perf_hpp *hpp,
 	unsigned header_width = 0;
 	struct perf_hpp_fmt *fmt;
 	struct perf_hpp_list_node *fmt_node;
+	const char *sep = symbol_conf.field_sep;
 
 	indent = hists->nr_hpp_node;
 
@@ -623,14 +624,6 @@ static int print_hierarchy_header(struct hists *hists, struct perf_hpp *hpp,
 	return 2;
 }
 
-static int
-hists__fprintf_hierarchy_headers(struct hists *hists,
-				 struct perf_hpp *hpp,
-				 FILE *fp)
-{
-	return print_hierarchy_header(hists, hpp, symbol_conf.field_sep, fp);
-}
-
 static void fprintf_line(struct hists *hists, struct perf_hpp *hpp,
 			 int line, FILE *fp)
 {
-- 
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.

Fixes: 1b2dbbf41a0f ("perf hists: Use own hpp_list for hierarchy mode")
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 record -e '{cycles,instructions}' make

  $ 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 1/7] perf hist: Introduce hists__match_hierarchy()

[Jiri Olsa]

On Tue, Sep 13, 2016 at 04:45:46PM +0900, Namhyung Kim wrote:

SNIP

> +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;
> +		}

could you call hist_entry__collapse in here instead of above code?

thanks,
jirka

> +
> +		if (cmp < 0)
> +			n = n->rb_left;
> +		else if (cmp > 0)
> +			n = n->rb_right;
> +		else
> +			return iter;
> +	}
> +
> +	return NULL;
> +}
> +

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

[Jiri Olsa]

On Tue, Sep 13, 2016 at 04:45:47PM +0900, Namhyung Kim wrote:
> 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.

SNIP

>  
> +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);

could we call hists__add_dummy_entry in here? seems like there's only
difference in function arguments.. we could safe one function ;-)

thanks,
jirka

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

[Jiri Olsa]

On Tue, Sep 13, 2016 at 04:45:51PM +0900, Namhyung Kim wrote:
> 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.
> 
> Fixes: 1b2dbbf41a0f ("perf hists: Use own hpp_list for hierarchy mode")
> 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);
> +	}

we could add new function hists__reset_width and use it from
both std and tui code

thanks,
jirka

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

[Namhyung Kim]

Hi Jiri,

On Mon, Sep 19, 2016 at 4:48 PM, Jiri Olsa <jolsa@redhat.com> wrote:
> On Tue, Sep 13, 2016 at 04:45:46PM +0900, Namhyung Kim wrote:
>
> SNIP
>
>> +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;
>> +             }
>
> could you call hist_entry__collapse in here instead of above code?

Nope.  The hist_entry__collapse() traverses the whole sort list of the
event (hists->hpp_list) while this function only traverses its own
sort list (he->hpp_list).  These are different when hierarchy is used.

Thanks,
Namhyung

>
>> +
>> +             if (cmp < 0)
>> +                     n = n->rb_left;
>> +             else if (cmp > 0)
>> +                     n = n->rb_right;
>> +             else
>> +                     return iter;
>> +     }
>> +
>> +     return NULL;
>> +}
>> +



-- 
Thanks,
Namhyung

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

[Namhyung Kim]

On Mon, Sep 19, 2016 at 4:59 PM, Jiri Olsa <jolsa@redhat.com> wrote:
> On Tue, Sep 13, 2016 at 04:45:47PM +0900, Namhyung Kim wrote:
>> 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.
>
> SNIP
>
>>
>> +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);
>
> could we call hists__add_dummy_entry in here? seems like there's only
> difference in function arguments.. we could safe one function ;-)

Maybe, but I don't see much value doing it.  IMHO this hists
match/link thing is already complex.  Combining normal and hierarchy
case in a single function will increase the complexity so I'd like to
keep it separate for hierarchy case.

Thanks,
Namhyung

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

[Namhyung Kim]

On Mon, Sep 19, 2016 at 5:05 PM, Jiri Olsa <jolsa@redhat.com> wrote:
> On Tue, Sep 13, 2016 at 04:45:51PM +0900, Namhyung Kim wrote:
>> 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.
>>
>> Fixes: 1b2dbbf41a0f ("perf hists: Use own hpp_list for hierarchy mode")
>> 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);
>> +     }
>
> we could add new function hists__reset_width and use it from
> both std and tui code

Will do as a follow-up patch!

Thanks,
Namhyung

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

[Arnaldo Carvalho de Melo]

Em Tue, Sep 20, 2016 at 10:18:50AM +0900, Namhyung Kim escreveu:
> On Mon, Sep 19, 2016 at 5:05 PM, Jiri Olsa <jolsa@redhat.com> wrote:
> > On Tue, Sep 13, 2016 at 04:45:51PM +0900, Namhyung Kim wrote:
> >> +     /* 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);
> >> +     }
> >
> > we could add new function hists__reset_width and use it from
> > both std and tui code
> 
> Will do as a follow-up patch!

Great, I saw no problems with the patches, merged them already, should
be on their way to Ingo as soon as my test suite passes.

- Arnaldo

[tip:perf/core] perf hists: Introduce hists__match_hierarchy()

[tip-bot for Namhyung Kim]

Commit-ID:  09034de63e427a86ba96bedf39410eef7c9014a5
Gitweb:     http://git.kernel.org/tip/09034de63e427a86ba96bedf39410eef7c9014a5
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Tue, 13 Sep 2016 16:45:46 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 13 Sep 2016 16:31:24 -0300

perf hists: Introduce hists__match_hierarchy()

The hists__match_hierarchy() is to find matching hist entries in a
group.  A matching entry has the same values for all sort keys given.

With an event group (e.g.: -e "{cycles,instructions}"), 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 looks like:

  #               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 overheads are shown - one for the leader and
another for the other group member.  They were matched since their
command, dso and symbol have the same values.

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>
Link: http://lkml.kernel.org/r/20160913074552.13284-2-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 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 de15dbc..be3f5ce 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

[tip:perf/core] perf hists: Introduce hists__link_hierarchy()

[tip-bot for Namhyung Kim]

Commit-ID:  9d97b8f512a0dd41819b8e3d9cdc7a59199e1b0c
Gitweb:     http://git.kernel.org/tip/9d97b8f512a0dd41819b8e3d9cdc7a59199e1b0c
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Tue, 13 Sep 2016 16:45:47 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 13 Sep 2016 16:35:46 -0300

perf hists: Introduce hists__link_hierarchy()

The hists__link_hierarchy() is to support hierarchy reports with an
event group.  When it matches the leader event and the other members
(using 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>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/20160913074552.13284-3-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 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 be3f5ce..702ba3a 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -2149,6 +2149,50 @@ out:
 	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

[tip:perf/core] perf hist: Initialize hierarchy tree explicitly

[tip-bot for Namhyung Kim]

Commit-ID:  d2580c7a5b4e78bffda1e53cfd583e7a2c7383a5
Gitweb:     http://git.kernel.org/tip/d2580c7a5b4e78bffda1e53cfd583e7a2c7383a5
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Tue, 13 Sep 2016 16:45:48 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 13 Sep 2016 16:36:46 -0300

perf hist: Initialize hierarchy tree explicitly

The hroot_in and hroot_out are roots of hierarchy trees of hist entries.

But when a hist entry is initialized by copying existing template entry,
it sometimes has non-empty tree and copies it incorrectly.  This is a
problem especially when an 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>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/20160913074552.13284-4-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 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 702ba3a..37a08f2 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;

[tip:perf/core] perf ui/stdio: Always reset output width for hierarchy

[tip-bot for Namhyung Kim]

Commit-ID:  9a6ad25b5a2026ba1399abc879ec623957867e79
Gitweb:     http://git.kernel.org/tip/9a6ad25b5a2026ba1399abc879ec623957867e79
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Tue, 13 Sep 2016 16:45:49 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 13 Sep 2016 16:41:21 -0300

perf ui/stdio: Always reset output width for hierarchy

When the --hierarchy option is used, each entry has its own hpp_list to
show the result.  But it is not updating the width of each column for
perf-top.  The perf-report command has no problem since it resets it
during header display.

  $ 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

Move the code into hists__fprintf() so that it can be called always.
Also it'd be better to put similar code together.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Fixes: 1b2dbbf41a0f ("perf hists: Use own hpp_list for hierarchy mode")
Link: http://lkml.kernel.org/r/20160913074552.13284-5-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/ui/stdio/hist.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/tools/perf/ui/stdio/hist.c b/tools/perf/ui/stdio/hist.c
index 9b65f4a..18b4fd9 100644
--- a/tools/perf/ui/stdio/hist.c
+++ b/tools/perf/ui/stdio/hist.c
@@ -628,14 +628,6 @@ hists__fprintf_hierarchy_headers(struct hists *hists,
 				 struct perf_hpp *hpp,
 				 FILE *fp)
 {
-	struct perf_hpp_list_node *fmt_node;
-	struct perf_hpp_fmt *fmt;
-
-	list_for_each_entry(fmt_node, &hists->hpp_formats, list) {
-		perf_hpp_list__for_each_format(&fmt_node->hpp, fmt)
-			perf_hpp__reset_width(fmt, hists);
-	}
-
 	return print_hierarchy_header(hists, hpp, symbol_conf.field_sep, fp);
 }
 
@@ -733,6 +725,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 +738,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);

[tip:perf/core] perf ui/stdio: Rename print_hierarchy_header()

[tip-bot for Namhyung Kim]

Commit-ID:  195bc0f8443d8d564ae95d2e9c19ac0edfd647c3
Gitweb:     http://git.kernel.org/tip/195bc0f8443d8d564ae95d2e9c19ac0edfd647c3
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Tue, 13 Sep 2016 16:45:50 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 13 Sep 2016 16:41:36 -0300

perf ui/stdio: Rename print_hierarchy_header()

Now the hists__fprintf_hierarchy_headers() is a simple wrapper passing
field separator.  Let's do it directly.

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>
Link: http://lkml.kernel.org/r/20160913074552.13284-6-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/ui/stdio/hist.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/tools/perf/ui/stdio/hist.c b/tools/perf/ui/stdio/hist.c
index 18b4fd9..a57131e 100644
--- a/tools/perf/ui/stdio/hist.c
+++ b/tools/perf/ui/stdio/hist.c
@@ -528,8 +528,8 @@ static int print_hierarchy_indent(const char *sep, int indent,
 	return fprintf(fp, "%-.*s", (indent - 2) * HIERARCHY_INDENT, line);
 }
 
-static int print_hierarchy_header(struct hists *hists, struct perf_hpp *hpp,
-				  const char *sep, FILE *fp)
+static int hists__fprintf_hierarchy_headers(struct hists *hists,
+					    struct perf_hpp *hpp, FILE *fp)
 {
 	bool first_node, first_col;
 	int indent;
@@ -538,6 +538,7 @@ static int print_hierarchy_header(struct hists *hists, struct perf_hpp *hpp,
 	unsigned header_width = 0;
 	struct perf_hpp_fmt *fmt;
 	struct perf_hpp_list_node *fmt_node;
+	const char *sep = symbol_conf.field_sep;
 
 	indent = hists->nr_hpp_node;
 
@@ -623,14 +624,6 @@ static int print_hierarchy_header(struct hists *hists, struct perf_hpp *hpp,
 	return 2;
 }
 
-static int
-hists__fprintf_hierarchy_headers(struct hists *hists,
-				 struct perf_hpp *hpp,
-				 FILE *fp)
-{
-	return print_hierarchy_header(hists, hpp, symbol_conf.field_sep, fp);
-}
-
 static void fprintf_line(struct hists *hists, struct perf_hpp *hpp,
 			 int line, FILE *fp)
 {

[tip:perf/core] perf report: Enable group view with hierarchy

[tip-bot for Namhyung Kim]

Commit-ID:  30d476ae738d1ce33f170dd79398ecd211274df6
Gitweb:     http://git.kernel.org/tip/30d476ae738d1ce33f170dd79398ecd211274df6
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Tue, 13 Sep 2016 16:45:52 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 13 Sep 2016 16:43:41 -0300

perf report: Enable group view with hierarchy

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

  $ perf record -e '{cycles,instructions}' make
  $ 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>
Requested-by: Andi Kleen <andi@firstfloor.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/20160913074552.13284-8-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 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 1a07c4c..6e88460 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -935,7 +935,6 @@ repeat:
 
 	if (symbol_conf.report_hierarchy) {
 		/* disable incompatible options */
-		symbol_conf.event_group = false;
 		symbol_conf.cumulate_callchain = false;
 
 		if (field_order) {

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

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

[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