{"thread":{"id":"22220","subject":"[PATCH 1/2] rm: only refresh entries that we may touch","startedAt":"2010-01-14T15:02:19Z","lastAt":"2010-01-17T08:43:13Z","messageCount":10,"participants":["Nguyễn Thái Ngọc Duy","Martin Langhoff","Sverre Rabbelier","Nguyen Thai Ngoc Duy","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"131651","messageId":"1263481341-28401-1-git-send-email-pclouds@gmail.com","threadId":"22220","inReplyTo":null,"subject":"[PATCH 0/2] Improve Git performance on big trees","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-01-14T15:02:19Z","receivedAt":"2010-01-14T15:02:19Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The discussion about using Git on Gentoo comes up again [1] and it\nlooks like we can improve Git a bit if user only works on some\nsubdirectories. In such cases, Git still does whole-tree check, which\ntakes quite some time. \"git status\" takes 3 seconds on my machine, but\nI suspect lstat() is not the only culprit, \"git diff --exit-code\" is\nabout 1 sec.\n\nThese patches makes \"git rm \" and \"git status \" a bit\nfaster. Almost 1 sec for \"git rm foo\" still seems too long though,\nprobably due to writing a 9MB index.\n\n[1] http://article.gmane.org/gmane.linux.gentoo.devel/64522\n\nNguyễn Thái Ngọc Duy (2):\n rm: only refresh entries that we may touch\n status: only touch path we may need to check\n\n builtin-commit.c | 2 +-\n builtin-rm.c | 2 +-\n wt-status.c | 2 +-\n 3 files changed, 3 insertions(+), 3 deletions(-)\n"},{"id":"131649","messageId":"1263481341-28401-2-git-send-email-pclouds@gmail.com","threadId":"22220","inReplyTo":"1263481341-28401-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 1/2] rm: only refresh entries that we may touch","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-01-14T15:02:20Z","receivedAt":"2010-01-14T15:02:20Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This gets rid of the whole tree cache refresh. Instead only path that\nwe touch will get refreshed. We may still lstat() more than needed,\nbut it'd be better playing safe.\n\nThis potentially reduces a large number of lstat() on big trees. Take\ngentoo-x86 tree for example, which has roughly 80k files:\n\nUnmodified Git:\n\n$ time git rm --cached skel.ChangeLog\nrm 'skel.ChangeLog'\n\nreal 0m1.323s\nuser 0m0.806s\nsys 0m0.517s\n\nModified Git:\n\n$ time git rm --cached skel.ChangeLog\nrm 'skel.ChangeLog'\n\nreal 0m0.903s\nuser 0m0.780s\nsys 0m0.100s\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin-rm.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-rm.c b/builtin-rm.c\nindex 57975db..4cac3d1 100644\n--- a/builtin-rm.c\n+++ b/builtin-rm.c\n@@ -169,7 +169,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \n \tif (read_cache() < 0)\n \t\tdie(\"index file corrupt\");\n-\trefresh_cache(REFRESH_QUIET);\n \n \tpathspec = get_pathspec(prefix, argv);\n \tseen = NULL;\n@@ -181,6 +180,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \t\tstruct cache_entry *ce = active_cache[i];\n \t\tif (!match_pathspec(pathspec, ce->name, ce_namelen(ce), 0, seen))\n \t\t\tcontinue;\n+\t\trefresh_cache_entry(ce, 1);\n \t\tadd_list(ce->name);\n \t}\n \n-- \n1.6.6.181.g5ee6\n"},{"id":"131650","messageId":"1263481341-28401-3-git-send-email-pclouds@gmail.com","threadId":"22220","inReplyTo":"1263481341-28401-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 2/2] status: only touch path we may need to check","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-01-14T15:02:21Z","receivedAt":"2010-01-14T15:02:21Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This patch gets rid of whole-tree cache refresh and untracked file\nsearch. Instead only specified path will be looked at.\n\nAgain some numbers on gentoo-x86, ~80k files:\n\nUnmodified Git:\n\n$ time git st eclass/\nnothing to commit (working directory clean)\n\nreal 0m3.211s\nuser 0m1.977s\nsys 0m1.135s\n\nModified Git:\n\n$ time ~/w/git/git st eclass/\nnothing to commit (working directory clean)\n\nreal 0m1.587s\nuser 0m1.426s\nsys 0m0.111s\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin-commit.c | 2 +-\n wt-status.c | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex 6199db7..dae6cb3 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -1024,7 +1024,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \t\ts.pathspec = get_pathspec(prefix, argv);\n \n \tread_cache();\n-\trefresh_cache(REFRESH_QUIET|REFRESH_UNMERGED);\n+\trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, s.pathspec, NULL, NULL);\n \ts.is_initial = get_sha1(s.reference, sha1) ? 1 : 0;\n \ts.in_merge = in_merge;\n \twt_status_collect(&s);\ndiff --git a/wt-status.c b/wt-status.c\nindex 5d56988..65feb29 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -343,7 +343,7 @@ static void wt_status_collect_untracked(struct wt_status *s)\n \t\t\tDIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;\n \tsetup_standard_excludes(&dir);\n \n-\tfill_directory(&dir, NULL);\n+\tfill_directory(&dir, s->pathspec);\n \tfor (i = 0; i < dir.nr; i++) {\n \t\tstruct dir_entry *ent = dir.entries[i];\n \t\tif (!cache_name_is_other(ent->name, ent->len))\n-- \n1.6.6.181.g5ee6\n"},{"id":"131652","messageId":"46a038f91001140721j5acb766cxfd777570d6edfca4@mail.gmail.com","threadId":"22220","inReplyTo":"1263481341-28401-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 0/2] Improve Git performance on big trees","fromName":"Martin Langhoff","fromEmail":"martin.langhoff@gmail.com","sentAt":"2010-01-14T15:21:39Z","receivedAt":"2010-01-14T15:21:39Z","isPatch":true,"sender":{"key":"martin.langhoff@gmail.com","avatar":"https://gravatar.com/avatar/1e3f311b6c4c15836501901ca58f8c0b0667246488084ba524d8bc9867e22fd9?d=mp&s=160"},"body":"2010/1/14 Nguyễn Thái Ngọc Duy :\n> Almost 1 sec for \"git rm foo\" still seems too long though,\n> probably due to writing a 9MB index.\n\nOne of the main issues there is that the Gentoo dir tree seems to be\nvery flat. The kernel tree is huge, but much deeper, and does not have\na huge top-level directory -- and git handles it fairly well.\n\nPerhaps the Gentoo tree can be rearranged to be more nested? If your\ndevs strongly prefer a flat view of it, that could be arranged with\nsymlinks.\n\nAlternatively, each port can be in its own repo, and you can make a\n\"top level repo\" using git submodules -- this is what Fedora/RH is\nexploring at the moment.\n\ncheers,\n\n\n\nm\n-- \n martin.langhoff@gmail.com\n martin@laptop.org -- School Server Architect\n - ask interesting questions\n - don't get distracted with shiny stuff - working code first\n - http://wiki.laptop.org/go/User:Martinlanghoff\n"},{"id":"131655","messageId":"fabb9a1e1001140802v7d1e535dn7cfca60c54ec7504@mail.gmail.com","threadId":"22220","inReplyTo":"1263481341-28401-3-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 2/2] status: only touch path we may need to check","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-01-14T16:02:50Z","receivedAt":"2010-01-14T16:02:50Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\n2010/1/14 Nguyễn Thái Ngọc Duy :\n> $ time git st eclass/\n> real    0m3.211s\n\n> $ time ~/w/git/git st eclass/\n> real    0m1.587s\n\nWow, nice reduction!\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"131720","messageId":"fcaeb9bf1001150536r445a9ee4jbf4fea4afa038c99@mail.gmail.com","threadId":"22220","inReplyTo":"46a038f91001140721j5acb766cxfd777570d6edfca4@mail.gmail.com","subject":"Re: [PATCH 0/2] Improve Git performance on big trees","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-01-15T13:36:42Z","receivedAt":"2010-01-15T13:36:42Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On 1/14/10, Martin Langhoff wrote:\n> 2010/1/14 Nguyễn Thái Ngọc Duy :\n>\n> > Almost 1 sec for \"git rm foo\" still seems too long though,\n> > probably due to writing a 9MB index.\n>\n>\n> One of the main issues there is that the Gentoo dir tree seems to be\n> very flat. The kernel tree is huge, but much deeper, and does not have\n> a huge top-level directory -- and git handles it fairly well.\n\nKernel tree is about one third the size of gentoo-x86 in terms of\nworktree. It takes ~1 sec to do \"git status\" on kernel tree and 3 secs\non gentoo-x86, quite proportional. Except that 3 secs feel much longer\nthan 1 :-)\n\nDirectory structure may also affect performance though. Kernel tree\nonly has ~2k directories while gentoo-x86 has ~20k shallow dirs.\n\n> Perhaps the Gentoo tree can be rearranged to be more nested? If your\n> devs strongly prefer a flat view of it, that could be arranged with\n> symlinks.\n>\n> Alternatively, each port can be in its own repo, and you can make a\n> \"top level repo\" using git submodules -- this is what Fedora/RH is\n> exploring at the moment.\n\nThere were discussions of these in the past. And to my experience,\nthese ideas won't go anywhere. The number of worktree files may drop\nsignificantly after Git migration because I believe all ChangeLog\n(~13k files on total ~80k) will be gone.\n-- \nDuy\n"},{"id":"131850","messageId":"7v8wbyiry4.fsf@alter.siamese.dyndns.org","threadId":"22220","inReplyTo":"1263481341-28401-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 1/2] rm: only refresh entries that we may touch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-16T04:39:31Z","receivedAt":"2010-01-16T04:39:31Z","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> diff --git a/builtin-rm.c b/builtin-rm.c\n> index 57975db..4cac3d1 100644\n> --- a/builtin-rm.c\n> +++ b/builtin-rm.c\n> @@ -169,7 +169,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n> \n> \tif (read_cache() < 0)\n> \t\tdie(\"index file corrupt\");\n> -\trefresh_cache(REFRESH_QUIET);\n> \n> \tpathspec = get_pathspec(prefix, argv);\n> \tseen = NULL;\n> @@ -181,6 +180,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n> \t\tstruct cache_entry *ce = active_cache[i];\n> \t\tif (!match_pathspec(pathspec, ce->name, ce_namelen(ce), 0, seen))\n> \t\t\tcontinue;\n> +\t\trefresh_cache_entry(ce, 1);\n\nWhy does this pass \"1\" instead of \"0\"? The existing code does not give\nrefresh_cache() REFRESH_REALLY bit, and a patch that is marked as a pure\noptimization should pass 0. If you really mean it, please spell it as\nCE_MATCH_IGNORE_VALID and justify why it is a good change in a separate\npatch.\n\nThanks.\n"},{"id":"131851","messageId":"7v4ommirv9.fsf@alter.siamese.dyndns.org","threadId":"22220","inReplyTo":"1263481341-28401-3-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 2/2] status: only touch path we may need to check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-16T04:41:14Z","receivedAt":"2010-01-16T04:41:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Will queue this one (but not the \"rm\" one, yet). Thanks.\n"},{"id":"131870","messageId":"fcaeb9bf1001160258w476be063s595568afef088f42@mail.gmail.com","threadId":"22220","inReplyTo":"7v8wbyiry4.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] rm: only refresh entries that we may touch","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-01-16T10:58:41Z","receivedAt":"2010-01-16T10:58:41Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On 1/16/10, Junio C Hamano wrote:\n> Nguyễn Thái Ngọc Duy writes:\n>\n> > diff --git a/builtin-rm.c b/builtin-rm.c\n> > index 57975db..4cac3d1 100644\n> > --- a/builtin-rm.c\n> > +++ b/builtin-rm.c\n> > @@ -169,7 +169,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n> >\n> > if (read_cache() < 0)\n> > die(\"index file corrupt\");\n> > - refresh_cache(REFRESH_QUIET);\n> >\n> > pathspec = get_pathspec(prefix, argv);\n> > seen = NULL;\n> > @@ -181,6 +180,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n> > struct cache_entry *ce = active_cache[i];\n> > if (!match_pathspec(pathspec, ce->name, ce_namelen(ce), 0, seen))\n> > continue;\n> > + refresh_cache_entry(ce, 1);\n>\n>\n> Why does this pass \"1\" instead of \"0\"? The existing code does not give\n> refresh_cache() REFRESH_REALLY bit, and a patch that is marked as a pure\n> optimization should pass 0. If you really mean it, please spell it as\n> CE_MATCH_IGNORE_VALID and justify why it is a good change in a separate\n> patch.\n\n\"refresh_cache(REFRESH_QUIET);\" does pass \"0\" indeed. Can you please\nchange it to \"0\"? Thanks.\n-- \nDuy\n"},{"id":"131926","messageId":"1263717793-24009-1-git-send-email-pclouds@gmail.com","threadId":"22220","inReplyTo":"1263481341-28401-1-git-send-email-pclouds@gmail.com","subject":"[PATCH] rm: only refresh entries that we may touch","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-01-17T08:43:13Z","receivedAt":"2010-01-17T08:43:13Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This gets rid of the whole tree cache refresh. Instead only path that\nwe touch will get refreshed. We may still lstat() more than needed,\nbut it'd be better playing safe.\n\nThis potentially reduces a large number of lstat() on big trees. Take\ngentoo-x86 tree for example, which has roughly 80k files:\n\nUnmodified Git:\n\n$ time git rm --cached skel.ebuild\nrm 'skel.ebuild'\n\nreal 0m1.441s\nuser 0m0.821s\nsys 0m0.531s\n\nModified Git:\n\n$ time ~/w/git/git rm --cached skel.ebuild\nrm 'skel.ebuild'\n\nreal 0m0.941s\nuser 0m0.828s\nsys 0m0.091s\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n In the previous attempt, refresh_cache_entry() returns a new cache_entry.\n It does not modify the_index, so tests failed.\n\n builtin-rm.c | 3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-rm.c b/builtin-rm.c\nindex 57975db..f3772c8 100644\n--- a/builtin-rm.c\n+++ b/builtin-rm.c\n@@ -169,9 +169,10 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \n \tif (read_cache() < 0)\n \t\tdie(\"index file corrupt\");\n-\trefresh_cache(REFRESH_QUIET);\n \n \tpathspec = get_pathspec(prefix, argv);\n+\trefresh_index(&the_index, REFRESH_QUIET, pathspec, NULL, NULL);\n+\n \tseen = NULL;\n \tfor (i = 0; pathspec[i] ; i++)\n \t\t/* nothing */;\n-- \n1.6.6.181.g5ee6\n"}]}