Re: [PATCH 3/4] perf stat: Add --metrics-file option

[Ian Rogers]

On Wed, May 13, 2020 at 4:33 AM Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
>
> On Wed, May 13, 2020 at 12:04:55AM -0700, Ian Rogers wrote:
>
> SNIP
>
> > > +METRICS FILE
> > > +------------
> > > +The file with metrics has following syntax:
> > > +
> > > +  NAME = EXPRESSION ;
> > > +  NAME = EXPRESSION ;
> > > +  ...
> > > +
> > > +where NAME is unique identifier of the metric, which is later used in
> > > +perf stat as -M option argument (see below).
> > > +
> > > +The EXPRESSION is the metric's formula with following grammar:
> > > +
> > > +  EXPR: EVENT
> > > +  EXPR: EXPR if EXPR else EXPR
> >
> > Not introduced by this patch, but this patch is exposing it as an API.
>
> yea, I was thinking about this and I think we will put a disclaimer in
> here that this is not an API and the interface can change.. it's really
> mostly intended to help out with running a custom metric which is not
> compiled in ... I don't want to be commited to support old API
>
> > This notion of if-else is really weird. For one thing there are no
> > comparison operators. The unit test doesn't really help:
> >         ret |= test(&ctx, "1+1 if 3*4 else 0", 2);
> > What kind of comparison is "3*4"? If 0.0 causes the else clause then will -0.0?
> > A typical expression I see written in C is to give a ratio such:
> >   value = denom == 0 ? 0 : nom / denom;
> > I've worked around encoding this by extending expr.y locally.
>
> AFAICS it's used only with #SMT_on in the condition, aybe we could limit
> the condition only for #SMT_on term?
>
>
> >
> > > +  EXPR: NUMBER
> > > +  EXPR: EXPR | EXPR
> > > +  EXPR: EXPR & EXPR
> > > +  EXPR: EXPR ^ EXPR
> >
> > Again, it's odd that these cast the double to a long and then assign
> > the result back to a double.
>
> is this even used anywhere? perhaps it was added just to be complete

I don't believe they are used and checked with the pmu parsing test
that removing them doesn't cause any x86 expressions to break. I'd
prefer it if we could remove the unused operators and avoid
advertising here.

Thanks,
Ian

> SNIP
>
> > > +       2.002460174                 0.86                23.37                 0.86
> > > +       3.003969795                 1.03                23.93                 1.03
> > > +  ...
> >
> > A feature request would be to allow metrics in terms of other metrics,
> > not just events :-) For example, it is common to sum all cache
> > hit/miss events. It is laborious to copy that expression for hit rate,
> > miss rate, etc.
> >
> > Perhaps the expression parsing code should be folded into the event
> > parsing code.
>
> nice idea, but let's finish straighten up what we have first ;-)
>
> I'll try to go through all the fixes/tests you posted and let's
> get it in first
>
> thanks,
> jirka
>