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

[Frederic Weisbecker <fweisbec@gmail.com>]

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@redhat.com>
> 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 :)


 
> Signed-off-by: John Kacur <jkacur@redhat.com>
>  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.


>  
>  	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.