[Question] about symbol__get_source_line() in util/annotate.c

[Question] about symbol__get_source_line() in util/annotate.c

[Taeung Song]

Hi Namhyung,

I have two question about the code util/annotate.c:1653~1676

https://git.kernel.org/cgit/linux/kernel/git/acme/linux.git/tree/tools/perf/util/annotate.c?h=perf/core#n1653

1653         for (i = 0; i < len; i++) {
1654                 u64 offset;
1655                 double percent_max = 0.0;
1656
1657                 src_line->nr_pcnt = nr_pcnt;
1658
1659                 for (k = 0; k < nr_pcnt; k++) {
1660                         h = annotation__histogram(notes, evidx + k);
1661                         src_line->samples[k].percent = 100.0 * 
h->addr[i] / h->sum;
1662
1663                         if (src_line->samples[k].percent > percent_max)
1664                                 percent_max = 
src_line->samples[k].percent;
1665                 }
1666
1667                 if (percent_max <= 0.5)
1668                         goto next;
1669
1670                 offset = start + i;
1671                 src_line->path = get_srcline(map->dso, offset, 
NULL, false);
1672                 insert_source_line(&tmp_root, src_line);
1673
1674         next:
1675                 src_line = (void *)src_line + sizeof_src_line;
1676         }


1) Why use 'offset = start + i;' ?
For example,
There are addresses matched with test.c:26 as below,

     400816:       push   %rbp
     400817:       mov    %rsp,%rbp
     40081a:       mov    %edi,-0x24(%rbp)
     40081d:       mov    %rsi,-0x30(%rbp)

If using 'offset = start + i;' in the above for loop,
needless addresses can be checked.


     i=0, 400816
     i=1, 400817
     i=2, 400818 (nonvalidated)
     i=3, 400819 (nonvalidated)
     i=4, 40081a
     i=5, 40081b (nonvalidated)
     i=6, 40081c (nonvalidated)
     i=7, 40081d

So I think it is better to use dissemble_line array such as
(in order to check only validated addresses.)

     list_for_each_entry(dl, &notes->src->source, node)

What about this ?

2) Why use the if statement as below ?

     if (percent_max <= 0.5)
             goto next;

I think it is more correct to use 0.0 instead of 0.5

What do you think about that ?

Thanks,
Taeung

Re: [Question] about symbol__get_source_line() in util/annotate.c

[Namhyung Kim]

Hi Taeung,

On Thu, Feb 23, 2017 at 12:31:08PM +0900, Taeung Song wrote:
> Hi Namhyung,
> 
> I have two question about the code util/annotate.c:1653~1676
> 
> https://git.kernel.org/cgit/linux/kernel/git/acme/linux.git/tree/tools/perf/util/annotate.c?h=perf/core#n1653
> 
> 1653         for (i = 0; i < len; i++) {
> 1654                 u64 offset;
> 1655                 double percent_max = 0.0;
> 1656
> 1657                 src_line->nr_pcnt = nr_pcnt;
> 1658
> 1659                 for (k = 0; k < nr_pcnt; k++) {
> 1660                         h = annotation__histogram(notes, evidx + k);
> 1661                         src_line->samples[k].percent = 100.0 * h->addr[i] / h->sum;
> 1662
> 1663                         if (src_line->samples[k].percent > percent_max)
> 1664                                 percent_max = src_line->samples[k].percent;
> 1665                 }
> 1666
> 1667                 if (percent_max <= 0.5)
> 1668                         goto next;
> 1669
> 1670                 offset = start + i;
> 1671                 src_line->path = get_srcline(map->dso, offset, NULL, false);
> 1672                 insert_source_line(&tmp_root, src_line);
> 1673
> 1674         next:
> 1675                 src_line = (void *)src_line + sizeof_src_line;
> 1676         }
> 
> 
> 1) Why use 'offset = start + i;' ?
> For example,
> There are addresses matched with test.c:26 as below,
> 
>     400816:       push   %rbp
>     400817:       mov    %rsp,%rbp
>     40081a:       mov    %edi,-0x24(%rbp)
>     40081d:       mov    %rsi,-0x30(%rbp)
> 
> If using 'offset = start + i;' in the above for loop,
> needless addresses can be checked.

Right, that's not nice.

> 
> 
>     i=0, 400816
>     i=1, 400817
>     i=2, 400818 (nonvalidated)
>     i=3, 400819 (nonvalidated)
>     i=4, 40081a
>     i=5, 40081b (nonvalidated)
>     i=6, 40081c (nonvalidated)
>     i=7, 40081d
> 
> So I think it is better to use dissemble_line array such as
> (in order to check only validated addresses.)
> 
>     list_for_each_entry(dl, &notes->src->source, node)
> 
> What about this ?

I agree with you.  Maybe we can get rid of the source_line struct
entirely.

> 
> 2) Why use the if statement as below ?
> 
>     if (percent_max <= 0.5)
>             goto next;
> 
> I think it is more correct to use 0.0 instead of 0.5
> 
> What do you think about that ?

Well, I think that the summary line doesn't want to show too many
(small) lines.  Using 0.0 instead seems meaningless though.

Thanks,
Namhyung

Re: [Question] about symbol__get_source_line() in util/annotate.c

[Namhyung Kim]

On Thu, Feb 23, 2017 at 02:14:13PM +0900, Namhyung Kim wrote:
> Hi Taeung,
> 
> On Thu, Feb 23, 2017 at 12:31:08PM +0900, Taeung Song wrote:
> > 2) Why use the if statement as below ?
> > 
> >     if (percent_max <= 0.5)
> >             goto next;
> > 
> > I think it is more correct to use 0.0 instead of 0.5
> > 
> > What do you think about that ?
> 
> Well, I think that the summary line doesn't want to show too many
> (small) lines.  Using 0.0 instead seems meaningless though.

Ah, that includes 0 percent..

Anyway I can see print_summary() uses MIN_GREEN for this.  We can use
it here instead of the magic number, or might change it to something
different if needed.

Thanks,
Namhyung

Re: [Question] about symbol__get_source_line() in util/annotate.c

[Taeung Song]

On 02/23/2017 02:14 PM, Namhyung Kim wrote:
> Hi Taeung,
>
> On Thu, Feb 23, 2017 at 12:31:08PM +0900, Taeung Song wrote:
>> Hi Namhyung,
>>
>> I have two question about the code util/annotate.c:1653~1676
>>
>> https://git.kernel.org/cgit/linux/kernel/git/acme/linux.git/tree/tools/perf/util/annotate.c?h=perf/core#n1653
>>
>> 1653         for (i = 0; i < len; i++) {
>> 1654                 u64 offset;
>> 1655                 double percent_max = 0.0;
>> 1656
>> 1657                 src_line->nr_pcnt = nr_pcnt;
>> 1658
>> 1659                 for (k = 0; k < nr_pcnt; k++) {
>> 1660                         h = annotation__histogram(notes, evidx + k);
>> 1661                         src_line->samples[k].percent = 100.0 * h->addr[i] / h->sum;
>> 1662
>> 1663                         if (src_line->samples[k].percent > percent_max)
>> 1664                                 percent_max = src_line->samples[k].percent;
>> 1665                 }
>> 1666
>> 1667                 if (percent_max <= 0.5)
>> 1668                         goto next;
>> 1669
>> 1670                 offset = start + i;
>> 1671                 src_line->path = get_srcline(map->dso, offset, NULL, false);
>> 1672                 insert_source_line(&tmp_root, src_line);
>> 1673
>> 1674         next:
>> 1675                 src_line = (void *)src_line + sizeof_src_line;
>> 1676         }
>>
>>
>> 1) Why use 'offset = start + i;' ?
>> For example,
>> There are addresses matched with test.c:26 as below,
>>
>>     400816:       push   %rbp
>>     400817:       mov    %rsp,%rbp
>>     40081a:       mov    %edi,-0x24(%rbp)
>>     40081d:       mov    %rsi,-0x30(%rbp)
>>
>> If using 'offset = start + i;' in the above for loop,
>> needless addresses can be checked.
>
> Right, that's not nice.
>
>>
>>
>>     i=0, 400816
>>     i=1, 400817
>>     i=2, 400818 (nonvalidated)
>>     i=3, 400819 (nonvalidated)
>>     i=4, 40081a
>>     i=5, 40081b (nonvalidated)
>>     i=6, 40081c (nonvalidated)
>>     i=7, 40081d
>>
>> So I think it is better to use dissemble_line array such as
>> (in order to check only validated addresses.)
>>
>>     list_for_each_entry(dl, &notes->src->source, node)
>>
>> What about this ?
>
> I agree with you.  Maybe we can get rid of the source_line struct
> entirely.

Sounds good. the source_line struct is a bit obstacle..

>>
>> 2) Why use the if statement as below ?
>>
>>     if (percent_max <= 0.5)
>>             goto next;
>>
>> I think it is more correct to use 0.0 instead of 0.5
>>
>> What do you think about that ?
>
> Well, I think that the summary line doesn't want to show too many
> (small) lines.  Using 0.0 instead seems meaningless though.
>
> Thanks,
> Namhyung
>

I got it!

Thanks,
Taeung

Re: [Question] about symbol__get_source_line() in util/annotate.c

[Taeung Song]

On 02/23/2017 02:26 PM, Namhyung Kim wrote:
> On Thu, Feb 23, 2017 at 02:14:13PM +0900, Namhyung Kim wrote:
>> Hi Taeung,
>>
>> On Thu, Feb 23, 2017 at 12:31:08PM +0900, Taeung Song wrote:
>>> 2) Why use the if statement as below ?
>>>
>>>     if (percent_max <= 0.5)
>>>             goto next;
>>>
>>> I think it is more correct to use 0.0 instead of 0.5
>>>
>>> What do you think about that ?
>>
>> Well, I think that the summary line doesn't want to show too many
>> (small) lines.  Using 0.0 instead seems meaningless though.
>
> Ah, that includes 0 percent..
>
> Anyway I can see print_summary() uses MIN_GREEN for this.  We can use
> it here instead of the magic number, or might change it to something
> different if needed.
>
> Thanks,
> Namhyung
>

I understood!
Thanks for your answer

P.S.
Hum.. And I think there are many things that seem to need to be
modified in util/annotate.c
The for loop util/annotate.c:1653~1676
have similar logic if compared with disasm__calc_percent

Anyway I'll keep trying to improve perf-annotate in terms of its 
features and its code..

Thanks,
Taeung