Re: [PATCH v2 7/7] perf expr: Migrate expr ids table to a hashmap

[Jiri Olsa <jolsa@redhat.com>]

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@redhat.com> 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@google.com/
> 
> > 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
> >
>