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

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

[Ramkumar Ramachandra]

Hi,

This iteration fixes several small problems in the previous iteration
pointed out by Jiri Olsa. Most significantly, the first patch has been
dropped, and there's just one switch-case statement for the entire
logic.

Thanks.

Ramkumar Ramachandra (4):
  perf diff: color the Delta column
  perf diff: generalize hpp__color_delta for -c
  perf diff: color the Ratio column
  perf diff: color the Weighted Diff column

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

-- 
1.8.5.rc0.5.g70ebc73.dirty

[PATCH v3 1/4] perf diff: color the Delta column

[Ramkumar Ramachandra]

Color the numbers in the Delta 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 | 37 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index 3b67ea2..fe6e200 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 (fabs(percent) >= 0.01) {
+		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)
 {
@@ -940,8 +967,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 v3 1/4] perf diff: color the Delta column

[Jiri Olsa]

On Wed, Nov 27, 2013 at 04:32:31PM +0530, Ramkumar Ramachandra wrote:
> Color the numbers in the Delta 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 | 37 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
> index 3b67ea2..fe6e200 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 (fabs(percent) >= 0.01) {
> +		scnprintf(pfmt, 20, "%%%+d.2f%%%%", dfmt->header_width - 1);
> +		return color_snprintf(hpp->buf, hpp->size,
> +				percent > 0 ? PERF_COLOR_GREEN : PERF_COLOR_RED,

these colors are not consistent with colors in baseline,
moreover all negative values are shown as red

- please check get_percent_color function (used for baseline),
  it checks the percentage against following values:

  #define MIN_GREEN       0.5
  #define MIN_RED         5.0

- you probably want to use that, or whatever other helper
  baseline uses
- also you need to change it to use/check the abs() value
  of the percentage, because it does not count with negative
  values (it's for baseline only so far), which we will get
  in here..


thanks,
jirka

Re: [PATCH v3 1/4] perf diff: color the Delta column

[Ramkumar Ramachandra]

Jiri Olsa wrote:
> these colors are not consistent with colors in baseline,
> moreover all negative values are shown as red
>
> - please check get_percent_color function (used for baseline),
>   it checks the percentage against following values:
>
>   #define MIN_GREEN       0.5
>   #define MIN_RED         5.0

That was intentional. If you want colors that are consistent with the
baseline, what do we do about [4/4], where wdiff numbers can be huge?
(currently I use [green, red] for [positive, negative] there too)

Re: [PATCH v3 1/4] perf diff: color the Delta column

[Jiri Olsa]

On Thu, Nov 28, 2013 at 10:54:25PM +0530, Ramkumar Ramachandra wrote:
> Jiri Olsa wrote:
> > these colors are not consistent with colors in baseline,
> > moreover all negative values are shown as red
> >
> > - please check get_percent_color function (used for baseline),
> >   it checks the percentage against following values:
> >
> >   #define MIN_GREEN       0.5
> >   #define MIN_RED         5.0
> 
> That was intentional. If you want colors that are consistent with the
> baseline, what do we do about [4/4], where wdiff numbers can be huge?
> (currently I use [green, red] for [positive, negative] there too)

you're, ratio and wdiff need special treatment..

but I expect delta to be have same colors as baseline
(regardless of the +-)

thanks,
jirka

[PATCH v3 2/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 | 39 ++++++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 13 deletions(-)

diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index fe6e200..20284c3 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -769,31 +769,44 @@ 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 diff;
 	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 (fabs(percent) >= 0.01) {
+	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 color_snprintf(hpp->buf, hpp->size,
-				percent > 0 ? PERF_COLOR_GREEN : PERF_COLOR_RED,
-				pfmt, percent);
-	} else
+				diff > 0 ? PERF_COLOR_GREEN : PERF_COLOR_RED,
+				pfmt, diff);
+	default:
+		BUG_ON(1);
+	}
 dummy_print:
-		return scnprintf(hpp->buf, hpp->size, "%*s",
-				dfmt->header_width, pfmt);
+	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
-- 
1.8.5.rc0.5.g70ebc73.dirty

Re: [PATCH v3 2/4] perf diff: generalize hpp__color_delta for -c

[Jiri Olsa]

On Wed, Nov 27, 2013 at 04:32:32PM +0530, Ramkumar Ramachandra wrote:
> Generalize the function so that we can accommodate all three comparison
> methods: delta, ratio, and wdiff.

This patch basicaly factor the code you added in previous one.

Could we use the function below from the beggining?

jirka
> 
> 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 | 39 ++++++++++++++++++++++++++-------------
>  1 file changed, 26 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
> index fe6e200..20284c3 100644
> --- a/tools/perf/builtin-diff.c
> +++ b/tools/perf/builtin-diff.c
> @@ -769,31 +769,44 @@ 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 diff;
>  	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 (fabs(percent) >= 0.01) {
> +	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 color_snprintf(hpp->buf, hpp->size,
> -				percent > 0 ? PERF_COLOR_GREEN : PERF_COLOR_RED,
> -				pfmt, percent);
> -	} else
> +				diff > 0 ? PERF_COLOR_GREEN : PERF_COLOR_RED,
> +				pfmt, diff);
> +	default:
> +		BUG_ON(1);
> +	}
>  dummy_print:
> -		return scnprintf(hpp->buf, hpp->size, "%*s",
> -				dfmt->header_width, pfmt);
> +	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
> -- 
> 1.8.5.rc0.5.g70ebc73.dirty
> 

Re: [PATCH v3 2/4] perf diff: generalize hpp__color_delta for -c

[Ramkumar Ramachandra]

Jiri Olsa wrote:
> This patch basicaly factor the code you added in previous one.
>
> Could we use the function below from the beggining?

Yeah, I suppose I could squash this into [2/4].

[PATCH v3 3/4] perf diff: color the Ratio column

[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>
---
 tools/perf/builtin-diff.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index 20284c3..26f6517 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -795,6 +795,17 @@ static int __hpp__color_compare(struct perf_hpp_fmt *fmt,
 		return color_snprintf(hpp->buf, hpp->size,
 				diff > 0 ? PERF_COLOR_GREEN : PERF_COLOR_RED,
 				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 percent_color_snprintf(hpp->buf, hpp->size,
+					pfmt, diff);
 	default:
 		BUG_ON(1);
 	}
@@ -809,6 +820,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)
 {
@@ -987,6 +1004,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

[PATCH v3 4/4] 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 26f6517..9422314 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)
@@ -806,6 +807,18 @@ static int __hpp__color_compare(struct perf_hpp_fmt *fmt,
 		scnprintf(pfmt, 20, "%%%d.6f", dfmt->header_width);
 		return percent_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);
 	}
@@ -826,6 +839,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)
 {
@@ -1007,6 +1026,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