Messages in this thread Patch in this message | | | From | Namhyung Kim <> | Subject | Re: [PATCH 2/5] perf hists: Exchange order of comparing items when collapsing hists | Date | Fri, 07 Dec 2012 17:38:22 +0900 |
| |
Hi Arnaldo,
On Thu, 6 Dec 2012 16:09:20 -0300, Arnaldo Carvalho de Melo wrote: > Em Thu, Dec 06, 2012 at 05:53:25PM +0100, Jiri Olsa escreveu: >> On Fri, Dec 07, 2012 at 12:09:38AM +0900, Namhyung Kim wrote: >> > From: Namhyung Kim <namhyung.kim@lge.com> >> > >> > When comparing entries for collapsing put the given entry first, and >> > then the iterated entry. This is not the case of hist_entry__cmp() >> > when called if given sort keys don't require collapsing. So change >> > the order for the sake of consistency. It will be required for >> > matching and/or linking multiple hist entries. >> >> As discussed with Arnadlo, this change seems like changing the >> sort order... could you ellaborate how it is usefull in future? > > In several places the order is (he, iter) then it became (iter, he), > something like that, so he inverted it for consistency, but then he > needs to invert in the cmp function too, unsure if this is worth the > trouble now, perhaps some comment placed in the right spot clarifies > things,
The point is that it needs to have a same order when comparing two entries for both of inserting (add_hist_entry) and linking (hists__add_ dummy_entry and hists__find_entry). This was simple when we used output tree, because it's a single tree so that we can make sure that it use the same order as of insertion. But by using internal trees we should select one between inserting (entries_in) and collapsing (entries_collapsed) based on the sort keys given.
Unfortunately, inserting and collapsing used different order - (he, iter) vs. (iter, he) - so we need to use corresponding (different) order for match/link also. That means that without this patch, we have to call corresponding function with different order like following:
@@ -739,6 +739,10 @@ static struct hist_entry *hists__add_dummy_entry(struct hists *hists,
cmp = hist_entry__collapse(he, pair);
+ if (sort__need_collapse) + cmp = hist_entry__collapse(he, pair); + else + cmp = hist_entry__cmp(pair, he); if (!cmp) goto out; @@ -772,7 +776,12 @@ static struct hist_entry *hists__find_entry(struct hists *hists, while (n) { struct hist_entry *iter = rb_entry(n, struct hist_entry, rb_node_in); - int64_t cmp = hist_entry__collapse(iter, he); + int64_t cmp; + + if (sort__need_collapse) + cmp = hist_entry__collapse(iter, he); + else + cmp = hist_entry__cmp(he, iter); if (cmp < 0) n = n->rb_left; It doesn't look good, especially hist_entry__collapse will be same as hist_entry__cmp if 'sort__need_collapse' is false. If we can make the order consistent, it'd be converted to a sigle _collapse() call without the conditional.
Thanks, Namhyung
| |