{"thread":{"id":"36239","subject":"[PATCH] diff: optimise parse_dirstat_params() to only compare strings when necessary","startedAt":"2014-03-20T00:07:56Z","lastAt":"2014-03-20T17:18:54Z","messageCount":3,"participants":["Dragos Foianu","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"237132","messageId":"1395274076-6720-1-git-send-email-dragos.foianu@gmail.com","threadId":"36239","inReplyTo":null,"subject":"[PATCH] diff: optimise parse_dirstat_params() to only compare strings when necessary","fromName":"Dragos Foianu","fromEmail":"dragos.foianu@gmail.com","sentAt":"2014-03-20T00:07:56Z","receivedAt":"2014-03-20T00:07:56Z","isPatch":true,"sender":{"key":"dragos.foianu@gmail.com","avatar":null},"body":"parse_dirstat_params() goes through a chain of if statements using\nstrcmp to parse parameters. When the parameter is a digit, the\nvalue must go through all comparisons before the function realises\nit is a digit. Optimise this logic by only going through the chain\nof string compares when the parameter is not a digit.\n\nSigned-off-by: Dragos Foianu <dragos.foianu@gmail.com>\n---\n diff.c | 37 +++++++++++++++++++------------------\n 1 file changed, 19 insertions(+), 18 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex e343191..733764e 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -84,20 +84,25 @@ static int parse_dirstat_params(struct diff_options *options, const char *params\n \t\tstring_list_split_in_place(&params, params_copy, ',', -1);\n \tfor (i = 0; i < params.nr; i++) {\n \t\tconst char *p = params.items[i].string;\n-\t\tif (!strcmp(p, \"changes\")) {\n-\t\t\tDIFF_OPT_CLR(options, DIRSTAT_BY_LINE);\n-\t\t\tDIFF_OPT_CLR(options, DIRSTAT_BY_FILE);\n-\t\t} else if (!strcmp(p, \"lines\")) {\n-\t\t\tDIFF_OPT_SET(options, DIRSTAT_BY_LINE);\n-\t\t\tDIFF_OPT_CLR(options, DIRSTAT_BY_FILE);\n-\t\t} else if (!strcmp(p, \"files\")) {\n-\t\t\tDIFF_OPT_CLR(options, DIRSTAT_BY_LINE);\n-\t\t\tDIFF_OPT_SET(options, DIRSTAT_BY_FILE);\n-\t\t} else if (!strcmp(p, \"noncumulative\")) {\n-\t\t\tDIFF_OPT_CLR(options, DIRSTAT_CUMULATIVE);\n-\t\t} else if (!strcmp(p, \"cumulative\")) {\n-\t\t\tDIFF_OPT_SET(options, DIRSTAT_CUMULATIVE);\n-\t\t} else if (isdigit(*p)) {\n+\t\tif (!isdigit(*p)) {\n+\t\t\tif (!strcmp(p, \"changes\")) {\n+\t\t\t\tDIFF_OPT_CLR(options, DIRSTAT_BY_LINE);\n+\t\t\t\tDIFF_OPT_CLR(options, DIRSTAT_BY_FILE);\n+\t\t\t} else if (!strcmp(p, \"lines\")) {\n+\t\t\t\tDIFF_OPT_SET(options, DIRSTAT_BY_LINE);\n+\t\t\t\tDIFF_OPT_CLR(options, DIRSTAT_BY_FILE);\n+\t\t\t} else if (!strcmp(p, \"files\")) {\n+\t\t\t\tDIFF_OPT_CLR(options, DIRSTAT_BY_LINE);\n+\t\t\t\tDIFF_OPT_SET(options, DIRSTAT_BY_FILE);\n+\t\t\t} else if (!strcmp(p, \"noncumulative\")) {\n+\t\t\t\tDIFF_OPT_CLR(options, DIRSTAT_CUMULATIVE);\n+\t\t\t} else if (!strcmp(p, \"cumulative\")) {\n+\t\t\t\tDIFF_OPT_SET(options, DIRSTAT_CUMULATIVE);\n+\t\t\t} else {\n+\t\t\t\tstrbuf_addf(errmsg, _(\"  Unknown dirstat parameter '%s'\\n\"), p);\n+\t\t\t\tret++;\n+\t\t\t}\n+\t\t} else  {\n \t\t\tchar *end;\n \t\t\tint permille = strtoul(p, &end, 10) * 10;\n \t\t\tif (*end == '.' && isdigit(*++end)) {\n@@ -114,11 +119,7 @@ static int parse_dirstat_params(struct diff_options *options, const char *params\n \t\t\t\t\t    p);\n \t\t\t\tret++;\n \t\t\t}\n-\t\t} else {\n-\t\t\tstrbuf_addf(errmsg, _(\"  Unknown dirstat parameter '%s'\\n\"), p);\n-\t\t\tret++;\n \t\t}\n-\n \t}\n \tstring_list_clear(&params, 0);\n \tfree(params_copy);\n-- \n1.8.3.2\n"},{"id":"237134","messageId":"loom.20140320T011443-681@post.gmane.org","threadId":"36239","inReplyTo":"1395274076-6720-1-git-send-email-dragos.foianu@gmail.com","subject":"Re: [PATCH] diff: optimise parse_dirstat_params() to only compare strings when necessary","fromName":"Dragos Foianu","fromEmail":"dragos.foianu@gmail.com","sentAt":"2014-03-20T00:15:26Z","receivedAt":"2014-03-20T00:15:26Z","isPatch":true,"sender":{"key":"dragos.foianu@gmail.com","avatar":null},"body":"I will send another version of this patch after review because there is an\nextra whitespace following the else statement.\n"},{"id":"237184","messageId":"xmqqmwgkzt35.fsf@gitster.dls.corp.google.com","threadId":"36239","inReplyTo":"1395274076-6720-1-git-send-email-dragos.foianu@gmail.com","subject":"Re: [PATCH] diff: optimise parse_dirstat_params() to only compare strings when necessary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-20T17:18:54Z","receivedAt":"2014-03-20T17:18:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dragos Foianu <dragos.foianu@gmail.com> writes:\n\n> parse_dirstat_params() goes through a chain of if statements using\n> strcmp to parse parameters. When the parameter is a digit, the\n> value must go through all comparisons before the function realises\n> it is a digit. Optimise this logic by only going through the chain\n> of string compares when the parameter is not a digit.\n\nThis change could be an optimization only if parse_dirstat_params()\nis called with a param that begins with a digit a lot more often\nthan with other forms of params, but that is a mere assumption.\nUnless that assumption is substantiated, this change can be a\npessimization.\n\nEven if the assumption were true (which I doubt), a simpler solution\nto optimize such a call pattern would be to simply tweak of the\norder if/else cascade to check if the param begins with a digit\nfirst before checking other keywords, wouldn't it?  I am not sure\nwhy you even need to change the structure into a nested if\nstatement.\n\n> Signed-off-by: Dragos Foianu <dragos.foianu@gmail.com>\n> ---\n>  diff.c | 37 +++++++++++++++++++------------------\n>  1 file changed, 19 insertions(+), 18 deletions(-)\n>\n> diff --git a/diff.c b/diff.c\n> index e343191..733764e 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -84,20 +84,25 @@ static int parse_dirstat_params(struct diff_options *options, const char *params\n>  \t\tstring_list_split_in_place(&params, params_copy, ',', -1);\n>  \tfor (i = 0; i < params.nr; i++) {\n>  \t\tconst char *p = params.items[i].string;\n> -\t\tif (!strcmp(p, \"changes\")) {\n> -\t\t\tDIFF_OPT_CLR(options, DIRSTAT_BY_LINE);\n> -\t\t\tDIFF_OPT_CLR(options, DIRSTAT_BY_FILE);\n> -\t\t} else if (!strcmp(p, \"lines\")) {\n> -\t\t\tDIFF_OPT_SET(options, DIRSTAT_BY_LINE);\n> -\t\t\tDIFF_OPT_CLR(options, DIRSTAT_BY_FILE);\n> -\t\t} else if (!strcmp(p, \"files\")) {\n> -\t\t\tDIFF_OPT_CLR(options, DIRSTAT_BY_LINE);\n> -\t\t\tDIFF_OPT_SET(options, DIRSTAT_BY_FILE);\n> -\t\t} else if (!strcmp(p, \"noncumulative\")) {\n> -\t\t\tDIFF_OPT_CLR(options, DIRSTAT_CUMULATIVE);\n> -\t\t} else if (!strcmp(p, \"cumulative\")) {\n> -\t\t\tDIFF_OPT_SET(options, DIRSTAT_CUMULATIVE);\n> -\t\t} else if (isdigit(*p)) {\n> +\t\tif (!isdigit(*p)) {\n> +\t\t\tif (!strcmp(p, \"changes\")) {\n> +\t\t\t\tDIFF_OPT_CLR(options, DIRSTAT_BY_LINE);\n> +\t\t\t\tDIFF_OPT_CLR(options, DIRSTAT_BY_FILE);\n> +\t\t\t} else if (!strcmp(p, \"lines\")) {\n> +\t\t\t\tDIFF_OPT_SET(options, DIRSTAT_BY_LINE);\n> +\t\t\t\tDIFF_OPT_CLR(options, DIRSTAT_BY_FILE);\n> +\t\t\t} else if (!strcmp(p, \"files\")) {\n> +\t\t\t\tDIFF_OPT_CLR(options, DIRSTAT_BY_LINE);\n> +\t\t\t\tDIFF_OPT_SET(options, DIRSTAT_BY_FILE);\n> +\t\t\t} else if (!strcmp(p, \"noncumulative\")) {\n> +\t\t\t\tDIFF_OPT_CLR(options, DIRSTAT_CUMULATIVE);\n> +\t\t\t} else if (!strcmp(p, \"cumulative\")) {\n> +\t\t\t\tDIFF_OPT_SET(options, DIRSTAT_CUMULATIVE);\n> +\t\t\t} else {\n> +\t\t\t\tstrbuf_addf(errmsg, _(\"  Unknown dirstat parameter '%s'\\n\"), p);\n> +\t\t\t\tret++;\n> +\t\t\t}\n> +\t\t} else  {\n>  \t\t\tchar *end;\n>  \t\t\tint permille = strtoul(p, &end, 10) * 10;\n>  \t\t\tif (*end == '.' && isdigit(*++end)) {\n> @@ -114,11 +119,7 @@ static int parse_dirstat_params(struct diff_options *options, const char *params\n>  \t\t\t\t\t    p);\n>  \t\t\t\tret++;\n>  \t\t\t}\n> -\t\t} else {\n> -\t\t\tstrbuf_addf(errmsg, _(\"  Unknown dirstat parameter '%s'\\n\"), p);\n> -\t\t\tret++;\n>  \t\t}\n> -\n>  \t}\n>  \tstring_list_clear(&params, 0);\n>  \tfree(params_copy);\n"}]}