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

[Ian Rogers]

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
>