On Fri, May 12, 2017 at 12:37:01PM +0200, Milian Wolff wrote:
> On Mittwoch, 10. Mai 2017 07:53:52 CEST Namhyung Kim wrote:
> > Hi,
> >
> > On Wed, May 03, 2017 at 11:35:36PM +0200, Milian Wolff wrote:
>
> <snip>
>
> > > +static enum match_result match_chain_srcline(struct callchain_cursor_node
> > > *node, + struct callchain_list
> > > *cnode)
> > > +{
> > > + char *left = get_srcline(cnode->ms.map->dso,
> > > + map__rip_2objdump(cnode->ms.map, cnode->ip),
> > > + cnode->ms.sym, true, false);
> > > + char *right = get_srcline(node->map->dso,
> > > + map__rip_2objdump(node->map, node->ip),
> > > + node->sym, true, false);
> > > + enum match_result ret = match_chain_strings(left, right);
> >
> > I think we need to check inlined srcline as well. There might be a
> > case that two samples have different addresses (and from different
> > callchains) but happens to be mapped to a same srcline IMHO.
>
> I think I'm missing something, but isn't this what this function provides?
> The
> function above is now being used by the match_chain_inliner function below.
>
> Ah, or do you mean for code such as this:
>
> ~~~~~
> inline_func_1(); inline_func_2();
> ~~~~~
>
> Here, both branches could be inlined into the same line and the same issue
> would occur, i.e. different branches get collapsed into the first match for
> the given srcline?
>
> Hm yes, this should be fixed too.
OK.
>
> But, quite frankly, I think it just shows more and more that the current
> inliner support is really fragile and leads to lots of issues throughout the
> code base as the inlined frames are different from non-inlined frames, but
> should most of the same be handled just like them.
>
> So, maybe it's time to once more think about going back to my initial
> approach: Make inlined frames code-wise equal to non-inlined frames, i.e.
> instead of requesting the inlined frames within match_chain, do it outside
> and
> create callchain_node/callchain_cursor instances (not sure which one right
> now) for the inlined frames too.
>
> This way, we should be able to centrally add support for inlined frames and
> all areas will benefit from it:
>
> - aggregation by srcline/function will magically work
> - all browsers will automatically display them, i.e. no longer need to
> duplicate the code for inliner support in perf script, perf report tui/
> stdio/...
> - we can easily support --inline in other tools, like `perf trace --call-
> graph`
>
> So before I invest more time trying to massage match_chain to behave well for
> inline nodes, can I get some feedback on the above?
Fair enough. I agree that it'd be better adding them as separate
callchain nodes when resolving callchains.
>
> Back then when Jin and me discussed this, noone from the core perf
> contributors ever bothered to give us any insight in what they think is the
> better approach.
That's unfortunate, sorry about that.
>
> > > +
> > >
> > > free_srcline(left);
> > > free_srcline(right);
> > > return ret;
> > >
> > > }
> > >
> > > +static enum match_result match_chain_inliner(struct callchain_cursor_node
> > > *node, + struct callchain_list
> > > *cnode)
> > > +{
> > > + u64 left_ip = map__rip_2objdump(cnode->ms.map, cnode->ip);
> > > + u64 right_ip = map__rip_2objdump(node->map, node->ip);
> > > + struct inline_node *left_node = NULL;
> > > + struct inline_node *right_node = NULL;
> > > + struct inline_list *left_entry = NULL;
> > > + struct inline_list *right_entry = NULL;
> > > + struct inline_list *left_last_entry = NULL;
> > > + struct inline_list *right_last_entry = NULL;
> > > + enum match_result ret = MATCH_EQ;
> > > +
> > > + left_node = dso__parse_addr_inlines(cnode->ms.map->dso, left_ip);
> > > + if (!left_node)
> > > + return MATCH_ERROR;
> > > +
> > > + right_node = dso__parse_addr_inlines(node->map->dso, right_ip);
> > > + if (!right_node) {
> > > + inline_node__delete(left_node);
> > > + return MATCH_ERROR;
> > > + }
> > > +
> > > + left_entry = list_first_entry(&left_node->val,
> > > + struct inline_list, list);
> > > + left_last_entry = list_last_entry(&left_node->val,
> > > + struct inline_list, list);
> > > + right_entry = list_first_entry(&right_node->val,
> > > + struct inline_list, list);
> > > + right_last_entry = list_last_entry(&right_node->val,
> > > + struct inline_list, list);
> >
> > What about keeping number of entries in a inline_node so that we can
> > check the numbers for faster comparison?
>
> What benefit would that have? The performance cost is dominated by finding
> the
> inlined nodes, not by doing the comparison on the callstack.
Well, I didn't measure the performance cost but your example contains
long symbols and they share some parts. So I guess it would hurt
performance as they'll be checked frequently.
>
> > > + while (ret == MATCH_EQ && (left_entry || right_entry)) {
> > > + ret = match_chain_strings(left_entry ? left_entry->funcname :
> NULL,
> > > + right_entry ? right_entry->funcname :
> > > NULL);
> > > +
> > > + if (left_entry && left_entry != left_last_entry)
> > > + left_entry = list_next_entry(left_entry, list);
> > > + else
> > > + left_entry = NULL;
> > > +
> > > + if (right_entry && right_entry != right_last_entry)
> > > + right_entry = list_next_entry(right_entry, list);
> > > + else
> > > + right_entry = NULL;
> > > + }
> > > +
> > > + inline_node__delete(left_node);
> > > + inline_node__delete(right_node);
> > > + return ret;
> > > +}
> > > +
> > >
> > > static enum match_result match_chain(struct callchain_cursor_node *node,
> > >
> > > struct callchain_list *cnode)
> > >
> > > {
> > >
> > > @@ -671,7 +728,13 @@ static enum match_result match_chain(struct
> > > callchain_cursor_node *node,>
> > > }
> > >
> > > if (left == right) {
> > >
> > > - if (node->branch) {
> > > + if (symbol_conf.inline_name && cnode->ip != node->ip) {
> > > + enum match_result match = match_chain_inliner(node,
> > > + cnode);
> > > +
> > > + if (match != MATCH_ERROR)
> > > + return match;
> >
> > I guess it'd be better just returning the match result. Otherwise
> > MATCH_ERROR will be converted to MATCH_EQ..
>
> This is done on purpose to fall-back to the IP-based comparison. That way,
> entries without inlined nodes will be sorted the same way as before this
> patch.
Hmm.. OK, but as I said in another thread, if one node has inlines and
the other don't, they should be separated.
Thanks,
Namhyung
