Re: [PATCH 10/10] perf, tools, stat: Output JSON MetricExpr metric

From: Jiri Olsa <jolsa@redhat.com>
To: Andi Kleen <andi@firstfloor.org>
Cc: Andi Kleen <ak@linux.intel.com>,
	acme@kernel.org, jolsa@kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 10/10] perf, tools, stat: Output JSON MetricExpr metric
Date: Fri, 10 Feb 2017 09:22:50 +0100	[thread overview]
Message-ID: <20170210082250.GA24530@krava> (raw)
In-Reply-To: <20170209185942.GS26852@two.firstfloor.org>

On Thu, Feb 09, 2017 at 10:59:43AM -0800, Andi Kleen wrote:
> On Thu, Feb 09, 2017 at 07:37:55PM +0100, Jiri Olsa wrote:
> > > The last time I proposed separate files Ingo vetoed it.
> > > He wanted everything built in.
> > 
> > sure, he veto it for event files.. expressions could be built
> > in same way as we have events now
> 
> That's exactly what I implemented. The expression is part
> of the JSON file, which seems like the logical place.
> 
> You just want it in a separate file in the source?
> 
> 
> > 
> > > > from which point we could point and configure events we need
> > > 
> > > If you want full flexibility you can use your perf stat report
> > > approach, or what most people do is to just run a script/spreadsheet
> > > over the the -x; output. This all continues to work.
> > > 
> > > This is just a minimum approach to provide some convenience
> > > integrated with the event list to provide something similar
> > > as the built in expressions in stat-shadow. 
> > > 
> > > It's not trying to build the great perf scripting language.
> > 
> > yea I understand that but can't ack that based on the points
> > I descibed in my other email
> 
> So what are the crucial points that prevent you?
> 
> - You want better column descriptions? 
> 
> I suppose could add another field for this.
> 
> - You want multiple expressions per event 
> (even though they are not needed today)?
> 
> It could be implemented, but seems like unnecessary
> complexity and overengineering to me at this point.
> If nobody ever uses it would it be time spent well?
> If someone really uses it they could add the support at that
> time.

ok, I checked optimization doc and you might be right about this one,
I couldn't find an example that'd prove you wrong ;-)

however I dont think expression should be annonymous number
tied to an event. For example there's this one:

  B.7.9.2  
  Fast Synchronization Penalty
  50. Locked Operations Impact: (L1D_CACHE_LOCK_DURATION + 20 * L1D_CACHE_LOCK.MESI) / CPU_CLK_UNHALTED.CORE * 100

  Fast synchronization is frequently implemented usin
  g locked memory accesses. A high value for Locked 
  Operations Impact indicates that locked operations us
  ed in the workload have high penalty. The latency 
  of a locked operation depends on the location of the 
  data: L1 data cache, L2 cache, other core cache or 
  memory.

There's a name and description.

which event will pick this expression?  L1D_CACHE_LOCK_DURATION or L1D_CACHE_LOCK?

How about we add:

"MetricExpr":      "(L1D_CACHE_LOCK_DURATION + 20 * L1D_CACHE_LOCK.MESI) / CPU_CLK_UNHALTED.CORE * 100"
"MetricExprName":  "Fast Synchronization Penalty"
"MetricExprDoc":   "Fast synchronization is frequently implemented... "

then it could be part of the event record and we can display it
like use the name in column and doc in the perf list

We could append number or something (MetricExpr2..) if there'll
be multiple expressions in some case.. or something like that

> 
> - You want automatic group creation?
> 
> It has nasty corner cases.
> It would be possible to build something that works
> for simple cases.

that would be nice.. when user says perf stat -e 'Fast Synchronization Penalty' ...
it'd create necessary events for him.. or some shorted name probably

jirka