threads / patch / 36239

patchdiff: optimise parse_dirstat_params() to only compare strings when necessary

Subject: [PATCH] diff: optimise parse_dirstat_params() to only compare strings when necessary

## tl;dr

3 messages between Mar 20, 2014 and Mar 20, 2014. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Dragos Foianu· Mar 20, 2014, 00:07 UTC · lore

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.

Signed-off-by: Dragos Foianu <dragos.foianu@gmail.com>
---
 diff.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)
Show changes to diff.c +19 −18
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(&params, 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(&params, 0);
 	free(params_copy);

-- 
1.8.3.2
Dragos Foianu· Mar 20, 2014, 00:15 UTC · re: Dragos Foianu · lore

Re: [PATCH] diff: optimise parse_dirstat_params() to only compare strings when necessary

I will send another version of this patch after review because there is an extra whitespace following the else statement.

Junio C Hamano· Mar 20, 2014, 17:18 UTC · re: Dragos Foianu · lore

Re: [PATCH] diff: optimise parse_dirstat_params() to only compare strings when necessary

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(&params, 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(&params, 0);
>  	free(params_copy);

← back to recent threads