{"thread":{"id":"49741","subject":"[PATCH v1] add: speed up cmd_add() by utilizing read_cache_preload()","startedAt":"2018-11-02T13:31:03Z","lastAt":"2018-11-03T04:48:01Z","messageCount":6,"participants":["Ben Peart","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"362269","messageId":"20181102133050.10756-1-peartben@gmail.com","threadId":"49741","inReplyTo":null,"subject":"[PATCH v1] add: speed up cmd_add() by utilizing read_cache_preload()","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-11-02T13:30:50Z","receivedAt":"2018-11-02T13:31:03Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"From: Ben Peart <benpeart@microsoft.com>\n\nDuring an \"add\", a call is made to run_diff_files() which calls\ncheck_remove() for each index-entry.  The preload_index() code distributes\nsome of the costs across multiple threads.\n\nBecause the files checked are restricted to pathspec, adding individual\nfiles makes no measurable impact but on a Windows repo with ~200K files,\n'git add .' drops from 6.3 seconds to 3.3 seconds for a 47% savings.\n\nSigned-off-by: Ben Peart <benpeart@microsoft.com>\n---\n\nNotes:\n    Base Ref: master\n    Web-Diff: https://github.com/benpeart/git/commit/fc4830b545\n    Checkout: git fetch https://github.com/benpeart/git add-preload-index-v1 && git checkout fc4830b545\n\n builtin/add.c | 9 ++++-----\n 1 file changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex ad49806ebf..f65c172299 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -445,11 +445,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\treturn 0;\n \t}\n \n-\tif (read_cache() < 0)\n-\t\tdie(_(\"index file corrupt\"));\n-\n-\tdie_in_unpopulated_submodule(&the_index, prefix);\n-\n \t/*\n \t * Check the \"pathspec '%s' did not match any files\" block\n \t * below before enabling new magic.\n@@ -459,6 +454,10 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\t       PATHSPEC_SYMLINK_LEADING_PATH,\n \t\t       prefix, argv);\n \n+\tif (read_cache_preload(&pathspec) < 0)\n+\t\tdie(_(\"index file corrupt\"));\n+\n+\tdie_in_unpopulated_submodule(&the_index, prefix);\n \tdie_path_inside_submodule(&the_index, &pathspec);\n \n \tif (add_new_files) {\n\nbase-commit: 4ede3d42dfb57f9a41ac96a1f216c62eb7566cc2\n-- \n2.18.0.windows.1\n\n"},{"id":"362274","messageId":"xmqqy3abo64r.fsf@gitster-ct.c.googlers.com","threadId":"49741","inReplyTo":"20181102133050.10756-1-peartben@gmail.com","subject":"Re: [PATCH v1] add: speed up cmd_add() by utilizing read_cache_preload()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-02T15:23:32Z","receivedAt":"2018-11-02T15:23:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Peart <peartben@gmail.com> writes:\n\n> From: Ben Peart <benpeart@microsoft.com>\n>\n> During an \"add\", a call is made to run_diff_files() which calls\n> check_remove() for each index-entry.  The preload_index() code\n> distributes some of the costs across multiple threads.\n\nNice.  I peeked around and noticed that we already do this in\nbuiltin_diff_index() before running run_diff_index() when !cached,\nand builtin_diff_files(), of course.\n\n> Because the files checked are restricted to pathspec, adding individual\n> files makes no measurable impact but on a Windows repo with ~200K files,\n> 'git add .' drops from 6.3 seconds to 3.3 seconds for a 47% savings.\n\n;-)\n\n> diff --git a/builtin/add.c b/builtin/add.c\n> index ad49806ebf..f65c172299 100644\n> --- a/builtin/add.c\n> +++ b/builtin/add.c\n> @@ -445,11 +445,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n>  \t\treturn 0;\n>  \t}\n>  \n> -\tif (read_cache() < 0)\n> -\t\tdie(_(\"index file corrupt\"));\n> -\n> -\tdie_in_unpopulated_submodule(&the_index, prefix);\n> -\n>  \t/*\n>  \t * Check the \"pathspec '%s' did not match any files\" block\n>  \t * below before enabling new magic.\n\nIt is not explained why this is not a mere s/read_cache/&_preload/\nin the log message.  I can see it is because you wanted to make the\npathspec available to preload to further cut down the preloaded\npaths, and I do not think it has any unintended (negatie) side\neffect to parse the pathspec before populating the in-core index,\nbut that would have been a good thing to mention in the proposed log\nmessage.\n\n> @@ -459,6 +454,10 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n>  \t\t       PATHSPEC_SYMLINK_LEADING_PATH,\n>  \t\t       prefix, argv);\n>  \n> +\tif (read_cache_preload(&pathspec) < 0)\n> +\t\tdie(_(\"index file corrupt\"));\n> +\n> +\tdie_in_unpopulated_submodule(&the_index, prefix);\n>  \tdie_path_inside_submodule(&the_index, &pathspec);\n>  \n>  \tif (add_new_files) {\n>\n> base-commit: 4ede3d42dfb57f9a41ac96a1f216c62eb7566cc2\n"},{"id":"362276","messageId":"CACsJy8CVPSe8TWYMrK9MiRCaG36qyWfd42cEPo5844XWuTmqew@mail.gmail.com","threadId":"49741","inReplyTo":"20181102133050.10756-1-peartben@gmail.com","subject":"Re: [PATCH v1] add: speed up cmd_add() by utilizing read_cache_preload()","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-02T15:49:59Z","receivedAt":"2018-11-02T15:50:29Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Nov 2, 2018 at 2:32 PM Ben Peart <peartben@gmail.com> wrote:\n>\n> From: Ben Peart <benpeart@microsoft.com>\n>\n> During an \"add\", a call is made to run_diff_files() which calls\n> check_remove() for each index-entry.  The preload_index() code distributes\n> some of the costs across multiple threads.\n\nInstead of doing this site by site. How about we make read_cache()\nalways do multithread preload?\n\nThe only downside I see is preload may actually harm when there are\ntoo few cache entries (but more than 500), but this needs to be\nverified. If the penalty is small enough, I think we could live with\nit since everything is fast when you have few entries anyway.\n\nBut if that's not true, we could add a threshold to activate preload.\nSomething like \"if you have 50k files or more, then activate preload\"\nwould do. I notice THREAD_COST in preload code, but I don't think it's\nthe same thing.\n\n>\n> Because the files checked are restricted to pathspec, adding individual\n> files makes no measurable impact but on a Windows repo with ~200K files,\n> 'git add .' drops from 6.3 seconds to 3.3 seconds for a 47% savings.\n>\n> Signed-off-by: Ben Peart <benpeart@microsoft.com>\n> ---\n>\n> Notes:\n>     Base Ref: master\n>     Web-Diff: https://github.com/benpeart/git/commit/fc4830b545\n>     Checkout: git fetch https://github.com/benpeart/git add-preload-index-v1 && git checkout fc4830b545\n>\n>  builtin/add.c | 9 ++++-----\n>  1 file changed, 4 insertions(+), 5 deletions(-)\n>\n> diff --git a/builtin/add.c b/builtin/add.c\n> index ad49806ebf..f65c172299 100644\n> --- a/builtin/add.c\n> +++ b/builtin/add.c\n> @@ -445,11 +445,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n>                 return 0;\n>         }\n>\n> -       if (read_cache() < 0)\n> -               die(_(\"index file corrupt\"));\n> -\n> -       die_in_unpopulated_submodule(&the_index, prefix);\n> -\n>         /*\n>          * Check the \"pathspec '%s' did not match any files\" block\n>          * below before enabling new magic.\n> @@ -459,6 +454,10 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n>                        PATHSPEC_SYMLINK_LEADING_PATH,\n>                        prefix, argv);\n>\n> +       if (read_cache_preload(&pathspec) < 0)\n> +               die(_(\"index file corrupt\"));\n> +\n> +       die_in_unpopulated_submodule(&the_index, prefix);\n>         die_path_inside_submodule(&the_index, &pathspec);\n>\n>         if (add_new_files) {\n>\n> base-commit: 4ede3d42dfb57f9a41ac96a1f216c62eb7566cc2\n> --\n> 2.18.0.windows.1\n>\n\n\n-- \nDuy\n"},{"id":"362280","messageId":"3dc46005-016f-e1c3-32ae-2797317aed08@gmail.com","threadId":"49741","inReplyTo":"xmqqy3abo64r.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v1] add: speed up cmd_add() by utilizing read_cache_preload()","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-11-02T16:14:22Z","receivedAt":"2018-11-02T16:14:27Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 11/2/2018 11:23 AM, Junio C Hamano wrote:\n> Ben Peart <peartben@gmail.com> writes:\n> \n>> From: Ben Peart <benpeart@microsoft.com>\n>>\n>> During an \"add\", a call is made to run_diff_files() which calls\n>> check_remove() for each index-entry.  The preload_index() code\n>> distributes some of the costs across multiple threads.\n> \n> Nice.  I peeked around and noticed that we already do this in\n> builtin_diff_index() before running run_diff_index() when !cached,\n> and builtin_diff_files(), of course.\n> \n\nThanks for double checking!\n\n>> Because the files checked are restricted to pathspec, adding individual\n>> files makes no measurable impact but on a Windows repo with ~200K files,\n>> 'git add .' drops from 6.3 seconds to 3.3 seconds for a 47% savings.\n> \n> ;-)\n> \n>> diff --git a/builtin/add.c b/builtin/add.c\n>> index ad49806ebf..f65c172299 100644\n>> --- a/builtin/add.c\n>> +++ b/builtin/add.c\n>> @@ -445,11 +445,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n>>   \t\treturn 0;\n>>   \t}\n>>   \n>> -\tif (read_cache() < 0)\n>> -\t\tdie(_(\"index file corrupt\"));\n>> -\n>> -\tdie_in_unpopulated_submodule(&the_index, prefix);\n>> -\n>>   \t/*\n>>   \t * Check the \"pathspec '%s' did not match any files\" block\n>>   \t * below before enabling new magic.\n> \n> It is not explained why this is not a mere s/read_cache/&_preload/\n> in the log message.  I can see it is because you wanted to make the\n> pathspec available to preload to further cut down the preloaded\n> paths, and I do not think it has any unintended (negatie) side\n> effect to parse the pathspec before populating the in-core index,\n> but that would have been a good thing to mention in the proposed log\n> message.\n> \n\nThat is correct.  parse_pathspec() was after read_cache() because it \n_used_ to use the index to determine whether a pathspec is in a \nsubmodule.  That was fixed by Brandon in Aug 2017 when he cleaned up all \nsubmodule config code so it is safe to move read_cache_preload() after \nthe call to parse_pathspec().\n\nHow about this for a revised commit message?\n\n\n\nDuring an \"add\", a call is made to run_diff_files() which calls\ncheck_remove() for each index-entry.  The preload_index() code \ndistributes some of the costs across multiple threads.\n\nLimit read_cache_preload() to pathspec, so that it doesn't process more \nentries than are needed and end up slowing things down instead of \nspeeding them up.\n\nBecause the files checked are restricted to pathspec, adding individual\nfiles makes no measurable impact but on a Windows repo with ~200K files,\n'git add .' drops from 6.3 seconds to 3.3 seconds for a 47% savings.\n\n\n\n>> @@ -459,6 +454,10 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n>>   \t\t       PATHSPEC_SYMLINK_LEADING_PATH,\n>>   \t\t       prefix, argv);\n>>   \n>> +\tif (read_cache_preload(&pathspec) < 0)\n>> +\t\tdie(_(\"index file corrupt\"));\n>> +\n>> +\tdie_in_unpopulated_submodule(&the_index, prefix);\n>>   \tdie_path_inside_submodule(&the_index, &pathspec);\n>>   \n>>   \tif (add_new_files) {\n>>\n>> base-commit: 4ede3d42dfb57f9a41ac96a1f216c62eb7566cc2\n"},{"id":"362324","messageId":"xmqqmuqrngfu.fsf@gitster-ct.c.googlers.com","threadId":"49741","inReplyTo":"CACsJy8CVPSe8TWYMrK9MiRCaG36qyWfd42cEPo5844XWuTmqew@mail.gmail.com","subject":"Re: [PATCH v1] add: speed up cmd_add() by utilizing read_cache_preload()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-03T00:38:29Z","receivedAt":"2018-11-03T00:38:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> On Fri, Nov 2, 2018 at 2:32 PM Ben Peart <peartben@gmail.com> wrote:\n>>\n>> From: Ben Peart <benpeart@microsoft.com>\n>>\n>> During an \"add\", a call is made to run_diff_files() which calls\n>> check_remove() for each index-entry.  The preload_index() code distributes\n>> some of the costs across multiple threads.\n>\n> Instead of doing this site by site. How about we make read_cache()\n> always do multithread preload?\n\nI suspect that it would be a huge performance killer. \n\nMany codepaths do not even want to know if the working tree files\nhave been modified, even though they need to know what's in the\nindex.  Think \"git commit-tree\", \"git diff --cached\", etc.\n\n\n"},{"id":"362331","messageId":"CACsJy8AZ4kxrpttfsHOWKP=Xg3HaTLySy7sepC5691mGzfgO5g@mail.gmail.com","threadId":"49741","inReplyTo":"xmqqmuqrngfu.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v1] add: speed up cmd_add() by utilizing read_cache_preload()","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-03T04:47:33Z","receivedAt":"2018-11-03T04:48:01Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Nov 3, 2018 at 1:38 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Duy Nguyen <pclouds@gmail.com> writes:\n>\n> > On Fri, Nov 2, 2018 at 2:32 PM Ben Peart <peartben@gmail.com> wrote:\n> >>\n> >> From: Ben Peart <benpeart@microsoft.com>\n> >>\n> >> During an \"add\", a call is made to run_diff_files() which calls\n> >> check_remove() for each index-entry.  The preload_index() code distributes\n> >> some of the costs across multiple threads.\n> >\n> > Instead of doing this site by site. How about we make read_cache()\n> > always do multithread preload?\n>\n> I suspect that it would be a huge performance killer.\n>\n> Many codepaths do not even want to know if the working tree files\n> have been modified, even though they need to know what's in the\n> index.  Think \"git commit-tree\", \"git diff --cached\", etc.\n\nAh. I keep forgetting read_cache_preload is loading the index _and_\nrefreshing. I thought the two had some different semantics but failed\nto see it last time.\n-- \nDuy\n"}]}