[PATCH 1/3] perf ui/tui: Reset output width for hierarchy

[PATCH 1/3] perf ui/tui: Reset output width for hierarchy

[Namhyung Kim]

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

Before:

  - 46.29% 48.12%        netctl-auto
     + 31.44% 29.25%        [kernel.vmlinux]
     + 8.52% 11.55%        libc-2.22.so
     + 5.19% 6.91%        bash
  + 10.75% 11.83%        wpa_cli
  + 8.25% 2.23%        swapper
  + 6.45% 5.40%        tr
  + 4.81% 8.09%        awk
  + 4.15% 2.85%        firefox
  + 3.86% 2.53%        sh

After:

  -  46.29%  48.12%        netctl-auto
      +  31.44%  29.25%        [kernel.vmlinux]
      +   8.52%  11.55%        libc-2.22.so
      +   5.19%   6.91%        bash
  +  10.75%  11.83%        wpa_cli
  +   8.25%   2.23%        swapper
  +   6.45%   5.40%        tr
  +   4.81%   8.09%        awk
  +   4.15%   2.85%        firefox
  +   3.86%   2.53%        sh

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

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

[PATCH 2/3] perf hists: Factor out hists__reset_column_width()

[Namhyung Kim]

The stdio and tui has same code to reset hpp format column width.
Factor it out as a new function.

Suggested-by: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/browsers/hists.c | 12 +++---------
 tools/perf/ui/hist.c           | 15 +++++++++++++++
 tools/perf/ui/stdio/hist.c     | 10 +---------
 tools/perf/util/hist.h         |  1 +
 4 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 49db16334814..a6d5d248b8fb 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -2067,7 +2067,6 @@ void hist_browser__init(struct hist_browser *browser,
 			struct hists *hists)
 {
 	struct perf_hpp_fmt *fmt;
-	struct perf_hpp_list_node *node;
 
 	browser->hists			= hists;
 	browser->b.refresh		= hist_browser__refresh;
@@ -2076,15 +2075,10 @@ void hist_browser__init(struct hist_browser *browser,
 	browser->b.use_navkeypressed	= true;
 	browser->show_headers		= symbol_conf.show_hist_headers;
 
-	hists__for_each_format(hists, fmt) {
-		perf_hpp__reset_width(fmt, hists);
+	hists__for_each_format(hists, fmt)
 		++browser->b.columns;
-	}
-	/* hierarchy entries have their own hpp list */
-	list_for_each_entry(node, &hists->hpp_formats, list) {
-		perf_hpp_list__for_each_format(&node->hpp, fmt)
-			perf_hpp__reset_width(fmt, hists);
-	}
+
+	hists__reset_column_width(hists);
 }
 
 struct hist_browser *hist_browser__new(struct hists *hists)
diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index b47fafc8ee2a..60c4a4d08374 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -699,6 +699,21 @@ void perf_hpp__reset_width(struct perf_hpp_fmt *fmt, struct hists *hists)
 	}
 }
 
+void hists__reset_column_width(struct hists *hists)
+{
+	struct perf_hpp_fmt *fmt;
+	struct perf_hpp_list_node *node;
+
+	hists__for_each_format(hists, fmt)
+		perf_hpp__reset_width(fmt, hists);
+
+	/* hierarchy entries have their own hpp list */
+	list_for_each_entry(node, &hists->hpp_formats, list) {
+		perf_hpp_list__for_each_format(&node->hpp, fmt)
+			perf_hpp__reset_width(fmt, hists);
+	}
+}
+
 void perf_hpp__set_user_width(const char *width_list_str)
 {
 	struct perf_hpp_fmt *fmt;
diff --git a/tools/perf/ui/stdio/hist.c b/tools/perf/ui/stdio/hist.c
index a57131e61fe3..8e1840bff29d 100644
--- a/tools/perf/ui/stdio/hist.c
+++ b/tools/perf/ui/stdio/hist.c
@@ -717,8 +717,6 @@ size_t hists__fprintf(struct hists *hists, bool show_header, int max_rows,
 		      int max_cols, float min_pcnt, FILE *fp,
 		      bool use_callchain)
 {
-	struct perf_hpp_fmt *fmt;
-	struct perf_hpp_list_node *node;
 	struct rb_node *nd;
 	size_t ret = 0;
 	const char *sep = symbol_conf.field_sep;
@@ -729,13 +727,7 @@ size_t hists__fprintf(struct hists *hists, bool show_header, int max_rows,
 
 	init_rem_hits();
 
-	hists__for_each_format(hists, fmt)
-		perf_hpp__reset_width(fmt, hists);
-	/* hierarchy entries have their own hpp list */
-	list_for_each_entry(node, &hists->hpp_formats, list) {
-		perf_hpp_list__for_each_format(&node->hpp, fmt)
-			perf_hpp__reset_width(fmt, hists);
-	}
+	hists__reset_column_width(hists);
 
 	if (symbol_conf.col_width_list_str)
 		perf_hpp__set_user_width(symbol_conf.col_width_list_str);
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index a002c93fe422..defa957f27df 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -368,6 +368,7 @@ static inline bool perf_hpp__should_skip(struct perf_hpp_fmt *format,
 void perf_hpp__reset_width(struct perf_hpp_fmt *fmt, struct hists *hists);
 void perf_hpp__reset_sort_width(struct perf_hpp_fmt *fmt, struct hists *hists);
 void perf_hpp__set_user_width(const char *width_list_str);
+void hists__reset_column_width(struct hists *hists);
 
 typedef u64 (*hpp_field_fn)(struct hist_entry *he);
 typedef int (*hpp_callback_fn)(struct perf_hpp *hpp, bool front);
-- 
2.9.3

[PATCH 3/3] perf report: Fix output of 'pid' sort key

[Namhyung Kim]

The thread->comm can be changed during the lifetime due to prctl() or
exec().  For this reason each hist entry has a pointer to a comm at the
time as well as a pointer to the thread.  So it should use the he->comm
instead of thread__comm(he->thread) which always returns the latest
comm.  This can be seen using following example:

  $ perf report --hierarchy -s comm,pid

Before:
    3.86%   2.53%     sh
       1.05%   0.45%         776:sh
       0.67%   0.49%        8190:sh
       0.65%   0.37%        8194:sh
       0.28%   0.21%        8192:awk
       0.24%   0.22%        8191:acpi
       0.24%   0.21%        8196:awk
       0.22%   0.15%        8193:tr
       0.21%   0.14%        8195:netctl-auto
       0.10%   0.30%        8319:date
       0.10%   0.00%        8320:xsetroot
       0.09%   0.00%        8321:sleep

After:
    3.86%   2.53%     sh
       1.05%   0.45%         776:sh
       0.67%   0.49%        8190:sh
       0.65%   0.37%        8194:sh
       0.28%   0.21%        8192:sh
       0.24%   0.22%        8191:sh
       0.24%   0.21%        8196:sh
       0.22%   0.15%        8193:sh
       0.21%   0.14%        8195:sh
       0.10%   0.30%        8319:sh
       0.10%   0.00%        8320:sh
       0.09%   0.00%        8321:sh

Cc: Frederic Weisbecker <fweisbec@gmail.com>
Fixes: 4dfced359fbc ("perf tools: Get current comm instead of last one")
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/sort.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 1884d7f9b9d2..549dfbdbe9c3 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -77,7 +77,7 @@ sort__thread_cmp(struct hist_entry *left, struct hist_entry *right)
 static int hist_entry__thread_snprintf(struct hist_entry *he, char *bf,
 				       size_t size, unsigned int width)
 {
-	const char *comm = thread__comm_str(he->thread);
+	const char *comm = comm__str(he->comm);
 
 	width = max(7U, width) - 8;
 	return repsep_snprintf(bf, size, "%7d:%-*.*s", he->thread->tid,
-- 
2.9.3

Re: [PATCH 1/3] perf ui/tui: Reset output width for hierarchy

[Jiri Olsa]

On Tue, Sep 20, 2016 at 02:30:23PM +0900, Namhyung Kim wrote:
> When --hierarchy option is used, each entry has its own hpp_list to show
> the result.  But it missed to update width of each column.
> 
> Before:
> 
>   - 46.29% 48.12%        netctl-auto
>      + 31.44% 29.25%        [kernel.vmlinux]
>      + 8.52% 11.55%        libc-2.22.so
>      + 5.19% 6.91%        bash
>   + 10.75% 11.83%        wpa_cli
>   + 8.25% 2.23%        swapper
>   + 6.45% 5.40%        tr
>   + 4.81% 8.09%        awk
>   + 4.15% 2.85%        firefox
>   + 3.86% 2.53%        sh
> 
> After:
> 
>   -  46.29%  48.12%        netctl-auto
>       +  31.44%  29.25%        [kernel.vmlinux]
>       +   8.52%  11.55%        libc-2.22.so
>       +   5.19%   6.91%        bash
>   +  10.75%  11.83%        wpa_cli
>   +   8.25%   2.23%        swapper
>   +   6.45%   5.40%        tr
>   +   4.81%   8.09%        awk
>   +   4.15%   2.85%        firefox
>   +   3.86%   2.53%        sh
> 
> Fixes: 1b2dbbf41a0f ("perf hists: Use own hpp_list for hierarchy mode")
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

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

thanks,
jirka

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

Re: [PATCH 2/3] perf hists: Factor out hists__reset_column_width()

[Jiri Olsa]

On Tue, Sep 20, 2016 at 02:30:24PM +0900, Namhyung Kim wrote:
> The stdio and tui has same code to reset hpp format column width.
> Factor it out as a new function.
> 
> Suggested-by: Jiri Olsa <jolsa@redhat.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

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

thanks,
jirka

[tip:perf/core] perf ui/tui: Reset output width for hierarchy

[tip-bot for Namhyung Kim]

Commit-ID:  5ff3e7a224d40f9dd73625b91377787034a8b35e
Gitweb:     http://git.kernel.org/tip/5ff3e7a224d40f9dd73625b91377787034a8b35e
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Tue, 20 Sep 2016 14:30:23 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 20 Sep 2016 16:08:30 -0300

perf ui/tui: Reset output width for hierarchy

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

Before:

  - 46.29% 48.12%        netctl-auto
     + 31.44% 29.25%        [kernel.vmlinux]
     + 8.52% 11.55%        libc-2.22.so
     + 5.19% 6.91%        bash
  + 10.75% 11.83%        wpa_cli
  + 8.25% 2.23%        swapper
  + 6.45% 5.40%        tr
  + 4.81% 8.09%        awk
  + 4.15% 2.85%        firefox
  + 3.86% 2.53%        sh

After:

  -  46.29%  48.12%        netctl-auto
      +  31.44%  29.25%        [kernel.vmlinux]
      +   8.52%  11.55%        libc-2.22.so
      +   5.19%   6.91%        bash
  +  10.75%  11.83%        wpa_cli
  +   8.25%   2.23%        swapper
  +   6.45%   5.40%        tr
  +   4.81%   8.09%        awk
  +   4.15%   2.85%        firefox
  +   3.86%   2.53%        sh

Committer note:

Full testing instructions:

1) Record with an event group:

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

2) Use report in hierarchy mode, to get a few expanded trees on
   the same screen, use --percent-limit:

  $ perf report --hierarchy --percent-limit 0.5

Samples: 103K of event 'anon group { cycles:u, instructions:u }',
Event count (approx.): 57317631725
         Overhead        Command / Shared Object / Symbol        ◆
-  58.89%  55.12%        cc1                                     ▒
   -  50.26%  48.10%        cc1                                  ▒
          3.61%   5.13%        [.] _cpp_lex_token                ▒
          2.58%   0.78%        [.] ht_lookup_with_hash           ▒
          1.31%   1.30%        [.] ggc_internal_alloc            ▒
          1.08%   2.25%        [.] get_combined_adhoc_loc        ▒
          1.01%   1.95%        [.] ira_init                      ▒
          0.96%   1.78%        [.] linemap_position_for_column   ▒
          0.65%   1.01%        [.] cpp_get_token_with_location   ▒
   -   7.52%   6.58%        libc-2.23.so                         ▒
          1.70%   1.78%        [.] _int_malloc                   ▒
          0.69%   0.75%        [.] _int_free                     ▒
          0.67%   0.42%        [.] malloc_consolidate            ▒
   -   0.58%   0.42%        ld-2.23.so                           ▒
                               no entry >= 0.50%                 ▒
   -   0.52%   0.03%        [kernel.vmlinux]                     ▒
                               no entry >= 0.50%                 ▒

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Fixes: 1b2dbbf41a0f ("perf hists: Use own hpp_list for hierarchy mode")
Link: http://lkml.kernel.org/r/20160920053025.13989-1-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/ui/browsers/hists.c | 6 ++++++
 1 file changed, 6 insertions(+)

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

[tip:perf/core] perf hists: Factor out hists__reset_column_width()

[tip-bot for Namhyung Kim]

Commit-ID:  e3b60bc93d81e0542ac433df226b8de8b963533e
Gitweb:     http://git.kernel.org/tip/e3b60bc93d81e0542ac433df226b8de8b963533e
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Tue, 20 Sep 2016 14:30:24 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 20 Sep 2016 16:13:37 -0300

perf hists: Factor out hists__reset_column_width()

The stdio and tui has same code to reset hpp format column width.
Factor it out as a new function.

Suggested-and-Acked-by: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/20160920053025.13989-2-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/ui/browsers/hists.c | 12 +++---------
 tools/perf/ui/hist.c           | 15 +++++++++++++++
 tools/perf/ui/stdio/hist.c     | 10 +---------
 tools/perf/util/hist.h         |  1 +
 4 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 49db163..a6d5d24 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -2067,7 +2067,6 @@ void hist_browser__init(struct hist_browser *browser,
 			struct hists *hists)
 {
 	struct perf_hpp_fmt *fmt;
-	struct perf_hpp_list_node *node;
 
 	browser->hists			= hists;
 	browser->b.refresh		= hist_browser__refresh;
@@ -2076,15 +2075,10 @@ void hist_browser__init(struct hist_browser *browser,
 	browser->b.use_navkeypressed	= true;
 	browser->show_headers		= symbol_conf.show_hist_headers;
 
-	hists__for_each_format(hists, fmt) {
-		perf_hpp__reset_width(fmt, hists);
+	hists__for_each_format(hists, fmt)
 		++browser->b.columns;
-	}
-	/* hierarchy entries have their own hpp list */
-	list_for_each_entry(node, &hists->hpp_formats, list) {
-		perf_hpp_list__for_each_format(&node->hpp, fmt)
-			perf_hpp__reset_width(fmt, hists);
-	}
+
+	hists__reset_column_width(hists);
 }
 
 struct hist_browser *hist_browser__new(struct hists *hists)
diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index b47fafc..60c4a4d 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -699,6 +699,21 @@ void perf_hpp__reset_width(struct perf_hpp_fmt *fmt, struct hists *hists)
 	}
 }
 
+void hists__reset_column_width(struct hists *hists)
+{
+	struct perf_hpp_fmt *fmt;
+	struct perf_hpp_list_node *node;
+
+	hists__for_each_format(hists, fmt)
+		perf_hpp__reset_width(fmt, hists);
+
+	/* hierarchy entries have their own hpp list */
+	list_for_each_entry(node, &hists->hpp_formats, list) {
+		perf_hpp_list__for_each_format(&node->hpp, fmt)
+			perf_hpp__reset_width(fmt, hists);
+	}
+}
+
 void perf_hpp__set_user_width(const char *width_list_str)
 {
 	struct perf_hpp_fmt *fmt;
diff --git a/tools/perf/ui/stdio/hist.c b/tools/perf/ui/stdio/hist.c
index a57131e..8e1840b 100644
--- a/tools/perf/ui/stdio/hist.c
+++ b/tools/perf/ui/stdio/hist.c
@@ -717,8 +717,6 @@ size_t hists__fprintf(struct hists *hists, bool show_header, int max_rows,
 		      int max_cols, float min_pcnt, FILE *fp,
 		      bool use_callchain)
 {
-	struct perf_hpp_fmt *fmt;
-	struct perf_hpp_list_node *node;
 	struct rb_node *nd;
 	size_t ret = 0;
 	const char *sep = symbol_conf.field_sep;
@@ -729,13 +727,7 @@ size_t hists__fprintf(struct hists *hists, bool show_header, int max_rows,
 
 	init_rem_hits();
 
-	hists__for_each_format(hists, fmt)
-		perf_hpp__reset_width(fmt, hists);
-	/* hierarchy entries have their own hpp list */
-	list_for_each_entry(node, &hists->hpp_formats, list) {
-		perf_hpp_list__for_each_format(&node->hpp, fmt)
-			perf_hpp__reset_width(fmt, hists);
-	}
+	hists__reset_column_width(hists);
 
 	if (symbol_conf.col_width_list_str)
 		perf_hpp__set_user_width(symbol_conf.col_width_list_str);
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index a002c93..defa957 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -368,6 +368,7 @@ static inline bool perf_hpp__should_skip(struct perf_hpp_fmt *format,
 void perf_hpp__reset_width(struct perf_hpp_fmt *fmt, struct hists *hists);
 void perf_hpp__reset_sort_width(struct perf_hpp_fmt *fmt, struct hists *hists);
 void perf_hpp__set_user_width(const char *width_list_str);
+void hists__reset_column_width(struct hists *hists);
 
 typedef u64 (*hpp_field_fn)(struct hist_entry *he);
 typedef int (*hpp_callback_fn)(struct perf_hpp *hpp, bool front);