Re: [PATCH] Put common histogram functions for perf in their ownfile

[John Kacur]

On Mon, 28 Sep 2009, Frederic Weisbecker wrote:

> On Mon, Sep 28, 2009 at 03:05:29PM +0200, John Kacur wrote:
> > From d187ab3a62526ffbdff91911f4b2da813ccf037f Mon Sep 17 00:00:00 2001
> > From: John Kacur <jkacur@xxxxxxxxxx>
> > Date: Mon, 28 Sep 2009 14:53:08 +0200
> > Subject: [PATCH] Move histogram related functions into their own files (hist.c and hist.h)
> >  and make use of them in builtin-annotate.c and builtin-report.c
> 
> 
> 
> I guess a part of the title should be in the changelog, and the title
> should be a compressed version :)

Okay - I added the title of the mail to the changelog

> 
> 
>  
> > Signed-off-by: John Kacur <jkacur@xxxxxxxxxx>
> >  static int
> >  process_sample_event(event_t *event, unsigned long offset, unsigned long head)
> >  {
> > @@ -861,7 +713,7 @@ more:
> >  		dsos__fprintf(stdout);
> >  
> >  	collapse__resort();
> > -	output__resort();
> > +	output__resort(total);
> 
> 
> 
> Now I wonder if we actually ever need to sort the entries
> in perf annotate. The only thing we need is to count the
> hits per ip.

perf annotate still will not sort the entries with these changes.
because callchain will be equal to zero.

Calling output__resort(total) will be harmless because the calculation
will be thrown away in output__insert_entry() here:
if (callchain)
		callchain_param.sort(&he->sorted_chain, &he->callchain,
				      min_callchain_hits, &callchain_param);

> 
> 
> >  
> >  	find_annotations();
> >  
> > diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> > index 7b43504..ff6de9e 100644
> > --- a/tools/perf/builtin-report.c
> > +++ b/tools/perf/builtin-report.c
> > @@ -28,6 +28,7 @@
> >  
> >  #include "util/thread.h"
> >  #include "util/sort.h"
> > +#include "util/hist.h"
> >  
> >  static char		const *input_name = "perf.data";
> >  
> > @@ -55,8 +56,6 @@ static int		exclude_other = 1;
> >  
> >  static char		callchain_default_opt[] = "fractal,0.5";
> >  
> > -static int		callchain;
> > -
> >  static char		__cwd[PATH_MAX];
> >  static char		*cwd = __cwd;
> >  static int		cwdlen;
> > @@ -66,50 +65,8 @@ static struct thread	*last_match;
> >  
> >  static struct perf_header *header;
> >  
> > -static
> > -struct callchain_param	callchain_param = {
> > -	.mode	= CHAIN_GRAPH_REL,
> > -	.min_percent = 0.5
> > -};
> 
> 
> 
> At least for now, this part is only used by report. May be better
> keep it in report?
> 
> Concerning the whole, that looks nice. Especially annotate doesn't
> even seem to need any sort.
> 
> So we could eventually keep the hist sorting only in report.
> But that said, it's nice to make all these sorting and histogram
> features in a separate file for general purpose. We can bet that will
> be reused soon or later. If not this is still good to split it, to clear
> a bit builtin-report.c
> 
> But the callchain bits should perhaps stay in report. If that needs
> to be used elsewhere, it would need a kind of cleanup before.
> 

Okay, I agree with you, so I am respinning the patch with callchain and
callchain_param put back in builtin-report.c.