Re: [PATCH 14/15] perf stat: Rename "aggregate-number" to "cpu-count" in JSON

[Ian Rogers]

On Thu, Nov 24, 2022 at 11:51 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
>
> Hi Ian,
>
> On Wed, Nov 23, 2022 at 3:31 PM Ian Rogers <irogers@xxxxxxxxxx> wrote:
> >
> > On Wed, Nov 23, 2022 at 10:02 AM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
> > >
> > > As the JSON output has been broken for a little while, I guess there are
> > > not many users.  Let's rename the field to more intuitive one. :)
> >
> > I'm not sure cpu-count is accurate. For example, an uncore counter in
> > a dual socket machine may have a CPU mask of "0, 36", ie one event per
> > socket. The aggregate-number in this case I believe is 2.
>
> You're right.  In case of uncore events, it can be confusing.  But in some
> sense it could be thought as cpu count as well since it aggregates the
> result from two cpus anyway. :)
>
> Note that the aggregate-number (or cpu-count) is only printed if users
> requested one of aggregation options like --per-socket or --per-core.
> In your example, then it could print 1 for each socket.
>
> But I think uncore events are different from core events, and hopefully
> they have separate instances for different sockets or something already.
> That means it doesn't need to use those aggregation options for them.
>
> Also the CSV output uses "cpus" for the same information.  It'd be nice
> we could have consistency.

So in the original patch from Claire she'd passed the name "number"
through to the json from the stat code. Having an integer called
"number" isn't exactly intention revealing - thank you for your clean
up work! :-) I switched "number" to be "aggregate number" as the
number comes from the "data" aggregated and the code refers to it as
aggregate data. I think aggregate-number is more consistent with the
code, and cpu-count would look strange in the uncore case above where
the number of CPUs (really hyperthreads) is 72. Perhaps we should also
be outputting the aggregation mode with the number. Anyway, I think
for the patch series I'd prefer we skipped this one and kept the rest.

Thanks,
Ian
> Namhyung