Skip to contentSkip to conversation

perf tools: Support hierarchy report with event group (v2)

PATCH In review Namhyung Kim 4 participants 21 messages Sep 13, 2016 – Sep 20, 2016 perf events

lore
Share & tools
lore

Namhyung Kim

author maintainer
#0 lore raw

cover perf tools: Support hierarchy report with event group (v2)

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(-)
Signature · 1 line
2.9.3
author maintainer
#1 lore raw

PATCH 1/7 perf hist: Introduce hists__match_hierarchy()

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.

---
 tools/perf/util/hist.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)
tools/perf/util/hist.c +51 -0
index de15dbcdcecf..be3f5ce31303 100644
@@ -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
Signature · 1 line
2.9.3
#2 lore raw

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

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

SNIP

Quoted 6 lines
tools/perf/util/hist.c +6 -0
+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) {

tools/perf/util/hist.c static struct hist_entry *hists__find_entry(struct hists *hists, · line 2192

Quoted 10 lines
tools/perf/util/hist.c +10 -0
+		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

Quoted 12 lines
tools/perf/util/hist.c +12 -0
+
+		if (cmp < 0)
+			n = n->rb_left;
+		else if (cmp > 0)
+			n = n->rb_right;
+		else
+			return iter;
+	}
+
+	return NULL;
+}
+
author maintainer
#3 lore raw

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

Hi Jiri,

Quoted 14 lines · Jiri Olsa

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

SNIP

Quoted 10 lines
tools/perf/util/hist.c +10 -0
+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;
+
Quoted 8 lines

tools/perf/util/hist.c static struct hist_entry *hists__find_entry(struct hists *hists, · line 2192

Quoted 6 lines
tools/perf/util/hist.c +6 -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

Quoted 13 lines
Quoted 13 lines
tools/perf/util/hist.c +12 -0
+
+             if (cmp < 0)
+                     n = n->rb_left;
+             else if (cmp > 0)
+                     n = n->rb_right;
+             else
+                     return iter;
+     }
+
+     return NULL;
+}
+
Signature · 2 lines
Thanks,
Namhyung
#4 lore raw

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

In reply to Namhyung Kim
perf hists: Introduce hists__match_hierarchy()
Commit-ID:  09034de63e427a86ba96bedf39410eef7c9014a5
Gitweb:     http://git.kernel.org/tip/09034de63e427a86ba96bedf39410eef7c9014a5
Author:     Namhyung Kim <[email protected]>
AuthorDate: Tue, 13 Sep 2016 16:45:46 +0900
Committer:  Arnaldo Carvalho de Melo <[email protected]>
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.

---
 tools/perf/util/hist.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)
tools/perf/util/hist.c +51 -0
index de15dbc..be3f5ce 100644
@@ -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
author maintainer
#5 lore raw

PATCH 2/7 perf hist: Introduce hists__link_hierarchy()

In reply to 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.

---
 tools/perf/util/hist.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 95 insertions(+)
tools/perf/util/hist.c +95 -0
index be3f5ce31303..702ba3a8ead6 100644
@@ -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
Signature · 1 line
2.9.3
#6 lore raw

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

Quoted 4 lines · 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.

SNIP

Quoted 16 lines
tools/perf/util/hist.c +15 -0
+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) {

tools/perf/util/hist.c void hists__match(struct hists *leader, struct hists *other) · line 2319

Quoted 10 lines
tools/perf/util/hist.c +10 -0
+				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

author maintainer
#7 lore raw

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

Quoted 29 lines · Jiri Olsa
Quoted 4 lines · 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.

SNIP

Quoted 21 lines
tools/perf/util/hist.c +20 -0
+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;
+                             }
+                     }
Quoted 8 lines

tools/perf/util/hist.c void hists__match(struct hists *leader, struct hists *other) · line 2319

Quoted 5 lines
tools/perf/util/hist.c +5 -0
+                     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

#8 lore raw

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

In reply to Namhyung Kim
perf hists: Introduce hists__link_hierarchy()
Commit-ID:  9d97b8f512a0dd41819b8e3d9cdc7a59199e1b0c
Gitweb:     http://git.kernel.org/tip/9d97b8f512a0dd41819b8e3d9cdc7a59199e1b0c
Author:     Namhyung Kim <[email protected]>
AuthorDate: Tue, 13 Sep 2016 16:45:47 +0900
Committer:  Arnaldo Carvalho de Melo <[email protected]>
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.

---
 tools/perf/util/hist.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 95 insertions(+)
tools/perf/util/hist.c +95 -0
index be3f5ce..702ba3a 100644
@@ -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
author maintainer
#9 lore raw

PATCH 3/7 perf hist: Initialize hierachy tree explicitly

In reply to 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.

---
 tools/perf/util/hist.c | 2 ++
 1 file changed, 2 insertions(+)
tools/perf/util/hist.c +2 -0
index 702ba3a8ead6..37a08f20730a 100644
@@ -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;
Signature · 1 line
2.9.3
#10 lore raw

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

perf hist: Initialize hierarchy tree explicitly
Commit-ID:  d2580c7a5b4e78bffda1e53cfd583e7a2c7383a5
Gitweb:     http://git.kernel.org/tip/d2580c7a5b4e78bffda1e53cfd583e7a2c7383a5
Author:     Namhyung Kim <[email protected]>
AuthorDate: Tue, 13 Sep 2016 16:45:48 +0900
Committer:  Arnaldo Carvalho de Melo <[email protected]>
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.

---
 tools/perf/util/hist.c | 2 ++
 1 file changed, 2 insertions(+)
tools/perf/util/hist.c +2 -0
index 702ba3a..37a08f2 100644
@@ -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;
author maintainer
#11 lore raw

PATCH 4/7 perf ui/stdio: Always reset output width for hierarchy

In reply to 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")
---
 tools/perf/ui/stdio/hist.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)
tools/perf/ui/stdio/hist.c +6 -8
index 9b65f4a6b35a..18b4fd9342cd 100644
@@ -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);
Signature · 1 line
2.9.3
#12 lore raw

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

perf ui/stdio: Always reset output width for hierarchy
Commit-ID:  9a6ad25b5a2026ba1399abc879ec623957867e79
Gitweb:     http://git.kernel.org/tip/9a6ad25b5a2026ba1399abc879ec623957867e79
Author:     Namhyung Kim <[email protected]>
AuthorDate: Tue, 13 Sep 2016 16:45:49 +0900
Committer:  Arnaldo Carvalho de Melo <[email protected]>
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.

Cc Andi Kleen <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Peter Zijlstra <[email protected]>
Fixes 1b2dbbf41a0f ("perf hists: Use own hpp_list for hierarchy mode")
---
 tools/perf/ui/stdio/hist.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)
tools/perf/ui/stdio/hist.c +6 -8
index 9b65f4a..18b4fd9 100644
@@ -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);
author maintainer
#13 lore raw

PATCH 5/7 perf ui/stdio: Rename print_hierarchy_header()

In reply to Namhyung Kim

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

---
 tools/perf/ui/stdio/hist.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)
tools/perf/ui/stdio/hist.c +3 -10
index 18b4fd9342cd..a57131e61fe3 100644
@@ -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)
 {
Signature · 1 line
2.9.3
#14 lore raw

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

perf ui/stdio: Rename print_hierarchy_header()
Commit-ID:  195bc0f8443d8d564ae95d2e9c19ac0edfd647c3
Gitweb:     http://git.kernel.org/tip/195bc0f8443d8d564ae95d2e9c19ac0edfd647c3
Author:     Namhyung Kim <[email protected]>
AuthorDate: Tue, 13 Sep 2016 16:45:50 +0900
Committer:  Arnaldo Carvalho de Melo <[email protected]>
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.

---
 tools/perf/ui/stdio/hist.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)
tools/perf/ui/stdio/hist.c +3 -10
index 18b4fd9..a57131e 100644
@@ -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)
 {
author maintainer
#15 lore raw

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

In reply to 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")
---
 tools/perf/ui/browsers/hists.c | 6 ++++++
 1 file changed, 6 insertions(+)
tools/perf/ui/browsers/hists.c +6 -0
index 35e44b1879e3..49db16334814 100644
@@ -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)
Signature · 1 line
2.9.3
#16 lore raw

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

Quoted 22 lines · 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")
---
 tools/perf/ui/browsers/hists.c | 6 ++++++
 1 file changed, 6 insertions(+)
tools/perf/ui/browsers/hists.c +1 -0
index 35e44b1879e3..49db16334814 100644
@@ -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;

tools/perf/ui/browsers/hists.c void hist_browser__init(struct hist_browser *browser, · line 2087

Quoted 8 lines
tools/perf/ui/browsers/hists.c +5 -0
@@ -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

author maintainer
#17 lore raw

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

Quoted 20 lines · Jiri Olsa
Quoted 19 lines · 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")
---
 tools/perf/ui/browsers/hists.c | 6 ++++++
 1 file changed, 6 insertions(+)
tools/perf/ui/browsers/hists.c +1 -0
index 35e44b1879e3..49db16334814 100644
@@ -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;
 
Quoted 14 lines

tools/perf/ui/browsers/hists.c void hist_browser__init(struct hist_browser *browser, · line 2087

Quoted 11 lines
tools/perf/ui/browsers/hists.c +5 -0
      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

#18 lore raw

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

Em Tue, Sep 20, 2016 at 10:18:50AM +0900, Namhyung Kim escreveu:

Quoted 8 lines · Jiri Olsa
Quoted 7 lines · Namhyung Kim
Quoted 5 lines
tools/perf/ui/browsers/hists.c +5 -0
+     /* 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);
+     }
Quoted 4 lines
Quoted 2 lines

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

author maintainer
#19 lore raw

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

In reply to 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
  ...
---
 tools/perf/builtin-report.c | 1 -
 1 file changed, 1 deletion(-)
tools/perf/builtin-report.c +0 -1
index 1a07c4cdf6ed..6e88460cd13d 100644
@@ -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) {
Signature · 1 line
2.9.3
#20 lore raw

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

perf report: Enable group view with hierarchy
Commit-ID:  30d476ae738d1ce33f170dd79398ecd211274df6
Gitweb:     http://git.kernel.org/tip/30d476ae738d1ce33f170dd79398ecd211274df6
Author:     Namhyung Kim <[email protected]>
AuthorDate: Tue, 13 Sep 2016 16:45:52 +0900
Committer:  Arnaldo Carvalho de Melo <[email protected]>
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
  ...
---
 tools/perf/builtin-report.c | 1 -
 1 file changed, 1 deletion(-)
tools/perf/builtin-report.c +0 -1
index 1a07c4c..6e88460 100644
@@ -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) {