* Re: [PATCH v1 0/4] perf annotate: Create a new '--tui-dump' option
2018-03-13 14:16 [PATCH v1 0/4] perf annotate: Create a new '--tui-dump' option Jin Yao
@ 2018-03-13 10:01 ` Mason
2018-03-13 14:16 ` [PATCH v1 1/4] perf browser: Add a new 'dump' flag Jin Yao
` (4 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Mason @ 2018-03-13 10:01 UTC (permalink / raw)
To: Jin Yao; +Cc: LKML
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v1 0/4] perf annotate: Create a new '--tui-dump' option
@ 2018-03-13 14:16 Jin Yao
2018-03-13 10:01 ` Mason
` (5 more replies)
0 siblings, 6 replies; 10+ messages in thread
From: Jin Yao @ 2018-03-13 14:16 UTC (permalink / raw)
To: acme, jolsa, peterz, mingo, alexander.shishkin
Cc: Linux-kernel, ak, kan.liang, yao.jin, Jin Yao
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'
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v1 1/4] perf browser: Add a new 'dump' flag
2018-03-13 14:16 [PATCH v1 0/4] perf annotate: Create a new '--tui-dump' option Jin Yao
2018-03-13 10:01 ` Mason
@ 2018-03-13 14:16 ` Jin Yao
2018-03-13 14:16 ` [PATCH v1 2/4] perf browser: bypass ui_init if in tui dump mode Jin Yao
` (3 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Jin Yao @ 2018-03-13 14:16 UTC (permalink / raw)
To: acme, jolsa, peterz, mingo, alexander.shishkin
Cc: Linux-kernel, ak, kan.liang, yao.jin, Jin Yao
We have a new requirement to provide the TUI output option for
non-interactive mode. For example, write the TUI output to stdio directly.
This patch creates a new flag 'dump' in struct ui_browser. Once it's on,
for the formatted buffer, we just print it on stdio.
Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
tools/perf/ui/browser.c | 38 +++++++++++++++++++++++++++++++++-----
tools/perf/ui/browser.h | 1 +
2 files changed, 34 insertions(+), 5 deletions(-)
diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
index 63399af..3534c6c 100644
--- a/tools/perf/ui/browser.c
+++ b/tools/perf/ui/browser.c
@@ -41,8 +41,13 @@ int ui_browser__set_color(struct ui_browser *browser, int color)
void ui_browser__set_percent_color(struct ui_browser *browser,
double percent, bool current)
{
- int color = ui_browser__percent_color(browser, percent, current);
- ui_browser__set_color(browser, color);
+ int color;
+
+ if (browser->dump)
+ return;
+
+ color = ui_browser__percent_color(browser, percent, current);
+ ui_browser__set_color(browser, color);
}
void ui_browser__gotorc(struct ui_browser *browser, int y, int x)
@@ -50,10 +55,27 @@ void ui_browser__gotorc(struct ui_browser *browser, int y, int x)
SLsmg_gotorc(browser->y + y, browser->x + x);
}
+static void write_nstring(const char *msg, unsigned int width)
+{
+ unsigned int len = strlen(msg);
+ unsigned int i = 0;
+
+ while (i < len && i < width) {
+ putchar(msg[i]);
+ i++;
+ }
+
+ while (i++ < width)
+ putchar(' ');
+}
+
void ui_browser__write_nstring(struct ui_browser *browser __maybe_unused, const char *msg,
unsigned int width)
{
- slsmg_write_nstring(msg, width);
+ if (browser->dump)
+ write_nstring(msg, width);
+ else
+ slsmg_write_nstring(msg, width);
}
void ui_browser__printf(struct ui_browser *browser __maybe_unused, const char *fmt, ...)
@@ -61,7 +83,10 @@ void ui_browser__printf(struct ui_browser *browser __maybe_unused, const char *f
va_list args;
va_start(args, fmt);
- slsmg_vprintf(fmt, args);
+ if (browser->dump)
+ vprintf(fmt, args);
+ else
+ slsmg_vprintf(fmt, args);
va_end(args);
}
@@ -496,7 +521,10 @@ unsigned int ui_browser__list_head_refresh(struct ui_browser *browser)
list_for_each_from(pos, head) {
if (!browser->filter || !browser->filter(browser, pos)) {
- ui_browser__gotorc(browser, row, 0);
+ if (!browser->dump)
+ ui_browser__gotorc(browser, row, 0);
+ else if (row > 0)
+ printf("\n");
browser->write(browser, pos, row);
if (++row == browser->rows)
break;
diff --git a/tools/perf/ui/browser.h b/tools/perf/ui/browser.h
index 03e1734..66d9405 100644
--- a/tools/perf/ui/browser.h
+++ b/tools/perf/ui/browser.h
@@ -28,6 +28,7 @@ struct ui_browser {
u32 nr_entries;
bool navkeypressed;
bool use_navkeypressed;
+ bool dump;
};
int ui_browser__set_color(struct ui_browser *browser, int color);
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v1 2/4] perf browser: bypass ui_init if in tui dump mode
2018-03-13 14:16 [PATCH v1 0/4] perf annotate: Create a new '--tui-dump' option Jin Yao
2018-03-13 10:01 ` Mason
2018-03-13 14:16 ` [PATCH v1 1/4] perf browser: Add a new 'dump' flag Jin Yao
@ 2018-03-13 14:16 ` Jin Yao
2018-03-13 14:16 ` [PATCH v1 3/4] perf annotate: Process the new switch flag tui_dump Jin Yao
` (2 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Jin Yao @ 2018-03-13 14:16 UTC (permalink / raw)
To: acme, jolsa, peterz, mingo, alexander.shishkin
Cc: Linux-kernel, ak, kan.liang, yao.jin, Jin Yao
We create a tui dump mode in previous patch. In tui dump mode, the output
is written to stdio. We need to bypass ui_init() in setup_browser().
Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
tools/perf/builtin-annotate.c | 2 +-
tools/perf/builtin-c2c.c | 2 +-
tools/perf/builtin-report.c | 2 +-
tools/perf/builtin-top.c | 2 +-
tools/perf/ui/setup.c | 9 +++++++--
tools/perf/ui/ui.h | 2 +-
6 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index ead6ae4..2db5b50 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -576,7 +576,7 @@ int cmd_annotate(int argc, const char **argv)
else if (annotate.use_gtk)
use_browser = 2;
- setup_browser(true);
+ setup_browser(true, false);
if (use_browser == 1 && annotate.has_br_stack) {
sort__mode = SORT_MODE__BRANCH;
diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index 98d243f..6abc13b 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -2618,7 +2618,7 @@ static int perf_c2c__report(int argc, const char **argv)
else
use_browser = 1;
- setup_browser(false);
+ setup_browser(false, false);
err = perf_session__process_events(session);
if (err) {
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 971ccba..99277b7 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -1261,7 +1261,7 @@ int cmd_report(int argc, const char **argv)
}
if (strcmp(input_name, "-") != 0)
- setup_browser(true);
+ setup_browser(true, false);
else
use_browser = 0;
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 0a26b56..b7db01a 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1430,7 +1430,7 @@ int cmd_top(int argc, const char **argv)
else if (top.use_tui)
use_browser = 1;
- setup_browser(false);
+ setup_browser(false, false);
if (setup_sorting(top.evlist) < 0) {
if (sort_order)
diff --git a/tools/perf/ui/setup.c b/tools/perf/ui/setup.c
index 44fe824..8219b2e 100644
--- a/tools/perf/ui/setup.c
+++ b/tools/perf/ui/setup.c
@@ -73,9 +73,9 @@ int stdio__config_color(const struct option *opt __maybe_unused,
return 0;
}
-void setup_browser(bool fallback_to_pager)
+void setup_browser(bool fallback_to_pager, bool tui_dump)
{
- if (use_browser < 2 && (!isatty(1) || dump_trace))
+ if (use_browser < 2 && (!isatty(1) || dump_trace) && !tui_dump)
use_browser = 0;
/* default to TUI */
@@ -92,6 +92,11 @@ void setup_browser(bool fallback_to_pager)
/* fall through */
case 1:
use_browser = 1;
+ if (tui_dump) {
+ setup_pager();
+ break;
+ }
+
if (ui__init() == 0)
break;
/* fall through */
diff --git a/tools/perf/ui/ui.h b/tools/perf/ui/ui.h
index 9b6fdf0..020a85b 100644
--- a/tools/perf/ui/ui.h
+++ b/tools/perf/ui/ui.h
@@ -11,7 +11,7 @@ extern void *perf_gtk_handle;
extern int use_browser;
-void setup_browser(bool fallback_to_pager);
+void setup_browser(bool fallback_to_pager, bool tui_dump);
void exit_browser(bool wait_for_ok);
#ifdef HAVE_SLANG_SUPPORT
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v1 3/4] perf annotate: Process the new switch flag tui_dump
2018-03-13 14:16 [PATCH v1 0/4] perf annotate: Create a new '--tui-dump' option Jin Yao
` (2 preceding siblings ...)
2018-03-13 14:16 ` [PATCH v1 2/4] perf browser: bypass ui_init if in tui dump mode Jin Yao
@ 2018-03-13 14:16 ` Jin Yao
2018-03-13 14:16 ` [PATCH v1 4/4] perf annotate: Enable the '--tui-dump' mode Jin Yao
2018-03-13 15:20 ` [PATCH v1 0/4] perf annotate: Create a new '--tui-dump' option Arnaldo Carvalho de Melo
5 siblings, 0 replies; 10+ messages in thread
From: Jin Yao @ 2018-03-13 14:16 UTC (permalink / raw)
To: acme, jolsa, peterz, mingo, alexander.shishkin
Cc: Linux-kernel, ak, kan.liang, yao.jin, Jin Yao
The tui_dump is created as a parameter and it will be finally passed to
symbol__tui_annotate() and be saved to browser.dump.
It's a switch for TUI routines to indicate if TUI output needs to be
dumped to stdio.
Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
tools/perf/builtin-annotate.c | 2 +-
tools/perf/ui/browsers/annotate.c | 41 +++++++++++++++++++++++++++------------
tools/perf/ui/browsers/hists.c | 2 +-
tools/perf/util/annotate.h | 6 ++++--
tools/perf/util/hist.h | 11 +++++++----
5 files changed, 42 insertions(+), 20 deletions(-)
diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 2db5b50..8ba8e2c 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -334,7 +334,7 @@ static void hists__find_annotations(struct hists *hists,
/* skip missing symbols */
nd = rb_next(nd);
} else if (use_browser == 1) {
- key = hist_entry__tui_annotate(he, evsel, NULL);
+ key = hist_entry__tui_annotate(he, evsel, NULL, false);
switch (key) {
case -1:
diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index 618edf9..bb7229d 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -122,7 +122,9 @@ static void disasm_line__write(struct disasm_line *dl, struct ui_browser *browse
char *bf, size_t size)
{
if (dl->ins.ops && dl->ins.ops->scnprintf) {
- if (ins__is_jump(&dl->ins)) {
+ if (browser->dump) {
+ ui_browser__write_nstring(browser, " ", 2);
+ } else if (ins__is_jump(&dl->ins)) {
bool fwd = dl->ops.target.offset > dl->al.offset;
ui_browser__write_graph(browser, fwd ? SLSMG_DARROW_CHAR :
@@ -217,7 +219,10 @@ static void annotate_browser__write(struct ui_browser *browser, void *entry, int
ui_browser__printf(browser, "%*s ", CYCLES_WIDTH - 1, "Cycle");
}
- SLsmg_write_char(' ');
+ if (browser->dump)
+ putchar(' ');
+ else
+ SLsmg_write_char(' ');
/* The scroll bar isn't being used */
if (!browser->navkeypressed)
@@ -388,6 +393,9 @@ static unsigned int annotate_browser__refresh(struct ui_browser *browser)
int ret = ui_browser__list_head_refresh(browser);
int pcnt_width = annotate_browser__pcnt_width(ab);
+ if (browser->dump)
+ return ret;
+
if (annotate_browser__opts.jump_arrows)
annotate_browser__draw_current_jump(browser);
@@ -589,7 +597,7 @@ static bool annotate_browser__callq(struct annotate_browser *browser,
}
pthread_mutex_unlock(¬es->lock);
- symbol__tui_annotate(dl->ops.target.sym, ms->map, evsel, hbt);
+ symbol__tui_annotate(dl->ops.target.sym, ms->map, evsel, hbt, false);
sym_title(ms->sym, ms->map, title, sizeof(title));
ui_browser__show_title(&browser->b, title);
return true;
@@ -961,7 +969,7 @@ static int annotate_browser__run(struct annotate_browser *browser,
}
int map_symbol__tui_annotate(struct map_symbol *ms, struct perf_evsel *evsel,
- struct hist_browser_timer *hbt)
+ struct hist_browser_timer *hbt, bool tui_dump)
{
/* Set default value for show_total_period and show_nr_samples */
annotate_browser__opts.show_total_period =
@@ -969,17 +977,19 @@ int map_symbol__tui_annotate(struct map_symbol *ms, struct perf_evsel *evsel,
annotate_browser__opts.show_nr_samples =
symbol_conf.show_nr_samples;
- return symbol__tui_annotate(ms->sym, ms->map, evsel, hbt);
+ return symbol__tui_annotate(ms->sym, ms->map, evsel, hbt, tui_dump);
}
int hist_entry__tui_annotate(struct hist_entry *he, struct perf_evsel *evsel,
- struct hist_browser_timer *hbt)
+ struct hist_browser_timer *hbt, bool tui_dump)
{
/* reset abort key so that it can get Ctrl-C as a key */
- SLang_reset_tty();
- SLang_init_tty(0, 0, 0);
+ if (!tui_dump) {
+ SLang_reset_tty();
+ SLang_init_tty(0, 0, 0);
+ }
- return map_symbol__tui_annotate(&he->ms, evsel, hbt);
+ return map_symbol__tui_annotate(&he->ms, evsel, hbt, tui_dump);
}
@@ -1100,7 +1110,8 @@ static inline int width_jumps(int n)
int symbol__tui_annotate(struct symbol *sym, struct map *map,
struct perf_evsel *evsel,
- struct hist_browser_timer *hbt)
+ struct hist_browser_timer *hbt,
+ bool tui_dump)
{
struct annotation_line *al;
struct annotation *notes;
@@ -1117,6 +1128,7 @@ int symbol__tui_annotate(struct symbol *sym, struct map *map,
.filter = disasm_line__filter,
.priv = &ms,
.use_navkeypressed = true,
+ .dump = tui_dump,
},
};
int ret = -1, err;
@@ -1149,7 +1161,8 @@ int symbol__tui_annotate(struct symbol *sym, struct map *map,
symbol__calc_percent(sym, evsel);
- ui_helpline__push("Press ESC to exit");
+ if (!tui_dump)
+ ui_helpline__push("Press ESC to exit");
notes = symbol__annotation(sym);
browser.start = map__rip_2objdump(map, sym->start);
@@ -1193,7 +1206,11 @@ int symbol__tui_annotate(struct symbol *sym, struct map *map,
annotate_browser__update_addr_width(&browser);
- ret = annotate_browser__run(&browser, evsel, hbt);
+ if (tui_dump) {
+ browser.b.width = 80;
+ annotate_browser__refresh(&browser.b);
+ } else
+ ret = annotate_browser__run(&browser, evsel, hbt);
annotated_source__purge(notes->src);
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 8b4e825..f64ef80 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -2418,7 +2418,7 @@ do_annotate(struct hist_browser *browser, struct popup_action *act)
return 0;
evsel = hists_to_evsel(browser->hists);
- err = map_symbol__tui_annotate(&act->ms, evsel, browser->hbt);
+ err = map_symbol__tui_annotate(&act->ms, evsel, browser->hbt, false);
he = hist_browser__selected_entry(browser);
/*
* offer option to annotate the other branch source or target
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 7e914e8..1938907 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -218,13 +218,15 @@ int symbol__tty_annotate(struct symbol *sym, struct map *map,
#ifdef HAVE_SLANG_SUPPORT
int symbol__tui_annotate(struct symbol *sym, struct map *map,
struct perf_evsel *evsel,
- struct hist_browser_timer *hbt);
+ struct hist_browser_timer *hbt,
+ bool tui_dump);
#else
static inline int symbol__tui_annotate(struct symbol *sym __maybe_unused,
struct map *map __maybe_unused,
struct perf_evsel *evsel __maybe_unused,
struct hist_browser_timer *hbt
- __maybe_unused)
+ __maybe_unused,
+ bool tui_dump)
{
return 0;
}
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index e869cad..72bd868 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -421,10 +421,11 @@ struct hist_browser_timer {
#ifdef HAVE_SLANG_SUPPORT
#include "../ui/keysyms.h"
int map_symbol__tui_annotate(struct map_symbol *ms, struct perf_evsel *evsel,
- struct hist_browser_timer *hbt);
+ struct hist_browser_timer *hbt,
+ bool tui_dump);
int hist_entry__tui_annotate(struct hist_entry *he, struct perf_evsel *evsel,
- struct hist_browser_timer *hbt);
+ struct hist_browser_timer *hbt, bool tui_dump);
int perf_evlist__tui_browse_hists(struct perf_evlist *evlist, const char *help,
struct hist_browser_timer *hbt,
@@ -445,14 +446,16 @@ int perf_evlist__tui_browse_hists(struct perf_evlist *evlist __maybe_unused,
}
static inline int map_symbol__tui_annotate(struct map_symbol *ms __maybe_unused,
struct perf_evsel *evsel __maybe_unused,
- struct hist_browser_timer *hbt __maybe_unused)
+ struct hist_browser_timer *hbt __maybe_unused,
+ bool tui_dump __maybe_unused)
{
return 0;
}
static inline int hist_entry__tui_annotate(struct hist_entry *he __maybe_unused,
struct perf_evsel *evsel __maybe_unused,
- struct hist_browser_timer *hbt __maybe_unused)
+ struct hist_browser_timer *hbt __maybe_unused,
+ bool tui_dump __maybe_unused)
{
return 0;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v1 4/4] perf annotate: Enable the '--tui-dump' mode
2018-03-13 14:16 [PATCH v1 0/4] perf annotate: Create a new '--tui-dump' option Jin Yao
` (3 preceding siblings ...)
2018-03-13 14:16 ` [PATCH v1 3/4] perf annotate: Process the new switch flag tui_dump Jin Yao
@ 2018-03-13 14:16 ` Jin Yao
2018-03-13 15:20 ` [PATCH v1 0/4] perf annotate: Create a new '--tui-dump' option Arnaldo Carvalho de Melo
5 siblings, 0 replies; 10+ messages in thread
From: Jin Yao @ 2018-03-13 14:16 UTC (permalink / raw)
To: acme, jolsa, peterz, mingo, alexander.shishkin
Cc: Linux-kernel, ak, kan.liang, yao.jin, Jin Yao
It creates a new option '--tui-dump' in perf annotate command line.
With this option, for example, the perf annotate output:
$ 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
Compared to TUI output,
$ perf annotate compute_flag
│ 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
Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
tools/perf/Documentation/perf-annotate.txt | 3 +++
tools/perf/builtin-annotate.c | 12 ++++++++++--
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/tools/perf/Documentation/perf-annotate.txt b/tools/perf/Documentation/perf-annotate.txt
index 292809c3..d52ae47 100644
--- a/tools/perf/Documentation/perf-annotate.txt
+++ b/tools/perf/Documentation/perf-annotate.txt
@@ -83,6 +83,9 @@ OPTIONS
--gtk:: Use the GTK interface.
+--tui-dump: Use the TUI interface. The TUI output is dumped to stdio
+ interface.
+
-C::
--cpu=<cpu>:: Only report samples for the list of CPUs provided. Multiple CPUs can
be provided as a comma-separated list with no space: 0,1. Ranges of
diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 8ba8e2c..5110160 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -41,6 +41,7 @@ struct perf_annotate {
struct perf_tool tool;
struct perf_session *session;
bool use_tui, use_stdio, use_gtk;
+ bool tui_dump;
bool full_paths;
bool print_line;
bool skip_missing;
@@ -334,7 +335,10 @@ static void hists__find_annotations(struct hists *hists,
/* skip missing symbols */
nd = rb_next(nd);
} else if (use_browser == 1) {
- key = hist_entry__tui_annotate(he, evsel, NULL, false);
+ key = hist_entry__tui_annotate(he, evsel, NULL,
+ ann->tui_dump);
+ if (ann->tui_dump)
+ return;
switch (key) {
case -1:
@@ -486,6 +490,7 @@ int cmd_annotate(int argc, const char **argv)
"dump raw trace in ASCII"),
OPT_BOOLEAN(0, "gtk", &annotate.use_gtk, "Use the GTK interface"),
OPT_BOOLEAN(0, "tui", &annotate.use_tui, "Use the TUI interface"),
+ OPT_BOOLEAN(0, "tui-dump", &annotate.tui_dump, "Dump TUI content"),
OPT_BOOLEAN(0, "stdio", &annotate.use_stdio, "Use the stdio interface"),
OPT_STRING('k', "vmlinux", &symbol_conf.vmlinux_name,
"file", "vmlinux pathname"),
@@ -576,7 +581,10 @@ int cmd_annotate(int argc, const char **argv)
else if (annotate.use_gtk)
use_browser = 2;
- setup_browser(true, false);
+ if (annotate.tui_dump)
+ use_browser = 1;
+
+ setup_browser(true, annotate.tui_dump);
if (use_browser == 1 && annotate.has_br_stack) {
sort__mode = SORT_MODE__BRANCH;
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v1 0/4] perf annotate: Create a new '--tui-dump' option
2018-03-13 14:16 [PATCH v1 0/4] perf annotate: Create a new '--tui-dump' option Jin Yao
` (4 preceding siblings ...)
2018-03-13 14:16 ` [PATCH v1 4/4] perf annotate: Enable the '--tui-dump' mode Jin Yao
@ 2018-03-13 15:20 ` Arnaldo Carvalho de Melo
2018-03-14 2:04 ` Jin, Yao
5 siblings, 1 reply; 10+ messages in thread
From: Arnaldo Carvalho de Melo @ 2018-03-13 15:20 UTC (permalink / raw)
To: Jin Yao
Cc: jolsa, peterz, mingo, alexander.shishkin, Linux-kernel, ak,
kan.liang, yao.jin
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 = 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);
}
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().
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.
- Arnaldo
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1 0/4] perf annotate: Create a new '--tui-dump' option
2018-03-13 15:20 ` [PATCH v1 0/4] perf annotate: Create a new '--tui-dump' option Arnaldo Carvalho de Melo
@ 2018-03-14 2:04 ` Jin, Yao
2018-03-14 13:54 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 10+ messages in thread
From: Jin, Yao @ 2018-03-14 2:04 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: jolsa, peterz, mingo, alexander.shishkin, Linux-kernel, ak,
kan.liang, yao.jin
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);
> 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.
> 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.
> 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.
>
> - Arnaldo
>
Will Arnaldo post your patch? Or do I need to improve my patch and post
again?
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1 0/4] perf annotate: Create a new '--tui-dump' option
2018-03-14 2:04 ` Jin, Yao
@ 2018-03-14 13:54 ` Arnaldo Carvalho de Melo
2018-03-15 3:12 ` Jin, Yao
0 siblings, 1 reply; 10+ messages in thread
From: Arnaldo Carvalho de Melo @ 2018-03-14 13:54 UTC (permalink / raw)
To: Jin, Yao
Cc: jolsa, peterz, mingo, alexander.shishkin, Linux-kernel, ak,
kan.liang, yao.jin
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1 0/4] perf annotate: Create a new '--tui-dump' option
2018-03-14 13:54 ` Arnaldo Carvalho de Melo
@ 2018-03-15 3:12 ` Jin, Yao
0 siblings, 0 replies; 10+ messages in thread
From: Jin, Yao @ 2018-03-15 3:12 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: jolsa, peterz, mingo, alexander.shishkin, Linux-kernel, ak,
kan.liang, yao.jin
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 features.
If we create a new code-patch for stdio2, looks we have to maintain
stdio2 code-path too when we add new feature to TUI mode.
If we use if(tui), since it's added in TUI code-path, though something
ugly, but we don't need much code work to support stdio for new added
feature.
That's my current thoughts. Correct me if my understanding is wrong.
Thanks
Jin Yao
>>> /* 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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-03-15 3:12 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-13 14:16 [PATCH v1 0/4] perf annotate: Create a new '--tui-dump' option Jin Yao
2018-03-13 10:01 ` Mason
2018-03-13 14:16 ` [PATCH v1 1/4] perf browser: Add a new 'dump' flag Jin Yao
2018-03-13 14:16 ` [PATCH v1 2/4] perf browser: bypass ui_init if in tui dump mode Jin Yao
2018-03-13 14:16 ` [PATCH v1 3/4] perf annotate: Process the new switch flag tui_dump Jin Yao
2018-03-13 14:16 ` [PATCH v1 4/4] perf annotate: Enable the '--tui-dump' mode Jin Yao
2018-03-13 15:20 ` [PATCH v1 0/4] perf annotate: Create a new '--tui-dump' option Arnaldo Carvalho de Melo
2018-03-14 2:04 ` Jin, Yao
2018-03-14 13:54 ` Arnaldo Carvalho de Melo
2018-03-15 3:12 ` Jin, Yao
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).