Re: [PATCH 3/4] perf hist: Do not use event index in hpp__fmt()

[Ian Rogers]

On Mon, Feb 12, 2024 at 11:52 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
>
> The __hpp__fmt() is to print period values in a hist entry.  It handles
> event groups using linked pair entries.  Until now, it used event index
> to print values of group members.  But we want to make it more robust
> and support groups even if some members in the group were removed.

I'm unclear how it breaks currently. The evsel idx is set the evlist
nr_entries on creation and not updated by a remove. A remove may
change a groups leader, should the remove also make the next entry's
index idx that of the previous group leader?

> Let's use an index table from evsel to value array so that we can skip
> dummy events in the output later.
>
> Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
> ---
>  tools/perf/ui/hist.c | 34 ++++++++++++++++++++++++++++------
>  1 file changed, 28 insertions(+), 6 deletions(-)
>
> diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> index 5f4c110d840f..9c4c738edde1 100644
> --- a/tools/perf/ui/hist.c
> +++ b/tools/perf/ui/hist.c
> @@ -48,15 +48,30 @@ static int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
>         if (evsel__is_group_event(evsel)) {
>                 int idx;
>                 struct hist_entry *pair;
> -               int nr_members = evsel->core.nr_members;
> +               int nr_members = evsel->core.nr_members - 1;

A comment on the -1 would be useful.

Thanks,
Ian


>                 union {
>                         u64 period;
>                         double percent;
>                 } *val;
> +               struct evsel *member;
> +               struct evsel **idx_table;
>
>                 val = calloc(nr_members, sizeof(*val));
>                 if (val == NULL)
> -                       return 0;
> +                       goto out;
> +
> +               idx_table = calloc(nr_members, sizeof(*idx_table));
> +               if (idx_table == NULL)
> +                       goto out;
> +
> +               /*
> +                * Build an index table for each evsel to the val array.
> +                * It cannot use evsel->core.idx because removed events might
> +                * create a hole so the index is not consecutive anymore.
> +                */
> +               idx = 0;
> +               for_each_group_member(member, evsel)
> +                       idx_table[idx++] = member;
>
>                 /* collect values in the group members */
>                 list_for_each_entry(pair, &he->pairs.head, pairs.node) {
> @@ -66,8 +81,15 @@ static int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
>                         if (!total)
>                                 continue;
>
> -                       evsel = hists_to_evsel(pair->hists);
> -                       idx = evsel__group_idx(evsel);
> +                       member = hists_to_evsel(pair->hists);
> +                       for (idx = 0; idx < nr_members; idx++) {
> +                               if (idx_table[idx] == member)
> +                                       break;
> +                       }
> +
> +                       /* this should not happen */
> +                       if (idx == nr_members)
> +                               continue;
>
>                         if (fmt_percent)
>                                 val[idx].percent = 100.0 * period / total;
> @@ -75,8 +97,7 @@ static int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
>                                 val[idx].period = period;
>                 }
>
> -               /* idx starts from 1 to skip the leader event */
> -               for (idx = 1; idx < nr_members; idx++) {
> +               for (idx = 0; idx < nr_members; idx++) {
>                         if (fmt_percent) {
>                                 ret += hpp__call_print_fn(hpp, print_fn,
>                                                           fmt, len, val[idx].percent);
> @@ -89,6 +110,7 @@ static int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
>                 free(val);
>         }
>
> +out:
>         /*
>          * Restore original buf and size as it's where caller expects
>          * the result will be saved.
> --
> 2.43.0.687.g38aa6559b0-goog
>