Re: [PATCH RESEND] perf diff: Introduce the new rules of colored printing of delta.

[Namhyung Kim]

Hi SeongSoo,

On Fri, Sep 30, 2016 at 8:44 AM, SeongSoo Cho <nexusz99@xxxxxxxxx> wrote:
> As you know, there are the common colored printing of percents so overhead(%) can be colored with the rule.
> But Delta means difference percents from percents of overhead between two files e.g. perf.data and perf.data.old.
> Although the rule is for overhead(%), Delta value also follow the same rule.
>
> So, I think that it would be better to use the new colored rule for the Delta as below.
>
> Increament: background colored in red (e.g. +0.50%)
> Decrement: colored in blue (e.g. -5.50%)
> Same: default color (e.g. +0.00%)

I'm not sure changing background color is good.  And you seems to
remove distinction between low, intermediate and high differences.
AFAIK same diff percentage is rare so you'll get all colorful output
and it'd make harder to focus on meaningful differences IMHO.

Thanks,
Namhyung

>
> Instead of percent_color_snprintf() function, use new delta_color_snprintf() function.
>
> Signed-off-by: SeongSoo Cho <nexusz99@xxxxxxxxx>
> Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
> Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
> Cc: Taeung Song <taeung@xxxxxxxxxx>
> ---
>  tools/perf/builtin-diff.c |  2 +-
>  tools/perf/util/color.c   | 21 +++++++++++++++++++++
>  tools/perf/util/color.h   |  1 +
>  3 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
> index 9ff0db4..228bad1 100644
> --- a/tools/perf/builtin-diff.c
> +++ b/tools/perf/builtin-diff.c
> @@ -868,7 +868,7 @@ static int __hpp__color_compare(struct perf_hpp_fmt *fmt,
>                         diff = compute_delta(he, pair);
>
>                 scnprintf(pfmt, 20, "%%%+d.2f%%%%", dfmt->header_width - 1);
> -               return percent_color_snprintf(hpp->buf, hpp->size,
> +               return delta_color_snprintf(hpp->buf, hpp->size,
>                                         pfmt, diff);
>         case COMPUTE_RATIO:
>                 if (he->dummy)
> diff --git a/tools/perf/util/color.c b/tools/perf/util/color.c
> index dbbf89b..643e932 100644
> --- a/tools/perf/util/color.c
> +++ b/tools/perf/util/color.c
> @@ -219,3 +219,24 @@ int percent_color_len_snprintf(char *bf, size_t size, const char *fmt, ...)
>         color = get_percent_color(percent);
>         return color_snprintf(bf, size, color, fmt, len, percent);
>  }
> +
> +int delta_color_snprintf(char *bf, size_t size, const char *fmt, ...)
> +{
> +       va_list args;
> +       double diff, percent;
> +       const char *color = PERF_COLOR_NORMAL;
> +
> +       va_start(args, fmt);
> +       diff = va_arg(args, double);
> +       va_end(args);
> +
> +       /* diff command printed second digit after the decimal point. */
> +       percent = roundf(diff * 100) / 100;
> +       if (percent < 0)
> +               color = PERF_COLOR_BLUE;
> +       else {
> +               if (percent > 0)
> +                       color = PERF_COLOR_BG_RED;
> +       }
> +       return color_snprintf(bf, size, color, fmt, diff);
> +}
> diff --git a/tools/perf/util/color.h b/tools/perf/util/color.h
> index a93997f..608edc9 100644
> --- a/tools/perf/util/color.h
> +++ b/tools/perf/util/color.h
> @@ -40,6 +40,7 @@ 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_len_snprintf(char *bf, size_t size, const char *fmt, ...);
>  int percent_color_fprintf(FILE *fp, const char *fmt, double percent);
> +int delta_color_snprintf(char *bf, size_t size, const char *fmt, ...);
>  const char *get_percent_color(double percent);
>
>  #endif /* __PERF_COLOR_H */
> --
> 2.7.4
>



-- 
Thanks,
Namhyung