[v2,1/3] perf stat: Pass fewer metric arguments
Commit Message
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@google.com>
---
tools/perf/util/stat-shadow.c | 38 +++++++++++++++++------------------
1 file changed, 18 insertions(+), 20 deletions(-)
Comments
On Mon, Feb 5, 2024 at 8:32 PM Ian Rogers <irogers@google.com> 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@google.com>
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@kernel.org>
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
>
@@ -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;