Re: [PATCH v1 0/4] perf annotate: Create a new '--tui-dump' option
From: Arnaldo Carvalho de Melo
Date: Wed Mar 14 2018 - 09:54:45 EST
Em Wed, Mar 14, 2018 at 10:04:49AM +0800, Jin, Yao escreveu:
>
>
> On 3/13/2018 11:20 PM, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Mar 13, 2018 at 10:16:50PM +0800, Jin Yao escreveu:
> > > There is a requirement to let perf annotate support displaying the IPC/Cycle.
> > > In previous patch, this is supported in TUI mode. While it's not convenient
> > > for users since they have to take screen shots and copy/paste data.
> > >
> > > This patch series introduces a new option '--tui-dump' in perf annotate to
> > > dump the TUI output to stdio.
> > >
> > > User can easily use the command line like:
> > > 'perf annotate --tui-dump > /tmp/log.txt'
> >
> > My first impression is that this pollutes the code with way too many
> > ifs, I was thinking more of a:
> >
> > while (read parsed objdump line) {
> > ins__fprintf();
> > }
> >
>
> Yes, the issue in my patch is that it uses many 'if' to check if it's
> tui_dump(or called 'stdio2') or tui mode.
>
> > Going from the refresh routine, I started doing the conversion, but haven't
> > completed it, there are opportunities for more __scnprintf like routines, also
> > one to find the percent_max, etc then those would be used both in these two for
> > --stdio2, that eventually would become --stdio with the old one becoming
> > --stdio1, till we're satisfied with the new default.
> >
>
> I have some questions for the following code. Please correct me if I
> misunderstand anything.
>
> > static void annotation_line__fprintf(struct annotate_line *al, FILE *fp)
> > {
> > struct browser_line *bl = browser_line(al);
> > int i;
> > double percent_max = 0.0;
> > char bf[256];
> >
> > for (i = 0; i < browser->nr_events; i++) {
> > if (al->samples[i].percent > percent_max)
> > percent_max = al->samples[i].percent;
> > }
> >
> > /* the following if/else block should be transformed into a __scnprintf routine
> > that formats a buffer and then the TUI and --stdio2 use it */
> >
> > if (al->offset != -1 && percent_max != 0.0) {
> > for (i = 0; i < ab->nr_events; i++) {
> > if (annotate_browser__opts.show_total_period) {
> > fprintf(fp, browser, "%11" PRIu64 " ", al->samples[i].he.period);
> > } else if (annotate_browser__opts.show_nr_samples) {
> > fprintf(fp, browser, "%6" PRIu64 " ", al->samples[i].he.nr_samples);
> > } else {
> > fprintf(fp, "%6.2f ", al->samples[i].percent);
> > }
> > }
> > } else {
> > ui_browser__printf(browser, "%*s", pcnt_width,
> > annotate_browser__opts.show_total_period ? "Period" :
> > annotate_browser__opts.show_nr_samples ? "Samples" : "Percent");
> >
> > }
> >
>
> I guess the above code has not been completed yet. My understanding for
> Arnaldo's idea is that the output should be written to a buffer via
> scnprintf and the buffer will be passed to TUI or stdio2 and printed out
> later.
>
> One potential issue is how to process the color for TUI? For example, the
> call to ui_browser__set_percent_color. If we need to set color for TUI, it
> looks we still have to check if it's a TUI mode.
That is part of the uncompleted code, if you want to show colors in
--stdio2 (and I think it is worth) then use color_fprintf() like other
stdio code.
For instance, 'perf top --stdio' uses red for hot lines, etc.
> For example,
>
> Suppose the bf[] has been written with the Percent string yet, next we need,
>
> if (--tui) {
> ui_browser__set_percent_color(...);
> ui_browser__printf(bf, ...);
> } else {
> printf(..., bf);
> }
>
> Is my understanding correct?
The way I am suggesting there will be no if (tui), it will just reuse
the scnprintf routines that are now used in the TUI code, but will only
use fprintf/color_fprintf, etc.
The loop iterating over the annotation_lines you just lift from the tui
code.
And parts that are useful for both and are not yet in __scnprintf form,
then we need to make it so.
> > /* The ab->have_cycles should go to a separate struct, outside
> > * annotation_browser, and the rest should go to something that just does scnprintf on a buffer
> > * that then is printed on the TUI or with fprintf */
> >
> > if (ab->have_cycles) {
> > if (al->ipc)
> > fprintf(fp, "%*.2f ", IPC_WIDTH - 1, al->ipc) > else if (show_title)
> > ui_browser__printf(browser, "%*s ", IPC_WIDTH - 1, "IPC");
> >
> > if (al->cycles)
> > ui_browser__printf(browser, "%*" PRIu64 " ",
> > CYCLES_WIDTH - 1, al->cycles);
> > else if (!show_title)
> > ui_browser__write_nstring(browser, " ", CYCLES_WIDTH);
> > else
> > ui_browser__printf(browser, "%*s ", CYCLES_WIDTH - 1, "Cycle");
> > }
> >
> > SLsmg_write_char(' ');
> >
> > /* The scroll bar isn't being used */
> > if (!browser->navkeypressed)
> > width += 1;
> >
> > if (!*al->line)
> > fprintf(fp, "\n");
> > else if (al->offset == -1) {
> > if (al->line_nr && annotate_browser__opts.show_linenr)
> > printed = scnprintf(bf, sizeof(bf), "%-*d ",
> > ab->addr_width + 1, al->line_nr);
> > else
> > printed = scnprintf(bf, sizeof(bf), "%*s ",
> > ab->addr_width, " ");
> > fprintf(fp, bf);
> > ui_browser__write_nstring(browser, al->line, width - printed - pcnt_width - cycles_width + 1);
> > } else {
> > u64 addr = al->offset;
> > int color = -1;
> >
> > if (!annotate_browser__opts.use_offset)
> > addr += ab->start;
> >
> > if (!annotate_browser__opts.use_offset) {
> > printed = scnprintf(bf, sizeof(bf), "%" PRIx64 ": ", addr);
> > } else {
> > if (bl->jump_sources) {
> > if (annotate_browser__opts.show_nr_jumps) {
> > int prev;
> > printed = scnprintf(bf, sizeof(bf), "%*d ",
> > ab->jumps_width,
> > bl->jump_sources);
> > prev = annotate_browser__set_jumps_percent_color(ab, bl->jump_sources,
> > current_entry);
> > ui_browser__write_nstring(browser, bf, printed);
> > ui_browser__set_color(browser, prev);
> > }
> >
>
> JUMP is another headache case. For TUI, we need to set color and write jump
> arrow. While for stdio2, we don't need that. So looks we also need to check
> if it's in TUI mode.
No, when in --stdio2 case you either show no jump characters, or you
show them if available, i.e. here are two stdio utilities showing those
arrows:
[root@seventh ~]# cat a.txt
â jmp 3f0
[root@seventh ~]# head a.txt
â jmp 3f0
[root@seventh ~]#
You just don't need to print the arrows connecting jumps to its targets,
because in a function with a lot of jumps, then it can get messy, but
yeah, that could even be an option, perhaps not the default.
> > printed = scnprintf(bf, sizeof(bf), "%*" PRIx64 ": ",
> > ab->target_width, addr);
> > } else {
> > printed = scnprintf(bf, sizeof(bf), "%*s ",
> > ab->addr_width, " ");
> > }
> > }
> >
> > fprintf(fp, bf);
> >
> > disasm_line__write(disasm_line(al), browser, bf, sizeof(bf));
> >
> > ui_browser__write_nstring(browser, bf, width - pcnt_width - cycles_width - 3 - printed);
> > }
> > }
> >
> > unsigned int annotation_lines__fprintf(struct list_head *lines, FILE *fp)
> > {
> > struct list_head *line;
> > struct annotation_line *al;
> >
> > list_for_each(line, lines) {
> > struct annotation_line *al = list_entry(line, struct annotation_line, node);
> > annotation_line__fprintf(al, line);
> > }
> >
> > return row;
> > }
> >
> > Then the main code would use the same code that creates the browser->b.entries
> > and would pass it to annpotation_lines__fprintf().
> >
>
> Is the main code mentioned here symbol__tui_annotate()?
>
> struct annotate_browser browser = {
> .b = {
> .refresh = annotate_browser__refresh,
> .write = annotate_browser__write,
> ....
> },
> };
>
> Remove the code line ".write = annotate_browser__write"? Don't need the
> browser op (write/refresh)? Sorry, I'm not very clear about this idea.
You would have a symbol__stdio2_annotate(), and it would be something
like:
int symbol__stdio2_annotate(FILE *fp)
{
struct annotation *notes;
symbol__annotate(sym, map, evsel, sizeof(struct annotatation_line), NULL);
symbol__calc_percent(sym, evsel);
notes = symbol__annotation(sym);
annotation_lines__fprintf(notes->src->source, fp);
}
Something like that.
> > i.e. we would disentanble the formatting of strings and auxiliary routines to
> > obtain the max_percent, i.e. nothing of TUI is needed for --stdio2, just the
> > formatting of strings.
> Will Arnaldo post your patch? Or do I need to improve my patch and post
> again?
We're still discussing what is the best way to implement this.
> Thanks
> Jin Yao
>
> > > For example:
> > > $ perf annotate compute_flag --tui-dump
> > >
> > > Percent IPC Cycle
> > >
> > > Disassembly of section .text:
> > >
> > > 0000000000400640 <compute_flag>:
> > > compute_flag():
> > > volatile int count;
> > > static unsigned int s_randseed;
> > >
> > > __attribute__((noinline))
> > > int compute_flag()
> > > {
> > > 23.00 1.16 sub $0x8,%rsp
> > > int i;
> > >
> > > i = rand() % 2;
> > > 23.06 1.16 1 callq rand@plt
> > >
> > > return i;
> > > 27.01 3.38 mov %eax,%edx
> > > }
> > > 3.38 add $0x8,%rsp
> > > {
> > > int i;
> > >
> > > i = rand() % 2;
> > >
> > > return i;
> > > 3.38 shr $0x1f,%edx
> > > 3.38 add %edx,%eax
> > > 3.38 and $0x1,%eax
> > > 3.38 sub %edx,%eax
> > > }
> > > 26.93 3.38 2 retq
> > >
> > > The '--stdio' option is still kept now. Maybe in future, we can only
> > > maintain the TUI routines and drop the lagacy stdio code. But right
> > > now we'd better keep it until the '--tui-dump' option is good enough.
> > >
> > > Jin Yao (4):
> > > perf browser: Add a new 'dump' flag
> > > perf browser: bypass ui_init if in tui dump mode
> > > perf annotate: Process the new switch flag tui_dump
> > > perf annotate: Enable the '--tui-dump' mode
> > >
> > > tools/perf/Documentation/perf-annotate.txt | 3 +++
> > > tools/perf/builtin-annotate.c | 12 +++++++--
> > > tools/perf/builtin-c2c.c | 2 +-
> > > tools/perf/builtin-report.c | 2 +-
> > > tools/perf/builtin-top.c | 2 +-
> > > tools/perf/ui/browser.c | 38 +++++++++++++++++++++++----
> > > tools/perf/ui/browser.h | 1 +
> > > tools/perf/ui/browsers/annotate.c | 41 +++++++++++++++++++++---------
> > > tools/perf/ui/browsers/hists.c | 2 +-
> > > tools/perf/ui/setup.c | 9 +++++--
> > > tools/perf/ui/ui.h | 2 +-
> > > tools/perf/util/annotate.h | 6 +++--
> > > tools/perf/util/hist.h | 11 +++++---
> > > 13 files changed, 99 insertions(+), 32 deletions(-)
> > >
> > > --
> > > 2.7.4