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

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

[Ramkumar Ramachandra]

Hi,

Two minor changes in this iteration:

- The numbers in [1/3] are colored using percent_color_snprintf()
  instead of using (green, red) for (positive, negative).

- Two of the patches in the previous iteration were squashed together
  to make [2/3] in this iteration.

Thanks again to Jiri Olsa.

Cc: Jiri Olsa <jolsa@redhat.com>
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 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 90 insertions(+), 1 deletion(-)

-- 
1.8.5.rc0.5.g70ebc73.dirty

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

[Ramkumar Ramachandra]

Color the numbers in the Delta column using percent_color_snprintf().
Generalize the function so that we can accommodate all three comparison
methods in the future: delta, ratio, and wdiff.

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

diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index 3b67ea2..c970aae 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, fabs(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);
-- 
1.8.5.rc0.5.g70ebc73.dirty

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

[Jiri Olsa]

On Fri, Nov 29, 2013 at 07:06:30PM +0530, Ramkumar Ramachandra wrote:
> Color the numbers in the Delta column using percent_color_snprintf().
> Generalize the function so that we can accommodate all three comparison
> methods in the future: delta, ratio, and wdiff.
> 
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  tools/perf/builtin-diff.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
> index 3b67ea2..c970aae 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, fabs(diff));

we dont want print fabs(diff)..  we just want percent_color_snprintf
assign proper color for negative numbers, the change needs to go there

jirka

[PATCH v4 2/3] 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: 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 c970aae..25dac6c 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, fabs(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);
 	}
@@ -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;
 	}
-- 
1.8.5.rc0.5.g70ebc73.dirty

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

[Jiri Olsa]

On Fri, Nov 29, 2013 at 07:06:31PM +0530, Ramkumar Ramachandra wrote:
> In
> 
>   $ perf diff -c ratio
> 
> color the Ratio column using percent_color_snprintf().
> 
> Cc: Jiri Olsa <jolsa@redhat.com>
> 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 c970aae..25dac6c 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, fabs(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);

ok, lets keep same limits for ratio and wdiff.. unless
we hear otherwise ;-)

Arnaldo,
I think we want to add something like 'value_color_snprintf' ?
to keep percent/values separated..

It'd do the same job, just the name does not fit in here,
because we are printing out ratio values.

thanks,
jirka

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

[Ramkumar Ramachandra]

Arnaldo Carvalho de Melo wrote:
> static inline percent_color_snprintf(...)
> {
>         return value_color_snprintf(...);
> }

The issue with this suggestion is that the prototype of
percent_color_snprintf() is:

  int percent_color_snprintf(char *bf, size_t size, const char *fmt, ...)

So, I can only pass value_color_snprintf() a va_list, making its prototype:

  int value_color_snprintf(char *bf, size_t size, const char *fmt, va_list args)

Is this worth the minor rename?

On a related note, does percent_color_snprintf() need to be a variadic
function? It only seems to process one percent value.

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

[Arnaldo Carvalho de Melo]

Em Wed, Dec 04, 2013 at 03:40:25PM +0530, Ramkumar Ramachandra escreveu:
> Arnaldo Carvalho de Melo wrote:
> > static inline percent_color_snprintf(...)
> > {
> >         return value_color_snprintf(...);
> > }
> 
> The issue with this suggestion is that the prototype of
> percent_color_snprintf() is:
> 
>   int percent_color_snprintf(char *bf, size_t size, const char *fmt, ...)
> 
> So, I can only pass value_color_snprintf() a va_list, making its prototype:
> 
>   int value_color_snprintf(char *bf, size_t size, const char *fmt, va_list args)
> 
> Is this worth the minor rename?

That is ok, I'd say. Or if that would be a problem one could consider
using a macro, perhaps.
 
> On a related note, does percent_color_snprintf() need to be a variadic
> function? It only seems to process one percent value.

I think that there are places where it is passed as a pointer that
expects it to have that prototype, this is from memory, so please check.

- Arnaldo

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

[Ramkumar Ramachandra]

Arnaldo Carvalho de Melo wrote:
>> The issue with this suggestion is that the prototype of
>> percent_color_snprintf() is:
>>
>>   int percent_color_snprintf(char *bf, size_t size, const char *fmt, ...)
>>
>> So, I can only pass value_color_snprintf() a va_list, making its prototype:
>>
>>   int value_color_snprintf(char *bf, size_t size, const char *fmt, va_list args)
>>
>> Is this worth the minor rename?
>
> That is ok, I'd say. Or if that would be a problem one could consider
> using a macro, perhaps.

If value_color_snprintf has the prototype mentioned above, I'd have to
build a va_list at each callsite; how is that convenient? Considering
the macro, can we just do

  #define percent_color_snprintf value_color_snprintf

in color.h?

>> On a related note, does percent_color_snprintf() need to be a variadic
>> function? It only seems to process one percent value.
>
> I think that there are places where it is passed as a pointer that
> expects it to have that prototype, this is from memory, so please check.

I meant that the function body itself seems to be wrong:

  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);

Unless I've misunderstood something horribly, doesn't `percent' just
get assigned to the last argument in the `args' va_list?

[PATCH v4 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: 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 25dac6c..d6ae643 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 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);
 	}
@@ -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