Re: [PATCH 1/8] perf, tools: Support handling complete branch stacks as histograms

[Andi Kleen <andi@firstfloor.org>]

Sorry for the long delay. Just revisiting that.

On Wed, Oct 22, 2014 at 10:03:51AM +0900, Namhyung Kim wrote:
> >             |          |          f2 tcall.c:5
> >             |          |          f1 tcall.c:12
> >             |          |          f1 tcall.c:12
> >             |          |          f2 tcall.c:7
> >             |          |          f2 tcall.c:5
> >             |          |          f1 tcall.c:11
> 
> I think it'd be better if it just prints function names as normal
> callchain does (and optionally srcline with a switch) and duplicates
> removed like below:
> 
>      54.91%  tcall.c:6  [.] f2                      tcall
>              |
>              |--65.53%-- f2 tcall.c:5
>              |          |
>              |          |--70.83%-- f1
>              |          |          main
>              |          |          f1
>              |          |          f2
>              |          |          f1
>              |          |          f2

I considered this. For this example it doesn't make much difference
because the functions are so small.

But for anything larger I really need the line numbers to make
sense of it. 

So I prefer to keep them. I'll look into some easy switch
to turn them off though.


> > +		if (sort__has_parent && !*parent &&
> > +		    symbol__match_regex(al.sym, &parent_regex))
> > +			*parent = al.sym;
> > +		else if (have_ignore_callees && root_al &&
> > +		  symbol__match_regex(al.sym, &ignore_callees_regex)) {
> > +			/* Treat this symbol as the root,
> > +			   forgetting its callees. */
> > +			*root_al = al;
> > +			callchain_cursor_reset(&callchain_cursor);
> > +		}
> > +		if (!symbol_conf.use_callchain)
> > +			return -EINVAL;
> 
> This check already went away.
> 
> And, to remove duplicates, I think we need to check last callchain
> cursor node wrt the callchain_param.key here.

I don't understand the comment. I'm not modifying anything
that has been already added to the callchain. Just things
to be added in the future. So why would I need to check
or change the cursor?

> 
> Also, by comparing 'from' address, I'd expect you add the from address
> alone but you add both of 'from' and 'to'.  Do we really need to do
> that?

Adding from and to makes it much clearer to the user what happens,
especially with conditional branches, so they can follow the 
control flow.


> And the first address saved in normal callchain is address of the
> function itself so it might be 'to' you need to check if sampled before
> any branch in a function.

I'm checking against the CALL, not the target.

> 
> > +			} else
> > +				be[i] = branch->entries[branch->nr - i - 1];
> > +		}
> > +
> > +		nr = remove_loops(be, nr);
> > +
> > +		for (i = 0; i < nr; i++) {
> > +			err = add_callchain_ip(machine, thread, parent,
> > +					       root_al,
> > +					       -1, be[i].to);
> > +			if (!err)
> > +				err = add_callchain_ip(machine, thread,
> > +						       parent, root_al,
> > +						       -1, be[i].from);
> > +			if (err == -EINVAL)
> > +				break;
> > +			if (err)
> > +				return err;
> > +		}
> > +		chain_nr -= nr;
> 
> I'm not sure this line is needed.

Without that i could exceed the limit.

-Andi