Re: [PATCH v1 2/2] perf report: Display titles in left frame of annotate browser

Re: [PATCH v1 2/2] perf report: Display titles in left frame of annotate browser

[Milian Wolff]

On Thursday, May 4, 2017 4:58:15 PM CEST Jin Yao wrote:
> The annotate browser is divided into 2 frames. Left frame
> contains 3 columns (some platforms only have one column).
> 
> For example:
> 
>                    │26  int compute_flag()
>                    │27  {
>  22.80  1.20       │      sub    $0x8,%rsp
>                    │25          int i;
>                    │
>                    │27          i = rand() % 2;
>  22.78  1.20     1 │    → callq  rand@plt
> 
> While it's hard for user to understand what the data is.
> 
> This patch adds the titles "Percent", "IPC" and "Cycle"
> on columns.
> 
> Percnt   IPC Cycle │
>                    │25  __attribute__((noinline))
>                    │26  int compute_flag()
>                    │27  {
>  22.80  1.20       │      sub    $0x8,%rsp
>                    │25          int i;
>                    │
>                    │27          i = rand() % 2;
>  22.78  1.20     1 │    → callq  rand@plt
> 
> The titles are displayed at row 0 of annotate browser if row 0
> doesn't have values of percent, ipc and cycle.

Functionality wise a really good improvement - thanks! But personally I find 
the abbreviation of one character (i.e. "Percnt" instead of "Percent") not so 
nice. If space really is an issue here, use "%"?

Also note though that it's unclear what this percentage actually is. I guess 
it's a sample percentage? Maybe a header should be added that explains these 
values to newbies. I bet many people won't even know what IPC is either.

Cheers

-- 
Milian Wolff | milian.wolff@kdab.com | Software Engineer
KDAB (Deutschland) GmbH&Co KG, a KDAB Group company
Tel: +49-30-521325470
KDAB - The Qt Experts

Re: [PATCH v1 2/2] perf report: Display titles in left frame of annotate browser

[Arnaldo Carvalho de Melo]

Em Thu, May 04, 2017 at 11:01:48AM +0200, Milian Wolff escreveu:
> On Thursday, May 4, 2017 4:58:15 PM CEST Jin Yao wrote:
> > This patch adds the titles "Percent", "IPC" and "Cycle"
> > on columns.

> > Percnt   IPC Cycle │
> >                    │25  __attribute__((noinline))
> >                    │26  int compute_flag()
> >                    │27  {
> >  22.80  1.20       │      sub    $0x8,%rsp
> >                    │25          int i;
> >                    │
> >                    │27          i = rand() % 2;
> >  22.78  1.20     1 │    → callq  rand@plt

> > The titles are displayed at row 0 of annotate browser if row 0
> > doesn't have values of percent, ipc and cycle.
 
> Functionality wise a really good improvement - thanks! But personally I find 
> the abbreviation of one character (i.e. "Percnt" instead of "Percent") not so 
> nice. If space really is an issue here, use "%"?

Ok, will make it 'Percent' as we have space for that, and will add
Acked-by: Millian, ok?
 
> Also note though that it's unclear what this percentage actually is. I guess 
> it's a sample percentage? Maybe a header should be added that explains these 
> values to newbies. I bet many people won't even know what IPC is either.

Perhaps we could have some help files, then when the user presses 'h'
one of the lines would be:

  h   Report explanation (columns, etc)

?

- Arnaldo

Re: [PATCH v1 2/2] perf report: Display titles in left frame of annotate browser

[Milian Wolff]

[-- Attachment #1: Type: text/plain, Size: 1870 bytes --]

On Thursday, May 4, 2017 3:12:50 PM CEST Arnaldo Carvalho de Melo wrote:
> Em Thu, May 04, 2017 at 11:01:48AM +0200, Milian Wolff escreveu:
> > On Thursday, May 4, 2017 4:58:15 PM CEST Jin Yao wrote:
> > > This patch adds the titles "Percent", "IPC" and "Cycle"
> > > on columns.
> > > 
> > > Percnt   IPC Cycle │
> > > 
> > >                    │25  __attribute__((noinline))
> > >                    │26  int compute_flag()
> > >                    │27  {
> > >  
> > >  22.80  1.20       │      sub    $0x8,%rsp
> > >  
> > >                    │25          int i;
> > >                    │
> > >                    │27          i = rand() % 2;
> > >  
> > >  22.78  1.20     1 │    → callq  rand@plt
> > > 
> > > The titles are displayed at row 0 of annotate browser if row 0
> > > doesn't have values of percent, ipc and cycle.
> > 
> > Functionality wise a really good improvement - thanks! But personally I
> > find the abbreviation of one character (i.e. "Percnt" instead of
> > "Percent") not so nice. If space really is an issue here, use "%"?
> 
> Ok, will make it 'Percent' as we have space for that, and will add
> Acked-by: Millian, ok?

Just one L, but otherwise yes :)

> > Also note though that it's unclear what this percentage actually is. I
> > guess it's a sample percentage? Maybe a header should be added that
> > explains these values to newbies. I bet many people won't even know what
> > IPC is either.
> Perhaps we could have some help files, then when the user presses 'h'
> one of the lines would be:
> 
>   h   Report explanation (columns, etc)

Yeah I think that would be a good addition for the future.

Cheers

-- 
Milian Wolff | milian.wolff@kdab.com | Software Engineer
KDAB (Deutschland) GmbH&Co KG, a KDAB Group company
Tel: +49-30-521325470
KDAB - The Qt Experts

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5903 bytes --]

Re: [PATCH v1 2/2] perf report: Display titles in left frame of annotate browser

[Arnaldo Carvalho de Melo]

Em Thu, May 04, 2017 at 04:04:42PM +0200, Milian Wolff escreveu:
> On Thursday, May 4, 2017 3:12:50 PM CEST Arnaldo Carvalho de Melo wrote:
> > Em Thu, May 04, 2017 at 11:01:48AM +0200, Milian Wolff escreveu:
> > > On Thursday, May 4, 2017 4:58:15 PM CEST Jin Yao wrote:
> > > > This patch adds the titles "Percent", "IPC" and "Cycle"
> > > > on columns.
> > > > 
> > > > Percnt   IPC Cycle │
> > > > 
> > > >                    │25  __attribute__((noinline))
> > > >                    │26  int compute_flag()
> > > >                    │27  {
> > > >  
> > > >  22.80  1.20       │      sub    $0x8,%rsp
> > > >  
> > > >                    │25          int i;
> > > >                    │
> > > >                    │27          i = rand() % 2;
> > > >  
> > > >  22.78  1.20     1 │    → callq  rand@plt
> > > > 
> > > > The titles are displayed at row 0 of annotate browser if row 0
> > > > doesn't have values of percent, ipc and cycle.
> > > 
> > > Functionality wise a really good improvement - thanks! But personally I
> > > find the abbreviation of one character (i.e. "Percnt" instead of
> > > "Percent") not so nice. If space really is an issue here, use "%"?
> > 
> > Ok, will make it 'Percent' as we have space for that, and will add
> > Acked-by: Millian, ok?
> 
> Just one L, but otherwise yes :)

Sure, sorry about that, c'n'pasted from a message from you, so should be
all well. 
 
> > > Also note though that it's unclear what this percentage actually is. I
> > > guess it's a sample percentage? Maybe a header should be added that
> > > explains these values to newbies. I bet many people won't even know what
> > > IPC is either.
> > Perhaps we could have some help files, then when the user presses 'h'
> > one of the lines would be:
> > 
> >   h   Report explanation (columns, etc)
> 
> Yeah I think that would be a good addition for the future.

:-)

- Arnaldo

[PATCH v1 0/2] perf report: Add titles for percent/ipc/cycle in annotate browser

[Jin Yao]

The patch series adds the titles "Percnt", "IPC" and "Cycle" in
annotate browser. It helps user to understand easily for what the
columns in left frame represents for.

For example:

    Percnt   IPC Cycle │
                       │25  __attribute__((noinline))
                       │26  int compute_flag()
                       │27  {
     22.80  1.20       │      sub    $0x8,%rsp
                       │25          int i;
                       │
                       │27          i = rand() % 2;
     22.78  1.20     1 │    → callq  rand@plt

The patch "perf report: Remove the unnecessary checking in
annotate_browser_write" removes the invalid condition checking in
current code.

Jin Yao (2):
  perf report: Remove the unnecessary checking in annotate_browser_write
  perf report: Display titles in left frame of annotate browser

 tools/perf/ui/browsers/annotate.c | 51 ++++++++++++++++++++++++++-------------
 1 file changed, 34 insertions(+), 17 deletions(-)

-- 
2.7.4

[PATCH v1 1/2] perf report: Remove the unnecessary checking in annotate_browser_write

[Jin Yao]

In annotate_browser_write(),

        if (dl->offset != -1 && percent_max != 0.0) {
                if (percent_max != 0.0) {
			...
                }
                ...
        }

The second checking of (percent_max != 0.0) is not necessary,
so remove it.

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/ui/browsers/annotate.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index d990ad0..52c1e8d 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -132,21 +132,17 @@ static void annotate_browser__write(struct ui_browser *browser, void *entry, int
 	}
 
 	if (dl->offset != -1 && percent_max != 0.0) {
-		if (percent_max != 0.0) {
-			for (i = 0; i < ab->nr_events; i++) {
-				ui_browser__set_percent_color(browser,
-							bdl->samples[i].percent,
-							current_entry);
-				if (annotate_browser__opts.show_total_period) {
-					ui_browser__printf(browser, "%6" PRIu64 " ",
-							   bdl->samples[i].nr);
-				} else {
-					ui_browser__printf(browser, "%6.2f ",
-							   bdl->samples[i].percent);
-				}
+		for (i = 0; i < ab->nr_events; i++) {
+			ui_browser__set_percent_color(browser,
+						bdl->samples[i].percent,
+						current_entry);
+			if (annotate_browser__opts.show_total_period) {
+				ui_browser__printf(browser, "%6" PRIu64 " ",
+						   bdl->samples[i].nr);
+			} else {
+				ui_browser__printf(browser, "%6.2f ",
+						   bdl->samples[i].percent);
 			}
-		} else {
-			ui_browser__write_nstring(browser, " ", 7 * ab->nr_events);
 		}
 	} else {
 		ui_browser__set_percent_color(browser, 0, current_entry);
-- 
2.7.4

[PATCH v1 2/2] perf report: Display titles in left frame of annotate browser

[Jin Yao]

The annotate browser is divided into 2 frames. Left frame
contains 3 columns (some platforms only have one column).

For example:

                   │26  int compute_flag()
                   │27  {
 22.80  1.20       │      sub    $0x8,%rsp
                   │25          int i;
                   │
                   │27          i = rand() % 2;
 22.78  1.20     1 │    → callq  rand@plt

While it's hard for user to understand what the data is.

This patch adds the titles "Percent", "IPC" and "Cycle"
on columns.

Percnt   IPC Cycle │
                   │25  __attribute__((noinline))
                   │26  int compute_flag()
                   │27  {
 22.80  1.20       │      sub    $0x8,%rsp
                   │25          int i;
                   │
                   │27          i = rand() % 2;
 22.78  1.20     1 │    → callq  rand@plt

The titles are displayed at row 0 of annotate browser if row 0
doesn't have values of percent, ipc and cycle.

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/ui/browsers/annotate.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index 52c1e8d..b1da5fb 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -125,12 +125,21 @@ static void annotate_browser__write(struct ui_browser *browser, void *entry, int
 	int i, pcnt_width = annotate_browser__pcnt_width(ab);
 	double percent_max = 0.0;
 	char bf[256];
+	bool show_title = false;
 
 	for (i = 0; i < ab->nr_events; i++) {
 		if (bdl->samples[i].percent > percent_max)
 			percent_max = bdl->samples[i].percent;
 	}
 
+	if ((row == 0) && (dl->offset == -1 || percent_max == 0.0)) {
+		if (ab->have_cycles) {
+			if (dl->ipc == 0.0 && dl->cycles == 0)
+				show_title = true;
+		} else
+			show_title = true;
+	}
+
 	if (dl->offset != -1 && percent_max != 0.0) {
 		for (i = 0; i < ab->nr_events; i++) {
 			ui_browser__set_percent_color(browser,
@@ -146,18 +155,30 @@ static void annotate_browser__write(struct ui_browser *browser, void *entry, int
 		}
 	} else {
 		ui_browser__set_percent_color(browser, 0, current_entry);
-		ui_browser__write_nstring(browser, " ", 7 * ab->nr_events);
+
+		if (!show_title)
+			ui_browser__write_nstring(browser, " ",
+						  7 * ab->nr_events);
+		else
+			ui_browser__printf(browser, "%*s ", 6, "Percnt");
 	}
 	if (ab->have_cycles) {
 		if (dl->ipc)
 			ui_browser__printf(browser, "%*.2f ", IPC_WIDTH - 1, dl->ipc);
-		else
+		else if (!show_title)
 			ui_browser__write_nstring(browser, " ", IPC_WIDTH);
+		else
+			ui_browser__printf(browser, "%*s ",
+					   IPC_WIDTH - 1, "IPC");
+
 		if (dl->cycles)
 			ui_browser__printf(browser, "%*" PRIu64 " ",
 					   CYCLES_WIDTH - 1, dl->cycles);
-		else
+		else if (!show_title)
 			ui_browser__write_nstring(browser, " ", CYCLES_WIDTH);
+		else
+			ui_browser__printf(browser, "%*s ",
+					   CYCLES_WIDTH - 1, "Cycle");
 	}
 
 	SLsmg_write_char(' ');
-- 
2.7.4

[tip:perf/core] perf report: Remove unnecessary check in annotate_browser_write()

[tip-bot for Jin Yao]

Commit-ID:  c564f0db92b7f8d734ce530e42a540e12ae3d583
Gitweb:     http://git.kernel.org/tip/c564f0db92b7f8d734ce530e42a540e12ae3d583
Author:     Jin Yao <yao.jin@linux.intel.com>
AuthorDate: Thu, 4 May 2017 22:58:14 +0800
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 19 Jun 2017 15:14:57 -0300

perf report: Remove unnecessary check in annotate_browser_write()

In annotate_browser_write(),

        if (dl->offset != -1 && percent_max != 0.0) {
                if (percent_max != 0.0) {
			...
                }
                ...
        }

The second check of (percent_max != 0.0) is not necessary, remove it.

Signed-off-by: Yao Jin <yao.jin@linux.intel.com>
Acked-by: Milian Wolff <milian.wolff@kdab.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Kan Liang <kan.liang@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Yao Jin <yao.jin@linux.intel.com>
Link: http://lkml.kernel.org/r/1493909895-9668-2-git-send-email-yao.jin@linux.intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/ui/browsers/annotate.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index d990ad0..52c1e8d 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -132,21 +132,17 @@ static void annotate_browser__write(struct ui_browser *browser, void *entry, int
 	}
 
 	if (dl->offset != -1 && percent_max != 0.0) {
-		if (percent_max != 0.0) {
-			for (i = 0; i < ab->nr_events; i++) {
-				ui_browser__set_percent_color(browser,
-							bdl->samples[i].percent,
-							current_entry);
-				if (annotate_browser__opts.show_total_period) {
-					ui_browser__printf(browser, "%6" PRIu64 " ",
-							   bdl->samples[i].nr);
-				} else {
-					ui_browser__printf(browser, "%6.2f ",
-							   bdl->samples[i].percent);
-				}
+		for (i = 0; i < ab->nr_events; i++) {
+			ui_browser__set_percent_color(browser,
+						bdl->samples[i].percent,
+						current_entry);
+			if (annotate_browser__opts.show_total_period) {
+				ui_browser__printf(browser, "%6" PRIu64 " ",
+						   bdl->samples[i].nr);
+			} else {
+				ui_browser__printf(browser, "%6.2f ",
+						   bdl->samples[i].percent);
 			}
-		} else {
-			ui_browser__write_nstring(browser, " ", 7 * ab->nr_events);
 		}
 	} else {
 		ui_browser__set_percent_color(browser, 0, current_entry);

[tip:perf/core] perf annotate browser: Display titles in left frame

[tip-bot for Jin Yao]

Commit-ID:  ec27ae1892f7f8119ce82535ffcc2889ea3bb3d8
Gitweb:     http://git.kernel.org/tip/ec27ae1892f7f8119ce82535ffcc2889ea3bb3d8
Author:     Jin Yao <yao.jin@linux.intel.com>
AuthorDate: Thu, 4 May 2017 22:58:15 +0800
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 19 Jun 2017 15:14:57 -0300

perf annotate browser: Display titles in left frame

The annotate browser is divided into 2 frames. Left frame contains 3
columns (some platforms only have one column).

For example:

                   │26  int compute_flag()
                   │27  {
 22.80  1.20       │      sub    $0x8,%rsp
                   │25          int i;
                   │
                   │27          i = rand() % 2;
 22.78  1.20     1 │    → callq  rand@plt

While it's hard for user to understand what the data is.

This patch adds the titles "Percent", "IPC" and "Cycle" on columns.

Percent  IPC Cycle │
                   │25  __attribute__((noinline))
                   │26  int compute_flag()
                   │27  {
 22.80  1.20       │      sub    $0x8,%rsp
                   │25          int i;
                   │
                   │27          i = rand() % 2;
 22.78  1.20     1 │    → callq  rand@plt

The titles are displayed at row 0 of annotate browser if row 0 doesn't
have values of percent, ipc and cycle.

Signed-off-by: Yao Jin <yao.jin@linux.intel.com>
Acked-by: Milian Wolff <milian.wolff@kdab.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Kan Liang <kan.liang@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Yao Jin <yao.jin@linux.intel.com>
Link: http://lkml.kernel.org/r/1493909895-9668-3-git-send-email-yao.jin@linux.intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/ui/browsers/annotate.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index 52c1e8d..7a03389 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -125,12 +125,21 @@ static void annotate_browser__write(struct ui_browser *browser, void *entry, int
 	int i, pcnt_width = annotate_browser__pcnt_width(ab);
 	double percent_max = 0.0;
 	char bf[256];
+	bool show_title = false;
 
 	for (i = 0; i < ab->nr_events; i++) {
 		if (bdl->samples[i].percent > percent_max)
 			percent_max = bdl->samples[i].percent;
 	}
 
+	if ((row == 0) && (dl->offset == -1 || percent_max == 0.0)) {
+		if (ab->have_cycles) {
+			if (dl->ipc == 0.0 && dl->cycles == 0)
+				show_title = true;
+		} else
+			show_title = true;
+	}
+
 	if (dl->offset != -1 && percent_max != 0.0) {
 		for (i = 0; i < ab->nr_events; i++) {
 			ui_browser__set_percent_color(browser,
@@ -146,18 +155,27 @@ static void annotate_browser__write(struct ui_browser *browser, void *entry, int
 		}
 	} else {
 		ui_browser__set_percent_color(browser, 0, current_entry);
-		ui_browser__write_nstring(browser, " ", 7 * ab->nr_events);
+
+		if (!show_title)
+			ui_browser__write_nstring(browser, " ", 7 * ab->nr_events);
+		else
+			ui_browser__printf(browser, "%*s", 7, "Percent");
 	}
 	if (ab->have_cycles) {
 		if (dl->ipc)
 			ui_browser__printf(browser, "%*.2f ", IPC_WIDTH - 1, dl->ipc);
-		else
+		else if (!show_title)
 			ui_browser__write_nstring(browser, " ", IPC_WIDTH);
+		else
+			ui_browser__printf(browser, "%*s ", IPC_WIDTH - 1, "IPC");
+
 		if (dl->cycles)
 			ui_browser__printf(browser, "%*" PRIu64 " ",
 					   CYCLES_WIDTH - 1, dl->cycles);
-		else
+		else if (!show_title)
 			ui_browser__write_nstring(browser, " ", CYCLES_WIDTH);
+		else
+			ui_browser__printf(browser, "%*s ", CYCLES_WIDTH - 1, "Cycle");
 	}
 
 	SLsmg_write_char(' ');