Re: [PATCH 2/7] perf hist: Introduce hists__link_hierarchy()

From: Namhyung Kim <namhyung@kernel.org>
To: Jiri Olsa <jolsa@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>,
	Ingo Molnar <mingo@kernel.org>,
	Peter Zijlstra <a.p.zijlstra@chello.nl>,
	Jiri Olsa <jolsa@kernel.org>, LKML <linux-kernel@vger.kernel.org>,
	Andi Kleen <andi@firstfloor.org>
Subject: Re: [PATCH 2/7] perf hist: Introduce hists__link_hierarchy()
Date: Tue, 20 Sep 2016 10:17:19 +0900	[thread overview]
Message-ID: <CAM9d7ciNoDyrqubUQRe_0zSxEEBOS83HOx_QSVg=WZg0w9PW3Q@mail.gmail.com> (raw)
In-Reply-To: <20160919075954.GB26906@krava>

On Mon, Sep 19, 2016 at 4:59 PM, Jiri Olsa <jolsa@redhat.com> wrote:
> On Tue, Sep 13, 2016 at 04:45:47PM +0900, Namhyung Kim wrote:
>> The hists__link_hierarchy() is to support hierarchy report with event
>> group.  When it matches leader event and other members (with the
>> hists__match_hierarchy), it also needs to link unmatched member entries
>> with a dummy leader event so that it can show up in the output.
>
> SNIP
>
>>
>> +static int hists__link_hierarchy(struct hists *leader_hists,
>> +                              struct hist_entry *parent,
>> +                              struct rb_root *leader_root,
>> +                              struct rb_root *other_root)
>> +{
>> +     struct rb_node *nd;
>> +     struct hist_entry *pos, *leader;
>> +
>> +     for (nd = rb_first(other_root); nd; nd = rb_next(nd)) {
>> +             pos = rb_entry(nd, struct hist_entry, rb_node_in);
>> +
>> +             if (hist_entry__has_pairs(pos)) {
>> +                     bool found = false;
>> +
>> +                     list_for_each_entry(leader, &pos->pairs.head, pairs.node) {
>> +                             if (leader->hists == leader_hists) {
>> +                                     found = true;
>> +                                     break;
>> +                             }
>> +                     }
>> +                     if (!found)
>> +                             return -1;
>> +             } else {
>> +                     leader = add_dummy_hierarchy_entry(leader_hists,
>> +                                                        leader_root, pos);
>
> could we call hists__add_dummy_entry in here? seems like there's only
> difference in function arguments.. we could safe one function ;-)

Maybe, but I don't see much value doing it.  IMHO this hists
match/link thing is already complex.  Combining normal and hierarchy
case in a single function will increase the complexity so I'd like to
keep it separate for hierarchy case.

Thanks,
Namhyung