[RFC/PATCHSET 00/17] perf tools: Add support for hierachy view (v2)

[RFC/PATCHSET 00/17] perf tools: Add support for hierachy view (v2)

[Namhyung Kim]

Hello,

This is v2 attempt of my earlier patchset [1].  This patchset
implements a new feature that collects hist entries in a hierachical
manner.  That means lower-level entries belong to an upper-level
entry.  The entry hierachy is built on the sort keys given, so users
can set it whatever they want.  It only shows top-level entries first,
and user can expand/collapse it dynamically.

This time I implemented it for every output browser including TUI.
A screenshot on TUI looks like below:

For normal output:

  $ perf report --tui
  Samples: 3K of event 'cycles:pp', Event count (approx.): 1695979674
    Overhead  Command        Shared Object         Symbol
  ------------------------------------------------------------------------
  -    7.57%  swapper        [kernel.vmlinux]      [k] intel_idle
       intel_idle
       cpuidle_enter_state
       cpuidle_enter
       call_cpuidle
     + cpu_startup_entry
  +    1.16   firefox        firefox               [.] 0x00000000000019433
  +    0.97%  firefox        libpthread-2.22.so    [.] pthread_mutex_lock
  ...


With hierarchy view,

  $ perf report --tui --hierarchy
  Samples: 3K of event 'cycles:pp', Event count (approx.): 1695979674
   Overhead        Command / Shared Object / Symbol
  -------------------------------------------------------------------
  +  76.30%        firefox
  -   9.95%        swapper
     -   9.51%        [kernel.vmlinux]
        -   7.57         [k] intel_idle
	     intel_idle
	     cpuidle_enter_state
	     cpuidle_enter
	     call_cpuidle
	   + cpu_startup_entry
	+   0.15%        [k] __schedule
	+   0.12%        [k] menu_select
	...
     +   0.34%        [sdhci]
     +   0.06%        [e1000e]
     ...
 +    5.65%        Xorg
 +    5.42%        Socket Thread
 ...

As you can see, overhead of an upper level entry is the sum of
overhead of lower level entries.  The entries are aligned by its order
of matching sort keys.

This is available from 'perf/hierarchy-v2' branch in my tree:

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


Any comments are welcome, thanks!
Namhyung


Cc: Don Zickus <dzickus@redhat.com>
Cc: Pekka Enberg <penberg@kernel.org>
Cc: Moinuddin Quadri <moin18@gmail.com>


[1] https://lkml.org/lkml/2013/5/21/24


Namhyung Kim (17):
  perf hists: Basic support of hierarchical report view
  perf hists: Resort hist entries with hierarchy
  perf hists: Add helper functions for hierarchy mode
  perf hists: Cleanup filtering functions
  perf hists: Support filtering in hierarchy mode
  perf ui/stdio: Implement hierarchy output mode
  perf ui/stdio: Align column header for hierarchy output
  perf hists browser: Fix context menu item
  perf hists browser: Count number of hierarchy entries
  perf hists browser: Support collapsing/expanding whole entries in
    hierarchy
  perf hists browser: Factor out hist_browser__show_callchain()
  perf hists browser: Implement hierarchy output
  perf hists browser: Align column header in hierarchy mode
  perf ui/gtk: Implement hierarchy output mode
  perf report: Add --hierarchy option
  perf hists: Support decaying in hierarchy mode
  perf top: Add --hierarchy option

 tools/perf/Documentation/perf-report.txt |   3 +
 tools/perf/Documentation/perf-top.txt    |   3 +
 tools/perf/Documentation/tips.txt        |   1 +
 tools/perf/builtin-report.c              |  17 +
 tools/perf/builtin-top.c                 |  15 +
 tools/perf/ui/browsers/hists.c           | 590 ++++++++++++++++++++++++++-----
 tools/perf/ui/gtk/hists.c                | 161 ++++++++-
 tools/perf/ui/hist.c                     |  14 +
 tools/perf/ui/stdio/hist.c               | 182 +++++++++-
 tools/perf/util/hist.c                   | 470 ++++++++++++++++++++----
 tools/perf/util/hist.h                   |  11 +
 tools/perf/util/sort.c                   | 116 ++++++
 tools/perf/util/sort.h                   |  16 +-
 tools/perf/util/symbol.h                 |   3 +-
 14 files changed, 1437 insertions(+), 165 deletions(-)

-- 
2.6.4

[PATCH 01/17] perf hists: Basic support of hierarchical report view

[Namhyung Kim]

In the hierarchical view, entries will be grouped and sorted on the
first key, and then second key, and so on.  Add he->hroot_{in,out} field
to keep lower level entries. Actually this can be shared with callchain
sorted_root since the hroots are only used by non-leaf entries and
callchain is only used by leaf entries.

It also adds parent_he and depth fields which can be used by browsers.

This patch only implements collapsing part which creates internal
entries for each sort key.  These need to be sorted by output_sort stage
and to be displayed properly in the later patch(es).

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/hist.c   | 93 ++++++++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/sort.h   | 13 ++++++-
 tools/perf/util/symbol.h |  3 +-
 3 files changed, 107 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index c226303e3da0..70293eb70430 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -1007,6 +1007,95 @@ void hist_entry__delete(struct hist_entry *he)
  * collapse the histogram
  */
 
+static void hists__apply_filters(struct hists *hists, struct hist_entry *he);
+
+static struct hist_entry *hierarchy_insert_entry(struct hists *hists,
+						 struct rb_root *root,
+						 struct hist_entry *he,
+						 struct perf_hpp_fmt *fmt)
+{
+	struct rb_node **p = &root->rb_node;
+	struct rb_node *parent = NULL;
+	struct hist_entry *iter, *new;
+	int64_t cmp;
+
+	while (*p != NULL) {
+		parent = *p;
+		iter = rb_entry(parent, struct hist_entry, rb_node_in);
+
+		cmp = fmt->collapse(fmt, iter, he);
+		if (!cmp) {
+			he_stat__add_stat(&iter->stat, &he->stat);
+			return iter;
+		}
+
+		if (cmp < 0)
+			p = &parent->rb_left;
+		else
+			p = &parent->rb_right;
+	}
+
+	new = hist_entry__new(he, true);
+	if (new == NULL)
+		return false;
+
+	hists__apply_filters(hists, new);
+	hists->nr_entries++;
+
+	/* save related format for output */
+	new->fmt = fmt;
+
+	/* it's now passed to 'new' */
+	he->trace_output = NULL;
+
+	rb_link_node(&new->rb_node_in, parent, p);
+	rb_insert_color(&new->rb_node_in, root);
+	return new;
+}
+
+static bool hists__hierarchy_insert_entry(struct hists *hists,
+					  struct rb_root *root,
+					  struct hist_entry *he)
+{
+	struct perf_hpp_fmt *fmt;
+	struct hist_entry *new = NULL;
+	struct hist_entry *parent = NULL;
+	int depth = 0;
+
+	perf_hpp__for_each_sort_list(fmt) {
+		if (!perf_hpp__is_sort_entry(fmt) &&
+		    !perf_hpp__is_dynamic_entry(fmt))
+			continue;
+
+		/* insert copy of 'he' for each fmt into the hierarchy */
+		new = hierarchy_insert_entry(hists, root, he, fmt);
+		if (new == NULL)
+			break;
+
+		root = &new->hroot_in;
+		new->parent_he = parent;
+		new->depth = depth++;
+		parent = new;
+	}
+
+	if (new) {
+		new->leaf = true;
+
+		if (symbol_conf.use_callchain) {
+			callchain_cursor_reset(&callchain_cursor);
+			callchain_merge(&callchain_cursor,
+					new->callchain,
+					he->callchain);
+		}
+	}
+
+	/* 'he' is no longer used */
+	hist_entry__delete(he);
+
+	/* it already applied filters */
+	return false;
+}
+
 bool hists__collapse_insert_entry(struct hists *hists __maybe_unused,
 				  struct rb_root *root, struct hist_entry *he)
 {
@@ -1015,6 +1104,9 @@ bool hists__collapse_insert_entry(struct hists *hists __maybe_unused,
 	struct hist_entry *iter;
 	int64_t cmp;
 
+	if (symbol_conf.report_hierarchy)
+		return hists__hierarchy_insert_entry(hists, root, he);
+
 	while (*p != NULL) {
 		parent = *p;
 		iter = rb_entry(parent, struct hist_entry, rb_node_in);
@@ -1042,6 +1134,7 @@ bool hists__collapse_insert_entry(struct hists *hists __maybe_unused,
 			p = &(*p)->rb_right;
 	}
 	hists->nr_entries++;
+	he->leaf = true;
 
 	rb_link_node(&he->rb_node_in, parent, p);
 	rb_insert_color(&he->rb_node_in, root);
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index 687bbb124428..15a75d44de91 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -94,9 +94,11 @@ struct hist_entry {
 	s32			socket;
 	s32			cpu;
 	u8			cpumode;
+	u8			depth;
 
 	/* We are added by hists__add_dummy_entry. */
 	bool			dummy;
+	bool			leaf;
 
 	char			level;
 	u8			filtered;
@@ -118,13 +120,22 @@ struct hist_entry {
 	char			*srcline;
 	char			*srcfile;
 	struct symbol		*parent;
-	struct rb_root		sorted_chain;
 	struct branch_info	*branch_info;
 	struct hists		*hists;
 	struct mem_info		*mem_info;
 	void			*raw_data;
 	u32			raw_size;
 	void			*trace_output;
+	struct perf_hpp_fmt	*fmt;
+	struct hist_entry	*parent_he;
+	union {
+		/* this is for hierarchical entry structure */
+		struct {
+			struct rb_root	hroot_in;
+			struct rb_root  hroot_out;
+		};				/* non-leaf entries */
+		struct rb_root	sorted_chain;	/* leaf entry has callchains */
+	};
 	struct callchain_root	callchain[0]; /* must be last member */
 };
 
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index ccd1caa40e11..a937053a0ae0 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -110,7 +110,8 @@ struct symbol_conf {
 			has_filter,
 			show_ref_callgraph,
 			hide_unresolved,
-			raw_trace;
+			raw_trace,
+			report_hierarchy;
 	const char	*vmlinux_name,
 			*kallsyms_name,
 			*source_prefix,
-- 
2.6.4

Re: [PATCH 01/17] perf hists: Basic support of hierarchical report view

[Jiri Olsa]

On Sun, Jan 17, 2016 at 01:03:01AM +0900, Namhyung Kim wrote:

SNIP

>  	char			*srcfile;
>  	struct symbol		*parent;
> -	struct rb_root		sorted_chain;
>  	struct branch_info	*branch_info;
>  	struct hists		*hists;
>  	struct mem_info		*mem_info;
>  	void			*raw_data;
>  	u32			raw_size;
>  	void			*trace_output;
> +	struct perf_hpp_fmt	*fmt;
> +	struct hist_entry	*parent_he;
> +	union {
> +		/* this is for hierarchical entry structure */
> +		struct {
> +			struct rb_root	hroot_in;
> +			struct rb_root  hroot_out;
> +		};				/* non-leaf entries */
> +		struct rb_root	sorted_chain;	/* leaf entry has callchains */
> +	};

looks like cool feature!

could we have the hist_entry storage little more generic?
and maybe dynamically allocated?

I'm doing the same thing for the c2c stuff, like having
for each hist_entry its own 'struct hists' object, which
records data related to parent hist_entry

maybe we could strip the 'hists' object to some bare minimum
which is needed for store/sort/stat and display entries
in hists_browser ;-)

I'm preparing RFC patchset in my perf/c2c branch, if you want
to take a look

however, as I said above, for my own sake it all boils down
to have 'hists' object within hist_entry, so I can use it
in the UI code easily

FYI I also added support for hists object's own sort/output
format lists.. which I'll curve out and send for review soon

thanks,
jirka

Re: [PATCH 01/17] perf hists: Basic support of hierarchical report view

[Namhyung Kim]

Hi Jiri,

On Sun, Jan 17, 2016 at 05:15:33PM +0100, Jiri Olsa wrote:
> On Sun, Jan 17, 2016 at 01:03:01AM +0900, Namhyung Kim wrote:
> 
> SNIP
> 
> >  	char			*srcfile;
> >  	struct symbol		*parent;
> > -	struct rb_root		sorted_chain;
> >  	struct branch_info	*branch_info;
> >  	struct hists		*hists;
> >  	struct mem_info		*mem_info;
> >  	void			*raw_data;
> >  	u32			raw_size;
> >  	void			*trace_output;
> > +	struct perf_hpp_fmt	*fmt;
> > +	struct hist_entry	*parent_he;
> > +	union {
> > +		/* this is for hierarchical entry structure */
> > +		struct {
> > +			struct rb_root	hroot_in;
> > +			struct rb_root  hroot_out;
> > +		};				/* non-leaf entries */
> > +		struct rb_root	sorted_chain;	/* leaf entry has callchains */
> > +	};
> 
> looks like cool feature!

Thanks!

> 
> could we have the hist_entry storage little more generic?
> and maybe dynamically allocated?

I'm fine with it.

> 
> I'm doing the same thing for the c2c stuff, like having
> for each hist_entry its own 'struct hists' object, which
> records data related to parent hist_entry
> 
> maybe we could strip the 'hists' object to some bare minimum
> which is needed for store/sort/stat and display entries
> in hists_browser ;-)

Yeah, as you can see, it basically needs a pointer to parent entry,
hpp output format, and rbtrees for chlidren.  Also I added depth and
leaf field to make it easy for browser routines.

> 
> I'm preparing RFC patchset in my perf/c2c branch, if you want
> to take a look

Sure.

> 
> however, as I said above, for my own sake it all boils down
> to have 'hists' object within hist_entry, so I can use it
> in the UI code easily
> 
> FYI I also added support for hists object's own sort/output
> format lists.. which I'll curve out and send for review soon

OK, will take a look.

Thanks,
Namhyung

Re: [PATCH 01/17] perf hists: Basic support of hierarchical report view

[Arnaldo Carvalho de Melo]

Em Tue, Jan 19, 2016 at 07:51:18PM +0900, Namhyung Kim escreveu:
> Hi Jiri,
> 
> On Sun, Jan 17, 2016 at 05:15:33PM +0100, Jiri Olsa wrote:
> > On Sun, Jan 17, 2016 at 01:03:01AM +0900, Namhyung Kim wrote:
> > 
> > SNIP
> > 
> > >  	char			*srcfile;
> > >  	struct symbol		*parent;
> > > -	struct rb_root		sorted_chain;
> > >  	struct branch_info	*branch_info;
> > >  	struct hists		*hists;
> > >  	struct mem_info		*mem_info;
> > >  	void			*raw_data;
> > >  	u32			raw_size;
> > >  	void			*trace_output;
> > > +	struct perf_hpp_fmt	*fmt;
> > > +	struct hist_entry	*parent_he;
> > > +	union {
> > > +		/* this is for hierarchical entry structure */
> > > +		struct {
> > > +			struct rb_root	hroot_in;
> > > +			struct rb_root  hroot_out;
> > > +		};				/* non-leaf entries */
> > > +		struct rb_root	sorted_chain;	/* leaf entry has callchains */
> > > +	};
> > 
> > looks like cool feature!
> 
> Thanks!
> 
> > 
> > could we have the hist_entry storage little more generic?
> > and maybe dynamically allocated?
> 
> I'm fine with it.

Ok, so how should we proceed? I propose I test this patchkit, which
indeed looks cool from this cover letter description, yay!

If I find no problems, I'll merge it and, then, on top of it, you guys
can work on having this per-feature priv storage sorted out?

Please advise, meanwhile I'll cherry-pick whatever seems easy from both
patchkits.

- Arnaldo
 
> > 
> > I'm doing the same thing for the c2c stuff, like having
> > for each hist_entry its own 'struct hists' object, which
> > records data related to parent hist_entry
> > 
> > maybe we could strip the 'hists' object to some bare minimum
> > which is needed for store/sort/stat and display entries
> > in hists_browser ;-)
> 
> Yeah, as you can see, it basically needs a pointer to parent entry,
> hpp output format, and rbtrees for chlidren.  Also I added depth and
> leaf field to make it easy for browser routines.
> 
> > 
> > I'm preparing RFC patchset in my perf/c2c branch, if you want
> > to take a look
> 
> Sure.
> 
> > 
> > however, as I said above, for my own sake it all boils down
> > to have 'hists' object within hist_entry, so I can use it
> > in the UI code easily
> > 
> > FYI I also added support for hists object's own sort/output
> > format lists.. which I'll curve out and send for review soon
> 
> OK, will take a look.
> 
> Thanks,
> Namhyung

Re: [PATCH 01/17] perf hists: Basic support of hierarchical report view

[Jiri Olsa]

On Tue, Jan 19, 2016 at 01:50:47PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Jan 19, 2016 at 07:51:18PM +0900, Namhyung Kim escreveu:
> > Hi Jiri,
> > 
> > On Sun, Jan 17, 2016 at 05:15:33PM +0100, Jiri Olsa wrote:
> > > On Sun, Jan 17, 2016 at 01:03:01AM +0900, Namhyung Kim wrote:
> > > 
> > > SNIP
> > > 
> > > >  	char			*srcfile;
> > > >  	struct symbol		*parent;
> > > > -	struct rb_root		sorted_chain;
> > > >  	struct branch_info	*branch_info;
> > > >  	struct hists		*hists;
> > > >  	struct mem_info		*mem_info;
> > > >  	void			*raw_data;
> > > >  	u32			raw_size;
> > > >  	void			*trace_output;
> > > > +	struct perf_hpp_fmt	*fmt;
> > > > +	struct hist_entry	*parent_he;
> > > > +	union {
> > > > +		/* this is for hierarchical entry structure */
> > > > +		struct {
> > > > +			struct rb_root	hroot_in;
> > > > +			struct rb_root  hroot_out;
> > > > +		};				/* non-leaf entries */
> > > > +		struct rb_root	sorted_chain;	/* leaf entry has callchains */
> > > > +	};
> > > 
> > > looks like cool feature!
> > 
> > Thanks!
> > 
> > > 
> > > could we have the hist_entry storage little more generic?
> > > and maybe dynamically allocated?
> > 
> > I'm fine with it.
> 
> Ok, so how should we proceed? I propose I test this patchkit, which
> indeed looks cool from this cover letter description, yay!
> 
> If I find no problems, I'll merge it and, then, on top of it, you guys
> can work on having this per-feature priv storage sorted out?
> 
> Please advise, meanwhile I'll cherry-pick whatever seems easy from both
> patchkits.

Namhyung,
are you going to send another version, or should I review this one?

thanks,
jirka

Re: [PATCH 01/17] perf hists: Basic support of hierarchical report view

[Arnaldo Carvalho de Melo]

Em Wed, Jan 20, 2016 at 06:00:59PM +0100, Jiri Olsa escreveu:
> On Tue, Jan 19, 2016 at 01:50:47PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Jan 19, 2016 at 07:51:18PM +0900, Namhyung Kim escreveu:
> > > Hi Jiri,
> > > 
> > > On Sun, Jan 17, 2016 at 05:15:33PM +0100, Jiri Olsa wrote:
> > > > On Sun, Jan 17, 2016 at 01:03:01AM +0900, Namhyung Kim wrote:
> > > > 
> > > > SNIP
> > > > 
> > > > >  	char			*srcfile;
> > > > >  	struct symbol		*parent;
> > > > > -	struct rb_root		sorted_chain;
> > > > >  	struct branch_info	*branch_info;
> > > > >  	struct hists		*hists;
> > > > >  	struct mem_info		*mem_info;
> > > > >  	void			*raw_data;
> > > > >  	u32			raw_size;
> > > > >  	void			*trace_output;
> > > > > +	struct perf_hpp_fmt	*fmt;
> > > > > +	struct hist_entry	*parent_he;
> > > > > +	union {
> > > > > +		/* this is for hierarchical entry structure */
> > > > > +		struct {
> > > > > +			struct rb_root	hroot_in;
> > > > > +			struct rb_root  hroot_out;
> > > > > +		};				/* non-leaf entries */
> > > > > +		struct rb_root	sorted_chain;	/* leaf entry has callchains */
> > > > > +	};
> > > > 
> > > > looks like cool feature!
> > > 
> > > Thanks!
> > > 
> > > > 
> > > > could we have the hist_entry storage little more generic?
> > > > and maybe dynamically allocated?
> > > 
> > > I'm fine with it.
> > 
> > Ok, so how should we proceed? I propose I test this patchkit, which
> > indeed looks cool from this cover letter description, yay!
> > 
> > If I find no problems, I'll merge it and, then, on top of it, you guys
> > can work on having this per-feature priv storage sorted out?
> > 
> > Please advise, meanwhile I'll cherry-pick whatever seems easy from both
> > patchkits.
> 
> Namhyung,
> are you going to send another version, or should I review this one?

This is what I am assuming, going thru the patches and replacing the
fourth (4/17) by the 4.1/17 and 4.2/17 that he sent.

- Arnaldo

Re: [PATCH 01/17] perf hists: Basic support of hierarchical report view

[Namhyung Kim]

Hi Jiri,

On Wed, Jan 20, 2016 at 06:00:59PM +0100, Jiri Olsa wrote:
> On Tue, Jan 19, 2016 at 01:50:47PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Jan 19, 2016 at 07:51:18PM +0900, Namhyung Kim escreveu:
> > > Hi Jiri,
> > > 
> > > On Sun, Jan 17, 2016 at 05:15:33PM +0100, Jiri Olsa wrote:
> > > > On Sun, Jan 17, 2016 at 01:03:01AM +0900, Namhyung Kim wrote:
> > > > 
> > > > SNIP
> > > > 
> > > > >  	char			*srcfile;
> > > > >  	struct symbol		*parent;
> > > > > -	struct rb_root		sorted_chain;
> > > > >  	struct branch_info	*branch_info;
> > > > >  	struct hists		*hists;
> > > > >  	struct mem_info		*mem_info;
> > > > >  	void			*raw_data;
> > > > >  	u32			raw_size;
> > > > >  	void			*trace_output;
> > > > > +	struct perf_hpp_fmt	*fmt;
> > > > > +	struct hist_entry	*parent_he;
> > > > > +	union {
> > > > > +		/* this is for hierarchical entry structure */
> > > > > +		struct {
> > > > > +			struct rb_root	hroot_in;
> > > > > +			struct rb_root  hroot_out;
> > > > > +		};				/* non-leaf entries */
> > > > > +		struct rb_root	sorted_chain;	/* leaf entry has callchains */
> > > > > +	};
> > > > 
> > > > looks like cool feature!
> > > 
> > > Thanks!
> > > 
> > > > 
> > > > could we have the hist_entry storage little more generic?
> > > > and maybe dynamically allocated?
> > > 
> > > I'm fine with it.
> > 
> > Ok, so how should we proceed? I propose I test this patchkit, which
> > indeed looks cool from this cover letter description, yay!
> > 
> > If I find no problems, I'll merge it and, then, on top of it, you guys
> > can work on having this per-feature priv storage sorted out?
> > 
> > Please advise, meanwhile I'll cherry-pick whatever seems easy from both
> > patchkits.
> 
> Namhyung,
> are you going to send another version, or should I review this one?

Please review this version!

Thanks,
Namhyung

Re: [PATCH 01/17] perf hists: Basic support of hierarchical report view

[Jiri Olsa]

On Sun, Jan 17, 2016 at 01:03:01AM +0900, Namhyung Kim wrote:

SNIP

> +}
> +
> +static bool hists__hierarchy_insert_entry(struct hists *hists,
> +					  struct rb_root *root,
> +					  struct hist_entry *he)
> +{
> +	struct perf_hpp_fmt *fmt;
> +	struct hist_entry *new = NULL;
> +	struct hist_entry *parent = NULL;
> +	int depth = 0;
> +
> +	perf_hpp__for_each_sort_list(fmt) {
> +		if (!perf_hpp__is_sort_entry(fmt) &&
> +		    !perf_hpp__is_dynamic_entry(fmt))
> +			continue;
> +
> +		/* insert copy of 'he' for each fmt into the hierarchy */
> +		new = hierarchy_insert_entry(hists, root, he, fmt);
> +		if (new == NULL)
> +			break;

so hierarchy_insert_entry can fail because of memory allocation
but the resort path does not cover any error path because it only
shuffles entries from in-tree into sorted tree

would it make more sense to do this in 'in-tree addition' path?
and keep the resort functions to do only resort stuff

thanks,
jirka

Re: [PATCH 01/17] perf hists: Basic support of hierarchical report view

[Namhyung Kim]

Hi Jiri,

On Thu, Jan 21, 2016 at 11:43:30AM +0100, Jiri Olsa wrote:
> On Sun, Jan 17, 2016 at 01:03:01AM +0900, Namhyung Kim wrote:
> 
> SNIP
> 
> > +}
> > +
> > +static bool hists__hierarchy_insert_entry(struct hists *hists,
> > +					  struct rb_root *root,
> > +					  struct hist_entry *he)
> > +{
> > +	struct perf_hpp_fmt *fmt;
> > +	struct hist_entry *new = NULL;
> > +	struct hist_entry *parent = NULL;
> > +	int depth = 0;
> > +
> > +	perf_hpp__for_each_sort_list(fmt) {
> > +		if (!perf_hpp__is_sort_entry(fmt) &&
> > +		    !perf_hpp__is_dynamic_entry(fmt))
> > +			continue;
> > +
> > +		/* insert copy of 'he' for each fmt into the hierarchy */
> > +		new = hierarchy_insert_entry(hists, root, he, fmt);
> > +		if (new == NULL)
> > +			break;
> 
> so hierarchy_insert_entry can fail because of memory allocation
> but the resort path does not cover any error path because it only
> shuffles entries from in-tree into sorted tree

Yes, memory allocation can fail anywhere.  If it happens, there's not
much thing we can do IMHO - just print warning and bail out.
Currently it silently ignores the allocation error and try to proceed.
But I guess it'll fail soon at other place anyway.

AFAICS current code also can fail in callchain_merge()..

Maybe we can change the return type of this function to int and treat
-1 as an error to detect such cases.


> 
> would it make more sense to do this in 'in-tree addition' path?
> and keep the resort functions to do only resort stuff

I don't follow.  There're 3 path to handle hist entries - let's say
them as 'addition', 'collapsing', and 'resort'.  This function does
the 'collapsing' part - it was originally intended to merge sharable
entries (namely for same 'comm' among different threads).  But I used
it to build a hierarchy since I found it useful as follows:

  1. it requires smaller change than doing it in the 'addition' path
  2. it can reuse current callback-based 'addition' paths so mem- and
     branch-mode can be supported easily (but it needs test..).
  3. the 'addition' path can be parallelized so it'll increase memory
     footprint if it build temporary local hierarchies during the path.

The 'resort' path always do sorting only..

Thanks,
Namhyung

Re: [PATCH 01/17] perf hists: Basic support of hierarchical report view

[Jiri Olsa]

On Thu, Jan 21, 2016 at 09:55:52PM +0900, Namhyung Kim wrote:

SNIP

> > > +		/* insert copy of 'he' for each fmt into the hierarchy */
> > > +		new = hierarchy_insert_entry(hists, root, he, fmt);
> > > +		if (new == NULL)
> > > +			break;
> > 
> > so hierarchy_insert_entry can fail because of memory allocation
> > but the resort path does not cover any error path because it only
> > shuffles entries from in-tree into sorted tree
> 
> Yes, memory allocation can fail anywhere.  If it happens, there's not
> much thing we can do IMHO - just print warning and bail out.
> Currently it silently ignores the allocation error and try to proceed.
> But I guess it'll fail soon at other place anyway.

I thought the 'policy' is to handle all allocation failures

> 
> AFAICS current code also can fail in callchain_merge()..
> 
> Maybe we can change the return type of this function to int and treat
> -1 as an error to detect such cases.
> 
> 
> > 
> > would it make more sense to do this in 'in-tree addition' path?
> > and keep the resort functions to do only resort stuff
> 
> I don't follow.  There're 3 path to handle hist entries - let's say
> them as 'addition', 'collapsing', and 'resort'.  This function does
> the 'collapsing' part - it was originally intended to merge sharable
> entries (namely for same 'comm' among different threads).  But I used
> it to build a hierarchy since I found it useful as follows:
> 
>   1. it requires smaller change than doing it in the 'addition' path
>   2. it can reuse current callback-based 'addition' paths so mem- and
>      branch-mode can be supported easily (but it needs test..).
>   3. the 'addition' path can be parallelized so it'll increase memory
>      footprint if it build temporary local hierarchies during the path.
> 
> The 'resort' path always do sorting only..

well, you are adding/duplicating entries now in resort path
and that is not just 'sorting only'

you allow only sort and tracepoint entries to be added in
hierrarych view, so there's no resort needed, but still it
could be added in future? not sure

it still makes more sense to me to do this in 'addition' path,
because you basically add new entries

but have no other grounds for this also I might be missing something ;-)

thanks,
jirka

Re: [PATCH 01/17] perf hists: Basic support of hierarchical report view

[Arnaldo Carvalho de Melo]

Em Thu, Jan 21, 2016 at 02:35:58PM +0100, Jiri Olsa escreveu:
> On Thu, Jan 21, 2016 at 09:55:52PM +0900, Namhyung Kim wrote:
> 
> SNIP
> 
> > > > +		/* insert copy of 'he' for each fmt into the hierarchy */
> > > > +		new = hierarchy_insert_entry(hists, root, he, fmt);
> > > > +		if (new == NULL)
> > > > +			break;

Also, can we rename 'new' to new_he? In the past I used 'self' and
Thomas rightly told me that 'self' didn't convey any info, likewise for
'new' (that is even a keyword in C++ and may confuse some syntax
highligting, etc).

> > > so hierarchy_insert_entry can fail because of memory allocation
> > > but the resort path does not cover any error path because it only
> > > shuffles entries from in-tree into sorted tree

> > Yes, memory allocation can fail anywhere.  If it happens, there's not
> > much thing we can do IMHO - just print warning and bail out.
> > Currently it silently ignores the allocation error and try to proceed.
> > But I guess it'll fail soon at other place anyway.
> 
> I thought the 'policy' is to handle all allocation failures

yup, if some place doesn't, we need to fix it, silently trowing away
stuff is not good. At the very least count the number of failures and
inform the user somewhere on the screen.
 
> > AFAICS current code also can fail in callchain_merge()..

That needs to be fixed too, then.

- Arnaldo

Re: [PATCH 01/17] perf hists: Basic support of hierarchical report view

[Namhyung Kim]

Hi Arnaldo,

On Thu, Jan 21, 2016 at 11:02:09AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Jan 21, 2016 at 02:35:58PM +0100, Jiri Olsa escreveu:
> > On Thu, Jan 21, 2016 at 09:55:52PM +0900, Namhyung Kim wrote:
> > 
> > SNIP
> > 
> > > > > +		/* insert copy of 'he' for each fmt into the hierarchy */
> > > > > +		new = hierarchy_insert_entry(hists, root, he, fmt);
> > > > > +		if (new == NULL)
> > > > > +			break;
> 
> Also, can we rename 'new' to new_he? In the past I used 'self' and
> Thomas rightly told me that 'self' didn't convey any info, likewise for
> 'new' (that is even a keyword in C++ and may confuse some syntax
> highligting, etc).

OK


> 
> > > > so hierarchy_insert_entry can fail because of memory allocation
> > > > but the resort path does not cover any error path because it only
> > > > shuffles entries from in-tree into sorted tree
> 
> > > Yes, memory allocation can fail anywhere.  If it happens, there's not
> > > much thing we can do IMHO - just print warning and bail out.
> > > Currently it silently ignores the allocation error and try to proceed.
> > > But I guess it'll fail soon at other place anyway.
> > 
> > I thought the 'policy' is to handle all allocation failures
> 
> yup, if some place doesn't, we need to fix it, silently trowing away
> stuff is not good. At the very least count the number of failures and
> inform the user somewhere on the screen.

OK, will change.


>  
> > > AFAICS current code also can fail in callchain_merge()..
> 
> That needs to be fixed too, then.

OK

Thanks,
Namhyung

Re: [PATCH 01/17] perf hists: Basic support of hierarchical report view

[Namhyung Kim]

Hi Jiri,

On Thu, Jan 21, 2016 at 02:35:58PM +0100, Jiri Olsa wrote:
> On Thu, Jan 21, 2016 at 09:55:52PM +0900, Namhyung Kim wrote:
> > > would it make more sense to do this in 'in-tree addition' path?
> > > and keep the resort functions to do only resort stuff
> > 
> > I don't follow.  There're 3 path to handle hist entries - let's say
> > them as 'addition', 'collapsing', and 'resort'.  This function does
> > the 'collapsing' part - it was originally intended to merge sharable
> > entries (namely for same 'comm' among different threads).  But I used
> > it to build a hierarchy since I found it useful as follows:
> > 
> >   1. it requires smaller change than doing it in the 'addition' path
> >   2. it can reuse current callback-based 'addition' paths so mem- and
> >      branch-mode can be supported easily (but it needs test..).
> >   3. the 'addition' path can be parallelized so it'll increase memory
> >      footprint if it build temporary local hierarchies during the path.
> > 
> > The 'resort' path always do sorting only..
> 
> well, you are adding/duplicating entries now in resort path
> and that is not just 'sorting only'

As I said, this is not the 'resort' path, the 'resort' path is the
subject of patch 02/17.


> 
> you allow only sort and tracepoint entries to be added in
> hierrarych view, so there's no resort needed, but still it
> could be added in future? not sure

Resorting is still needed since it should sort entries by overhead
(period).  Anyway, in this 'collapsing' path, we need to build a
hierarchy to be sorted at the resort path.


> 
> it still makes more sense to me to do this in 'addition' path,
> because you basically add new entries
> 
> but have no other grounds for this also I might be missing something ;-)

What about thinking like this?

1. addition   - add samples into hist entries
2. collapsing - build complete hists (hierarchy) to be sorted
3. resort     - sort final entries based on the sort keys

Thanks,
Namhyung

Re: [PATCH 01/17] perf hists: Basic support of hierarchical report view

[Jiri Olsa]

On Fri, Jan 22, 2016 at 07:43:12PM +0900, Namhyung Kim wrote:
> Hi Jiri,
> 
> On Thu, Jan 21, 2016 at 02:35:58PM +0100, Jiri Olsa wrote:
> > On Thu, Jan 21, 2016 at 09:55:52PM +0900, Namhyung Kim wrote:
> > > > would it make more sense to do this in 'in-tree addition' path?
> > > > and keep the resort functions to do only resort stuff
> > > 
> > > I don't follow.  There're 3 path to handle hist entries - let's say
> > > them as 'addition', 'collapsing', and 'resort'.  This function does
> > > the 'collapsing' part - it was originally intended to merge sharable
> > > entries (namely for same 'comm' among different threads).  But I used
> > > it to build a hierarchy since I found it useful as follows:
> > > 
> > >   1. it requires smaller change than doing it in the 'addition' path
> > >   2. it can reuse current callback-based 'addition' paths so mem- and
> > >      branch-mode can be supported easily (but it needs test..).
> > >   3. the 'addition' path can be parallelized so it'll increase memory
> > >      footprint if it build temporary local hierarchies during the path.
> > > 
> > > The 'resort' path always do sorting only..
> > 
> > well, you are adding/duplicating entries now in resort path
> > and that is not just 'sorting only'
> 
> As I said, this is not the 'resort' path, the 'resort' path is the
> subject of patch 02/17.

ok, I took the collapsing as a part of resort

> 
> 
> > 
> > you allow only sort and tracepoint entries to be added in
> > hierrarych view, so there's no resort needed, but still it
> > could be added in future? not sure
> 
> Resorting is still needed since it should sort entries by overhead
> (period).  Anyway, in this 'collapsing' path, we need to build a
> hierarchy to be sorted at the resort path.
> 
> 
> > 
> > it still makes more sense to me to do this in 'addition' path,
> > because you basically add new entries
> > 
> > but have no other grounds for this also I might be missing something ;-)
> 
> What about thinking like this?
> 
> 1. addition   - add samples into hist entries
> 2. collapsing - build complete hists (hierarchy) to be sorted
> 3. resort     - sort final entries based on the sort keys

ok, sounds good.. maybe we could use some other name
for collapsing then.. like build,setup or such

thanks for bearing with me ;-)
jirka

Re: [PATCH 01/17] perf hists: Basic support of hierarchical report view

[Jiri Olsa]

On Sun, Jan 17, 2016 at 01:03:01AM +0900, Namhyung Kim wrote:

SNIP

> diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
> index 687bbb124428..15a75d44de91 100644
> --- a/tools/perf/util/sort.h
> +++ b/tools/perf/util/sort.h
> @@ -94,9 +94,11 @@ struct hist_entry {
>  	s32			socket;
>  	s32			cpu;
>  	u8			cpumode;
> +	u8			depth;
>  
>  	/* We are added by hists__add_dummy_entry. */
>  	bool			dummy;
> +	bool			leaf;
>  
>  	char			level;
>  	u8			filtered;
> @@ -118,13 +120,22 @@ struct hist_entry {
>  	char			*srcline;
>  	char			*srcfile;
>  	struct symbol		*parent;
> -	struct rb_root		sorted_chain;
>  	struct branch_info	*branch_info;
>  	struct hists		*hists;
>  	struct mem_info		*mem_info;
>  	void			*raw_data;
>  	u32			raw_size;
>  	void			*trace_output;
> +	struct perf_hpp_fmt	*fmt;
> +	struct hist_entry	*parent_he;
> +	union {
> +		/* this is for hierarchical entry structure */
> +		struct {
> +			struct rb_root	hroot_in;
> +			struct rb_root  hroot_out;

we use 'entries_in' and 'entries' in hists object

could we keep the names in here as well to indicate
it's doing the same stuff..?

thanks,
jirka

Re: [PATCH 01/17] perf hists: Basic support of hierarchical report view

[Namhyung Kim]

On Thu, Jan 21, 2016 at 12:35:11PM +0100, Jiri Olsa wrote:
> On Sun, Jan 17, 2016 at 01:03:01AM +0900, Namhyung Kim wrote:
> 
> SNIP
> 
> > diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
> > index 687bbb124428..15a75d44de91 100644
> > --- a/tools/perf/util/sort.h
> > +++ b/tools/perf/util/sort.h
> > @@ -94,9 +94,11 @@ struct hist_entry {
> >  	s32			socket;
> >  	s32			cpu;
> >  	u8			cpumode;
> > +	u8			depth;
> >  
> >  	/* We are added by hists__add_dummy_entry. */
> >  	bool			dummy;
> > +	bool			leaf;
> >  
> >  	char			level;
> >  	u8			filtered;
> > @@ -118,13 +120,22 @@ struct hist_entry {
> >  	char			*srcline;
> >  	char			*srcfile;
> >  	struct symbol		*parent;
> > -	struct rb_root		sorted_chain;
> >  	struct branch_info	*branch_info;
> >  	struct hists		*hists;
> >  	struct mem_info		*mem_info;
> >  	void			*raw_data;
> >  	u32			raw_size;
> >  	void			*trace_output;
> > +	struct perf_hpp_fmt	*fmt;
> > +	struct hist_entry	*parent_he;
> > +	union {
> > +		/* this is for hierarchical entry structure */
> > +		struct {
> > +			struct rb_root	hroot_in;
> > +			struct rb_root  hroot_out;
> 
> we use 'entries_in' and 'entries' in hists object
> 
> could we keep the names in here as well to indicate
> it's doing the same stuff..?

Maybe it's a preference.  I don't think it's necessary, sometimes we
need to differentiate whether it accesses to (top-level) hists or hist
entry IMHO.

Thanks,
Namhyung

[PATCH 02/17] perf hists: Resort hist entries with hierarchy

[Namhyung Kim]

For hierarchical output, each entries should be sorted in their
rbtree (hroot) properly.  Add hists__hierarchy_output_resort() to do the
job.  Note that those hierarchy entries share the period counts, it'd be
important to update the hists->stats only once (for leaves).

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

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 70293eb70430..931353f49c72 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -1245,6 +1245,74 @@ void hists__inc_stats(struct hists *hists, struct hist_entry *h)
 	hists->stats.total_period += h->stat.period;
 }
 
+static void hierarchy_insert_output_entry(struct rb_root *root,
+					  struct hist_entry *he)
+{
+	struct rb_node **p = &root->rb_node;
+	struct rb_node *parent = NULL;
+	struct hist_entry *iter;
+
+	while (*p != NULL) {
+		parent = *p;
+		iter = rb_entry(parent, struct hist_entry, rb_node);
+
+		if (hist_entry__sort(he, iter) > 0)
+			p = &parent->rb_left;
+		else
+			p = &parent->rb_right;
+	}
+
+	rb_link_node(&he->rb_node, parent, p);
+	rb_insert_color(&he->rb_node, root);
+}
+
+static void hists__hierarchy_output_resort(struct hists *hists,
+					   struct ui_progress *prog,
+					   struct rb_root *root_in,
+					   struct rb_root *root_out,
+					   u64 min_callchain_hits,
+					   bool use_callchain)
+{
+	struct rb_node *node;
+	struct hist_entry *he;
+
+	*root_out = RB_ROOT;
+	node = rb_first(root_in);
+
+	while (node) {
+		he = rb_entry(node, struct hist_entry, rb_node_in);
+		node = rb_next(node);
+
+		hierarchy_insert_output_entry(root_out, he);
+
+		if (prog)
+			ui_progress__update(prog, 1);
+
+		if (!he->leaf) {
+			hists__hierarchy_output_resort(hists, prog,
+						       &he->hroot_in,
+						       &he->hroot_out,
+						       min_callchain_hits,
+						       use_callchain);
+			hists->nr_entries++;
+			if (!he->filtered)
+				hists->nr_non_filtered_entries++;
+
+			continue;
+		}
+
+		/* only update stat for leaf entries to avoid duplication */
+		hists__inc_stats(hists, he);
+		if (!he->filtered)
+			hists__calc_col_len(hists, he);
+
+		if (use_callchain)
+			callchain_param.sort(&he->sorted_chain, he->callchain,
+					     min_callchain_hits,
+					     &callchain_param);
+	}
+}
+
 static void __hists__insert_output_entry(struct rb_root *entries,
 					 struct hist_entry *he,
 					 u64 min_callchain_hits,
@@ -1288,6 +1356,17 @@ void hists__output_resort(struct hists *hists, struct ui_progress *prog)
 
 	min_callchain_hits = hists->stats.total_period * (callchain_param.min_percent / 100);
 
+	hists__reset_stats(hists);
+	hists__reset_col_len(hists);
+
+	if (symbol_conf.report_hierarchy) {
+		return hists__hierarchy_output_resort(hists, prog,
+						      &hists->entries_collapsed,
+						      &hists->entries,
+						      min_callchain_hits,
+						      use_callchain);
+	}
+
 	if (sort__need_collapse)
 		root = &hists->entries_collapsed;
 	else
@@ -1296,9 +1375,6 @@ void hists__output_resort(struct hists *hists, struct ui_progress *prog)
 	next = rb_first(root);
 	hists->entries = RB_ROOT;
 
-	hists__reset_stats(hists);
-	hists__reset_col_len(hists);
-
 	while (next) {
 		n = rb_entry(next, struct hist_entry, rb_node_in);
 		next = rb_next(&n->rb_node_in);
-- 
2.6.4

Re: [PATCH 02/17] perf hists: Resort hist entries with hierarchy

[Jiri Olsa]

On Sun, Jan 17, 2016 at 01:03:02AM +0900, Namhyung Kim wrote:

SNIP

> +					     min_callchain_hits,
> +					     &callchain_param);
> +	}
> +}
> +
>  static void __hists__insert_output_entry(struct rb_root *entries,
>  					 struct hist_entry *he,
>  					 u64 min_callchain_hits,
> @@ -1288,6 +1356,17 @@ void hists__output_resort(struct hists *hists, struct ui_progress *prog)
>  
>  	min_callchain_hits = hists->stats.total_period * (callchain_param.min_percent / 100);
>  
> +	hists__reset_stats(hists);
> +	hists__reset_col_len(hists);
> +
> +	if (symbol_conf.report_hierarchy) {
> +		return hists__hierarchy_output_resort(hists, prog,
> +						      &hists->entries_collapsed,

is the 'in-root' always hists->entries_collapsed in here?

should you use the 'root' var from the condition below?

thanks,
jirka

> +						      &hists->entries,
> +						      min_callchain_hits,
> +						      use_callchain);
> +	}
> +
>  	if (sort__need_collapse)
>  		root = &hists->entries_collapsed;
>  	else
> @@ -1296,9 +1375,6 @@ void hists__output_resort(struct hists *hists, struct ui_progress *prog)
>  	next = rb_first(root);
>  	hists->entries = RB_ROOT;
>  
> -	hists__reset_stats(hists);
> -	hists__reset_col_len(hists);
> -
>  	while (next) {
>  		n = rb_entry(next, struct hist_entry, rb_node_in);
>  		next = rb_next(&n->rb_node_in);
> -- 
> 2.6.4
> 

Re: [PATCH 02/17] perf hists: Resort hist entries with hierarchy

[Namhyung Kim]

On Thu, Jan 21, 2016 at 12:41:49PM +0100, Jiri Olsa wrote:
> On Sun, Jan 17, 2016 at 01:03:02AM +0900, Namhyung Kim wrote:
> 
> SNIP
> 
> > +					     min_callchain_hits,
> > +					     &callchain_param);
> > +	}
> > +}
> > +
> >  static void __hists__insert_output_entry(struct rb_root *entries,
> >  					 struct hist_entry *he,
> >  					 u64 min_callchain_hits,
> > @@ -1288,6 +1356,17 @@ void hists__output_resort(struct hists *hists, struct ui_progress *prog)
> >  
> >  	min_callchain_hits = hists->stats.total_period * (callchain_param.min_percent / 100);
> >  
> > +	hists__reset_stats(hists);
> > +	hists__reset_col_len(hists);
> > +
> > +	if (symbol_conf.report_hierarchy) {
> > +		return hists__hierarchy_output_resort(hists, prog,
> > +						      &hists->entries_collapsed,
> 
> is the 'in-root' always hists->entries_collapsed in here?
> 
> should you use the 'root' var from the condition below?

The symbol_conf.report_hierarchy imples sort__need_collapse since it
needs to build the hierarchy during 'collapse' stage.  So yes, it's
always hists->entries_collapsed.

Thanks,
Namhyung


> 
> > +						      &hists->entries,
> > +						      min_callchain_hits,
> > +						      use_callchain);
> > +	}
> > +
> >  	if (sort__need_collapse)
> >  		root = &hists->entries_collapsed;
> >  	else
> > @@ -1296,9 +1375,6 @@ void hists__output_resort(struct hists *hists, struct ui_progress *prog)
> >  	next = rb_first(root);
> >  	hists->entries = RB_ROOT;
> >  
> > -	hists__reset_stats(hists);
> > -	hists__reset_col_len(hists);
> > -
> >  	while (next) {
> >  		n = rb_entry(next, struct hist_entry, rb_node_in);
> >  		next = rb_next(&n->rb_node_in);
> > -- 
> > 2.6.4
> > 

[PATCH 03/17] perf hists: Add helper functions for hierarchy mode

[Namhyung Kim]

The rb_hierarchy_{first,last,next,prev} functions are to traverse all
hist entries in a hierarchy.  They will be used by various function
which supports hierarchy output.

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

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 931353f49c72..9354455aec5b 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -1390,6 +1390,56 @@ void hists__output_resort(struct hists *hists, struct ui_progress *prog)
 	}
 }
 
+struct rb_node *rb_hierarchy_first(struct rb_node *node)
+{
+	return node;
+}
+
+struct rb_node *rb_hierarchy_last(struct rb_node *node)
+{
+	struct hist_entry *he = rb_entry(node, struct hist_entry, rb_node);
+
+	while (he->unfolded && !he->leaf) {
+		node = rb_last(&he->hroot_out);
+		he = rb_entry(node, struct hist_entry, rb_node);
+	}
+	return node;
+}
+
+struct rb_node *rb_hierarchy_next(struct rb_node *node)
+{
+	struct hist_entry *he = rb_entry(node, struct hist_entry, rb_node);
+
+	if (!he->leaf && he->unfolded)
+		node = rb_first(&he->hroot_out);
+	else
+		node = rb_next(node);
+
+	while (node == NULL) {
+		he = he->parent_he;
+		if (he == NULL)
+			break;
+
+		node = rb_next(&he->rb_node);
+	}
+	return node;
+}
+
+struct rb_node *rb_hierarchy_prev(struct rb_node *node)
+{
+	struct hist_entry *he = rb_entry(node, struct hist_entry, rb_node);
+
+	node = rb_prev(node);
+	if (node)
+		return rb_hierarchy_last(node);
+
+	he = he->parent_he;
+	if (he == NULL)
+		return NULL;
+
+	return &he->rb_node;
+}
+
 static void hists__remove_entry_filter(struct hists *hists, struct hist_entry *h,
 				       enum hist_filter filter)
 {
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index d4ec4822a103..96b7ff817d3e 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -381,4 +381,13 @@ int parse_filter_percentage(const struct option *opt __maybe_unused,
 			    const char *arg, int unset __maybe_unused);
 int perf_hist_config(const char *var, const char *value);
 
+#define HIERARCHY_INDENT  3
+
+int perf_hpp__count_sort_keys(void);
+
+struct rb_node *rb_hierarchy_first(struct rb_node *node);
+struct rb_node *rb_hierarchy_last(struct rb_node *node);
+struct rb_node *rb_hierarchy_next(struct rb_node *node);
+struct rb_node *rb_hierarchy_prev(struct rb_node *node);
+
 #endif	/* __PERF_HIST_H */
-- 
2.6.4

Re: [PATCH 03/17] perf hists: Add helper functions for hierarchy mode

[Arnaldo Carvalho de Melo]

Em Sun, Jan 17, 2016 at 01:03:03AM +0900, Namhyung Kim escreveu:
> The rb_hierarchy_{first,last,next,prev} functions are to traverse all
> hist entries in a hierarchy.  They will be used by various function
> which supports hierarchy output.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/hist.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  tools/perf/util/hist.h |  9 +++++++++
>  2 files changed, 59 insertions(+)
> 
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index 931353f49c72..9354455aec5b 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -1390,6 +1390,56 @@ void hists__output_resort(struct hists *hists, struct ui_progress *prog)
>  	}
>  }
>  
> +struct rb_node *rb_hierarchy_first(struct rb_node *node)
> +{
> +	return node;
> +}

rb_hierarchy_first() is not used in this patchkit, you end up starting
from rb_first(&hists->entries), so I guess we can remove this one, if
you ever need it, then reintroduce it.

- Arnaldo

> +
> +struct rb_node *rb_hierarchy_last(struct rb_node *node)
> +{
> +	struct hist_entry *he = rb_entry(node, struct hist_entry, rb_node);
> +
> +	while (he->unfolded && !he->leaf) {
> +		node = rb_last(&he->hroot_out);
> +		he = rb_entry(node, struct hist_entry, rb_node);
> +	}
> +	return node;
> +}
> +
> +struct rb_node *rb_hierarchy_next(struct rb_node *node)
> +{
> +	struct hist_entry *he = rb_entry(node, struct hist_entry, rb_node);
> +
> +	if (!he->leaf && he->unfolded)
> +		node = rb_first(&he->hroot_out);
> +	else
> +		node = rb_next(node);
> +
> +	while (node == NULL) {
> +		he = he->parent_he;
> +		if (he == NULL)
> +			break;
> +
> +		node = rb_next(&he->rb_node);
> +	}
> +	return node;
> +}
> +
> +struct rb_node *rb_hierarchy_prev(struct rb_node *node)
> +{
> +	struct hist_entry *he = rb_entry(node, struct hist_entry, rb_node);
> +
> +	node = rb_prev(node);
> +	if (node)
> +		return rb_hierarchy_last(node);
> +
> +	he = he->parent_he;
> +	if (he == NULL)
> +		return NULL;
> +
> +	return &he->rb_node;
> +}
> +
>  static void hists__remove_entry_filter(struct hists *hists, struct hist_entry *h,
>  				       enum hist_filter filter)
>  {
> diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
> index d4ec4822a103..96b7ff817d3e 100644
> --- a/tools/perf/util/hist.h
> +++ b/tools/perf/util/hist.h
> @@ -381,4 +381,13 @@ int parse_filter_percentage(const struct option *opt __maybe_unused,
>  			    const char *arg, int unset __maybe_unused);
>  int perf_hist_config(const char *var, const char *value);
>  
> +#define HIERARCHY_INDENT  3
> +
> +int perf_hpp__count_sort_keys(void);
> +
> +struct rb_node *rb_hierarchy_first(struct rb_node *node);
> +struct rb_node *rb_hierarchy_last(struct rb_node *node);
> +struct rb_node *rb_hierarchy_next(struct rb_node *node);
> +struct rb_node *rb_hierarchy_prev(struct rb_node *node);
> +
>  #endif	/* __PERF_HIST_H */
> -- 
> 2.6.4

Re: [PATCH 03/17] perf hists: Add helper functions for hierarchy mode

[Namhyung Kim]

Hi Arnaldo,

On Wed, Jan 20, 2016 at 07:19:03PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Sun, Jan 17, 2016 at 01:03:03AM +0900, Namhyung Kim escreveu:
> > The rb_hierarchy_{first,last,next,prev} functions are to traverse all
> > hist entries in a hierarchy.  They will be used by various function
> > which supports hierarchy output.
> > 
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/util/hist.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  tools/perf/util/hist.h |  9 +++++++++
> >  2 files changed, 59 insertions(+)
> > 
> > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> > index 931353f49c72..9354455aec5b 100644
> > --- a/tools/perf/util/hist.c
> > +++ b/tools/perf/util/hist.c
> > @@ -1390,6 +1390,56 @@ void hists__output_resort(struct hists *hists, struct ui_progress *prog)
> >  	}
> >  }
> >  
> > +struct rb_node *rb_hierarchy_first(struct rb_node *node)
> > +{
> > +	return node;
> > +}
> 
> rb_hierarchy_first() is not used in this patchkit, you end up starting
> from rb_first(&hists->entries), so I guess we can remove this one, if
> you ever need it, then reintroduce it.

Right.  I firstly thought it might be needed, but it's not.  Will remove.

Thanks,
Namhyung


> > +
> > +struct rb_node *rb_hierarchy_last(struct rb_node *node)
> > +{
> > +	struct hist_entry *he = rb_entry(node, struct hist_entry, rb_node);
> > +
> > +	while (he->unfolded && !he->leaf) {
> > +		node = rb_last(&he->hroot_out);
> > +		he = rb_entry(node, struct hist_entry, rb_node);
> > +	}
> > +	return node;
> > +}
> > +
> > +struct rb_node *rb_hierarchy_next(struct rb_node *node)
> > +{
> > +	struct hist_entry *he = rb_entry(node, struct hist_entry, rb_node);
> > +
> > +	if (!he->leaf && he->unfolded)
> > +		node = rb_first(&he->hroot_out);
> > +	else
> > +		node = rb_next(node);
> > +
> > +	while (node == NULL) {
> > +		he = he->parent_he;
> > +		if (he == NULL)
> > +			break;
> > +
> > +		node = rb_next(&he->rb_node);
> > +	}
> > +	return node;
> > +}
> > +
> > +struct rb_node *rb_hierarchy_prev(struct rb_node *node)
> > +{
> > +	struct hist_entry *he = rb_entry(node, struct hist_entry, rb_node);
> > +
> > +	node = rb_prev(node);
> > +	if (node)
> > +		return rb_hierarchy_last(node);
> > +
> > +	he = he->parent_he;
> > +	if (he == NULL)
> > +		return NULL;
> > +
> > +	return &he->rb_node;
> > +}
> > +
> >  static void hists__remove_entry_filter(struct hists *hists, struct hist_entry *h,
> >  				       enum hist_filter filter)
> >  {
> > diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
> > index d4ec4822a103..96b7ff817d3e 100644
> > --- a/tools/perf/util/hist.h
> > +++ b/tools/perf/util/hist.h
> > @@ -381,4 +381,13 @@ int parse_filter_percentage(const struct option *opt __maybe_unused,
> >  			    const char *arg, int unset __maybe_unused);
> >  int perf_hist_config(const char *var, const char *value);
> >  
> > +#define HIERARCHY_INDENT  3
> > +
> > +int perf_hpp__count_sort_keys(void);
> > +
> > +struct rb_node *rb_hierarchy_first(struct rb_node *node);
> > +struct rb_node *rb_hierarchy_last(struct rb_node *node);
> > +struct rb_node *rb_hierarchy_next(struct rb_node *node);
> > +struct rb_node *rb_hierarchy_prev(struct rb_node *node);
> > +
> >  #endif	/* __PERF_HIST_H */
> > -- 
> > 2.6.4

[PATCH v2 03/17] perf hists: Add helper functions for hierarchy mode

[Namhyung Kim]

The rb_hierarchy_{first,last,next,prev} functions are to traverse all
hist entries in a hierarchy.  They will be used by various function
which supports hierarchy output.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
removed rb_hierarchy_first().

 tools/perf/util/hist.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/hist.h |  8 ++++++++
 2 files changed, 53 insertions(+)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 931353f49c72..e9eff7c4bbaf 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -1390,6 +1390,51 @@ void hists__output_resort(struct hists *hists, struct ui_progress *prog)
 	}
 }
 
+struct rb_node *rb_hierarchy_last(struct rb_node *node)
+{
+	struct hist_entry *he = rb_entry(node, struct hist_entry, rb_node);
+
+	while (he->unfolded && !he->leaf) {
+		node = rb_last(&he->hroot_out);
+		he = rb_entry(node, struct hist_entry, rb_node);
+	}
+	return node;
+}
+
+struct rb_node *rb_hierarchy_next(struct rb_node *node)
+{
+	struct hist_entry *he = rb_entry(node, struct hist_entry, rb_node);
+
+	if (!he->leaf && he->unfolded)
+		node = rb_first(&he->hroot_out);
+	else
+		node = rb_next(node);
+
+	while (node == NULL) {
+		he = he->parent_he;
+		if (he == NULL)
+			break;
+
+		node = rb_next(&he->rb_node);
+	}
+	return node;
+}
+
+struct rb_node *rb_hierarchy_prev(struct rb_node *node)
+{
+	struct hist_entry *he = rb_entry(node, struct hist_entry, rb_node);
+
+	node = rb_prev(node);
+	if (node)
+		return rb_hierarchy_last(node);
+
+	he = he->parent_he;
+	if (he == NULL)
+		return NULL;
+
+	return &he->rb_node;
+}
+
 static void hists__remove_entry_filter(struct hists *hists, struct hist_entry *h,
 				       enum hist_filter filter)
 {
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index d4ec4822a103..2b8a73ea12c3 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -381,4 +381,12 @@ int parse_filter_percentage(const struct option *opt __maybe_unused,
 			    const char *arg, int unset __maybe_unused);
 int perf_hist_config(const char *var, const char *value);
 
+#define HIERARCHY_INDENT  3
+
+int perf_hpp__count_sort_keys(void);
+
+struct rb_node *rb_hierarchy_last(struct rb_node *node);
+struct rb_node *rb_hierarchy_next(struct rb_node *node);
+struct rb_node *rb_hierarchy_prev(struct rb_node *node);
+
 #endif	/* __PERF_HIST_H */
-- 
2.7.0

Re: [PATCH v2 03/17] perf hists: Add helper functions for hierarchy mode

[Namhyung Kim]

On Thu, Jan 21, 2016 at 01:19:31PM +0900, Namhyung Kim wrote:
> The rb_hierarchy_{first,last,next,prev} functions are to traverse all

Ouch, missed to remove 'first' here.


> hist entries in a hierarchy.  They will be used by various function
> which supports hierarchy output.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
> removed rb_hierarchy_first().

[PATCH 04/17] perf hists: Cleanup filtering functions

[Namhyung Kim]

The hists__filter_by_xxx functions share same logic with different
filters.  Factor out the common code into the hists__filter_by_type.

The hists__filter_by_dso() code contained a check for parent, but I
think it should not be there.  The PARENT filter bit was set by
symbol__parent_filter() which is related to symbol instead of dso.  Also
it didn't change the filter state anyway so end result will be same.

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

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 9354455aec5b..0790c053f65c 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -1471,28 +1471,6 @@ static bool hists__filter_entry_by_dso(struct hists *hists,
 	return false;
 }
 
-void hists__filter_by_dso(struct hists *hists)
-{
-	struct rb_node *nd;
-
-	hists->stats.nr_non_filtered_samples = 0;
-
-	hists__reset_filter_stats(hists);
-	hists__reset_col_len(hists);
-
-	for (nd = rb_first(&hists->entries); nd; nd = rb_next(nd)) {
-		struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
-
-		if (symbol_conf.exclude_other && !h->parent)
-			continue;
-
-		if (hists__filter_entry_by_dso(hists, h))
-			continue;
-
-		hists__remove_entry_filter(hists, h, HIST_FILTER__DSO);
-	}
-}
-
 static bool hists__filter_entry_by_thread(struct hists *hists,
 					  struct hist_entry *he)
 {
@@ -1505,25 +1483,6 @@ static bool hists__filter_entry_by_thread(struct hists *hists,
 	return false;
 }
 
-void hists__filter_by_thread(struct hists *hists)
-{
-	struct rb_node *nd;
-
-	hists->stats.nr_non_filtered_samples = 0;
-
-	hists__reset_filter_stats(hists);
-	hists__reset_col_len(hists);
-
-	for (nd = rb_first(&hists->entries); nd; nd = rb_next(nd)) {
-		struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
-
-		if (hists__filter_entry_by_thread(hists, h))
-			continue;
-
-		hists__remove_entry_filter(hists, h, HIST_FILTER__THREAD);
-	}
-}
-
 static bool hists__filter_entry_by_symbol(struct hists *hists,
 					  struct hist_entry *he)
 {
@@ -1537,25 +1496,6 @@ static bool hists__filter_entry_by_symbol(struct hists *hists,
 	return false;
 }
 
-void hists__filter_by_symbol(struct hists *hists)
-{
-	struct rb_node *nd;
-
-	hists->stats.nr_non_filtered_samples = 0;
-
-	hists__reset_filter_stats(hists);
-	hists__reset_col_len(hists);
-
-	for (nd = rb_first(&hists->entries); nd; nd = rb_next(nd)) {
-		struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
-
-		if (hists__filter_entry_by_symbol(hists, h))
-			continue;
-
-		hists__remove_entry_filter(hists, h, HIST_FILTER__SYMBOL);
-	}
-}
-
 static bool hists__filter_entry_by_socket(struct hists *hists,
 					  struct hist_entry *he)
 {
@@ -1568,7 +1508,9 @@ static bool hists__filter_entry_by_socket(struct hists *hists,
 	return false;
 }
 
-void hists__filter_by_socket(struct hists *hists)
+typedef bool (*filter_fn_t)(struct hists *hists, struct hist_entry *he);
+
+static void hists__filter_by_type(struct hists *hists, int type, filter_fn_t filter)
 {
 	struct rb_node *nd;
 
@@ -1580,13 +1522,37 @@ void hists__filter_by_socket(struct hists *hists)
 	for (nd = rb_first(&hists->entries); nd; nd = rb_next(nd)) {
 		struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
 
-		if (hists__filter_entry_by_socket(hists, h))
+		if (filter(hists, h))
 			continue;
 
-		hists__remove_entry_filter(hists, h, HIST_FILTER__SOCKET);
+		hists__remove_entry_filter(hists, h, type);
 	}
 }
 
+void hists__filter_by_thread(struct hists *hists)
+{
+	hists__filter_by_type(hists, HIST_FILTER__THREAD,
+			      hists__filter_entry_by_thread);
+}
+
+void hists__filter_by_dso(struct hists *hists)
+{
+	hists__filter_by_type(hists, HIST_FILTER__DSO,
+			      hists__filter_entry_by_dso);
+}
+
+void hists__filter_by_symbol(struct hists *hists)
+{
+	hists__filter_by_type(hists, HIST_FILTER__SYMBOL,
+			      hists__filter_entry_by_symbol);
+}
+
+void hists__filter_by_socket(struct hists *hists)
+{
+	hists__filter_by_type(hists, HIST_FILTER__SOCKET,
+			      hists__filter_entry_by_socket);
+}
+
 void events_stats__inc(struct events_stats *stats, u32 type)
 {
 	++stats->nr_events[0];
-- 
2.6.4

Re: [PATCH 04/17] perf hists: Cleanup filtering functions

[Arnaldo Carvalho de Melo]

Em Sun, Jan 17, 2016 at 01:03:04AM +0900, Namhyung Kim escreveu:
> The hists__filter_by_xxx functions share same logic with different
> filters.  Factor out the common code into the hists__filter_by_type.
> 
> The hists__filter_by_dso() code contained a check for parent, but I
> think it should not be there.  The PARENT filter bit was set by
> symbol__parent_filter() which is related to symbol instead of dso.  Also

Ok, so break the patch in two, one removing the check for parent in
hists__filter_by_dso(), then the patch introducing the function that
receives the filter callback + type,

Thanks,

- Arnaldo

> it didn't change the filter state anyway so end result will be same.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/hist.c | 92 ++++++++++++++++----------------------------------
>  1 file changed, 29 insertions(+), 63 deletions(-)
> 
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index 9354455aec5b..0790c053f65c 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -1471,28 +1471,6 @@ static bool hists__filter_entry_by_dso(struct hists *hists,
>  	return false;
>  }
>  
> -void hists__filter_by_dso(struct hists *hists)
> -{
> -	struct rb_node *nd;
> -
> -	hists->stats.nr_non_filtered_samples = 0;
> -
> -	hists__reset_filter_stats(hists);
> -	hists__reset_col_len(hists);
> -
> -	for (nd = rb_first(&hists->entries); nd; nd = rb_next(nd)) {
> -		struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
> -
> -		if (symbol_conf.exclude_other && !h->parent)
> -			continue;
> -
> -		if (hists__filter_entry_by_dso(hists, h))
> -			continue;
> -
> -		hists__remove_entry_filter(hists, h, HIST_FILTER__DSO);
> -	}
> -}
> -
>  static bool hists__filter_entry_by_thread(struct hists *hists,
>  					  struct hist_entry *he)
>  {
> @@ -1505,25 +1483,6 @@ static bool hists__filter_entry_by_thread(struct hists *hists,
>  	return false;
>  }
>  
> -void hists__filter_by_thread(struct hists *hists)
> -{
> -	struct rb_node *nd;
> -
> -	hists->stats.nr_non_filtered_samples = 0;
> -
> -	hists__reset_filter_stats(hists);
> -	hists__reset_col_len(hists);
> -
> -	for (nd = rb_first(&hists->entries); nd; nd = rb_next(nd)) {
> -		struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
> -
> -		if (hists__filter_entry_by_thread(hists, h))
> -			continue;
> -
> -		hists__remove_entry_filter(hists, h, HIST_FILTER__THREAD);
> -	}
> -}
> -
>  static bool hists__filter_entry_by_symbol(struct hists *hists,
>  					  struct hist_entry *he)
>  {
> @@ -1537,25 +1496,6 @@ static bool hists__filter_entry_by_symbol(struct hists *hists,
>  	return false;
>  }
>  
> -void hists__filter_by_symbol(struct hists *hists)
> -{
> -	struct rb_node *nd;
> -
> -	hists->stats.nr_non_filtered_samples = 0;
> -
> -	hists__reset_filter_stats(hists);
> -	hists__reset_col_len(hists);
> -
> -	for (nd = rb_first(&hists->entries); nd; nd = rb_next(nd)) {
> -		struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
> -
> -		if (hists__filter_entry_by_symbol(hists, h))
> -			continue;
> -
> -		hists__remove_entry_filter(hists, h, HIST_FILTER__SYMBOL);
> -	}
> -}
> -
>  static bool hists__filter_entry_by_socket(struct hists *hists,
>  					  struct hist_entry *he)
>  {
> @@ -1568,7 +1508,9 @@ static bool hists__filter_entry_by_socket(struct hists *hists,
>  	return false;
>  }
>  
> -void hists__filter_by_socket(struct hists *hists)
> +typedef bool (*filter_fn_t)(struct hists *hists, struct hist_entry *he);
> +
> +static void hists__filter_by_type(struct hists *hists, int type, filter_fn_t filter)
>  {
>  	struct rb_node *nd;
>  
> @@ -1580,13 +1522,37 @@ void hists__filter_by_socket(struct hists *hists)
>  	for (nd = rb_first(&hists->entries); nd; nd = rb_next(nd)) {
>  		struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
>  
> -		if (hists__filter_entry_by_socket(hists, h))
> +		if (filter(hists, h))
>  			continue;
>  
> -		hists__remove_entry_filter(hists, h, HIST_FILTER__SOCKET);
> +		hists__remove_entry_filter(hists, h, type);
>  	}
>  }
>  
> +void hists__filter_by_thread(struct hists *hists)
> +{
> +	hists__filter_by_type(hists, HIST_FILTER__THREAD,
> +			      hists__filter_entry_by_thread);
> +}
> +
> +void hists__filter_by_dso(struct hists *hists)
> +{
> +	hists__filter_by_type(hists, HIST_FILTER__DSO,
> +			      hists__filter_entry_by_dso);
> +}
> +
> +void hists__filter_by_symbol(struct hists *hists)
> +{
> +	hists__filter_by_type(hists, HIST_FILTER__SYMBOL,
> +			      hists__filter_entry_by_symbol);
> +}
> +
> +void hists__filter_by_socket(struct hists *hists)
> +{
> +	hists__filter_by_type(hists, HIST_FILTER__SOCKET,
> +			      hists__filter_entry_by_socket);
> +}
> +
>  void events_stats__inc(struct events_stats *stats, u32 type)
>  {
>  	++stats->nr_events[0];
> -- 
> 2.6.4

[PATCH v2 04.1/17] perf hists: Remove parent filter check in DSO filter function

[Namhyung Kim]

The --exclude-other option sets HIST_FILTER__PARENT bit and it's only
set when a hist entry was created.  DSO filters don't change this so
no need to have the check in hists__filter_by_dso() IMHO.

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

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 9354455aec5b..7c9af05726ad 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -1483,9 +1483,6 @@ void hists__filter_by_dso(struct hists *hists)
 	for (nd = rb_first(&hists->entries); nd; nd = rb_next(nd)) {
 		struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
 
-		if (symbol_conf.exclude_other && !h->parent)
-			continue;
-
 		if (hists__filter_entry_by_dso(hists, h))
 			continue;
 
-- 
2.7.0

[PATCH v2 04.2/17] perf hists: Cleanup filtering functions

[Namhyung Kim]

The hists__filter_by_xxx functions share same logic with different
filters.  Factor out the common code into the hists__filter_by_type.

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

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 7c9af05726ad..0790c053f65c 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -1471,25 +1471,6 @@ static bool hists__filter_entry_by_dso(struct hists *hists,
 	return false;
 }
 
-void hists__filter_by_dso(struct hists *hists)
-{
-	struct rb_node *nd;
-
-	hists->stats.nr_non_filtered_samples = 0;
-
-	hists__reset_filter_stats(hists);
-	hists__reset_col_len(hists);
-
-	for (nd = rb_first(&hists->entries); nd; nd = rb_next(nd)) {
-		struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
-
-		if (hists__filter_entry_by_dso(hists, h))
-			continue;
-
-		hists__remove_entry_filter(hists, h, HIST_FILTER__DSO);
-	}
-}
-
 static bool hists__filter_entry_by_thread(struct hists *hists,
 					  struct hist_entry *he)
 {
@@ -1502,25 +1483,6 @@ static bool hists__filter_entry_by_thread(struct hists *hists,
 	return false;
 }
 
-void hists__filter_by_thread(struct hists *hists)
-{
-	struct rb_node *nd;
-
-	hists->stats.nr_non_filtered_samples = 0;
-
-	hists__reset_filter_stats(hists);
-	hists__reset_col_len(hists);
-
-	for (nd = rb_first(&hists->entries); nd; nd = rb_next(nd)) {
-		struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
-
-		if (hists__filter_entry_by_thread(hists, h))
-			continue;
-
-		hists__remove_entry_filter(hists, h, HIST_FILTER__THREAD);
-	}
-}
-
 static bool hists__filter_entry_by_symbol(struct hists *hists,
 					  struct hist_entry *he)
 {
@@ -1534,25 +1496,6 @@ static bool hists__filter_entry_by_symbol(struct hists *hists,
 	return false;
 }
 
-void hists__filter_by_symbol(struct hists *hists)
-{
-	struct rb_node *nd;
-
-	hists->stats.nr_non_filtered_samples = 0;
-
-	hists__reset_filter_stats(hists);
-	hists__reset_col_len(hists);
-
-	for (nd = rb_first(&hists->entries); nd; nd = rb_next(nd)) {
-		struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
-
-		if (hists__filter_entry_by_symbol(hists, h))
-			continue;
-
-		hists__remove_entry_filter(hists, h, HIST_FILTER__SYMBOL);
-	}
-}
-
 static bool hists__filter_entry_by_socket(struct hists *hists,
 					  struct hist_entry *he)
 {
@@ -1565,7 +1508,9 @@ static bool hists__filter_entry_by_socket(struct hists *hists,
 	return false;
 }
 
-void hists__filter_by_socket(struct hists *hists)
+typedef bool (*filter_fn_t)(struct hists *hists, struct hist_entry *he);
+
+static void hists__filter_by_type(struct hists *hists, int type, filter_fn_t filter)
 {
 	struct rb_node *nd;
 
@@ -1577,13 +1522,37 @@ void hists__filter_by_socket(struct hists *hists)
 	for (nd = rb_first(&hists->entries); nd; nd = rb_next(nd)) {
 		struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
 
-		if (hists__filter_entry_by_socket(hists, h))
+		if (filter(hists, h))
 			continue;
 
-		hists__remove_entry_filter(hists, h, HIST_FILTER__SOCKET);
+		hists__remove_entry_filter(hists, h, type);
 	}
 }
 
+void hists__filter_by_thread(struct hists *hists)
+{
+	hists__filter_by_type(hists, HIST_FILTER__THREAD,
+			      hists__filter_entry_by_thread);
+}
+
+void hists__filter_by_dso(struct hists *hists)
+{
+	hists__filter_by_type(hists, HIST_FILTER__DSO,
+			      hists__filter_entry_by_dso);
+}
+
+void hists__filter_by_symbol(struct hists *hists)
+{
+	hists__filter_by_type(hists, HIST_FILTER__SYMBOL,
+			      hists__filter_entry_by_symbol);
+}
+
+void hists__filter_by_socket(struct hists *hists)
+{
+	hists__filter_by_type(hists, HIST_FILTER__SOCKET,
+			      hists__filter_entry_by_socket);
+}
+
 void events_stats__inc(struct events_stats *stats, u32 type)
 {
 	++stats->nr_events[0];
-- 
2.7.0

Re: [PATCH v2 04.2/17] perf hists: Cleanup filtering functions

[Jiri Olsa]

On Wed, Jan 20, 2016 at 10:15:21AM +0900, Namhyung Kim wrote:
> The hists__filter_by_xxx functions share same logic with different
> filters.  Factor out the common code into the hists__filter_by_type.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Acked-by: Jiri Olsa <jolsa@kernel.org>

jirka

[tip:perf/core] perf hists: Cleanup filtering functions

[tip-bot for Namhyung Kim]

Commit-ID:  1f7c254132f098d19ff3fd452ba9f826cd85c4c0
Gitweb:     http://git.kernel.org/tip/1f7c254132f098d19ff3fd452ba9f826cd85c4c0
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Wed, 20 Jan 2016 10:15:21 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 26 Jan 2016 11:52:45 -0300

perf hists: Cleanup filtering functions

The hists__filter_by_xxx functions share same logic with different
filters.  Factor out the common code into the hists__filter_by_type.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Pekka Enberg <penberg@kernel.org>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Stephane Eranian <eranian@google.com>
Cc: Wang Nan <wangnan0@huawei.com>
Link: http://lkml.kernel.org/r/1453252521-24398-2-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/hist.c | 89 ++++++++++++++++----------------------------------
 1 file changed, 29 insertions(+), 60 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 1d8c8ea..81ce0af 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -1254,25 +1254,6 @@ static bool hists__filter_entry_by_dso(struct hists *hists,
 	return false;
 }
 
-void hists__filter_by_dso(struct hists *hists)
-{
-	struct rb_node *nd;
-
-	hists->stats.nr_non_filtered_samples = 0;
-
-	hists__reset_filter_stats(hists);
-	hists__reset_col_len(hists);
-
-	for (nd = rb_first(&hists->entries); nd; nd = rb_next(nd)) {
-		struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
-
-		if (hists__filter_entry_by_dso(hists, h))
-			continue;
-
-		hists__remove_entry_filter(hists, h, HIST_FILTER__DSO);
-	}
-}
-
 static bool hists__filter_entry_by_thread(struct hists *hists,
 					  struct hist_entry *he)
 {
@@ -1285,25 +1266,6 @@ static bool hists__filter_entry_by_thread(struct hists *hists,
 	return false;
 }
 
-void hists__filter_by_thread(struct hists *hists)
-{
-	struct rb_node *nd;
-
-	hists->stats.nr_non_filtered_samples = 0;
-
-	hists__reset_filter_stats(hists);
-	hists__reset_col_len(hists);
-
-	for (nd = rb_first(&hists->entries); nd; nd = rb_next(nd)) {
-		struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
-
-		if (hists__filter_entry_by_thread(hists, h))
-			continue;
-
-		hists__remove_entry_filter(hists, h, HIST_FILTER__THREAD);
-	}
-}
-
 static bool hists__filter_entry_by_symbol(struct hists *hists,
 					  struct hist_entry *he)
 {
@@ -1317,25 +1279,6 @@ static bool hists__filter_entry_by_symbol(struct hists *hists,
 	return false;
 }
 
-void hists__filter_by_symbol(struct hists *hists)
-{
-	struct rb_node *nd;
-
-	hists->stats.nr_non_filtered_samples = 0;
-
-	hists__reset_filter_stats(hists);
-	hists__reset_col_len(hists);
-
-	for (nd = rb_first(&hists->entries); nd; nd = rb_next(nd)) {
-		struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
-
-		if (hists__filter_entry_by_symbol(hists, h))
-			continue;
-
-		hists__remove_entry_filter(hists, h, HIST_FILTER__SYMBOL);
-	}
-}
-
 static bool hists__filter_entry_by_socket(struct hists *hists,
 					  struct hist_entry *he)
 {
@@ -1348,7 +1291,9 @@ static bool hists__filter_entry_by_socket(struct hists *hists,
 	return false;
 }
 
-void hists__filter_by_socket(struct hists *hists)
+typedef bool (*filter_fn_t)(struct hists *hists, struct hist_entry *he);
+
+static void hists__filter_by_type(struct hists *hists, int type, filter_fn_t filter)
 {
 	struct rb_node *nd;
 
@@ -1360,13 +1305,37 @@ void hists__filter_by_socket(struct hists *hists)
 	for (nd = rb_first(&hists->entries); nd; nd = rb_next(nd)) {
 		struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
 
-		if (hists__filter_entry_by_socket(hists, h))
+		if (filter(hists, h))
 			continue;
 
-		hists__remove_entry_filter(hists, h, HIST_FILTER__SOCKET);
+		hists__remove_entry_filter(hists, h, type);
 	}
 }
 
+void hists__filter_by_thread(struct hists *hists)
+{
+	hists__filter_by_type(hists, HIST_FILTER__THREAD,
+			      hists__filter_entry_by_thread);
+}
+
+void hists__filter_by_dso(struct hists *hists)
+{
+	hists__filter_by_type(hists, HIST_FILTER__DSO,
+			      hists__filter_entry_by_dso);
+}
+
+void hists__filter_by_symbol(struct hists *hists)
+{
+	hists__filter_by_type(hists, HIST_FILTER__SYMBOL,
+			      hists__filter_entry_by_symbol);
+}
+
+void hists__filter_by_socket(struct hists *hists)
+{
+	hists__filter_by_type(hists, HIST_FILTER__SOCKET,
+			      hists__filter_entry_by_socket);
+}
+
 void events_stats__inc(struct events_stats *stats, u32 type)
 {
 	++stats->nr_events[0];

Re: [PATCH v2 04.1/17] perf hists: Remove parent filter check in DSO filter function

[Jiri Olsa]

On Wed, Jan 20, 2016 at 10:15:20AM +0900, Namhyung Kim wrote:
> The --exclude-other option sets HIST_FILTER__PARENT bit and it's only
> set when a hist entry was created.  DSO filters don't change this so
> no need to have the check in hists__filter_by_dso() IMHO.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Acked-by: Jiri Olsa <jolsa@kernel.org>

jirka

> ---
>  tools/perf/util/hist.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index 9354455aec5b..7c9af05726ad 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -1483,9 +1483,6 @@ void hists__filter_by_dso(struct hists *hists)
>  	for (nd = rb_first(&hists->entries); nd; nd = rb_next(nd)) {
>  		struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
>  
> -		if (symbol_conf.exclude_other && !h->parent)
> -			continue;
> -
>  		if (hists__filter_entry_by_dso(hists, h))
>  			continue;
>  
> -- 
> 2.7.0
> 

[tip:perf/core] perf hists: Remove parent filter check in DSO filter function

[tip-bot for Namhyung Kim]

Commit-ID:  c84a5d16711619621f368e84a179790df3377c87
Gitweb:     http://git.kernel.org/tip/c84a5d16711619621f368e84a179790df3377c87
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Wed, 20 Jan 2016 10:15:20 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 26 Jan 2016 11:52:44 -0300

perf hists: Remove parent filter check in DSO filter function

The --exclude-other option sets HIST_FILTER__PARENT bit and it's only
set when a hist entry was created.  DSO filters don't change this so no
need to have the check in hists__filter_by_dso() IMHO.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Pekka Enberg <penberg@kernel.org>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Stephane Eranian <eranian@google.com>
Cc: Wang Nan <wangnan0@huawei.com>
Link: http://lkml.kernel.org/r/1453252521-24398-1-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/hist.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 68a7612..1d8c8ea 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -1266,9 +1266,6 @@ void hists__filter_by_dso(struct hists *hists)
 	for (nd = rb_first(&hists->entries); nd; nd = rb_next(nd)) {
 		struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
 
-		if (symbol_conf.exclude_other && !h->parent)
-			continue;
-
 		if (hists__filter_entry_by_dso(hists, h))
 			continue;
 

[PATCH 05/17] perf hists: Support filtering in hierarchy mode

[Namhyung Kim]

The hists__filter_hierarchy() function implements filtering in hierarchy
mode.  Now struct perf_hpp_fmt has optional ->filter callback which is
called from this function.  The callback can return 3 kind of values.

A negative value means that it's not filtered by this type.  It marks
current entry as filtered tentatively so if a lower level entry removes
the filter it also removes the all parent so that we can find the entry
in the output.

Zero means it's filtered out by this type and positive value means it's
not filtered so it removes the filter.  In these cases, it moves to next
entry since lower level entry won't match by this type of filter
anymore.  Thus all children will be filtered or not together.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/hist.c | 131 ++++++++++++++++++++++++++++++++++++++++++++++---
 tools/perf/util/hist.h |   2 +
 tools/perf/util/sort.c | 113 ++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/sort.h |   1 +
 4 files changed, 239 insertions(+), 8 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 0790c053f65c..a0808f73e0d9 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -1444,6 +1444,27 @@ static void hists__remove_entry_filter(struct hists *hists, struct hist_entry *h
 				       enum hist_filter filter)
 {
 	h->filtered &= ~(1 << filter);
+
+	if (symbol_conf.report_hierarchy) {
+		struct hist_entry *parent = h->parent_he;
+
+		while (parent) {
+			he_stat__add_stat(&parent->stat, &h->stat);
+
+			parent->filtered &= ~(1 << filter);
+
+			if (parent->filtered)
+				goto next;
+
+			/* force fold unfiltered entry for simplicity */
+			parent->unfolded = false;
+			parent->row_offset = 0;
+			parent->nr_rows = 0;
+next:
+			parent = parent->parent_he;
+		}
+	}
+
 	if (h->filtered)
 		return;
 
@@ -1529,28 +1550,122 @@ static void hists__filter_by_type(struct hists *hists, int type, filter_fn_t fil
 	}
 }
 
+static void hists__filter_hierarchy(struct hists *hists, int type, const void *arg)
+{
+	struct rb_node *nd;
+	struct rb_root tmp = RB_ROOT;
+	bool saved_unfolded;
+
+	hists->stats.nr_non_filtered_samples = 0;
+
+	hists__reset_filter_stats(hists);
+	hists__reset_col_len(hists);
+
+	nd = rb_first(&hists->entries);
+	while (nd) {
+		struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
+		int ret;
+
+		ret = hist_entry__filter(h, type, arg);
+
+		/*
+		 * case 1. non-matching type
+		 * zero out the period, set filtered and move to child
+		 */
+		if (ret < 0) {
+			memset(&h->stat, 0, sizeof(h->stat));
+			h->filtered |= (1 << type);
+
+			/* force to go down in the hierarchy */
+			saved_unfolded = h->unfolded;
+			h->unfolded = true;
+
+			nd = rb_hierarchy_next(&h->rb_node);
+			h->unfolded = saved_unfolded;
+		}
+		/*
+		 * case 2. matched (filter out)
+		 * set filtered and move to next
+		 */
+		else if (ret == 1) {
+			h->filtered |= (1 << type);
+
+			/* force to go to sibling in the hierarchy */
+			saved_unfolded = h->unfolded;
+			h->unfolded = false;
+
+			nd = rb_hierarchy_next(&h->rb_node);
+			h->unfolded = saved_unfolded;
+		}
+		/*
+		 * case 3. ok (not filtered)
+		 * add period to hists and parents, erase filtered
+		 * and move to next
+		 */
+		else {
+			hists__remove_entry_filter(hists, h, type);
+
+			/* force to go to sibling in the hierarchy */
+			saved_unfolded = h->unfolded;
+			h->unfolded = false;
+
+			nd = rb_hierarchy_next(&h->rb_node);
+			h->unfolded = saved_unfolded;
+		}
+	}
+
+	/* resort output (top-level entries only) */
+	nd = rb_first(&hists->entries);
+	while (nd) {
+		struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
+
+		nd = rb_next(nd);
+		rb_erase(&h->rb_node, &hists->entries);
+
+		__hists__insert_output_entry(&tmp, h, 0, false);
+	}
+
+	hists->entries = tmp;
+}
+
 void hists__filter_by_thread(struct hists *hists)
 {
-	hists__filter_by_type(hists, HIST_FILTER__THREAD,
-			      hists__filter_entry_by_thread);
+	if (symbol_conf.report_hierarchy)
+		hists__filter_hierarchy(hists, HIST_FILTER__THREAD,
+					hists->thread_filter);
+	else
+		hists__filter_by_type(hists, HIST_FILTER__THREAD,
+				      hists__filter_entry_by_thread);
 }
 
 void hists__filter_by_dso(struct hists *hists)
 {
-	hists__filter_by_type(hists, HIST_FILTER__DSO,
-			      hists__filter_entry_by_dso);
+	if (symbol_conf.report_hierarchy)
+		hists__filter_hierarchy(hists, HIST_FILTER__DSO,
+					hists->dso_filter);
+	else
+		hists__filter_by_type(hists, HIST_FILTER__DSO,
+				      hists__filter_entry_by_dso);
 }
 
 void hists__filter_by_symbol(struct hists *hists)
 {
-	hists__filter_by_type(hists, HIST_FILTER__SYMBOL,
-			      hists__filter_entry_by_symbol);
+	if (symbol_conf.report_hierarchy)
+		hists__filter_hierarchy(hists, HIST_FILTER__SYMBOL,
+					hists->symbol_filter_str);
+	else
+		hists__filter_by_type(hists, HIST_FILTER__SYMBOL,
+				      hists__filter_entry_by_symbol);
 }
 
 void hists__filter_by_socket(struct hists *hists)
 {
-	hists__filter_by_type(hists, HIST_FILTER__SOCKET,
-			      hists__filter_entry_by_socket);
+	if (symbol_conf.report_hierarchy)
+		hists__filter_hierarchy(hists, HIST_FILTER__SOCKET,
+					&hists->socket_filter);
+	else
+		hists__filter_by_type(hists, HIST_FILTER__SOCKET,
+				      hists__filter_entry_by_socket);
 }
 
 void events_stats__inc(struct events_stats *stats, u32 type)
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index 96b7ff817d3e..6c276c87cfc3 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -270,6 +270,8 @@ bool perf_hpp__same_sort_entry(struct perf_hpp_fmt *a, struct perf_hpp_fmt *b);
 bool perf_hpp__is_dynamic_entry(struct perf_hpp_fmt *format);
 bool perf_hpp__defined_dynamic_entry(struct perf_hpp_fmt *fmt, struct hists *hists);
 
+int hist_entry__filter(struct hist_entry *he, int type, const void *arg);
+
 static inline bool perf_hpp__should_skip(struct perf_hpp_fmt *format,
 					 struct hists *hists)
 {
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index ec722346e6ff..4632475bc5e4 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -80,10 +80,21 @@ static int hist_entry__thread_snprintf(struct hist_entry *he, char *bf,
 			       width, width, comm ?: "");
 }
 
+static int hist_entry__thread_filter(struct hist_entry *he, int type, const void *arg)
+{
+	const struct thread *th = arg;
+
+	if (type != HIST_FILTER__THREAD)
+		return -1;
+
+	return th && he->thread != th;
+}
+
 struct sort_entry sort_thread = {
 	.se_header	= "  Pid:Command",
 	.se_cmp		= sort__thread_cmp,
 	.se_snprintf	= hist_entry__thread_snprintf,
+	.se_filter	= hist_entry__thread_filter,
 	.se_width_idx	= HISTC_THREAD,
 };
 
@@ -121,6 +132,7 @@ struct sort_entry sort_comm = {
 	.se_collapse	= sort__comm_collapse,
 	.se_sort	= sort__comm_sort,
 	.se_snprintf	= hist_entry__comm_snprintf,
+	.se_filter	= hist_entry__thread_filter,
 	.se_width_idx	= HISTC_COMM,
 };
 
@@ -170,10 +182,21 @@ static int hist_entry__dso_snprintf(struct hist_entry *he, char *bf,
 	return _hist_entry__dso_snprintf(he->ms.map, bf, size, width);
 }
 
+static int hist_entry__dso_filter(struct hist_entry *he, int type, const void *arg)
+{
+	const struct dso *dso = arg;
+
+	if (type != HIST_FILTER__DSO)
+		return -1;
+
+	return dso && (!he->ms.map || he->ms.map->dso != dso);
+}
+
 struct sort_entry sort_dso = {
 	.se_header	= "Shared Object",
 	.se_cmp		= sort__dso_cmp,
 	.se_snprintf	= hist_entry__dso_snprintf,
+	.se_filter	= hist_entry__dso_filter,
 	.se_width_idx	= HISTC_DSO,
 };
 
@@ -274,11 +297,22 @@ static int hist_entry__sym_snprintf(struct hist_entry *he, char *bf,
 					 he->level, bf, size, width);
 }
 
+static int hist_entry__sym_filter(struct hist_entry *he, int type, const void *arg)
+{
+	const char *sym = arg;
+
+	if (type != HIST_FILTER__SYMBOL)
+		return -1;
+
+	return sym && (!he->ms.sym || !strstr(he->ms.sym->name, sym));
+}
+
 struct sort_entry sort_sym = {
 	.se_header	= "Symbol",
 	.se_cmp		= sort__sym_cmp,
 	.se_sort	= sort__sym_sort,
 	.se_snprintf	= hist_entry__sym_snprintf,
+	.se_filter	= hist_entry__sym_filter,
 	.se_width_idx	= HISTC_SYMBOL,
 };
 
@@ -439,10 +473,21 @@ static int hist_entry__socket_snprintf(struct hist_entry *he, char *bf,
 	return repsep_snprintf(bf, size, "%*.*d", width, width-3, he->socket);
 }
 
+static int hist_entry__socket_filter(struct hist_entry *he, int type, const void *arg)
+{
+	int socket = *(const int *)arg;
+
+	if (type != HIST_FILTER__SOCKET)
+		return -1;
+
+	return socket >= 0 && he->socket != socket;
+}
+
 struct sort_entry sort_socket = {
 	.se_header      = "Socket",
 	.se_cmp	        = sort__socket_cmp,
 	.se_snprintf    = hist_entry__socket_snprintf,
+	.se_filter      = hist_entry__socket_filter,
 	.se_width_idx	= HISTC_SOCKET,
 };
 
@@ -532,6 +577,18 @@ static int hist_entry__dso_from_snprintf(struct hist_entry *he, char *bf,
 		return repsep_snprintf(bf, size, "%-*.*s", width, width, "N/A");
 }
 
+static int hist_entry__dso_from_filter(struct hist_entry *he, int type,
+				       const void *arg)
+{
+	const struct dso *dso = arg;
+
+	if (type != HIST_FILTER__DSO)
+		return -1;
+
+	return dso && (!he->branch_info || !he->branch_info->from.map ||
+		       he->branch_info->from.map->dso != dso);
+}
+
 static int64_t
 sort__dso_to_cmp(struct hist_entry *left, struct hist_entry *right)
 {
@@ -552,6 +609,18 @@ static int hist_entry__dso_to_snprintf(struct hist_entry *he, char *bf,
 		return repsep_snprintf(bf, size, "%-*.*s", width, width, "N/A");
 }
 
+static int hist_entry__dso_to_filter(struct hist_entry *he, int type,
+				     const void *arg)
+{
+	const struct dso *dso = arg;
+
+	if (type != HIST_FILTER__DSO)
+		return -1;
+
+	return dso && (!he->branch_info || !he->branch_info->to.map ||
+		       he->branch_info->to.map->dso != dso);
+}
+
 static int64_t
 sort__sym_from_cmp(struct hist_entry *left, struct hist_entry *right)
 {
@@ -613,10 +682,35 @@ static int hist_entry__sym_to_snprintf(struct hist_entry *he, char *bf,
 	return repsep_snprintf(bf, size, "%-*.*s", width, width, "N/A");
 }
 
+static int hist_entry__sym_from_filter(struct hist_entry *he, int type,
+				       const void *arg)
+{
+	const char *sym = arg;
+
+	if (type != HIST_FILTER__SYMBOL)
+		return -1;
+
+	return sym && !(he->branch_info && he->branch_info->from.sym &&
+			strstr(he->branch_info->from.sym->name, sym));
+}
+
+static int hist_entry__sym_to_filter(struct hist_entry *he, int type,
+				       const void *arg)
+{
+	const char *sym = arg;
+
+	if (type != HIST_FILTER__SYMBOL)
+		return -1;
+
+	return sym && !(he->branch_info && he->branch_info->to.sym &&
+		        strstr(he->branch_info->to.sym->name, sym));
+}
+
 struct sort_entry sort_dso_from = {
 	.se_header	= "Source Shared Object",
 	.se_cmp		= sort__dso_from_cmp,
 	.se_snprintf	= hist_entry__dso_from_snprintf,
+	.se_filter	= hist_entry__dso_from_filter,
 	.se_width_idx	= HISTC_DSO_FROM,
 };
 
@@ -624,6 +718,7 @@ struct sort_entry sort_dso_to = {
 	.se_header	= "Target Shared Object",
 	.se_cmp		= sort__dso_to_cmp,
 	.se_snprintf	= hist_entry__dso_to_snprintf,
+	.se_filter	= hist_entry__dso_to_filter,
 	.se_width_idx	= HISTC_DSO_TO,
 };
 
@@ -631,6 +726,7 @@ struct sort_entry sort_sym_from = {
 	.se_header	= "Source Symbol",
 	.se_cmp		= sort__sym_from_cmp,
 	.se_snprintf	= hist_entry__sym_from_snprintf,
+	.se_filter	= hist_entry__sym_from_filter,
 	.se_width_idx	= HISTC_SYMBOL_FROM,
 };
 
@@ -638,6 +734,7 @@ struct sort_entry sort_sym_to = {
 	.se_header	= "Target Symbol",
 	.se_cmp		= sort__sym_to_cmp,
 	.se_snprintf	= hist_entry__sym_to_snprintf,
+	.se_filter	= hist_entry__sym_to_filter,
 	.se_width_idx	= HISTC_SYMBOL_TO,
 };
 
@@ -1575,6 +1672,22 @@ bool perf_hpp__is_sort_entry(struct perf_hpp_fmt *format)
 	return format->header == __sort__hpp_header;
 }
 
+int hist_entry__filter(struct hist_entry *he, int type, const void *arg)
+{
+	struct perf_hpp_fmt *fmt;
+	struct hpp_sort_entry *hse;
+
+	fmt = he->fmt;
+	if (fmt == NULL || !perf_hpp__is_sort_entry(fmt))
+		return -1;
+
+	hse = container_of(fmt, struct hpp_sort_entry, hpp);
+	if (hse->se->se_filter == NULL)
+		return -1;
+
+	return hse->se->se_filter(he, type, arg);
+}
+
 static int __sort_dimension__add_hpp_sort(struct sort_dimension *sd)
 {
 	struct hpp_sort_entry *hse = __sort_dimension__alloc_hpp(sd);
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index 15a75d44de91..051739615847 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -232,6 +232,7 @@ struct sort_entry {
 	int64_t	(*se_sort)(struct hist_entry *, struct hist_entry *);
 	int	(*se_snprintf)(struct hist_entry *he, char *bf, size_t size,
 			       unsigned int width);
+	int	(*se_filter)(struct hist_entry *he, int type, const void *arg);
 	u8	se_width_idx;
 };
 
-- 
2.6.4

[PATCH 06/17] perf ui/stdio: Implement hierarchy output mode

[Namhyung Kim]

The hierarchy output mode is to group entries for each level so that
user can see higher level picture more easily.  It also helps to find
out which component is most costly.  The output will look like below:

      15.11%     swapper
         14.97%     [kernel.vmlinux]
          0.09%     [libahci]
          0.05%     [iwlwifi]
      10.29%     irq/33-iwlwifi
          6.45%     [kernel.vmlinux]
          1.41%     [mac80211]
          1.15%     [iwldvm]
          1.14%     [iwlwifi]
          0.14%     [cfg80211]
       4.81%     firefox
          3.92%     libxul.so
          0.34%     [kernel.vmlinux]

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/hist.c       | 14 +++++++++
 tools/perf/ui/stdio/hist.c | 75 +++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 88 insertions(+), 1 deletion(-)

diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index bf2a66e254ea..86c0f3d0ae04 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -679,3 +679,17 @@ void perf_hpp__set_user_width(const char *width_list_str)
 			break;
 	}
 }
+
+int perf_hpp__count_sort_keys(void)
+{
+	int nr_sort = 0;
+	struct perf_hpp_fmt *fmt;
+
+	perf_hpp__for_each_format(fmt) {
+		if (perf_hpp__is_sort_entry(fmt) ||
+		    perf_hpp__is_dynamic_entry(fmt))
+			nr_sort++;
+	}
+
+	return nr_sort;
+}
diff --git a/tools/perf/ui/stdio/hist.c b/tools/perf/ui/stdio/hist.c
index 387110d50b00..0cd7c651a0e8 100644
--- a/tools/perf/ui/stdio/hist.c
+++ b/tools/perf/ui/stdio/hist.c
@@ -409,6 +409,69 @@ static int hist_entry__snprintf(struct hist_entry *he, struct perf_hpp *hpp)
 	return hpp->buf - start;
 }
 
+static int hist_entry__hierarchy_fprintf(struct hist_entry *he,
+					 struct perf_hpp *hpp,
+					 int nr_sort_key, struct hists *hists,
+					 FILE *fp)
+{
+	const char *sep = symbol_conf.field_sep;
+	struct perf_hpp_fmt *fmt;
+	char *buf = hpp->buf;
+	int ret, printed = 0;
+	bool first = true;
+
+	if (symbol_conf.exclude_other && !he->parent)
+		return 0;
+
+	ret = scnprintf(hpp->buf, hpp->size, "%*s", he->depth * HIERARCHY_INDENT, "");
+	advance_hpp(hpp, ret);
+
+	perf_hpp__for_each_format(fmt) {
+		if (perf_hpp__is_sort_entry(fmt) || perf_hpp__is_dynamic_entry(fmt))
+			break;
+
+		/*
+		 * If there's no field_sep, we still need
+		 * to display initial '  '.
+		 */
+		if (!sep || !first) {
+			ret = scnprintf(hpp->buf, hpp->size, "%s", sep ?: "  ");
+			advance_hpp(hpp, ret);
+		} else
+			first = false;
+
+		if (perf_hpp__use_color() && fmt->color)
+			ret = fmt->color(fmt, hpp, he);
+		else
+			ret = fmt->entry(fmt, hpp, he);
+
+		advance_hpp(hpp, ret);
+	}
+
+	if (sep)
+		ret = scnprintf(hpp->buf, hpp->size, "%s", sep);
+	else
+		ret = scnprintf(hpp->buf, hpp->size, "%*s",
+				(nr_sort_key - 1) * HIERARCHY_INDENT + 2, "");
+	advance_hpp(hpp, ret);
+
+	fmt = he->fmt;
+	if (perf_hpp__use_color() && fmt->color)
+		fmt->color(fmt, hpp, he);
+	else
+		fmt->entry(fmt, hpp, he);
+
+	printed += fprintf(fp, "%s\n", buf);
+
+	if (symbol_conf.use_callchain && he->leaf) {
+		printed += hist_entry__callchain_fprintf(he, hists, fp);
+		goto out;
+	}
+
+out:
+	return printed;
+}
+
 static int hist_entry__fprintf(struct hist_entry *he, size_t size,
 			       struct hists *hists,
 			       char *bf, size_t bfsz, FILE *fp)
@@ -422,6 +485,13 @@ static int hist_entry__fprintf(struct hist_entry *he, size_t size,
 	if (size == 0 || size > bfsz)
 		size = hpp.size = bfsz;
 
+	if (symbol_conf.report_hierarchy) {
+		int nr_sort = perf_hpp__count_sort_keys();
+
+		return hist_entry__hierarchy_fprintf(he, &hpp, nr_sort,
+						     hists, fp);
+	}
+
 	hist_entry__snprintf(he, &hpp);
 
 	ret = fprintf(fp, "%s\n", bf);
@@ -520,7 +590,7 @@ print_entries:
 		goto out;
 	}
 
-	for (nd = rb_first(&hists->entries); nd; nd = rb_next(nd)) {
+	for (nd = rb_first(&hists->entries); nd; nd = rb_hierarchy_next(nd)) {
 		struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
 		float percent;
 
@@ -541,6 +611,9 @@ print_entries:
 						   MAP__FUNCTION, fp);
 			fprintf(fp, "%.10s end\n", graph_dotted_line);
 		}
+
+		if (symbol_conf.report_hierarchy)
+			h->unfolded = true;
 	}
 
 	free(line);
-- 
2.6.4

[PATCH 07/17] perf ui/stdio: Align column header for hierarchy output

[Namhyung Kim]

The hierarchy output mode is to group entries so the existing columns
won't fit to the new output.  Treat all sort keys as a single column and
separate headers by "/".

  #    Overhead  Command / Shared Object
  # ...........  ................................
  #
      15.11%     swapper
         14.97%     [kernel.vmlinux]
          0.09%     [libahci]
          0.05%     [iwlwifi]
  ...

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

diff --git a/tools/perf/ui/stdio/hist.c b/tools/perf/ui/stdio/hist.c
index 0cd7c651a0e8..38279478a06d 100644
--- a/tools/perf/ui/stdio/hist.c
+++ b/tools/perf/ui/stdio/hist.c
@@ -502,6 +502,108 @@ static int hist_entry__fprintf(struct hist_entry *he, size_t size,
 	return ret;
 }
 
+static int print_hierarchy_indent(const char *sep, int nr_sort,
+				  const char *line, FILE *fp)
+{
+	if (sep != NULL || nr_sort < 1)
+		return 0;
+
+	return fprintf(fp, "%-.*s", (nr_sort - 1) * HIERARCHY_INDENT, line);
+}
+
+static int print_hierarchy_header(struct hists *hists, struct perf_hpp *hpp,
+				  const char *sep, FILE *fp)
+{
+	bool first = true;
+	int nr_sort;
+	unsigned width = 0;
+	unsigned header_width = 0;
+	struct perf_hpp_fmt *fmt;
+	const char spaces[] = "                                               "
+	"                                                                     "
+	"                                                                     ";
+	const char dots[] = "................................................."
+	"....................................................................."
+	".....................................................................";
+
+	nr_sort = perf_hpp__count_sort_keys();
+
+	/* preserve max indent depth for column headers */
+	print_hierarchy_indent(sep, nr_sort, spaces, fp);
+
+	perf_hpp__for_each_format(fmt) {
+		if (perf_hpp__is_sort_entry(fmt) || perf_hpp__is_dynamic_entry(fmt))
+			break;
+
+		if (!first)
+			fprintf(fp, "%s", sep ?: "  ");
+		else
+			first = false;
+
+		fmt->header(fmt, hpp, hists_to_evsel(hists));
+		fprintf(fp, "%s", hpp->buf);
+	}
+
+	/* combine sort headers with ' / ' */
+	first = true;
+	perf_hpp__for_each_format(fmt) {
+		if (!perf_hpp__is_sort_entry(fmt) && !perf_hpp__is_dynamic_entry(fmt))
+			continue;
+
+		if (!first)
+			header_width += fprintf(fp, " / ");
+		else {
+			header_width += fprintf(fp, "%s", sep ?: "  ");
+			first = false;
+		}
+
+		fmt->header(fmt, hpp, hists_to_evsel(hists));
+		rtrim(hpp->buf);
+
+		header_width += fprintf(fp, "%s", hpp->buf);
+	}
+
+	/* preserve max indent depth for combined sort headers */
+	print_hierarchy_indent(sep, nr_sort, spaces, fp);
+
+	fprintf(fp, "\n# ");
+
+	/* preserve max indent depth for initial dots */
+	print_hierarchy_indent(sep, nr_sort, dots, fp);
+
+	first = true;
+	perf_hpp__for_each_format(fmt) {
+		if (perf_hpp__is_sort_entry(fmt) || perf_hpp__is_dynamic_entry(fmt))
+			break;
+
+		if (!first)
+			fprintf(fp, "%s", sep ?: "  ");
+		else
+			first = false;
+
+		width = fmt->width(fmt, hpp, hists_to_evsel(hists));
+		fprintf(fp, "%.*s", width, dots);
+	}
+
+	perf_hpp__for_each_format(fmt) {
+		if (!perf_hpp__is_sort_entry(fmt) && !perf_hpp__is_dynamic_entry(fmt))
+			continue;
+
+		width = fmt->width(fmt, hpp, hists_to_evsel(hists));
+		if (width > header_width)
+			header_width = width;
+	}
+
+	fprintf(fp, "%s%-.*s", sep ?: "  ", header_width, dots);
+
+	/* preserve max indent depth for dots under sort headers */
+	print_hierarchy_indent(sep, nr_sort, dots, fp);
+
+	fprintf(fp, "\n#\n");
+
+	return 2;
+}
+
 size_t hists__fprintf(struct hists *hists, bool show_header, int max_rows,
 		      int max_cols, float min_pcnt, FILE *fp)
 {
@@ -533,6 +635,11 @@ size_t hists__fprintf(struct hists *hists, bool show_header, int max_rows,
 
 	fprintf(fp, "# ");
 
+	if (symbol_conf.report_hierarchy) {
+		nr_rows += print_hierarchy_header(hists, &dummy_hpp, sep, fp);
+		goto print_entries;
+	}
+
 	perf_hpp__for_each_format(fmt) {
 		if (perf_hpp__should_skip(fmt, hists))
 			continue;
-- 
2.6.4

Re: [PATCH 07/17] perf ui/stdio: Align column header for hierarchy output

[Arnaldo Carvalho de Melo]

Em Sun, Jan 17, 2016 at 01:03:07AM +0900, Namhyung Kim escreveu:
> The hierarchy output mode is to group entries so the existing columns
> won't fit to the new output.  Treat all sort keys as a single column and
> separate headers by "/".
> 
>   #    Overhead  Command / Shared Object
>   # ...........  ................................
>   #
>       15.11%     swapper
>          14.97%     [kernel.vmlinux]
>           0.09%     [libahci]
>           0.05%     [iwlwifi]
>   ...
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/ui/stdio/hist.c | 107 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 107 insertions(+)
> 
> diff --git a/tools/perf/ui/stdio/hist.c b/tools/perf/ui/stdio/hist.c
> index 0cd7c651a0e8..38279478a06d 100644
> --- a/tools/perf/ui/stdio/hist.c
> +++ b/tools/perf/ui/stdio/hist.c
> @@ -502,6 +502,108 @@ static int hist_entry__fprintf(struct hist_entry *he, size_t size,
>  	return ret;
>  }
>  
> +static int print_hierarchy_indent(const char *sep, int nr_sort,
> +				  const char *line, FILE *fp)
> +{
> +	if (sep != NULL || nr_sort < 1)
> +		return 0;
> +
> +	return fprintf(fp, "%-.*s", (nr_sort - 1) * HIERARCHY_INDENT, line);
> +}
> +
> +static int print_hierarchy_header(struct hists *hists, struct perf_hpp *hpp,
> +				  const char *sep, FILE *fp)
> +{
> +	bool first = true;
> +	int nr_sort;
> +	unsigned width = 0;
> +	unsigned header_width = 0;
> +	struct perf_hpp_fmt *fmt;
> +	const char spaces[] = "                                               "
> +	"                                                                     "
> +	"                                                                     ";
> +	const char dots[] = "................................................."
> +	"....................................................................."
> +	".....................................................................";
> +
> +	nr_sort = perf_hpp__count_sort_keys();
> +
> +	/* preserve max indent depth for column headers */
> +	print_hierarchy_indent(sep, nr_sort, spaces, fp);
> +
> +	perf_hpp__for_each_format(fmt) {
> +		if (perf_hpp__is_sort_entry(fmt) || perf_hpp__is_dynamic_entry(fmt))
> +			break;
> +
> +		if (!first)
> +			fprintf(fp, "%s", sep ?: "  ");
> +		else
> +			first = false;
> +
> +		fmt->header(fmt, hpp, hists_to_evsel(hists));
> +		fprintf(fp, "%s", hpp->buf);

We already have code that prints dots or spaces according to a given
width, but in tools/perf/ui/stdio/hist.c hists__fprintf() it does it
using a fprintf loop, gack, would be better to settle in one way.

Can be done later, sure, processing as-is.

- Arnaldo

> +	}
> +
> +	/* combine sort headers with ' / ' */
> +	first = true;
> +	perf_hpp__for_each_format(fmt) {
> +		if (!perf_hpp__is_sort_entry(fmt) && !perf_hpp__is_dynamic_entry(fmt))
> +			continue;
> +
> +		if (!first)
> +			header_width += fprintf(fp, " / ");
> +		else {
> +			header_width += fprintf(fp, "%s", sep ?: "  ");
> +			first = false;
> +		}
> +
> +		fmt->header(fmt, hpp, hists_to_evsel(hists));
> +		rtrim(hpp->buf);
> +
> +		header_width += fprintf(fp, "%s", hpp->buf);
> +	}
> +
> +	/* preserve max indent depth for combined sort headers */
> +	print_hierarchy_indent(sep, nr_sort, spaces, fp);
> +
> +	fprintf(fp, "\n# ");
> +
> +	/* preserve max indent depth for initial dots */
> +	print_hierarchy_indent(sep, nr_sort, dots, fp);
> +
> +	first = true;
> +	perf_hpp__for_each_format(fmt) {
> +		if (perf_hpp__is_sort_entry(fmt) || perf_hpp__is_dynamic_entry(fmt))
> +			break;
> +
> +		if (!first)
> +			fprintf(fp, "%s", sep ?: "  ");
> +		else
> +			first = false;
> +
> +		width = fmt->width(fmt, hpp, hists_to_evsel(hists));
> +		fprintf(fp, "%.*s", width, dots);
> +	}
> +
> +	perf_hpp__for_each_format(fmt) {
> +		if (!perf_hpp__is_sort_entry(fmt) && !perf_hpp__is_dynamic_entry(fmt))
> +			continue;
> +
> +		width = fmt->width(fmt, hpp, hists_to_evsel(hists));
> +		if (width > header_width)
> +			header_width = width;
> +	}
> +
> +	fprintf(fp, "%s%-.*s", sep ?: "  ", header_width, dots);
> +
> +	/* preserve max indent depth for dots under sort headers */
> +	print_hierarchy_indent(sep, nr_sort, dots, fp);
> +
> +	fprintf(fp, "\n#\n");
> +
> +	return 2;
> +}
> +
>  size_t hists__fprintf(struct hists *hists, bool show_header, int max_rows,
>  		      int max_cols, float min_pcnt, FILE *fp)
>  {
> @@ -533,6 +635,11 @@ size_t hists__fprintf(struct hists *hists, bool show_header, int max_rows,
>  
>  	fprintf(fp, "# ");
>  
> +	if (symbol_conf.report_hierarchy) {
> +		nr_rows += print_hierarchy_header(hists, &dummy_hpp, sep, fp);
> +		goto print_entries;
> +	}
> +
>  	perf_hpp__for_each_format(fmt) {
>  		if (perf_hpp__should_skip(fmt, hists))
>  			continue;
> -- 
> 2.6.4

Re: [PATCH 07/17] perf ui/stdio: Align column header for hierarchy output

[Namhyung Kim]

On Wed, Jan 20, 2016 at 07:40:43PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Sun, Jan 17, 2016 at 01:03:07AM +0900, Namhyung Kim escreveu:
> > The hierarchy output mode is to group entries so the existing columns
> > won't fit to the new output.  Treat all sort keys as a single column and
> > separate headers by "/".

[SNIP]
> > +static int print_hierarchy_header(struct hists *hists, struct perf_hpp *hpp,
> > +				  const char *sep, FILE *fp)
> > +{
> > +	bool first = true;
> > +	int nr_sort;
> > +	unsigned width = 0;
> > +	unsigned header_width = 0;
> > +	struct perf_hpp_fmt *fmt;
> > +	const char spaces[] = "                                               "
> > +	"                                                                     "
> > +	"                                                                     ";
> > +	const char dots[] = "................................................."
> > +	"....................................................................."
> > +	".....................................................................";
> > +
> > +	nr_sort = perf_hpp__count_sort_keys();
> > +
> > +	/* preserve max indent depth for column headers */
> > +	print_hierarchy_indent(sep, nr_sort, spaces, fp);
> > +
> > +	perf_hpp__for_each_format(fmt) {
> > +		if (perf_hpp__is_sort_entry(fmt) || perf_hpp__is_dynamic_entry(fmt))
> > +			break;
> > +
> > +		if (!first)
> > +			fprintf(fp, "%s", sep ?: "  ");
> > +		else
> > +			first = false;
> > +
> > +		fmt->header(fmt, hpp, hists_to_evsel(hists));
> > +		fprintf(fp, "%s", hpp->buf);
> 
> We already have code that prints dots or spaces according to a given
> width, but in tools/perf/ui/stdio/hist.c hists__fprintf() it does it
> using a fprintf loop, gack, would be better to settle in one way.

Agreed.

> 
> Can be done later, sure, processing as-is.

Thanks!
Namhyung


> 
> > +	}
> > +
> > +	/* combine sort headers with ' / ' */
> > +	first = true;
> > +	perf_hpp__for_each_format(fmt) {
> > +		if (!perf_hpp__is_sort_entry(fmt) && !perf_hpp__is_dynamic_entry(fmt))
> > +			continue;
> > +
> > +		if (!first)
> > +			header_width += fprintf(fp, " / ");
> > +		else {
> > +			header_width += fprintf(fp, "%s", sep ?: "  ");
> > +			first = false;
> > +		}
> > +
> > +		fmt->header(fmt, hpp, hists_to_evsel(hists));
> > +		rtrim(hpp->buf);
> > +
> > +		header_width += fprintf(fp, "%s", hpp->buf);
> > +	}
> > +
> > +	/* preserve max indent depth for combined sort headers */
> > +	print_hierarchy_indent(sep, nr_sort, spaces, fp);
> > +
> > +	fprintf(fp, "\n# ");
> > +
> > +	/* preserve max indent depth for initial dots */
> > +	print_hierarchy_indent(sep, nr_sort, dots, fp);
> > +
> > +	first = true;
> > +	perf_hpp__for_each_format(fmt) {
> > +		if (perf_hpp__is_sort_entry(fmt) || perf_hpp__is_dynamic_entry(fmt))
> > +			break;
> > +
> > +		if (!first)
> > +			fprintf(fp, "%s", sep ?: "  ");
> > +		else
> > +			first = false;
> > +
> > +		width = fmt->width(fmt, hpp, hists_to_evsel(hists));
> > +		fprintf(fp, "%.*s", width, dots);
> > +	}
> > +
> > +	perf_hpp__for_each_format(fmt) {
> > +		if (!perf_hpp__is_sort_entry(fmt) && !perf_hpp__is_dynamic_entry(fmt))
> > +			continue;
> > +
> > +		width = fmt->width(fmt, hpp, hists_to_evsel(hists));
> > +		if (width > header_width)
> > +			header_width = width;
> > +	}
> > +
> > +	fprintf(fp, "%s%-.*s", sep ?: "  ", header_width, dots);
> > +
> > +	/* preserve max indent depth for dots under sort headers */
> > +	print_hierarchy_indent(sep, nr_sort, dots, fp);
> > +
> > +	fprintf(fp, "\n#\n");
> > +
> > +	return 2;
> > +}
> > +
> >  size_t hists__fprintf(struct hists *hists, bool show_header, int max_rows,
> >  		      int max_cols, float min_pcnt, FILE *fp)
> >  {
> > @@ -533,6 +635,11 @@ size_t hists__fprintf(struct hists *hists, bool show_header, int max_rows,
> >  
> >  	fprintf(fp, "# ");
> >  
> > +	if (symbol_conf.report_hierarchy) {
> > +		nr_rows += print_hierarchy_header(hists, &dummy_hpp, sep, fp);
> > +		goto print_entries;
> > +	}
> > +
> >  	perf_hpp__for_each_format(fmt) {
> >  		if (perf_hpp__should_skip(fmt, hists))
> >  			continue;
> > -- 
> > 2.6.4

[PATCH 08/17] perf hists browser: Fix context menu item

[Namhyung Kim]

When symbol sort key is not given, it doesn't show any item other than
exit.  Check sort key to select possible items.  Also check items more
strictly using sort key information.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/browsers/hists.c | 50 ++++++++++++++++++++++++------------------
 tools/perf/util/sort.c         |  3 +++
 tools/perf/util/sort.h         |  2 ++
 3 files changed, 34 insertions(+), 21 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 08c09ad755d2..cd6349ebd0d6 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -2263,10 +2263,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 			continue;
 		}
 
-		if (!sort__has_sym)
-			goto add_exit_option;
-
-		if (browser->selection == NULL)
+		if (!sort__has_sym || browser->selection == NULL)
 			goto skip_annotation;
 
 		if (sort__mode == SORT_MODE__BRANCH) {
@@ -2294,23 +2291,33 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 						       browser->selection->sym);
 		}
 skip_annotation:
-		nr_options += add_thread_opt(browser, &actions[nr_options],
-					     &options[nr_options], thread);
-		nr_options += add_dso_opt(browser, &actions[nr_options],
-					  &options[nr_options], map);
-		nr_options += add_map_opt(browser, &actions[nr_options],
-					  &options[nr_options],
-					  browser->selection ?
-						browser->selection->map : NULL);
-		nr_options += add_socket_opt(browser, &actions[nr_options],
-					     &options[nr_options],
-					     socked_id);
+		if (sort__has_thread) {
+			nr_options += add_thread_opt(browser, &actions[nr_options],
+						     &options[nr_options], thread);
+		}
+		if (sort__has_dso) {
+			nr_options += add_dso_opt(browser, &actions[nr_options],
+						  &options[nr_options], map);
+			nr_options += add_map_opt(browser, &actions[nr_options],
+						  &options[nr_options],
+						  browser->selection ?
+						  browser->selection->map : NULL);
+		}
+		if (sort__has_socket) {
+			nr_options += add_socket_opt(browser, &actions[nr_options],
+						     &options[nr_options],
+						     socked_id);
+		}
+
 		/* perf script support */
 		if (browser->he_selection) {
-			nr_options += add_script_opt(browser,
-						     &actions[nr_options],
-						     &options[nr_options],
-						     thread, NULL);
+			if (sort__has_thread) {
+				nr_options += add_script_opt(browser,
+							     &actions[nr_options],
+							     &options[nr_options],
+							     thread, NULL);
+			}
+
 			/*
 			 * Note that browser->selection != NULL
 			 * when browser->he_selection is not NULL,
@@ -2320,16 +2327,17 @@ skip_annotation:
 			 *
 			 * See hist_browser__show_entry.
 			 */
-			nr_options += add_script_opt(browser,
+			if (sort__has_sym && browser->selection->sym) {
+				nr_options += add_script_opt(browser,
 						     &actions[nr_options],
 						     &options[nr_options],
 						     NULL, browser->selection->sym);
+			}
 		}
 		nr_options += add_script_opt(browser, &actions[nr_options],
 					     &options[nr_options], NULL, NULL);
 		nr_options += add_switch_opt(browser, &actions[nr_options],
 					     &options[nr_options]);
-add_exit_option:
 		nr_options += add_exit_opt(browser, &actions[nr_options],
 					   &options[nr_options]);
 
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 4632475bc5e4..8ff873ee39a8 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -21,6 +21,7 @@ const char	*field_order;
 regex_t		ignore_callees_regex;
 int		have_ignore_callees = 0;
 int		sort__need_collapse = 0;
+int		sort__has_thread = 0;
 int		sort__has_parent = 0;
 int		sort__has_sym = 0;
 int		sort__has_dso = 0;
@@ -2249,6 +2250,8 @@ static int sort_dimension__add(const char *tok,
 			sort__has_dso = 1;
 		} else if (sd->entry == &sort_socket) {
 			sort__has_socket = 1;
+		} else if (sd->entry == &sort_comm || sd->entry == &sort_thread) {
+			sort__has_thread = 1;
 		}
 
 		return __sort_dimension__add(sd);
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index 051739615847..879513e61dba 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -32,7 +32,9 @@ extern const char default_sort_order[];
 extern regex_t ignore_callees_regex;
 extern int have_ignore_callees;
 extern int sort__need_collapse;
+extern int sort__has_thread;
 extern int sort__has_parent;
+extern int sort__has_dso;
 extern int sort__has_sym;
 extern int sort__has_socket;
 extern enum sort_mode sort__mode;
-- 
2.6.4

Re: [PATCH 08/17] perf hists browser: Fix context menu item

[Arnaldo Carvalho de Melo]

Em Sun, Jan 17, 2016 at 01:03:08AM +0900, Namhyung Kim escreveu:
> When symbol sort key is not given, it doesn't show any item other than
> exit.  Check sort key to select possible items.  Also check items more
> strictly using sort key information.

So, without this patch when I press enter on 'perf top' I can zoom into
threads, with it I lose that option.

- Arnaldo
 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/ui/browsers/hists.c | 50 ++++++++++++++++++++++++------------------
>  tools/perf/util/sort.c         |  3 +++
>  tools/perf/util/sort.h         |  2 ++
>  3 files changed, 34 insertions(+), 21 deletions(-)
> 
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 08c09ad755d2..cd6349ebd0d6 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -2263,10 +2263,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
>  			continue;
>  		}
>  
> -		if (!sort__has_sym)
> -			goto add_exit_option;
> -
> -		if (browser->selection == NULL)
> +		if (!sort__has_sym || browser->selection == NULL)
>  			goto skip_annotation;
>  
>  		if (sort__mode == SORT_MODE__BRANCH) {
> @@ -2294,23 +2291,33 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
>  						       browser->selection->sym);
>  		}
>  skip_annotation:
> -		nr_options += add_thread_opt(browser, &actions[nr_options],
> -					     &options[nr_options], thread);
> -		nr_options += add_dso_opt(browser, &actions[nr_options],
> -					  &options[nr_options], map);
> -		nr_options += add_map_opt(browser, &actions[nr_options],
> -					  &options[nr_options],
> -					  browser->selection ?
> -						browser->selection->map : NULL);
> -		nr_options += add_socket_opt(browser, &actions[nr_options],
> -					     &options[nr_options],
> -					     socked_id);
> +		if (sort__has_thread) {
> +			nr_options += add_thread_opt(browser, &actions[nr_options],
> +						     &options[nr_options], thread);
> +		}
> +		if (sort__has_dso) {
> +			nr_options += add_dso_opt(browser, &actions[nr_options],
> +						  &options[nr_options], map);
> +			nr_options += add_map_opt(browser, &actions[nr_options],
> +						  &options[nr_options],
> +						  browser->selection ?
> +						  browser->selection->map : NULL);
> +		}
> +		if (sort__has_socket) {
> +			nr_options += add_socket_opt(browser, &actions[nr_options],
> +						     &options[nr_options],
> +						     socked_id);
> +		}
> +
>  		/* perf script support */
>  		if (browser->he_selection) {
> -			nr_options += add_script_opt(browser,
> -						     &actions[nr_options],
> -						     &options[nr_options],
> -						     thread, NULL);
> +			if (sort__has_thread) {
> +				nr_options += add_script_opt(browser,
> +							     &actions[nr_options],
> +							     &options[nr_options],
> +							     thread, NULL);
> +			}
> +
>  			/*
>  			 * Note that browser->selection != NULL
>  			 * when browser->he_selection is not NULL,
> @@ -2320,16 +2327,17 @@ skip_annotation:
>  			 *
>  			 * See hist_browser__show_entry.
>  			 */
> -			nr_options += add_script_opt(browser,
> +			if (sort__has_sym && browser->selection->sym) {
> +				nr_options += add_script_opt(browser,
>  						     &actions[nr_options],
>  						     &options[nr_options],
>  						     NULL, browser->selection->sym);
> +			}
>  		}
>  		nr_options += add_script_opt(browser, &actions[nr_options],
>  					     &options[nr_options], NULL, NULL);
>  		nr_options += add_switch_opt(browser, &actions[nr_options],
>  					     &options[nr_options]);
> -add_exit_option:
>  		nr_options += add_exit_opt(browser, &actions[nr_options],
>  					   &options[nr_options]);
>  
> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index 4632475bc5e4..8ff873ee39a8 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
> @@ -21,6 +21,7 @@ const char	*field_order;
>  regex_t		ignore_callees_regex;
>  int		have_ignore_callees = 0;
>  int		sort__need_collapse = 0;
> +int		sort__has_thread = 0;
>  int		sort__has_parent = 0;
>  int		sort__has_sym = 0;
>  int		sort__has_dso = 0;
> @@ -2249,6 +2250,8 @@ static int sort_dimension__add(const char *tok,
>  			sort__has_dso = 1;
>  		} else if (sd->entry == &sort_socket) {
>  			sort__has_socket = 1;
> +		} else if (sd->entry == &sort_comm || sd->entry == &sort_thread) {
> +			sort__has_thread = 1;
>  		}
>  
>  		return __sort_dimension__add(sd);
> diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
> index 051739615847..879513e61dba 100644
> --- a/tools/perf/util/sort.h
> +++ b/tools/perf/util/sort.h
> @@ -32,7 +32,9 @@ extern const char default_sort_order[];
>  extern regex_t ignore_callees_regex;
>  extern int have_ignore_callees;
>  extern int sort__need_collapse;
> +extern int sort__has_thread;
>  extern int sort__has_parent;
> +extern int sort__has_dso;
>  extern int sort__has_sym;
>  extern int sort__has_socket;
>  extern enum sort_mode sort__mode;
> -- 
> 2.6.4

Re: [PATCH 08/17] perf hists browser: Fix context menu item

[Namhyung Kim]

On Wed, Jan 20, 2016 at 09:52:45PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Sun, Jan 17, 2016 at 01:03:08AM +0900, Namhyung Kim escreveu:
> > When symbol sort key is not given, it doesn't show any item other than
> > exit.  Check sort key to select possible items.  Also check items more
> > strictly using sort key information.
> 
> So, without this patch when I press enter on 'perf top' I can zoom into
> threads, with it I lose that option.

Yes, but it was incorrect information.  The default sort key of 'perf
top' doesn't contain 'comm' (or 'pid') so hist entries it shows can
have samples from different threads.  The result is that it only shows
thread of the first sample of the entry.  Filtering based on this
incorrect info should be avoided IMHO.

Thanks,
Namhyung

Re: [PATCH 08/17] perf hists browser: Fix context menu item

[Arnaldo Carvalho de Melo]

Em Thu, Jan 21, 2016 at 01:07:10PM +0900, Namhyung Kim escreveu:
> On Wed, Jan 20, 2016 at 09:52:45PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Sun, Jan 17, 2016 at 01:03:08AM +0900, Namhyung Kim escreveu:
> > > When symbol sort key is not given, it doesn't show any item other than
> > > exit.  Check sort key to select possible items.  Also check items more
> > > strictly using sort key information.

> > So, without this patch when I press enter on 'perf top' I can zoom into
> > threads, with it I lose that option.
 
> Yes, but it was incorrect information.  The default sort key of 'perf
> top' doesn't contain 'comm' (or 'pid') so hist entries it shows can
> have samples from different threads.  The result is that it only shows
> thread of the first sample of the entry.  Filtering based on this
> incorrect info should be avoided IMHO.

Ok, agreed, but this patch is doing way too many things at once, please
take a look at my current perf/core branch, it has the first few patches
I carved out from this one, see if you are ok with it, will continue.

And this is not hierarchy related, so will go first, as fixes in
perf/core.

- Arnaldo

Re: [PATCH 08/17] perf hists browser: Fix context menu item

[Namhyung Kim]

On Thu, Jan 21, 2016 at 08:51:33PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Jan 21, 2016 at 01:07:10PM +0900, Namhyung Kim escreveu:
> > On Wed, Jan 20, 2016 at 09:52:45PM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Sun, Jan 17, 2016 at 01:03:08AM +0900, Namhyung Kim escreveu:
> > > > When symbol sort key is not given, it doesn't show any item other than
> > > > exit.  Check sort key to select possible items.  Also check items more
> > > > strictly using sort key information.
> 
> > > So, without this patch when I press enter on 'perf top' I can zoom into
> > > threads, with it I lose that option.
>  
> > Yes, but it was incorrect information.  The default sort key of 'perf
> > top' doesn't contain 'comm' (or 'pid') so hist entries it shows can
> > have samples from different threads.  The result is that it only shows
> > thread of the first sample of the entry.  Filtering based on this
> > incorrect info should be avoided IMHO.
> 
> Ok, agreed, but this patch is doing way too many things at once, please
> take a look at my current perf/core branch, it has the first few patches
> I carved out from this one, see if you are ok with it, will continue.

Look good to me!

> 
> And this is not hierarchy related, so will go first, as fixes in
> perf/core.

Thanks,
Namhyung

Dynamicly add/remove sort keys was: Re: [PATCH 08/17] perf hists browser: Fix context menu item

[Arnaldo Carvalho de Melo]

Hi Namhyung,

	While continuing to process this series, I thought about one new
feature: A popup menu that would allow adding/removing sort keys, which
would entail resorting with the new sort order, what do you think?

	It may be relatively simple to implement, or I may be missing
something, this is just a brainstorm...

	To avoid reprocessing everything, which may not even be
possible (perf top case) we could mark the field as such and when
showing such hist_entries it would have a <?> marker.

	Say DSO wasn't in the sort order, then each bucket wouldn't be
for just one DSO, sure, in this case, when invoking the popup to add DSO
to the sort order, we would keep those old hist entries around but when
showing them the column for DSO would have <?>, in the top case those
would at some point decay, with the new ones with this info appearing as
time goes by.

	This is part of a general trend of removing the need to restart
the session when wanting to change the sort order, min percent, max
stack, etc, etc.

	Anyway, back to processing patches :-)

- Arnaldo

[tip:perf/core] perf sort: Provide a way to find out if per-thread bucketing is in place

[tip-bot for Namhyung Kim]

Commit-ID:  cfd92dadc5e830268036efb25ff41618f29c3306
Gitweb:     http://git.kernel.org/tip/cfd92dadc5e830268036efb25ff41618f29c3306
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Thu, 21 Jan 2016 19:13:24 -0300
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 26 Jan 2016 11:52:49 -0300

perf sort: Provide a way to find out if per-thread bucketing is in place

Now the UI browsers will be able to offer thread related operations only
if the thread is part of the sort order in use, i.e. if hist_entry stats
are all for a single thread.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@kernel.org>,
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Wang Nan <wangnan0@huawei.com>
Link: http://lkml.kernel.org/r/1452960197-5323-9-git-send-email-namhyung@kernel.org
[ Carved out from a  larger patch ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/sort.c | 3 +++
 tools/perf/util/sort.h | 1 +
 2 files changed, 4 insertions(+)

diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index ec72234..898e4b0 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -25,6 +25,7 @@ int		sort__has_parent = 0;
 int		sort__has_sym = 0;
 int		sort__has_dso = 0;
 int		sort__has_socket = 0;
+int		sort__has_thread = 0;
 enum sort_mode	sort__mode = SORT_MODE__NORMAL;
 
 
@@ -2136,6 +2137,8 @@ static int sort_dimension__add(const char *tok,
 			sort__has_dso = 1;
 		} else if (sd->entry == &sort_socket) {
 			sort__has_socket = 1;
+		} else if (sd->entry == &sort_thread) {
+			sort__has_thread = 1;
 		}
 
 		return __sort_dimension__add(sd);
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index 687bbb12..09616f0 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -35,6 +35,7 @@ extern int sort__need_collapse;
 extern int sort__has_parent;
 extern int sort__has_sym;
 extern int sort__has_socket;
+extern int sort__has_thread;
 extern enum sort_mode sort__mode;
 extern struct sort_entry sort_comm;
 extern struct sort_entry sort_dso;

[tip:perf/core] perf hists browser: Only 'Zoom into thread' only when sort order has 'pid'

[tip-bot for Namhyung Kim]

Commit-ID:  2eafd410e669c744208f8110940e42caa7d79447
Gitweb:     http://git.kernel.org/tip/2eafd410e669c744208f8110940e42caa7d79447
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Thu, 21 Jan 2016 19:13:24 -0300
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 26 Jan 2016 11:52:49 -0300

perf hists browser: Only 'Zoom into thread' only when sort order has 'pid'

We can't offer a zoom into thread when a bucket (struct hist_entry) may
have samples for more than one thread, i.e. when 'pid' is not part of
the sort order, fix it.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@kernel.org>,
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Wang Nan <wangnan0@huawei.com>
Link: http://lkml.kernel.org/r/1452960197-5323-9-git-send-email-namhyung@kernel.org
[ Carved out from a  larger patch ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/ui/browsers/hists.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 08c09ad..e66b3a3 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1782,7 +1782,7 @@ static int
 add_thread_opt(struct hist_browser *browser, struct popup_action *act,
 	       char **optstr, struct thread *thread)
 {
-	if (thread == NULL)
+	if (!sort__has_thread || thread == NULL)
 		return 0;
 
 	if (asprintf(optstr, "Zoom %s %s(%d) thread",
@@ -2307,10 +2307,12 @@ skip_annotation:
 					     socked_id);
 		/* perf script support */
 		if (browser->he_selection) {
-			nr_options += add_script_opt(browser,
-						     &actions[nr_options],
-						     &options[nr_options],
-						     thread, NULL);
+			if (sort__has_thread && thread) {
+				nr_options += add_script_opt(browser,
+							     &actions[nr_options],
+							     &options[nr_options],
+							     thread, NULL);
+			}
 			/*
 			 * Note that browser->selection != NULL
 			 * when browser->he_selection is not NULL,

[tip:perf/core] perf hists browser: Only offer symbol scripting when a symbol is under the cursor

[tip-bot for Namhyung Kim]

Commit-ID:  c221acb0f970d3b80d72c812cda19c121acf5d52
Gitweb:     http://git.kernel.org/tip/c221acb0f970d3b80d72c812cda19c121acf5d52
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Thu, 21 Jan 2016 19:50:09 -0300
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 26 Jan 2016 11:52:49 -0300

perf hists browser: Only offer symbol scripting when a symbol is under the cursor

When this feature was introduced a check was made if there was a
resolved symbol under the cursor, it got lost in commit ea7cd5923309
("perf hists browser: Split popup menu actions - part 2"), reinstate it.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@kernel.org>,
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Wang Nan <wangnan0@huawei.com>
Fixes: ea7cd5923309 ("perf hists browser: Split popup menu actions - part 2")
Link: http://lkml.kernel.org/r/1452960197-5323-9-git-send-email-namhyung@kernel.org
[ Carved out from a  larger patch ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/ui/browsers/hists.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index e66b3a3..2801d80 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -2322,10 +2322,12 @@ skip_annotation:
 			 *
 			 * See hist_browser__show_entry.
 			 */
-			nr_options += add_script_opt(browser,
-						     &actions[nr_options],
-						     &options[nr_options],
-						     NULL, browser->selection->sym);
+			if (sort__has_sym && browser->selection->sym) {
+				nr_options += add_script_opt(browser,
+							     &actions[nr_options],
+							     &options[nr_options],
+							     NULL, browser->selection->sym);
+			}
 		}
 		nr_options += add_script_opt(browser, &actions[nr_options],
 					     &options[nr_options], NULL, NULL);

[tip:perf/core] perf hists browser: Offer 'Zoom into DSO'/' Map details' only when sort order has 'dso'

[tip-bot for Namhyung Kim]

Commit-ID:  b1447a54f5b41eaf1cc469d9bd3834caa2ff9afb
Gitweb:     http://git.kernel.org/tip/b1447a54f5b41eaf1cc469d9bd3834caa2ff9afb
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Fri, 22 Jan 2016 11:22:41 -0300
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 26 Jan 2016 11:52:50 -0300

perf hists browser: Offer 'Zoom into DSO'/'Map details' only when sort order has 'dso'

We can't offer a zoom into DSO when a bucket (struct hist_entry) may
have samples for more than one DSO, i.e. when 'dso' is not part of
the sort order, ditto for 'Map details', fix it.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@kernel.org>,
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Wang Nan <wangnan0@huawei.com>
Link: http://lkml.kernel.org/r/1452960197-5323-9-git-send-email-namhyung@kernel.org
[ Carved out from a  larger patch, moved check to add_{dso,map}_opt() ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/ui/browsers/hists.c | 4 ++--
 tools/perf/util/sort.h         | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 2801d80..e892106 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1825,7 +1825,7 @@ static int
 add_dso_opt(struct hist_browser *browser, struct popup_action *act,
 	    char **optstr, struct map *map)
 {
-	if (map == NULL)
+	if (!sort__has_dso || map == NULL)
 		return 0;
 
 	if (asprintf(optstr, "Zoom %s %s DSO",
@@ -1850,7 +1850,7 @@ static int
 add_map_opt(struct hist_browser *browser __maybe_unused,
 	    struct popup_action *act, char **optstr, struct map *map)
 {
-	if (map == NULL)
+	if (!sort__has_dso || map == NULL)
 		return 0;
 
 	if (asprintf(optstr, "Browse map details") < 0)
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index 09616f0..89a1273 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -32,6 +32,7 @@ extern const char default_sort_order[];
 extern regex_t ignore_callees_regex;
 extern int have_ignore_callees;
 extern int sort__need_collapse;
+extern int sort__has_dso;
 extern int sort__has_parent;
 extern int sort__has_sym;
 extern int sort__has_socket;

[tip:perf/core] perf hists browser: Be a bit more strict about presenting CPU socket zoom

[tip-bot for Namhyung Kim]

Commit-ID:  d9695d9f93649ecc00877ec2c847739c54a4cbb3
Gitweb:     http://git.kernel.org/tip/d9695d9f93649ecc00877ec2c847739c54a4cbb3
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Fri, 22 Jan 2016 12:20:18 -0300
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 26 Jan 2016 11:52:50 -0300

perf hists browser: Be a bit more strict about presenting CPU socket zoom

For consistency with the other sort order checks.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@kernel.org>,
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Wang Nan <wangnan0@huawei.com>
Link: http://lkml.kernel.org/r/1452960197-5323-9-git-send-email-namhyung@kernel.org
[ Carved out from a  larger patch, moved check to add_socket_opt() ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/ui/browsers/hists.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index e892106..b919582 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1971,7 +1971,7 @@ static int
 add_socket_opt(struct hist_browser *browser, struct popup_action *act,
 	       char **optstr, int socket_id)
 {
-	if (socket_id < 0)
+	if (!sort__has_socket || socket_id < 0)
 		return 0;
 
 	if (asprintf(optstr, "Zoom %s Processor Socket %d",

[tip:perf/core] perf hists browser: Offer non-symbol specific menu options for --sort without 'sym'

[tip-bot for Namhyung Kim]

Commit-ID:  4056132e1072f02bbad77f2071770271cc5b58fc
Gitweb:     http://git.kernel.org/tip/4056132e1072f02bbad77f2071770271cc5b58fc
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Fri, 22 Jan 2016 12:26:06 -0300
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 26 Jan 2016 11:52:51 -0300

perf hists browser: Offer non-symbol specific menu options for --sort without 'sym'

Now that we check more strictly what each of the menu entries need, we
can stop bailing out when 'sym' is not in the --sort order, instead we
let each option be added if what it needs is present.

This way, for instance, we can run scripts on all samples, see DSO map
details when 'dso' is in the --sort provided, etc.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@kernel.org>,
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Wang Nan <wangnan0@huawei.com>
Link: http://lkml.kernel.org/r/1452960197-5323-9-git-send-email-namhyung@kernel.org
[ Carved out from a  larger patch ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/ui/browsers/hists.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index b919582..d07e6be 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -2263,10 +2263,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 			continue;
 		}
 
-		if (!sort__has_sym)
-			goto add_exit_option;
-
-		if (browser->selection == NULL)
+		if (!sort__has_sym || browser->selection == NULL)
 			goto skip_annotation;
 
 		if (sort__mode == SORT_MODE__BRANCH) {
@@ -2333,7 +2330,6 @@ skip_annotation:
 					     &options[nr_options], NULL, NULL);
 		nr_options += add_switch_opt(browser, &actions[nr_options],
 					     &options[nr_options]);
-add_exit_option:
 		nr_options += add_exit_opt(browser, &actions[nr_options],
 					   &options[nr_options]);
 

[PATCH 09/17] perf hists browser: Count number of hierarchy entries

[Namhyung Kim]

Add nr_hierarchy_entries field to keep current number of (unfolded) hist
entries.  And the hist_entry->nr_rows carries number of direct children.
But in the hierarchy mode, entry can have grand children and callchains.
So update the number properly using hierarchy_count_rows() when toggling
the folded state (by pressing ENTER key).

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

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index cd6349ebd0d6..ae9e6407ea3c 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -32,6 +32,7 @@ struct hist_browser {
 	bool		     show_headers;
 	float		     min_pcnt;
 	u64		     nr_non_filtered_entries;
+	u64		     nr_hierarchy_entries;
 	u64		     nr_callchain_rows;
 };
 
@@ -58,11 +59,11 @@ static int hist_browser__get_folding(struct hist_browser *browser)
 
 	for (nd = rb_first(&hists->entries);
 	     (nd = hists__filter_entries(nd, browser->min_pcnt)) != NULL;
-	     nd = rb_next(nd)) {
+	     nd = rb_hierarchy_next(nd)) {
 		struct hist_entry *he =
 			rb_entry(nd, struct hist_entry, rb_node);
 
-		if (he->unfolded)
+		if (he->leaf && he->unfolded)
 			unfolded_rows += he->nr_rows;
 	}
 	return unfolded_rows;
@@ -72,7 +73,9 @@ static u32 hist_browser__nr_entries(struct hist_browser *hb)
 {
 	u32 nr_entries;
 
-	if (hist_browser__has_filter(hb))
+	if (symbol_conf.report_hierarchy)
+		nr_entries = hb->nr_hierarchy_entries;
+	else if (hist_browser__has_filter(hb))
 		nr_entries = hb->nr_non_filtered_entries;
 	else
 		nr_entries = hb->hists->nr_entries;
@@ -215,7 +218,7 @@ static int callchain_node__count_folded_rows(struct callchain_node *node __maybe
 static int callchain_node__count_rows(struct callchain_node *node)
 {
 	struct callchain_list *chain;
-	bool unfolded = false;
+	char folded_sign = ' ';
 	int n = 0;
 
 	if (callchain_param.mode == CHAIN_FLAT)
@@ -225,10 +228,13 @@ static int callchain_node__count_rows(struct callchain_node *node)
 
 	list_for_each_entry(chain, &node->val, list) {
 		++n;
-		unfolded = chain->unfolded;
+
+		folded_sign = callchain_list__folded(chain);
+		if (folded_sign == '+')
+			break;
 	}
 
-	if (unfolded)
+	if (folded_sign == '-')
 		n += callchain_node__count_rows_rb_tree(node);
 
 	return n;
@@ -247,6 +253,35 @@ static int callchain__count_rows(struct rb_root *chain)
 	return n;
 }
 
+static int hierarchy_count_rows(struct hist_browser *hb, struct hist_entry *he,
+				bool include_children)
+{
+	int count = 0;
+	struct rb_node *node;
+	struct hist_entry *child;
+
+	if (he->leaf)
+		return callchain__count_rows(&he->sorted_chain);
+
+	node = rb_first(&he->hroot_out);
+	while (node) {
+		float percent;
+
+		child = rb_entry(node, struct hist_entry, rb_node);
+		percent = hist_entry__get_percent_limit(child);
+
+		if (!child->filtered && percent >= hb->min_pcnt) {
+			count++;
+
+			if (include_children && child->unfolded)
+				count += hierarchy_count_rows(hb, child, true);
+		}
+
+		node = rb_next(node);
+	}
+	return count;
+}
+
 static bool hist_entry__toggle_fold(struct hist_entry *he)
 {
 	if (!he)
@@ -326,11 +361,17 @@ static void callchain__init_have_children(struct rb_root *root)
 
 static void hist_entry__init_have_children(struct hist_entry *he)
 {
-	if (!he->init_have_children) {
+	if (he->init_have_children)
+		return;
+
+	if (he->leaf) {
 		he->has_children = !RB_EMPTY_ROOT(&he->sorted_chain);
 		callchain__init_have_children(&he->sorted_chain);
-		he->init_have_children = true;
+	} else {
+		he->has_children = !RB_EMPTY_ROOT(&he->hroot_out);
 	}
+
+	he->init_have_children = true;
 }
 
 static bool hist_browser__toggle_fold(struct hist_browser *browser)
@@ -349,17 +390,41 @@ static bool hist_browser__toggle_fold(struct hist_browser *browser)
 		has_children = callchain_list__toggle_fold(cl);
 
 	if (has_children) {
+		int child_rows = 0;
+
 		hist_entry__init_have_children(he);
 		browser->b.nr_entries -= he->nr_rows;
-		browser->nr_callchain_rows -= he->nr_rows;
 
-		if (he->unfolded)
-			he->nr_rows = callchain__count_rows(&he->sorted_chain);
+		if (he->leaf)
+			browser->nr_callchain_rows -= he->nr_rows;
 		else
+			browser->nr_hierarchy_entries -= he->nr_rows;
+
+		if (symbol_conf.report_hierarchy)
+			child_rows = hierarchy_count_rows(browser, he, true);
+
+		if (he->unfolded) {
+			if (he->leaf)
+				he->nr_rows = callchain__count_rows(&he->sorted_chain);
+			else
+				he->nr_rows = hierarchy_count_rows(browser, he, false);
+
+			/* account grand children */
+			if (symbol_conf.report_hierarchy)
+				browser->b.nr_entries += child_rows - he->nr_rows;
+		} else {
+			if (symbol_conf.report_hierarchy)
+				browser->b.nr_entries -= child_rows - he->nr_rows;
+
 			he->nr_rows = 0;
+		}
 
 		browser->b.nr_entries += he->nr_rows;
-		browser->nr_callchain_rows += he->nr_rows;
+
+		if (he->leaf)
+			browser->nr_callchain_rows += he->nr_rows;
+		else
+			browser->nr_hierarchy_entries += he->nr_rows;
 
 		return true;
 	}
@@ -1989,17 +2054,18 @@ static void hist_browser__update_nr_entries(struct hist_browser *hb)
 	u64 nr_entries = 0;
 	struct rb_node *nd = rb_first(&hb->hists->entries);
 
-	if (hb->min_pcnt == 0) {
+	if (hb->min_pcnt == 0 && !symbol_conf.report_hierarchy) {
 		hb->nr_non_filtered_entries = hb->hists->nr_non_filtered_entries;
 		return;
 	}
 
 	while ((nd = hists__filter_entries(nd, hb->min_pcnt)) != NULL) {
 		nr_entries++;
-		nd = rb_next(nd);
+		nd = rb_hierarchy_next(nd);
 	}
 
 	hb->nr_non_filtered_entries = nr_entries;
+	hb->nr_hierarchy_entries = nr_entries;
 }
 
 static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
-- 
2.6.4

[PATCH 10/17] perf hists browser: Support collapsing/expanding whole entries in hierarchy

[Namhyung Kim]

The 'C' and 'E' keys are to collapse/expand all hist entries.  Update
nr_hierarchy_entries properly in this case.

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

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index ae9e6407ea3c..8452528fa29b 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -487,13 +487,37 @@ static int callchain__set_folding(struct rb_root *chain, bool unfold)
 	return n;
 }
 
-static void hist_entry__set_folding(struct hist_entry *he, bool unfold)
+static int hierarchy_set_folding(struct hist_browser *hb, struct hist_entry *he,
+				 bool unfold __maybe_unused)
+{
+	float percent;
+	struct rb_node *nd;
+	struct hist_entry *child;
+	int n = 0;
+
+	for (nd = rb_first(&he->hroot_out); nd; nd = rb_next(nd)) {
+		child = rb_entry(nd, struct hist_entry, rb_node);
+		percent = hist_entry__get_percent_limit(child);
+		if (!child->filtered && percent >= hb->min_pcnt)
+			n++;
+	}
+
+	return n;
+}
+
+static void hist_entry__set_folding(struct hist_entry *he, struct hist_browser *hb,
+				    bool unfold)
 {
-	hist_entry__init_have_children(he);
 	he->unfolded = unfold ? he->has_children : false;
 
 	if (he->has_children) {
-		int n = callchain__set_folding(&he->sorted_chain, unfold);
+		int n;
+
+		if (he->leaf)
+			n = callchain__set_folding(&he->sorted_chain, unfold);
+		else
+			n = hierarchy_set_folding(hb, he, unfold);
+
 		he->nr_rows = unfold ? n : 0;
 	} else
 		he->nr_rows = 0;
@@ -505,17 +529,36 @@ __hist_browser__set_folding(struct hist_browser *browser, bool unfold)
 	struct rb_node *nd;
 	struct hists *hists = browser->hists;
 
-	for (nd = rb_first(&hists->entries);
-	     (nd = hists__filter_entries(nd, browser->min_pcnt)) != NULL;
-	     nd = rb_next(nd)) {
+	nd = rb_first(&hists->entries);
+	while (nd) {
 		struct hist_entry *he = rb_entry(nd, struct hist_entry, rb_node);
-		hist_entry__set_folding(he, unfold);
-		browser->nr_callchain_rows += he->nr_rows;
+		float percent;
+
+		hist_entry__init_have_children(he);
+
+		/*
+		 * Tentatively set unfolded so that the rb_hierarchy_next()
+		 * can toggle children of folded entries too.
+		 */
+		he->unfolded = he->has_children;
+		nd = rb_hierarchy_next(nd);
+
+		hist_entry__set_folding(he, browser, unfold);
+
+		percent = hist_entry__get_percent_limit(he);
+		if (he->filtered || percent < browser->min_pcnt)
+			continue;
+
+		if (!he->depth || unfold)
+			browser->nr_hierarchy_entries++;
+		if (he->leaf)
+			browser->nr_callchain_rows += he->nr_rows;
 	}
 }
 
 static void hist_browser__set_folding(struct hist_browser *browser, bool unfold)
 {
+	browser->nr_hierarchy_entries = 0;
 	browser->nr_callchain_rows = 0;
 	__hist_browser__set_folding(browser, unfold);
 
-- 
2.6.4

[PATCH 11/17] perf hists browser: Factor out hist_browser__show_callchain()

[Namhyung Kim]

Factor out callchain printing code so that it can be called by other
functions easily.  It renames the existing function which prints
graph-style callchain.  This also fixes hist_browser__dump() which
missed to be updated by commit 8c430a348699 ("perf hists browser:
Support folded callchains").

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

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 8452528fa29b..353e0477c1f5 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -952,7 +952,7 @@ next:
 	return row - first_row;
 }
 
-static int hist_browser__show_callchain(struct hist_browser *browser,
+static int hist_browser__show_callchain_graph(struct hist_browser *browser,
 					struct rb_root *root, int level,
 					unsigned short row, u64 total,
 					print_callchain_entry_fn print,
@@ -1006,7 +1006,7 @@ static int hist_browser__show_callchain(struct hist_browser *browser,
 			else
 				new_total = total;
 
-			row += hist_browser__show_callchain(browser, &child->rb_root,
+			row += hist_browser__show_callchain_graph(browser, &child->rb_root,
 							    new_level, row, new_total,
 							    print, arg, is_output_full);
 		}
@@ -1018,6 +1018,43 @@ out:
 	return row - first_row;
 }
 
+static int hist_browser__show_callchain(struct hist_browser *browser,
+					struct hist_entry *entry, int level,
+					unsigned short row,
+					print_callchain_entry_fn print,
+					struct callchain_print_arg *arg,
+					check_output_full_fn is_output_full)
+{
+	u64 total = hists__total_period(entry->hists);
+	int printed;
+
+	if (callchain_param.mode == CHAIN_GRAPH_REL) {
+		if (symbol_conf.cumulate_callchain)
+			total = entry->stat_acc->period;
+		else
+			total = entry->stat.period;
+	}
+
+	if (callchain_param.mode == CHAIN_FLAT) {
+		printed = hist_browser__show_callchain_flat(browser,
+						&entry->sorted_chain, row, total,
+						print, arg, is_output_full);
+	} else if (callchain_param.mode == CHAIN_FOLDED) {
+		printed = hist_browser__show_callchain_folded(browser,
+						&entry->sorted_chain, row, total,
+						print, arg, is_output_full);
+	} else {
+		printed = hist_browser__show_callchain_graph(browser,
+						&entry->sorted_chain, level, row, total,
+						print, arg, is_output_full);
+	}
+
+	if (arg->is_current_entry)
+		browser->he_selection = entry;
+
+	return printed;
+}
+
 struct hpp_arg {
 	struct ui_browser *b;
 	char folded_sign;
@@ -1192,38 +1229,14 @@ static int hist_browser__show_entry(struct hist_browser *browser,
 		--row_offset;
 
 	if (folded_sign == '-' && row != browser->b.rows) {
-		u64 total = hists__total_period(entry->hists);
 		struct callchain_print_arg arg = {
 			.row_offset = row_offset,
 			.is_current_entry = current_entry,
 		};
 
-		if (callchain_param.mode == CHAIN_GRAPH_REL) {
-			if (symbol_conf.cumulate_callchain)
-				total = entry->stat_acc->period;
-			else
-				total = entry->stat.period;
-		}
-
-		if (callchain_param.mode == CHAIN_FLAT) {
-			printed += hist_browser__show_callchain_flat(browser,
-					&entry->sorted_chain, row, total,
+		printed += hist_browser__show_callchain(browser, entry, 1, row,
 					hist_browser__show_callchain_entry, &arg,
 					hist_browser__check_output_full);
-		} else if (callchain_param.mode == CHAIN_FOLDED) {
-			printed += hist_browser__show_callchain_folded(browser,
-					&entry->sorted_chain, row, total,
-					hist_browser__show_callchain_entry, &arg,
-					hist_browser__check_output_full);
-		} else {
-			printed += hist_browser__show_callchain(browser,
-					&entry->sorted_chain, 1, row, total,
-					hist_browser__show_callchain_entry, &arg,
-					hist_browser__check_output_full);
-		}
-
-		if (arg.is_current_entry)
-			browser->he_selection = entry;
 	}
 
 	return printed;
@@ -1488,15 +1501,11 @@ do_offset:
 static int hist_browser__fprintf_callchain(struct hist_browser *browser,
 					   struct hist_entry *he, FILE *fp)
 {
-	u64 total = hists__total_period(he->hists);
 	struct callchain_print_arg arg  = {
 		.fp = fp,
 	};
 
-	if (symbol_conf.cumulate_callchain)
-		total = he->stat_acc->period;
-
-	hist_browser__show_callchain(browser, &he->sorted_chain, 1, 0, total,
+	hist_browser__show_callchain(browser, he, 1, 0,
 				     hist_browser__fprintf_callchain_entry, &arg,
 				     hist_browser__check_dump_full);
 	return arg.printed;
-- 
2.6.4

[PATCH 12/17] perf hists browser: Implement hierarchy output

[Namhyung Kim]

Implement hierarchy mode in TUI.  The output is look like stdio but it
also supports to fold/unfold children dynamically.

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

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 353e0477c1f5..ded026e213cc 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1242,6 +1242,137 @@ static int hist_browser__show_entry(struct hist_browser *browser,
 	return printed;
 }
 
+static int hist_browser__show_hierarchy_entry(struct hist_browser *browser,
+					      struct hist_entry *entry,
+					      unsigned short row,
+					      int level, int nr_sort_keys)
+{
+	char s[256];
+	int printed = 0;
+	int width = browser->b.width;
+	char folded_sign = ' ';
+	bool current_entry = ui_browser__is_current_entry(&browser->b, row);
+	off_t row_offset = entry->row_offset;
+	bool first = true;
+	struct perf_hpp_fmt *fmt;
+	struct hpp_arg arg = {
+		.b		= &browser->b,
+		.current_entry	= current_entry,
+	};
+	struct perf_hpp hpp = {
+		.buf		= s,
+		.size		= sizeof(s),
+		.ptr		= &arg,
+	};
+	int column = 0;
+	int hierarchy_indent = (nr_sort_keys - 1) * HIERARCHY_INDENT;
+
+	if (current_entry) {
+		browser->he_selection = entry;
+		browser->selection = &entry->ms;
+	}
+
+	hist_entry__init_have_children(entry);
+	folded_sign = hist_entry__folded(entry);
+	arg.folded_sign = folded_sign;
+
+	if (entry->leaf && row_offset) {
+		row_offset--;
+		goto show_callchain;
+	}
+
+	hist_browser__gotorc(browser, row, 0);
+
+	if (current_entry && browser->b.navkeypressed)
+		ui_browser__set_color(&browser->b, HE_COLORSET_SELECTED);
+	else
+		ui_browser__set_color(&browser->b, HE_COLORSET_NORMAL);
+
+	ui_browser__write_nstring(&browser->b, "", level * HIERARCHY_INDENT);
+	width -= level * HIERARCHY_INDENT;
+
+	perf_hpp__for_each_format(fmt) {
+		if (perf_hpp__should_skip(fmt, entry->hists) ||
+		    column++ < browser->b.horiz_scroll)
+			continue;
+
+		if (perf_hpp__is_sort_entry(fmt) ||
+		    perf_hpp__is_dynamic_entry(fmt))
+			break;
+
+		if (current_entry && browser->b.navkeypressed) {
+			ui_browser__set_color(&browser->b,
+					      HE_COLORSET_SELECTED);
+		} else {
+			ui_browser__set_color(&browser->b,
+					      HE_COLORSET_NORMAL);
+		}
+
+		if (first) {
+			ui_browser__printf(&browser->b, "%c", folded_sign);
+			width--;
+			first = false;
+		} else {
+			ui_browser__printf(&browser->b, "  ");
+			width -= 2;
+		}
+
+		if (fmt->color) {
+			width -= fmt->color(fmt, &hpp, entry);
+		} else {
+			width -= fmt->entry(fmt, &hpp, entry);
+			ui_browser__printf(&browser->b, "%s", s);
+		}
+	}
+
+	ui_browser__write_nstring(&browser->b, "", hierarchy_indent);
+	width -= hierarchy_indent;
+
+	if (column >= browser->b.horiz_scroll) {
+		if (current_entry && browser->b.navkeypressed) {
+			ui_browser__set_color(&browser->b,
+					      HE_COLORSET_SELECTED);
+		} else {
+			ui_browser__set_color(&browser->b,
+					      HE_COLORSET_NORMAL);
+		}
+
+		ui_browser__write_nstring(&browser->b, "", 2);
+		width -= 2;
+
+		fmt = entry->fmt;
+		if (fmt->color) {
+			width -= fmt->color(fmt, &hpp, entry);
+		} else {
+			width -= fmt->entry(fmt, &hpp, entry);
+			ui_browser__printf(&browser->b, "%s", s);
+		}
+	}
+
+	/* The scroll bar isn't being used */
+	if (!browser->b.navkeypressed)
+		width += 1;
+
+	ui_browser__write_nstring(&browser->b, "", width);
+
+	++row;
+	++printed;
+
+show_callchain:
+	if (entry->leaf && folded_sign == '-' && row != browser->b.rows) {
+		struct callchain_print_arg carg = {
+			.row_offset = row_offset,
+		};
+
+		printed += hist_browser__show_callchain(browser, entry,
+					level + 1, row,
+					hist_browser__show_callchain_entry, &carg,
+					hist_browser__check_output_full);
+	}
+
+	return printed;
+}
+
 static int advance_hpp_check(struct perf_hpp *hpp, int inc)
 {
 	advance_hpp(hpp, inc);
@@ -1307,6 +1438,7 @@ static unsigned int hist_browser__refresh(struct ui_browser *browser)
 	u16 header_offset = 0;
 	struct rb_node *nd;
 	struct hist_browser *hb = container_of(browser, struct hist_browser, b);
+	int nr_sort = perf_hpp__count_sort_keys();
 
 	if (hb->show_headers) {
 		hist_browser__show_headers(hb);
@@ -1317,18 +1449,28 @@ static unsigned int hist_browser__refresh(struct ui_browser *browser)
 	hb->he_selection = NULL;
 	hb->selection = NULL;
 
-	for (nd = browser->top; nd; nd = rb_next(nd)) {
+	for (nd = browser->top; nd; nd = rb_hierarchy_next(nd)) {
 		struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
 		float percent;
 
-		if (h->filtered)
+		if (h->filtered) {
+			/* let it move to sibling */
+			h->unfolded = false;
 			continue;
+		}
 
 		percent = hist_entry__get_percent_limit(h);
 		if (percent < hb->min_pcnt)
 			continue;
 
-		row += hist_browser__show_entry(hb, h, row);
+		if (symbol_conf.report_hierarchy) {
+			row += hist_browser__show_hierarchy_entry(hb, h, row,
+								  h->depth,
+								  nr_sort);
+		} else {
+			row += hist_browser__show_entry(hb, h, row);
+		}
+
 		if (row == browser->rows)
 			break;
 	}
@@ -1346,7 +1488,14 @@ static struct rb_node *hists__filter_entries(struct rb_node *nd,
 		if (!h->filtered && percent >= min_pcnt)
 			return nd;
 
-		nd = rb_next(nd);
+		/*
+		 * If it's filtered, its all children also were filtered.
+		 * So move to sibling node.
+		 */
+		if (rb_next(nd))
+			nd = rb_next(nd);
+		else
+			nd = rb_hierarchy_next(nd);
 	}
 
 	return NULL;
@@ -1362,7 +1511,7 @@ static struct rb_node *hists__filter_prev_entries(struct rb_node *nd,
 		if (!h->filtered && percent >= min_pcnt)
 			return nd;
 
-		nd = rb_prev(nd);
+		nd = rb_hierarchy_prev(nd);
 	}
 
 	return NULL;
@@ -1392,8 +1541,8 @@ static void ui_browser__hists_seek(struct ui_browser *browser,
 		nd = browser->top;
 		goto do_offset;
 	case SEEK_END:
-		nd = hists__filter_prev_entries(rb_last(browser->entries),
-						hb->min_pcnt);
+		nd = rb_hierarchy_last(rb_last(browser->entries));
+		nd = hists__filter_prev_entries(nd, hb->min_pcnt);
 		first = false;
 		break;
 	default:
@@ -1427,7 +1576,7 @@ do_offset:
 	if (offset > 0) {
 		do {
 			h = rb_entry(nd, struct hist_entry, rb_node);
-			if (h->unfolded) {
+			if (h->unfolded && h->leaf) {
 				u16 remaining = h->nr_rows - h->row_offset;
 				if (offset > remaining) {
 					offset -= remaining;
@@ -1439,7 +1588,8 @@ do_offset:
 					break;
 				}
 			}
-			nd = hists__filter_entries(rb_next(nd), hb->min_pcnt);
+			nd = hists__filter_entries(rb_hierarchy_next(nd),
+						   hb->min_pcnt);
 			if (nd == NULL)
 				break;
 			--offset;
@@ -1448,7 +1598,7 @@ do_offset:
 	} else if (offset < 0) {
 		while (1) {
 			h = rb_entry(nd, struct hist_entry, rb_node);
-			if (h->unfolded) {
+			if (h->unfolded && h->leaf) {
 				if (first) {
 					if (-offset > h->row_offset) {
 						offset += h->row_offset;
@@ -1472,7 +1622,7 @@ do_offset:
 				}
 			}
 
-			nd = hists__filter_prev_entries(rb_prev(nd),
+			nd = hists__filter_prev_entries(rb_hierarchy_prev(nd),
 							hb->min_pcnt);
 			if (nd == NULL)
 				break;
@@ -1485,7 +1635,7 @@ do_offset:
 				 * row_offset at its last entry.
 				 */
 				h = rb_entry(nd, struct hist_entry, rb_node);
-				if (h->unfolded)
+				if (h->unfolded && h->leaf)
 					h->row_offset = h->nr_rows;
 				break;
 			}
@@ -1499,13 +1649,14 @@ do_offset:
 }
 
 static int hist_browser__fprintf_callchain(struct hist_browser *browser,
-					   struct hist_entry *he, FILE *fp)
+					   struct hist_entry *he, FILE *fp,
+					   int level)
 {
 	struct callchain_print_arg arg  = {
 		.fp = fp,
 	};
 
-	hist_browser__show_callchain(browser, he, 1, 0,
+	hist_browser__show_callchain(browser, he, level, 0,
 				     hist_browser__fprintf_callchain_entry, &arg,
 				     hist_browser__check_dump_full);
 	return arg.printed;
@@ -1547,7 +1698,65 @@ static int hist_browser__fprintf_entry(struct hist_browser *browser,
 	printed += fprintf(fp, "%s\n", rtrim(s));
 
 	if (folded_sign == '-')
-		printed += hist_browser__fprintf_callchain(browser, he, fp);
+		printed += hist_browser__fprintf_callchain(browser, he, fp, 1);
+
+	return printed;
+}
+
+
+static int hist_browser__fprintf_hierarchy_entry(struct hist_browser *browser,
+						 struct hist_entry *he,
+						 FILE *fp, int level,
+						 int nr_sort_keys)
+{
+	char s[8192];
+	int printed = 0;
+	char folded_sign = ' ';
+	struct perf_hpp hpp = {
+		.buf = s,
+		.size = sizeof(s),
+	};
+	struct perf_hpp_fmt *fmt;
+	bool first = true;
+	int ret;
+	int hierarchy_indent = (nr_sort_keys + 1) * HIERARCHY_INDENT;
+
+	printed = fprintf(fp, "%*s", level * HIERARCHY_INDENT, "");
+
+	folded_sign = hist_entry__folded(he);
+	printed += fprintf(fp, "%c", folded_sign);
+
+	perf_hpp__for_each_format(fmt) {
+		if (perf_hpp__should_skip(fmt, he->hists))
+			continue;
+
+		if (perf_hpp__is_sort_entry(fmt) ||
+		    perf_hpp__is_dynamic_entry(fmt))
+			break;
+
+		if (!first) {
+			ret = scnprintf(hpp.buf, hpp.size, "  ");
+			advance_hpp(&hpp, ret);
+		} else
+			first = false;
+
+		ret = fmt->entry(fmt, &hpp, he);
+		advance_hpp(&hpp, ret);
+	}
+
+	ret = scnprintf(hpp.buf, hpp.size, "%*s", hierarchy_indent, "");
+	advance_hpp(&hpp, ret);
+
+	fmt = he->fmt;
+	ret = fmt->entry(fmt, &hpp, he);
+	advance_hpp(&hpp, ret);
+
+	printed += fprintf(fp, "%s\n", rtrim(s));
+
+	if (he->leaf && folded_sign == '-') {
+		printed += hist_browser__fprintf_callchain(browser, he, fp,
+							   he->depth + 1);
+	}
 
 	return printed;
 }
@@ -1557,12 +1766,22 @@ static int hist_browser__fprintf(struct hist_browser *browser, FILE *fp)
 	struct rb_node *nd = hists__filter_entries(rb_first(browser->b.entries),
 						   browser->min_pcnt);
 	int printed = 0;
+	int nr_sort = perf_hpp__count_sort_keys();
 
 	while (nd) {
 		struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
 
-		printed += hist_browser__fprintf_entry(browser, h, fp);
-		nd = hists__filter_entries(rb_next(nd), browser->min_pcnt);
+		if (symbol_conf.report_hierarchy) {
+			printed += hist_browser__fprintf_hierarchy_entry(browser,
+									 h, fp,
+									 h->depth,
+									 nr_sort);
+		} else {
+			printed += hist_browser__fprintf_entry(browser, h, fp);
+		}
+
+		nd = hists__filter_entries(rb_hierarchy_next(nd),
+					   browser->min_pcnt);
 	}
 
 	return printed;
-- 
2.6.4

[PATCH 13/17] perf hists browser: Align column header in hierarchy mode

[Namhyung Kim]

Like in stdio, fit column header to hierarchy output.  Merge column
headers with "/" as a separator.

   Overhead        Command / Shared Object / Symbol
  ...
  +   0.09%        dwm
  +   0.06%        emacs
  -   0.05%        perf
     -   0.05%        [kernel.vmlinux]
        +   0.03%        [k] memcpy_orig
        +   0.01%        [k] unmap_single_vma
        +   0.01%        [k] smp_call_function_single
        +   0.00%        [k] native_irq_return_iret
        +   0.00%        [k] arch_trigger_all_cpu_backtrace_handler
        +   0.00%        [k] native_write_msr_safe

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

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index ded026e213cc..11728249f70a 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1412,11 +1412,78 @@ static int hists_browser__scnprintf_headers(struct hist_browser *browser, char *
 	return ret;
 }
 
+static int hists_browser__scnprintf_hierarchy_headers(struct hist_browser *browser, char *buf, size_t size)
+{
+	struct hists *hists = browser->hists;
+	struct perf_hpp dummy_hpp = {
+		.buf    = buf,
+		.size   = size,
+	};
+	struct perf_hpp_fmt *fmt;
+	size_t ret = 0;
+	int column = 0;
+	int nr_sort_keys = perf_hpp__count_sort_keys();
+	bool first = true;
+
+	ret = scnprintf(buf, size, " ");
+	if (advance_hpp_check(&dummy_hpp, ret))
+		return ret;
+
+	perf_hpp__for_each_format(fmt) {
+		if (column++ < browser->b.horiz_scroll)
+			continue;
+
+		if (perf_hpp__is_sort_entry(fmt) || perf_hpp__is_dynamic_entry(fmt))
+			break;
+
+		ret = fmt->header(fmt, &dummy_hpp, hists_to_evsel(hists));
+		if (advance_hpp_check(&dummy_hpp, ret))
+			break;
+
+		ret = scnprintf(dummy_hpp.buf, dummy_hpp.size, "  ");
+		if (advance_hpp_check(&dummy_hpp, ret))
+			break;
+	}
+
+	ret = scnprintf(dummy_hpp.buf, dummy_hpp.size, "%*s",
+			(nr_sort_keys - 1) * HIERARCHY_INDENT, "");
+	if (advance_hpp_check(&dummy_hpp, ret))
+		return ret;
+
+	perf_hpp__for_each_format(fmt) {
+		if (!perf_hpp__is_sort_entry(fmt) && !perf_hpp__is_dynamic_entry(fmt))
+			continue;
+
+		if (first) {
+			first = false;
+		} else {
+			ret = scnprintf(dummy_hpp.buf, dummy_hpp.size, " / ");
+			if (advance_hpp_check(&dummy_hpp, ret))
+				break;
+		}
+
+		ret = fmt->header(fmt, &dummy_hpp, hists_to_evsel(hists));
+		dummy_hpp.buf[ret] = '\0';
+		rtrim(dummy_hpp.buf);
+
+		ret = strlen(dummy_hpp.buf);
+		if (advance_hpp_check(&dummy_hpp, ret))
+			break;
+	}
+
+	return ret;
+}
+
 static void hist_browser__show_headers(struct hist_browser *browser)
 {
 	char headers[1024];
 
-	hists_browser__scnprintf_headers(browser, headers, sizeof(headers));
+	if (symbol_conf.report_hierarchy)
+		hists_browser__scnprintf_hierarchy_headers(browser, headers,
+							   sizeof(headers));
+	else
+		hists_browser__scnprintf_headers(browser, headers,
+						 sizeof(headers));
 	ui_browser__gotorc(&browser->b, 0, 0);
 	ui_browser__set_color(&browser->b, HE_COLORSET_ROOT);
 	ui_browser__write_nstring(&browser->b, headers, browser->b.width + 1);
-- 
2.6.4

[PATCH 14/17] perf ui/gtk: Implement hierarchy output mode

[Namhyung Kim]

The hierarchy output mode is to group entries for each level so that
user can see higher level picture more easily.

Cc: Pekka Enberg <penberg@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/gtk/hists.c | 161 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 160 insertions(+), 1 deletion(-)

diff --git a/tools/perf/ui/gtk/hists.c b/tools/perf/ui/gtk/hists.c
index 0f8dcfdfb10f..f33df340fc45 100644
--- a/tools/perf/ui/gtk/hists.c
+++ b/tools/perf/ui/gtk/hists.c
@@ -396,6 +396,162 @@ static void perf_gtk__show_hists(GtkWidget *window, struct hists *hists,
 	gtk_container_add(GTK_CONTAINER(window), view);
 }
 
+static void perf_gtk__add_hierarchy_entries(struct hists *hists,
+					    struct rb_root *root,
+					    GtkTreeStore *store,
+					    GtkTreeIter *parent,
+					    struct perf_hpp *hpp,
+					    float min_pcnt)
+{
+	int col_idx = 0;
+	struct rb_node *node;
+	struct hist_entry *he;
+	struct perf_hpp_fmt *fmt;
+	u64 total = hists__total_period(hists);
+
+	for (node = rb_first(root); node; node = rb_next(node)) {
+		GtkTreeIter iter;
+		float percent;
+
+		he = rb_entry(node, struct hist_entry, rb_node);
+		if (he->filtered)
+			continue;
+
+		percent = hist_entry__get_percent_limit(he);
+		if (percent < min_pcnt)
+			continue;
+
+		gtk_tree_store_append(store, &iter, parent);
+
+		col_idx = 0;
+		perf_hpp__for_each_format(fmt) {
+			if (perf_hpp__is_sort_entry(fmt) ||
+			    perf_hpp__is_dynamic_entry(fmt))
+				break;
+
+			if (fmt->color)
+				fmt->color(fmt, hpp, he);
+			else
+				fmt->entry(fmt, hpp, he);
+
+			gtk_tree_store_set(store, &iter, col_idx++, hpp->buf, -1);
+		}
+
+		fmt = he->fmt;
+		if (fmt->color)
+			fmt->color(fmt, hpp, he);
+		else
+			fmt->entry(fmt, hpp, he);
+
+		gtk_tree_store_set(store, &iter, col_idx, rtrim(hpp->buf), -1);
+
+		if (!he->leaf) {
+			perf_gtk__add_hierarchy_entries(hists, &he->hroot_out,
+							store, &iter, hpp,
+							min_pcnt);
+		}
+
+		if (symbol_conf.use_callchain && he->leaf) {
+			if (callchain_param.mode == CHAIN_GRAPH_REL)
+				total = symbol_conf.cumulate_callchain ?
+					he->stat_acc->period : he->stat.period;
+
+			perf_gtk__add_callchain(&he->sorted_chain, store, &iter,
+						col_idx, total);
+		}
+	}
+
+}
+
+static void perf_gtk__show_hierarchy(GtkWidget *window, struct hists *hists,
+				     float min_pcnt)
+{
+	struct perf_hpp_fmt *fmt;
+	GType col_types[MAX_COLUMNS];
+	GtkCellRenderer *renderer;
+	GtkTreeStore *store;
+	GtkWidget *view;
+	int col_idx;
+	int nr_cols = 0;
+	char s[512];
+	char buf[512];
+	bool first = true;
+	struct perf_hpp hpp = {
+		.buf		= s,
+		.size		= sizeof(s),
+	};
+
+	perf_hpp__for_each_format(fmt) {
+		if (perf_hpp__is_sort_entry(fmt) ||
+		    perf_hpp__is_dynamic_entry(fmt))
+			break;
+
+		col_types[nr_cols++] = G_TYPE_STRING;
+	}
+	col_types[nr_cols++] = G_TYPE_STRING;
+
+	store = gtk_tree_store_newv(nr_cols, col_types);
+	view = gtk_tree_view_new();
+	renderer = gtk_cell_renderer_text_new();
+
+	col_idx = 0;
+	perf_hpp__for_each_format(fmt) {
+		if (perf_hpp__is_sort_entry(fmt) ||
+		    perf_hpp__is_dynamic_entry(fmt))
+			break;
+
+		gtk_tree_view_insert_column_with_attributes(GTK_TREE_VIEW(view),
+							    -1, fmt->name,
+							    renderer, "markup",
+							    col_idx++, NULL);
+	}
+
+	/* construct merged column header since sort keys share single column */
+	buf[0] = '\0';
+	perf_hpp__for_each_format(fmt) {
+		if (!perf_hpp__is_sort_entry(fmt) &&
+		    !perf_hpp__is_dynamic_entry(fmt))
+			continue;
+
+		if (first)
+			first = false;
+		else
+			strcat(buf, " / ");
+
+		fmt->header(fmt, &hpp, hists_to_evsel(hists));
+		strcat(buf, rtrim(hpp.buf));
+	}
+
+	gtk_tree_view_insert_column_with_attributes(GTK_TREE_VIEW(view),
+						    -1, buf,
+						    renderer, "markup",
+						    col_idx++, NULL);
+
+	for (col_idx = 0; col_idx < nr_cols; col_idx++) {
+		GtkTreeViewColumn *column;
+
+		column = gtk_tree_view_get_column(GTK_TREE_VIEW(view), col_idx);
+		gtk_tree_view_column_set_resizable(column, TRUE);
+
+		if (col_idx == 0) {
+			gtk_tree_view_set_expander_column(GTK_TREE_VIEW(view),
+							  column);
+		}
+	}
+
+	gtk_tree_view_set_model(GTK_TREE_VIEW(view), GTK_TREE_MODEL(store));
+	g_object_unref(GTK_TREE_MODEL(store));
+
+	perf_gtk__add_hierarchy_entries(hists, &hists->entries, store,
+					NULL, &hpp, min_pcnt);
+
+	gtk_tree_view_set_rules_hint(GTK_TREE_VIEW(view), TRUE);
+
+	g_signal_connect(view, "row-activated",
+			 G_CALLBACK(on_row_activated), NULL);
+	gtk_container_add(GTK_CONTAINER(window), view);
+}
+
 int perf_evlist__gtk_browse_hists(struct perf_evlist *evlist,
 				  const char *help,
 				  struct hist_browser_timer *hbt __maybe_unused,
@@ -463,7 +619,10 @@ int perf_evlist__gtk_browse_hists(struct perf_evlist *evlist,
 							GTK_POLICY_AUTOMATIC,
 							GTK_POLICY_AUTOMATIC);
 
-		perf_gtk__show_hists(scrolled_window, hists, min_pcnt);
+		if (symbol_conf.report_hierarchy)
+			perf_gtk__show_hierarchy(scrolled_window, hists, min_pcnt);
+		else
+			perf_gtk__show_hists(scrolled_window, hists, min_pcnt);
 
 		tab_label = gtk_label_new(evname);
 
-- 
2.6.4

[PATCH 15/17] perf report: Add --hierarchy option

[Namhyung Kim]

The --hierarchy option is to show output in hierarchy mode.  It extends
folding/unfolding in the TUI and GTK browsers to support sort items as
well as callchains.  Users can toggle the items to see the performance
result at wanted level.

  $ perf report --hierarchy --tui
   Overhead        Command / Shared Object / Symbol
  +  32.96%        gnome-shell
  -  15.11%        swapper
     -  14.97%        [kernel.vmlinux]
            6.82%        [k] intel_idle
	    0.66%        [k] menu_select
	    0.43%        [k] __hrtimer_start_range_ns
  ...

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/Documentation/perf-report.txt |  3 +++
 tools/perf/Documentation/tips.txt        |  1 +
 tools/perf/builtin-report.c              | 17 +++++++++++++++++
 3 files changed, 21 insertions(+)

diff --git a/tools/perf/Documentation/perf-report.txt b/tools/perf/Documentation/perf-report.txt
index 8a301f6afb37..d2b4d6e15622 100644
--- a/tools/perf/Documentation/perf-report.txt
+++ b/tools/perf/Documentation/perf-report.txt
@@ -398,6 +398,9 @@ include::itrace.txt[]
 --raw-trace::
 	When displaying traceevent output, do not use print fmt or plugins.
 
+--hierarchy::
+	Enable hierarchical output.
+
 include::callchain-overhead-calculation.txt[]
 
 SEE ALSO
diff --git a/tools/perf/Documentation/tips.txt b/tools/perf/Documentation/tips.txt
index e0ce9573b79b..5950b5a24efd 100644
--- a/tools/perf/Documentation/tips.txt
+++ b/tools/perf/Documentation/tips.txt
@@ -27,3 +27,4 @@ Skip collecing build-id when recording: perf record -B
 To change sampling frequency to 100 Hz: perf record -F 100
 See assembly instructions with percentage: perf annotate <symbol>
 If you prefer Intel style assembly, try: perf annotate -M intel
+For hierarchical output, try: perf report --hierarchy
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 2bf537f190a0..fd26db0f4c74 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -798,6 +798,8 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
 		    "only show processor socket that match with this filter"),
 	OPT_BOOLEAN(0, "raw-trace", &symbol_conf.raw_trace,
 		    "Show raw trace event output (do not use print fmt or plugins)"),
+	OPT_BOOLEAN(0, "hierarchy", &symbol_conf.report_hierarchy,
+		    "Show entries in a hierarchy"),
 	OPT_END()
 	};
 	struct perf_data_file file = {
@@ -907,6 +909,21 @@ repeat:
 		symbol_conf.cumulate_callchain = false;
 	}
 
+	if (symbol_conf.report_hierarchy) {
+		/* disable incompatible options */
+		symbol_conf.event_group = false;
+		symbol_conf.cumulate_callchain = false;
+
+		if (field_order) {
+			pr_err("Error: --hierarchy and --fields options cannot be used together\n");
+			parse_options_usage(report_usage, options, "F", 1);
+			parse_options_usage(NULL, options, "hierarchy", 0);
+			goto error;
+		}
+
+		sort__need_collapse = true;
+	}
+
 	if (setup_sorting(session->evlist) < 0) {
 		if (sort_order)
 			parse_options_usage(report_usage, options, "s", 1);
-- 
2.6.4

[PATCH 16/17] perf hists: Support decaying in hierarchy mode

[Namhyung Kim]

In the hierarchy mode, hist entries should decay their children too.
Also update hists__delete_entry() to be able to free child entries.

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

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index a0808f73e0d9..979cd6091034 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -243,6 +243,8 @@ static void he_stat__decay(struct he_stat *he_stat)
 	/* XXX need decay for weight too? */
 }
 
+static void hists__delete_entry(struct hists *hists, struct hist_entry *he);
+
 static bool hists__decay_entry(struct hists *hists, struct hist_entry *he)
 {
 	u64 prev_period = he->stat.period;
@@ -258,21 +260,45 @@ static bool hists__decay_entry(struct hists *hists, struct hist_entry *he)
 
 	diff = prev_period - he->stat.period;
 
-	hists->stats.total_period -= diff;
-	if (!he->filtered)
-		hists->stats.total_non_filtered_period -= diff;
+	if (!he->depth) {
+		hists->stats.total_period -= diff;
+		if (!he->filtered)
+			hists->stats.total_non_filtered_period -= diff;
+	}
+
+	if (!he->leaf) {
+		struct hist_entry *child;
+		struct rb_node *node = rb_first(&he->hroot_out);
+		while (node) {
+			child = rb_entry(node, struct hist_entry, rb_node);
+			node = rb_next(node);
+
+			if (hists__decay_entry(hists, child))
+				hists__delete_entry(hists, child);
+		}
+	}
 
 	return he->stat.period == 0;
 }
 
 static void hists__delete_entry(struct hists *hists, struct hist_entry *he)
 {
-	rb_erase(&he->rb_node, &hists->entries);
+	struct rb_root *root_in;
+	struct rb_root *root_out;
 
-	if (sort__need_collapse)
-		rb_erase(&he->rb_node_in, &hists->entries_collapsed);
-	else
-		rb_erase(&he->rb_node_in, hists->entries_in);
+	if (he->parent_he) {
+		root_in  = &he->parent_he->hroot_in;
+		root_out = &he->parent_he->hroot_out;
+	} else {
+		if (sort__need_collapse)
+			root_in = &hists->entries_collapsed;
+		else
+			root_in = hists->entries_in;
+		root_out = &hists->entries;
+	}
+
+	rb_erase(&he->rb_node_in, root_in);
+	rb_erase(&he->rb_node, root_out);
 
 	--hists->nr_entries;
 	if (!he->filtered)
-- 
2.6.4

[PATCH 17/17] perf top: Add --hierarchy option

[Namhyung Kim]

Support hierarchy output for perf-top using --hierarchy option.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/Documentation/perf-top.txt |  3 +++
 tools/perf/builtin-top.c              | 15 +++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/tools/perf/Documentation/perf-top.txt b/tools/perf/Documentation/perf-top.txt
index b0e60e17db38..19f046f027cd 100644
--- a/tools/perf/Documentation/perf-top.txt
+++ b/tools/perf/Documentation/perf-top.txt
@@ -233,6 +233,9 @@ Default is to monitor all CPUS.
 --raw-trace::
 	When displaying traceevent output, do not use print fmt or plugins.
 
+--hierarchy::
+	Enable hierarchy output.
+
 INTERACTIVE PROMPTING KEYS
 --------------------------
 
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index bf01cbb0ef23..5da5b098f141 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1212,6 +1212,8 @@ int cmd_top(int argc, const char **argv, const char *prefix __maybe_unused)
 		     parse_branch_stack),
 	OPT_BOOLEAN(0, "raw-trace", &symbol_conf.raw_trace,
 		    "Show raw trace event output (do not use print fmt or plugins)"),
+	OPT_BOOLEAN(0, "hierarchy", &symbol_conf.report_hierarchy,
+		    "Show entries in a hierarchy"),
 	OPT_END()
 	};
 	const char * const top_usage[] = {
@@ -1239,6 +1241,19 @@ int cmd_top(int argc, const char **argv, const char *prefix __maybe_unused)
 		goto out_delete_evlist;
 	}
 
+	if (symbol_conf.report_hierarchy) {
+		/* disable incompatible options */
+		symbol_conf.event_group = false;
+		symbol_conf.cumulate_callchain = false;
+
+		if (field_order) {
+			pr_err("Error: --hierarchy and --fields options cannot be used together\n");
+			parse_options_usage(top_usage, options, "fields", 0);
+			parse_options_usage(NULL, options, "hierarchy", 0);
+			goto out_delete_evlist;
+		}
+	}
+
 	sort__mode = SORT_MODE__TOP;
 	/* display thread wants entries to be collapsed in a different tree */
 	sort__need_collapse = 1;
-- 
2.6.4

Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view (v2)

[Pekka Enberg]

On Sat, Jan 16, 2016 at 6:03 PM, Namhyung Kim <namhyung@kernel.org> wrote:
> This is v2 attempt of my earlier patchset [1].  This patchset
> implements a new feature that collects hist entries in a hierachical
> manner.  That means lower-level entries belong to an upper-level
> entry.  The entry hierachy is built on the sort keys given, so users
> can set it whatever they want.  It only shows top-level entries first,
> and user can expand/collapse it dynamically.

FWIW, for the series:

Acked-by: Pekka Enberg <penberg@kernel.org>

- Pekka

Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view (v2)

[Namhyung Kim]

Hi Pekka,

On Sun, Jan 17, 2016 at 12:25:00PM +0200, Pekka Enberg wrote:
> On Sat, Jan 16, 2016 at 6:03 PM, Namhyung Kim <namhyung@kernel.org> wrote:
> > This is v2 attempt of my earlier patchset [1].  This patchset
> > implements a new feature that collects hist entries in a hierachical
> > manner.  That means lower-level entries belong to an upper-level
> > entry.  The entry hierachy is built on the sort keys given, so users
> > can set it whatever they want.  It only shows top-level entries first,
> > and user can expand/collapse it dynamically.
> 
> FWIW, for the series:
> 
> Acked-by: Pekka Enberg <penberg@kernel.org>

Thank you!
Namhyung

Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view (v2)

[Andi Kleen]

On Sun, Jan 17, 2016 at 01:03:00AM +0900, Namhyung Kim wrote:
> Hello,
> 
> This is v2 attempt of my earlier patchset [1].  This patchset
> implements a new feature that collects hist entries in a hierachical
> manner.  That means lower-level entries belong to an upper-level
> entry.  The entry hierachy is built on the sort keys given, so users
> can set it whatever they want.  It only shows top-level entries first,
> and user can expand/collapse it dynamically.

Thanks that's very nice. Especially for TSX profiling it was always
very annoying that the other sort keys didn't actually sort.
It should be very useful for LBR view too.

Now we only need a better percent-limit that handles all
leaves correctly...

-Andi

Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view (v2)

[Namhyung Kim]

Hi Andi,

On Sun, Jan 17, 2016 at 08:31:53PM +0100, Andi Kleen wrote:
> On Sun, Jan 17, 2016 at 01:03:00AM +0900, Namhyung Kim wrote:
> > Hello,
> > 
> > This is v2 attempt of my earlier patchset [1].  This patchset
> > implements a new feature that collects hist entries in a hierachical
> > manner.  That means lower-level entries belong to an upper-level
> > entry.  The entry hierachy is built on the sort keys given, so users
> > can set it whatever they want.  It only shows top-level entries first,
> > and user can expand/collapse it dynamically.
> 
> Thanks that's very nice. Especially for TSX profiling it was always
> very annoying that the other sort keys didn't actually sort.
> It should be very useful for LBR view too.

Glad to hear that you like it. :)


> 
> Now we only need a better percent-limit that handles all
> leaves correctly...

Hmm.. could you tell me where the percent-limit doesn't work
correctly?

Thanks,
Namhyung

Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view (v2)

[Arnaldo Carvalho de Melo]

Em Tue, Jan 19, 2016 at 07:45:06PM +0900, Namhyung Kim escreveu:
> Hi Andi,
> 
> On Sun, Jan 17, 2016 at 08:31:53PM +0100, Andi Kleen wrote:
> > On Sun, Jan 17, 2016 at 01:03:00AM +0900, Namhyung Kim wrote:
> > > Hello,
> > > 
> > > This is v2 attempt of my earlier patchset [1].  This patchset
> > > implements a new feature that collects hist entries in a hierachical
> > > manner.  That means lower-level entries belong to an upper-level
> > > entry.  The entry hierachy is built on the sort keys given, so users
> > > can set it whatever they want.  It only shows top-level entries first,
> > > and user can expand/collapse it dynamically.
> > 
> > Thanks that's very nice. Especially for TSX profiling it was always
> > very annoying that the other sort keys didn't actually sort.
> > It should be very useful for LBR view too.
> 
> Glad to hear that you like it. :)
> 
> 
> > 
> > Now we only need a better percent-limit that handles all
> > leaves correctly...
> 
> Hmm.. could you tell me where the percent-limit doesn't work
> correctly?

Yeah, please elaborate, I'm testing:

perf top -s comm,dso,sym --hierarchy --percent-limit 0.5


And it seems to work...

- Arnaldo

Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view (v2)

[Arnaldo Carvalho de Melo]

Em Tue, Jan 19, 2016 at 06:03:39PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Jan 19, 2016 at 07:45:06PM +0900, Namhyung Kim escreveu:
> > On Sun, Jan 17, 2016 at 08:31:53PM +0100, Andi Kleen wrote:
> > > Now we only need a better percent-limit that handles all
> > > leaves correctly...

> > Hmm.. could you tell me where the percent-limit doesn't work
> > correctly?

> Yeah, please elaborate, I'm testing:

> perf top -s comm,dso,sym --hierarchy --percent-limit 0.5
> 
> And it seems to work...

For instance, running that top command above, then expanding some of the
callchains and pressing 'P' to generate a perf.hist.0 output file, all
obey the 0.5 threshold for discading entries with less than that
percentage, what is the problem you're noticing? Is it only in stdio or
what?

-  76.76%            cc1
   -  62.43%            cc1
          2.82%            [.] _cpp_lex_token
          1.59%            [.] ht_lookup_with_hash
          1.23%            [.] ggc_internal_alloc
          0.85%            [.] bitmap_set_bit
          0.53%            [.] cpp_get_token_with_location
          0.51%            [.] ht_lookup
   -   7.77%            libc-2.22.so
          2.06%            [.] _int_malloc
          0.84%            [.] _int_free
          0.57%            [.] _IO_putc
          0.55%            [.] malloc_consolidate
   -   5.86%            [kernel]
-   9.38%            as
   +   6.03%            as
   +   1.39%            [kernel]
   +   1.28%            libc-2.22.so
-   3.41%            gcc
   +   1.63%            [kernel]
   +   0.64%            [unknown]
   -   0.52%            ccache
+   2.28%            ld
+   1.85%            perf
+   1.45%            sh
+   1.16%            perl
+   0.55%            make

Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view (v2)

[Andi Kleen]

> > Now we only need a better percent-limit that handles all
> > leaves correctly...
> 
> Hmm.. could you tell me where the percent-limit doesn't work
> correctly?

It doesn't work well together with call chains.

It only considers the hits on the leaf function, but doesn't hide
the individual call chains leading to that function which are below
the limit.

So if you have a lot of different cold callers you still end up
with far too much output.

Also would be nice to be able to set it dynamically from the UI.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view (v2)

[Arnaldo Carvalho de Melo]

Em Tue, Jan 19, 2016 at 11:12:04PM +0100, Andi Kleen escreveu:
> > > Now we only need a better percent-limit that handles all
> > > leaves correctly...
> > 
> > Hmm.. could you tell me where the percent-limit doesn't work
> > correctly?
> 
> It doesn't work well together with call chains.

Ok, that is the missing piece of info, lemme check... Yeah that seems
to be the case.
 
> It only considers the hits on the leaf function, but doesn't hide
> the individual call chains leading to that function which are below
> the limit.
> 
> So if you have a lot of different cold callers you still end up
> with far too much output.
> 
> Also would be nice to be able to set it dynamically from the UI.

To set the mim percent? Yeah, we need to allow setting all those knobs
from the UI.

I.e. should be like when we press '/' to set a substring filter.

- Arnaldo

Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view (v2)

[Namhyung Kim]

On Tue, Jan 19, 2016 at 07:24:49PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Jan 19, 2016 at 11:12:04PM +0100, Andi Kleen escreveu:
> > > > Now we only need a better percent-limit that handles all
> > > > leaves correctly...
> > > 
> > > Hmm.. could you tell me where the percent-limit doesn't work
> > > correctly?
> > 
> > It doesn't work well together with call chains.
> 
> Ok, that is the missing piece of info, lemme check... Yeah that seems
> to be the case.

So technically this is a callchain issue not a hist entry, right?  If
so, it should be fixed separately.  Currently --percent-limit option
works only for hist entries, and for callchain, users should use
-g/--call-graph option.  With hierarchy report, we might consider
merging them since it provides continuous interface to callchains..


>  
> > It only considers the hits on the leaf function, but doesn't hide
> > the individual call chains leading to that function which are below
> > the limit.
> > 
> > So if you have a lot of different cold callers you still end up
> > with far too much output.

Could you please be more specific?  Let me try with an example..

  $ perf report
  ...
  +    1.00%  perf  perf  some_function
           some_function
         - common_caller
            - 0.6% caller1
              + 0.4% caller1_1
              + 0.2% caller1_2
            + 0.3% caller2
            + 0.1% caller3

In this case, do you want this?

  $ perf report --percent-limit 0.5
  ...
  +    1.00%  perf  perf  some_function
           some_function
         - common_caller
            - 0.6% caller1


> > 
> > Also would be nice to be able to set it dynamically from the UI.
> 
> To set the mim percent? Yeah, we need to allow setting all those knobs
> from the UI.
> 
> I.e. should be like when we press '/' to set a substring filter.

Yes, how about 'L' key for limit?

Thanks,
Namhyung

Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view (v2)

[Andi Kleen]

On Wed, Jan 20, 2016 at 09:56:47AM +0900, Namhyung Kim wrote:
> On Tue, Jan 19, 2016 at 07:24:49PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Jan 19, 2016 at 11:12:04PM +0100, Andi Kleen escreveu:
> > > > > Now we only need a better percent-limit that handles all
> > > > > leaves correctly...
> > > > 
> > > > Hmm.. could you tell me where the percent-limit doesn't work
> > > > correctly?
> > > 
> > > It doesn't work well together with call chains.
> > 
> > Ok, that is the missing piece of info, lemme check... Yeah that seems
> > to be the case.
> 
> So technically this is a callchain issue not a hist entry, right?  If

Correct.

> so, it should be fixed separately.  Currently --percent-limit option
> works only for hist entries, and for callchain, users should use
> -g/--call-graph option.  With hierarchy report, we might consider
> merging them since it provides continuous interface to callchains..

Ideally it should work all modes, not just hierarchy.

> 
> >  
> > > It only considers the hits on the leaf function, but doesn't hide
> > > the individual call chains leading to that function which are below
> > > the limit.
> > > 
> > > So if you have a lot of different cold callers you still end up
> > > with far too much output.
> 
> Could you please be more specific?  Let me try with an example..
> 
>   $ perf report
>   ...
>   +    1.00%  perf  perf  some_function
>            some_function
>          - common_caller
>             - 0.6% caller1
>               + 0.4% caller1_1
>               + 0.2% caller1_2
>             + 0.3% caller2
>             + 0.1% caller3
> 
> In this case, do you want this?
> 
>   $ perf report --percent-limit 0.5
>   ...
>   +    1.00%  perf  perf  some_function
>            some_function
>          - common_caller
>             - 0.6% caller1

Right. Only call chains whose total percentage is larger than the limit.

> 
> 
> > > 
> > > Also would be nice to be able to set it dynamically from the UI.
> > 
> > To set the mim percent? Yeah, we need to allow setting all those knobs
> > from the UI.
> > 
> > I.e. should be like when we press '/' to set a substring filter.
> 
> Yes, how about 'L' key for limit?

Fine for me.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view (v2)

[Namhyung Kim]

Hi Andi,

On Wed, Jan 20, 2016 at 02:11:00AM +0100, Andi Kleen wrote:
> On Wed, Jan 20, 2016 at 09:56:47AM +0900, Namhyung Kim wrote:
> > On Tue, Jan 19, 2016 at 07:24:49PM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Tue, Jan 19, 2016 at 11:12:04PM +0100, Andi Kleen escreveu:
> > > > It only considers the hits on the leaf function, but doesn't hide
> > > > the individual call chains leading to that function which are below
> > > > the limit.
> > > > 
> > > > So if you have a lot of different cold callers you still end up
> > > > with far too much output.
> > 
> > Could you please be more specific?  Let me try with an example..
> > 
> >   $ perf report
> >   ...
> >   +    1.00%  perf  perf  some_function
> >            some_function
> >          - common_caller
> >             - 0.6% caller1
> >               + 0.4% caller1_1
> >               + 0.2% caller1_2
> >             + 0.3% caller2
> >             + 0.1% caller3
> > 
> > In this case, do you want this?
> > 
> >   $ perf report --percent-limit 0.5
> >   ...
> >   +    1.00%  perf  perf  some_function
> >            some_function
> >          - common_caller
> >             - 0.6% caller1
> 
> Right. Only call chains whose total percentage is larger than the limit.

So in this case, caller1_1 and caller1_2 are both under the percent
limit but after they merged at caller1, it should be shown.  Look like
a reasonable result.  I'll add it into my TODO list. :)

Btw, is it all you wanted to say?  Is there any other case for trouble
(wrt callchain percent limit)?

Thanks,
Namhyung

Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view (v2)

[Andi Kleen]

> Btw, is it all you wanted to say?  Is there any other case for trouble
> (wrt callchain percent limit)?

Yes that's all.  Don't know of more issues.

-Andi


-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view (v2)

[Arnaldo Carvalho de Melo]

Em Wed, Jan 20, 2016 at 09:56:47AM +0900, Namhyung Kim escreveu:
> On Tue, Jan 19, 2016 at 07:24:49PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Jan 19, 2016 at 11:12:04PM +0100, Andi Kleen escreveu:
> In this case, do you want this?
 
>   $ perf report --percent-limit 0.5
>   ...
>   +    1.00%  perf  perf  some_function
>            some_function
>          - common_caller
>             - 0.6% caller1

> > > Also would be nice to be able to set it dynamically from the UI.

> > To set the mim percent? Yeah, we need to allow setting all those knobs
> > from the UI.

> > I.e. should be like when we press '/' to set a substring filter.
 
> Yes, how about 'L' key for limit?

We have other limits we can set, perhaps we should use 'l' and do
something like mutt does, i.e. accept expressions that would allow us
to have a flexible way of setting limits, not just for the percentage
thresholds for hist_entries (callchains later).

- Arnaldo

Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view (v2)

[Arnaldo Carvalho de Melo]

Em Sun, Jan 17, 2016 at 01:03:00AM +0900, Namhyung Kim escreveu:
> Hello,
> 
> This is v2 attempt of my earlier patchset [1].  This patchset
> implements a new feature that collects hist entries in a hierachical
> manner.  That means lower-level entries belong to an upper-level
> entry.  The entry hierachy is built on the sort keys given, so users
> can set it whatever they want.  It only shows top-level entries first,
> and user can expand/collapse it dynamically.
> 
> This time I implemented it for every output browser including TUI.
> A screenshot on TUI looks like below:
> 
> For normal output:
> 
>   $ perf report --tui
>   Samples: 3K of event 'cycles:pp', Event count (approx.): 1695979674
>     Overhead  Command        Shared Object         Symbol
>   ------------------------------------------------------------------------
>   -    7.57%  swapper        [kernel.vmlinux]      [k] intel_idle
>        intel_idle
>        cpuidle_enter_state
>        cpuidle_enter
>        call_cpuidle
>      + cpu_startup_entry
>   +    1.16   firefox        firefox               [.] 0x00000000000019433
>   +    0.97%  firefox        libpthread-2.22.so    [.] pthread_mutex_lock
>   ...
> 
> 
> With hierarchy view,

So, it shows the overhead and the first of the sort keys, "comm", what
about the dso, how, from the following output, i.e. the initial screen
one gets from 'perf report --tui --hierarchy' we can switch to "dso",
i.e. the "Shared Object"?
 
>   $ perf report --tui --hierarchy
>   Samples: 3K of event 'cycles:pp', Event count (approx.): 1695979674
>    Overhead        Command / Shared Object / Symbol
>   -------------------------------------------------------------------
>   +  76.30%        firefox
>   -   9.95%        swapper
>      -   9.51%        [kernel.vmlinux]
>         -   7.57         [k] intel_idle
> 	     intel_idle
> 	     cpuidle_enter_state
> 	     cpuidle_enter
> 	     call_cpuidle
> 	   + cpu_startup_entry
> 	+   0.15%        [k] __schedule
> 	+   0.12%        [k] menu_select
> 	...
>      +   0.34%        [sdhci]
>      +   0.06%        [e1000e]
>      ...
>  +    5.65%        Xorg
>  +    5.42%        Socket Thread
>  ...
> 
> As you can see, overhead of an upper level entry is the sum of
> overhead of lower level entries.  The entries are aligned by its order
> of matching sort keys.
> 
> This is available from 'perf/hierarchy-v2' branch in my tree:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/namhyung-perf.git
> 
> 
> Any comments are welcome, thanks!
> Namhyung
> 
> 
> Cc: Don Zickus <dzickus@redhat.com>
> Cc: Pekka Enberg <penberg@kernel.org>
> Cc: Moinuddin Quadri <moin18@gmail.com>
> 
> 
> [1] https://lkml.org/lkml/2013/5/21/24
> 
> 
> Namhyung Kim (17):
>   perf hists: Basic support of hierarchical report view
>   perf hists: Resort hist entries with hierarchy
>   perf hists: Add helper functions for hierarchy mode
>   perf hists: Cleanup filtering functions
>   perf hists: Support filtering in hierarchy mode
>   perf ui/stdio: Implement hierarchy output mode
>   perf ui/stdio: Align column header for hierarchy output
>   perf hists browser: Fix context menu item
>   perf hists browser: Count number of hierarchy entries
>   perf hists browser: Support collapsing/expanding whole entries in
>     hierarchy
>   perf hists browser: Factor out hist_browser__show_callchain()
>   perf hists browser: Implement hierarchy output
>   perf hists browser: Align column header in hierarchy mode
>   perf ui/gtk: Implement hierarchy output mode
>   perf report: Add --hierarchy option
>   perf hists: Support decaying in hierarchy mode
>   perf top: Add --hierarchy option
> 
>  tools/perf/Documentation/perf-report.txt |   3 +
>  tools/perf/Documentation/perf-top.txt    |   3 +
>  tools/perf/Documentation/tips.txt        |   1 +
>  tools/perf/builtin-report.c              |  17 +
>  tools/perf/builtin-top.c                 |  15 +
>  tools/perf/ui/browsers/hists.c           | 590 ++++++++++++++++++++++++++-----
>  tools/perf/ui/gtk/hists.c                | 161 ++++++++-
>  tools/perf/ui/hist.c                     |  14 +
>  tools/perf/ui/stdio/hist.c               | 182 +++++++++-
>  tools/perf/util/hist.c                   | 470 ++++++++++++++++++++----
>  tools/perf/util/hist.h                   |  11 +
>  tools/perf/util/sort.c                   | 116 ++++++
>  tools/perf/util/sort.h                   |  16 +-
>  tools/perf/util/symbol.h                 |   3 +-
>  14 files changed, 1437 insertions(+), 165 deletions(-)
> 
> -- 
> 2.6.4

Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view (v2)

[Arnaldo Carvalho de Melo]

Em Sun, Jan 17, 2016 at 01:03:00AM +0900, Namhyung Kim escreveu:
> Hello,
> 
> This is v2 attempt of my earlier patchset [1].  This patchset
> implements a new feature that collects hist entries in a hierachical
> manner.  That means lower-level entries belong to an upper-level
> entry.  The entry hierachy is built on the sort keys given, so users
> can set it whatever they want.  It only shows top-level entries first,
> and user can expand/collapse it dynamically.
> 
> This time I implemented it for every output browser including TUI.
> A screenshot on TUI looks like below:
> 
> For normal output:
> 
>   $ perf report --tui
>   Samples: 3K of event 'cycles:pp', Event count (approx.): 1695979674
>     Overhead  Command        Shared Object         Symbol
>   ------------------------------------------------------------------------
>   -    7.57%  swapper        [kernel.vmlinux]      [k] intel_idle
>        intel_idle
>        cpuidle_enter_state
>        cpuidle_enter
>        call_cpuidle
>      + cpu_startup_entry
>   +    1.16   firefox        firefox               [.] 0x00000000000019433
>   +    0.97%  firefox        libpthread-2.22.so    [.] pthread_mutex_lock
>   ...
> 
> 
> With hierarchy view,
> 
>   $ perf report --tui --hierarchy
>   Samples: 3K of event 'cycles:pp', Event count (approx.): 1695979674
>    Overhead        Command / Shared Object / Symbol
>   -------------------------------------------------------------------
>   +  76.30%        firefox
>   -   9.95%        swapper
>      -   9.51%        [kernel.vmlinux]
>         -   7.57         [k] intel_idle
> 	     intel_idle
> 	     cpuidle_enter_state
> 	     cpuidle_enter
> 	     call_cpuidle
> 	   + cpu_startup_entry
> 	+   0.15%        [k] __schedule
> 	+   0.12%        [k] menu_select
> 	...
>      +   0.34%        [sdhci]
>      +   0.06%        [e1000e]
>      ...
>  +    5.65%        Xorg
>  +    5.42%        Socket Thread
>  ...
> 
> As you can see, overhead of an upper level entry is the sum of
> overhead of lower level entries.  The entries are aligned by its order
> of matching sort keys.
> 
> This is available from 'perf/hierarchy-v2' branch in my tree:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/namhyung-perf.git

Didn't work:

[acme@jouet linux]$ git remote add namhyung git://git.kernel.org/pub/scm/linux/kernel/git/namhyung-perf.git
[acme@jouet linux]$ git remote update namhyung
Fetching namhyung
fatal: remote error: access denied or repository not exported: /pub/scm/linux/kernel/git/namhyung-perf.git
error: Could not fetch namhyung
[acme@jouet linux]$

I looked it up and this one works:

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

Doing a test merge to test this.

- Arnaldo

Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view (v2)

[Namhyung Kim]

Hi Arnaldo,

On Tue, Jan 19, 2016 at 05:52:26PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Sun, Jan 17, 2016 at 01:03:00AM +0900, Namhyung Kim escreveu:
> > Hello,
> > 
> > This is v2 attempt of my earlier patchset [1].  This patchset
> > implements a new feature that collects hist entries in a hierachical
> > manner.  That means lower-level entries belong to an upper-level
> > entry.  The entry hierachy is built on the sort keys given, so users
> > can set it whatever they want.  It only shows top-level entries first,
> > and user can expand/collapse it dynamically.
> > 
> > This time I implemented it for every output browser including TUI.
> > A screenshot on TUI looks like below:
> > 
> > For normal output:
> > 
> >   $ perf report --tui
> >   Samples: 3K of event 'cycles:pp', Event count (approx.): 1695979674
> >     Overhead  Command        Shared Object         Symbol
> >   ------------------------------------------------------------------------
> >   -    7.57%  swapper        [kernel.vmlinux]      [k] intel_idle
> >        intel_idle
> >        cpuidle_enter_state
> >        cpuidle_enter
> >        call_cpuidle
> >      + cpu_startup_entry
> >   +    1.16   firefox        firefox               [.] 0x00000000000019433
> >   +    0.97%  firefox        libpthread-2.22.so    [.] pthread_mutex_lock
> >   ...
> > 
> > 
> > With hierarchy view,
> > 
> >   $ perf report --tui --hierarchy
> >   Samples: 3K of event 'cycles:pp', Event count (approx.): 1695979674
> >    Overhead        Command / Shared Object / Symbol
> >   -------------------------------------------------------------------
> >   +  76.30%        firefox
> >   -   9.95%        swapper
> >      -   9.51%        [kernel.vmlinux]
> >         -   7.57         [k] intel_idle
> > 	     intel_idle
> > 	     cpuidle_enter_state
> > 	     cpuidle_enter
> > 	     call_cpuidle
> > 	   + cpu_startup_entry
> > 	+   0.15%        [k] __schedule
> > 	+   0.12%        [k] menu_select
> > 	...
> >      +   0.34%        [sdhci]
> >      +   0.06%        [e1000e]
> >      ...
> >  +    5.65%        Xorg
> >  +    5.42%        Socket Thread
> >  ...
> > 
> > As you can see, overhead of an upper level entry is the sum of
> > overhead of lower level entries.  The entries are aligned by its order
> > of matching sort keys.
> > 
> > This is available from 'perf/hierarchy-v2' branch in my tree:
> > 
> >   git://git.kernel.org/pub/scm/linux/kernel/git/namhyung-perf.git
> 
> Didn't work:
> 
> [acme@jouet linux]$ git remote add namhyung git://git.kernel.org/pub/scm/linux/kernel/git/namhyung-perf.git
> [acme@jouet linux]$ git remote update namhyung
> Fetching namhyung
> fatal: remote error: access denied or repository not exported: /pub/scm/linux/kernel/git/namhyung-perf.git
> error: Could not fetch namhyung
> [acme@jouet linux]$
> 
> I looked it up and this one works:
> 
>     git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
> 
> Doing a test merge to test this.

Oops, sorry about that.  I should not type it manually..

Thanks,
Namhyung

Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view (v2)

[Arnaldo Carvalho de Melo]

Em Sun, Jan 17, 2016 at 01:03:00AM +0900, Namhyung Kim escreveu:
> Hello,
> 
> This is v2 attempt of my earlier patchset [1].  This patchset
> implements a new feature that collects hist entries in a hierachical
> manner.  That means lower-level entries belong to an upper-level
> entry.  The entry hierachy is built on the sort keys given, so users
> can set it whatever they want.  It only shows top-level entries first,
> and user can expand/collapse it dynamically.
> 
> This time I implemented it for every output browser including TUI.
> A screenshot on TUI looks like below:
> 
> For normal output:
> 
>   $ perf report --tui
>   Samples: 3K of event 'cycles:pp', Event count (approx.): 1695979674
>     Overhead  Command        Shared Object         Symbol
>   ------------------------------------------------------------------------
>   -    7.57%  swapper        [kernel.vmlinux]      [k] intel_idle
>        intel_idle
>        cpuidle_enter_state
>        cpuidle_enter
>        call_cpuidle
>      + cpu_startup_entry
>   +    1.16   firefox        firefox               [.] 0x00000000000019433
>   +    0.97%  firefox        libpthread-2.22.so    [.] pthread_mutex_lock
>   ...
> 
> 
> With hierarchy view,

Ok, tested, this is really nice, I think it should be the default, from
where to drill down, we could have a '--no-hierarchy', Ingo?

I'll took a quick look patch by patch, seems ok, will try to look deeper
to get this merged soon,

keep up the great work!

- Arnaldo
 
>   $ perf report --tui --hierarchy
>   Samples: 3K of event 'cycles:pp', Event count (approx.): 1695979674
>    Overhead        Command / Shared Object / Symbol
>   -------------------------------------------------------------------
>   +  76.30%        firefox
>   -   9.95%        swapper
>      -   9.51%        [kernel.vmlinux]
>         -   7.57         [k] intel_idle
> 	     intel_idle
> 	     cpuidle_enter_state
> 	     cpuidle_enter
> 	     call_cpuidle
> 	   + cpu_startup_entry
> 	+   0.15%        [k] __schedule
> 	+   0.12%        [k] menu_select
> 	...
>      +   0.34%        [sdhci]
>      +   0.06%        [e1000e]
>      ...
>  +    5.65%        Xorg
>  +    5.42%        Socket Thread
>  ...
> 
> As you can see, overhead of an upper level entry is the sum of
> overhead of lower level entries.  The entries are aligned by its order
> of matching sort keys.
> 
> This is available from 'perf/hierarchy-v2' branch in my tree:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/namhyung-perf.git
> 
> 
> Any comments are welcome, thanks!
> Namhyung
> 
> 
> Cc: Don Zickus <dzickus@redhat.com>
> Cc: Pekka Enberg <penberg@kernel.org>
> Cc: Moinuddin Quadri <moin18@gmail.com>
> 
> 
> [1] https://lkml.org/lkml/2013/5/21/24
> 
> 
> Namhyung Kim (17):
>   perf hists: Basic support of hierarchical report view
>   perf hists: Resort hist entries with hierarchy
>   perf hists: Add helper functions for hierarchy mode
>   perf hists: Cleanup filtering functions
>   perf hists: Support filtering in hierarchy mode
>   perf ui/stdio: Implement hierarchy output mode
>   perf ui/stdio: Align column header for hierarchy output
>   perf hists browser: Fix context menu item
>   perf hists browser: Count number of hierarchy entries
>   perf hists browser: Support collapsing/expanding whole entries in
>     hierarchy
>   perf hists browser: Factor out hist_browser__show_callchain()
>   perf hists browser: Implement hierarchy output
>   perf hists browser: Align column header in hierarchy mode
>   perf ui/gtk: Implement hierarchy output mode
>   perf report: Add --hierarchy option
>   perf hists: Support decaying in hierarchy mode
>   perf top: Add --hierarchy option
> 
>  tools/perf/Documentation/perf-report.txt |   3 +
>  tools/perf/Documentation/perf-top.txt    |   3 +
>  tools/perf/Documentation/tips.txt        |   1 +
>  tools/perf/builtin-report.c              |  17 +
>  tools/perf/builtin-top.c                 |  15 +
>  tools/perf/ui/browsers/hists.c           | 590 ++++++++++++++++++++++++++-----
>  tools/perf/ui/gtk/hists.c                | 161 ++++++++-
>  tools/perf/ui/hist.c                     |  14 +
>  tools/perf/ui/stdio/hist.c               | 182 +++++++++-
>  tools/perf/util/hist.c                   | 470 ++++++++++++++++++++----
>  tools/perf/util/hist.h                   |  11 +
>  tools/perf/util/sort.c                   | 116 ++++++
>  tools/perf/util/sort.h                   |  16 +-
>  tools/perf/util/symbol.h                 |   3 +-
>  14 files changed, 1437 insertions(+), 165 deletions(-)
> 
> -- 
> 2.6.4

Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view (v2)

[Namhyung Kim]

On Tue, Jan 19, 2016 at 05:59:41PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Sun, Jan 17, 2016 at 01:03:00AM +0900, Namhyung Kim escreveu:
> > Hello,
> > 
> > This is v2 attempt of my earlier patchset [1].  This patchset
> > implements a new feature that collects hist entries in a hierachical
> > manner.  That means lower-level entries belong to an upper-level
> > entry.  The entry hierachy is built on the sort keys given, so users
> > can set it whatever they want.  It only shows top-level entries first,
> > and user can expand/collapse it dynamically.
> > 
> > This time I implemented it for every output browser including TUI.
> > A screenshot on TUI looks like below:
> > 
> > For normal output:
> > 
> >   $ perf report --tui
> >   Samples: 3K of event 'cycles:pp', Event count (approx.): 1695979674
> >     Overhead  Command        Shared Object         Symbol
> >   ------------------------------------------------------------------------
> >   -    7.57%  swapper        [kernel.vmlinux]      [k] intel_idle
> >        intel_idle
> >        cpuidle_enter_state
> >        cpuidle_enter
> >        call_cpuidle
> >      + cpu_startup_entry
> >   +    1.16   firefox        firefox               [.] 0x00000000000019433
> >   +    0.97%  firefox        libpthread-2.22.so    [.] pthread_mutex_lock
> >   ...
> > 
> > 
> > With hierarchy view,
> 
> Ok, tested, this is really nice, I think it should be the default, from
> where to drill down, we could have a '--no-hierarchy', Ingo?

Yeah, we already have --no-hierarchy (as a side effect of having
--hierarchy) but I don't want to change the default now since existing
users will complain.  Now we have 'tips' in the perf report browser,
maybe it's enough to add a line to suggest to use it (and it's already
done by this patchset).  I remember the time we changed default for
'--children' and many people complained about it.

We maybe change the default later but I think it's better to have some
time to people can play with it and find it useful. :)  And, as always,
we can have a config option to control the default.

Btw, do you think it's worth adding a short option (-H) for
--hierarchy (instead of making it default)?

Thanks,
Namhyung


> 
> I'll took a quick look patch by patch, seems ok, will try to look deeper
> to get this merged soon,
> 
> keep up the great work!

Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view (v2)

[Andi Kleen]

> Btw, do you think it's worth adding a short option (-H) for
> --hierarchy (instead of making it default)?

Yes please.

-Andi

Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view (v2)

[Taeung Song]

On 01/20/2016 09:34 AM, Namhyung Kim wrote:
> On Tue, Jan 19, 2016 at 05:59:41PM -0300, Arnaldo Carvalho de Melo wrote:
>> Em Sun, Jan 17, 2016 at 01:03:00AM +0900, Namhyung Kim escreveu:
>>> Hello,
>>>
>>> This is v2 attempt of my earlier patchset [1].  This patchset
>>> implements a new feature that collects hist entries in a hierachical
>>> manner.  That means lower-level entries belong to an upper-level
>>> entry.  The entry hierachy is built on the sort keys given, so users
>>> can set it whatever they want.  It only shows top-level entries first,
>>> and user can expand/collapse it dynamically.
>>>
>>> This time I implemented it for every output browser including TUI.
>>> A screenshot on TUI looks like below:
>>>
>>> For normal output:
>>>
>>>    $ perf report --tui
>>>    Samples: 3K of event 'cycles:pp', Event count (approx.): 1695979674
>>>      Overhead  Command        Shared Object         Symbol
>>>    ------------------------------------------------------------------------
>>>    -    7.57%  swapper        [kernel.vmlinux]      [k] intel_idle
>>>         intel_idle
>>>         cpuidle_enter_state
>>>         cpuidle_enter
>>>         call_cpuidle
>>>       + cpu_startup_entry
>>>    +    1.16   firefox        firefox               [.] 0x00000000000019433
>>>    +    0.97%  firefox        libpthread-2.22.so    [.] pthread_mutex_lock
>>>    ...
>>>
>>>
>>> With hierarchy view,
>>
>> Ok, tested, this is really nice, I think it should be the default, from
>> where to drill down, we could have a '--no-hierarchy', Ingo?
>
> Yeah, we already have --no-hierarchy (as a side effect of having
> --hierarchy) but I don't want to change the default now since existing
> users will complain.  Now we have 'tips' in the perf report browser,
> maybe it's enough to add a line to suggest to use it (and it's already
> done by this patchset).  I remember the time we changed default for
> '--children' and many people complained about it.
>
> We maybe change the default later but I think it's better to have some
> time to people can play with it and find it useful. :)  And, as always,
> we can have a config option to control the default.

If adding this config option,
can this be included in 'hist' section ?
If it isn't, 'report' and 'top' section ?
i.e.

[report]
     hierarchy = true
[top]
     hierarchy = false

Thanks,
Taeung

>
> Btw, do you think it's worth adding a short option (-H) for
> --hierarchy (instead of making it default)?
>
> Thanks,
> Namhyung
>
>
>>
>> I'll took a quick look patch by patch, seems ok, will try to look deeper
>> to get this merged soon,
>>
>> keep up the great work!

Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view (v2)

[Namhyung Kim]

Hi Taeung,

On Wed, Jan 20, 2016 at 04:49:29PM +0900, Taeung Song wrote:
> 
> 
> On 01/20/2016 09:34 AM, Namhyung Kim wrote:
> >On Tue, Jan 19, 2016 at 05:59:41PM -0300, Arnaldo Carvalho de Melo wrote:
> >>Em Sun, Jan 17, 2016 at 01:03:00AM +0900, Namhyung Kim escreveu:
> >>>Hello,
> >>>
> >>>This is v2 attempt of my earlier patchset [1].  This patchset
> >>>implements a new feature that collects hist entries in a hierachical
> >>>manner.  That means lower-level entries belong to an upper-level
> >>>entry.  The entry hierachy is built on the sort keys given, so users
> >>>can set it whatever they want.  It only shows top-level entries first,
> >>>and user can expand/collapse it dynamically.
> >>>
> >>>This time I implemented it for every output browser including TUI.
> >>>A screenshot on TUI looks like below:
> >>>
> >>>For normal output:
> >>>
> >>>   $ perf report --tui
> >>>   Samples: 3K of event 'cycles:pp', Event count (approx.): 1695979674
> >>>     Overhead  Command        Shared Object         Symbol
> >>>   ------------------------------------------------------------------------
> >>>   -    7.57%  swapper        [kernel.vmlinux]      [k] intel_idle
> >>>        intel_idle
> >>>        cpuidle_enter_state
> >>>        cpuidle_enter
> >>>        call_cpuidle
> >>>      + cpu_startup_entry
> >>>   +    1.16   firefox        firefox               [.] 0x00000000000019433
> >>>   +    0.97%  firefox        libpthread-2.22.so    [.] pthread_mutex_lock
> >>>   ...
> >>>
> >>>
> >>>With hierarchy view,
> >>
> >>Ok, tested, this is really nice, I think it should be the default, from
> >>where to drill down, we could have a '--no-hierarchy', Ingo?
> >
> >Yeah, we already have --no-hierarchy (as a side effect of having
> >--hierarchy) but I don't want to change the default now since existing
> >users will complain.  Now we have 'tips' in the perf report browser,
> >maybe it's enough to add a line to suggest to use it (and it's already
> >done by this patchset).  I remember the time we changed default for
> >'--children' and many people complained about it.
> >
> >We maybe change the default later but I think it's better to have some
> >time to people can play with it and find it useful. :)  And, as always,
> >we can have a config option to control the default.
> 
> If adding this config option,
> can this be included in 'hist' section ?
> If it isn't, 'report' and 'top' section ?
> i.e.
> 
> [report]
>     hierarchy = true
> [top]
>     hierarchy = false

Either is fine.  But as we already have report.children and
top.children, I'd follow the convention.  Also I think we should set
priority of the two configs - children and hierarchy.  IMHO hierarchy
should be considered first.

Or maybe we could have 'report.output-default' being one of
'hierarchy', 'children', or 'normal'.  This way we can set the default
behavior easily including possible future changes.

Thanks,
Namhyung

Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view (v2)

[Taeung Song]

Hi, Namhyung

On 01/21/2016 12:08 AM, Namhyung Kim wrote:
> Hi Taeung,
>
> On Wed, Jan 20, 2016 at 04:49:29PM +0900, Taeung Song wrote:
>>
>>
>> On 01/20/2016 09:34 AM, Namhyung Kim wrote:
>>> On Tue, Jan 19, 2016 at 05:59:41PM -0300, Arnaldo Carvalho de Melo wrote:
>>>> Em Sun, Jan 17, 2016 at 01:03:00AM +0900, Namhyung Kim escreveu:
>>>>> Hello,
>>>>>
>>>>> This is v2 attempt of my earlier patchset [1].  This patchset
>>>>> implements a new feature that collects hist entries in a hierachical
>>>>> manner.  That means lower-level entries belong to an upper-level
>>>>> entry.  The entry hierachy is built on the sort keys given, so users
>>>>> can set it whatever they want.  It only shows top-level entries first,
>>>>> and user can expand/collapse it dynamically.
>>>>>
>>>>> This time I implemented it for every output browser including TUI.
>>>>> A screenshot on TUI looks like below:
>>>>>
>>>>> For normal output:
>>>>>
>>>>>    $ perf report --tui
>>>>>    Samples: 3K of event 'cycles:pp', Event count (approx.): 1695979674
>>>>>      Overhead  Command        Shared Object         Symbol
>>>>>    ------------------------------------------------------------------------
>>>>>    -    7.57%  swapper        [kernel.vmlinux]      [k] intel_idle
>>>>>         intel_idle
>>>>>         cpuidle_enter_state
>>>>>         cpuidle_enter
>>>>>         call_cpuidle
>>>>>       + cpu_startup_entry
>>>>>    +    1.16   firefox        firefox               [.] 0x00000000000019433
>>>>>    +    0.97%  firefox        libpthread-2.22.so    [.] pthread_mutex_lock
>>>>>    ...
>>>>>
>>>>>
>>>>> With hierarchy view,
>>>>
>>>> Ok, tested, this is really nice, I think it should be the default, from
>>>> where to drill down, we could have a '--no-hierarchy', Ingo?
>>>
>>> Yeah, we already have --no-hierarchy (as a side effect of having
>>> --hierarchy) but I don't want to change the default now since existing
>>> users will complain.  Now we have 'tips' in the perf report browser,
>>> maybe it's enough to add a line to suggest to use it (and it's already
>>> done by this patchset).  I remember the time we changed default for
>>> '--children' and many people complained about it.
>>>
>>> We maybe change the default later but I think it's better to have some
>>> time to people can play with it and find it useful. :)  And, as always,
>>> we can have a config option to control the default.
>>
>> If adding this config option,
>> can this be included in 'hist' section ?
>> If it isn't, 'report' and 'top' section ?
>> i.e.
>>
>> [report]
>>      hierarchy = true
>> [top]
>>      hierarchy = false
>
> Either is fine.  But as we already have report.children and
> top.children, I'd follow the convention.  Also I think we should set
> priority of the two configs - children and hierarchy.  IMHO hierarchy
> should be considered first.
>
> Or maybe we could have 'report.output-default' being one of
> 'hierarchy', 'children', or 'normal'.  This way we can set the default
> behavior easily including possible future changes.
>

Oh, IMHO I think the latter is better than the former.
If using 'report.output-default' instead of 'report.children'
and 'report.hierarchy' etc integrating the configs,
it seems to be tidy.
Whatever this config variables will be set as,
after this patchset are merged I'll ask about this configs, again.

Thanks,
Taeung

Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view (v2)

[Namhyung Kim]

Hi Taeung,

On Thu, Jan 21, 2016 at 01:34:57AM +0900, Taeung Song wrote:
> On 01/21/2016 12:08 AM, Namhyung Kim wrote:
> >On Wed, Jan 20, 2016 at 04:49:29PM +0900, Taeung Song wrote:
> >>On 01/20/2016 09:34 AM, Namhyung Kim wrote:
> >>>On Tue, Jan 19, 2016 at 05:59:41PM -0300, Arnaldo Carvalho de Melo wrote:
> >>>>Ok, tested, this is really nice, I think it should be the default, from
> >>>>where to drill down, we could have a '--no-hierarchy', Ingo?
> >>>
> >>>Yeah, we already have --no-hierarchy (as a side effect of having
> >>>--hierarchy) but I don't want to change the default now since existing
> >>>users will complain.  Now we have 'tips' in the perf report browser,
> >>>maybe it's enough to add a line to suggest to use it (and it's already
> >>>done by this patchset).  I remember the time we changed default for
> >>>'--children' and many people complained about it.
> >>>
> >>>We maybe change the default later but I think it's better to have some
> >>>time to people can play with it and find it useful. :)  And, as always,
> >>>we can have a config option to control the default.
> >>
> >>If adding this config option,
> >>can this be included in 'hist' section ?
> >>If it isn't, 'report' and 'top' section ?
> >>i.e.
> >>
> >>[report]
> >>     hierarchy = true
> >>[top]
> >>     hierarchy = false
> >
> >Either is fine.  But as we already have report.children and
> >top.children, I'd follow the convention.  Also I think we should set
> >priority of the two configs - children and hierarchy.  IMHO hierarchy
> >should be considered first.
> >
> >Or maybe we could have 'report.output-default' being one of
> >'hierarchy', 'children', or 'normal'.  This way we can set the default
> >behavior easily including possible future changes.
> >
> 
> Oh, IMHO I think the latter is better than the former.
> If using 'report.output-default' instead of 'report.children'
> and 'report.hierarchy' etc integrating the configs,
> it seems to be tidy.

OK

> Whatever this config variables will be set as,
> after this patchset are merged I'll ask about this configs, again.

I'll add you in the CC list wrt config changes.

Thanks,
Namhyung

Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view (v2)

[Taeung Song]

Hi, Namhyung

On 01/21/2016 01:17 PM, Namhyung Kim wrote:
> Hi Taeung,
>
> On Thu, Jan 21, 2016 at 01:34:57AM +0900, Taeung Song wrote:
>> On 01/21/2016 12:08 AM, Namhyung Kim wrote:
>>> On Wed, Jan 20, 2016 at 04:49:29PM +0900, Taeung Song wrote:
>>>> On 01/20/2016 09:34 AM, Namhyung Kim wrote:
>>>>> On Tue, Jan 19, 2016 at 05:59:41PM -0300, Arnaldo Carvalho de Melo wrote:
>>>>>> Ok, tested, this is really nice, I think it should be the default, from
>>>>>> where to drill down, we could have a '--no-hierarchy', Ingo?
>>>>>
>>>>> Yeah, we already have --no-hierarchy (as a side effect of having
>>>>> --hierarchy) but I don't want to change the default now since existing
>>>>> users will complain.  Now we have 'tips' in the perf report browser,
>>>>> maybe it's enough to add a line to suggest to use it (and it's already
>>>>> done by this patchset).  I remember the time we changed default for
>>>>> '--children' and many people complained about it.
>>>>>
>>>>> We maybe change the default later but I think it's better to have some
>>>>> time to people can play with it and find it useful. :)  And, as always,
>>>>> we can have a config option to control the default.
>>>>
>>>> If adding this config option,
>>>> can this be included in 'hist' section ?
>>>> If it isn't, 'report' and 'top' section ?
>>>> i.e.
>>>>
>>>> [report]
>>>>      hierarchy = true
>>>> [top]
>>>>      hierarchy = false
>>>
>>> Either is fine.  But as we already have report.children and
>>> top.children, I'd follow the convention.  Also I think we should set
>>> priority of the two configs - children and hierarchy.  IMHO hierarchy
>>> should be considered first.
>>>
>>> Or maybe we could have 'report.output-default' being one of
>>> 'hierarchy', 'children', or 'normal'.  This way we can set the default
>>> behavior easily including possible future changes.
>>>
>>
>> Oh, IMHO I think the latter is better than the former.
>> If using 'report.output-default' instead of 'report.children'
>> and 'report.hierarchy' etc integrating the configs,
>> it seems to be tidy.
>
> OK
>
>> Whatever this config variables will be set as,
>> after this patchset are merged I'll ask about this configs, again.
>
> I'll add you in the CC list wrt config changes.
>

Thank you!
Taeung

Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view (v2)

[Arnaldo Carvalho de Melo]

Em Wed, Jan 20, 2016 at 09:34:51AM +0900, Namhyung Kim escreveu:
> On Tue, Jan 19, 2016 at 05:59:41PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Sun, Jan 17, 2016 at 01:03:00AM +0900, Namhyung Kim escreveu:
> > > Hello,
> > > 
> > > This is v2 attempt of my earlier patchset [1].  This patchset
> > > implements a new feature that collects hist entries in a hierachical
> > > manner.  That means lower-level entries belong to an upper-level
> > > entry.  The entry hierachy is built on the sort keys given, so users
> > > can set it whatever they want.  It only shows top-level entries first,
> > > and user can expand/collapse it dynamically.
> > > 
> > > This time I implemented it for every output browser including TUI.
> > > A screenshot on TUI looks like below:
> > > 
> > > For normal output:
> > > 
> > >   $ perf report --tui
> > >   Samples: 3K of event 'cycles:pp', Event count (approx.): 1695979674
> > >     Overhead  Command        Shared Object         Symbol
> > >   ------------------------------------------------------------------------
> > >   -    7.57%  swapper        [kernel.vmlinux]      [k] intel_idle
> > >        intel_idle
> > >        cpuidle_enter_state
> > >        cpuidle_enter
> > >        call_cpuidle
> > >      + cpu_startup_entry
> > >   +    1.16   firefox        firefox               [.] 0x00000000000019433
> > >   +    0.97%  firefox        libpthread-2.22.so    [.] pthread_mutex_lock
> > >   ...
> > > 
> > > 
> > > With hierarchy view,
> > 
> > Ok, tested, this is really nice, I think it should be the default, from
> > where to drill down, we could have a '--no-hierarchy', Ingo?
> 
> Yeah, we already have --no-hierarchy (as a side effect of having
> --hierarchy) but I don't want to change the default now since existing
> users will complain.  Now we have 'tips' in the perf report browser,
> maybe it's enough to add a line to suggest to use it (and it's already
> done by this patchset).  I remember the time we changed default for
> '--children' and many people complained about it.

:-) Ok, what about an extra patch that, when alt+H is pressed (we're
running out of appropriate hotkeys) we toggle to --hierarchy view, press
it again, back to --no-hierarchy?
 
> We maybe change the default later but I think it's better to have some
> time to people can play with it and find it useful. :)  And, as always,
> we can have a config option to control the default.
> 
> Btw, do you think it's worth adding a short option (-H) for
> --hierarchy (instead of making it default)?

I don't think this will remain for long as something we would want to
use in the command line, i.e. that we would have about half the number
of users wanting one hierarchy while the others wouldn't, for such cases
probably the best is to keep it as just a long option?

I.e. either the user uses --hierarchy and finds it so useful that he
will want to have it as his default or dislikes it and stops passing
that option in the command line, end result is that it will be a one
time use of such option, no need to have the one letter option used in
this case.

One thing I just noticed was that right after I ran:

 $ perf report --hie

The tool suggests that I use the brand new '--hierarchy' option :-)

How hard would it be to provide a way to disable some of the suggestions
when what is randomly suggested is already what the user is doing?

- Arnaldo

Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view (v2)

[Namhyung Kim]

Hi Arnaldo,

On Wed, Jan 20, 2016 at 10:32:04AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Jan 20, 2016 at 09:34:51AM +0900, Namhyung Kim escreveu:
> > On Tue, Jan 19, 2016 at 05:59:41PM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Sun, Jan 17, 2016 at 01:03:00AM +0900, Namhyung Kim escreveu:
> > > > Hello,
> > > > 
> > > > This is v2 attempt of my earlier patchset [1].  This patchset
> > > > implements a new feature that collects hist entries in a hierachical
> > > > manner.  That means lower-level entries belong to an upper-level
> > > > entry.  The entry hierachy is built on the sort keys given, so users
> > > > can set it whatever they want.  It only shows top-level entries first,
> > > > and user can expand/collapse it dynamically.
> > > > 
> > > > This time I implemented it for every output browser including TUI.
> > > > A screenshot on TUI looks like below:
> > > > 
> > > > For normal output:
> > > > 
> > > >   $ perf report --tui
> > > >   Samples: 3K of event 'cycles:pp', Event count (approx.): 1695979674
> > > >     Overhead  Command        Shared Object         Symbol
> > > >   ------------------------------------------------------------------------
> > > >   -    7.57%  swapper        [kernel.vmlinux]      [k] intel_idle
> > > >        intel_idle
> > > >        cpuidle_enter_state
> > > >        cpuidle_enter
> > > >        call_cpuidle
> > > >      + cpu_startup_entry
> > > >   +    1.16   firefox        firefox               [.] 0x00000000000019433
> > > >   +    0.97%  firefox        libpthread-2.22.so    [.] pthread_mutex_lock
> > > >   ...
> > > > 
> > > > 
> > > > With hierarchy view,
> > > 
> > > Ok, tested, this is really nice, I think it should be the default, from
> > > where to drill down, we could have a '--no-hierarchy', Ingo?
> > 
> > Yeah, we already have --no-hierarchy (as a side effect of having
> > --hierarchy) but I don't want to change the default now since existing
> > users will complain.  Now we have 'tips' in the perf report browser,
> > maybe it's enough to add a line to suggest to use it (and it's already
> > done by this patchset).  I remember the time we changed default for
> > '--children' and many people complained about it.
> 
> :-) Ok, what about an extra patch that, when alt+H is pressed (we're
> running out of appropriate hotkeys) we toggle to --hierarchy view, press
> it again, back to --no-hierarchy?

I guess it'd require non-trivial changes to the patchset and add some
amount of complexity.  I'll check whether it can be done easily later..


>  
> > We maybe change the default later but I think it's better to have some
> > time to people can play with it and find it useful. :)  And, as always,
> > we can have a config option to control the default.
> > 
> > Btw, do you think it's worth adding a short option (-H) for
> > --hierarchy (instead of making it default)?
> 
> I don't think this will remain for long as something we would want to
> use in the command line, i.e. that we would have about half the number
> of users wanting one hierarchy while the others wouldn't, for such cases
> probably the best is to keep it as just a long option?
> 
> I.e. either the user uses --hierarchy and finds it so useful that he
> will want to have it as his default or dislikes it and stops passing
> that option in the command line, end result is that it will be a one
> time use of such option, no need to have the one letter option used in
> this case.

Some people might find it useful sometimes (or only for specific
workloads), but don't want make it default.  If they want to use it
from time to time, maybe worth adding a short option.


> 
> One thing I just noticed was that right after I ran:
> 
>  $ perf report --hie
> 
> The tool suggests that I use the brand new '--hierarchy' option :-)
> 
> How hard would it be to provide a way to disable some of the suggestions
> when what is randomly suggested is already what the user is doing?

We could add callbacks for each tip to check it.

Thanks,
Namhyung

Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view (v2)

[Arnaldo Carvalho de Melo]

Em Thu, Jan 21, 2016 at 12:01:36AM +0900, Namhyung Kim escreveu:
> On Wed, Jan 20, 2016 at 10:32:04AM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Jan 20, 2016 at 09:34:51AM +0900, Namhyung Kim escreveu:
> > > On Tue, Jan 19, 2016 at 05:59:41PM -0300, Arnaldo Carvalho de Melo wrote:
> > One thing I just noticed was that right after I ran:

> >  $ perf report --hie

> > The tool suggests that I use the brand new '--hierarchy' option :-)

> > How hard would it be to provide a way to disable some of the suggestions
> > when what is randomly suggested is already what the user is doing?

> We could add callbacks for each tip to check it.

Right, I quickly thought about it and there are some issues, perhaps, in
addition to the existing "unconditional" tips, we could introduce some
way to register conditional tips, something like:

	tip__add_cond("For hierarchical output, try: perf report --hierarchy",
                      &symbol_conf, symbol_conf__is_hierarchy_on);

bool symbol_conf__is_hierarchy_on(const void *parm)
{
	const struct symbol_conf *conf = parm;
	return conf->report_hierarchy;
}

But then this needs to run after we parse options, so it has to be added
to both builtin-top.c and builtin-report.c and any other tool that needs
to have whatever state evaluated after options are parsed, for state
changed via option parsing.

Yeah, in this specific case we wouldn't need to pass a parameter, as
symbol_conf is global, but just to make the mechanism more general.

Sometimes you'll need to pass say, &report, to have access to state for
'struct report' to look at report.max_stack and check if the user set
--max-stack, etc.

Ah, I should have the hierarchy patches processed today, hopefully
posted to Ingo, thanks for splitting that patch as I asked, appreciated.

- Arnaldo

Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view (v2)

[Arnaldo Carvalho de Melo]

Em Wed, Jan 20, 2016 at 12:25:48PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Thu, Jan 21, 2016 at 12:01:36AM +0900, Namhyung Kim escreveu:
> > On Wed, Jan 20, 2016 at 10:32:04AM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Wed, Jan 20, 2016 at 09:34:51AM +0900, Namhyung Kim escreveu:
> > > > On Tue, Jan 19, 2016 at 05:59:41PM -0300, Arnaldo Carvalho de Melo wrote:
> > > One thing I just noticed was that right after I ran:
> 
> > >  $ perf report --hie
> 
> > > The tool suggests that I use the brand new '--hierarchy' option :-)
> 
> > > How hard would it be to provide a way to disable some of the suggestions
> > > when what is randomly suggested is already what the user is doing?
> 
> > We could add callbacks for each tip to check it.
> 
> Right, I quickly thought about it and there are some issues, perhaps, in
> addition to the existing "unconditional" tips, we could introduce some
> way to register conditional tips, something like:
> 
> 	tip__add_cond("For hierarchical output, try: perf report --hierarchy",
>                       &symbol_conf, symbol_conf__is_hierarchy_on);
> 
> bool symbol_conf__is_hierarchy_on(const void *parm)
> {
> 	const struct symbol_conf *conf = parm;
> 	return conf->report_hierarchy;
> }
> 
> But then this needs to run after we parse options, so it has to be added
> to both builtin-top.c and builtin-report.c and any other tool that needs
> to have whatever state evaluated after options are parsed, for state
> changed via option parsing.
> 
> Yeah, in this specific case we wouldn't need to pass a parameter, as
> symbol_conf is global, but just to make the mechanism more general.
> 
> Sometimes you'll need to pass say, &report, to have access to state for
> 'struct report' to look at report.max_stack and check if the user set
> --max-stack, etc.

Also say the user used some option, say '--call-graph dwarf', a tip
could notice that and state that one can control the amount of stack to
copy, or tell about some recent improvement about that specific option,
say a speedup, a fix, whatever.

- Arnaldo