[PATCH 0/4] More color in 'perf diff'

[PATCH 0/4] More color in 'perf diff'

[Ramkumar Ramachandra]

Hi,

I thought it would be nice to color the Delta column as a short
evening exercise. While at it, I thought it would be nice to color the
Ratio column (see [4/4]) as well. It's trivial to write a patch to
color the Weighted Diff column now, but I haven't done so because of
the problem in [4/4].

Thanks.

Ramkumar Ramachandra (4):
  perf diff: don't compute Delta if he->dummy
  perf diff: give Delta column an appropriate color
  perf diff: generalize hpp__color_delta for -c
  perf diff: give Ratio column an appropriate color

 tools/perf/builtin-diff.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 85 insertions(+), 1 deletion(-)

-- 
1.8.5.rc0.5.g70ebc73.dirty

[PATCH 1/4] perf diff: don't compute Delta if he->dummy

[Ramkumar Ramachandra]

Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 tools/perf/builtin-diff.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index 3b67ea2..79e0448 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -792,6 +792,9 @@ hpp__entry_pair(struct hist_entry *he, struct hist_entry *pair,
 
 	switch (idx) {
 	case PERF_HPP_DIFF__DELTA:
+		if (he->dummy)
+			break;
+
 		if (pair->diff.computed)
 			diff = pair->diff.period_ratio_delta;
 		else
-- 
1.8.5.rc0.5.g70ebc73.dirty

[PATCH 2/4] perf diff: give Delta column an appropriate color

[Ramkumar Ramachandra]

Colorize the numbers in the Delta column either green or red depending
on whether the it is positive or negative.

Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 tools/perf/builtin-diff.c | 37 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index 79e0448..692c155 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -769,6 +769,33 @@ static int hpp__entry_baseline(struct hist_entry *he, char *buf, size_t size)
 	return ret;
 }
 
+static int hpp__color_delta(struct perf_hpp_fmt *fmt,
+			struct perf_hpp *hpp, struct hist_entry *he)
+{
+	struct diff_hpp_fmt *dfmt =
+		container_of(fmt, struct diff_hpp_fmt, fmt);
+	struct hist_entry *pair = get_pair_fmt(he, dfmt);
+	double percent;
+	char pfmt[20] = " ";
+
+	if (!pair)
+		goto dummy_print;
+	if (pair->diff.computed)
+		percent = pair->diff.period_ratio_delta;
+	else
+		percent = compute_delta(he, pair);
+
+	if (!he->dummy) {
+		scnprintf(pfmt, 20, "%%%+d.2f%%%%", dfmt->header_width - 1);
+		return color_snprintf(hpp->buf, hpp->size,
+				percent > 0 ? PERF_COLOR_GREEN : PERF_COLOR_RED,
+				pfmt, percent);
+	} else
+dummy_print:
+		return scnprintf(hpp->buf, hpp->size, "%*s",
+				dfmt->header_width, pfmt);
+}
+
 static void
 hpp__entry_unpair(struct hist_entry *he, int idx, char *buf, size_t size)
 {
@@ -943,8 +970,16 @@ static void data__hpp_register(struct data__file *d, int idx)
 	fmt->entry  = hpp__entry_global;
 
 	/* TODO more colors */
-	if (idx == PERF_HPP_DIFF__BASELINE)
+	switch (idx) {
+	case PERF_HPP_DIFF__BASELINE:
 		fmt->color = hpp__color_baseline;
+		break;
+	case PERF_HPP_DIFF__DELTA:
+		fmt->color = hpp__color_delta;
+		break;
+	default:
+		break;
+	}
 
 	init_header(d, dfmt);
 	perf_hpp__column_register(fmt);
-- 
1.8.5.rc0.5.g70ebc73.dirty

Re: [PATCH 2/4] perf diff: give Delta column an appropriate color

[Jiri Olsa]

On Thu, Nov 21, 2013 at 08:03:45PM +0530, Ramkumar Ramachandra wrote:
> Colorize the numbers in the Delta column either green or red depending
> on whether the it is positive or negative.
> 

this patch changes the diff output for 'perf diff'

file '1' - old perf
file '2' - your change

I expected only the difference would be only color
not numbers.

thanks,
jirka

---
--- 1   2013-11-22 16:05:36.224136167 +0100
+++ 2   2013-11-22 16:05:40.399152889 +0100
@@ -9,128 +9,128 @@
     11.78%   -0.14%  libc-2.17.so       [.] fputs_unlocked
      9.77%   +0.02%  libc-2.17.so       [.] __GI___mempcpy
      1.61%   +0.21%  yes                [.] fputs_unlocked@plt
-     0.08%           [kernel.kallsyms]  [k] system_call                           
-     0.07%           [kernel.kallsyms]  [k] __srcu_read_lock                      
-     0.06%           [kernel.kallsyms]  [k] __srcu_read_unlock                    
+     0.08%   -0.01%  [kernel.kallsyms]  [k] system_call                           
+     0.07%   -0.01%  [kernel.kallsyms]  [k] __srcu_read_lock                      
+     0.06%   -0.01%  [kernel.kallsyms]  [k] __srcu_read_unlock                    
      0.05%   -0.03%  [kernel.kallsyms]  [k] native_write_msr_safe
      0.04%   -0.03%  [kernel.kallsyms]  [k] apic_timer_interrupt
-     0.04%           libc-2.17.so       [.] __GI___libc_write                     
-     0.04%           [kernel.kallsyms]  [k] __audit_syscall_exit                  
-     0.04%           [kernel.kallsyms]  [k] vfs_write                             
-     0.03%           [kernel.kallsyms]  [k] fget_light                            
-     0.03%           [kernel.kallsyms]  [k] __audit_syscall_entry                 
-     0.03%           [kernel.kallsyms]  [k] fsnotify                              
-     0.02%           libc-2.17.so       [.] _IO_do_write@@GLIBC_2.2.5             
+     0.04%   +0.00%  libc-2.17.so       [.] __GI___libc_write                     
+     0.04%   +0.01%  [kernel.kallsyms]  [k] __audit_syscall_exit                  
+     0.04%   +0.00%  [kernel.kallsyms]  [k] vfs_write                             
+     0.03%   -0.01%  [kernel.kallsyms]  [k] fget_light                            
+     0.03%   -0.00%  [kernel.kallsyms]  [k] __audit_syscall_entry                 
+     0.03%   -0.00%  [kernel.kallsyms]  [k] fsnotify                              
+     0.02%   -0.00%  libc-2.17.so       [.] _IO_do_write@@GLIBC_2.2.5             
      0.02%   +0.01%  libc-2.17.so       [.] _IO_file_write@@GLIBC_2.2.5
      0.02%   -0.01%  [kernel.kallsyms]  [k] task_tick_fair
-     0.02%           libc-2.17.so       [.] _IO_default_xsputn                    
+     0.02%   -0.00%  libc-2.17.so       [.] _IO_default_xsputn                    
      0.02%   -0.01%  [kernel.kallsyms]  [k] native_read_tsc
      0.02%   -0.01%  [kernel.kallsyms]  [k] _raw_spin_lock
-     0.01%           libc-2.17.so       [.] _IO_file_overflow@@GLIBC_2.2.5        
-     0.01%           [kernel.kallsyms]  [k] do_timer                              
+     0.01%   -0.00%  libc-2.17.so       [.] _IO_file_overflow@@GLIBC_2.2.5        
+     0.01%   -0.01%  [kernel.kallsyms]  [k] do_timer                              
      0.01%   -0.01%  [kernel.kallsyms]  [k] update_cfs_rq_blocked_load
-     0.01%           [kernel.kallsyms]  [k] sys_write                             
-     0.01%           [kernel.kallsyms]  [k] irqtime_account_irq                   
-     0.01%           [kernel.kallsyms]  [k] __fsnotify_parent                     
-     0.01%           [kernel.kallsyms]  [k] irqtime_account_process_tick.isra.2   
-     0.01%           [kernel.kallsyms]  [k] native_read_msr_safe                  
-     0.01%           [kernel.kallsyms]  [k] rw_verify_area                        
-     0.01%           [kernel.kallsyms]  [k] sched_clock_cpu                       
-     0.01%           [kernel.kallsyms]  [k] rcu_check_callbacks                   
-     0.01%           [kernel.kallsyms]  [k] run_timer_softirq                     
-     0.01%           [kernel.kallsyms]  [k] path_put                              
-     0.01%           [kernel.kallsyms]  [k] unroll_tree_refs                      
-     0.01%           [kernel.kallsyms]  [k] hrtimer_interrupt                     
-     0.01%           [kernel.kallsyms]  [k] security_file_permission              
-     0.01%           [kernel.kallsyms]  [k] kfree                                 
-     0.01%           [kernel.kallsyms]  [k] native_sched_clock                    
-     0.01%           [kernel.kallsyms]  [k] _raw_spin_lock_irqsave                
-     0.01%           [kernel.kallsyms]  [k] trigger_load_balance                  
-     0.01%           [kernel.kallsyms]  [k] update_cfs_shares                     
-     0.01%           [kernel.kallsyms]  [k] __acct_update_integrals               
-     0.01%           [kernel.kallsyms]  [k] __update_cpu_load                     
-     0.01%           [kernel.kallsyms]  [k] __do_softirq                          
+     0.01%   +0.01%  [kernel.kallsyms]  [k] sys_write                             
+     0.01%   -0.00%  [kernel.kallsyms]  [k] irqtime_account_irq                   
+     0.01%   -0.00%  [kernel.kallsyms]  [k] __fsnotify_parent                     
+     0.01%   -0.01%  [kernel.kallsyms]  [k] irqtime_account_process_tick.isra.2   
+     0.01%   +0.00%  [kernel.kallsyms]  [k] native_read_msr_safe                  
+     0.01%   +0.00%  [kernel.kallsyms]  [k] rw_verify_area                        
+     0.01%   -0.01%  [kernel.kallsyms]  [k] sched_clock_cpu                       
+     0.01%   -0.01%  [kernel.kallsyms]  [k] rcu_check_callbacks                   
+     0.01%   -0.01%  [kernel.kallsyms]  [k] run_timer_softirq                     
+     0.01%   -0.00%  [kernel.kallsyms]  [k] path_put                              
+     0.01%   -0.00%  [kernel.kallsyms]  [k] unroll_tree_refs                      
+     0.01%   -0.01%  [kernel.kallsyms]  [k] hrtimer_interrupt                     
+     0.01%   +0.00%  [kernel.kallsyms]  [k] security_file_permission              
+     0.01%   +0.00%  [kernel.kallsyms]  [k] kfree                                 
+     0.01%   -0.00%  [kernel.kallsyms]  [k] native_sched_clock                    
+     0.01%   -0.00%  [kernel.kallsyms]  [k] _raw_spin_lock_irqsave                
+     0.01%   -0.00%  [kernel.kallsyms]  [k] trigger_load_balance                  
+     0.01%   -0.00%  [kernel.kallsyms]  [k] update_cfs_shares                     
+     0.01%   -0.00%  [kernel.kallsyms]  [k] __acct_update_integrals               

Re: [PATCH 2/4] perf diff: give Delta column an appropriate color

[Ramkumar Ramachandra]

Jiri Olsa wrote:
> this patch changes the diff output for 'perf diff'
>
> file '1' - old perf
> file '2' - your change
>
> I expected only the difference would be only color
> not numbers.

Oh, sorry; I forgot about the threshold completely. I'll submit
another iteration with a fresh [4/4] once you tell me how to fix that.

[PATCH 3/4] perf diff: generalize hpp__color_delta for -c

[Ramkumar Ramachandra]

Generalize the function so that we can accommodate all three comparison
methods: delta, ratio, and wdiff.

Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 tools/perf/builtin-diff.c | 43 ++++++++++++++++++++++++++++++++++---------
 1 file changed, 34 insertions(+), 9 deletions(-)

diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index 692c155..4477aa2 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -769,33 +769,58 @@ static int hpp__entry_baseline(struct hist_entry *he, char *buf, size_t size)
 	return ret;
 }
 
-static int hpp__color_delta(struct perf_hpp_fmt *fmt,
-			struct perf_hpp *hpp, struct hist_entry *he)
+static int __hpp__color_compare(struct perf_hpp_fmt *fmt,
+				struct perf_hpp *hpp, struct hist_entry *he,
+				int comparison_method)
 {
 	struct diff_hpp_fmt *dfmt =
 		container_of(fmt, struct diff_hpp_fmt, fmt);
 	struct hist_entry *pair = get_pair_fmt(he, dfmt);
-	double percent;
+	double delta;
 	char pfmt[20] = " ";
 
 	if (!pair)
 		goto dummy_print;
 	if (pair->diff.computed)
-		percent = pair->diff.period_ratio_delta;
+		switch (comparison_method) {
+		case COMPUTE_DELTA:
+			delta = pair->diff.period_ratio_delta;
+			break;
+		default:
+			BUG_ON(1);
+		}
 	else
-		percent = compute_delta(he, pair);
+		switch (comparison_method) {
+		case COMPUTE_DELTA:
+			delta = compute_delta(he, pair);
+			break;
+		default:
+			BUG_ON(1);
+		}
 
 	if (!he->dummy) {
-		scnprintf(pfmt, 20, "%%%+d.2f%%%%", dfmt->header_width - 1);
-		return color_snprintf(hpp->buf, hpp->size,
-				percent > 0 ? PERF_COLOR_GREEN : PERF_COLOR_RED,
-				pfmt, percent);
+		switch (comparison_method) {
+		case COMPUTE_DELTA:
+			scnprintf(pfmt, 20, "%%%+d.2f%%%%", dfmt->header_width - 1);
+			return color_snprintf(hpp->buf, hpp->size,
+					delta > 0 ? PERF_COLOR_GREEN : PERF_COLOR_RED,
+					pfmt, delta);
+			break;
+		default:
+			BUG_ON(1);
+		}
 	} else
 dummy_print:
 		return scnprintf(hpp->buf, hpp->size, "%*s",
 				dfmt->header_width, pfmt);
 }
 
+static int hpp__color_delta(struct perf_hpp_fmt *fmt,
+			struct perf_hpp *hpp, struct hist_entry *he)
+{
+	return __hpp__color_compare(fmt, hpp, he, COMPUTE_DELTA);
+}
+
 static void
 hpp__entry_unpair(struct hist_entry *he, int idx, char *buf, size_t size)
 {
-- 
1.8.5.rc0.5.g70ebc73.dirty

[INCOMPLETE PATCH 4/4] perf diff: give Ratio column an appropriate color

[Ramkumar Ramachandra]

In

  $ perf diff -c ratio

color the Ratio column using percent_color_snprintf().

Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 The incomplete bit is marked using a TODO; removing two trailing '%'
 symbols doesn't seem to work. Can someone please tell me how to fix
 this problem?

 Thanks.

 tools/perf/builtin-diff.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index 4477aa2..844841e 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -786,6 +786,9 @@ static int __hpp__color_compare(struct perf_hpp_fmt *fmt,
 		case COMPUTE_DELTA:
 			delta = pair->diff.period_ratio_delta;
 			break;
+		case COMPUTE_RATIO:
+			delta = pair->diff.period_ratio;
+			break;
 		default:
 			BUG_ON(1);
 		}
@@ -794,6 +797,9 @@ static int __hpp__color_compare(struct perf_hpp_fmt *fmt,
 		case COMPUTE_DELTA:
 			delta = compute_delta(he, pair);
 			break;
+		case COMPUTE_RATIO:
+			delta = compute_ratio(he, pair);
+			break;
 		default:
 			BUG_ON(1);
 		}
@@ -806,6 +812,12 @@ static int __hpp__color_compare(struct perf_hpp_fmt *fmt,
 					delta > 0 ? PERF_COLOR_GREEN : PERF_COLOR_RED,
 					pfmt, delta);
 			break;
+		case COMPUTE_RATIO:
+			/* TODO: strip trailing % without breaking color */
+			scnprintf(pfmt, 20, "%%%d.6f%%%%", dfmt->header_width - 1);
+			return percent_color_snprintf(hpp->buf, hpp->size,
+						pfmt, delta);
+			break;
 		default:
 			BUG_ON(1);
 		}
@@ -821,6 +833,12 @@ static int hpp__color_delta(struct perf_hpp_fmt *fmt,
 	return __hpp__color_compare(fmt, hpp, he, COMPUTE_DELTA);
 }
 
+static int hpp__color_ratio(struct perf_hpp_fmt *fmt,
+			struct perf_hpp *hpp, struct hist_entry *he)
+{
+	return __hpp__color_compare(fmt, hpp, he, COMPUTE_RATIO);
+}
+
 static void
 hpp__entry_unpair(struct hist_entry *he, int idx, char *buf, size_t size)
 {
@@ -1002,6 +1020,9 @@ static void data__hpp_register(struct data__file *d, int idx)
 	case PERF_HPP_DIFF__DELTA:
 		fmt->color = hpp__color_delta;
 		break;
+	case PERF_HPP_DIFF__RATIO:
+		fmt->color = hpp__color_ratio;
+		break;
 	default:
 		break;
 	}
-- 
1.8.5.rc0.5.g70ebc73.dirty

Re: [INCOMPLETE PATCH 4/4] perf diff: give Ratio column an appropriate color

[Jiri Olsa]

On Thu, Nov 21, 2013 at 08:03:47PM +0530, Ramkumar Ramachandra wrote:
> In
> 
>   $ perf diff -c ratio
> 
> color the Ratio column using percent_color_snprintf().

why is there 'INCOMPLETE' in the subject?

jirka

Re: [INCOMPLETE PATCH 4/4] perf diff: give Ratio column an appropriate color

[Ramkumar Ramachandra]

Jiri Olsa wrote:
> On Thu, Nov 21, 2013 at 08:03:47PM +0530, Ramkumar Ramachandra wrote:
>> In
>>
>>   $ perf diff -c ratio
>>
>> color the Ratio column using percent_color_snprintf().
>
> why is there 'INCOMPLETE' in the subject?

As explained in the segment below the diffstat:

The incomplete bit is marked using a TODO; removing two trailing '%'
symbols doesn't seem to work. Can someone please tell me how to fix
this problem?

Re: [INCOMPLETE PATCH 4/4] perf diff: give Ratio column an appropriate color

[Jiri Olsa]

On Thu, Nov 21, 2013 at 08:03:47PM +0530, Ramkumar Ramachandra wrote:
> In
> 
>   $ perf diff -c ratio
> 
> color the Ratio column using percent_color_snprintf().
> 
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  The incomplete bit is marked using a TODO; removing two trailing '%'
>  symbols doesn't seem to work. Can someone please tell me how to fix
>  this problem?

aah here it is..

jirka

> 
>  Thanks.
> 
>  tools/perf/builtin-diff.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
> index 4477aa2..844841e 100644
> --- a/tools/perf/builtin-diff.c
> +++ b/tools/perf/builtin-diff.c
> @@ -786,6 +786,9 @@ static int __hpp__color_compare(struct perf_hpp_fmt *fmt,
>  		case COMPUTE_DELTA:
>  			delta = pair->diff.period_ratio_delta;
>  			break;
> +		case COMPUTE_RATIO:
> +			delta = pair->diff.period_ratio;
> +			break;
>  		default:
>  			BUG_ON(1);
>  		}
> @@ -794,6 +797,9 @@ static int __hpp__color_compare(struct perf_hpp_fmt *fmt,
>  		case COMPUTE_DELTA:
>  			delta = compute_delta(he, pair);
>  			break;
> +		case COMPUTE_RATIO:
> +			delta = compute_ratio(he, pair);
> +			break;
>  		default:
>  			BUG_ON(1);
>  		}
> @@ -806,6 +812,12 @@ static int __hpp__color_compare(struct perf_hpp_fmt *fmt,
>  					delta > 0 ? PERF_COLOR_GREEN : PERF_COLOR_RED,
>  					pfmt, delta);
>  			break;
> +		case COMPUTE_RATIO:
> +			/* TODO: strip trailing % without breaking color */
> +			scnprintf(pfmt, 20, "%%%d.6f%%%%", dfmt->header_width - 1);
> +			return percent_color_snprintf(hpp->buf, hpp->size,
> +						pfmt, delta);
> +			break;
>  		default:
>  			BUG_ON(1);
>  		}
> @@ -821,6 +833,12 @@ static int hpp__color_delta(struct perf_hpp_fmt *fmt,
>  	return __hpp__color_compare(fmt, hpp, he, COMPUTE_DELTA);
>  }
>  
> +static int hpp__color_ratio(struct perf_hpp_fmt *fmt,
> +			struct perf_hpp *hpp, struct hist_entry *he)
> +{
> +	return __hpp__color_compare(fmt, hpp, he, COMPUTE_RATIO);
> +}
> +
>  static void
>  hpp__entry_unpair(struct hist_entry *he, int idx, char *buf, size_t size)
>  {
> @@ -1002,6 +1020,9 @@ static void data__hpp_register(struct data__file *d, int idx)
>  	case PERF_HPP_DIFF__DELTA:
>  		fmt->color = hpp__color_delta;
>  		break;
> +	case PERF_HPP_DIFF__RATIO:
> +		fmt->color = hpp__color_ratio;
> +		break;
>  	default:
>  		break;
>  	}
> -- 
> 1.8.5.rc0.5.g70ebc73.dirty
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [INCOMPLETE PATCH 4/4] perf diff: give Ratio column an appropriate color

[Ramkumar Ramachandra]

Jiri Olsa wrote:
>>  The incomplete bit is marked using a TODO; removing two trailing '%'
>>  symbols doesn't seem to work. Can someone please tell me how to fix
>>  this problem?
>
> aah here it is..

I fixed it and sent another iteration.