# [PATCH] diff: optimise parse_dirstat_params() to only compare strings when necessary 3 messages from 2014-03-20 to 2014-03-20. Participants: Dragos Foianu, Junio C Hamano. Thread: https://gitlist.dev/t/36239 ## Dragos Foianu, 2014-03-20 00:07 Subject: [PATCH] diff: optimise parse_dirstat_params() to only compare strings when necessary Message-ID: <1395274076-6720-1-git-send-email-dragos.foianu@gmail.com> URL: https://gitlist.dev/e/1395274076-6720-1-git-send-email-dragos.foianu%40gmail.com ``` 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 --- 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); -- 1.8.3.2 ``` ## Dragos Foianu, 2014-03-20 00:15 Subject: Re: [PATCH] diff: optimise parse_dirstat_params() to only compare strings when necessary Message-ID: URL: https://gitlist.dev/e/loom.20140320T011443-681%40post.gmane.org In-Reply-To: <1395274076-6720-1-git-send-email-dragos.foianu@gmail.com> ``` I will send another version of this patch after review because there is an extra whitespace following the else statement. ``` ## Junio C Hamano, 2014-03-20 17:18 Subject: Re: [PATCH] diff: optimise parse_dirstat_params() to only compare strings when necessary Message-ID: URL: https://gitlist.dev/e/xmqqmwgkzt35.fsf%40gitster.dls.corp.google.com In-Reply-To: <1395274076-6720-1-git-send-email-dragos.foianu@gmail.com> ``` Dragos Foianu writes: > 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. > Signed-off-by: Dragos Foianu > --- > 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); ```