{"thread":{"id":"32632","subject":"git grep performance regression","startedAt":"2013-01-14T22:38:00Z","lastAt":"2013-01-15T07:36:28Z","messageCount":5,"participants":["Ross Lagerwall","Duy Nguyen","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"206848","messageId":"20130114223800.GA12708@hobo.wolfson.cam.ac.uk","threadId":"32632","inReplyTo":null,"subject":"git grep performance regression","fromName":"Ross Lagerwall","fromEmail":"rosslagerwall@gmail.com","sentAt":"2013-01-14T22:38:00Z","receivedAt":"2013-01-14T22:38:00Z","isPatch":false,"sender":{"key":"rosslagerwall@gmail.com","avatar":"https://avatars.githubusercontent.com/u/878055?v=4"},"body":"Hi,\n\nI have noticed a performance regression in git grep between v1.8.1 and\nv1.8.1.1:\n\nOn the kernel tree:\nFor git 1.8.1:\n$ time git grep foodsgsg\n\nreal 0m0.158s\nuser 0m0.290s\nsys 0m0.207s\n\nFor git 1.8.1.1:\n$ time /tmp/g/bin/git grep foodsgsg\n\nreal 0m0.501s\nuser 0m0.707s\nsys 0m0.493s\n\n\nA bisect seems to indicate that it was introduced by 94bc67:\ncommit 94bc671a1f2e8610de475c2494d2763355a99f65\nAuthor: Jean-Noël AVILA \nDate: Sat Dec 8 21:04:39 2012 +0100\n\n Add directory pattern matching to attributes\n \n The manpage of gitattributes says: \"The rules how the pattern\n matches paths are the same as in .gitignore files\" and the gitignore\n pattern matching has a pattern ending with / for directory matching.\n \n This rule is specifically relevant for the 'export-ignore' rule used\n for git archive.\n \n Signed-off-by: Jean-Noel Avila \n Signed-off-by: Junio C Hamano \n\nRegards\n-- \nRoss Lagerwall\n"},{"id":"206877","messageId":"CACsJy8A7FLYqdY2Mt5pUq0nH3N8mbZ4crkYJYFfepp19c0aWhg@mail.gmail.com","threadId":"32632","inReplyTo":"20130114223800.GA12708@hobo.wolfson.cam.ac.uk","subject":"Re: git grep performance regression","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-15T02:46:51Z","receivedAt":"2013-01-15T02:46:51Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jan 15, 2013 at 5:38 AM, Ross Lagerwall wrote:\n> Hi,\n>\n> I have noticed a performance regression in git grep between v1.8.1 and\n> v1.8.1.1:\n>\n> On the kernel tree:\n> For git 1.8.1:\n> $ time git grep foodsgsg\n>\n> real 0m0.158s\n> user 0m0.290s\n> sys 0m0.207s\n>\n> For git 1.8.1.1:\n> $ time /tmp/g/bin/git grep foodsgsg\n>\n> real 0m0.501s\n> user 0m0.707s\n> sys 0m0.493s\n\nInteresting. I see the regression on linux-2.6 too (0.6s real vs 0.9s).\n\n> A bisect seems to indicate that it was introduced by 94bc67:\n> commit 94bc671a1f2e8610de475c2494d2763355a99f65\n> Author: Jean-Noël AVILA \n> Date: Sat Dec 8 21:04:39 2012 +0100\n>\n> Add directory pattern matching to attributes\n>\n> The manpage of gitattributes says: \"The rules how the pattern\n> matches paths are the same as in .gitignore files\" and the gitignore\n> pattern matching has a pattern ending with / for directory matching.\n>\n> This rule is specifically relevant for the 'export-ignore' rule used\n> for git archive.\n\nNot the real contributor to the regression, but it should be noted\nthat glibc's strrchr does not employ a typical loop. Instead it\nadvances with strchr with a note that strchr is much faster. It looks\nlike strchr compares a word at a time instead of a byte. We might want\nto do the same.\n\nI don't have time to look into details now, but by enabling\nDEBUG_ATTR, it looks like this commit makes it push and pop patterns a\nlot more than without the commit.\n-- \nDuy\n"},{"id":"206878","messageId":"7v622z6u1g.fsf@alter.siamese.dyndns.org","threadId":"32632","inReplyTo":"20130114223800.GA12708@hobo.wolfson.cam.ac.uk","subject":"Re: git grep performance regression","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-15T02:48:11Z","receivedAt":"2013-01-15T02:48:11Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ross Lagerwall writes:\n\n> I have noticed a performance regression in git grep between v1.8.1 and\n> v1.8.1.1:\n>\n> On the kernel tree:\n> For git 1.8.1:\n> $ time git grep foodsgsg\n>\n> real 0m0.158s\n> user 0m0.290s\n> sys 0m0.207s\n>\n> For git 1.8.1.1:\n> $ time /tmp/g/bin/git grep foodsgsg\n>\n> real 0m0.501s\n> user 0m0.707s\n> sys 0m0.493s\n>\n>\n> A bisect seems to indicate that it was introduced by 94bc67:\n> commit 94bc671a1f2e8610de475c2494d2763355a99f65\n> Author: Jean-Noël AVILA \n> Date: Sat Dec 8 21:04:39 2012 +0100\n>\n> Add directory pattern matching to attributes\n> \n> The manpage of gitattributes says: \"The rules how the pattern\n> matches paths are the same as in .gitignore files\" and the gitignore\n> pattern matching has a pattern ending with / for directory matching.\n> \n> This rule is specifically relevant for the 'export-ignore' rule used\n> for git archive.\n> \n> Signed-off-by: Jean-Noel Avila \n> Signed-off-by: Junio C Hamano \n\nHmph, that looks really bad, especially given that in the normal\ncodepath like \"git grep\", we would never care about directories (the\nattributes are normally applied to real paths with contents, and the\nuse by archive is an anomaly) and the implementation should be done\nin a way not to impose such excess and useless overhead.\n\nWe may end up reverting that patch for the time being X-<.\n\nJean-Noël, ideas?\n"},{"id":"206882","messageId":"CACsJy8AnABCisgSVL7Qh_-uejAJA3x1kzy=4i+uXm3+90m5tuA@mail.gmail.com","threadId":"32632","inReplyTo":"CACsJy8A7FLYqdY2Mt5pUq0nH3N8mbZ4crkYJYFfepp19c0aWhg@mail.gmail.com","subject":"Re: git grep performance regression","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-15T04:38:32Z","receivedAt":"2013-01-15T04:38:32Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jan 15, 2013 at 9:46 AM, Duy Nguyen wrote:\n> I don't have time to look into details now, but by enabling\n> DEBUG_ATTR, it looks like this commit makes it push and pop patterns a\n> lot more than without the commit.\n\nI think the culprit is at this chunk:\n\n static void prepare_attr_stack(const char *path)\n {\n struct attr_stack *elem, *info;\n int dirlen, len;\n const char *cp;\n\n- cp = strrchr(path, '/');\n- if (!cp)\n- dirlen = 0;\n- else\n- dirlen = cp - path;\n+ dirlen = find_basename(path) - path;\n\ndirlen is not expected to include the trailing slash, but\nfind_basename() does that. It messes up with the path filters for\npush/pop in the next code. This brings grep performance closely back\nto before for me. Ross, can you check (patch could be whitespace\ndamaged by gmail)?\n\n-- 8< --\ndiff --git a/attr.c b/attr.c\nindex b05110d..1e96e26 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -583,6 +583,9 @@ static void prepare_attr_stack(const char *path)\n\n dirlen = find_basename(path) - path;\n\n+ if (dirlen)\n+ dirlen--;\n+\n /*\n * At the bottom of the attribute stack is the built-in\n * set of attribute definitions, followed by the contents\n-- 8< --\n"},{"id":"206911","messageId":"20130115073628.GA647@hobo.wolfson.cam.ac.uk","threadId":"32632","inReplyTo":"CACsJy8AnABCisgSVL7Qh_-uejAJA3x1kzy=4i+uXm3+90m5tuA@mail.gmail.com","subject":"Re: git grep performance regression","fromName":"Ross Lagerwall","fromEmail":"rosslagerwall@gmail.com","sentAt":"2013-01-15T07:36:28Z","receivedAt":"2013-01-15T07:36:28Z","isPatch":false,"sender":{"key":"rosslagerwall@gmail.com","avatar":"https://avatars.githubusercontent.com/u/878055?v=4"},"body":"On Tue, Jan 15, 2013 at 11:38:32AM +0700, Duy Nguyen wrote:\n> dirlen is not expected to include the trailing slash, but\n> find_basename() does that. It messes up with the path filters for\n> push/pop in the next code. This brings grep performance closely back\n> to before for me. Ross, can you check (patch could be whitespace\n> damaged by gmail)?\n> \n\nYes, that did seem to bring back the performance to the old level.\n\nI noticed that before the patch, the strace output was something like\nthis:\nopen(\"tools/vm/.gitattributes\", O_RDONLY) = -1 ENOENT\nopen(\"tools/vm//.gitattributes\", O_RDONLY) = -1 ENOENT\nopen(\"tools/vm//.gitattributes\", O_RDONLY) = -1 ENOENT\nopen(\"tools/vm//.gitattributes\", O_RDONLY) = -1 ENOENT\nopen(\"usr/.gitattributes\", O_RDONLY) = -1 ENOENT\nopen(\"usr//.gitattributes\", O_RDONLY) = -1 ENOENT\nopen(\"usr//.gitattributes\", O_RDONLY) = -1 ENOENT\nopen(\"usr//.gitattributes\", O_RDONLY) = -1 ENOENT\nopen(\"usr//.gitattributes\", O_RDONLY) = -1 ENOENT\nopen(\"usr//.gitattributes\", O_RDONLY) = -1 ENOENT\n\n(and yes, the whitespace was damaged by Gmail!)\n\nRegards\n-- \nRoss Lagerwall\n"}]}