Re: [PATCH v3 0/5] perf report: Show inline stack

[Jin, Yao]

Hi Wolff,

Thanks so much for your testing. I also wish this feature could be 
upstreamed.


I will send a v4 series soon. In v4, It removes the options 
"--inline-line" and "--inline-name".


It just uses a new option "--inline" to print the inline function 
information. The policy is if the inline function name can be resolved 
then print the function name otherwise it prints the source line number.


For example:
perf report --stdio --inline

It prints:

0.69% 0.00%  inline   ld-2.23.so   [.] dl_main
   |
   ---dl_main
  |
   --0.56%--_dl_relocate_object
 |
 ---_dl_relocate_object (inline)
elf_dynamic_do_Rela (inline)

Thanks

Jin Yao

On 3/3/2017 5:42 AM, Milian Wolff wrote:

On Dienstag, 21. Februar 2017 01:28:17 CET Jin, Yao wrote:

Hi,

Any comments for this patch series?

Sorry for the delay. I just tested it again.

Overall, this is a clear improvement, so I'm all for getting this
functionality in.

But from a usability point of view, I still have the some of the issues that I
have raised in the past:

a) --inline should be a boolean setting that enables inline resolution on
demand

b) the other callgraph settings and formatting should be used for inlined
frames, i.e.

- instead of `perf report --inline-name`
   it should be: `perf report --inline -g function`
   and since `-g function` is the default, it would be the same as:
   `perf report --inline`

- instead of `perf report --inline-line -g address`
   it should be: `perf report --inline -g address`

Again: As a user of `perf report`, I do not care whether a frame is an inlined
one or a non-inlined one - both should be grouped and displayed the same way.

I.e. this is unusable (imo):

~
perf report --inline-line --stdio

 99.81%35.99%  cpp-inlining  cpp-inlining  [.] main
 |
 |--63.82%--main
|
---/home/milian/projects/kdab/rnd/hotspot/tests/test-
clients/cpp-inlining/main.cpp:39 (inline)
   /usr/include/c++/6.3.1/complex:664 (inline)
 |  |
 |  |--63.19%--hypot
 |  |  |
 |  |   --58.04%--__hypot_finite
 |  |
 |   --0.62%--cabs
~

Dito for this:

~
perf report --stdio --inline-name -g address --stdio

 99.81%35.99%  cpp-inlining  cpp-inlining  [.] main
 |
 |--63.82%--main complex:655
|
---main (inline)
   std::norm (inline)
~

But, again, even with these gripes, I think it's a very useful feature and I
would like to see it integrated upstream. From my POV, you can add

Tested-by: Milian Wolff 

to all patches in this series.

Many thanks!

Re: [PATCH v3 0/5] perf report: Show inline stack

[Jin, Yao]

Hi Wolff,

Thanks so much for your testing. I also wish this feature could be 
upstreamed.


I will send a v4 series soon. In v4, It removes the options 
"--inline-line" and "--inline-name".


It just uses a new option "--inline" to print the inline function 
information. The policy is if the inline function name can be resolved 
then print the function name otherwise it prints the source line number.


For example:
perf report --stdio --inline

It prints:

0.69% 0.00%  inline   ld-2.23.so   [.] dl_main
   |
   ---dl_main
  |
   --0.56%--_dl_relocate_object
 |
 ---_dl_relocate_object (inline)
elf_dynamic_do_Rela (inline)

Thanks

Jin Yao

On 3/3/2017 5:42 AM, Milian Wolff wrote:

On Dienstag, 21. Februar 2017 01:28:17 CET Jin, Yao wrote:

Hi,

Any comments for this patch series?

Sorry for the delay. I just tested it again.

Overall, this is a clear improvement, so I'm all for getting this
functionality in.

But from a usability point of view, I still have the some of the issues that I
have raised in the past:

a) --inline should be a boolean setting that enables inline resolution on
demand

b) the other callgraph settings and formatting should be used for inlined
frames, i.e.

- instead of `perf report --inline-name`
   it should be: `perf report --inline -g function`
   and since `-g function` is the default, it would be the same as:
   `perf report --inline`

- instead of `perf report --inline-line -g address`
   it should be: `perf report --inline -g address`

Again: As a user of `perf report`, I do not care whether a frame is an inlined
one or a non-inlined one - both should be grouped and displayed the same way.

I.e. this is unusable (imo):

~
perf report --inline-line --stdio

 99.81%35.99%  cpp-inlining  cpp-inlining  [.] main
 |
 |--63.82%--main
|
---/home/milian/projects/kdab/rnd/hotspot/tests/test-
clients/cpp-inlining/main.cpp:39 (inline)
   /usr/include/c++/6.3.1/complex:664 (inline)
 |  |
 |  |--63.19%--hypot
 |  |  |
 |  |   --58.04%--__hypot_finite
 |  |
 |   --0.62%--cabs
~

Dito for this:

~
perf report --stdio --inline-name -g address --stdio

 99.81%35.99%  cpp-inlining  cpp-inlining  [.] main
 |
 |--63.82%--main complex:655
|
---main (inline)
   std::norm (inline)
~

But, again, even with these gripes, I think it's a very useful feature and I
would like to see it integrated upstream. From my POV, you can add

Tested-by: Milian Wolff 

to all patches in this series.

Many thanks!

Re: [PATCH v3 0/5] perf report: Show inline stack

[Milian Wolff]

On Dienstag, 21. Februar 2017 01:28:17 CET Jin, Yao wrote:
> Hi,
> 
> Any comments for this patch series?

Sorry for the delay. I just tested it again.

Overall, this is a clear improvement, so I'm all for getting this 
functionality in.

But from a usability point of view, I still have the some of the issues that I 
have raised in the past:

a) --inline should be a boolean setting that enables inline resolution on 
demand

b) the other callgraph settings and formatting should be used for inlined 
frames, i.e.

- instead of `perf report --inline-name`
  it should be: `perf report --inline -g function`
  and since `-g function` is the default, it would be the same as:
  `perf report --inline`

- instead of `perf report --inline-line -g address`
  it should be: `perf report --inline -g address`

Again: As a user of `perf report`, I do not care whether a frame is an inlined 
one or a non-inlined one - both should be grouped and displayed the same way. 

I.e. this is unusable (imo):

~
perf report --inline-line --stdio

99.81%35.99%  cpp-inlining  cpp-inlining  [.] main
|  
|--63.82%--main
   |
   ---/home/milian/projects/kdab/rnd/hotspot/tests/test-
clients/cpp-inlining/main.cpp:39 (inline)
  /usr/include/c++/6.3.1/complex:664 (inline)
|  |  
|  |--63.19%--hypot
|  |  |  
|  |   --58.04%--__hypot_finite
|  |  
|   --0.62%--cabs
~

Dito for this:

~
perf report --stdio --inline-name -g address --stdio

99.81%35.99%  cpp-inlining  cpp-inlining  [.] main
|  
|--63.82%--main complex:655
   |
   ---main (inline)
  std::norm (inline)
~

But, again, even with these gripes, I think it's a very useful feature and I 
would like to see it integrated upstream. From my POV, you can add

Tested-by: Milian Wolff 

to all patches in this series.

Many thanks!

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

smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH v3 0/5] perf report: Show inline stack

[Milian Wolff]

On Dienstag, 21. Februar 2017 01:28:17 CET Jin, Yao wrote:
> Hi,
> 
> Any comments for this patch series?

Sorry for the delay. I just tested it again.

Overall, this is a clear improvement, so I'm all for getting this 
functionality in.

But from a usability point of view, I still have the some of the issues that I 
have raised in the past:

a) --inline should be a boolean setting that enables inline resolution on 
demand

b) the other callgraph settings and formatting should be used for inlined 
frames, i.e.

- instead of `perf report --inline-name`
  it should be: `perf report --inline -g function`
  and since `-g function` is the default, it would be the same as:
  `perf report --inline`

- instead of `perf report --inline-line -g address`
  it should be: `perf report --inline -g address`

Again: As a user of `perf report`, I do not care whether a frame is an inlined 
one or a non-inlined one - both should be grouped and displayed the same way. 

I.e. this is unusable (imo):

~
perf report --inline-line --stdio

99.81%35.99%  cpp-inlining  cpp-inlining  [.] main
|  
|--63.82%--main
   |
   ---/home/milian/projects/kdab/rnd/hotspot/tests/test-
clients/cpp-inlining/main.cpp:39 (inline)
  /usr/include/c++/6.3.1/complex:664 (inline)
|  |  
|  |--63.19%--hypot
|  |  |  
|  |   --58.04%--__hypot_finite
|  |  
|   --0.62%--cabs
~

Dito for this:

~
perf report --stdio --inline-name -g address --stdio

99.81%35.99%  cpp-inlining  cpp-inlining  [.] main
|  
|--63.82%--main complex:655
   |
   ---main (inline)
  std::norm (inline)
~

But, again, even with these gripes, I think it's a very useful feature and I 
would like to see it integrated upstream. From my POV, you can add

Tested-by: Milian Wolff 

to all patches in this series.

Many thanks!

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

smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH v3 0/5] perf report: Show inline stack

[Jin, Yao]

Hi,

Any comments for this patch series?

Thanks

Jin Yao


On 1/20/2017 5:39 PM, Jin Yao wrote:

v3: Iterate on RIPs of all callchain entries to check if the RIP is in
 inline functions.

 Reverse the order of the inliner printout if necessary.

 Provide new options "--inline-line" / "--inline-name" to print
 inline function name or print inline function source line.

v2: Thanks so much for Arnaldo's comments!
 The modifications are:

 1. Divide v1 patch "perf report: Find the inline stack for a
given address" into 2 patches:
a. perf report: Refactor common code in srcline.c
b. perf report: Find the inline stack for a given address

Some function names are changed:
dso_name_get -> dso__name
ilist_apend -> inline_list__append
get_inline_node -> dso__parse_addr_inlines
free_inline_node -> inline_node__delete

 2. Since the function name are changed, update following patches
accordingly.
a. perf report: Show inline stack in stdio mode
b. perf report: Show inline stack in browser mode

 3. Rebase to latest perf/core branch. This patch is impacted.
a. perf report: Create a new option "--inline"

v1: Initial post

It would be useful for perf to support a mode to query the
inline stack for callgraph addresses. This would simplify
finding the right code in code that does a lot of inlining.

For example, the c code:

static inline void f3(void)
{
 int i;
 for (i = 0; i < 1000;) {

 if(i%2)
 i++;
 else
 i++;
 }
 printf("hello f3\n");   /* D */
}

/* < CALLCHAIN: f2 <- f1 > */
static inline void f2(void)
{
 int i;
 for (i = 0; i < 100; i++) {
 f3();   /* C */
 }
}

/* < CALLCHAIN: f1 <- main > */
static inline void f1(void)
{
 int i;
 for (i = 0; i < 100; i++) {
 f2();   /* B */
 }
}

/* < CALLCHAIN: main <- TOP > */
int main()
{
 struct timeval tv;
 time_t start, end;

 gettimeofday(, NULL);
 start = end = tv.tv_sec;
 while((end - start) < 5) {
 f1();   /* A */
 gettimeofday(, NULL);
 end = tv.tv_sec;
 }
 return 0;
}

The printed inline stack is:

0.05%  test2test2  [.] main
|
---/home/perf-dev/lck-2867/test/test2.c:27 (inline)
   /home/perf-dev/lck-2867/test/test2.c:35 (inline)
   /home/perf-dev/lck-2867/test/test2.c:45 (inline)
   /home/perf-dev/lck-2867/test/test2.c:61 (inline)

I tag A/B/C/D in above c code to indicate the source line,
actually the inline stack is equal to:

0.05%  test2test2  [.] main
|
---D
   C
   B
   A

Jin Yao (5):
   perf report: Refactor common code in srcline.c
   perf report: Find the inline stack for a given address
   perf report: Create new inline options
   perf report: Show inline stack in stdio mode
   perf report: Show inline stack in browser mode

  tools/perf/Documentation/perf-report.txt |   8 ++
  tools/perf/builtin-report.c  |   4 +
  tools/perf/ui/browsers/hists.c   | 170 --
  tools/perf/ui/stdio/hist.c   |  75 +-
  tools/perf/util/hist.c   |   5 +
  tools/perf/util/sort.h   |   1 +
  tools/perf/util/srcline.c| 237 +++
  tools/perf/util/symbol-elf.c |   5 +
  tools/perf/util/symbol.h |   6 +-
  tools/perf/util/util.h   |  16 +++
  10 files changed, 489 insertions(+), 38 deletions(-)

Re: [PATCH v3 0/5] perf report: Show inline stack

[Jin, Yao]

Hi,

Any comments for this patch series?

Thanks

Jin Yao


On 1/20/2017 5:39 PM, Jin Yao wrote:

v3: Iterate on RIPs of all callchain entries to check if the RIP is in
 inline functions.

 Reverse the order of the inliner printout if necessary.

 Provide new options "--inline-line" / "--inline-name" to print
 inline function name or print inline function source line.

v2: Thanks so much for Arnaldo's comments!
 The modifications are:

 1. Divide v1 patch "perf report: Find the inline stack for a
given address" into 2 patches:
a. perf report: Refactor common code in srcline.c
b. perf report: Find the inline stack for a given address

Some function names are changed:
dso_name_get -> dso__name
ilist_apend -> inline_list__append
get_inline_node -> dso__parse_addr_inlines
free_inline_node -> inline_node__delete

 2. Since the function name are changed, update following patches
accordingly.
a. perf report: Show inline stack in stdio mode
b. perf report: Show inline stack in browser mode

 3. Rebase to latest perf/core branch. This patch is impacted.
a. perf report: Create a new option "--inline"

v1: Initial post

It would be useful for perf to support a mode to query the
inline stack for callgraph addresses. This would simplify
finding the right code in code that does a lot of inlining.

For example, the c code:

static inline void f3(void)
{
 int i;
 for (i = 0; i < 1000;) {

 if(i%2)
 i++;
 else
 i++;
 }
 printf("hello f3\n");   /* D */
}

/* < CALLCHAIN: f2 <- f1 > */
static inline void f2(void)
{
 int i;
 for (i = 0; i < 100; i++) {
 f3();   /* C */
 }
}

/* < CALLCHAIN: f1 <- main > */
static inline void f1(void)
{
 int i;
 for (i = 0; i < 100; i++) {
 f2();   /* B */
 }
}

/* < CALLCHAIN: main <- TOP > */
int main()
{
 struct timeval tv;
 time_t start, end;

 gettimeofday(, NULL);
 start = end = tv.tv_sec;
 while((end - start) < 5) {
 f1();   /* A */
 gettimeofday(, NULL);
 end = tv.tv_sec;
 }
 return 0;
}

The printed inline stack is:

0.05%  test2test2  [.] main
|
---/home/perf-dev/lck-2867/test/test2.c:27 (inline)
   /home/perf-dev/lck-2867/test/test2.c:35 (inline)
   /home/perf-dev/lck-2867/test/test2.c:45 (inline)
   /home/perf-dev/lck-2867/test/test2.c:61 (inline)

I tag A/B/C/D in above c code to indicate the source line,
actually the inline stack is equal to:

0.05%  test2test2  [.] main
|
---D
   C
   B
   A

Jin Yao (5):
   perf report: Refactor common code in srcline.c
   perf report: Find the inline stack for a given address
   perf report: Create new inline options
   perf report: Show inline stack in stdio mode
   perf report: Show inline stack in browser mode

  tools/perf/Documentation/perf-report.txt |   8 ++
  tools/perf/builtin-report.c  |   4 +
  tools/perf/ui/browsers/hists.c   | 170 --
  tools/perf/ui/stdio/hist.c   |  75 +-
  tools/perf/util/hist.c   |   5 +
  tools/perf/util/sort.h   |   1 +
  tools/perf/util/srcline.c| 237 +++
  tools/perf/util/symbol-elf.c |   5 +
  tools/perf/util/symbol.h |   6 +-
  tools/perf/util/util.h   |  16 +++
  10 files changed, 489 insertions(+), 38 deletions(-)