On Fri, Apr 12, 2024 at 2:08 PM <weilin.wang@intel.com> wrote:
>
> From: Weilin Wang <weilin.wang@intel.com>
>
> 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@intel.com>
> ---
> 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
>