Re: [PATCH] perf report: Add option to collapse undesired parts of call graph

[Greg Price <price@MIT.EDU>]

On Thu, Jun 27, 2013 at 01:58:16PM +0900, Namhyung Kim wrote:
> On Wed, 26 Jun 2013 18:25:01 -0400, Greg Price wrote:
> > On Wed, Jun 26, 2013 at 10:28:56AM +0900, Namhyung Kim wrote:
> >> On Sat, 22 Jun 2013 23:17:20 -0400, Greg Price wrote:
> >> > @@ -1211,8 +1214,13 @@ static int machine__resolve_callchain_sample(struct machine *machine,
> >> >  					   MAP__FUNCTION, ip, &al, NULL);
> >> >  		if (al.sym != NULL) {
> >> >  			if (sort__has_parent && !*parent &&
> >> > -			    symbol__match_parent_regex(al.sym))
> >> > +			    symbol__match_regex(al.sym, &parent_regex))
> >> >  				*parent = al.sym;
> >> > +			else if (have_blackbox && root_al &&
> >> > +			         symbol__match_regex(al.sym, &blackbox_regex)) {
> >> > +				*root_al = al;
> >> > +				callchain_cursor_reset(&callchain_cursor);
> >> 
> >> Okay, this is where the magic happens. :)
> >
> > Indeed! :)
> >
> >> So it overwrites the original 'al' in process_sample_event() to
> >> blackboxed symbol and drop the callchain.  Wouldn't it deserve a
> >> comment? :)
> >
> > I could do that.  Perhaps something like
> >   /* ignore the callchain we had so far, i.e. this symbol's callees */
> > Sound like what you had in mind?
> 
> More important thing is, I think, it updates the original al (root_al)
> so that the perf will see the new symbol as if it was sampled in the
> first place and it will collect all of its callers in one place.

OK, I'll try to capture that in a comment in v2.



> >> > +			}
> >> >  			if (!symbol_conf.use_callchain)
> >> >  				break;
> >> pp
> >> This is unrelated to this patch, but why is this line needed?  I guess
> >> this check should be done before calling this function.
> >
> > Hmm.  We actually can get into this function when
> > !symbol_conf.use_callchain, if we're using say --sort=parent.  But I'm
> > still somewhat puzzled, because in that case it looks like we'll break
> > the loop after the first address with a symbol, even if we didn't find
> > the 'parent' there, which seems like it wouldn't serve the purpose.
> 
> Right, that's what I want to say.
> 
> We already have the check before calling this function so breaking the
> loop after checking only first callchain node looks strange.  If we
> don't want to see callchains but only parents, it should either check
> every callchain nodes or fail out as an unsupported operation IMHO.

I agree.  Will reply momentarily with a patch.

I actually wrote a version that tries to keep the optimization, but
decided it was too complicated for what it gains us.

Cheers,
Greg