{"thread":{"id":"38150","subject":"[PATCH/RFC 0/4] some attr optimizations","startedAt":"2014-12-09T13:53:21Z","lastAt":"2014-12-27T23:59:30Z","messageCount":16,"participants":["Nguyễn Thái Ngọc Duy","Eric Sunshine","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"253411","messageId":"1418133205-18213-1-git-send-email-pclouds@gmail.com","threadId":"38150","inReplyTo":null,"subject":"[PATCH/RFC 0/4] some attr optimizations","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-12-09T13:53:21Z","receivedAt":"2014-12-09T13:53:21Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"I wondered if I could mark some untracked files but 'precious' using\ngit attributes. Then I worried that the majority of people who don't\ncare about this precious thing will have to pay for git_check_attr()\njust because some people want it.\n\nWhich led me to try to optimize the attr machinery so that, if there's\nno sign of \"precious\" (or any attribute of interest) being defined, we\ncan keep the overhead down to minimum. The test suite passes, but\nthese optimizations could break down in subtle ways...\n\nNguyễn Thái Ngọc Duy (4):\n attr.c: rename global var attr_nr to git_attr_nr\n attr.c: split path processing code out of collect_all_attrs()\n attr: do not attempt to expand when we know it's not a macro\n attr: avoid heavy work when we know the specified attr is not defined\n\n attr.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++++-------------\n 1 file changed, 74 insertions(+), 18 deletions(-)\n\n-- \n2.2.0.84.ge9c7a8a\n"},{"id":"253412","messageId":"1418133205-18213-2-git-send-email-pclouds@gmail.com","threadId":"38150","inReplyTo":"1418133205-18213-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 1/4] attr.c: rename global var attr_nr to git_attr_nr","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-12-09T13:53:22Z","receivedAt":"2014-12-09T13:53:22Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This name \"attr_nr\" is used elsewhere as local variable, shadowing the\nglobal one. Let's rename the global one into something else to avoid\naccidents due to shadow in future.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n attr.c | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex cd54697..583d36a 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -34,7 +34,7 @@ struct git_attr {\n \tint attr_nr;\n \tchar name[FLEX_ARRAY];\n };\n-static int attr_nr;\n+static int git_attr_nr;\n \n static struct git_attr_check *check_all_attr;\n static struct git_attr *(git_attr_hash[HASHSIZE]);\n@@ -94,10 +94,10 @@ static struct git_attr *git_attr_internal(const char *name, int len)\n \ta->name[len] = 0;\n \ta->h = hval;\n \ta->next = git_attr_hash[pos];\n-\ta->attr_nr = attr_nr++;\n+\ta->attr_nr = git_attr_nr++;\n \tgit_attr_hash[pos] = a;\n \n-\tREALLOC_ARRAY(check_all_attr, attr_nr);\n+\tREALLOC_ARRAY(check_all_attr, git_attr_nr);\n \tcheck_all_attr[a->attr_nr].attr = a;\n \tcheck_all_attr[a->attr_nr].value = ATTR__UNKNOWN;\n \treturn a;\n@@ -730,10 +730,10 @@ static void collect_all_attrs(const char *path)\n \t}\n \n \tprepare_attr_stack(path, dirlen);\n-\tfor (i = 0; i < attr_nr; i++)\n+\tfor (i = 0; i < git_attr_nr; i++)\n \t\tcheck_all_attr[i].value = ATTR__UNKNOWN;\n \n-\trem = attr_nr;\n+\trem = git_attr_nr;\n \tfor (stk = attr_stack; 0 < rem && stk; stk = stk->prev)\n \t\trem = fill(path, pathlen, basename_offset, stk, rem);\n }\n@@ -762,7 +762,7 @@ int git_all_attrs(const char *path, int *num, struct git_attr_check **check)\n \n \t/* Count the number of attributes that are set. */\n \tcount = 0;\n-\tfor (i = 0; i < attr_nr; i++) {\n+\tfor (i = 0; i < git_attr_nr; i++) {\n \t\tconst char *value = check_all_attr[i].value;\n \t\tif (value != ATTR__UNSET && value != ATTR__UNKNOWN)\n \t\t\t++count;\n@@ -770,7 +770,7 @@ int git_all_attrs(const char *path, int *num, struct git_attr_check **check)\n \t*num = count;\n \t*check = xmalloc(sizeof(**check) * count);\n \tj = 0;\n-\tfor (i = 0; i < attr_nr; i++) {\n+\tfor (i = 0; i < git_attr_nr; i++) {\n \t\tconst char *value = check_all_attr[i].value;\n \t\tif (value != ATTR__UNSET && value != ATTR__UNKNOWN) {\n \t\t\t(*check)[j].attr = check_all_attr[i].attr;\n-- \n2.2.0.84.ge9c7a8a\n"},{"id":"253413","messageId":"1418133205-18213-3-git-send-email-pclouds@gmail.com","threadId":"38150","inReplyTo":"1418133205-18213-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 2/4] attr.c: split path processing code out of collect_all_attrs()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-12-09T13:53:23Z","receivedAt":"2014-12-09T13:53:23Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy \n---\n attr.c | 24 +++++++++++++++++-------\n 1 file changed, 17 insertions(+), 7 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 583d36a..def09c7 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -705,14 +705,9 @@ static int macroexpand_one(int attr_nr, int rem)\n \treturn rem;\n }\n \n-/*\n- * Collect all attributes for path into the array pointed to by\n- * check_all_attr.\n- */\n-static void collect_all_attrs(const char *path)\n+static int split_path(const char *path, int *dirlen_p, int *basename_offset_p)\n {\n-\tstruct attr_stack *stk;\n-\tint i, pathlen, rem, dirlen;\n+\tint pathlen, dirlen;\n \tconst char *cp, *last_slash = NULL;\n \tint basename_offset;\n \n@@ -728,7 +723,22 @@ static void collect_all_attrs(const char *path)\n \t\tbasename_offset = 0;\n \t\tdirlen = 0;\n \t}\n+\t*dirlen_p = dirlen;\n+\t*basename_offset_p = basename_offset;\n+\treturn pathlen;\n+}\n+\n+/*\n+ * Collect all attributes for path into the array pointed to by\n+ * check_all_attr.\n+ */\n+static void collect_all_attrs(const char *path)\n+{\n+\tstruct attr_stack *stk;\n+\tint i, pathlen, rem, dirlen;\n+\tint basename_offset;\n \n+\tpathlen = split_path(path, &dirlen, &basename_offset);\n \tprepare_attr_stack(path, dirlen);\n \tfor (i = 0; i < git_attr_nr; i++)\n \t\tcheck_all_attr[i].value = ATTR__UNKNOWN;\n-- \n2.2.0.84.ge9c7a8a\n"},{"id":"253415","messageId":"1418133205-18213-4-git-send-email-pclouds@gmail.com","threadId":"38150","inReplyTo":"1418133205-18213-1-git-send-email-pclouds@gmail.com","subject":"[PATCH/RFC 3/4] attr: do not attempt to expand when we know it's not a macro","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-12-09T13:53:24Z","receivedAt":"2014-12-09T13:53:24Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Keep track of all recognized macros in the new \"maybe_macro\" field.\nThis this field is true, it _may_ be a macro (depending on what's in\nthe current attr stack). But if the field is false, it's definitely\nnot a macro, no need to go through the whole attr stack in\nmacroexpand_one() to search for one.\n\nWithout this, \"git grep abcdefghi\" on git.git hits the inner loop in\nmacroexpand_one() about 2500 times. With this, it's about 60 times.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n attr.c | 10 +++++++---\n 1 file changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex def09c7..4ec6186 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -32,6 +32,7 @@ struct git_attr {\n \tstruct git_attr *next;\n \tunsigned h;\n \tint attr_nr;\n+\tint maybe_macro;\n \tchar name[FLEX_ARRAY];\n };\n static int git_attr_nr;\n@@ -95,6 +96,7 @@ static struct git_attr *git_attr_internal(const char *name, int len)\n \ta->h = hval;\n \ta->next = git_attr_hash[pos];\n \ta->attr_nr = git_attr_nr++;\n+\ta->maybe_macro = 0;\n \tgit_attr_hash[pos] = a;\n \n \tREALLOC_ARRAY(check_all_attr, git_attr_nr);\n@@ -244,9 +246,10 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,\n \t\t sizeof(*res) +\n \t\t sizeof(struct attr_state) * num_attr +\n \t\t (is_macro ? 0 : namelen + 1));\n-\tif (is_macro)\n+\tif (is_macro) {\n \t\tres->u.attr = git_attr_internal(name, namelen);\n-\telse {\n+\t\tres->u.attr->maybe_macro = 1;\n+\t} else {\n \t\tchar *p = (char *)&(res->state[num_attr]);\n \t\tmemcpy(p, name, namelen);\n \t\tres->u.pat.pattern = p;\n@@ -687,7 +690,8 @@ static int macroexpand_one(int attr_nr, int rem)\n \tstruct match_attr *a = NULL;\n \tint i;\n \n-\tif (check_all_attr[attr_nr].value != ATTR__TRUE)\n+\tif (check_all_attr[attr_nr].value != ATTR__TRUE ||\n+\t !check_all_attr[attr_nr].attr->maybe_macro)\n \t\treturn rem;\n \n \tfor (stk = attr_stack; !a && stk; stk = stk->prev)\n-- \n2.2.0.84.ge9c7a8a\n"},{"id":"253414","messageId":"1418133205-18213-5-git-send-email-pclouds@gmail.com","threadId":"38150","inReplyTo":"1418133205-18213-1-git-send-email-pclouds@gmail.com","subject":"[PATCH/RFC 4/4] attr: avoid heavy work when we know the specified attr is not defined","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-12-09T13:53:25Z","receivedAt":"2014-12-09T13:53:25Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"If we have never seen attr 'X' in any .gitattributes file we have\nexamined so far, we can be sure that 'X' is not defined. So no need to\ngo over all the attr stack to look for attr 'X'. This is the purpose\nbehind this new field maybe_real.\n\nThis optimization breaks down if macros are involved because we can't\nknow for sure what macro would expand to 'X' at attr parsing time. But\nif we go the permisstic way and assume all macros are expanded, we hit\nthe builtin \"binary\" macro. At least the \"diff\" attr defined in this\nmacro will disable this optimization for git-grep. So we wait until\nany attr lines _may_ reference to a macro before we turn this off.\n\nIn git.git, this reduces the number of fill_one() call for \"git grep\nabcdefghi\" from ~5300 to 3000. The optimization stops when it reads\nt/.gitattributes, which uses 'binary' macro.\n\n\"git grep\" is actually a good example to justify this patch. The\ncommand checks \"diff\" attribute on every file. People usually don't\ndefine this attribute. But they pay the attr lookup penalty anyway\nwithout this patch, proportional to the number of attr lines they have\nin repo.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n attr.c | 44 +++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 43 insertions(+), 1 deletion(-)\n\ndiff --git a/attr.c b/attr.c\nindex 4ec6186..ba41761 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -33,9 +33,11 @@ struct git_attr {\n \tunsigned h;\n \tint attr_nr;\n \tint maybe_macro;\n+\tint maybe_real;\n \tchar name[FLEX_ARRAY];\n };\n static int git_attr_nr;\n+static int cannot_trust_maybe_real;\n \n static struct git_attr_check *check_all_attr;\n static struct git_attr *(git_attr_hash[HASHSIZE]);\n@@ -97,6 +99,7 @@ static struct git_attr *git_attr_internal(const char *name, int len)\n \ta->next = git_attr_hash[pos];\n \ta->attr_nr = git_attr_nr++;\n \ta->maybe_macro = 0;\n+\ta->maybe_real = 0;\n \tgit_attr_hash[pos] = a;\n \n \tREALLOC_ARRAY(check_all_attr, git_attr_nr);\n@@ -269,6 +272,10 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,\n \t/* Second pass to fill the attr_states */\n \tfor (cp = states, i = 0; *cp; i++) {\n \t\tcp = parse_attr(src, lineno, cp, &(res->state[i]));\n+\t\tif (!is_macro)\n+\t\t\tres->state[i].attr->maybe_real = 1;\n+\t\tif (res->state[i].attr->maybe_macro)\n+\t\t\tcannot_trust_maybe_real = 1;\n \t}\n \n \treturn res;\n@@ -752,11 +759,46 @@ static void collect_all_attrs(const char *path)\n \t\trem = fill(path, pathlen, basename_offset, stk, rem);\n }\n \n+static void collect_selected_attrs(const char *path, int num,\n+\t\t\t\t struct git_attr_check *check)\n+{\n+\tstruct attr_stack *stk;\n+\tint i, pathlen, rem, dirlen;\n+\tint basename_offset;\n+\n+\tpathlen = split_path(path, &dirlen, &basename_offset);\n+\tprepare_attr_stack(path, dirlen);\n+\tif (cannot_trust_maybe_real) {\n+\t\tfor (i = 0; i < git_attr_nr; i++)\n+\t\t\tcheck_all_attr[i].value = ATTR__UNKNOWN;\n+\t} else {\n+\t\trem = num;\n+\t\tfor (i = 0; i < num; i++) {\n+\t\t\tstruct git_attr_check *c;\n+\t\t\tc = check_all_attr + check[i].attr->attr_nr;\n+\t\t\tif (check[i].attr->maybe_real)\n+\t\t\t\tc->value = ATTR__UNKNOWN;\n+\t\t\telse {\n+\t\t\t\tc->value = ATTR__UNSET;\n+\t\t\t\trem--;\n+\t\t\t}\n+\t\t}\n+\t\tif (!rem)\n+\t\t\treturn;\n+\t}\n+\trem = git_attr_nr;\n+\tfor (stk = attr_stack; 0 < rem && stk; stk = stk->prev)\n+\t\trem = fill(path, pathlen, basename_offset, stk, rem);\n+}\n+\n int git_check_attr(const char *path, int num, struct git_attr_check *check)\n {\n \tint i;\n \n-\tcollect_all_attrs(path);\n+\tif (cannot_trust_maybe_real)\n+\t\tcollect_all_attrs(path);\n+\telse\n+\t\tcollect_selected_attrs(path, num, check);\n \n \tfor (i = 0; i < num; i++) {\n \t\tconst char *value = check_all_attr[check[i].attr->attr_nr].value;\n-- \n2.2.0.84.ge9c7a8a\n"},{"id":"253465","messageId":"CAPig+cSej-7iFOJcprEnkqhsbt87htZ+OB37HE6RuiXdG_F3Yw@mail.gmail.com","threadId":"38150","inReplyTo":"1418133205-18213-4-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH/RFC 3/4] attr: do not attempt to expand when we know it's not a macro","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-12-09T23:27:39Z","receivedAt":"2014-12-09T23:27:39Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Dec 9, 2014 at 8:53 AM, Nguyễn Thái Ngọc Duy wrote:\n> Keep track of all recognized macros in the new \"maybe_macro\" field.\n> This this field is true, it _may_ be a macro (depending on what's in\n\ns/This this/If this/\n\n> the current attr stack). But if the field is false, it's definitely\n> not a macro, no need to go through the whole attr stack in\n> macroexpand_one() to search for one.\n>\n> Without this, \"git grep abcdefghi\" on git.git hits the inner loop in\n> macroexpand_one() about 2500 times. With this, it's about 60 times.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n"},{"id":"253467","messageId":"xmqq1to8l6qr.fsf@gitster.dls.corp.google.com","threadId":"38150","inReplyTo":"1418133205-18213-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 1/4] attr.c: rename global var attr_nr to git_attr_nr","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-09T23:54:04Z","receivedAt":"2014-12-09T23:54:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> This name \"attr_nr\" is used elsewhere as local variable, shadowing the\n> global one. Let's rename the global one into something else to avoid\n> accidents due to shadow in future.\n\nEven though I do not think I would reject a patch to add this entire\nfile if the variable were named git_attr_nr from day one (read: the\nresult of the patch is not wrong per-se), I am not sure if the\nvariable deserves \"git_\" prefix that makes it look as if it may need\nto be protected from global namespace contamination, given that this\nis a file-scope static.\n\nIt might make a lot more sense not to do this rename, but change the\nfirst parameter of macroexpand_one() from attr_nr to something more\nappropriate, like \"struct git_attr *attr\"?\n\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> attr.c | 14 +++++++-------\n> 1 file changed, 7 insertions(+), 7 deletions(-)\n>\n> diff --git a/attr.c b/attr.c\n> index cd54697..583d36a 100644\n> --- a/attr.c\n> +++ b/attr.c\n> @@ -34,7 +34,7 @@ struct git_attr {\n> \tint attr_nr;\n> \tchar name[FLEX_ARRAY];\n> };\n> -static int attr_nr;\n> +static int git_attr_nr;\n> \n> static struct git_attr_check *check_all_attr;\n> static struct git_attr *(git_attr_hash[HASHSIZE]);\n> @@ -94,10 +94,10 @@ static struct git_attr *git_attr_internal(const char *name, int len)\n> \ta->name[len] = 0;\n> \ta->h = hval;\n> \ta->next = git_attr_hash[pos];\n> -\ta->attr_nr = attr_nr++;\n> +\ta->attr_nr = git_attr_nr++;\n> \tgit_attr_hash[pos] = a;\n> \n> -\tREALLOC_ARRAY(check_all_attr, attr_nr);\n> +\tREALLOC_ARRAY(check_all_attr, git_attr_nr);\n> \tcheck_all_attr[a->attr_nr].attr = a;\n> \tcheck_all_attr[a->attr_nr].value = ATTR__UNKNOWN;\n> \treturn a;\n> @@ -730,10 +730,10 @@ static void collect_all_attrs(const char *path)\n> \t}\n> \n> \tprepare_attr_stack(path, dirlen);\n> -\tfor (i = 0; i < attr_nr; i++)\n> +\tfor (i = 0; i < git_attr_nr; i++)\n> \t\tcheck_all_attr[i].value = ATTR__UNKNOWN;\n> \n> -\trem = attr_nr;\n> +\trem = git_attr_nr;\n> \tfor (stk = attr_stack; 0 < rem && stk; stk = stk->prev)\n> \t\trem = fill(path, pathlen, basename_offset, stk, rem);\n> }\n> @@ -762,7 +762,7 @@ int git_all_attrs(const char *path, int *num, struct git_attr_check **check)\n> \n> \t/* Count the number of attributes that are set. */\n> \tcount = 0;\n> -\tfor (i = 0; i < attr_nr; i++) {\n> +\tfor (i = 0; i < git_attr_nr; i++) {\n> \t\tconst char *value = check_all_attr[i].value;\n> \t\tif (value != ATTR__UNSET && value != ATTR__UNKNOWN)\n> \t\t\t++count;\n> @@ -770,7 +770,7 @@ int git_all_attrs(const char *path, int *num, struct git_attr_check **check)\n> \t*num = count;\n> \t*check = xmalloc(sizeof(**check) * count);\n> \tj = 0;\n> -\tfor (i = 0; i < attr_nr; i++) {\n> +\tfor (i = 0; i < git_attr_nr; i++) {\n> \t\tconst char *value = check_all_attr[i].value;\n> \t\tif (value != ATTR__UNSET && value != ATTR__UNKNOWN) {\n> \t\t\t(*check)[j].attr = check_all_attr[i].attr;\n"},{"id":"253468","messageId":"xmqqwq60js2z.fsf@gitster.dls.corp.google.com","threadId":"38150","inReplyTo":"1418133205-18213-4-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH/RFC 3/4] attr: do not attempt to expand when we know it's not a macro","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-09T23:56:04Z","receivedAt":"2014-12-09T23:56:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> Keep track of all recognized macros in the new \"maybe_macro\" field.\n> This this field is true, it _may_ be a macro (depending on what's in\n> the current attr stack). But if the field is false, it's definitely\n> not a macro, no need to go through the whole attr stack in\n> macroexpand_one() to search for one.\n>\n> Without this, \"git grep abcdefghi\" on git.git hits the inner loop in\n> macroexpand_one() about 2500 times. With this, it's about 60 times.\n\nNice ;-)\n\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> attr.c | 10 +++++++---\n> 1 file changed, 7 insertions(+), 3 deletions(-)\n>\n> diff --git a/attr.c b/attr.c\n> index def09c7..4ec6186 100644\n> --- a/attr.c\n> +++ b/attr.c\n> @@ -32,6 +32,7 @@ struct git_attr {\n> \tstruct git_attr *next;\n> \tunsigned h;\n> \tint attr_nr;\n> +\tint maybe_macro;\n> \tchar name[FLEX_ARRAY];\n> };\n> static int git_attr_nr;\n> @@ -95,6 +96,7 @@ static struct git_attr *git_attr_internal(const char *name, int len)\n> \ta->h = hval;\n> \ta->next = git_attr_hash[pos];\n> \ta->attr_nr = git_attr_nr++;\n> +\ta->maybe_macro = 0;\n> \tgit_attr_hash[pos] = a;\n> \n> \tREALLOC_ARRAY(check_all_attr, git_attr_nr);\n> @@ -244,9 +246,10 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,\n> \t\t sizeof(*res) +\n> \t\t sizeof(struct attr_state) * num_attr +\n> \t\t (is_macro ? 0 : namelen + 1));\n> -\tif (is_macro)\n> +\tif (is_macro) {\n> \t\tres->u.attr = git_attr_internal(name, namelen);\n> -\telse {\n> +\t\tres->u.attr->maybe_macro = 1;\n> +\t} else {\n> \t\tchar *p = (char *)&(res->state[num_attr]);\n> \t\tmemcpy(p, name, namelen);\n> \t\tres->u.pat.pattern = p;\n> @@ -687,7 +690,8 @@ static int macroexpand_one(int attr_nr, int rem)\n> \tstruct match_attr *a = NULL;\n> \tint i;\n> \n> -\tif (check_all_attr[attr_nr].value != ATTR__TRUE)\n> +\tif (check_all_attr[attr_nr].value != ATTR__TRUE ||\n> +\t !check_all_attr[attr_nr].attr->maybe_macro)\n> \t\treturn rem;\n> \n> \tfor (stk = attr_stack; !a && stk; stk = stk->prev)\n"},{"id":"253469","messageId":"xmqqsigojr0u.fsf@gitster.dls.corp.google.com","threadId":"38150","inReplyTo":"1418133205-18213-5-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH/RFC 4/4] attr: avoid heavy work when we know the specified attr is not defined","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-10T00:18:57Z","receivedAt":"2014-12-10T00:18:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> +static void collect_selected_attrs(const char *path, int num,\n> +\t\t\t\t struct git_attr_check *check)\n> +{\n> +\tstruct attr_stack *stk;\n> +\tint i, pathlen, rem, dirlen;\n> +\tint basename_offset;\n> +\n> +\tpathlen = split_path(path, &dirlen, &basename_offset);\n> +\tprepare_attr_stack(path, dirlen);\n> +\tif (cannot_trust_maybe_real) {\n> +\t\tfor (i = 0; i < git_attr_nr; i++)\n> +\t\t\tcheck_all_attr[i].value = ATTR__UNKNOWN;\n\nJudging from the fact that\n\n (1) the only caller calls this function in this fashion based on the\n setting of \"cannot-trust\" bit,\n\n (2) this and the other function the only caller calls share the\n same code in their beginning part, and\n\n (3) the body of the if() statement here duplicates the code from\n collect_all_attrs(),\n\nI smell that a much better split is possible.\n\nWhy isn't this all inside a single function collect_all_attrs()?\nThat single function may no longer be collect_ALL_attrs, so renaming\nit to collect_attrs() is fine, but then that function may have this\nif () to initialize all of them to ATTR__UNKNOWN or do the else part\nwe see below, and when organized that way we do not need to have\nduplicated code (or split_path() helper function), no?\n\n> +\t} else {\n> +\t\trem = num;\n> +\t\tfor (i = 0; i < num; i++) {\n> +\t\t\tstruct git_attr_check *c;\n> +\t\t\tc = check_all_attr + check[i].attr->attr_nr;\n> +\t\t\tif (check[i].attr->maybe_real)\n> +\t\t\t\tc->value = ATTR__UNKNOWN;\n> +\t\t\telse {\n> +\t\t\t\tc->value = ATTR__UNSET;\n> +\t\t\t\trem--;\n> +\t\t\t}\n> +\t\t}\n> +\t\tif (!rem)\n> +\t\t\treturn;\n> +\t}\n> +\trem = git_attr_nr;\n> +\tfor (stk = attr_stack; 0 < rem && stk; stk = stk->prev)\n> +\t\trem = fill(path, pathlen, basename_offset, stk, rem);\n> +}\n> +\n> int git_check_attr(const char *path, int num, struct git_attr_check *check)\n> {\n> \tint i;\n> \n> -\tcollect_all_attrs(path);\n> +\tif (cannot_trust_maybe_real)\n> +\t\tcollect_all_attrs(path);\n> +\telse\n> +\t\tcollect_selected_attrs(path, num, check);\n> \n> \tfor (i = 0; i < num; i++) {\n> \t\tconst char *value = check_all_attr[check[i].attr->attr_nr].value;\n"},{"id":"253688","messageId":"20141215005018.GA31006@lanh","threadId":"38150","inReplyTo":"xmqqsigojr0u.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH/RFC 4/4] attr: avoid heavy work when we know the specified attr is not defined","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-12-15T00:50:18Z","receivedAt":"2014-12-15T00:50:18Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Dec 09, 2014 at 04:18:57PM -0800, Junio C Hamano wrote:\n> Nguyễn Thái Ngọc Duy writes:\n> \n> > +static void collect_selected_attrs(const char *path, int num,\n> > +\t\t\t\t struct git_attr_check *check)\n> > +{\n> > +\tstruct attr_stack *stk;\n> > +\tint i, pathlen, rem, dirlen;\n> > +\tint basename_offset;\n> > +\n> > +\tpathlen = split_path(path, &dirlen, &basename_offset);\n> > +\tprepare_attr_stack(path, dirlen);\n> > +\tif (cannot_trust_maybe_real) {\n> > +\t\tfor (i = 0; i < git_attr_nr; i++)\n> > +\t\t\tcheck_all_attr[i].value = ATTR__UNKNOWN;\n> \n> Judging from the fact that\n> \n> (1) the only caller calls this function in this fashion based on the\n> setting of \"cannot-trust\" bit,\n> \n> (2) this and the other function the only caller calls share the\n> same code in their beginning part, and\n> \n> (3) the body of the if() statement here duplicates the code from\n> collect_all_attrs(),\n> \n> I smell that a much better split is possible.\n> \n> Why isn't this all inside a single function collect_all_attrs()?\n> That single function may no longer be collect_ALL_attrs, so renaming\n> it to collect_attrs() is fine, but then that function may have this\n> if () to initialize all of them to ATTR__UNKNOWN or do the else part\n> we see below, and when organized that way we do not need to have\n> duplicated code (or split_path() helper function), no?\n\nSomething like this? Definitely looks better.\n\n-- 8< --\ndiff --git a/attr.c b/attr.c\nindex b80e52b..0f828e3 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -33,9 +33,11 @@ struct git_attr {\n \tunsigned h;\n \tint attr_nr;\n \tint maybe_macro;\n+\tint maybe_real;\n \tchar name[FLEX_ARRAY];\n };\n static int attr_nr;\n+static int cannot_trust_maybe_real;\n \n static struct git_attr_check *check_all_attr;\n static struct git_attr *(git_attr_hash[HASHSIZE]);\n@@ -97,6 +99,7 @@ static struct git_attr *git_attr_internal(const char *name, int len)\n \ta->next = git_attr_hash[pos];\n \ta->attr_nr = attr_nr++;\n \ta->maybe_macro = 0;\n+\ta->maybe_real = 0;\n \tgit_attr_hash[pos] = a;\n \n \tREALLOC_ARRAY(check_all_attr, attr_nr);\n@@ -269,6 +272,10 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,\n \t/* Second pass to fill the attr_states */\n \tfor (cp = states, i = 0; *cp; i++) {\n \t\tcp = parse_attr(src, lineno, cp, &(res->state[i]));\n+\t\tif (!is_macro)\n+\t\t\tres->state[i].attr->maybe_real = 1;\n+\t\tif (res->state[i].attr->maybe_macro)\n+\t\t\tcannot_trust_maybe_real = 1;\n \t}\n \n \treturn res;\n@@ -713,7 +720,9 @@ static int macroexpand_one(int nr, int rem)\n * Collect all attributes for path into the array pointed to by\n * check_all_attr.\n */\n-static void collect_all_attrs(const char *path)\n+static void collect_some_attrs(const char *path, int num,\n+\t\t\t struct git_attr_check *check)\n+\n {\n \tstruct attr_stack *stk;\n \tint i, pathlen, rem, dirlen;\n@@ -736,6 +745,19 @@ static void collect_all_attrs(const char *path)\n \tprepare_attr_stack(path, dirlen);\n \tfor (i = 0; i < attr_nr; i++)\n \t\tcheck_all_attr[i].value = ATTR__UNKNOWN;\n+\tif (num && !cannot_trust_maybe_real) {\n+\t\trem = 0;\n+\t\tfor (i = 0; i < num; i++) {\n+\t\t\tif (!check[i].attr->maybe_real) {\n+\t\t\t\tstruct git_attr_check *c;\n+\t\t\t\tc = check_all_attr + check[i].attr->attr_nr;\n+\t\t\t\tc->value = ATTR__UNSET;\n+\t\t\t\trem++;\n+\t\t\t}\n+\t\t}\n+\t\tif (rem == num)\n+\t\t\treturn;\n+\t}\n \n \trem = attr_nr;\n \tfor (stk = attr_stack; 0 < rem && stk; stk = stk->prev)\n@@ -746,7 +768,7 @@ int git_check_attr(const char *path, int num, struct git_attr_check *check)\n {\n \tint i;\n \n-\tcollect_all_attrs(path);\n+\tcollect_some_attrs(path, num, check);\n \n \tfor (i = 0; i < num; i++) {\n \t\tconst char *value = check_all_attr[check[i].attr->attr_nr].value;\n@@ -762,7 +784,7 @@ int git_all_attrs(const char *path, int *num, struct git_attr_check **check)\n {\n \tint i, count, j;\n \n-\tcollect_all_attrs(path);\n+\tcollect_some_attrs(path, 0, NULL);\n \n \t/* Count the number of attributes that are set. */\n \tcount = 0;\n-- 8< --\n"},{"id":"253694","messageId":"xmqqfvcges78.fsf@gitster.dls.corp.google.com","threadId":"38150","inReplyTo":"20141215005018.GA31006@lanh","subject":"Re: [PATCH/RFC 4/4] attr: avoid heavy work when we know the specified attr is not defined","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-15T17:30:35Z","receivedAt":"2014-12-15T17:30:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Tue, Dec 09, 2014 at 04:18:57PM -0800, Junio C Hamano wrote:\n> ...\n>> I smell that a much better split is possible.\n>> ...\n>\n> Something like this? Definitely looks better.\n\nYeah, I was lazy and did not try it myself to see what the end\nresult would look like when I commented, but doing it this way\navoids needless repetitions.\n\nThe comment block before the collect_*_attrs function need to be\nadjusted to match the updated behaviour, though.\n\nThanks.\n\n> -- 8< --\n> diff --git a/attr.c b/attr.c\n> index b80e52b..0f828e3 100644\n> --- a/attr.c\n> +++ b/attr.c\n> @@ -33,9 +33,11 @@ struct git_attr {\n> \tunsigned h;\n> \tint attr_nr;\n> \tint maybe_macro;\n> +\tint maybe_real;\n> \tchar name[FLEX_ARRAY];\n> };\n> static int attr_nr;\n> +static int cannot_trust_maybe_real;\n> \n> static struct git_attr_check *check_all_attr;\n> static struct git_attr *(git_attr_hash[HASHSIZE]);\n> @@ -97,6 +99,7 @@ static struct git_attr *git_attr_internal(const char *name, int len)\n> \ta->next = git_attr_hash[pos];\n> \ta->attr_nr = attr_nr++;\n> \ta->maybe_macro = 0;\n> +\ta->maybe_real = 0;\n> \tgit_attr_hash[pos] = a;\n> \n> \tREALLOC_ARRAY(check_all_attr, attr_nr);\n> @@ -269,6 +272,10 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,\n> \t/* Second pass to fill the attr_states */\n> \tfor (cp = states, i = 0; *cp; i++) {\n> \t\tcp = parse_attr(src, lineno, cp, &(res->state[i]));\n> +\t\tif (!is_macro)\n> +\t\t\tres->state[i].attr->maybe_real = 1;\n> +\t\tif (res->state[i].attr->maybe_macro)\n> +\t\t\tcannot_trust_maybe_real = 1;\n> \t}\n> \n> \treturn res;\n> @@ -713,7 +720,9 @@ static int macroexpand_one(int nr, int rem)\n> * Collect all attributes for path into the array pointed to by\n> * check_all_attr.\n> */\n> -static void collect_all_attrs(const char *path)\n> +static void collect_some_attrs(const char *path, int num,\n> +\t\t\t struct git_attr_check *check)\n> +\n> {\n> \tstruct attr_stack *stk;\n> \tint i, pathlen, rem, dirlen;\n> @@ -736,6 +745,19 @@ static void collect_all_attrs(const char *path)\n> \tprepare_attr_stack(path, dirlen);\n> \tfor (i = 0; i < attr_nr; i++)\n> \t\tcheck_all_attr[i].value = ATTR__UNKNOWN;\n> +\tif (num && !cannot_trust_maybe_real) {\n> +\t\trem = 0;\n> +\t\tfor (i = 0; i < num; i++) {\n> +\t\t\tif (!check[i].attr->maybe_real) {\n> +\t\t\t\tstruct git_attr_check *c;\n> +\t\t\t\tc = check_all_attr + check[i].attr->attr_nr;\n> +\t\t\t\tc->value = ATTR__UNSET;\n> +\t\t\t\trem++;\n> +\t\t\t}\n> +\t\t}\n> +\t\tif (rem == num)\n> +\t\t\treturn;\n> +\t}\n> \n> \trem = attr_nr;\n> \tfor (stk = attr_stack; 0 < rem && stk; stk = stk->prev)\n> @@ -746,7 +768,7 @@ int git_check_attr(const char *path, int num, struct git_attr_check *check)\n> {\n> \tint i;\n> \n> -\tcollect_all_attrs(path);\n> +\tcollect_some_attrs(path, num, check);\n> \n> \tfor (i = 0; i < num; i++) {\n> \t\tconst char *value = check_all_attr[check[i].attr->attr_nr].value;\n> @@ -762,7 +784,7 @@ int git_all_attrs(const char *path, int *num, struct git_attr_check **check)\n> {\n> \tint i, count, j;\n> \n> -\tcollect_all_attrs(path);\n> +\tcollect_some_attrs(path, 0, NULL);\n> \n> \t/* Count the number of attributes that are set. */\n> \tcount = 0;\n> -- 8< --\n"},{"id":"254117","messageId":"1419723588-13236-1-git-send-email-pclouds@gmail.com","threadId":"38150","inReplyTo":"1418133205-18213-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 0/3] some attr optimizations","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-12-27T23:39:45Z","receivedAt":"2014-12-27T23:39:45Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"v2 updates 01/03 (rename the local var to avoid shadowing instead) and\nmerges 02/04 and 04/04 to 03/03.\n\nNguyễn Thái Ngọc Duy (3):\n attr.c: rename arg name attr_nr to avoid shadowing the global one\n attr: do not attempt to expand when we know it's not a macro\n attr: avoid heavy work when we know the specified attr is not defined\n\n attr.c | 47 +++++++++++++++++++++++++++++++++++++----------\n 1 file changed, 37 insertions(+), 10 deletions(-)\n\n-- \n2.2.0.84.ge9c7a8a\n"},{"id":"254118","messageId":"1419723588-13236-2-git-send-email-pclouds@gmail.com","threadId":"38150","inReplyTo":"1419723588-13236-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 1/3] attr.c: rename arg name attr_nr to avoid shadowing the global one","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-12-27T23:39:46Z","receivedAt":"2014-12-27T23:39:46Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Helped-by: Junio C Hamano \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n attr.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex cd54697..a1758bf 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -681,13 +681,13 @@ static int fill(const char *path, int pathlen, int basename_offset,\n \treturn rem;\n }\n \n-static int macroexpand_one(int attr_nr, int rem)\n+static int macroexpand_one(int nr, int rem)\n {\n \tstruct attr_stack *stk;\n \tstruct match_attr *a = NULL;\n \tint i;\n \n-\tif (check_all_attr[attr_nr].value != ATTR__TRUE)\n+\tif (check_all_attr[nr].value != ATTR__TRUE)\n \t\treturn rem;\n \n \tfor (stk = attr_stack; !a && stk; stk = stk->prev)\n@@ -695,7 +695,7 @@ static int macroexpand_one(int attr_nr, int rem)\n \t\t\tstruct match_attr *ma = stk->attrs[i];\n \t\t\tif (!ma->is_macro)\n \t\t\t\tcontinue;\n-\t\t\tif (ma->u.attr->attr_nr == attr_nr)\n+\t\t\tif (ma->u.attr->attr_nr == nr)\n \t\t\t\ta = ma;\n \t\t}\n \n-- \n2.2.0.84.ge9c7a8a\n"},{"id":"254119","messageId":"1419723588-13236-3-git-send-email-pclouds@gmail.com","threadId":"38150","inReplyTo":"1419723588-13236-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 2/3] attr: do not attempt to expand when we know it's not a macro","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-12-27T23:39:47Z","receivedAt":"2014-12-27T23:39:47Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Keep track of all recognized macros in the new \"maybe_macro\" field.\nIf this field is true, it _may_ be a macro (depending on what's in the\ncurrent attr stack). But if the field is false, it's definitely not a\nmacro, no need to go through the whole attr stack in macroexpand_one()\nto search for one.\n\nWithout this, \"git grep abcdefghi\" on git.git hits the inner loop in\nmacroexpand_one() 2481 times. With this, it's 66 times.\n\nHelped-by: Eric Sunshine \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n attr.c | 10 +++++++---\n 1 file changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex a1758bf..b80e52b 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -32,6 +32,7 @@ struct git_attr {\n \tstruct git_attr *next;\n \tunsigned h;\n \tint attr_nr;\n+\tint maybe_macro;\n \tchar name[FLEX_ARRAY];\n };\n static int attr_nr;\n@@ -95,6 +96,7 @@ static struct git_attr *git_attr_internal(const char *name, int len)\n \ta->h = hval;\n \ta->next = git_attr_hash[pos];\n \ta->attr_nr = attr_nr++;\n+\ta->maybe_macro = 0;\n \tgit_attr_hash[pos] = a;\n \n \tREALLOC_ARRAY(check_all_attr, attr_nr);\n@@ -244,9 +246,10 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,\n \t\t sizeof(*res) +\n \t\t sizeof(struct attr_state) * num_attr +\n \t\t (is_macro ? 0 : namelen + 1));\n-\tif (is_macro)\n+\tif (is_macro) {\n \t\tres->u.attr = git_attr_internal(name, namelen);\n-\telse {\n+\t\tres->u.attr->maybe_macro = 1;\n+\t} else {\n \t\tchar *p = (char *)&(res->state[num_attr]);\n \t\tmemcpy(p, name, namelen);\n \t\tres->u.pat.pattern = p;\n@@ -687,7 +690,8 @@ static int macroexpand_one(int nr, int rem)\n \tstruct match_attr *a = NULL;\n \tint i;\n \n-\tif (check_all_attr[nr].value != ATTR__TRUE)\n+\tif (check_all_attr[nr].value != ATTR__TRUE ||\n+\t !check_all_attr[nr].attr->maybe_macro)\n \t\treturn rem;\n \n \tfor (stk = attr_stack; !a && stk; stk = stk->prev)\n-- \n2.2.0.84.ge9c7a8a\n"},{"id":"254120","messageId":"1419723588-13236-4-git-send-email-pclouds@gmail.com","threadId":"38150","inReplyTo":"1419723588-13236-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 3/3] attr: avoid heavy work when we know the specified attr is not defined","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-12-27T23:39:48Z","receivedAt":"2014-12-27T23:39:48Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"If we have never seen attr 'X' in any .gitattributes file we have\nexamined so far, we can be sure that 'X' is not defined. So no need to\ngo over all the attr stack to look for attr 'X'. This is the purpose\nbehind this new field maybe_real.\n\nThis optimization breaks down if macros are involved because we can't\nknow for sure what macro would expand to 'X' at attr parsing time. But\nif we go the pessimistic way and assume all macros are expanded, we hit\nthe builtin \"binary\" macro. At least the \"diff\" attr defined in this\nmacro will disable this optimization for git-grep. So we wait until\nany attr lines _may_ reference to a macro before we turn this off.\n\nIn git.git, this reduces the number of fill_one() call for \"git grep\nabcdefghi\" from ~5348 to 2955. The optimization stops when it reads\nt/.gitattributes, which uses 'binary' macro. We could probably reduce\nit further by limiting the 'binary' reference to t/ and subdirs only\nin this case.\n\n\"git grep\" is actually a good example to justify this patch. The\ncommand checks \"diff\" attribute on every file. People usually don't\ndefine this attribute. But they pay the attr lookup penalty anyway\nwithout this patch, proportional to the number of attr lines they have\nin repo.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n attr.c | 33 ++++++++++++++++++++++++++++-----\n 1 file changed, 28 insertions(+), 5 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex b80e52b..1f9eebd 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -33,9 +33,11 @@ struct git_attr {\n \tunsigned h;\n \tint attr_nr;\n \tint maybe_macro;\n+\tint maybe_real;\n \tchar name[FLEX_ARRAY];\n };\n static int attr_nr;\n+static int cannot_trust_maybe_real;\n \n static struct git_attr_check *check_all_attr;\n static struct git_attr *(git_attr_hash[HASHSIZE]);\n@@ -97,6 +99,7 @@ static struct git_attr *git_attr_internal(const char *name, int len)\n \ta->next = git_attr_hash[pos];\n \ta->attr_nr = attr_nr++;\n \ta->maybe_macro = 0;\n+\ta->maybe_real = 0;\n \tgit_attr_hash[pos] = a;\n \n \tREALLOC_ARRAY(check_all_attr, attr_nr);\n@@ -269,6 +272,10 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,\n \t/* Second pass to fill the attr_states */\n \tfor (cp = states, i = 0; *cp; i++) {\n \t\tcp = parse_attr(src, lineno, cp, &(res->state[i]));\n+\t\tif (!is_macro)\n+\t\t\tres->state[i].attr->maybe_real = 1;\n+\t\tif (res->state[i].attr->maybe_macro)\n+\t\t\tcannot_trust_maybe_real = 1;\n \t}\n \n \treturn res;\n@@ -710,10 +717,13 @@ static int macroexpand_one(int nr, int rem)\n }\n \n /*\n- * Collect all attributes for path into the array pointed to by\n- * check_all_attr.\n+ * Collect attributes for path into the array pointed to by\n+ * check_all_attr. If num is non-zero, only attributes in check[] are\n+ * collected. Otherwise all attributes are collected.\n */\n-static void collect_all_attrs(const char *path)\n+static void collect_some_attrs(const char *path, int num,\n+\t\t\t struct git_attr_check *check)\n+\n {\n \tstruct attr_stack *stk;\n \tint i, pathlen, rem, dirlen;\n@@ -736,6 +746,19 @@ static void collect_all_attrs(const char *path)\n \tprepare_attr_stack(path, dirlen);\n \tfor (i = 0; i < attr_nr; i++)\n \t\tcheck_all_attr[i].value = ATTR__UNKNOWN;\n+\tif (num && !cannot_trust_maybe_real) {\n+\t\trem = 0;\n+\t\tfor (i = 0; i < num; i++) {\n+\t\t\tif (!check[i].attr->maybe_real) {\n+\t\t\t\tstruct git_attr_check *c;\n+\t\t\t\tc = check_all_attr + check[i].attr->attr_nr;\n+\t\t\t\tc->value = ATTR__UNSET;\n+\t\t\t\trem++;\n+\t\t\t}\n+\t\t}\n+\t\tif (rem == num)\n+\t\t\treturn;\n+\t}\n \n \trem = attr_nr;\n \tfor (stk = attr_stack; 0 < rem && stk; stk = stk->prev)\n@@ -746,7 +769,7 @@ int git_check_attr(const char *path, int num, struct git_attr_check *check)\n {\n \tint i;\n \n-\tcollect_all_attrs(path);\n+\tcollect_some_attrs(path, num, check);\n \n \tfor (i = 0; i < num; i++) {\n \t\tconst char *value = check_all_attr[check[i].attr->attr_nr].value;\n@@ -762,7 +785,7 @@ int git_all_attrs(const char *path, int *num, struct git_attr_check **check)\n {\n \tint i, count, j;\n \n-\tcollect_all_attrs(path);\n+\tcollect_some_attrs(path, 0, NULL);\n \n \t/* Count the number of attributes that are set. */\n \tcount = 0;\n-- \n2.2.0.84.ge9c7a8a\n"},{"id":"254121","messageId":"CAPig+cRp-UJSuQv=GrhL16LZudjTf_8grZrZk-dNc7PHcHSHmg@mail.gmail.com","threadId":"38150","inReplyTo":"1419723588-13236-3-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2 2/3] attr: do not attempt to expand when we know it's not a macro","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-12-27T23:59:30Z","receivedAt":"2014-12-27T23:59:30Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Dec 27, 2014 at 6:39 PM, Nguyễn Thái Ngọc Duy wrote:\n> Keep track of all recognized macros in the new \"maybe_macro\" field.\n> If this field is true, it _may_ be a macro (depending on what's in the\n> current attr stack). But if the field is false, it's definitely not a\n> macro, no need to go through the whole attr stack in macroexpand_one()\n> to search for one.\n>\n> Without this, \"git grep abcdefghi\" on git.git hits the inner loop in\n> macroexpand_one() 2481 times. With this, it's 66 times.\n>\n> Helped-by: Eric Sunshine \n\nMy only contribution was pointing out a typographical error in the\ncommit message[1]; hardly worthy of a Helped-by:.\n\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/261177\n"}]}