[PATCH v5 0/3] More color in 'perf diff'

[PATCH v5 0/3] More color in 'perf diff'

[Ramkumar Ramachandra]

Hi,

The changes in this iteration are:

[1/3]: Use fabs() in get_percent_color() instead of
       __hpp_color_compare(), as suggested by Jiri.

[2/3]: Use value_color_snprintf(), a new function functionally
       equivalent to percent_color_snprintf().

[3/3]: Unchanged.

Thanks.

Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>

Ramkumar Ramachandra (3):
  perf diff: color the Delta column
  perf diff: color the Ratio column
  perf diff: color the Weighted Diff column

 tools/perf/builtin-diff.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++-
 tools/perf/util/color.c   | 16 ++++++---
 tools/perf/util/color.h   |  1 +
 3 files changed, 102 insertions(+), 6 deletions(-)

-- 
1.8.5.rc0.5.g70ebc73.dirty

[PATCH v5 1/3] perf diff: color the Delta column

[Ramkumar Ramachandra]

Color the numbers in the Delta column using percent_color_snprintf(),
making sure that it can handle negative values first. Generalize the
coloring function so that we can accommodate all three comparison
methods in future patches: 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 | 49 ++++++++++++++++++++++++++++++++++++++++++++++-
 tools/perf/util/color.c   |  6 ++++--
 2 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index 3b67ea2..a8fc525 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -769,6 +769,45 @@ static int hpp__entry_baseline(struct hist_entry *he, char *buf, size_t size)
 	return ret;
 }
 
+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 diff;
+	char pfmt[20] = " ";
+
+	if (!pair)
+		goto dummy_print;
+
+	switch (comparison_method) {
+	case COMPUTE_DELTA:
+		if (pair->diff.computed)
+			diff = pair->diff.period_ratio_delta;
+		else
+			diff = compute_delta(he, pair);
+
+		if (fabs(diff) < 0.01)
+			goto dummy_print;
+		scnprintf(pfmt, 20, "%%%+d.2f%%%%", dfmt->header_width - 1);
+		return percent_color_snprintf(hpp->buf, hpp->size,
+					pfmt, diff);
+	default:
+		BUG_ON(1);
+	}
+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)
 {
@@ -940,8 +979,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);
diff --git a/tools/perf/util/color.c b/tools/perf/util/color.c
index 66e44a5..ec5c756 100644
--- a/tools/perf/util/color.c
+++ b/tools/perf/util/color.c
@@ -2,6 +2,8 @@
 #include "cache.h"
 #include "color.h"
 
+#include <math.h>
+
 int perf_use_color_default = -1;
 
 static int parse_color(const char *name, int len)
@@ -298,10 +300,10 @@ const char *get_percent_color(double percent)
 	 * entries in green - and keep the low overhead places
 	 * normal:
 	 */
-	if (percent >= MIN_RED)
+	if (fabs(percent) >= MIN_RED)
 		color = PERF_COLOR_RED;
 	else {
-		if (percent > MIN_GREEN)
+		if (fabs(percent) > MIN_GREEN)
 			color = PERF_COLOR_GREEN;
 	}
 	return color;
-- 
1.8.5.rc0.5.g70ebc73.dirty

Re: [PATCH v5 1/3] perf diff: color the Delta column

[Jiri Olsa]

On Tue, Dec 10, 2013 at 06:19:00PM +0530, Ramkumar Ramachandra wrote:

SNIP

> +	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)
>  {
> @@ -940,8 +979,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;
> +	}

please put changes below into separated patch

>  
>  	init_header(d, dfmt);
>  	perf_hpp__column_register(fmt);
> diff --git a/tools/perf/util/color.c b/tools/perf/util/color.c
> index 66e44a5..ec5c756 100644
> --- a/tools/perf/util/color.c
> +++ b/tools/perf/util/color.c
> @@ -2,6 +2,8 @@
>  #include "cache.h"
>  #include "color.h"
>  
> +#include <math.h>
> +

nit: extra empty line

>  int perf_use_color_default = -1;
>  
>  static int parse_color(const char *name, int len)
> @@ -298,10 +300,10 @@ const char *get_percent_color(double percent)
>  	 * entries in green - and keep the low overhead places
>  	 * normal:
>  	 */
> -	if (percent >= MIN_RED)
> +	if (fabs(percent) >= MIN_RED)
>  		color = PERF_COLOR_RED;
>  	else {
> -		if (percent > MIN_GREEN)
> +		if (fabs(percent) > MIN_GREEN)
>  			color = PERF_COLOR_GREEN;
>  	}
>  	return color;
> -- 
> 1.8.5.rc0.5.g70ebc73.dirty
> 

[PATCH v5 2/3] perf diff: color the Ratio column

[Ramkumar Ramachandra]

In

  $ perf diff -c ratio

color the Ratio column using value_color_snprintf(), a new function that
operates exactly like percent_color_snprintf(). At first glance, it
looks like percent_color_snprintf() can be turned into a non-variadic
function simplifying things; however, 53805ec (perf tools: Remove cast
of non-variadic function to variadic, 2013-10-31) explains why it needs
to be a variadic function.

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 | 20 ++++++++++++++++++++
 tools/perf/util/color.c   | 10 +++++++---
 tools/perf/util/color.h   |  1 +
 3 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index a8fc525..c07cd3c 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -794,6 +794,17 @@ static int __hpp__color_compare(struct perf_hpp_fmt *fmt,
 		scnprintf(pfmt, 20, "%%%+d.2f%%%%", dfmt->header_width - 1);
 		return percent_color_snprintf(hpp->buf, hpp->size,
 					pfmt, diff);
+	case COMPUTE_RATIO:
+		if (he->dummy)
+			goto dummy_print;
+		if (pair->diff.computed)
+			diff = pair->diff.period_ratio;
+		else
+			diff = compute_ratio(he, pair);
+
+		scnprintf(pfmt, 20, "%%%d.6f", dfmt->header_width);
+		return value_color_snprintf(hpp->buf, hpp->size,
+					pfmt, diff);
 	default:
 		BUG_ON(1);
 	}
@@ -808,6 +819,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)
 {
@@ -986,6 +1003,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;
 	}
diff --git a/tools/perf/util/color.c b/tools/perf/util/color.c
index ec5c756..0c0b552 100644
--- a/tools/perf/util/color.c
+++ b/tools/perf/util/color.c
@@ -320,15 +320,19 @@ int percent_color_fprintf(FILE *fp, const char *fmt, double percent)
 	return r;
 }
 
+int value_color_snprintf(char *bf, size_t size, const char *fmt, double value)
+{
+	const char *color = get_percent_color(value);
+	return color_snprintf(bf, size, color, fmt, value);
+}
+
 int percent_color_snprintf(char *bf, size_t size, const char *fmt, ...)
 {
 	va_list args;
 	double percent;
-	const char *color;
 
 	va_start(args, fmt);
 	percent = va_arg(args, double);
 	va_end(args);
-	color = get_percent_color(percent);
-	return color_snprintf(bf, size, color, fmt, percent);
+	return value_color_snprintf(bf, size, fmt, percent);
 }
diff --git a/tools/perf/util/color.h b/tools/perf/util/color.h
index fced384..7ff30a6 100644
--- a/tools/perf/util/color.h
+++ b/tools/perf/util/color.h
@@ -39,6 +39,7 @@ int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);
 int color_snprintf(char *bf, size_t size, const char *color, const char *fmt, ...);
 int color_fprintf_ln(FILE *fp, const char *color, const char *fmt, ...);
 int color_fwrite_lines(FILE *fp, const char *color, size_t count, const char *buf);
+int value_color_snprintf(char *bf, size_t size, const char *fmt, double value);
 int percent_color_snprintf(char *bf, size_t size, const char *fmt, ...);
 int percent_color_fprintf(FILE *fp, const char *fmt, double percent);
 const char *get_percent_color(double percent);
-- 
1.8.5.rc0.5.g70ebc73.dirty

[PATCH v5 3/3] perf diff: color the Weighted Diff column

[Ramkumar Ramachandra]

In

  $ perf diff -c wdiff:M,N

color the numbers in the Weighted Diff column either green or red,
depending on whether the number 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 | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index c07cd3c..8e6e541 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -777,6 +777,7 @@ static int __hpp__color_compare(struct perf_hpp_fmt *fmt,
 		container_of(fmt, struct diff_hpp_fmt, fmt);
 	struct hist_entry *pair = get_pair_fmt(he, dfmt);
 	double diff;
+	s64 wdiff;
 	char pfmt[20] = " ";
 
 	if (!pair)
@@ -805,6 +806,18 @@ static int __hpp__color_compare(struct perf_hpp_fmt *fmt,
 		scnprintf(pfmt, 20, "%%%d.6f", dfmt->header_width);
 		return value_color_snprintf(hpp->buf, hpp->size,
 					pfmt, diff);
+	case COMPUTE_WEIGHTED_DIFF:
+		if (he->dummy)
+			goto dummy_print;
+		if (pair->diff.computed)
+			wdiff = pair->diff.wdiff;
+		else
+			wdiff = compute_wdiff(he, pair);
+
+		scnprintf(pfmt, 20, "%%14ld", dfmt->header_width);
+		return color_snprintf(hpp->buf, hpp->size,
+				wdiff > 0 ? PERF_COLOR_GREEN : PERF_COLOR_RED,
+				pfmt, wdiff);
 	default:
 		BUG_ON(1);
 	}
@@ -825,6 +838,12 @@ static int hpp__color_ratio(struct perf_hpp_fmt *fmt,
 	return __hpp__color_compare(fmt, hpp, he, COMPUTE_RATIO);
 }
 
+static int hpp__color_wdiff(struct perf_hpp_fmt *fmt,
+			struct perf_hpp *hpp, struct hist_entry *he)
+{
+	return __hpp__color_compare(fmt, hpp, he, COMPUTE_WEIGHTED_DIFF);
+}
+
 static void
 hpp__entry_unpair(struct hist_entry *he, int idx, char *buf, size_t size)
 {
@@ -1006,6 +1025,9 @@ static void data__hpp_register(struct data__file *d, int idx)
 	case PERF_HPP_DIFF__RATIO:
 		fmt->color = hpp__color_ratio;
 		break;
+	case PERF_HPP_DIFF__WEIGHTED_DIFF:
+		fmt->color = hpp__color_wdiff;
+		break;
 	default:
 		break;
 	}
-- 
1.8.5.rc0.5.g70ebc73.dirty

Re: [PATCH v5 3/3] perf diff: color the Weighted Diff column

[Jiri Olsa]

On Tue, Dec 10, 2013 at 06:19:02PM +0530, Ramkumar Ramachandra wrote:
> In
> 
>   $ perf diff -c wdiff:M,N
> 
> color the numbers in the Weighted Diff column either green or red,
> depending on whether the number 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 | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
> index c07cd3c..8e6e541 100644
> --- a/tools/perf/builtin-diff.c
> +++ b/tools/perf/builtin-diff.c
> @@ -777,6 +777,7 @@ static int __hpp__color_compare(struct perf_hpp_fmt *fmt,
>  		container_of(fmt, struct diff_hpp_fmt, fmt);
>  	struct hist_entry *pair = get_pair_fmt(he, dfmt);
>  	double diff;
> +	s64 wdiff;
>  	char pfmt[20] = " ";
>  
>  	if (!pair)
> @@ -805,6 +806,18 @@ static int __hpp__color_compare(struct perf_hpp_fmt *fmt,
>  		scnprintf(pfmt, 20, "%%%d.6f", dfmt->header_width);
>  		return value_color_snprintf(hpp->buf, hpp->size,
>  					pfmt, diff);
> +	case COMPUTE_WEIGHTED_DIFF:
> +		if (he->dummy)
> +			goto dummy_print;
> +		if (pair->diff.computed)
> +			wdiff = pair->diff.wdiff;
> +		else
> +			wdiff = compute_wdiff(he, pair);
> +
> +		scnprintf(pfmt, 20, "%%14ld", dfmt->header_width);
> +		return color_snprintf(hpp->buf, hpp->size,
> +				wdiff > 0 ? PERF_COLOR_GREEN : PERF_COLOR_RED,
> +				pfmt, wdiff);

this one is still wrong, you either need to use 'abs(wdiff) > 0'
or call value_color_snprintf (which is better) as in previous cases

jirka

Re: [PATCH v5 3/3] perf diff: color the Weighted Diff column

[Ramkumar Ramachandra]

Jiri Olsa wrote:
> this one is still wrong, you either need to use 'abs(wdiff) > 0'
> or call value_color_snprintf (which is better) as in previous cases

This was very much intentional on my part (see commit message): I
thought it would be nicer to color it this way because all the numbers
tend to be large. However, I'll resubmit with value_color_snprintf()
now.

p.s- Sorry about the terribly late response; was on vacation.