[PATCH v2 0/5] More color in 'perf diff'

[PATCH v2 0/5] More color in 'perf diff'

[Ramkumar Ramachandra]

Hi,

This round fixes two problems in the last round:

- In [2/5], the color version includes the 0.01 threshold that was
  present in the non-color version (thanks to Jiri).

- In [4/5], the trailing '%' is gone (maked as a TODO in the last
  round).

and introduces one new patch for wdiff: [5/5].

Thanks.

Ramkumar Ramachandra (5):
  perf diff: don't compute Delta if he->dummy
  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 | 109 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 108 insertions(+), 1 deletion(-)

-- 
1.8.5.rc0.5.g70ebc73.dirty

[PATCH v2 1/5] 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 v2 2/5] 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 79e0448..84d5f2d 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 && 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)
 {
@@ -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

[PATCH v2 3/5] 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 | 47 +++++++++++++++++++++++++++++++++++++----------
 1 file changed, 37 insertions(+), 10 deletions(-)

diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index 84d5f2d..a54736c 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -769,33 +769,60 @@ 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 && 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);
+	if (!he->dummy) {
+		switch (comparison_method) {
+		case COMPUTE_DELTA:
+			if (fabs(delta) < 0.01)
+				goto dummy_print;
+			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

[PATCH v2 4/5] 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 a54736c..be2550c 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);
 		}
@@ -808,6 +814,11 @@ static int __hpp__color_compare(struct perf_hpp_fmt *fmt,
 					delta > 0 ? PERF_COLOR_GREEN : PERF_COLOR_RED,
 					pfmt, delta);
 			break;
+		case COMPUTE_RATIO:
+			scnprintf(pfmt, 20, "%%%d.6f", dfmt->header_width);
+			return percent_color_snprintf(hpp->buf, hpp->size,
+						pfmt, delta);
+			break;
 		default:
 			BUG_ON(1);
 		}
@@ -823,6 +834,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)
 {
@@ -1004,6 +1021,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 v2 5/5] 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 be2550c..f3f5d8e 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 delta;
+	s64 wdiff;
 	char pfmt[20] = " ";
 
 	if (!pair)
@@ -789,6 +790,9 @@ static int __hpp__color_compare(struct perf_hpp_fmt *fmt,
 		case COMPUTE_RATIO:
 			delta = pair->diff.period_ratio;
 			break;
+		case COMPUTE_WEIGHTED_DIFF:
+			wdiff = pair->diff.wdiff;
+			break;
 		default:
 			BUG_ON(1);
 		}
@@ -800,6 +804,9 @@ static int __hpp__color_compare(struct perf_hpp_fmt *fmt,
 		case COMPUTE_RATIO:
 			delta = compute_ratio(he, pair);
 			break;
+		case COMPUTE_WEIGHTED_DIFF:
+			wdiff = compute_wdiff(he, pair);
+			break;
 		default:
 			BUG_ON(1);
 		}
@@ -819,6 +826,12 @@ static int __hpp__color_compare(struct perf_hpp_fmt *fmt,
 			return percent_color_snprintf(hpp->buf, hpp->size,
 						pfmt, delta);
 			break;
+		case COMPUTE_WEIGHTED_DIFF:
+			scnprintf(pfmt, 20, "%%14ld", dfmt->header_width);
+			return color_snprintf(hpp->buf, hpp->size,
+					wdiff > 0 ? PERF_COLOR_GREEN : PERF_COLOR_RED,
+					pfmt, wdiff);
+			break;
 		default:
 			BUG_ON(1);
 		}
@@ -840,6 +853,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)
 {
@@ -1024,6 +1043,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 v2 1/5] perf diff: don't compute Delta if he->dummy

[Jiri Olsa]

On Sun, Nov 24, 2013 at 11:50:21AM +0530, Ramkumar Ramachandra wrote:
> 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;
> +

the reason I disabled the computation for dummy pairs was
that the number (ratio and weighted diff) would not make much
sense (to me) without the real pair

but within the dummy delta column we actually see the compared
file % overheader which is helpfull, even if there's no real pair
in the baseline

jirka

Re: [PATCH v2 2/5] perf diff: color the Delta column

[Jiri Olsa]

On Sun, Nov 24, 2013 at 11:50:22AM +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 79e0448..84d5f2d 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;

same comment here ^^^ as for previous patch

jirka

Re: [PATCH v2 3/5] perf diff: generalize hpp__color_delta for -c

[Jiri Olsa]

On Sun, Nov 24, 2013 at 11:50:23AM +0530, Ramkumar Ramachandra wrote:
> 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 | 47 +++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 37 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
> index 84d5f2d..a54736c 100644
> --- a/tools/perf/builtin-diff.c
> +++ b/tools/perf/builtin-diff.c
> @@ -769,33 +769,60 @@ 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 && 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);
> +	if (!he->dummy) {

isn't this check superfluous because of the above (!pair) check?

jirka

Re: [PATCH v2 3/5] perf diff: generalize hpp__color_delta for -c

[Jiri Olsa]

On Sun, Nov 24, 2013 at 11:50:23AM +0530, Ramkumar Ramachandra wrote:

SNIP

>  		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 && 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);
> +	if (!he->dummy) {
> +		switch (comparison_method) {
> +		case COMPUTE_DELTA:
> +			if (fabs(delta) < 0.01)
> +				goto dummy_print;
> +			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;

I think it'd be better to have just one switch for
comparison_method and do all the processing within

thanks,
jirka

Re: [PATCH v2 3/5] perf diff: generalize hpp__color_delta for -c

[Ramkumar Ramachandra]

Jiri Olsa wrote:
>>       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 && 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);
>> +     if (!he->dummy) {
>
> isn't this check superfluous because of the above (!pair) check?

>From builtin-diff.c:get_pair_data(), we see that `pair' is one of the
pairs in he->pairs. he->dummy is set in
util/hist.c:hists__add_dummy_entry() which is called only when he
doesn't have pairs (util/hist.c:942). Wait, couldn't
util/hist.c:hists__add_dummy_entry() also have returned NULL in the
case when he is NULL? But  __hpp__color_compare wouldn't have been
called with a NULL he in the first place. So yeah, the check is
redundant although it wasn't immediately obvious to me.

Thanks.

Re: [PATCH v2 3/5] perf diff: generalize hpp__color_delta for -c

[Ramkumar Ramachandra]

Jiri Olsa wrote:
> I think it'd be better to have just one switch for
> comparison_method and do all the processing within

Okay, will fix in the next iteration.

Re: [PATCH v2 2/5] perf diff: color the Delta column

[Ramkumar Ramachandra]

Jiri Olsa wrote:
>> diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
>> index 79e0448..84d5f2d 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;
>
> same comment here ^^^ as for previous patch

Hm? The comment on the previous patch pertained to printing the delta
if he->dummy was set (ie. if `he' doesn't have any pairs). This
statement is analogous to the logic in __hpp__entry_global(): if
(!pair), then the next line checking pair->diff.computed will segfault
without this check.

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

[Ramkumar Ramachandra]

Jiri Olsa wrote:
>> 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;
>> +
>
> the reason I disabled the computation for dummy pairs was
> that the number (ratio and weighted diff) would not make much
> sense (to me) without the real pair
>
> but within the dummy delta column we actually see the compared
> file % overheader which is helpfull, even if there's no real pair
> in the baseline

Wait a minute. Doesn't your comment on [3/5] apply here too?
__hpp__entry_global() computes and checks `pair' before calling
hpp__entry_pair(). If he->dummy is set, it means that `he' doesn't
have any pairs: which means that hpp_entry_pair() wouldn't have been
called by __hpp_entry_global() in the first place. So aren't the
he->dummy checks in the RATIO and WEIGHTED_DIFF case redundant?

Re: [PATCH v2 3/5] perf diff: generalize hpp__color_delta for -c

[Ramkumar Ramachandra]

Ramkumar Ramachandra wrote:
> Jiri Olsa wrote:
>>>       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 && 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);
>>> +     if (!he->dummy) {
>>
>> isn't this check superfluous because of the above (!pair) check?
>
> From builtin-diff.c:get_pair_data(), we see that `pair' is one of the
> pairs in he->pairs. he->dummy is set in
> util/hist.c:hists__add_dummy_entry() which is called only when he
> doesn't have pairs (util/hist.c:942).

Sorry about the major thinko: hists__add_dummy_entry() returns a new
`he' with he->dummy set; it has nothing to do with pair->pairs of the
`pair' that it is passed.

I think I understand what's going on: I'll submit a new iteration soon.