On Fri, May 15, 2020 at 02:35:43PM -0700, Ian Rogers wrote:
> On Fri, May 15, 2020 at 12:41 PM Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
> >
> > On Fri, May 15, 2020 at 09:50:07AM -0700, Ian Rogers wrote:
> >
> > SNIP
> >
> > > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> > > index b071df373f8b..37be5a368d6e 100644
> > > --- a/tools/perf/util/metricgroup.c
> > > +++ b/tools/perf/util/metricgroup.c
> > > @@ -85,8 +85,7 @@ static void metricgroup__rblist_init(struct rblist *metric_events)
> > >
> > > struct egroup {
> > > struct list_head nd;
> > > - int idnum;
> > > - const char **ids;
> > > + struct expr_parse_ctx pctx;
> > > const char *metric_name;
> > > const char *metric_expr;
> > > const char *metric_unit;
> > > @@ -94,19 +93,21 @@ struct egroup {
> > > };
> > >
> > > static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> > > - const char **ids,
> > > - int idnum,
> > > + struct expr_parse_ctx *pctx,
> > > struct evsel **metric_events,
> > > bool *evlist_used)
> > > {
> > > struct evsel *ev;
> > > - int i = 0, j = 0;
> > > bool leader_found;
> > > + const size_t idnum = hashmap__size(&pctx->ids);
> > > + size_t i = 0;
> > > + int j = 0;
> > > + double *val_ptr;
> > >
> > > evlist__for_each_entry (perf_evlist, ev) {
> > > if (evlist_used[j++])
> > > continue;
> > > - if (!strcmp(ev->name, ids[i])) {
> > > + if (hashmap__find(&pctx->ids, ev->name, (void **)&val_ptr)) {
> >
> > hum, you sure it's doing the same thing as before?
> >
> > hashmap__find will succede all the time in here, while the
> > previous code was looking for the start of the group ...
> > the logic in here is little convoluted, so maybe I'm
> > missing some point in here ;-)
>
> If we have a metric like "A + B" and another like "C / D" then by
> we'll generate a string (the extra_events strbuf in the code) like
> "{A,B}:W,{C,D}:W" from __metricgroup__add_metric. This will turn into
> an evlist in metricgroup__parse_groups of A,B,C,D. The code is trying
> to associate the events A,B with the first metric and C,D with the
> second. The code doesn't support sharing of events and events are
> marked as used and can't be part of other metrics. The evlist order is
> also reflective of the order of metrics, so if there were metrics "A +
> B + C" and "A + B", as the first metric is first in the evlist we
> don't run the risk of C being placed with A and B in a different
> group.
>
> The old code used the order of events to match within a metric and say
> for metric "A+B+C" we want to match A then B, and so on. The new code
> acts more like a set, so "A + B + C" becomes a set containing A, B and
> C, we check A is in the set then B and then C. For both pieces of code
> they are only working because of the evlist_used "bitmap" and that the
> order in the evlists and metrics matches.
>
> The current code could just use ordering to match first n1 events with
> the first metric, the next n2 events with the second and so on. So
> both the find now, and the strcmp before always return true in this
> branch.
>
> In the RFC patch set I want to share events and so I do checks related
> to the group leader so that I know when moving from one group to
> another in the evlist. The find/strcmp becomes load bearing as I will
> re-use events as long as they match.
> https://lore.kernel.org/lkml/20200508053629.210324-14-irogers@xxxxxxxxxx/
>
> > jirka
> >
> > > if (!metric_events[i])
> > > metric_events[i] = ev;
> > > i++;
> > > @@ -118,7 +119,8 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> > > memset(metric_events, 0,
> > > sizeof(struct evsel *) * idnum);
>
> This re-check was unnecessary in the old code and unnecessary even
> more so now as the hashmap_find is given exactly the same arguments.
> I'll remove it in v3 while addressing Andrii's memory leak fixes.
ok, that was my other confusion, because it's useless ;-)
thanks for the explanation,
jirka
>
> Thanks,
> Ian
>
> > > - if (!strcmp(ev->name, ids[i])) {
> > > + if (hashmap__find(&pctx->ids, ev->name,
> > > + (void **)&val_ptr)) {
> > > if (!metric_events[i])
> > > metric_events[i] = ev;
> >
> > SNIP
> >
>
