Re: [PATCH] diff: optimise parse_dirstat_params() to only compare strings when necessary
From: Junio C Hamano <hidden>
Date: 2016-06-15 23:00:23
Dragos Foianu [off-list ref] 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.
quoted hunk
Signed-off-by: Dragos Foianu <redacted> --- 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);