Re: [PATCH 07/10] perf, tools: Collapse identically named events in perf stat
On Mon, Oct 17, 2016 at 07:28:36PM +0200, Jiri Olsa wrote: > On Mon, Oct 17, 2016 at 09:30:17AM -0700, Andi Kleen wrote: > > > this leads to my next question: why this merging should be default? > > > > It's the right default for uncore, and it doesn't do anything for > > non uncore because these usually don't have duplicated event aliases > > over different PMUs. > > I don't like the part when we depends on 'usually' I'm not aware of any counter example in today's perf. -Andi
Re: [PATCH 07/10] perf, tools: Collapse identically named events in perf stat
On Mon, Oct 17, 2016 at 09:30:17AM -0700, Andi Kleen wrote: > > this leads to my next question: why this merging should be default? > > It's the right default for uncore, and it doesn't do anything for > non uncore because these usually don't have duplicated event aliases > over different PMUs. I don't like the part when we depends on 'usually' jirka
Re: [PATCH 07/10] perf, tools: Collapse identically named events in perf stat
> this leads to my next question: why this merging should be default? It's the right default for uncore, and it doesn't do anything for non uncore because these usually don't have duplicated event aliases over different PMUs. -Andi
Re: [PATCH 07/10] perf, tools: Collapse identically named events in perf stat
On Thu, Oct 13, 2016 at 02:15:29PM -0700, Andi Kleen wrote:
SNIP
>
>
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 688dea7cb08f..76304f27c090 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -140,6 +140,7 @@ static unsigned int unit_width
> = 4; /* strlen("unit") */
> static bool forever = false;
> static bool metric_only = false;
> static bool force_metric_only = false;
> +static bool no_merge= false;
> static struct timespec ref_time;
> static struct cpu_map*aggr_map;
> static aggr_get_id_t aggr_get_id;
> @@ -1178,11 +1179,59 @@ static void aggr_update_shadow(void)
> }
> }
>
> +static void collect_aliases(struct perf_evsel *counter,
> + void (*cb)(struct perf_evsel *counter, void *data,
> +bool first),
> + void *data)
merges_stats might be better name
> +{
> + struct perf_evsel *alias;
> +
> + alias = list_prepare_entry(counter, &(evsel_list->entries), node);
> + cb(counter, data, true);
> + if (no_merge)
> + return;
please put this decision (no_merge) outside this function,
so the normal path is straight
this leads to my next question: why this merging should be default?
it seems to make sense just for uncore events
thanks,
jirka
Re: [PATCH 07/10] perf, tools: Collapse identically named events in perf stat
On Thu, Oct 13, 2016 at 02:15:29PM -0700, Andi Kleen wrote:
SNIP
> + OPT_BOOLEAN(0, "no-merge", &no_merge, "Do not merge identical named
> events"),
> OPT_STRING('x', "field-separator", &csv_sep, "separator",
> "print counts with custom separator"),
> OPT_CALLBACK('G', "cgroup", &evsel_list, "name",
> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> index b1503b0ecdff..4e3158fe79c2 100644
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -128,6 +128,7 @@ struct perf_evsel {
> boolcmdline_group_boundary;
> struct list_headconfig_terms;
> int bpf_fd;
> + boolalias;
I think we should call this some other name,
we already use alias for something else..
also alias->alias looks bad ;-)
how about 'merged_stats'
jirka
Re: [PATCH 07/10] perf, tools: Collapse identically named events in perf stat
On Thu, Oct 13, 2016 at 02:15:29PM -0700, Andi Kleen wrote: > From: Andi Kleen > > The uncore PMU has a lot of duplicated PMUs for different subsystems. > When expanding an uncore alias we usually end up with a large > number of identically named aliases, which makes perf stat > output difficult to read. please provide output examples thanks, jirka
