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

[Arnaldo Carvalho de Melo <acme@kernel.org>]

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?

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