Re: [RFC PATCH v1 10/14] perf stat: Implement streaming JSON formatting callbacks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The JSON metric output permanently drops the metric unit string, improperly placing the metric name inside the 'metric-unit' field.
- [Medium] Dynamic memory allocation in `json_metric_only_print_metric` contradicts the commit's 'zero-allocation' claim and introduces unnecessary overhead.
--

commit 739e10b4f20e27c02302af087d48ca22809c20c9
Author: Ian Rogers <irogers@google.com>

perf stat: Implement streaming JSON formatting callbacks

This patch implements streaming JSON formatting callbacks inside
util/stat-print-json.c, replacing previous empty stubs. It bypasses
dynamic queue events and metric lists entirely, formatting JSON objects
directly onto the output file descriptor.

> Delivers a highly optimized, zero-allocation, and 100% streaming print engine
> for JSON normal and metric-only modes. It bypasses dynamic queue events and
> metric lists entirely, formatting and streaming JSON objects directly onto the
> output file descriptor.

[ ... ]

> --- a/tools/perf/util/stat-print-json.c
> +++ b/tools/perf/util/stat-print-json.c

[ ... ]

> +static int json_print_metric(void *ctx, const struct perf_stat_config *config, struct evsel *evsel,
> +			     int aggr_idx, const char *name, const char *unit __maybe_unused,
> +			     double val, enum metric_threshold_classify thresh)
> +{

[ ... ]

> +	fprintf(output, ", \"metric-value\" : \"%f\"", val);
> +	if (name && name[0])
> +		fprintf(output, ", \"metric-unit\" : \"%s\"", name);

[Severity: High]
Does this inadvertently drop the metric unit string?

The unit argument is marked __maybe_unused, and the name argument is printed
in the "metric-unit" JSON field instead. This seems like it might break
downstream JSON parsers that expect the actual unit string.

[ ... ]

> +static int json_metric_only_print_metric(void *ctx,
> +					 const struct perf_stat_config *config __maybe_unused,
> +					 struct evsel *evsel __maybe_unused, int aggr_idx,
> +					 const char *name, const char *unit, double val,
> +					 enum metric_threshold_classify thresh __maybe_unused)
> +{
> +	struct json_metric_only_print_state *ps = ctx;
> +	FILE *output = ps->fp;
> +	char *header_name;
> +	int err;

[ ... ]

> +	if (unit && unit[0]) {
> +		err = asprintf(&header_name, "%s  %s", unit, name);
> +	} else {
> +		header_name = strdup(name);
> +		err = header_name ? 0 : -1;
> +	}
> +	if (err < 0)
> +		return -ENOMEM;
> +
> +	fprintf(output, "\"%s\" : \"%.1f\"", header_name, val);
> +	free(header_name);
> +	return 0;
> +}

[Severity: Medium]
The commit message states this delivers a "zero-allocation" print engine,
but memory is dynamically allocated here using asprintf() and strdup() on
every invocation.

Could these allocations be avoided to prevent a potential -ENOMEM failure on
the fast path? It looks like the combined string could be formatted directly
into the stream without intermediate allocations.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260522223342.2393553-1-irogers@google.com?part=10