Re: [PATCH 5/5] perf annotate: Add --skip-empty option

[Arnaldo Carvalho de Melo <acme@kernel.org>]

On Sat, Aug 03, 2024 at 02:13:32PM -0700, Namhyung Kim wrote:
> Like in perf report, we want to hide empty events in the perf annotate
> output.  This is consistent when the option is set in perf report.
> 
> For example, the following command would use 3 events including dummy.

The option --skip-empty is useful, but I wonder if for "dummy" it
shouldn't be the default, i.e. a per-event "skip" or "hide" flag that we
would set for the "dummy" event in addition to this --skip-empty command
line option?

- Arnaldo
 
>   $ perf mem record -a -- perf test -w noploop
> 
>   $ perf evlist
>   cpu/mem-loads,ldlat=30/P
>   cpu/mem-stores/P
>   dummy:u
> 
> Just using perf annotate with --group will show the all 3 events.
> 
>   $ perf annotate --group --stdio | head
>    Percent                 |	Source code & Disassembly of ...
>   --------------------------------------------------------------
>                            : 0     0xe060 <_dl_relocate_object>:
>       0.00    0.00    0.00 :    e060:       pushq   %rbp
>       0.00    0.00    0.00 :    e061:       movq    %rsp, %rbp
>       0.00    0.00    0.00 :    e064:       pushq   %r15
>       0.00    0.00    0.00 :    e066:       movq    %rdi, %r15
>       0.00    0.00    0.00 :    e069:       pushq   %r14
>       0.00    0.00    0.00 :    e06b:       pushq   %r13
>       0.00    0.00    0.00 :    e06d:       movl    %edx, %r13d
> 
> Now with --skip-empty, it'll hide the last dummy event.
> 
>   $ perf annotate --group --stdio --skip-empty | head
>    Percent         |	Source code & Disassembly of ...
>   ------------------------------------------------------
>                    : 0     0xe060 <_dl_relocate_object>:
>       0.00    0.00 :    e060:       pushq   %rbp
>       0.00    0.00 :    e061:       movq    %rsp, %rbp
>       0.00    0.00 :    e064:       pushq   %r15
>       0.00    0.00 :    e066:       movq    %rdi, %r15
>       0.00    0.00 :    e069:       pushq   %r14
>       0.00    0.00 :    e06b:       pushq   %r13
>       0.00    0.00 :    e06d:       movl    %edx, %r13d
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/Documentation/perf-annotate.txt |  3 +++
>  tools/perf/builtin-annotate.c              |  2 ++
>  tools/perf/util/annotate.c                 | 22 +++++++++++++++++-----
>  3 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/Documentation/perf-annotate.txt b/tools/perf/Documentation/perf-annotate.txt
> index b95524bea021..156c5f37b051 100644
> --- a/tools/perf/Documentation/perf-annotate.txt
> +++ b/tools/perf/Documentation/perf-annotate.txt
> @@ -165,6 +165,9 @@ include::itrace.txt[]
>  --type-stat::
>  	Show stats for the data type annotation.
>  
> +--skip-empty::
> +	Do not display empty (or dummy) events.
> +
>  
>  SEE ALSO
>  --------
> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> index cf60392b1c19..efcadb7620b8 100644
> --- a/tools/perf/builtin-annotate.c
> +++ b/tools/perf/builtin-annotate.c
> @@ -795,6 +795,8 @@ int cmd_annotate(int argc, const char **argv)
>  		    "Show stats for the data type annotation"),
>  	OPT_BOOLEAN(0, "insn-stat", &annotate.insn_stat,
>  		    "Show instruction stats for the data type annotation"),
> +	OPT_BOOLEAN(0, "skip-empty", &symbol_conf.skip_empty,
> +		    "Do not display empty (or dummy) events in the output"),
>  	OPT_END()
>  	};
>  	int ret;
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 917897fe44a2..eafe8d65052e 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -848,6 +848,10 @@ static void annotation__calc_percent(struct annotation *notes,
>  
>  			BUG_ON(i >= al->data_nr);
>  
> +			if (symbol_conf.skip_empty &&
> +			    evsel__hists(evsel)->stats.nr_samples == 0)
> +				continue;
> +
>  			data = &al->data[i++];
>  
>  			calc_percent(notes, evsel, data, al->offset, end);
> @@ -901,7 +905,7 @@ int symbol__annotate(struct map_symbol *ms, struct evsel *evsel,
>  		.options	= &annotate_opts,
>  	};
>  	struct arch *arch = NULL;
> -	int err;
> +	int err, nr;
>  
>  	err = evsel__get_arch(evsel, &arch);
>  	if (err < 0)
> @@ -922,10 +926,18 @@ int symbol__annotate(struct map_symbol *ms, struct evsel *evsel,
>  			return -1;
>  	}
>  
> -	if (evsel__is_group_event(evsel))
> -		notes->src->nr_events = evsel->core.nr_members;
> -	else
> -		notes->src->nr_events = 1;
> +	nr = 0;
> +	if (evsel__is_group_event(evsel)) {
> +		struct evsel *pos;
> +
> +		for_each_group_evsel(pos, evsel) {
> +			if (symbol_conf.skip_empty &&
> +			    evsel__hists(pos)->stats.nr_samples == 0)
> +				continue;
> +			nr++;
> +		}
> +	}
> +	notes->src->nr_events = nr ? nr : 1;
>  
>  	if (annotate_opts.full_addr)
>  		notes->src->start = map__objdump_2mem(ms->map, ms->sym->start);
> -- 
> 2.46.0.rc2.264.g509ed76dc8-goog