Re: [PATCH v2 1/3] perf stat: Pass fewer metric arguments

[Namhyung Kim]

On Mon, Feb 5, 2024 at 8:32 PM Ian Rogers <irogers@xxxxxxxxxx> wrote:
>
> Pass metric_expr and evsel rather than specific variables from the
> struct, thereby reducing the number of arguments. This will enable
> later fixes.
>
> To reduce the size of the diff, local variables are added to match the
> previous parameter names. This isn't done in the case of "name" as
> evsel->name is more intention revealing. A whitespace issue is also
> addressed.
>
> Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>

This doesn't apply to perf-tools-next anymore.  Can you please refresh?
Anyway the change looks good to me now.  For the series:

  Acked-by: Namhyung Kim <namhyung@xxxxxxxxxx>

Thanks,
Namhyung

> ---
>  tools/perf/util/stat-shadow.c | 38 +++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 20 deletions(-)
>
> diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
> index e31426167852..b3a25659b9e6 100644
> --- a/tools/perf/util/stat-shadow.c
> +++ b/tools/perf/util/stat-shadow.c
> @@ -355,11 +355,12 @@ static void print_nsecs(struct perf_stat_config *config,
>                 print_metric(config, ctxp, NULL, NULL, "CPUs utilized", 0);
>  }
>
> -static int prepare_metric(struct evsel **metric_events,
> -                         struct metric_ref *metric_refs,
> +static int prepare_metric(const struct metric_expr *mexp,
>                           struct expr_parse_ctx *pctx,
>                           int aggr_idx)
>  {
> +       struct evsel * const *metric_events = mexp->metric_events;
> +       struct metric_ref *metric_refs = mexp->metric_refs;
>         int i;
>
>         for (i = 0; metric_events[i]; i++) {
> @@ -403,7 +404,7 @@ static int prepare_metric(struct evsel **metric_events,
>                         if (!aggr)
>                                 break;
>
> -                        if (!metric_events[i]->supported) {
> +                       if (!metric_events[i]->supported) {
>                                 /*
>                                  * Not supported events will have a count of 0,
>                                  * which can be confusing in a
> @@ -441,18 +442,18 @@ static int prepare_metric(struct evsel **metric_events,
>  }
>
>  static void generic_metric(struct perf_stat_config *config,
> -                          const char *metric_expr,
> -                          const char *metric_threshold,
> -                          struct evsel **metric_events,
> -                          struct metric_ref *metric_refs,
> -                          char *name,
> -                          const char *metric_name,
> -                          const char *metric_unit,
> -                          int runtime,
> +                          struct metric_expr *mexp,
> +                          struct evsel *evsel,
>                            int aggr_idx,
>                            struct perf_stat_output_ctx *out)
>  {
>         print_metric_t print_metric = out->print_metric;
> +       const char *metric_name = mexp->metric_name;
> +       const char *metric_expr = mexp->metric_expr;
> +       const char *metric_threshold = mexp->metric_threshold;
> +       const char *metric_unit = mexp->metric_unit;
> +       struct evsel * const *metric_events = mexp->metric_events;
> +       int runtime = mexp->runtime;
>         struct expr_parse_ctx *pctx;
>         double ratio, scale, threshold;
>         int i;
> @@ -467,7 +468,7 @@ static void generic_metric(struct perf_stat_config *config,
>                 pctx->sctx.user_requested_cpu_list = strdup(config->user_requested_cpu_list);
>         pctx->sctx.runtime = runtime;
>         pctx->sctx.system_wide = config->system_wide;
> -       i = prepare_metric(metric_events, metric_refs, pctx, aggr_idx);
> +       i = prepare_metric(mexp, pctx, aggr_idx);
>         if (i < 0) {
>                 expr__ctx_free(pctx);
>                 return;
> @@ -502,18 +503,18 @@ static void generic_metric(struct perf_stat_config *config,
>                                 print_metric(config, ctxp, color, "%8.2f",
>                                         metric_name ?
>                                         metric_name :
> -                                       out->force_header ?  name : "",
> +                                       out->force_header ?  evsel->name : "",
>                                         ratio);
>                         }
>                 } else {
>                         print_metric(config, ctxp, color, /*unit=*/NULL,
>                                      out->force_header ?
> -                                    (metric_name ? metric_name : name) : "", 0);
> +                                    (metric_name ?: evsel->name) : "", 0);
>                 }
>         } else {
>                 print_metric(config, ctxp, color, /*unit=*/NULL,
>                              out->force_header ?
> -                            (metric_name ? metric_name : name) : "", 0);
> +                            (metric_name ?: evsel->name) : "", 0);
>         }
>
>         expr__ctx_free(pctx);
> @@ -528,7 +529,7 @@ double test_generic_metric(struct metric_expr *mexp, int aggr_idx)
>         if (!pctx)
>                 return NAN;
>
> -       if (prepare_metric(mexp->metric_events, mexp->metric_refs, pctx, aggr_idx) < 0)
> +       if (prepare_metric(mexp, pctx, aggr_idx) < 0)
>                 goto out;
>
>         if (expr__parse(&ratio, pctx, mexp->metric_expr))
> @@ -630,10 +631,7 @@ void *perf_stat__print_shadow_stats_metricgroup(struct perf_stat_config *config,
>
>                 if ((*num)++ > 0)
>                         out->new_line(config, ctxp);
> -               generic_metric(config, mexp->metric_expr, mexp->metric_threshold,
> -                              mexp->metric_events, mexp->metric_refs, evsel->name,
> -                              mexp->metric_name, mexp->metric_unit, mexp->runtime,
> -                              aggr_idx, out);
> +               generic_metric(config, mexp, evsel, aggr_idx, out);
>         }
>
>         return NULL;
> --
> 2.43.0.594.gd9cf4e227d-goog
>