Re: [PATCH 09/10] perf tools: Compute other metrics

From: Ian Rogers
Date: Fri Jun 26 2020 - 17:24:54 EST


On Fri, Jun 26, 2020 at 12:48 PM Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
>
> Adding computation (expr__parse call) of 'other' metric at
> the point when it needs to be resolved during the 'master'
> metric computation.
>
> Once the inner metric is computed, the result is stored and
> used if there's another usage of that metric.
>
> Signed-off-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> ---
> tools/perf/util/expr.c | 3 +++
> tools/perf/util/expr.h | 1 +
> tools/perf/util/expr.y | 20 +++++++++++++++++++-
> 3 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
> index 32f7acac7c19..1b6d550cec5f 100644
> --- a/tools/perf/util/expr.c
> +++ b/tools/perf/util/expr.c
> @@ -91,6 +91,7 @@ int expr__add_other(struct expr_parse_ctx *ctx, struct metric_other *other)
>
> data_ptr->other.metric_name = other->metric_name;
> data_ptr->other.metric_expr = other->metric_expr;
> + data_ptr->other.counted = false;
> data_ptr->is_other = true;
>
> ret = hashmap__set(&ctx->ids, name, data_ptr,
> @@ -150,6 +151,8 @@ __expr__parse(double *val, struct expr_parse_ctx *ctx, const char *expr,
> void *scanner;
> int ret;
>
> + pr_debug2("parsing metric: %s\n", expr);
> +
> ret = expr_lex_init_extra(&scanner_ctx, &scanner);
> if (ret)
> return ret;
> diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
> index ed60f9227b43..f85f3941eda5 100644
> --- a/tools/perf/util/expr.h
> +++ b/tools/perf/util/expr.h
> @@ -25,6 +25,7 @@ struct expr_parse_data {
> struct {
> const char *metric_name;
> const char *metric_expr;
> + bool counted;
> } other;
> };
> };
> diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
> index 6252d9f6cfc8..cca423331f65 100644
> --- a/tools/perf/util/expr.y
> +++ b/tools/perf/util/expr.y
> @@ -89,12 +89,30 @@ if_expr:
> expr: NUMBER
> | ID {
> struct expr_parse_data *data;
> + char *lookup = $1;
> + const char *name;
>
> - if (expr__get_id(ctx, $1, &data) || !data) {
> + if (expr__is_metric($1, &name))
> + lookup = name;
> +
> + if (expr__get_id(ctx, lookup, &data) || !data) {
> pr_debug("%s not found\n", $1);
> free($1);
> YYABORT;
> }
> +
> + pr_debug2("lookup: is_other %d, counted %d: %s\n",
> + data->is_other, data->other.counted, lookup);
> +
> + if (data->is_other && !data->other.counted) {
> + data->other.counted = true;
> + if (expr__parse(&data->val, ctx, data->other.metric_expr, 1)) {

Ah, so this handles the problem the referenced metric isn't calculated
and calculates it - with the sharing of events this doesn't impose
extra pmu cost. Do we need to worry about detecting recursion? For
example:

"MetricName": "Foo",
"MetricExpr": "1/metric:Foo",

It seems unfortunate to have the MetricExpr calculated twice, but it
is understandable. Is it also a property that referenced/other metrics
won't be reported individually? Perhaps these are sub-metrics?

Thanks,
Ian

> + pr_debug("%s failed to count\n", $1);
> + free($1);
> + YYABORT;
> + }
> + }
> +
> $$ = data->val;
> free($1);
> }
> --
> 2.25.4
>