{"thread":{"id":"51488","subject":"[PATCH] fetch: add \"--parallel\", which fetches all remotes in parallel","startedAt":"2019-07-17T02:00:05Z","lastAt":"2019-07-19T15:12:20Z","messageCount":4,"participants":["Palmer Dabbelt","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"379000","messageId":"20190717015903.4384-1-palmer@sifive.com","threadId":"51488","inReplyTo":null,"subject":"[PATCH] fetch: add \"--parallel\", which fetches all remotes in parallel","fromName":"Palmer Dabbelt","fromEmail":"palmer@sifive.com","sentAt":"2019-07-17T01:59:03Z","receivedAt":"2019-07-17T02:00:05Z","isPatch":true,"sender":{"key":"palmer@sifive.com","avatar":"https://gravatar.com/avatar/bf8be091de4e6d14300576518ac2a35e0871bd4948140539ce9ef6efade35c09?d=mp&s=160"},"body":"I have a handful of repositories with a bunch of remotes and this week\nI'm on an internet connection with a high bandwidth high latency\nconnection to those remotes. This results in \"fetch --all\" being\npainfully slow: 3-4 seconds per remote, all at almost zero link\nutilization (presumably doing an SSH handshake), for a total of a\nminute or so.\n\nThis patch fixes the issue in the simplest way I could come up with: it\nadds a \"--parallel\" argument to fetch that selects when to block on the\nfetch children. This results in every fetch child running in parallel,\nwhich provides per-remote parallelism. My \"fetch --all\" times go down\nfrom ~60 seconds to ~5 seconds, which is great!\n\nSigned-off-by: Palmer Dabbelt \n\n---\n\nThis is more of an RFC that an actual patch, for three reasons:\n\n* I'm not sure if it's safe to access the .git database from multiple\n processes at the same time. This is my first time messing around with\n git to this degree, so I figured it's better to just ask than to try\n to figure it out.\n* As I was writing the documentation I found \"fetch --jobs\". It seems\n like I should use that instead of adding a new argrument, but I wasn't\n sure. If I do this, I'd add some other config argument because\n submodule.jobs seems like a bad name for this.\n* This implementation is recursive and I'm a bit worried about stack\n usage. The stack-based implementation doesn't make nearly as much\n sense with --jobs, so I'd probably just do it right.\n\nIt's at the point where I've spent about as long writing the code as\nI'll save waiting on \"fetch --all\" over the rest of my life, and given\nthe unknows I thought it would be best to just send out the patch and\nsee if it has any legs.\n\nIf this seems reasonable then I'll try to find some time to finish it.\nSpecificially, I'd like to:\n\n* Use \"--jobs\" instead of \"--parallel\", to avoid the extra argument.\n* Write some sort of test case.\n---\n Documentation/fetch-options.txt | 3 +++\n builtin/fetch.c | 45 +++++++++++++++++++++++----------\n 2 files changed, 34 insertions(+), 14 deletions(-)\n\ndiff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\nindex 3c9b4f9e0951..99666f40e405 100644\n--- a/Documentation/fetch-options.txt\n+++ b/Documentation/fetch-options.txt\n@@ -92,6 +92,9 @@ ifndef::git-pull[]\n \tRun `git gc --auto` at the end to perform garbage collection\n \tif needed. This is enabled by default.\n \n+--parallel::\n+\tFetch all remotes in parallel.\n+\n -p::\n --prune::\n \tBefore fetching, remove any remote-tracking references that no\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 53ce99d2bbc4..2dfdeea3b3ec 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -71,6 +71,7 @@ static struct refspec refmap = REFSPEC_INIT_FETCH;\n static struct list_objects_filter_options filter_options;\n static struct string_list server_options = STRING_LIST_INIT_DUP;\n static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;\n+static int parallel = 0;\n \n static int git_fetch_config(const char *k, const char *v, void *cb)\n {\n@@ -178,6 +179,8 @@ static struct option builtin_fetch_options[] = {\n \t\t\tTRANSPORT_FAMILY_IPV6),\n \tOPT_STRING_LIST(0, \"negotiation-tip\", &negotiation_tip, N_(\"revision\"),\n \t\t\tN_(\"report that we have only objects reachable from this object\")),\n+\tOPT_BOOL(0, \"parallel\", ¶llel,\n+\t\t N_(\"fetch in parallel from each remote\")),\n \tOPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),\n \tOPT_BOOL(0, \"auto-gc\", &enable_auto_gc,\n \t\t N_(\"run 'gc --auto' after fetching\")),\n@@ -1456,12 +1459,15 @@ static void add_options_to_argv(struct argv_array *argv)\n \n }\n \n-static int fetch_multiple(struct string_list *list)\n+static int fetch_multiple(struct string_list *list, int i)\n {\n-\tint i, result = 0;\n+\tint result = 0;\n \tstruct argv_array argv = ARGV_ARRAY_INIT;\n+\tconst char *name = list->items[i].string;\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\tint code;\n \n-\tif (!append && !dry_run) {\n+\tif (i == 0 && !append && !dry_run) {\n \t\tint errcode = truncate_fetch_head();\n \t\tif (errcode)\n \t\t\treturn errcode;\n@@ -1469,20 +1475,31 @@ static int fetch_multiple(struct string_list *list)\n \n \targv_array_pushl(&argv, \"fetch\", \"--append\", \"--no-auto-gc\", NULL);\n \tadd_options_to_argv(&argv);\n+\targv_array_push(&argv, name);\n \n-\tfor (i = 0; i < list->nr; i++) {\n-\t\tconst char *name = list->items[i].string;\n-\t\targv_array_push(&argv, name);\n-\t\tif (verbosity >= 0)\n-\t\t\tprintf(_(\"Fetching %s\\n\"), name);\n-\t\tif (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {\n-\t\t\terror(_(\"Could not fetch %s\"), name);\n-\t\t\tresult = 1;\n-\t\t}\n-\t\targv_array_pop(&argv);\n+\tif (verbosity >= 0)\n+\t\tprintf(_(\"Fetching %s\\n\"), name);\n+\n+\tcmd.argv = argv.argv;\n+\tcmd.git_cmd = 1;\n+\tcode = start_command(&cmd);\n+\n+\tif (!code && !parallel)\n+\t\tcode = finish_command(&cmd);\n+\n+\tif (i+1 < list->nr)\n+\t\tresult |= fetch_multiple(list, i+1);\n+\n+\tif (!code && parallel)\n+\t\tcode |= finish_command(&cmd);\n+\n+\tif (code) {\n+\t\terror(_(\"Could not fetch %s\"), name);\n+\t\tresult |= 1;\n \t}\n \n \targv_array_clear(&argv);\n+\n \treturn result;\n }\n \n@@ -1696,7 +1713,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"--filter can only be used with the remote \"\n \t\t\t \"configured in extensions.partialclone\"));\n \t\t/* TODO should this also die if we have a previous partial-clone? */\n-\t\tresult = fetch_multiple(&list);\n+\t\tresult = fetch_multiple(&list, 0);\n \t}\n \n \tif (!result && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {\n-- \n2.21.0\n\n"},{"id":"379065","messageId":"xmqq5znzz17j.fsf@gitster-ct.c.googlers.com","threadId":"51488","inReplyTo":"20190717015903.4384-1-palmer@sifive.com","subject":"Re: [PATCH] fetch: add \"--parallel\", which fetches all remotes in parallel","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-18T17:45:04Z","receivedAt":"2019-07-18T17:45:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Palmer Dabbelt writes:\n\n> * I'm not sure if it's safe to access the .git database from multiple\n> processes at the same time.\n\nIt is supposed to, and also you are supposed to keep it that way ;-)\n\n> * As I was writing the documentation I found \"fetch --jobs\". It seems\n> like I should use that instead of adding a new argrument, but I wasn't\n> sure.\n\nWhy not? What makes you feel it is a bad idea to follow that\npattern?\n\nAh, --jobs that is taken already is right now too tied to fetches\nthat happen in submodules, which arguably was a design mistake.\n\n -j::\n --jobs=::\n Number of parallel children to be used for fetching\n submodules. Each will fetch from different submodules,\n such that fetching many submodules will be faster. By\n default submodules will be fetched one at a time.\n\nThe simplest endgame would be to replace \"submodule\" with\n\"repository\" in the above description, perhaps like\n\n\tNumber of parallel jobs to be used for fetching from\n\tmultiple repositories (both fetching with \"--multiple\" from\n\tmultiple repositories, and also fetching updated contents\n\tfor submodules). By default, fetching from multiple\n\trepositories and submodules is done one at a time.\n\nand nobody would have complained if the system were like so from the\nbeginning. Existing users, however, may want extra flexibility, and\nwould complain loudly if we did the above, in which case, we may\nhave to\n\n - introduce --fetch-jobs= for what you are adding;\n\n - introduce --submodule-fetch-jobs= as a synonym for existing\n --jobs= and deprecate the current use of --jobs=;\n\n - eventually repurpose --jobs= as a short-hand to give both\n --fetch-jobs and --submoduje-fetch-jobs at the same time.\n\n> +static int parallel = 0;\n\nDon't explicitly \"= 0;\" initialize file-scope static. Instead let\nBSS take care of it.\n\n> static int git_fetch_config(const char *k, const char *v, void *cb)\n> {\n> @@ -178,6 +179,8 @@ static struct option builtin_fetch_options[] = {\n> \t\t\tTRANSPORT_FAMILY_IPV6),\n> \tOPT_STRING_LIST(0, \"negotiation-tip\", &negotiation_tip, N_(\"revision\"),\n> \t\t\tN_(\"report that we have only objects reachable from this object\")),\n> +\tOPT_BOOL(0, \"parallel\", ¶llel,\n> +\t\t N_(\"fetch in parallel from each remote\")),\n> \tOPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),\n> \tOPT_BOOL(0, \"auto-gc\", &enable_auto_gc,\n> \t\t N_(\"run 'gc --auto' after fetching\")),\n> @@ -1456,12 +1459,15 @@ static void add_options_to_argv(struct argv_array *argv)\n> \n> }\n> \n> -static int fetch_multiple(struct string_list *list)\n> +static int fetch_multiple(struct string_list *list, int i)\n> {\n> -\tint i, result = 0;\n\n'i' is perfectly a good name for a local variable that is used for\nloop control purposes, but makes a horrible name for a parameter.\n\nExisting 'list' is not any better either---we know it is a list by\nits type already, the name should say what the list is about, what\nit represents. But having a horribly named parameter already is not\na good reason to make the code even worse.\n\nAnd as you said, recursion makes the code structure harder to follow\nhere. Keeping an array of --jobs= cmd structures, looping to\nfill them by starting, doing wait() to reap any of the started ones\nthat first exits to refill the slot just opened, etc. would be easier\nto see if done in a loop, I think.\n"},{"id":"379097","messageId":"nycvar.QRO.7.76.6.1907191507420.47@tvgsbejvaqbjf.bet","threadId":"51488","inReplyTo":"xmqq5znzz17j.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] fetch: add \"--parallel\", which fetches all remotes in parallel","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-07-19T13:23:29Z","receivedAt":"2019-07-19T13:24:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 18 Jul 2019, Junio C Hamano wrote:\n\n> Palmer Dabbelt writes:\n>\n> > * As I was writing the documentation I found \"fetch --jobs\". It seems\n> > like I should use that instead of adding a new argrument, but I wasn't\n> > sure.\n>\n> Why not? What makes you feel it is a bad idea to follow that\n> pattern?\n>\n> Ah, --jobs that is taken already is right now too tied to fetches\n> that happen in submodules, which arguably was a design mistake.\n>\n> -j::\n> --jobs=::\n> Number of parallel children to be used for fetching\n> submodules. Each will fetch from different submodules,\n> such that fetching many submodules will be faster. By\n> default submodules will be fetched one at a time.\n>\n> The simplest endgame would be to replace \"submodule\" with\n> \"repository\" in the above description, perhaps like\n>\n> \tNumber of parallel jobs to be used for fetching from\n> \tmultiple repositories (both fetching with \"--multiple\" from\n> \tmultiple repositories, and also fetching updated contents\n> \tfor submodules). By default, fetching from multiple\n> \trepositories and submodules is done one at a time.\n>\n> and nobody would have complained if the system were like so from the\n> beginning. Existing users, however, may want extra flexibility, and\n> would complain loudly if we did the above, in which case, we may\n> have to\n>\n> - introduce --fetch-jobs= for what you are adding;\n>\n> - introduce --submodule-fetch-jobs= as a synonym for existing\n> --jobs= and deprecate the current use of --jobs=;\n>\n> - eventually repurpose --jobs= as a short-hand to give both\n> --fetch-jobs and --submoduje-fetch-jobs at the same time.\n\nGiven that the relevant code looks like this:\n\n if (remote) {\n if (filter_options.choice || repository_format_partial_clone)\n fetch_one_setup_partial(remote);\n result = fetch_one(remote, argc, argv, prune_tags_ok);\n } else {\n if (filter_options.choice)\n die(_(\"--filter can only be used with the remote \"\n \"configured in extensions.partialclone\"));\n /* TODO should this also die if we have a previous partial-clone? */\n result = fetch_multiple(&list);\n }\n\n if (!result && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {\n struct argv_array options = ARGV_ARRAY_INIT;\n\n add_options_to_argv(&options);\n result = fetch_populated_submodules(the_repository,\n &options,\n submodule_prefix,\n recurse_submodules,\n recurse_submodules_default,\n verbosity < 0,\n max_children);\n argv_array_clear(&options);\n }\n\ni.e. the `fetch_multiple()` call is _strictly_ before the call to\n`fetch_populated_submodules()`, I would contend that this level of\nseparation does not serve anybody but a fan of the complicators' gloves.\n\nYou would also find yourself in quite a pickle if you wanted to explain\nto a user why those `--fetch-jobs` and `--submodule-fetch-jobs` are\nseparate options _and_ why they repeat the command name in the option\nname. And if it is hard to explain, there is usually a better design\nchoice in the first place.\n\nIn other words, I would be much more in favor of `--jobs` _also_\nextending to the `fetch_multiple()` call, not just the\n`fetch_populated_submodules()` call.\n\n> > @@ -1456,12 +1459,15 @@ static void add_options_to_argv(struct argv_array *argv)\n> >\n> > }\n> >\n> > -static int fetch_multiple(struct string_list *list)\n> > +static int fetch_multiple(struct string_list *list, int i)\n> > {\n> > -\tint i, result = 0;\n>\n> 'i' is perfectly a good name for a local variable that is used for\n> loop control purposes, but makes a horrible name for a parameter.\n>\n> Existing 'list' is not any better either---we know it is a list by\n> its type already, the name should say what the list is about, what\n> it represents. But having a horribly named parameter already is not\n> a good reason to make the code even worse.\n>\n> And as you said, recursion makes the code structure harder to follow\n> here. Keeping an array of --jobs= cmd structures, looping to\n> fill them by starting, doing wait() to reap any of the started ones\n> that first exits to refill the slot just opened, etc. would be easier\n> to see if done in a loop, I think.\n\nI have to admit that I'd _much_ rather see the strategy of\n`fetch_populated_submodules()` emulated, where it uses the\n`run_processes_parallel_tr2()` function to perform the actual fetches.\n\nThat buys us a lot of advantages:\n\n- the recursion problems mentioned in the original mail will _not_ be\n any issue,\n- the progress won't be garbled,\n- we can automatically restrict the maximal number of parallel fetches.\n\nIt should be super easy to get started with this, simply by moving the\n`list` and the `i` variables into a `struct` that is then passed to\n`run_processes_parallel_tr2()`, then implementing the three callbacks.\n\nThose callbacks can take inspiration from `submodule.c`'s\n`get_next_submodule()`, `fetch_start_failure()`, and `fetch_finish()`\nfunctions, but rest assured that the `fetch_multiple()` ones will look\n_a lot_ simpler.\n\nCiao,\nDscho\n\nP.S.: It would probably also make sense to extend this contribution by a\nsecond patch that teaches `git remote update` a `--jobs` option.\n"},{"id":"379103","messageId":"xmqqh87ivz1s.fsf@gitster-ct.c.googlers.com","threadId":"51488","inReplyTo":"nycvar.QRO.7.76.6.1907191507420.47@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] fetch: add \"--parallel\", which fetches all remotes in parallel","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-19T15:12:15Z","receivedAt":"2019-07-19T15:12:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> I have to admit that I'd _much_ rather see the strategy of\n> `fetch_populated_submodules()` emulated, where it uses the\n> `run_processes_parallel_tr2()` function to perform the actual fetches.\n\nGood. Thanks for chiming in.\n"}]}