Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1656804 > unrolled thread

Re: [PATCH 6/7] perf report: mark inlined frames in output by " (inlined)" suffix

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 6/7] perf report: mark inlined frames in output by " (inlined)" suffix Milian Wolff <milian.wolff@kdab.com> - 2017-06-03 16:00 +0200
    Re: [PATCH 6/7] perf report: mark inlined frames in output by "  (inlined)" suffix Namhyung Kim <namhyung@kernel.org> - 2017-06-06 03:40 +0200
      Re: [PATCH 6/7] perf report: mark inlined frames in output by " (inlined)" suffix Milian Wolff <milian.wolff@kdab.com> - 2017-06-06 09:30 +0200
        Re: [PATCH 6/7] perf report: mark inlined frames in output by "  (inlined)" suffix Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-06-06 22:00 +0200

#1656804 — Re: [PATCH 6/7] perf report: mark inlined frames in output by " (inlined)" suffix

On Montag, 22. Mai 2017 14:48:18 CEST Namhyung Kim wrote:
> On Thu, May 18, 2017 at 09:34:10PM +0200, Milian Wolff wrote:
> > The original patch that introduced inline frame output in the
> > various browsers used this suffix already. The new centralized
> > approach that uses fake symbols for inlined frames was missing
> > this approach so far.
> > 
> > Instead of changing the symbol name itself, we only print the
> > suffix where needed. This allows us to efficiently lookup
> > the symbol for a given name without first having to append the
> > suffix before the lookup.
> 
> You also need to do same thing for hist_entry__sym_snprintf().

Hey Namhyung,

I'm working on the next iteration of this patch series, the WIP can be found 
here: https://github.com/milianw/linux/tree/wip/distinguish-inliners.

I just stumbled upon another issue:

~~~~~
perf report --inline -s srcline -g srcline  --stdio -g none
    61.22%     0.00%  main+378
    61.22%     0.00%  std::_Norm_helper<true>::_S_do_it<double>+378
    61.22%     0.00%  std::__complex_abs+378
    61.22%     0.00%  std::abs<double>+378
    61.22%     0.00%  std::norm<double>+378
~~~~~

The problem here is that the srcline in the hist is detached from what we 
actually produce in the callchain nodes. We will have to somehow hand through 
the srcline from the callchain node to the hist entry, but this seems to be a 
super large invasive change - which is why I need your input on how to resolve 
this.

The current API seems to pass the data around mostly using the addr_location 
struct, which is usually constructed on the stack and not always memset to 
zero. As such, my initial plan of adding a srcline member there would require 
me to go through all the code to ensure that we memset the struct to zero...

Alternatively, I'd have to change the API of hist_iter_ops, to let the 
callback take another `const char **srcline` out parameter. This is also going 
to be quite a large invasive change.

Do you have any suggestions on how to make this work?

Thanks

-- 
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

[toc] | [next] | [standalone]

#1658307 — Re: [PATCH 6/7] perf report: mark inlined frames in output by " (inlined)" suffix

On Sat, Jun 3, 2017 at 10:51 PM, Milian Wolff <milian.wolff@kdab.com> wrote:
> On Montag, 22. Mai 2017 14:48:18 CEST Namhyung Kim wrote:
>> On Thu, May 18, 2017 at 09:34:10PM +0200, Milian Wolff wrote:
>> > The original patch that introduced inline frame output in the
>> > various browsers used this suffix already. The new centralized
>> > approach that uses fake symbols for inlined frames was missing
>> > this approach so far.
>> >
>> > Instead of changing the symbol name itself, we only print the
>> > suffix where needed. This allows us to efficiently lookup
>> > the symbol for a given name without first having to append the
>> > suffix before the lookup.
>>
>> You also need to do same thing for hist_entry__sym_snprintf().
>
> Hey Namhyung,
>
> I'm working on the next iteration of this patch series, the WIP can be found
> here: https://github.com/milianw/linux/tree/wip/distinguish-inliners.
>
> I just stumbled upon another issue:
>
> ~~~~~
> perf report --inline -s srcline -g srcline  --stdio -g none
>     61.22%     0.00%  main+378
>     61.22%     0.00%  std::_Norm_helper<true>::_S_do_it<double>+378
>     61.22%     0.00%  std::__complex_abs+378
>     61.22%     0.00%  std::abs<double>+378
>     61.22%     0.00%  std::norm<double>+378
> ~~~~~
>
> The problem here is that the srcline in the hist is detached from what we
> actually produce in the callchain nodes. We will have to somehow hand through
> the srcline from the callchain node to the hist entry, but this seems to be a
> super large invasive change - which is why I need your input on how to resolve
> this.
>
> The current API seems to pass the data around mostly using the addr_location
> struct, which is usually constructed on the stack and not always memset to
> zero. As such, my initial plan of adding a srcline member there would require
> me to go through all the code to ensure that we memset the struct to zero...
>
> Alternatively, I'd have to change the API of hist_iter_ops, to let the
> callback take another `const char **srcline` out parameter. This is also going
> to be quite a large invasive change.
>
> Do you have any suggestions on how to make this work?

I think passing srcline via addr_location might not be very invasive
since it calls machine__resolve() in most cases.  Missing places that
set al->sym should set al->srcline as well IMHO.

Thanks,
Namhyung

[toc] | [prev] | [next] | [standalone]

#1658443

[Multipart message — attachments visible in raw view] — view raw

On Tuesday, June 6, 2017 3:33:49 AM CEST Namhyung Kim wrote:
> On Sat, Jun 3, 2017 at 10:51 PM, Milian Wolff <milian.wolff@kdab.com> wrote:
> > On Montag, 22. Mai 2017 14:48:18 CEST Namhyung Kim wrote:
> >> On Thu, May 18, 2017 at 09:34:10PM +0200, Milian Wolff wrote:
> >> > The original patch that introduced inline frame output in the
> >> > various browsers used this suffix already. The new centralized
> >> > approach that uses fake symbols for inlined frames was missing
> >> > this approach so far.
> >> > 
> >> > Instead of changing the symbol name itself, we only print the
> >> > suffix where needed. This allows us to efficiently lookup
> >> > the symbol for a given name without first having to append the
> >> > suffix before the lookup.
> >> 
> >> You also need to do same thing for hist_entry__sym_snprintf().
> > 
> > Hey Namhyung,
> > 
> > I'm working on the next iteration of this patch series, the WIP can be
> > found here:
> > https://github.com/milianw/linux/tree/wip/distinguish-inliners.
> > 
> > I just stumbled upon another issue:
> > 
> > ~~~~~
> > perf report --inline -s srcline -g srcline  --stdio -g none
> > 
> >     61.22%     0.00%  main+378
> >     61.22%     0.00%  std::_Norm_helper<true>::_S_do_it<double>+378
> >     61.22%     0.00%  std::__complex_abs+378
> >     61.22%     0.00%  std::abs<double>+378
> >     61.22%     0.00%  std::norm<double>+378
> > 
> > ~~~~~
> > 
> > The problem here is that the srcline in the hist is detached from what we
> > actually produce in the callchain nodes. We will have to somehow hand
> > through the srcline from the callchain node to the hist entry, but this
> > seems to be a super large invasive change - which is why I need your
> > input on how to resolve this.
> > 
> > The current API seems to pass the data around mostly using the
> > addr_location struct, which is usually constructed on the stack and not
> > always memset to zero. As such, my initial plan of adding a srcline
> > member there would require me to go through all the code to ensure that
> > we memset the struct to zero...
> > 
> > Alternatively, I'd have to change the API of hist_iter_ops, to let the
> > callback take another `const char **srcline` out parameter. This is also
> > going to be quite a large invasive change.
> > 
> > Do you have any suggestions on how to make this work?
> 
> I think passing srcline via addr_location might not be very invasive
> since it calls machine__resolve() in most cases.  Missing places that
> set al->sym should set al->srcline as well IMHO.

OK, perfect - I'll implement that then.

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

[toc] | [prev] | [next] | [standalone]

#1659161 — Re: [PATCH 6/7] perf report: mark inlined frames in output by " (inlined)" suffix

Em Tue, Jun 06, 2017 at 09:26:47AM +0200, Milian Wolff escreveu:
> On Tuesday, June 6, 2017 3:33:49 AM CEST Namhyung Kim wrote:
> > On Sat, Jun 3, 2017 at 10:51 PM, Milian Wolff <milian.wolff@kdab.com> wrote:
> > > The current API seems to pass the data around mostly using the
> > > addr_location struct, which is usually constructed on the stack and not
> > > always memset to zero. As such, my initial plan of adding a srcline
> > > member there would require me to go through all the code to ensure that
> > > we memset the struct to zero...

> > > Alternatively, I'd have to change the API of hist_iter_ops, to let the
> > > callback take another `const char **srcline` out parameter. This is also
> > > going to be quite a large invasive change.

> > > Do you have any suggestions on how to make this work?

> > I think passing srcline via addr_location might not be very invasive
> > since it calls machine__resolve() in most cases.  Missing places that
> > set al->sym should set al->srcline as well IMHO.

I haven't looked if it would be invasive or not, good that it isn't, but
then I think addr_location is the right place for this to be stored.
 
> OK, perfect - I'll implement that then.

Thanks,

- Arnaldo

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web