Messages in this thread | | | From | Ian Rogers <> | Date | Fri, 26 Jun 2020 14:24:38 -0700 | Subject | Re: [PATCH 09/10] perf tools: Compute other metrics |
| |
On Fri, Jun 26, 2020 at 12:48 PM Jiri Olsa <jolsa@kernel.org> 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@kernel.org> > --- > 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 >
| |