Re: [PATCH v1 1/4] perf annotate: create a new hists to manage multiple events samples

From: Jin, Yao
Date: Wed Oct 11 2017 - 21:39:49 EST




On 10/9/2017 9:40 AM, Jin, Yao wrote:


On 10/7/2017 2:54 AM, Arnaldo Carvalho de Melo wrote:
Em Sat, Oct 07, 2017 at 12:31:37AM +0800, Jin, Yao escreveu:
On 10/5/2017 9:21 PM, Arnaldo Carvalho de Melo wrote:
Em Wed, Aug 16, 2017 at 06:18:33PM +0800, Jin Yao escreveu:
An issue is found during using perf annotate.

perf record -e cycles,branches ...
perf annotate main --stdio

The result only shows cycles. It should show both cycles and
branches on the left side. It works with "--group", but need
this to work even without groups.

Right, for --group we know that we'll be reading all the counters at
each sample, so it all works and we can use the current design.

When we're not using groups tho, each sample has just one of the events
and we end up with separate views.

Note tho that since the annotation buckets are kept per 'struct symbol'
instance, this problem should be present only in the hist_entry based
views, i.e. 'perf report' and 'perf top', right?
Yes, it seems to be in hist_entry based view.

Ok.

But note that your initial statement of the problem:

<quote Ji, Yao>
An issue is found during using perf annotate.

perf record -e cycles,branches ...
perf annotate main --stdio

The result only shows cycles. It should show both cycles and
branches on the left side. It works with "--group", but need
this to work even without groups.
</>

Can be solved right now, its just a matter of accessing the other
buckets in a given symbol, just like we have for --group.

The only problem is in presenting a list of symbols which can be
annotated, we have them for each evsel, and you, rightly, want to show
the list of all symbols in all evsels.

Ok?


Hi Arnaldo, I just feel there might be another problem when displaying the result.

I will talk about this in below.

I.e. all struct hist_entry->ms.sym instances point to the same stuct
symbol and thus will use the same annotation histogram buckets, i.e.
symbol__annotation(hist_entry->ms.sym) point to the same 'struct
annotation' instance, and then its a matter of passing this pointer to
annotation__histogram(notes, IDX) where this IDX is perf_evsel->idx.

I wonder if we can't just add a new rb_node in struct hist_entry and
have it in two rb_trees, i.e. in two 'struct hists' instances, one per
evsel and one per evlist.

Currently we just have per-evsel hists. This idea will create a new per-evlist hists.
struct perf_evlist {
......
struct hists hists;
};
And let the hist_entry be linked in both per-evsel hists and per-evlist hists.
Please correct me if my understanding is wrong for this idea.

Yes, do you see problems with trying to do it this way? At a first sight
seems like it will reuse more code, no?

I.e. in hists__findnew_entry(), when not finding an existing hist_entry
in the per-evsel hists you end up calling hist_entry__new(), right here
you'll add it to the evsel->evlist->hists, and when we want to go from
a hist_entry to the evlist it is in we'll use:

hists_to_evsel(he->hists)->evlist

Right?

- Arnaldo


With this method, we can get the events from per-evlist hists.

For example, when printing the symbol annotate of 'cycles', we can also get the 'branches' from per-evlist hists. So we can print the values of both 'cycles' and 'branches' in annotate view of 'cycles'.

The problem might be, once the annotate view of 'cycles' being printed, the view of 'branches' will be printed in next. Maybe they are the same symbol (e.g. function A), so duplicated.

The problem is that we only sort the per-evsel hists, but we don't sort the per-evlist hists.

From my personal opinion, we may need a new sorted hists for multiple events. That will avoid the symbol duplication.

Maybe I misunderstand something, please correct me if I'm wrong.

Thanks
Jin Yao



Hi Arnaldo,

Is my above explanation OK or anything I should improve?

Thanks
Jin Yao