Re: [PATCH 5/5] perf report: Update column width of dynamic entries

[Arnaldo Carvalho de Melo]

Em Sat, Feb 27, 2016 at 03:52:47AM +0900, Namhyung Kim escreveu:
> The column width of dynamic entries is updated when comparing hist
> entries.  However some unique entries can miss the chance to update.  So
> move the update to output resort stage to make sure every entry will get
> called before display.
> 
> To do that, abuse ->sort callback to update the width when the third
> argument is NULL.  When resorting entries in normal path, it never be
> NULL so it should be fine IMHO.
> 
> Before:
> 
>   #       Overhead  ptr / bytes_req / gfp_flags
>   # ..............  ..........................................
>   #
>       37.50%        0xffff8803f7669400
>          37.50%        448
>             37.50%        GFP_ATOMIC|GFP_NOWARN|GFP_NOMEMALLOC
>       10.42%        0xffff8803f766be00
>           8.33%        96
>              8.33%        GFP_ATOMIC|GFP_NOWARN|GFP_NOMEMALLOC
>           2.08%        512
>              2.08%        GFP_KERNEL|GFP_NOWARN|GFP_REPEAT|GFP   <-- here

So, I tested this with:

 perf report ... > /tmp/before

apply the patch, and then do > /tmp/after, do the diff, reproduced what
you described, slightly different output:

[root@felicio ~]# diff -u /tmp/before /tmp/after | grep ^[+-]
--- /tmp/before	2016-02-26 19:38:31.321497576 -0300
+++ /tmp/after	2016-02-26 19:39:24.043614934 -0300
-# ..............  ....................................
+# ..............  ........................................
-           0.35%        GFP_ATOMIC|GFP_ZERO|GFP_NOMEMA
+           0.35%        GFP_ATOMIC|GFP_ZERO|GFP_NOMEMALLOC
-           0.35%        GFP_ATOMIC|GFP_ZERO|GFP_NOMEMA
+           0.35%        GFP_ATOMIC|GFP_ZERO|GFP_NOMEMALLOC
-           0.35%        GFP_ATOMIC|GFP_ZERO|GFP_NOMEMA
+           0.35%        GFP_ATOMIC|GFP_ZERO|GFP_NOMEMALLOC
-           0.35%        GFP_ATOMIC|GFP_ZERO|GFP_NOMEMA
+           0.35%        GFP_ATOMIC|GFP_ZERO|GFP_NOMEMALLOC
-           0.35%        GFP_ATOMIC|GFP_ZERO|GFP_NOMEMA
+           0.35%        GFP_ATOMIC|GFP_ZERO|GFP_NOMEMALLOC
-           0.35%        GFP_ATOMIC|GFP_ZERO|GFP_NOMEMA
+           0.35%        GFP_ATOMIC|GFP_ZERO|GFP_NOMEMALLOC
-           0.35%        GFP_ATOMIC|GFP_ZERO|GFP_NOMEMA
+           0.35%        GFP_ATOMIC|GFP_ZERO|GFP_NOMEMALLOC
[root@felicio ~]# diff -u /tmp/before /tmp/after | grep ^[+-]

This is something else to add to the regression tests, output diff for
as many command line combos outputs as we can...

Anyway, good work, all applied with Jiri's acks.

- Arnaldo

> 
> After:
> 
>   #       Overhead  ptr / bytes_req / gfp_flags
>   # ..............  .....................................................
>   #
>       37.50%        0xffff8803f7669400
>          37.50%        448
>             37.50%        GFP_ATOMIC|GFP_NOWARN|GFP_NOMEMALLOC
>       10.42%        0xffff8803f766be00
>           8.33%        96
>              8.33%        GFP_ATOMIC|GFP_NOWARN|GFP_NOMEMALLOC
>           2.08%        512
>              2.08%        GFP_KERNEL|GFP_NOWARN|GFP_REPEAT|GFP_NOMEMALLOC
> 
> Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
> ---
>  tools/perf/util/hist.c | 11 +++++++++++
>  tools/perf/util/sort.c |  8 +++++---
>  2 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index 9b3f582867d6..4b8b67bc0cd8 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -1371,6 +1371,10 @@ static void hierarchy_insert_output_entry(struct rb_root *root,
>  
>  	rb_link_node(&he->rb_node, parent, p);
>  	rb_insert_color(&he->rb_node, root);
> +
> +	/* update column width of dynamic entry */
> +	if (perf_hpp__is_dynamic_entry(he->fmt))
> +		he->fmt->sort(he->fmt, he, NULL);
>  }
>  
>  static void hists__hierarchy_output_resort(struct hists *hists,
> @@ -1440,6 +1444,7 @@ static void __hists__insert_output_entry(struct rb_root *entries,
>  	struct rb_node **p = &entries->rb_node;
>  	struct rb_node *parent = NULL;
>  	struct hist_entry *iter;
> +	struct perf_hpp_fmt *fmt;
>  
>  	if (use_callchain) {
>  		if (callchain_param.mode == CHAIN_GRAPH_REL) {
> @@ -1466,6 +1471,12 @@ static void __hists__insert_output_entry(struct rb_root *entries,
>  
>  	rb_link_node(&he->rb_node, parent, p);
>  	rb_insert_color(&he->rb_node, entries);
> +
> +	perf_hpp_list__for_each_sort_list(&perf_hpp_list, fmt) {
> +		if (perf_hpp__is_dynamic_entry(fmt) &&
> +		    perf_hpp__defined_dynamic_entry(fmt, he->hists))
> +			fmt->sort(fmt, he, NULL);  /* update column width */
> +	}
>  }
>  
>  static void output_resort(struct hists *hists, struct ui_progress *prog,
> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index 3b1b4018f111..1457c9d975a0 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
> @@ -1819,6 +1819,11 @@ static int64_t __sort__hde_cmp(struct perf_hpp_fmt *fmt,
>  
>  	hde = container_of(fmt, struct hpp_dynamic_entry, hpp);
>  
> +	if (b == NULL) {
> +		update_dynamic_len(hde, a);
> +		return 0;
> +	}
> +
>  	field = hde->field;
>  	if (field->flags & FIELD_IS_DYNAMIC) {
>  		unsigned long long dyn;
> @@ -1833,9 +1838,6 @@ static int64_t __sort__hde_cmp(struct perf_hpp_fmt *fmt,
>  	} else {
>  		offset = field->offset;
>  		size = field->size;
> -
> -		update_dynamic_len(hde, a);
> -		update_dynamic_len(hde, b);
>  	}
>  
>  	return memcmp(a->raw_data + offset, b->raw_data + offset, size);
> -- 
> 2.7.1