[v1,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.
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/util/stat-shadow.c | 75 +++++++++++++++--------------------
1 file changed, 33 insertions(+), 42 deletions(-)
Comments
On Thu, Feb 1, 2024 at 6:25 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.
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
> tools/perf/util/stat-shadow.c | 75 +++++++++++++++--------------------
> 1 file changed, 33 insertions(+), 42 deletions(-)
>
> diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
> index e31426167852..f6c9d2f98835 100644
> --- a/tools/perf/util/stat-shadow.c
> +++ b/tools/perf/util/stat-shadow.c
> @@ -355,23 +355,22 @@ 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)
> {
> int i;
You may add local variables with the same name to reduce
the amount of diff.
Thanks,
Namhyung
>
> - for (i = 0; metric_events[i]; i++) {
> + for (i = 0; mexp->metric_events[i]; i++) {
> char *n;
> double val;
> int source_count = 0;
>
> - if (evsel__is_tool(metric_events[i])) {
> + if (evsel__is_tool(mexp->metric_events[i])) {
> struct stats *stats;
> double scale;
>
> - switch (metric_events[i]->tool_event) {
> + switch (mexp->metric_events[i]->tool_event) {
> case PERF_TOOL_DURATION_TIME:
> stats = &walltime_nsecs_stats;
> scale = 1e-9;
> @@ -391,19 +390,20 @@ static int prepare_metric(struct evsel **metric_events,
> pr_err("Invalid tool event 'max'");
> abort();
> default:
> - pr_err("Unknown tool event '%s'", evsel__name(metric_events[i]));
> + pr_err("Unknown tool event '%s'",
> + evsel__name(mexp->metric_events[i]));
> abort();
> }
> val = avg_stats(stats) * scale;
> source_count = 1;
> } else {
> - struct perf_stat_evsel *ps = metric_events[i]->stats;
> + struct perf_stat_evsel *ps = mexp->metric_events[i]->stats;
> struct perf_stat_aggr *aggr = &ps->aggr[aggr_idx];
>
> if (!aggr)
> break;
>
> - if (!metric_events[i]->supported) {
> + if (!mexp->metric_events[i]->supported) {
> /*
> * Not supported events will have a count of 0,
> * which can be confusing in a
> @@ -419,19 +419,19 @@ static int prepare_metric(struct evsel **metric_events,
> * reverse the scale before computing the
> * metric.
> */
> - val = aggr->counts.val * (1.0 / metric_events[i]->scale);
> - source_count = evsel__source_count(metric_events[i]);
> + val = aggr->counts.val * (1.0 / mexp->metric_events[i]->scale);
> + source_count = evsel__source_count(mexp->metric_events[i]);
> }
> }
> - n = strdup(evsel__metric_id(metric_events[i]));
> + n = strdup(evsel__metric_id(mexp->metric_events[i]));
> if (!n)
> return -ENOMEM;
>
> expr__add_id_val_source_count(pctx, n, val, source_count);
> }
>
> - for (int j = 0; metric_refs && metric_refs[j].metric_name; j++) {
> - int ret = expr__add_ref(pctx, &metric_refs[j]);
> + for (int j = 0; mexp->metric_refs && mexp->metric_refs[j].metric_name; j++) {
> + int ret = expr__add_ref(pctx, &mexp->metric_refs[j]);
>
> if (ret)
> return ret;
> @@ -441,14 +441,8 @@ 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)
> {
> @@ -465,55 +459,55 @@ static void generic_metric(struct perf_stat_config *config,
>
> if (config->user_requested_cpu_list)
> pctx->sctx.user_requested_cpu_list = strdup(config->user_requested_cpu_list);
> - pctx->sctx.runtime = runtime;
> + pctx->sctx.runtime = mexp->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;
> }
> - if (!metric_events[i]) {
> - if (expr__parse(&ratio, pctx, metric_expr) == 0) {
> + if (!mexp->metric_events[i]) {
> + if (expr__parse(&ratio, pctx, mexp->metric_expr) == 0) {
> char *unit;
> char metric_bf[64];
>
> - if (metric_threshold &&
> - expr__parse(&threshold, pctx, metric_threshold) == 0 &&
> + if (mexp->metric_threshold &&
> + expr__parse(&threshold, pctx, mexp->metric_threshold) == 0 &&
> !isnan(threshold)) {
> color = fpclassify(threshold) == FP_ZERO
> ? PERF_COLOR_GREEN : PERF_COLOR_RED;
> }
>
> - if (metric_unit && metric_name) {
> - if (perf_pmu__convert_scale(metric_unit,
> + if (mexp->metric_unit && mexp->metric_name) {
> + if (perf_pmu__convert_scale(mexp->metric_unit,
> &unit, &scale) >= 0) {
> ratio *= scale;
> }
> - if (strstr(metric_expr, "?"))
> + if (strstr(mexp->metric_expr, "?"))
> scnprintf(metric_bf, sizeof(metric_bf),
> - "%s %s_%d", unit, metric_name, runtime);
> + "%s %s_%d", unit, mexp->metric_name, mexp->runtime);
> else
> scnprintf(metric_bf, sizeof(metric_bf),
> - "%s %s", unit, metric_name);
> + "%s %s", unit, mexp->metric_name);
>
> print_metric(config, ctxp, color, "%8.1f",
> metric_bf, ratio);
> } else {
> print_metric(config, ctxp, color, "%8.2f",
> - metric_name ?
> - metric_name :
> - out->force_header ? name : "",
> + mexp->metric_name ?
> + mexp->metric_name :
> + out->force_header ? evsel->name : "",
> ratio);
> }
> } else {
> print_metric(config, ctxp, color, /*unit=*/NULL,
> out->force_header ?
> - (metric_name ? metric_name : name) : "", 0);
> + (mexp->metric_name ?: evsel->name) : "", 0);
> }
> } else {
> print_metric(config, ctxp, color, /*unit=*/NULL,
> out->force_header ?
> - (metric_name ? metric_name : name) : "", 0);
> + (mexp->metric_name ?: evsel->name) : "", 0);
> }
>
> expr__ctx_free(pctx);
> @@ -528,7 +522,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 +624,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
>
On Mon, Feb 5, 2024 at 5:59 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Thu, Feb 1, 2024 at 6:25 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.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> > tools/perf/util/stat-shadow.c | 75 +++++++++++++++--------------------
> > 1 file changed, 33 insertions(+), 42 deletions(-)
> >
> > diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
> > index e31426167852..f6c9d2f98835 100644
> > --- a/tools/perf/util/stat-shadow.c
> > +++ b/tools/perf/util/stat-shadow.c
> > @@ -355,23 +355,22 @@ 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)
> > {
> > int i;
>
> You may add local variables with the same name to reduce
> the amount of diff.
I'll see what I can do in v2.
Thanks,
Ian
> Thanks,
> Namhyung
>
> >
> > - for (i = 0; metric_events[i]; i++) {
> > + for (i = 0; mexp->metric_events[i]; i++) {
> > char *n;
> > double val;
> > int source_count = 0;
> >
> > - if (evsel__is_tool(metric_events[i])) {
> > + if (evsel__is_tool(mexp->metric_events[i])) {
> > struct stats *stats;
> > double scale;
> >
> > - switch (metric_events[i]->tool_event) {
> > + switch (mexp->metric_events[i]->tool_event) {
> > case PERF_TOOL_DURATION_TIME:
> > stats = &walltime_nsecs_stats;
> > scale = 1e-9;
> > @@ -391,19 +390,20 @@ static int prepare_metric(struct evsel **metric_events,
> > pr_err("Invalid tool event 'max'");
> > abort();
> > default:
> > - pr_err("Unknown tool event '%s'", evsel__name(metric_events[i]));
> > + pr_err("Unknown tool event '%s'",
> > + evsel__name(mexp->metric_events[i]));
> > abort();
> > }
> > val = avg_stats(stats) * scale;
> > source_count = 1;
> > } else {
> > - struct perf_stat_evsel *ps = metric_events[i]->stats;
> > + struct perf_stat_evsel *ps = mexp->metric_events[i]->stats;
> > struct perf_stat_aggr *aggr = &ps->aggr[aggr_idx];
> >
> > if (!aggr)
> > break;
> >
> > - if (!metric_events[i]->supported) {
> > + if (!mexp->metric_events[i]->supported) {
> > /*
> > * Not supported events will have a count of 0,
> > * which can be confusing in a
> > @@ -419,19 +419,19 @@ static int prepare_metric(struct evsel **metric_events,
> > * reverse the scale before computing the
> > * metric.
> > */
> > - val = aggr->counts.val * (1.0 / metric_events[i]->scale);
> > - source_count = evsel__source_count(metric_events[i]);
> > + val = aggr->counts.val * (1.0 / mexp->metric_events[i]->scale);
> > + source_count = evsel__source_count(mexp->metric_events[i]);
> > }
> > }
> > - n = strdup(evsel__metric_id(metric_events[i]));
> > + n = strdup(evsel__metric_id(mexp->metric_events[i]));
> > if (!n)
> > return -ENOMEM;
> >
> > expr__add_id_val_source_count(pctx, n, val, source_count);
> > }
> >
> > - for (int j = 0; metric_refs && metric_refs[j].metric_name; j++) {
> > - int ret = expr__add_ref(pctx, &metric_refs[j]);
> > + for (int j = 0; mexp->metric_refs && mexp->metric_refs[j].metric_name; j++) {
> > + int ret = expr__add_ref(pctx, &mexp->metric_refs[j]);
> >
> > if (ret)
> > return ret;
> > @@ -441,14 +441,8 @@ 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)
> > {
> > @@ -465,55 +459,55 @@ static void generic_metric(struct perf_stat_config *config,
> >
> > if (config->user_requested_cpu_list)
> > pctx->sctx.user_requested_cpu_list = strdup(config->user_requested_cpu_list);
> > - pctx->sctx.runtime = runtime;
> > + pctx->sctx.runtime = mexp->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;
> > }
> > - if (!metric_events[i]) {
> > - if (expr__parse(&ratio, pctx, metric_expr) == 0) {
> > + if (!mexp->metric_events[i]) {
> > + if (expr__parse(&ratio, pctx, mexp->metric_expr) == 0) {
> > char *unit;
> > char metric_bf[64];
> >
> > - if (metric_threshold &&
> > - expr__parse(&threshold, pctx, metric_threshold) == 0 &&
> > + if (mexp->metric_threshold &&
> > + expr__parse(&threshold, pctx, mexp->metric_threshold) == 0 &&
> > !isnan(threshold)) {
> > color = fpclassify(threshold) == FP_ZERO
> > ? PERF_COLOR_GREEN : PERF_COLOR_RED;
> > }
> >
> > - if (metric_unit && metric_name) {
> > - if (perf_pmu__convert_scale(metric_unit,
> > + if (mexp->metric_unit && mexp->metric_name) {
> > + if (perf_pmu__convert_scale(mexp->metric_unit,
> > &unit, &scale) >= 0) {
> > ratio *= scale;
> > }
> > - if (strstr(metric_expr, "?"))
> > + if (strstr(mexp->metric_expr, "?"))
> > scnprintf(metric_bf, sizeof(metric_bf),
> > - "%s %s_%d", unit, metric_name, runtime);
> > + "%s %s_%d", unit, mexp->metric_name, mexp->runtime);
> > else
> > scnprintf(metric_bf, sizeof(metric_bf),
> > - "%s %s", unit, metric_name);
> > + "%s %s", unit, mexp->metric_name);
> >
> > print_metric(config, ctxp, color, "%8.1f",
> > metric_bf, ratio);
> > } else {
> > print_metric(config, ctxp, color, "%8.2f",
> > - metric_name ?
> > - metric_name :
> > - out->force_header ? name : "",
> > + mexp->metric_name ?
> > + mexp->metric_name :
> > + out->force_header ? evsel->name : "",
> > ratio);
> > }
> > } else {
> > print_metric(config, ctxp, color, /*unit=*/NULL,
> > out->force_header ?
> > - (metric_name ? metric_name : name) : "", 0);
> > + (mexp->metric_name ?: evsel->name) : "", 0);
> > }
> > } else {
> > print_metric(config, ctxp, color, /*unit=*/NULL,
> > out->force_header ?
> > - (metric_name ? metric_name : name) : "", 0);
> > + (mexp->metric_name ?: evsel->name) : "", 0);
> > }
> >
> > expr__ctx_free(pctx);
> > @@ -528,7 +522,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 +624,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,23 +355,22 @@ 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)
{
int i;
- for (i = 0; metric_events[i]; i++) {
+ for (i = 0; mexp->metric_events[i]; i++) {
char *n;
double val;
int source_count = 0;
- if (evsel__is_tool(metric_events[i])) {
+ if (evsel__is_tool(mexp->metric_events[i])) {
struct stats *stats;
double scale;
- switch (metric_events[i]->tool_event) {
+ switch (mexp->metric_events[i]->tool_event) {
case PERF_TOOL_DURATION_TIME:
stats = &walltime_nsecs_stats;
scale = 1e-9;
@@ -391,19 +390,20 @@ static int prepare_metric(struct evsel **metric_events,
pr_err("Invalid tool event 'max'");
abort();
default:
- pr_err("Unknown tool event '%s'", evsel__name(metric_events[i]));
+ pr_err("Unknown tool event '%s'",
+ evsel__name(mexp->metric_events[i]));
abort();
}
val = avg_stats(stats) * scale;
source_count = 1;
} else {
- struct perf_stat_evsel *ps = metric_events[i]->stats;
+ struct perf_stat_evsel *ps = mexp->metric_events[i]->stats;
struct perf_stat_aggr *aggr = &ps->aggr[aggr_idx];
if (!aggr)
break;
- if (!metric_events[i]->supported) {
+ if (!mexp->metric_events[i]->supported) {
/*
* Not supported events will have a count of 0,
* which can be confusing in a
@@ -419,19 +419,19 @@ static int prepare_metric(struct evsel **metric_events,
* reverse the scale before computing the
* metric.
*/
- val = aggr->counts.val * (1.0 / metric_events[i]->scale);
- source_count = evsel__source_count(metric_events[i]);
+ val = aggr->counts.val * (1.0 / mexp->metric_events[i]->scale);
+ source_count = evsel__source_count(mexp->metric_events[i]);
}
}
- n = strdup(evsel__metric_id(metric_events[i]));
+ n = strdup(evsel__metric_id(mexp->metric_events[i]));
if (!n)
return -ENOMEM;
expr__add_id_val_source_count(pctx, n, val, source_count);
}
- for (int j = 0; metric_refs && metric_refs[j].metric_name; j++) {
- int ret = expr__add_ref(pctx, &metric_refs[j]);
+ for (int j = 0; mexp->metric_refs && mexp->metric_refs[j].metric_name; j++) {
+ int ret = expr__add_ref(pctx, &mexp->metric_refs[j]);
if (ret)
return ret;
@@ -441,14 +441,8 @@ 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)
{
@@ -465,55 +459,55 @@ static void generic_metric(struct perf_stat_config *config,
if (config->user_requested_cpu_list)
pctx->sctx.user_requested_cpu_list = strdup(config->user_requested_cpu_list);
- pctx->sctx.runtime = runtime;
+ pctx->sctx.runtime = mexp->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;
}
- if (!metric_events[i]) {
- if (expr__parse(&ratio, pctx, metric_expr) == 0) {
+ if (!mexp->metric_events[i]) {
+ if (expr__parse(&ratio, pctx, mexp->metric_expr) == 0) {
char *unit;
char metric_bf[64];
- if (metric_threshold &&
- expr__parse(&threshold, pctx, metric_threshold) == 0 &&
+ if (mexp->metric_threshold &&
+ expr__parse(&threshold, pctx, mexp->metric_threshold) == 0 &&
!isnan(threshold)) {
color = fpclassify(threshold) == FP_ZERO
? PERF_COLOR_GREEN : PERF_COLOR_RED;
}
- if (metric_unit && metric_name) {
- if (perf_pmu__convert_scale(metric_unit,
+ if (mexp->metric_unit && mexp->metric_name) {
+ if (perf_pmu__convert_scale(mexp->metric_unit,
&unit, &scale) >= 0) {
ratio *= scale;
}
- if (strstr(metric_expr, "?"))
+ if (strstr(mexp->metric_expr, "?"))
scnprintf(metric_bf, sizeof(metric_bf),
- "%s %s_%d", unit, metric_name, runtime);
+ "%s %s_%d", unit, mexp->metric_name, mexp->runtime);
else
scnprintf(metric_bf, sizeof(metric_bf),
- "%s %s", unit, metric_name);
+ "%s %s", unit, mexp->metric_name);
print_metric(config, ctxp, color, "%8.1f",
metric_bf, ratio);
} else {
print_metric(config, ctxp, color, "%8.2f",
- metric_name ?
- metric_name :
- out->force_header ? name : "",
+ mexp->metric_name ?
+ mexp->metric_name :
+ out->force_header ? evsel->name : "",
ratio);
}
} else {
print_metric(config, ctxp, color, /*unit=*/NULL,
out->force_header ?
- (metric_name ? metric_name : name) : "", 0);
+ (mexp->metric_name ?: evsel->name) : "", 0);
}
} else {
print_metric(config, ctxp, color, /*unit=*/NULL,
out->force_header ?
- (metric_name ? metric_name : name) : "", 0);
+ (mexp->metric_name ?: evsel->name) : "", 0);
}
expr__ctx_free(pctx);
@@ -528,7 +522,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 +624,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;