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

[Jiri Olsa]

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

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