Re: [PATCH v3 3/4] perf tools: Add support for displaying weights in annotate

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The 'w' hotkey caches the active event's weight mask into a global configuration variable, corrupting the display of weight columns for any subsequently viewed events.
--

--- Patch [3]: [PATCH v3 3/4] perf tools: Add support for displaying weights in annotate ---
commit 4886f581054e1fcacbf3f1649fad566092ced17b
Author: Andi Kleen <[email protected]>

perf tools: Add support for displaying weights in annotate

This commit adds support for showing all three possible per-IP weights in
the annotation view, enabled by default if any are non-zero. It also
introduces a hotkey ('w') and a command line option to toggle these weight
columns.

> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> index e47a467750890..d6126a7d51e4a 100644
> --- a/tools/perf/ui/browsers/annotate.c
> +++ b/tools/perf/ui/browsers/annotate.c
[ ... ]
> @@ -1091,6 +1093,15 @@ static int annotate_browser__run(struct annotate_browser *browser,
>  				symbol_conf.show_total_period = true;
>  			annotation__update_column_widths(notes);
>  			continue;
> +		case 'w':
> +			symbol_conf.hide_weight = !symbol_conf.hide_weight;
> +			symbol_conf.weight_mask_user = true;
> +			symbol_conf.weight_mask = symbol_conf.hide_weight ?
> +				0 : sym_hist__weight_mask(annotation__histogram(notes, evsel));

[Severity: Medium]
Will caching the active event's weight mask into the global symbol_conf
corrupt the display for subsequently viewed events?

If the user toggles weights with 'w', symbol_conf.weight_mask_user is set
to true and the current event's mask is saved to the global
symbol_conf.weight_mask. If the user then opens an annotation for a
different event, it seems this stale global state will persist and
override the dynamic column visibility for the new event.

> +			browser->b.width = notes->src->widths.max_line_len +
> +					   annotation__pcnt_width(notes, evsel);
> +			ui_browser__refresh_dimensions(&browser->b);
> +			continue;
[ ... ]
> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> index fa08d09b80f76..63bce7a920f32 100644
> --- a/tools/perf/util/annotate.h
> +++ b/tools/perf/util/annotate.h
[ ... ]
> @@ -376,9 +391,22 @@ static inline int annotation__cycles_width(struct annotation *notes)
>  	return notes->branch ? ANNOTATION__IPC_WIDTH + ANNOTATION__CYCLES_WIDTH : 0;
>  }
>  
> -static inline int annotation__pcnt_width(struct annotation *notes)
> +static inline u8 annotation__weight_mask(struct annotation *notes,
> +					 const struct evsel *evsel)
> +{
> +	if (symbol_conf.weight_mask_user)
> +		return symbol_conf.weight_mask;

[Severity: Medium]
Because symbol_conf is a global variable, does applying it uniformly here
break the dynamic weight mask calculation for other events once the user
has triggered the hotkey?

> +	if (symbol_conf.hide_weight)
> +		return 0;
> +	return sym_hist__weight_mask(&notes->src->histograms[evsel->core.idx]);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3