Re: [PATCH] diff: optimise parse_dirstat_params() to only compare strings when necessary
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 20, 2014, 17:18 UTC
- Message-ID
- <xmqqmwgkzt35.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <1395274076-6720-1-git-send-email-dragos.foianu@gmail.com>
Dragos Foianu <dragos.foianu@gmail.com> writes:
Show 5 quoted lines
> parse_dirstat_params() goes through a chain of if statements using > strcmp to parse parameters. When the parameter is a digit, the > value must go through all comparisons before the function realises > it is a digit. Optimise this logic by only going through the chain > of string compares when the parameter is not a digit.
This change could be an optimization only if parse_dirstat_params() is called with a param that begins with a digit a lot more often than with other forms of params, but that is a mere assumption. Unless that assumption is substantiated, this change can be a pessimization.
Even if the assumption were true (which I doubt), a simpler solution to optimize such a call pattern would be to simply tweak of the order if/else cascade to check if the param begins with a digit first before checking other keywords, wouldn't it? I am not sure why you even need to change the structure into a nested if statement.
Show 61 quoted lines
> Signed-off-by: Dragos Foianu <dragos.foianu@gmail.com>
> ---
> diff.c | 37 +++++++++++++++++++------------------
> 1 file changed, 19 insertions(+), 18 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index e343191..733764e 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -84,20 +84,25 @@ static int parse_dirstat_params(struct diff_options *options, const char *params
> string_list_split_in_place(¶ms, params_copy, ',', -1);
> for (i = 0; i < params.nr; i++) {
> const char *p = params.items[i].string;
> - if (!strcmp(p, "changes")) {
> - DIFF_OPT_CLR(options, DIRSTAT_BY_LINE);
> - DIFF_OPT_CLR(options, DIRSTAT_BY_FILE);
> - } else if (!strcmp(p, "lines")) {
> - DIFF_OPT_SET(options, DIRSTAT_BY_LINE);
> - DIFF_OPT_CLR(options, DIRSTAT_BY_FILE);
> - } else if (!strcmp(p, "files")) {
> - DIFF_OPT_CLR(options, DIRSTAT_BY_LINE);
> - DIFF_OPT_SET(options, DIRSTAT_BY_FILE);
> - } else if (!strcmp(p, "noncumulative")) {
> - DIFF_OPT_CLR(options, DIRSTAT_CUMULATIVE);
> - } else if (!strcmp(p, "cumulative")) {
> - DIFF_OPT_SET(options, DIRSTAT_CUMULATIVE);
> - } else if (isdigit(*p)) {
> + if (!isdigit(*p)) {
> + if (!strcmp(p, "changes")) {
> + DIFF_OPT_CLR(options, DIRSTAT_BY_LINE);
> + DIFF_OPT_CLR(options, DIRSTAT_BY_FILE);
> + } else if (!strcmp(p, "lines")) {
> + DIFF_OPT_SET(options, DIRSTAT_BY_LINE);
> + DIFF_OPT_CLR(options, DIRSTAT_BY_FILE);
> + } else if (!strcmp(p, "files")) {
> + DIFF_OPT_CLR(options, DIRSTAT_BY_LINE);
> + DIFF_OPT_SET(options, DIRSTAT_BY_FILE);
> + } else if (!strcmp(p, "noncumulative")) {
> + DIFF_OPT_CLR(options, DIRSTAT_CUMULATIVE);
> + } else if (!strcmp(p, "cumulative")) {
> + DIFF_OPT_SET(options, DIRSTAT_CUMULATIVE);
> + } else {
> + strbuf_addf(errmsg, _(" Unknown dirstat parameter '%s'\n"), p);
> + ret++;
> + }
> + } else {
> char *end;
> int permille = strtoul(p, &end, 10) * 10;
> if (*end == '.' && isdigit(*++end)) {
> @@ -114,11 +119,7 @@ static int parse_dirstat_params(struct diff_options *options, const char *params
> p);
> ret++;
> }
> - } else {
> - strbuf_addf(errmsg, _(" Unknown dirstat parameter '%s'\n"), p);
> - ret++;
> }
> -
> }
> string_list_clear(¶ms, 0);
> free(params_copy);