Re: [PATCH v1] perf stat: Remove evlist__add_default_attrs use strings

[Ian Rogers]

On Tue, Apr 9, 2024 at 9:00 AM Liang, Kan <kan.liang@xxxxxxxxxxxxxxx> wrote:
>
>
>
> On 2024-04-09 11:20 a.m., Ian Rogers wrote:
> >>> +             ret = parse_events(evlist,
> >>> +                             "context-switches,"
> >>> +                             "cpu-migrations,"
> >>> +                             "page-faults,"
> >>> +                             "instructions,"
> >>> +                             "cycles,"
> >> "cycles",
> >> "instructions",
> >>
> >> It's better to keep the original order.
> > So the original order was:
> > "cycles,"
> > "stalled-cycles-frontend,"
> > "stalled-cycles-backend,"
> > "instructions"
> >
>
> Right. The stalled-* events are added between default_attrs0 and
> default_attrs1.
>
>
> > but many/most/all core PMUs don't provide the stalled-* events. At the
> > programmer level instructions is the most fundamental thing, so having
> > it last felt wrong hence moving it to be the first after the software
> > events. My thought was, if we're going to reorder things then let's
> > not do a half measure like:
> > "cycles,"
> > "instructions,"
> > "stalled-cycles-frontend,"
> > "stalled-cycles-backend"
> >
> > let's just put things into their best order. It is obviously easy to
> > change but having this way wasn't an accident. There's obviously
> > subjectivity about whether cycles is more fundamental than
> > instructions, my thought is that you get taught instructions first and
> > that these take some number of cycles to execute, hence thinking
> > instructions should have some priority in the output over cycles -
> > some people may not even know what cycles means, it is hard enough
> > when you do given the variety of different clocks 🙂
> >
>
> My concern is that there may be someone who still relies on the std
> output of perf stat default. So the output format/order matters for
> them. Their scripts probably be broken if the order is changed.

I think making everyone suffer for the case of a tool that may behave
in this way doesn't make sense. The tool should transition to not care
or to say the json output, or at least contribute a test. There is
precedent for this attitude, the default metrics for topdown removed
the event names in perf stat default output - no one screamed, and I
expect that to be the case here.

Thanks,
Ian

> Thanks,
> Kan
>