LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Ian Rogers <irogers@google.com>
To: Andi Kleen <andi@firstfloor.org>
Cc: Claire Jensen <cjense@google.com>,
	peterz@infradead.org, mingo@redhat.com, acme@kernel.org,
	mark.rutland@arm.com, alexander.shishkin@linux.intel.com,
	jolsa@redhat.com, namhyung@kernel.org, yao.jin@linux.intel.com,
	song@kernel.org, adrian.hunter@intel.com,
	kan.liang@linux.intel.com, james.clark@arm.com,
	alexander.antonov@linux.intel.com, changbin.du@intel.com,
	liuqi115@huawei.com, eranian@google.com,
	linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org,
	clairej735@gmail.com
Subject: Re: [PATCH v2 1/1] perf stat: Add JSON output option.
Date: Tue, 17 Aug 2021 10:59:54 -0700	[thread overview]
Message-ID: <CAP-5=fUg=LU96ATtZ4OtJpyfe75bHwpkD+XTkoZPXVeJdPAVOQ@mail.gmail.com> (raw)
In-Reply-To: <20210815144007.3e7cwiecbre2nt6y@two.firstfloor.org>

On Sun, Aug 15, 2021 at 7:40 AM Andi Kleen <andi@firstfloor.org> wrote:
>
> > CSV output example:
> >
> > 1.20,msec,task-clock:u,1204272,100.00,0.697,CPUs utilized
> > 0,,context-switches:u,1204272,100.00,0.000,/sec
> > 0,,cpu-migrations:u,1204272,100.00,0.000,/sec
> > 70,,page-faults:u,1204272,100.00,58.126,K/sec
>
> The difficult part of such changes to perf stat is that it has
> so many different output modes that all need to be tested.
> Unfortunately the unit tests in perf test are not really
> enough for it.
>
> I have an older script (attached) that tests a lot of these outputs. It just
> exercises them, you still need to check the output manually
>
> Can you check that all these modes work correctly both
> with and without json?

Hi Andi,

Completely agreed on the need to make sure output isn't broken.
Claire's changes include tests for CSV and json output:

CSV:
https://lore.kernel.org/lkml/20210813220936.2105426-1-cjense@google.com/

json:
https://lore.kernel.org/lkml/20210813220936.2105426-1-cjense@google.com/

I think we can improve the json test by making sure the json output is
parseable, which can be a follow up patch. For the CSV output, an
unfortunate aspect to Claire's test was to discover that the current
CSV output is broken with summaries enabled. Specifically, when there
are more than one "shadow stat" to display after the event the summary
column disappears for the additional shadow stats. I'll point at the
specific problems in the code as-is below as I'd like to refactor it,
but it'd be nice to land Claire's work to build upon, including the
tests.

The current stat output display code works through a large number of
"ifs" as well as through function pointers specialized to the style of
output, the complexity of this leads to the CSV summary bug. With
summaries enabled a summary column is added on the left here:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/stat-display.c?h=perf/core&id=e0a7ef2a62e4f61a751bccfc79b9e7acb51474de#n453

but then in the shadow stats a newline may get printed like:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/stat-shadow.c?h=perf/core&id=fba7c86601e2e42d7057db47bf6d45865a208b8c#n986

the CSV newline code doesn't know of the summary column:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/stat-display.c?h=perf/core&id=e0a7ef2a62e4f61a751bccfc79b9e7acb51474de#n203

which causes a row with fewer columns and the shadow stats out of place.

There is a notion in the output of a prefix:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/stat-display.c?h=perf/core&id=e0a7ef2a62e4f61a751bccfc79b9e7acb51474de#n148

but it seems to be unused.

To refactor the code I'd like the printing code to focus on computing
"stats" and "shadow stats" and calling helpers to print these. The
helpers would be specialized per output kind, much as the new_line and
print_metric function pointers currently do. To avoid the problem of
the missing column, I'd like the abstraction for the printing to be
slightly higher level - so things like, print_header, print_stat,
print_shadow_stat and then we have functions for each output kind
implementing this. These functions should be sufficiently specialized
to avoid "ifs".

In doing the refactor it is going to correct bugs like the missing
column, and so we'll need to be mindful that some small changes in
output are intentional. This brittleness points to why Claire's
addition of json output is so useful :-)

Thanks,
Ian

> -Andi

  reply	other threads:[~2021-08-17 18:00 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-13 22:07 Claire Jensen
2021-08-14 13:26 ` kajoljain
2021-08-15 14:40 ` Andi Kleen
2021-08-17 17:59   ` Ian Rogers [this message]
2021-08-17 19:56     ` Andi Kleen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAP-5=fUg=LU96ATtZ4OtJpyfe75bHwpkD+XTkoZPXVeJdPAVOQ@mail.gmail.com' \
    --to=irogers@google.com \
    --cc=acme@kernel.org \
    --cc=adrian.hunter@intel.com \
    --cc=alexander.antonov@linux.intel.com \
    --cc=alexander.shishkin@linux.intel.com \
    --cc=andi@firstfloor.org \
    --cc=changbin.du@intel.com \
    --cc=cjense@google.com \
    --cc=clairej735@gmail.com \
    --cc=eranian@google.com \
    --cc=james.clark@arm.com \
    --cc=jolsa@redhat.com \
    --cc=kan.liang@linux.intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-perf-users@vger.kernel.org \
    --cc=liuqi115@huawei.com \
    --cc=mark.rutland@arm.com \
    --cc=mingo@redhat.com \
    --cc=namhyung@kernel.org \
    --cc=peterz@infradead.org \
    --cc=song@kernel.org \
    --cc=yao.jin@linux.intel.com \
    --subject='Re: [PATCH v2 1/1] perf stat: Add JSON output option.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).