[PATCH] perf stat: Make metric only column line up with header

[PATCH] perf stat: Make metric only column line up with header

[Andi Kleen]

Since some time the metric-only output columns are messed up and do not
line up with the header, which makes it hard to read. I haven't bisected
it, but presumably it was broken for some time.

There were multiple problems:
- The dummy pm invocation at the beginning did print a bogus field
- The column computation in pm did not agree with the header length
- The color escape strings from highlighting confuse printf's field
  length computation

Fix all those. I simplified the column width computation significantly,
ignoring EVNAME_LEN, MGROUP_LEN, config->unit_width which don't
seem to be useful in the metric only context. It now only uses
the actual unit width as well as config->metric_only_len. The result
is more code removed than added.

Before:

% perf stat --topdown -a -I 1000
     1.000190386                                         45.5                   40.0                     5.3                     9.2
     2.005185654                                         45.3                   40.1                     5.6                     9.0
     3.009193207                                         45.4                   39.9                     5.6                     9.1

After:

% perf stat --topdown -a -I 1000
     1.000810024                 46.3                 39.7                   5.3                 8.7
     2.004800656                 45.8                 39.8                   5.4                 8.9
     3.008804783                 46.0                 39.6                   5.4                 9.0

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 tools/perf/util/stat-display.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index 993f4c4b8f44..402611ecaf42 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -580,16 +580,13 @@ static void print_metricgroup_header_std(struct perf_stat_config *config,
 					 const char *metricgroup_name)
 {
 	struct outstate *os = ctx;
-	int n;
 
 	if (!metricgroup_name) {
 		__new_line_std(config, os);
 		return;
 	}
 
-	n = fprintf(config->output, " %*s", EVNAME_LEN, metricgroup_name);
-
-	fprintf(config->output, "%*s", MGROUP_LEN + config->unit_width + 2 - n, "");
+	fprintf(config->output, " %*s", config->metric_only_len, metricgroup_name);
 }
 
 static void print_metric_only(struct perf_stat_config *config,
@@ -599,19 +596,20 @@ static void print_metric_only(struct perf_stat_config *config,
 	struct outstate *os = ctx;
 	FILE *out = os->fh;
 	char str[1024];
-	unsigned mlen = config->metric_only_len;
+	unsigned mlen;
 	const char *color = metric_threshold_classify__color(thresh);
+	int olen;
 
-	if (!unit)
-		unit = "";
-	if (mlen < strlen(unit))
-		mlen = strlen(unit) + 1;
+	if (!unit) {
+		os->first = false;
+		return;
+	}
 
-	if (color)
-		mlen += strlen(color) + sizeof(PERF_COLOR_RESET) - 1;
+	mlen = max_t(unsigned, strlen(unit), config->metric_only_len);
 
+	olen = snprintf(str, sizeof(str), fmt ?: "", val);
 	color_snprintf(str, sizeof(str), color ?: "", fmt ?: "", val);
-	fprintf(out, "%*s ", mlen, str);
+	fprintf(out, "%*s%s", max_t(int, mlen - olen, 1), "", str);
 	os->first = false;
 }
 
@@ -633,7 +631,7 @@ static void print_metric_only_csv(struct perf_stat_config *config __maybe_unused
 	while (isdigit(*ends) || *ends == '.')
 		ends++;
 	*ends = 0;
-	fprintf(out, "%s%s", vals, config->csv_sep);
+	fprintf(out, "%s%s ", vals, config->csv_sep);
 	os->first = false;
 }
 
-- 
2.54.0

Re: [PATCH] perf stat: Make metric only column line up with header

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Corruption of default metricgroup alignment in standard (non-metric-only) output.
- [High] Extraneous trailing space added to CSV output breaks machine readability.
- [Medium] Off-by-one column misalignment in metric-only standard output.
--

commit 29839374a38b0f40e2eb452aeaffa530277247ff
Author: Andi Kleen <ak@linux.intel.com>

perf stat: Make metric only column line up with header

This commit attempts to fix column alignment in the --metric-only output
mode. It simplifies the column width computation and adjusts string
formatting for both standard and CSV outputs.

> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index 993f4c4b8f442..402611ecaf423 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -580,16 +580,13 @@ static void print_metricgroup_header_std(struct perf_stat_config *config,
>  					 const char *metricgroup_name)
>  {
[ ... ]
> -	n = fprintf(config->output, " %*s", EVNAME_LEN, metricgroup_name);
> -
> -	fprintf(config->output, "%*s", MGROUP_LEN + config->unit_width + 2 - n, "");
> +	fprintf(config->output, " %*s", config->metric_only_len, metricgroup_name);
>  }

Does this change break tabular alignment for default metricgroups in standard
non-metric-only mode?

The commit message states that EVNAME_LEN and MGROUP_LEN are not useful in
the metric only context. However, print_metricgroup_header_std() is
completely bypassed in --metric-only mode.

In standard mode, this function uses these lengths to pad the metricgroup
name and align it with the preceding counter values. Replacing this padding
with config->metric_only_len could shift all subsequent metrics to the left.

> @@ -599,19 +596,20 @@ static void print_metric_only(struct perf_stat_config *config,
[ ... ]
> +	olen = snprintf(str, sizeof(str), fmt ?: "", val);
>  	color_snprintf(str, sizeof(str), color ?: "", fmt ?: "", val);
> -	fprintf(out, "%*s ", mlen, str);
> +	fprintf(out, "%*s%s", max_t(int, mlen - olen, 1), "", str);
>  	os->first = false;
>  }

Could this lead to an off-by-one misalignment in the data columns?

This new formatting removes the trailing space and outputs exactly mlen
visual characters. However, the header formatting in print_metric_header()
was left unchanged and still outputs a trailing space:

tools/perf/util/stat-display.c:print_metric_header() {
    ...
    fprintf(os->fh, "%*s ", config->metric_only_len, unit);
    ...
}

Because each data column is exactly one character narrower than its header,
will the data rows progressively drift to the left compared to the headers?

> @@ -633,7 +631,7 @@ static void print_metric_only_csv(struct perf_stat_config *config __maybe_unused
>  	while (isdigit(*ends) || *ends == '.')
>  		ends++;
>  	*ends = 0;
> -	fprintf(out, "%s%s", vals, config->csv_sep);
> +	fprintf(out, "%s%s ", vals, config->csv_sep);
>  	os->first = false;
>  }

Does adding this trailing space after the CSV separator break machine
readability for strict CSV parsers?

The CSV header output does not appear to add this space, which might cause a
format mismatch between the headers and the data rows.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512234345.554166-1-ak@linux.intel.com?part=1