Re: [PATCH 2/3] perf diff: Add diff.order config option

[Namhyung Kim <namhyung@kernel.org>]

Hi Taeung,

On Mon, Feb 06, 2017 at 06:44:42PM +0900, Taeung Song wrote:
> Hi, Namhyung :)
> 
> On 02/06/2017 04:20 PM, Namhyung Kim wrote:
> > In many cases, I need to look at differences between two data so I often
> > used the -o option to sort the result base on the difference first.
> > It'd be nice to have a config option to set it by default.
> > 
> > The diff.order config option is to set the default value of -o/--order
> > option.
> > 
> > Cc: Taeung Song <treeze.taeung@gmail.com>
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/Documentation/perf-config.txt |  7 +++++++
> >  tools/perf/Documentation/perf-diff.txt   |  6 +++++-
> >  tools/perf/builtin-diff.c                | 14 ++++++++++++++
> >  3 files changed, 26 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/perf/Documentation/perf-config.txt b/tools/perf/Documentation/perf-config.txt
> > index 9365b75fd04f..5b54d47ef713 100644
> > --- a/tools/perf/Documentation/perf-config.txt
> > +++ b/tools/perf/Documentation/perf-config.txt
> > @@ -498,6 +498,13 @@ Variables
> >  		But if this option is 'no-cache', it will not update the build-id cache.
> >  		'skip' skips post-processing and does not update the cache.
> > 
> > +diff.*::
> > +	diff.order::
> > +		This option sets the number of column to sort the result.
> > +		Default is 0 which means sorting by baseline.
> > +		Setting it to 1 will sort the result by delta (or other
> > +		compute method selected).
> > +
> >  SEE ALSO
> >  --------
> >  linkperf:perf[1]
> > diff --git a/tools/perf/Documentation/perf-diff.txt b/tools/perf/Documentation/perf-diff.txt
> > index af80284cd2f6..6ba3bf582d79 100644
> > --- a/tools/perf/Documentation/perf-diff.txt
> > +++ b/tools/perf/Documentation/perf-diff.txt
> > @@ -99,7 +99,11 @@ OPTIONS
> > 
> >  -o::
> >  --order::
> > -       Specify compute sorting column number.
> > +       Specify compute sorting column number.  0 means sorting by baseline
> > +       overhead (default) and 1 means sorting by computed value of column 1
> > +       (data from the first file other base baseline).  Values more than 1
> > +       can be used only if enough data files are provided.
> > +       Default value can be set using diff.order config option.
> > 
> >  --percentage::
> >  	Determine how to display the overhead percentage of filtered entries.
> > diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
> > index 781c9e60bd21..181ff996e039 100644
> > --- a/tools/perf/builtin-diff.c
> > +++ b/tools/perf/builtin-diff.c
> > @@ -17,6 +17,7 @@
> >  #include "util/symbol.h"
> >  #include "util/util.h"
> >  #include "util/data.h"
> > +#include "util/config.h"
> > 
> >  #include <stdlib.h>
> >  #include <math.h>
> > @@ -1291,6 +1292,17 @@ static int data_init(int argc, const char **argv)
> >  	return 0;
> >  }
> > 
> > +static int diff__config(const char *var, const char *value,
> > +			void *cb __maybe_unused)
> > +{
> 
> What about adding if statement as below ?
> (such as builtin-ftrace.c:perf_ftrace_config)
> 
>          if (prefixcmp(var, "diff."))
>                  return 0;
> 
> > +	if (!strcmp(var, "diff.order")) {
> 
> And.. var + 5 instead of var as below ?
> (such as perf.c:pager_command_config)
> 
>          if (!strcmp(var + 5, "order"))
> 
> It is just my opinion. :)

Yep, looks good.  Will change if I send v2.

Thanks,
Namhyung


> 
> > +		sort_compute = perf_config_int(var, value);
> > +		return 0;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  int cmd_diff(int argc, const char **argv, const char *prefix __maybe_unused)
> >  {
> >  	int ret = hists__init();
> > @@ -1298,6 +1310,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix __maybe_unused)
> >  	if (ret < 0)
> >  		return ret;
> > 
> > +	perf_config(diff__config, NULL);
> > +
> >  	argc = parse_options(argc, argv, options, diff_usage, 0);
> > 
> >  	if (symbol__init(NULL) < 0)
> >