Re: [PATCH 09/12] perf diff: Add weighted diff computation way to compare hist entries

From: Namhyung Kim
Date: Fri Sep 07 2012 - 02:05:29 EST

Next message: Shawn Guo: "Re: Tracking down suspend/resume ext3/mmc issues on imx233"
Previous message: tip-bot for Suzuki K. Poulose: "[tip:perf/core] perf tools: Remove the node from rblist in strlist__remove"
In reply to: Jiri Olsa: "[PATCH 09/12] perf diff: Add weighted diff computation way to compare hist entries"
Next in thread: Jiri Olsa: "Re: [PATCH 09/12] perf diff: Add weighted diff computation way tocompare hist entries"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, 6 Sep 2012 17:47:03 +0200, Jiri Olsa wrote:
> Adding 'wdiff' as new computation way to compare hist entries.
>
> If specified the 'Weighted diff' column is displayed with value 'd'
> computed as:
>
> d = B->period * WEIGHT-A - A->period * WEIGHT-B
>
> - A/B being matching hist entry from first/second file specified
> (or perf.data/perf.data.old) respectively.
>
> - period being the hist entry period value
>
> - WEIGHT-A/WEIGHT-B being user suplied weights in the the '-c' option
> behind ':' separator like '-c wdiff:1,2'.
>
> Cc: Arnaldo Carvalho de Melo <acme@xxxxxxxxxxxxxxxxxx>
> Cc: Peter Zijlstra <a.p.zijlstra@xxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxx>
> Cc: Paul Mackerras <paulus@xxxxxxxxx>
> Cc: Corey Ashford <cjashfor@xxxxxxxxxxxxxxxxxx>
> Cc: Frederic Weisbecker <fweisbec@xxxxxxxxx>
> Cc: Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx>
> Cc: Andi Kleen <andi@xxxxxxxxxxxxxx>
> Cc: David Ahern <dsahern@xxxxxxxxx>
> Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
> Signed-off-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> ---
> tools/perf/Documentation/perf-diff.txt | 15 +++++-
> tools/perf/builtin-diff.c | 95 +++++++++++++++++++++++++++++++++-
> tools/perf/ui/stdio/hist.c | 15 ++++++
> tools/perf/ui/stdio/hist.h | 1 +
> tools/perf/util/hist.h | 1 +
> tools/perf/util/sort.h | 3 ++
> 6 files changed, 127 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/Documentation/perf-diff.txt b/tools/perf/Documentation/perf-diff.txt
> index cff3d9b..fa413ac 100644
> --- a/tools/perf/Documentation/perf-diff.txt
> +++ b/tools/perf/Documentation/perf-diff.txt
> @@ -78,7 +78,7 @@ OPTIONS
>
> -c::
> --compute::
> - Differential computation selection - delta,ratio (default is delta).
> + Differential computation selection - delta,ratio,wdiff (default is delta).
> If '+' is specified as a first character, the output is sorted based
> on the computation results.
> See COMPARISON METHODS section for more info.
> @@ -110,6 +110,19 @@ with:
>
> - period being the hist entry period value
>
> +wdiff
> +~~~~~
> +If specified the 'Weighted diff' column is displayed with value 'd' computed as:
> +
> + d = B->period * WEIGHT-A - A->period * WEIGHT-B
> +
> + - A/B being matching hist entry from first/second file specified
> + (or perf.data/perf.data.old) respectively.
> +
> + - period being the hist entry period value
> +
> + - WEIGHT-A/WEIGHT-B being user suplied weights in the the '-c' option
> + behind ':' separator like '-c wdiff:1,2'.
>
> SEE ALSO
> --------
> diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
> index f72a2e4..6d8aba8 100644
> --- a/tools/perf/builtin-diff.c
> +++ b/tools/perf/builtin-diff.c
> @@ -28,23 +28,79 @@ static bool show_displacement;
> static bool show_baseline_only;
> static bool sort_compute;
>
> +static s64 compute_wdiff_w1;
> +static s64 compute_wdiff_w2;
> +
> enum {
> COMPUTE_DELTA,
> COMPUTE_RATIO,
> + COMPUTE_WEIGHTED_DIFF,
> COMPUTE_MAX,
> };
>
> const char *compute_names[COMPUTE_MAX] = {
> [COMPUTE_DELTA] = "delta",
> [COMPUTE_RATIO] = "ratio",
> + [COMPUTE_WEIGHTED_DIFF] = "wdiff",
> };
>
> static const char *compute_str;
> static int compute;
>
> +static int setup_compute_opt_wdiff(char *opt)
> +{
> + char *w1_str = opt;
> + char *w2_str;
> +
> + int ret = -EINVAL;
> +
> + do {
> + if (!opt)
> + break;
> +
> + w2_str = strchr(opt, ',');
> + if (!w2_str)
> + break;
> +
> + *w2_str++ = 0x0;
> + if (!*w2_str)
> + break;
> +
> + compute_wdiff_w1 = strtol(w1_str, NULL, 10);
> + compute_wdiff_w2 = strtol(w2_str, NULL, 10);
> +
> + if (!compute_wdiff_w1 || !compute_wdiff_w2)
> + break;
> +
> + pr_debug("compute wdiff w1(%" PRId64 ") w2(%" PRId64 ")\n",
> + compute_wdiff_w1, compute_wdiff_w2);
> + ret = 0;
> +
> + } while (0);

I don't see why this do { } while(0) loop is necessary.
How about this?

w1 = strtol(opt, &tmp, 10);
if (*tmp != ',')
goto error;

opt = tmp + 1;
w2 = strtol(opt, &tmp, 10);
if (*tmp != '\0')
goto error;

if (!w1 || !w2)
goto error;

Thanks,
Namhyung

> +
> + if (ret)
> + pr_err("Weight parsing failed.");
> +
> + return ret;
> +}
> +
> +static int setup_compute_opt(char *opt)
> +{
> + if (compute == COMPUTE_WEIGHTED_DIFF)
> + return setup_compute_opt_wdiff(opt);
> +
> + if (opt) {
> + pr_err("Extra option specified.");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> static int setup_compute(void)
> {
> unsigned i;
> + char *opt;
>
> if (!compute_str) {
> compute = COMPUTE_DELTA;
> @@ -58,10 +114,14 @@ static int setup_compute(void)
> return 0;
> }
>
> + opt = strchr(compute_str, ':');
> + if (opt)
> + *opt++ = 0x0;
> +
> for (i = 0; i < COMPUTE_MAX; i++)
> if (!strcmp(compute_str, compute_names[i])) {
> compute = i;
> - return 0;
> + return setup_compute_opt(opt);
> }
>
> pr_err("Failed to find valid compute string\n");
> @@ -96,6 +156,23 @@ double perf_diff__compute_ratio(struct hist_entry *he)
> return he->diff.period_ratio;
> }
>
> +double perf_diff__compute_wdiff(struct hist_entry *he)
> +{
> + struct hist_entry *pair = he->pair;
> + u64 new_period = he->period;
> + u64 old_period = pair ? pair->period : 0;
> +
> + he->diff.computed = true;
> +
> + if (!pair)
> + he->diff.wdiff = 0;
> + else
> + he->diff.wdiff = new_period * compute_wdiff_w2 -
> + old_period * compute_wdiff_w1;
> +
> + return he->diff.wdiff;
> +}
> +
> static int hists__add_entry(struct hists *self,
> struct addr_location *al, u64 period)
> {
> @@ -277,6 +354,9 @@ static void hists__precompute(struct hists *hists)
> case COMPUTE_RATIO:
> perf_diff__compute_ratio(he);
> break;
> + case COMPUTE_WEIGHTED_DIFF:
> + perf_diff__compute_wdiff(he);
> + break;
> default:
> BUG_ON(1);
> }
> @@ -312,6 +392,13 @@ hist_entry__cmp_compute(struct hist_entry *left, struct hist_entry *right,
>
> return cmp_doubles(l, r);
> }
> + case COMPUTE_WEIGHTED_DIFF:
> + {
> + s64 l = left->diff.wdiff;
> + s64 r = right->diff.wdiff;
> +
> + return r - l;
> + }
> default:
> BUG_ON(1);
> }
> @@ -436,7 +523,8 @@ static const struct option options[] = {
> "Show position displacement relative to baseline"),
> OPT_BOOLEAN('b', "baseline-only", &show_baseline_only,
> "Show only items with match in baseline"),
> - OPT_STRING('c', "compute", &compute_str, "delta,ratio (default delta)",
> + OPT_STRING('c', "compute", &compute_str,
> + "delta,ratio,wdiff:w1,w2 (default delta)",
> "Entries differential computation selection"),
> OPT_BOOLEAN('D', "dump-raw-trace", &dump_trace,
> "dump raw trace in ASCII"),
> @@ -471,6 +559,9 @@ static void setup_ui_stdio(void)
> case COMPUTE_RATIO:
> hists_stdio_column__register_idx(HISTC_RATIO);
> break;
> + case COMPUTE_WEIGHTED_DIFF:
> + hists_stdio_column__register_idx(HISTC_WEIGHTED_DIFF);
> + break;
> default:
> BUG_ON(1);
> };
> diff --git a/tools/perf/ui/stdio/hist.c b/tools/perf/ui/stdio/hist.c
> index 8c717ab..f580085 100644
> --- a/tools/perf/ui/stdio/hist.c
> +++ b/tools/perf/ui/stdio/hist.c
> @@ -47,6 +47,20 @@ hists_stdio_column__ratio_snprintf(struct hist_entry *he, char *bf,
> }
>
> static int
> +hists_stdio_column__wdiff_snprintf(struct hist_entry *he, char *bf,
> + size_t size, unsigned int width __used)
> +{
> + u64 wdiff;
> +
> + if (he->diff.computed)
> + wdiff = he->diff.wdiff;
> + else
> + wdiff = perf_diff__compute_wdiff(he);
> +
> + return scnprintf(bf, size , "%+13ld", wdiff);
> +}
> +
> +static int
> hists_stdio_column__baseline_snprintf(struct hist_entry *he, char *bf,
> size_t size, unsigned int width __used)
> {
> @@ -141,6 +155,7 @@ DEF_COLUMN(nr_samples, HISTC_NR_SAMPLES, 12, "Samples")
> DEF_COLUMN(total_period, HISTC_TOTAL_PERIOD, 12, "Period")
> DEF_COLUMN(delta, HISTC_DELTA, 8, "Delta")
> DEF_COLUMN(ratio, HISTC_RATIO, 14, "Ratio")
> +DEF_COLUMN(wdiff, HISTC_WEIGHTED_DIFF, 13, "Weighted diff")
> DEF_COLUMN(displacement, HISTC_DISPLACEMENT, 5, "Displ")
> };
>
> diff --git a/tools/perf/ui/stdio/hist.h b/tools/perf/ui/stdio/hist.h
> index c8ac633..f725189 100644
> --- a/tools/perf/ui/stdio/hist.h
> +++ b/tools/perf/ui/stdio/hist.h
> @@ -19,5 +19,6 @@ void hists_stdio_column__set_width(struct hists *hists);
>
> double perf_diff__compute_delta(struct hist_entry *he);
> double perf_diff__compute_ratio(struct hist_entry *he);
> +double perf_diff__compute_wdiff(struct hist_entry *he);
>
> #endif /* __PERF_UI_STDIO_HIST_H */
> diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
> index b24341d..745e0cc 100644
> --- a/tools/perf/util/hist.h
> +++ b/tools/perf/util/hist.h
> @@ -47,6 +47,7 @@ enum hist_column {
> HISTC_TOTAL_PERIOD,
> HISTC_DELTA,
> HISTC_RATIO,
> + HISTC_WEIGHTED_DIFF,
> HISTC_DISPLACEMENT,
>
> /* sorted (hist_entry__sort_list) */
> diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
> index 9f707b7..73f1ffe 100644
> --- a/tools/perf/util/sort.h
> +++ b/tools/perf/util/sort.h
> @@ -53,6 +53,9 @@ struct hist_entry_diff {
>
> /* HISTC_RATIO */
> double period_ratio;
> +
> + /* HISTC_WEIGHTED_DIFF */
> + s64 wdiff;
> };
>
> /**
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Shawn Guo: "Re: Tracking down suspend/resume ext3/mmc issues on imx233"
Previous message: tip-bot for Suzuki K. Poulose: "[tip:perf/core] perf tools: Remove the node from rblist in strlist__remove"
In reply to: Jiri Olsa: "[PATCH 09/12] perf diff: Add weighted diff computation way to compare hist entries"
Next in thread: Jiri Olsa: "Re: [PATCH 09/12] perf diff: Add weighted diff computation way tocompare hist entries"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]