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<double> (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 <milian.wo...@kdab.com>
to all patches in this series.
Many thanks!
--
Milian Wolff | milian.wo...@kdab.com | Software Engineer
KDAB (Deutschland) GmbH&Co KG, a KDAB Group company
Tel: +49-30-521325470
KDAB - The Qt Experts
smime.p7s
Description: S/MIME cryptographic signature
