Re: [RFC PATCH v5 15/16] perf stat: use tool event helper function in metricgroup__build_event_string

[Ian Rogers]

On Fri, Apr 12, 2024 at 2:08 PM <weilin.wang@xxxxxxxxx> wrote:
>
> From: Weilin Wang <weilin.wang@xxxxxxxxx>
>
> We want to include all used tool events in each event group to ensure
> sharing events so that help improve multiplexing.
>
> This change updates parse_id and metricgroupg__build_event_string to use

nit: typo in metricgroup__build_event_string

> the get_tool_event_str helper function to generate tool event string
> instead of inserting temporary tool event ids and generate string from
> the tool event ids.
>
> Signed-off-by: Weilin Wang <weilin.wang@xxxxxxxxx>
> ---
>  tools/perf/util/metricgroup.c | 61 +++++++++++++----------------------
>  1 file changed, 22 insertions(+), 39 deletions(-)
>
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 86b6528e5a44..39746d18f078 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -774,6 +774,18 @@ static int get_tool_event_str(struct strbuf *events,
>         int i = 0;
>         int ret;
>
> +       /*
> +        * We may fail to share events between metrics because a tool
> +        * event isn't present in one metric. For example, a ratio of
> +        * cache misses doesn't need duration_time but the same events
> +        * may be used for a misses per second. Events without sharing
> +        * implies multiplexing, that is best avoided, so place
> +        * all tool events in every group.
> +        * This function helps place all tool events in every group by
> +        * generating the tool event strbuf that to be added in event
> +        * group strings.
> +        */
> +
>         perf_tool_event__for_each_event(i) {
>                 if (tool_events[i]) {
>                         const char *tmp = strdup(perf_tool_event__to_str(i));
> @@ -795,15 +807,18 @@ static int get_tool_event_str(struct strbuf *events,
>  static int metricgroup__build_event_string(struct strbuf *events,
>                                            const struct expr_parse_ctx *ctx,
>                                            const char *modifier,
> -                                          bool group_events)
> +                                          bool group_events,
> +                                          const bool tool_events[PERF_TOOL_MAX])
>  {
>         struct hashmap_entry *cur;
>         size_t bkt;
>         bool no_group = true, has_tool_events = false;
> -       bool tool_events[PERF_TOOL_MAX] = {false};
>         int ret = 0;
> +       struct strbuf tool_event_str = STRBUF_INIT;
>
>  #define RETURN_IF_NON_ZERO(x) do { if (x) return x; } while (0)
> +       ret = get_tool_event_str(&tool_event_str, tool_events, &has_tool_events);
> +       RETURN_IF_NON_ZERO(ret);
>
>         hashmap__for_each_entry(ctx->ids, cur, bkt) {
>                 const char *sep, *rsep, *id = cur->pkey;
> @@ -814,8 +829,6 @@ static int metricgroup__build_event_string(struct strbuf *events,
>                 /* Always move tool events outside of the group. */
>                 ev = perf_tool_event__from_str(id);
>                 if (ev != PERF_TOOL_NONE) {
> -                       has_tool_events = true;
> -                       tool_events[ev] = true;
>                         continue;
>                 }
>                 /* Separate events with commas and open the group if necessary. */
> @@ -879,19 +892,8 @@ static int metricgroup__build_event_string(struct strbuf *events,
>                 RETURN_IF_NON_ZERO(ret);
>         }
>         if (has_tool_events) {
> -               int i;
> -
> -               perf_tool_event__for_each_event(i) {
> -                       if (tool_events[i]) {
> -                               if (!no_group) {
> -                                       ret = strbuf_addch(events, ',');
> -                                       RETURN_IF_NON_ZERO(ret);
> -                               }
> -                               no_group = false;
> -                               ret = strbuf_addstr(events, perf_tool_event__to_str(i));
> -                               RETURN_IF_NON_ZERO(ret);
> -                       }
> -               }
> +               ret = strbuf_addstr(events, tool_event_str.buf);
> +               RETURN_IF_NON_ZERO(ret);
>         }
>
>         return ret;
> @@ -2421,32 +2423,13 @@ static int parse_ids(bool metric_no_merge, struct perf_pmu *fake_pmu,
>
>         *out_evlist = NULL;
>         if (!metric_no_merge || hashmap__size(ids->ids) == 0) {
> -               bool added_event = false;
> -               int i;
>                 /*
> -                * We may fail to share events between metrics because a tool
> -                * event isn't present in one metric. For example, a ratio of
> -                * cache misses doesn't need duration_time but the same events
> -                * may be used for a misses per second. Events without sharing
> -                * implies multiplexing, that is best avoided, so place
> -                * all tool events in every group.
> -                *
> -                * Also, there may be no ids/events in the expression parsing
> +                * There may be no ids/events in the expression parsing
>                  * context because of constant evaluation, e.g.:
>                  *    event1 if #smt_on else 0
>                  * Add a tool event to avoid a parse error on an empty string.
>                  */
> -               perf_tool_event__for_each_event(i) {
> -                       if (tool_events[i]) {
> -                               char *tmp = strdup(perf_tool_event__to_str(i));
> -
> -                               if (!tmp)
> -                                       return -ENOMEM;
> -                               ids__insert(ids->ids, tmp);
> -                               added_event = true;
> -                       }
> -               }
> -               if (!added_event && hashmap__size(ids->ids) == 0) {
> +               if (hashmap__size(ids->ids) == 0) {
>                         char *tmp = strdup("duration_time");
>
>                         if (!tmp)
> @@ -2455,7 +2438,7 @@ static int parse_ids(bool metric_no_merge, struct perf_pmu *fake_pmu,
>                 }
>         }
>         ret = metricgroup__build_event_string(&events, ids, modifier,
> -                                             group_events);
> +                                             group_events, tool_events);
>         if (ret)
>                 return ret;
>
> --
> 2.42.0
>