Re: [PATCH v1 0/4] perf annotate: Create a new '--tui-dump' option
On 3/14/2018 9:54 PM, Arnaldo Carvalho de Melo wrote:
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.
Maybe we can support showing color in a followup patch after the basic
function is ready.
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.
OK, I understand, the key point is to avoid using if (tui).
So looks the code will be divided into 2 parts.
Part1: symbol__tui_annotate ->
annotate_browser__refresh/annotate_browser__write
It's current code-path and it's for TUI mode. We will not change it.
Part2:
symbol__stdio2_annotate -> annotation_lines__fprintf ->
annotation_line__fprintf
It's a new code-path and it's for --stdio2 only.
Is my understanding correct? If so, the good thing is we can avoid using
if(tui), while maybe we can't easily reuse common code.
I just think what's the final target we want to reach. For me, I just
wish we had better maintain only one mode (e.g. TUI mode) in future and
for other modes (e.g. stdio), there will be no code work (or no much
code work) for new added
Re: [PATCH v1 0/4] perf annotate: Create a new '--tui-dump' option
On 3/14/2018 9:54 PM, Arnaldo Carvalho de Melo wrote:
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.
Maybe we can support showing color in a followup patch after the basic
function is ready.
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.
OK, I understand, the key point is to avoid using if (tui).
So looks the code will be divided into 2 parts.
Part1: symbol__tui_annotate ->
annotate_browser__refresh/annotate_browser__write
It's current code-path and it's for TUI mode. We will not change it.
Part2:
symbol__stdio2_annotate -> annotation_lines__fprintf ->
annotation_line__fprintf
It's a new code-path and it's for --stdio2 only.
Is my understanding correct? If so, the good thing is we can avoid using
if(tui), while maybe we can't easily reuse common code.
I just think what's the final target we want to reach. For me, I just
wish we had better maintain only one mode (e.g. TUI mode) in future and
for other modes (e.g. stdio), there will be no code work (or no much
code work) for new added
Re: [PATCH v1 0/4] perf annotate: Create a new '--tui-dump' option
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
Re: [PATCH v1 0/4] perf annotate: Create a new '--tui-dump' option
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
Re: [PATCH v1 0/4] perf annotate: Create a new '--tui-dump' option
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.
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 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);
Re: [PATCH v1 0/4] perf annotate: Create a new '--tui-dump' option
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.
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 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);
Re: [PATCH v1 0/4] perf annotate: Create a new '--tui-dump' option
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();
}
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.
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");
}
/* 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 =
Re: [PATCH v1 0/4] perf annotate: Create a new '--tui-dump' option
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();
}
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.
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");
}
/* 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 =
Re: [PATCH v1 0/4] perf annotate: Create a new '--tui-dump' option
On 13/03/2018 15:16, Jin Yao wrote: [snip] There seems to be something wonky about your system's clock? The Date field in your messages: Date: Tue, 13 Mar 2018 22:16:50 +0800 i.e. 14:16:50 GMT Yet it was actually processed at 06:20:35 GMT So it looks like your clock is 7h56 ahead of the actual time. Regards.
Re: [PATCH v1 0/4] perf annotate: Create a new '--tui-dump' option
On 13/03/2018 15:16, Jin Yao wrote: [snip] There seems to be something wonky about your system's clock? The Date field in your messages: Date: Tue, 13 Mar 2018 22:16:50 +0800 i.e. 14:16:50 GMT Yet it was actually processed at 06:20:35 GMT So it looks like your clock is 7h56 ahead of the actual time. Regards.
