Subject: Re: [PATCH v3 0/5] perf report: Show inline stack
To: Milian Wolff <milian.wolff@kdab.com>
References: <1484905166-10609-1-git-send-email-yao.jin@linux.intel.com>
 <47669ba5-6f26-2842-e274-949411c5cb36@linux.intel.com>
 <7369636.07Z93mMh8K@agathebauer>
Cc: acme@kernel.org, jolsa@kernel.org, Linux-kernel@vger.kernel.org,
        ak@linux.intel.com, kan.liang@intel.com
From: "Jin, Yao" <yao.jin@linux.intel.com>
Message-ID: <bd2f8e05-86b0-f419-0d7d-8a7d453a71e9@linux.intel.com>
Date: Fri, 3 Mar 2017 10:38:40 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101
 Thunderbird/45.7.1
MIME-Version: 1.0
In-Reply-To: <7369636.07Z93mMh8K@agathebauer>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3184
Lines: 102

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<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.wolff@kdab.com>
>
> to all patches in this series.
>
> Many thanks!
>