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

[Taeung Song <treeze.taeung@gmail.com>]

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

Thanks,
Taeung

> +		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)
>