{"thread":{"id":"58499","subject":"[PATCH 0/4] submodule: parallelize status","startedAt":"2022-09-22T23:30:16Z","lastAt":"2022-09-29T20:52:26Z","messageCount":32,"participants":["Calvin Wan","Ævar Arnfjörð Bjarmason","Junio C Hamano","Phillip Wood","Emily Shaffer"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"463485","messageId":"20220922232947.631309-1-calvinwan@google.com","threadId":"58499","inReplyTo":null,"subject":"[PATCH 0/4] submodule: parallelize status","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-09-22T23:29:43Z","receivedAt":"2022-09-22T23:30:16Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"When running 'git status' in a superproject, git spawns a subprocess in\nseries to run status for every submodule. For projects with many large\nsubmodules, parallelizing status subprocesses can significantly speed\nup the runtime for both cold and warm caches.\n\nHere are some timing tests from running status on the Android Open\nSource Project (AOSP). My machine has an SSD and 48 cores. \n Warm Cache:\n git 2.37\nTime (mean ± σ): 17.685 s ± 2.040 s [User: 5.041 s, System: 22.799 s]\nRange (min … max): 16.168 s … 22.804 s 10 runs\n\n this patch (status.parallelSubmodules=1)\nTime (mean ± σ): 13.102 s ± 0.500 s [User: 4.894 s, System: 19.533 s]\nRange (min … max): 12.841 s … 14.447 s 10 runs\n\n this patch (status.parallelSubmodules=5)\nTime (mean ± σ): 3.994 s ± 0.152 s [User: 4.998 s, System: 20.805 s]\nRange (min … max): 3.744 s … 4.163 s 10 runs\n\n this patch (status.parallelSubmodules=10)\nTime (mean ± σ): 3.445 s ± 0.085 s [User: 5.151 s, System: 20.208 s]\nRange (min … max): 3.319 s … 3.586 s 10 runs\n\n this patch (status.parallelSubmodules=20)\nTime (mean ± σ): 3.626 s ± 0.109 s [User: 5.087 s, System: 20.366 s]\nRange (min … max): 3.438 s … 3.763 s 10 runs\n\nWe can see that there are diminishing returns and even slightly worse\nperformance after a certain number of max processes, but optimally\nthere is a speed up factor of around 5.\n\n Cold Cache:\n git 2.37\n mean of 3 runs: 6m32s\n this patch (status.parallelSubmodules=1)\n mean of 3 runs: 5m34s\n this patch (status.parallelSubmodules=5)\n mean of 3 runs: 2m23s\n this patch (status.parallelSubmodules=10)\n mean of 3 runs: 2m45s\n this patch (status.parallelSubmodules=20)\n mean of 3 runs: 3m23s\n\nWe can witness the same phenomenon as above and optimally there is a\nspeed up factor of around 2.7.\n\nPatch 1 adds output piping to run_processes_parallel so the output\nfrom each submodule can be parsed. Patches 2 and 3 move preexisting\nfunctionality into separate functions and refactor code to prepare\nfor patch 4 to implement parallelization.\n\nFuture work: The reason why status is much slower on a cold cache vs\nwarm cache is mainly due to refreshing the index. It is worth\ninvestigating whether this is entirely necessary.\n\nCalvin Wan (4):\n run-command: add pipe_output to run_processes_parallel\n submodule: move status parsing into function\n diff-lib: refactor functions\n diff-lib: parallelize run_diff_files for submodules\n\n Documentation/config/status.txt | 6 +\n add-interactive.c | 2 +-\n builtin/add.c | 4 +-\n builtin/commit.c | 6 +\n builtin/diff-files.c | 2 +-\n builtin/diff.c | 2 +-\n builtin/merge.c | 2 +-\n builtin/stash.c | 2 +-\n builtin/submodule--helper.c | 4 +-\n diff-lib.c | 120 +++++++++++++-----\n diff.h | 2 +-\n run-command.c | 6 +-\n run-command.h | 9 ++\n submodule.c | 213 +++++++++++++++++++++++++++-----\n submodule.h | 9 ++\n t/helper/test-run-command.c | 31 ++++-\n t/t0061-run-command.sh | 26 ++++\n wt-status.c | 6 +-\n wt-status.h | 1 +\n 19 files changed, 372 insertions(+), 81 deletions(-)\n\n\nbase-commit: 5502f77b6944eda8e26813d8f542cffe7d110aea\n-- \n2.37.3.998.g577e59143f-goog\n\n"},{"id":"463486","messageId":"20220922232947.631309-2-calvinwan@google.com","threadId":"58499","inReplyTo":"20220922232947.631309-1-calvinwan@google.com","subject":"[PATCH 1/4] run-command: add pipe_output to run_processes_parallel","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-09-22T23:29:44Z","receivedAt":"2022-09-22T23:30:18Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"run_processes_parallel periodically collects output from its child\nprocesses, prints it, and then resets the buffers for each child.\nAdd run_processes_parallel_pipe_output variable so output can be\ncollected and fed to task_finished. When set, the function referenced\nby task_finished should parse the output of each child process.\n\nSigned-off-by: Calvin Wan \n---\n run-command.c | 6 +++++-\n run-command.h | 9 +++++++++\n t/helper/test-run-command.c | 31 ++++++++++++++++++++++++++++---\n t/t0061-run-command.sh | 26 ++++++++++++++++++++++++++\n 4 files changed, 68 insertions(+), 4 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 14f17830f5..893bc1d294 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1474,6 +1474,7 @@ enum child_state {\n };\n \n int run_processes_parallel_ungroup;\n+int run_processes_parallel_pipe_output;\n struct parallel_processes {\n \tvoid *data;\n \n@@ -1770,10 +1771,12 @@ int run_processes_parallel(int n,\n \tint output_timeout = 100;\n \tint spawn_cap = 4;\n \tint ungroup = run_processes_parallel_ungroup;\n+\tint pipe_output = run_processes_parallel_pipe_output;\n \tstruct parallel_processes pp;\n \n \t/* unset for the next API user */\n \trun_processes_parallel_ungroup = 0;\n+\trun_processes_parallel_pipe_output = 0;\n \n \tpp_init(&pp, n, get_next_task, start_failure, task_finished, pp_cb,\n \t\tungroup);\n@@ -1800,7 +1803,8 @@ int run_processes_parallel(int n,\n \t\t\t\tpp.children[i].state = GIT_CP_WAIT_CLEANUP;\n \t\t} else {\n \t\t\tpp_buffer_stderr(&pp, output_timeout);\n-\t\t\tpp_output(&pp);\n+\t\t\tif (!pipe_output)\n+\t\t\t\tpp_output(&pp);\n \t\t}\n \t\tcode = pp_collect_finished(&pp);\n \t\tif (code) {\ndiff --git a/run-command.h b/run-command.h\nindex 0e85e5846a..a5b1d63f49 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -483,8 +483,17 @@ typedef int (*task_finished_fn)(int result,\n * \"run_processes_parallel_ungroup\" to \"1\" before invoking\n * run_processes_parallel(), it will be set back to \"0\" as soon as the\n * API reads that setting.\n+ * \n+ * If the \"pipe_output\" option is specified, the output will be piped\n+ * to task_finished_fn in the \"struct strbuf *out\" variable. The output\n+ * will still be printed unless the callback resets the strbuf. The\n+ * \"pipe_output\" option can be enabled by setting the global\n+ * \"run_processes_parallel_pipe_output\" to \"1\" before invoking\n+ * run_processes_parallel(), it will be set back to \"0\" as soon as the\n+ * API reads that setting.\n */\n extern int run_processes_parallel_ungroup;\n+extern int run_processes_parallel_pipe_output;\n int run_processes_parallel(int n,\n \t\t\t get_next_task_fn,\n \t\t\t start_failure_fn,\ndiff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c\nindex c9283b47af..030e533c6b 100644\n--- a/t/helper/test-run-command.c\n+++ b/t/helper/test-run-command.c\n@@ -20,6 +20,7 @@\n #include \"wildmatch.h\"\n #include \"gettext.h\"\n \n+static int pipe_output = 0;\n static int number_callbacks;\n static int parallel_next(struct child_process *cp,\n \t\t\t struct strbuf *err,\n@@ -52,15 +53,32 @@ static int no_job(struct child_process *cp,\n \treturn 0;\n }\n \n+static int task_finished_pipe_output(int result,\n+\t\t\t struct strbuf *err,\n+\t\t\t void *pp_cb,\n+\t\t\t void *pp_task_cb)\n+{\n+\tif (err && pipe_output) {\n+\t\tfprintf(stderr, \"%s\", err->buf);\n+\t\tstrbuf_reset(err);\n+\t}\n+\treturn 0;\n+}\n+\n static int task_finished(int result,\n \t\t\t struct strbuf *err,\n \t\t\t void *pp_cb,\n \t\t\t void *pp_task_cb)\n {\n-\tif (err)\n+\tif (err) {\n \t\tstrbuf_addstr(err, \"asking for a quick stop\\n\");\n-\telse\n+\t\tif (pipe_output) {\n+\t\t\tfprintf(stderr, \"%s\", err->buf);\n+\t\t\tstrbuf_reset(err);\n+\t\t}\n+\t} else {\n \t\tfprintf(stderr, \"asking for a quick stop\\n\");\n+\t}\n \treturn 1;\n }\n \n@@ -423,13 +441,20 @@ int cmd__run_command(int argc, const char **argv)\n \t\trun_processes_parallel_ungroup = 1;\n \t}\n \n+\tif (!strcmp(argv[1], \"--pipe-output\")) {\n+\t\targv += 1;\n+\t\targc -= 1;\n+\t\trun_processes_parallel_pipe_output = 1;\n+\t\tpipe_output = 1;\n+\t}\n+\n \tjobs = atoi(argv[2]);\n \tstrvec_clear(&proc.args);\n \tstrvec_pushv(&proc.args, (const char **)argv + 3);\n \n \tif (!strcmp(argv[1], \"run-command-parallel\"))\n \t\texit(run_processes_parallel(jobs, parallel_next,\n-\t\t\t\t\t NULL, NULL, &proc));\n+\t\t\t\t\t NULL, task_finished_pipe_output, &proc));\n \n \tif (!strcmp(argv[1], \"run-command-abort\"))\n \t\texit(run_processes_parallel(jobs, parallel_next,\ndiff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\nindex 7b5423eebd..97ca942a74 100755\n--- a/t/t0061-run-command.sh\n+++ b/t/t0061-run-command.sh\n@@ -140,6 +140,11 @@ test_expect_success 'run_command runs ungrouped in parallel with more jobs avail\n \ttest_line_count = 4 err\n '\n \n+test_expect_success 'run_command runs pipe_output in parallel with more jobs available than tasks' '\n+\ttest-tool run-command --pipe-output run-command-parallel 5 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'run_command runs in parallel with as many jobs as tasks' '\n \ttest-tool run-command run-command-parallel 4 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" 2>actual &&\n \ttest_cmp expect actual\n@@ -151,6 +156,11 @@ test_expect_success 'run_command runs ungrouped in parallel with as many jobs as\n \ttest_line_count = 4 err\n '\n \n+test_expect_success 'run_command runs pipe_output in parallel with as many jobs as tasks' '\n+\ttest-tool run-command --pipe-output run-command-parallel 4 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'run_command runs in parallel with more tasks than jobs available' '\n \ttest-tool run-command run-command-parallel 3 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" 2>actual &&\n \ttest_cmp expect actual\n@@ -162,6 +172,12 @@ test_expect_success 'run_command runs ungrouped in parallel with more tasks than\n \ttest_line_count = 4 err\n '\n \n+test_expect_success 'run_command runs pipe_output in parallel with more tasks than jobs available' '\n+\ttest-tool run-command --pipe-output run-command-parallel 3 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+\n cat >expect <<-EOF\n preloaded output of a child\n asking for a quick stop\n@@ -182,6 +198,11 @@ test_expect_success 'run_command is asked to abort gracefully (ungroup)' '\n \ttest_line_count = 6 err\n '\n \n+test_expect_success 'run_command is asked to abort gracefully (pipe_output)' '\n+\ttest-tool run-command --pipe-output run-command-abort 3 false 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n cat >expect <<-EOF\n no further jobs available\n EOF\n@@ -197,6 +218,11 @@ test_expect_success 'run_command outputs (ungroup) ' '\n \ttest_cmp expect err\n '\n \n+test_expect_success 'run_command outputs (pipe_output) ' '\n+\ttest-tool run-command --pipe-output run-command-no-jobs 3 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_trace () {\n \texpect=\"$1\"\n \tshift\n-- \n2.37.3.998.g577e59143f-goog\n\n"},{"id":"463487","messageId":"20220922232947.631309-3-calvinwan@google.com","threadId":"58499","inReplyTo":"20220922232947.631309-1-calvinwan@google.com","subject":"[PATCH 2/4] submodule: move status parsing into function","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-09-22T23:29:45Z","receivedAt":"2022-09-22T23:30:20Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"A future patch requires the ability to parse the output of git\nstatus --porcelain=2. Move parsing code from is_submodule_modified to\nparse_status_porcelain.\n\nSigned-off-by: Calvin Wan \n---\n submodule.c | 71 +++++++++++++++++++++++++++++------------------------\n 1 file changed, 39 insertions(+), 32 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 3fa5db3ecd..91213ba83c 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1753,6 +1753,43 @@ static int commit_missing_in_sub(const struct object_id *oid, void *data)\n \treturn type != OBJ_COMMIT;\n }\n \n+static int parse_status_porcelain(char *buf, unsigned *dirty_submodule, int ignore_untracked)\n+{\n+\t/* regular untracked files */\n+\tif (buf[0] == '?')\n+\t\t*dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n+\n+\tif (buf[0] == 'u' ||\n+\t\tbuf[0] == '1' ||\n+\t\tbuf[0] == '2') {\n+\t\t/* T = line type, XY = status, SSSS = submodule state */\n+\t\tif (strlen(buf) < strlen(\"T XY SSSS\"))\n+\t\t\tBUG(\"invalid status --porcelain=2 line %s\",\n+\t\t\t\tbuf);\n+\n+\t\tif (buf[5] == 'S' && buf[8] == 'U')\n+\t\t\t/* nested untracked file */\n+\t\t\t*dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n+\n+\t\tif (buf[0] == 'u' ||\n+\t\t\tbuf[0] == '2' ||\n+\t\t\tmemcmp(buf + 5, \"S..U\", 4))\n+\t\t\t/* other change */\n+\t\t\t*dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n+\t}\n+\n+\tif ((*dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&\n+\t\t((*dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) ||\n+\t\tignore_untracked)) {\n+\t\t/*\n+\t\t* We're not interested in any further information from\n+\t\t* the child any more, neither output nor its exit code.\n+\t\t*/\n+\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n static int fetch_finish(int retvalue, struct strbuf *err,\n \t\t\tvoid *cb, void *task_cb)\n {\n@@ -1893,39 +1930,9 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \n \tfp = xfdopen(cp.out, \"r\");\n \twhile (strbuf_getwholeline(&buf, fp, '\\n') != EOF) {\n-\t\t/* regular untracked files */\n-\t\tif (buf.buf[0] == '?')\n-\t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n-\n-\t\tif (buf.buf[0] == 'u' ||\n-\t\t buf.buf[0] == '1' ||\n-\t\t buf.buf[0] == '2') {\n-\t\t\t/* T = line type, XY = status, SSSS = submodule state */\n-\t\t\tif (buf.len < strlen(\"T XY SSSS\"))\n-\t\t\t\tBUG(\"invalid status --porcelain=2 line %s\",\n-\t\t\t\t buf.buf);\n-\n-\t\t\tif (buf.buf[5] == 'S' && buf.buf[8] == 'U')\n-\t\t\t\t/* nested untracked file */\n-\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n-\n-\t\t\tif (buf.buf[0] == 'u' ||\n-\t\t\t buf.buf[0] == '2' ||\n-\t\t\t memcmp(buf.buf + 5, \"S..U\", 4))\n-\t\t\t\t/* other change */\n-\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n-\t\t}\n-\n-\t\tif ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&\n-\t\t ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) ||\n-\t\t ignore_untracked)) {\n-\t\t\t/*\n-\t\t\t * We're not interested in any further information from\n-\t\t\t * the child any more, neither output nor its exit code.\n-\t\t\t */\n-\t\t\tignore_cp_exit_code = 1;\n+\t\tignore_cp_exit_code = parse_status_porcelain(buf.buf, &dirty_submodule, ignore_untracked);\n+\t\tif (ignore_cp_exit_code)\n \t\t\tbreak;\n-\t\t}\n \t}\n \tfclose(fp);\n \n-- \n2.37.3.998.g577e59143f-goog\n\n"},{"id":"463488","messageId":"20220922232947.631309-4-calvinwan@google.com","threadId":"58499","inReplyTo":"20220922232947.631309-1-calvinwan@google.com","subject":"[PATCH 3/4] diff-lib: refactor functions","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-09-22T23:29:46Z","receivedAt":"2022-09-22T23:30:23Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"Flatten out the if statements in match_stat_with_submodule so the\nlogic is more readable and easier for future patches to add to.\n\nMove code that updates relevant variables from the end of\nrun_diff_files to finish_run_diff_files. A future patch will utilize\nsaid function.\n\nSigned-off-by: Calvin Wan \n---\n diff-lib.c | 71 ++++++++++++++++++++++++++++++++----------------------\n 1 file changed, 42 insertions(+), 29 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 7eb66a417a..2e148b79e6 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -73,21 +73,50 @@ static int match_stat_with_submodule(struct diff_options *diffopt,\n \t\t\t\t unsigned *dirty_submodule)\n {\n \tint changed = ie_match_stat(diffopt->repo->index, ce, st, ce_option);\n-\tif (S_ISGITLINK(ce->ce_mode)) {\n-\t\tstruct diff_flags orig_flags = diffopt->flags;\n-\t\tif (!diffopt->flags.override_submodule_config)\n-\t\t\tset_diffopt_flags_from_submodule_config(diffopt, ce->name);\n-\t\tif (diffopt->flags.ignore_submodules)\n-\t\t\tchanged = 0;\n-\t\telse if (!diffopt->flags.ignore_dirty_submodules &&\n-\t\t\t (!changed || diffopt->flags.dirty_submodules))\n-\t\t\t*dirty_submodule = is_submodule_modified(ce->name,\n-\t\t\t\t\t\t\t\t diffopt->flags.ignore_untracked_in_submodules);\n-\t\tdiffopt->flags = orig_flags;\n+\tstruct diff_flags orig_flags = diffopt->flags;\n+\tif (!S_ISGITLINK(ce->ce_mode))\n+\t\tgoto cleanup;\n+\tif (!diffopt->flags.override_submodule_config)\n+\t\tset_diffopt_flags_from_submodule_config(diffopt, ce->name);\n+\tif (diffopt->flags.ignore_submodules) {\n+\t\tchanged = 0;\n+\t\tgoto cleanup;\n \t}\n+\tif (!diffopt->flags.ignore_dirty_submodules &&\n+\t\t(!changed || diffopt->flags.dirty_submodules))\n+\t\t\t*dirty_submodule = is_submodule_modified(ce->name,\n+\t\t\t\t\t\t\tdiffopt->flags.ignore_untracked_in_submodules);\n+cleanup:\n+\tdiffopt->flags = orig_flags;\n \treturn changed;\n }\n \n+static void finish_run_diff_files(struct rev_info *revs,\n+\t\t\t\t\t\t struct cache_entry *ce,\n+\t\t\t\t\t\t struct index_state *istate,\n+\t\t\t\t\t\t int changed, int dirty_submodule,\n+\t\t\t\t\t\t unsigned int newmode)\n+{\n+\tunsigned int oldmode;\n+\tconst struct object_id *old_oid, *new_oid;\n+\n+\tif (!changed && !dirty_submodule) {\n+\t\t\tce_mark_uptodate(ce);\n+\t\t\tif (!S_ISGITLINK(ce->ce_mode))\n+\t\t\t\tmark_fsmonitor_valid(istate, ce);\n+\t\t\tif (!revs->diffopt.flags.find_copies_harder)\n+\t\t\t\treturn;\n+\t\t}\n+\t\toldmode = ce->ce_mode;\n+\t\told_oid = &ce->oid;\n+\t\tnew_oid = changed ? null_oid() : &ce->oid;\n+\t\tdiff_change(&revs->diffopt, oldmode, newmode,\n+\t\t\t old_oid, new_oid,\n+\t\t\t !is_null_oid(old_oid),\n+\t\t\t !is_null_oid(new_oid),\n+\t\t\t ce->name, 0, dirty_submodule);\n+}\n+\n int run_diff_files(struct rev_info *revs, unsigned int option)\n {\n \tint entries, i;\n@@ -105,11 +134,10 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\tdiff_unmerged_stage = 2;\n \tentries = istate->cache_nr;\n \tfor (i = 0; i < entries; i++) {\n-\t\tunsigned int oldmode, newmode;\n+\t\tunsigned int newmode;\n \t\tstruct cache_entry *ce = istate->cache[i];\n \t\tint changed;\n \t\tunsigned dirty_submodule = 0;\n-\t\tconst struct object_id *old_oid, *new_oid;\n \n \t\tif (diff_can_quit_early(&revs->diffopt))\n \t\t\tbreak;\n@@ -244,22 +272,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\t\t\t\t\t ce_option, &dirty_submodule);\n \t\t\tnewmode = ce_mode_from_stat(ce, st.st_mode);\n \t\t}\n-\n-\t\tif (!changed && !dirty_submodule) {\n-\t\t\tce_mark_uptodate(ce);\n-\t\t\tmark_fsmonitor_valid(istate, ce);\n-\t\t\tif (!revs->diffopt.flags.find_copies_harder)\n-\t\t\t\tcontinue;\n-\t\t}\n-\t\toldmode = ce->ce_mode;\n-\t\told_oid = &ce->oid;\n-\t\tnew_oid = changed ? null_oid() : &ce->oid;\n-\t\tdiff_change(&revs->diffopt, oldmode, newmode,\n-\t\t\t old_oid, new_oid,\n-\t\t\t !is_null_oid(old_oid),\n-\t\t\t !is_null_oid(new_oid),\n-\t\t\t ce->name, 0, dirty_submodule);\n-\n+\t\tfinish_run_diff_files(revs, ce, istate, changed, dirty_submodule, newmode);\n \t}\n \tdiffcore_std(&revs->diffopt);\n \tdiff_flush(&revs->diffopt);\n-- \n2.37.3.998.g577e59143f-goog\n\n"},{"id":"463489","messageId":"20220922232947.631309-5-calvinwan@google.com","threadId":"58499","inReplyTo":"20220922232947.631309-1-calvinwan@google.com","subject":"[PATCH 4/4] diff-lib: parallelize run_diff_files for submodules","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-09-22T23:29:47Z","receivedAt":"2022-09-22T23:30:25Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"During the iteration of the index entries in run_diff_files, whenever\na submodule is found and needs its status checked, a subprocess is\nspawned for it. Instead of spawning the subprocess immediately and\nwaiting for its completion to continue, hold onto all submodules and\nrelevant information in a list. Then use that list to create tasks for\nrun_processes_parallel. Finished subprocesses pipe their output to\nstatus_finish which parses it and sets the relevant variables.\n\nAdd config option status.parallelSubmodules to set the maximum number\nof parallel jobs. \n\nSigned-off-by: Calvin Wan \n---\n Documentation/config/status.txt | 6 ++\n add-interactive.c | 2 +-\n builtin/add.c | 4 +-\n builtin/commit.c | 6 ++\n builtin/diff-files.c | 2 +-\n builtin/diff.c | 2 +-\n builtin/merge.c | 2 +-\n builtin/stash.c | 2 +-\n builtin/submodule--helper.c | 4 +-\n diff-lib.c | 55 +++++++++++--\n diff.h | 2 +-\n submodule.c | 142 ++++++++++++++++++++++++++++++++\n submodule.h | 9 ++\n wt-status.c | 6 +-\n wt-status.h | 1 +\n 15 files changed, 226 insertions(+), 19 deletions(-)\n\ndiff --git a/Documentation/config/status.txt b/Documentation/config/status.txt\nindex 0fc704ab80..92c5511fec 100644\n--- a/Documentation/config/status.txt\n+++ b/Documentation/config/status.txt\n@@ -75,3 +75,9 @@ status.submoduleSummary::\n \tthe --ignore-submodules=dirty command-line option or the 'git\n \tsubmodule summary' command, which shows a similar output but does\n \tnot honor these settings.\n+\n+status.parallelSubmodules::\n+\tWhen linkgit:git-status[1] is run in a superproject with\n+\tsubmodules, a status subprocess is spawned for every submodule.\n+\tThis option specifies the number of submodule status subprocesses\n+\tto run in parallel. If unset, it defaults to 1.\ndiff --git a/add-interactive.c b/add-interactive.c\nindex 22fcd3412c..3e38aa833d 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -565,7 +565,7 @@ static int get_modified_files(struct repository *r,\n \t\t\trun_diff_index(&rev, 1);\n \t\telse {\n \t\t\trev.diffopt.flags.ignore_dirty_submodules = 1;\n-\t\t\trun_diff_files(&rev, 0);\n+\t\t\trun_diff_files(&rev, 0, -1);\n \t\t}\n \n \t\trelease_revisions(&rev);\ndiff --git a/builtin/add.c b/builtin/add.c\nindex f84372964c..094d59c1b4 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -148,7 +148,7 @@ int add_files_to_cache(const char *prefix,\n \t * may not have their own transaction active.\n \t */\n \tbegin_odb_transaction();\n-\trun_diff_files(&rev, DIFF_RACY_IS_MODIFIED);\n+\trun_diff_files(&rev, DIFF_RACY_IS_MODIFIED, -1);\n \tend_odb_transaction();\n \n \trelease_revisions(&rev);\n@@ -325,7 +325,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)\n \tout = xopen(file, O_CREAT | O_WRONLY | O_TRUNC, 0666);\n \trev.diffopt.file = xfdopen(out, \"w\");\n \trev.diffopt.close_file = 1;\n-\tif (run_diff_files(&rev, 0))\n+\tif (run_diff_files(&rev, 0, -1))\n \t\tdie(_(\"Could not write patch\"));\n \n \tif (launch_editor(file, NULL, NULL))\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex fcf9c85947..c5147a7952 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1468,6 +1468,12 @@ static int git_status_config(const char *k, const char *v, void *cb)\n \t\ts->detect_rename = git_config_rename(k, v);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(k, \"status.parallelsubmodules\")) {\n+\t\ts->parallel_jobs_submodules = git_config_int(k, v);\n+\t\tif (s->parallel_jobs_submodules < 0)\n+\t\t\tdie(_(\"status.parallelsubmodules cannot be negative\"));\n+\t\treturn 0;\n+\t}\n \treturn git_diff_ui_config(k, v, NULL);\n }\n \ndiff --git a/builtin/diff-files.c b/builtin/diff-files.c\nindex 92cf6e1e92..eb1b576440 100644\n--- a/builtin/diff-files.c\n+++ b/builtin/diff-files.c\n@@ -80,7 +80,7 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)\n \t\tresult = -1;\n \t\tgoto cleanup;\n \t}\n-\tresult = run_diff_files(&rev, options);\n+\tresult = run_diff_files(&rev, options, -1);\n \tresult = diff_result_code(&rev.diffopt, result);\n cleanup:\n \trelease_revisions(&rev);\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 54bb3de964..c3c532517b 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -275,7 +275,7 @@ static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv\n \t\tperror(\"read_cache_preload\");\n \t\treturn -1;\n \t}\n-\treturn run_diff_files(revs, options);\n+\treturn run_diff_files(revs, options, -1);\n }\n \n struct symdiff {\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex f7c92c0e64..ff373fa880 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1001,7 +1001,7 @@ static int evaluate_result(void)\n \t\tDIFF_FORMAT_CALLBACK;\n \trev.diffopt.format_callback = count_diff_files;\n \trev.diffopt.format_callback_data = &cnt;\n-\trun_diff_files(&rev, 0);\n+\trun_diff_files(&rev, 0, -1);\n \n \t/*\n \t * Check how many unmerged entries are\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 30fa101460..097ea55214 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -1075,7 +1075,7 @@ static int check_changes_tracked_files(const struct pathspec *ps)\n \t\tgoto done;\n \t}\n \n-\tresult = run_diff_files(&rev, 0);\n+\tresult = run_diff_files(&rev, 0, -1);\n \tif (diff_result_code(&rev.diffopt, result)) {\n \t\tret = 1;\n \t\tgoto done;\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex fac52ade5e..b50dbf5e36 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -679,7 +679,7 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,\n \tdiff_files_args.nr = setup_revisions(diff_files_args.nr,\n \t\t\t\t\t diff_files_args.v,\n \t\t\t\t\t &rev, NULL);\n-\tdiff_files_result = run_diff_files(&rev, 0);\n+\tdiff_files_result = run_diff_files(&rev, 0, -1);\n \n \tif (!diff_result_code(&rev.diffopt, diff_files_result)) {\n \t\tprint_status(flags, ' ', path, ce_oid,\n@@ -1143,7 +1143,7 @@ static int compute_summary_module_list(struct object_id *head_oid,\n \tif (diff_cmd == DIFF_INDEX)\n \t\trun_diff_index(&rev, info->cached);\n \telse\n-\t\trun_diff_files(&rev, 0);\n+\t\trun_diff_files(&rev, 0, -1);\n \tprepare_submodule_summary(info, &list);\n cleanup:\n \tstrvec_clear(&diff_args);\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 2e148b79e6..ec745755fc 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -65,14 +65,19 @@ static int check_removed(const struct index_state *istate, const struct cache_en\n * Return 1 when changes are detected, 0 otherwise. If the DIRTY_SUBMODULES\n * option is set, the caller does not only want to know if a submodule is\n * modified at all but wants to know all the conditions that are met (new\n- * commits, untracked content and/or modified content).\n+ * commits, untracked content and/or modified content). If\n+ * defer_submodule_status bit is set, dirty_submodule will be left to the\n+ * caller to set. defer_submodule_status can also be set to 0 in this\n+ * function if there is no need to check if the submodule is modified.\n */\n static int match_stat_with_submodule(struct diff_options *diffopt,\n \t\t\t\t const struct cache_entry *ce,\n \t\t\t\t struct stat *st, unsigned ce_option,\n-\t\t\t\t unsigned *dirty_submodule)\n+\t\t\t\t unsigned *dirty_submodule, int *defer_submodule_status,\n+\t\t\t\t\t int *ignore_untracked_in_submodules)\n {\n \tint changed = ie_match_stat(diffopt->repo->index, ce, st, ce_option);\n+\tint defer = 0;\n \tstruct diff_flags orig_flags = diffopt->flags;\n \tif (!S_ISGITLINK(ce->ce_mode))\n \t\tgoto cleanup;\n@@ -83,11 +88,20 @@ static int match_stat_with_submodule(struct diff_options *diffopt,\n \t\tgoto cleanup;\n \t}\n \tif (!diffopt->flags.ignore_dirty_submodules &&\n-\t\t(!changed || diffopt->flags.dirty_submodules))\n+\t\t(!changed || diffopt->flags.dirty_submodules)) {\n+\t\tif (defer_submodule_status && *defer_submodule_status) {\n+\t\t\tdefer = 1;\n+\t\t\t*ignore_untracked_in_submodules =\n+\t\t\t\t\t\t\tdiffopt->flags.ignore_untracked_in_submodules;\n+\t\t} else {\n \t\t\t*dirty_submodule = is_submodule_modified(ce->name,\n \t\t\t\t\t\t\tdiffopt->flags.ignore_untracked_in_submodules);\n+\t\t}\n+\t}\n cleanup:\n \tdiffopt->flags = orig_flags;\n+\tif (defer_submodule_status)\n+\t\t*defer_submodule_status = defer;\n \treturn changed;\n }\n \n@@ -117,7 +131,7 @@ static void finish_run_diff_files(struct rev_info *revs,\n \t\t\t ce->name, 0, dirty_submodule);\n }\n \n-int run_diff_files(struct rev_info *revs, unsigned int option)\n+int run_diff_files(struct rev_info *revs, unsigned int option, int parallel_jobs)\n {\n \tint entries, i;\n \tint diff_unmerged_stage = revs->max_count;\n@@ -125,6 +139,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\t ? CE_MATCH_RACY_IS_DIRTY : 0);\n \tuint64_t start = getnanotime();\n \tstruct index_state *istate = revs->diffopt.repo->index;\n+\tstruct string_list submodules = STRING_LIST_INIT_NODUP;\n \n \tdiff_set_mnemonic_prefix(&revs->diffopt, \"i/\", \"w/\");\n \n@@ -138,6 +153,9 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\tstruct cache_entry *ce = istate->cache[i];\n \t\tint changed;\n \t\tunsigned dirty_submodule = 0;\n+\t\tint defer_submodule_status = revs && revs->repo &&\n+\t\t\t\t\t\t\t!strcmp(revs->repo->gitdir, \".git\");\n+\t\tint ignore_untracked_in_submodules;\n \n \t\tif (diff_can_quit_early(&revs->diffopt))\n \t\t\tbreak;\n@@ -269,11 +287,36 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\t}\n \n \t\t\tchanged = match_stat_with_submodule(&revs->diffopt, ce, &st,\n-\t\t\t\t\t\t\t ce_option, &dirty_submodule);\n+\t\t\t\t\t\t\tce_option, &dirty_submodule,\n+\t\t\t\t\t\t\t&defer_submodule_status,\n+\t\t\t\t\t\t\t&ignore_untracked_in_submodules);\n \t\t\tnewmode = ce_mode_from_stat(ce, st.st_mode);\n+\t\t\tif (defer_submodule_status) {\n+\t\t\t\tstruct string_list_item *item =\n+\t\t\t\t\t\t\t\tstring_list_append(&submodules, ce->name);\n+\t\t\t\tstruct submodule_status_util *util = xmalloc(sizeof(*util));\n+\t\t\t\tutil->changed = changed;\n+\t\t\t\tutil->dirty_submodule = 0;\n+\t\t\t\tutil->ignore_untracked = ignore_untracked_in_submodules;\n+\t\t\t\tutil->newmode = newmode;\n+\t\t\t\tutil->ce = ce;\n+\t\t\t\titem->util = util;\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t}\n \t\tfinish_run_diff_files(revs, ce, istate, changed, dirty_submodule, newmode);\n \t}\n+\tif (submodules.nr > 0) {\n+\t\tif (get_submodules_status(revs->repo, &submodules,\n+\t\t\t\t\t\tparallel_jobs > 0 ? parallel_jobs : 1))\n+\t\t\tBUG(\"Submodule status failed\");\n+\t\tfor (int i = 0; i < submodules.nr; i++) {\n+\t\t\tstruct submodule_status_util *util = submodules.items[i].util;\n+\n+\t\t\tfinish_run_diff_files(revs, util->ce, NULL, util->changed,\n+\t\t\t\t\t\t\tutil->dirty_submodule, util->newmode);\n+\t\t}\n+\t}\n \tdiffcore_std(&revs->diffopt);\n \tdiff_flush(&revs->diffopt);\n \ttrace_performance_since(start, \"diff-files\");\n@@ -321,7 +364,7 @@ static int get_stat_data(const struct index_state *istate,\n \t\t\treturn -1;\n \t\t}\n \t\tchanged = match_stat_with_submodule(diffopt, ce, &st,\n-\t\t\t\t\t\t 0, dirty_submodule);\n+\t\t\t\t\t\t 0, dirty_submodule, NULL, NULL);\n \t\tif (changed) {\n \t\t\tmode = ce_mode_from_stat(ce, st.st_mode);\n \t\t\toid = null_oid();\ndiff --git a/diff.h b/diff.h\nindex 8ae18e5ab1..5a6a615381 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -627,7 +627,7 @@ void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb);\n #define DIFF_SILENT_ON_REMOVED 01\n /* report racily-clean paths as modified */\n #define DIFF_RACY_IS_MODIFIED 02\n-int run_diff_files(struct rev_info *revs, unsigned int option);\n+int run_diff_files(struct rev_info *revs, unsigned int option, int parallel_jobs);\n \n #define DIFF_INDEX_CACHED 01\n #define DIFF_INDEX_MERGE_BASE 02\ndiff --git a/submodule.c b/submodule.c\nindex 91213ba83c..15729bb327 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1362,6 +1362,20 @@ int submodule_touches_in_range(struct repository *r,\n \treturn ret;\n }\n \n+struct submodule_parallel_status {\n+\tint index_count;\n+\tint result;\n+\n+\tstruct string_list *submodule_names;\n+\tstruct repository *r;\n+\n+\t/* Pending statuses by OIDs */\n+\tstruct status_task **oid_status_tasks;\n+\tint oid_status_tasks_nr, oid_status_tasks_alloc;\n+};\n+\n+#define SPS_INIT { 0 }\n+\n struct submodule_parallel_fetch {\n \t/*\n \t * The index of the last index entry processed by\n@@ -1444,6 +1458,12 @@ struct fetch_task {\n \tstruct oid_array *commits; /* Ensure these commits are fetched */\n };\n \n+struct status_task {\n+\tstruct repository *repo;\n+\tconst char *path;\n+\tint ignore_untracked;\n+};\n+\n /**\n * When a submodule is not defined in .gitmodules, we cannot access it\n * via the regular submodule-config. Create a fake submodule, which we can\n@@ -1547,6 +1567,41 @@ static struct fetch_task *fetch_task_create(struct submodule_parallel_fetch *spf\n \treturn NULL;\n }\n \n+static struct status_task *\n+get_status_task_from_index(struct submodule_parallel_status *sps,\n+\t\t\t struct strbuf *err)\n+{\n+\tfor (; sps->index_count < sps->submodule_names->nr; sps->index_count++) {\n+\t\tstruct submodule_status_util *util = sps->submodule_names->items[sps->index_count].util;\n+\t\tconst struct cache_entry *ce = util->ce;\n+\t\tstruct status_task *task;\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\tconst char *git_dir;\n+\n+\t\tstrbuf_addf(&buf, \"%s/.git\", ce->name);\n+\t\tgit_dir = read_gitfile(buf.buf);\n+\t\tif (!git_dir)\n+\t\t\tgit_dir = buf.buf;\n+\t\tif (!is_git_directory(git_dir)) {\n+\t\t\tif (is_directory(git_dir))\n+\t\t\t\tdie(_(\"'%s' not recognized as a git repository\"), git_dir);\n+\t\t\tstrbuf_release(&buf);\n+\t\t\t/* The submodule is not checked out, so it is not modified */\n+\t\t\tutil->dirty_submodule = 0;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tstrbuf_release(&buf);\n+\n+\t\ttask = xmalloc(sizeof(*task));\n+\t\tmemset(task, 0, sizeof(*task));\n+\t\ttask->path = ce->name;\n+\t\ttask->ignore_untracked = util->ignore_untracked;\n+\t\tsps->index_count++;\n+\t\treturn task;\n+\t}\n+\treturn NULL;\n+}\n+\n static struct fetch_task *\n get_fetch_task_from_index(struct submodule_parallel_fetch *spf,\n \t\t\t struct strbuf *err)\n@@ -1744,6 +1799,16 @@ static int fetch_start_failure(struct strbuf *err,\n \treturn 0;\n }\n \n+static int status_start_failure(struct strbuf *err,\n+\t\t\t void *cb, void *task_cb)\n+{\n+\tstruct submodule_parallel_status *sps = cb;\n+\n+\tsps->result = 1;\n+\treturn 0;\n+}\n+\n+\n static int commit_missing_in_sub(const struct object_id *oid, void *data)\n {\n \tstruct repository *subrepo = data;\n@@ -1790,6 +1855,30 @@ static int parse_status_porcelain(char *buf, unsigned *dirty_submodule, int igno\n \treturn 0;\n }\n \n+static int status_finish(int retvalue, struct strbuf *err,\n+\t\t\tvoid *cb, void *task_cb)\n+{\n+\tstruct submodule_parallel_status *sps = cb;\n+\tstruct status_task *task = task_cb;\n+\tstruct string_list list = STRING_LIST_INIT_DUP;\n+\tstruct string_list_item *it = string_list_lookup(sps->submodule_names,\n+\t\t\t\t\t\t\t\t\t\t\ttask->path);\n+\tstruct submodule_status_util *util = it->util;\n+\n+\tint i;\n+\n+\tstring_list_split(&list, err->buf, '\\n', -1);\n+\n+\tfor (i = 0; i < list.nr; i++) {\n+\t\tif (parse_status_porcelain(list.items[i].string,\n+\t\t\t\t\t\t\t&util->dirty_submodule, util->ignore_untracked))\n+\t\t\tbreak;\n+\t}\n+\tstrbuf_reset(err);\n+\n+\treturn 0;\n+}\n+\n static int fetch_finish(int retvalue, struct strbuf *err,\n \t\t\tvoid *cb, void *task_cb)\n {\n@@ -1943,6 +2032,59 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \treturn dirty_submodule;\n }\n \n+static int get_next_submodule_status(struct child_process *cp,\n+\t\t\t\t\t\tstruct strbuf *err, void *data, void **task_cb)\n+{\n+\tstruct submodule_parallel_status *sps = data;\n+\tstruct status_task *task = get_status_task_from_index(sps, err);\n+\n+\tint ignore_untracked;\n+\n+\tif (!task) {\n+\t\treturn 0;\n+\t}\n+\n+\tignore_untracked = task->ignore_untracked;\n+\n+\tchild_process_init(cp);\n+\tprepare_submodule_repo_env_in_gitdir(&cp->env);\n+\n+\tstrvec_init(&cp->args);\n+\tstrvec_pushl(&cp->args, \"status\", \"--porcelain=2\", NULL);\n+\tif (ignore_untracked)\n+\t\tstrvec_push(&cp->args, \"-uno\");\n+\n+\tprepare_submodule_repo_env(&cp->env);\n+\tcp->git_cmd = 1;\n+\tcp->no_stdin = 1;\n+\tcp->dir = task->path;\n+\t*task_cb = task;\n+\treturn 1;\n+}\n+\n+int get_submodules_status(struct repository *r,\n+\t\t\tstruct string_list *submodules,\n+\t\t int max_parallel_jobs)\n+{\n+\tstruct submodule_parallel_status sps = SPS_INIT;\n+\n+\tsps.r = r;\n+\n+\tif (repo_read_index(r) < 0)\n+\t\tdie(_(\"index file corrupt\"));\n+\n+\tsps.submodule_names = submodules;\n+\tstring_list_sort(sps.submodule_names);\n+\trun_processes_parallel_pipe_output = 1;\n+\trun_processes_parallel_tr2(max_parallel_jobs,\n+\t\t\t\tget_next_submodule_status,\n+\t\t\t\tstatus_start_failure,\n+\t\t\t\tstatus_finish,\n+\t\t\t\t&sps,\n+\t\t\t\t\"submodule\", \"parallel/status\");\n+\treturn sps.result;\n+}\n+\n int submodule_uses_gitfile(const char *path)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\ndiff --git a/submodule.h b/submodule.h\nindex bfaa9da186..18a42c64ce 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -41,6 +41,12 @@ struct submodule_update_strategy {\n \t.type = SM_UPDATE_UNSPECIFIED, \\\n }\n \n+struct submodule_status_util {\n+\tint changed, ignore_untracked;\n+\tunsigned dirty_submodule, newmode;\n+\tstruct cache_entry *ce;\n+};\n+\n int is_gitmodules_unmerged(struct index_state *istate);\n int is_writing_gitmodules_ok(void);\n int is_staging_gitmodules_ok(struct index_state *istate);\n@@ -94,6 +100,9 @@ int fetch_submodules(struct repository *r,\n \t\t int command_line_option,\n \t\t int default_option,\n \t\t int quiet, int max_parallel_jobs);\n+int get_submodules_status(struct repository *r,\n+\t\t\t struct string_list *submodules,\n+\t\t int max_parallel_jobs);\n unsigned is_submodule_modified(const char *path, int ignore_untracked);\n int submodule_uses_gitfile(const char *path);\n \ndiff --git a/wt-status.c b/wt-status.c\nindex 867e3e417e..9864484f81 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -615,7 +615,7 @@ static void wt_status_collect_changes_worktree(struct wt_status *s)\n \trev.diffopt.rename_limit = s->rename_limit >= 0 ? s->rename_limit : rev.diffopt.rename_limit;\n \trev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : rev.diffopt.rename_score;\n \tcopy_pathspec(&rev.prune_data, &s->pathspec);\n-\trun_diff_files(&rev, 0);\n+\trun_diff_files(&rev, 0, s->parallel_jobs_submodules);\n \trelease_revisions(&rev);\n }\n \n@@ -1149,7 +1149,7 @@ static void wt_longstatus_print_verbose(struct wt_status *s)\n \t\tsetup_work_tree();\n \t\trev.diffopt.a_prefix = \"i/\";\n \t\trev.diffopt.b_prefix = \"w/\";\n-\t\trun_diff_files(&rev, 0);\n+\t\trun_diff_files(&rev, 0, -1);\n \t}\n \trelease_revisions(&rev);\n }\n@@ -2544,7 +2544,7 @@ int has_unstaged_changes(struct repository *r, int ignore_submodules)\n \t}\n \trev_info.diffopt.flags.quick = 1;\n \tdiff_setup_done(&rev_info.diffopt);\n-\tresult = run_diff_files(&rev_info, 0);\n+\tresult = run_diff_files(&rev_info, 0, -1);\n \tresult = diff_result_code(&rev_info.diffopt, result);\n \trelease_revisions(&rev_info);\n \treturn result;\ndiff --git a/wt-status.h b/wt-status.h\nindex ab9cc9d8f0..2ea2317715 100644\n--- a/wt-status.h\n+++ b/wt-status.h\n@@ -131,6 +131,7 @@ struct wt_status {\n \tenum wt_status_format status_format;\n \tstruct wt_status_state state;\n \tstruct object_id oid_commit; /* when not Initial */\n+\tint parallel_jobs_submodules;\n \n \t/* These are computed during processing of the individual sections */\n \tint committable;\n-- \n2.37.3.998.g577e59143f-goog\n\n"},{"id":"463502","messageId":"220923.86sfki4ize.gmgdl@evledraar.gmail.com","threadId":"58499","inReplyTo":"20220922232947.631309-2-calvinwan@google.com","subject":"Re: [PATCH 1/4] run-command: add pipe_output to run_processes_parallel","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-23T07:52:58Z","receivedAt":"2022-09-23T08:01:57Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Sep 22 2022, Calvin Wan wrote:\n\n> diff --git a/run-command.c b/run-command.c\n> index 14f17830f5..893bc1d294 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -1474,6 +1474,7 @@ enum child_state {\n> };\n> \n> int run_processes_parallel_ungroup;\n> +int run_processes_parallel_pipe_output;\n> struct parallel_processes {\n> \tvoid *data;\n> \n> @@ -1770,10 +1771,12 @@ int run_processes_parallel(int n,\n> \tint output_timeout = 100;\n> \tint spawn_cap = 4;\n> \tint ungroup = run_processes_parallel_ungroup;\n> +\tint pipe_output = run_processes_parallel_pipe_output;\n> \tstruct parallel_processes pp;\n> \n> \t/* unset for the next API user */\n> \trun_processes_parallel_ungroup = 0;\n> +\trun_processes_parallel_pipe_output = 0;\n> \n> \tpp_init(&pp, n, get_next_task, start_failure, task_finished, pp_cb,\n> \t\tungroup);\n\nI guess we could live with this, but this passing a function argument as\na global variable interface is something that came out of a topic to fix\na release regression:\nhttps://lore.kernel.org/git/cover-0.6-00000000000-20220421T122108Z-avarab@gmail.com/\n\nAn earlier version of that series simply changed the API to pass an\n\"opts\" struct instead:\nhttps://lore.kernel.org/git/patch-v2-2.8-5f0a6e9925f-20220518T195858Z-avarab@gmail.com/\n\nI really should have submitted those post-release cleanup patches\nalready, and I'm not sure whether the right thing at this point is to\ntake this & do the cleanup for \"ungroup\" *and* this new argument later.\n\nBut maybe you're interested in cherry-picking & adjusting the relevant\npart of that series for this one? I.e. we're not in some post-release\nregression hurry, so rather than extending the use of this odd interface\nwe could (and maybe should) just fix how we're doing it first.\n\nOn the implementation:\n\n> + * If the \"pipe_output\" option is specified, the output will be piped\n> + * to task_finished_fn in the \"struct strbuf *out\" variable. The output\n> + * will still be printed unless the callback resets the strbuf. The\n> + * \"pipe_output\" option can be enabled by setting the global\n> + * \"run_processes_parallel_pipe_output\" to \"1\" before invoking\n> + * run_processes_parallel(), it will be set back to \"0\" as soon as the\n> + * API reads that setting.\n\n...okey, but...\n\n> +static int task_finished_pipe_output(int result,\n> +\t\t\t struct strbuf *err,\n> +\t\t\t void *pp_cb,\n> +\t\t\t void *pp_task_cb)\n> +{\n> +\tif (err && pipe_output) {\n> +\t\tfprintf(stderr, \"%s\", err->buf);\n> +\t\tstrbuf_reset(err);\n\n...my memory's hazy, and I haven't re-logged in any detail, but is it\nreally the API interface here that the \"output\" callback function is\nresponsible for resetting the strbuf that the API gives to it?\n\nThat seems backwards to me, and e.g. a look at \"start_failure\" shows\nthat we strbuf_reset() the \"err\".\n\nWhat's the point of doing it in the API consumer? If it doesn't do it\nwe'll presumably keep accumulating output. Is there a use-case for that?\n\nOr perhaps it's not needed & this is really just misleading boilerplate?\n\n> @@ -140,6 +140,11 @@ test_expect_success 'run_command runs ungrouped in parallel with more jobs avail\n> \ttest_line_count = 4 err\n> '\n> \n> +test_expect_success 'run_command runs pipe_output in parallel with more jobs available than tasks' '\n> +\ttest-tool run-command --pipe-output run-command-parallel 5 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" 2>actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n\nLike the global argument, the copy/pasting for \"ungroup\" was mostly a\nmatter of expediency.\n\nBut at least in that case we have a different assertion (test_cmp\nv.s. test_line_count).\n\nBut here this test case seems to be exactly the same as for the\n\"vanilla\" version.\n\nSo can't we make this some:\n\n\tfor opt in '' '--pipe-output'\n\tdo\n\t\ttest_expect_success ...\n\tdone\n\n?\n"},{"id":"463504","messageId":"220923.86o7v64ik0.gmgdl@evledraar.gmail.com","threadId":"58499","inReplyTo":"20220922232947.631309-5-calvinwan@google.com","subject":"Re: [PATCH 4/4] diff-lib: parallelize run_diff_files for submodules","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-23T08:06:56Z","receivedAt":"2022-09-23T08:11:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Sep 22 2022, Calvin Wan wrote:\n\n> +status.parallelSubmodules::\n> +\tWhen linkgit:git-status[1] is run in a superproject with\n> +\tsubmodules, a status subprocess is spawned for every submodule.\n> +\tThis option specifies the number of submodule status subprocesses\n> +\tto run in parallel. If unset, it defaults to 1.\n\nWhy do we default to 1, instead of e.g. grep.threads defaulting to the\n\"cores available\"?\n\n> +struct submodule_status_util {\n> +\tint changed, ignore_untracked;\n\nnit: we tend not to group by type, which also makes adding API docs\nlater easier.\n\nExcept that we tend to do that if all the things are really boolean flags, which these are, so maybe:\n\n\tunsigned int changed:1,\n \t ignore_untracked:1;\n\n?\n"},{"id":"463543","messageId":"xmqqy1u9uddc.fsf@gitster.g","threadId":"58499","inReplyTo":"20220922232947.631309-2-calvinwan@google.com","subject":"Re: [PATCH 1/4] run-command: add pipe_output to run_processes_parallel","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-23T18:58:23Z","receivedAt":"2022-09-23T18:58:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Calvin Wan writes:\n\n> run_processes_parallel periodically collects output from its child\n> processes, prints it, and then resets the buffers for each child.\n> Add run_processes_parallel_pipe_output variable so output can be\n> collected and fed to task_finished. When set, the function referenced\n> by task_finished should parse the output of each child process.\n\nI am puzzled.\n\n * Why are we configuring an API behaviour via a global variable in\n 21st century?\n\n * The name \"task_finished\" is mentioned, but it is unclear what it\n is. Is it one of the parameters to run_process_parallel()?\n\n * Is the effect of the new feature that task_finished callback is\n called with the output, in addition to the normal output? I am\n not sure why it is called \"pipe\". The task_finished callback may\n be free to fork a child and send the received output from the\n task to that child over the pipe, but that is what a client code\n could do and is inappropriate to base the name of the mechanism,\n isn't it?\n\n> @@ -1770,10 +1771,12 @@ int run_processes_parallel(int n,\n> \tint output_timeout = 100;\n> \tint spawn_cap = 4;\n> \tint ungroup = run_processes_parallel_ungroup;\n> +\tint pipe_output = run_processes_parallel_pipe_output;\n> \tstruct parallel_processes pp;\n> \n> \t/* unset for the next API user */\n> \trun_processes_parallel_ungroup = 0;\n> +\trun_processes_parallel_pipe_output = 0;\n> \n> \tpp_init(&pp, n, get_next_task, start_failure, task_finished, pp_cb,\n> \t\tungroup);\n> @@ -1800,7 +1803,8 @@ int run_processes_parallel(int n,\n> \t\t\t\tpp.children[i].state = GIT_CP_WAIT_CLEANUP;\n> \t\t} else {\n> \t\t\tpp_buffer_stderr(&pp, output_timeout);\n> -\t\t\tpp_output(&pp);\n> +\t\t\tif (!pipe_output)\n> +\t\t\t\tpp_output(&pp);\n\nSo, we do not send the output from the child to the regular output\nchannel when pipe_output is in effect. OK.\n\n> \t\t}\n> \t\tcode = pp_collect_finished(&pp);\n> \t\tif (code) {\n\nAnd no other code changes? This is quite different from what I\nexpected from reading the proposed log message.\n\nAm I correct to say that under this new mode, we no longer flush any\noutput while the child task is running (due to the change in the\nabove hunk to omit calls to pp_output() during the run) and instead\nkeep accumulating in the strbuf, until the child task finishes, at\nwhich time pp_collect_finished() will call task_finished callback.\n\nEven though the callback usually consumes the last piece of the\noutput since the last pp_output() call made during the normal\nexecution of the run_processes_parallel() loop, because we omitted\nthese calls, we have full output from the child task accumulated in\nthe children[].err strbuf. We may still not output .err for real,\nas we may not be the output_owner, in which case we may only append\nto .buffered_output member.\n\nI am puzzled simply because, if the above summary is correct, I do\nnot see how a word \"pipe\" have a chance to come into the picture.\n\nI can sort of see that in this mode, we would end up buffering the\nentire output from each child task into one strbuf each, and can\navoid stalling the child tasks waiting for their turn to see their\noutput pipes drained. But is this a reasonable thing to do? How do\nwe control the memory consumption to avoid having to spool unbounded\namount of output from child tasks in core, or do we have a good\nreason to believe that we do not have to bother?\n\nThanks.\n\n"},{"id":"463549","messageId":"xmqqleq9u8u3.fsf@gitster.g","threadId":"58499","inReplyTo":"20220922232947.631309-4-calvinwan@google.com","subject":"Re: [PATCH 3/4] diff-lib: refactor functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-23T20:36:20Z","receivedAt":"2022-09-23T20:43:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Calvin Wan writes:\n\n> Flatten out the if statements in match_stat_with_submodule so the\n> logic is more readable and easier for future patches to add to.\n>\n> Move code that updates relevant variables from the end of\n> run_diff_files to finish_run_diff_files. A future patch will utilize\n> said function.\n>\n> Signed-off-by: Calvin Wan \n> ---\n> diff-lib.c | 71 ++++++++++++++++++++++++++++++++----------------------\n> 1 file changed, 42 insertions(+), 29 deletions(-)\n>\n> diff --git a/diff-lib.c b/diff-lib.c\n> index 7eb66a417a..2e148b79e6 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -73,21 +73,50 @@ static int match_stat_with_submodule(struct diff_options *diffopt,\n> \t\t\t\t unsigned *dirty_submodule)\n> {\n> \tint changed = ie_match_stat(diffopt->repo->index, ce, st, ce_option);\n> -\tif (S_ISGITLINK(ce->ce_mode)) {\n> -\t\tstruct diff_flags orig_flags = diffopt->flags;\n> -\t\tif (!diffopt->flags.override_submodule_config)\n> -\t\t\tset_diffopt_flags_from_submodule_config(diffopt, ce->name);\n> -\t\tif (diffopt->flags.ignore_submodules)\n> -\t\t\tchanged = 0;\n> -\t\telse if (!diffopt->flags.ignore_dirty_submodules &&\n> -\t\t\t (!changed || diffopt->flags.dirty_submodules))\n> -\t\t\t*dirty_submodule = is_submodule_modified(ce->name,\n> -\t\t\t\t\t\t\t\t diffopt->flags.ignore_untracked_in_submodules);\n> -\t\tdiffopt->flags = orig_flags;\n> +\tstruct diff_flags orig_flags = diffopt->flags;\n> +\tif (!S_ISGITLINK(ce->ce_mode))\n> +\t\tgoto cleanup;\n> +\tif (!diffopt->flags.override_submodule_config)\n> +\t\tset_diffopt_flags_from_submodule_config(diffopt, ce->name);\n> +\tif (diffopt->flags.ignore_submodules) {\n> +\t\tchanged = 0;\n> +\t\tgoto cleanup;\n> \t}\n> +\tif (!diffopt->flags.ignore_dirty_submodules &&\n> +\t\t(!changed || diffopt->flags.dirty_submodules))\n> +\t\t\t*dirty_submodule = is_submodule_modified(ce->name,\n> +\t\t\t\t\t\t\tdiffopt->flags.ignore_untracked_in_submodules);\n> +cleanup:\n> +\tdiffopt->flags = orig_flags;\n> \treturn changed;\n> }\n\nUnlike the original, this always makes two needless structure\nassignments for anything that is not a submodule. \n\nCan we fix that regression before moving forward?\n\nEven when ce_mode is a gitlink, if .ignore_submodules bit is set,\nthe two structure assignments between diffopt->flags and orig_flags\nbecome necessary, so the original was already doing extra copies but\nwe do not have to make it worse.\n\n> +static void finish_run_diff_files(struct rev_info *revs,\n> +\t\t\t\t\t\t struct cache_entry *ce,\n> +\t\t\t\t\t\t struct index_state *istate,\n> +\t\t\t\t\t\t int changed, int dirty_submodule,\n> +\t\t\t\t\t\t unsigned int newmode)\n> +{\n> +\tunsigned int oldmode;\n> +\tconst struct object_id *old_oid, *new_oid;\n> +\n> +\tif (!changed && !dirty_submodule) {\n> +\t\t\tce_mark_uptodate(ce);\n> +\t\t\tif (!S_ISGITLINK(ce->ce_mode))\n> +\t\t\t\tmark_fsmonitor_valid(istate, ce);\n> +\t\t\tif (!revs->diffopt.flags.find_copies_harder)\n> +\t\t\t\treturn;\n> +\t\t}\n> +\t\toldmode = ce->ce_mode;\n> +\t\told_oid = &ce->oid;\n> +\t\tnew_oid = changed ? null_oid() : &ce->oid;\n> +\t\tdiff_change(&revs->diffopt, oldmode, newmode,\n> +\t\t\t old_oid, new_oid,\n> +\t\t\t !is_null_oid(old_oid),\n> +\t\t\t !is_null_oid(new_oid),\n> +\t\t\t ce->name, 0, dirty_submodule);\n> +}\n\nStrange indentation. It is unclear why this bottom 1/3 of the loop\nbody of run_diff_files() need to be a separate helper function,\nwhile the top 2/3 does not. The resulting loop (below) becomes very\nhard to follow because the reader cannot tell when diff_change() is\ncalled and when it is not.\n\nOverall, I see this change detrimental to diff-lib API at this step\nin the series. Later steps may show something more rewarding than\nthe downsides we see here, hopefully.\n"},{"id":"463555","messageId":"xmqqy1u9sr3a.fsf@gitster.g","threadId":"58499","inReplyTo":"20220922232947.631309-5-calvinwan@google.com","subject":"Re: [PATCH 4/4] diff-lib: parallelize run_diff_files for submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-23T21:44:57Z","receivedAt":"2022-09-23T21:45:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Calvin Wan writes:\n\n> During the iteration of the index entries in run_diff_files, whenever\n> a submodule is found and needs its status checked, a subprocess is\n> spawned for it. Instead of spawning the subprocess immediately and\n> waiting for its completion to continue, hold onto all submodules and\n> relevant information in a list. Then use that list to create tasks for\n> run_processes_parallel. Finished subprocesses pipe their output to\n> status_finish which parses it and sets the relevant variables.\n\nExcellent---I like the idea very much.\n\nYou make it sound as if this is only for \"git status\", but shouldn't\nit benefit the usual \"git diff\" the same way when you have\nsubmodules that can be dirty?\n\n> Add config option status.parallelSubmodules to set the maximum number\n> of parallel jobs. \n\nConfiguration is fine, but I am having a hard time justifying the\naddition of an extra parameter to run_diff_files(). It might be\nmore palatable to give a new bit (default off) in the rev structure\nthat tells it that it is OK to go into the \"defer_submodule_status\"\nmode, if we absolutely want to change the behaviour of\nrun_diff_files() only for a single caller or something, but I doubt\nit should even be needed.\n\nI cannot think of a sane user interface that says \"when\nrun_diff_files() is invoked as an implementation detail of 'status',\nuse this value, and when it is running for another command 'foo',\nuse this other value\". How would a user decide to pick a different\nvalue for different commands?\n\nLetting a single configuration variable to decide the degree of\nparallelism inside run_diff_files() would be sufficient, and we\nshouldn't have to touch all the existing calling sites of\nrun_diff_files() all over the place. If you absolutely want to do\nthis, I'd rather see the new member for the configuration variable\nnot in wt_status but in rev_info.\n\n> +status.parallelSubmodules::\n> +\tWhen linkgit:git-status[1] is run in a superproject with\n> +\tsubmodules, a status subprocess is spawned for every submodule.\n> +\tThis option specifies the number of submodule status subprocesses\n> +\tto run in parallel. If unset, it defaults to 1.\n\nAs I said, I am not sure .parallelSubmodules per command makes\nmuch sense. \"If unset, it defaults to\" sounds a bit redundant (if\nit is explicitly set, the default value should not matter).\n\n> @@ -83,11 +88,20 @@ static int match_stat_with_submodule(struct diff_options *diffopt,\n> \t\tgoto cleanup;\n> \t}\n> \tif (!diffopt->flags.ignore_dirty_submodules &&\n> -\t\t(!changed || diffopt->flags.dirty_submodules))\n> +\t\t(!changed || diffopt->flags.dirty_submodules)) {\n> +\t\tif (defer_submodule_status && *defer_submodule_status) {\n> +\t\t\tdefer = 1;\n> +\t\t\t*ignore_untracked_in_submodules =\n> +\t\t\t\t\t\t\tdiffopt->flags.ignore_untracked_in_submodules;\n> +\t\t} else {\n> \t\t\t*dirty_submodule = is_submodule_modified(ce->name,\n> \t\t\t\t\t\t\tdiffopt->flags.ignore_untracked_in_submodules);\n> +\t\t}\n> +\t}\n\nOK, so the caller can look at *defer\n\n> cleanup:\n> \tdiffopt->flags = orig_flags;\n> +\tif (defer_submodule_status)\n> +\t\t*defer_submodule_status = defer;\n> \treturn changed;\n> }\n> \n> @@ -117,7 +131,7 @@ static void finish_run_diff_files(struct rev_info *revs,\n> \t\t\t ce->name, 0, dirty_submodule);\n> }\n> \n> -int run_diff_files(struct rev_info *revs, unsigned int option)\n> +int run_diff_files(struct rev_info *revs, unsigned int option, int parallel_jobs)\n> {\n> \tint entries, i;\n> \tint diff_unmerged_stage = revs->max_count;\n> @@ -125,6 +139,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n> \t\t\t ? CE_MATCH_RACY_IS_DIRTY : 0);\n> \tuint64_t start = getnanotime();\n> \tstruct index_state *istate = revs->diffopt.repo->index;\n> +\tstruct string_list submodules = STRING_LIST_INIT_NODUP;\n> \n> \tdiff_set_mnemonic_prefix(&revs->diffopt, \"i/\", \"w/\");\n> \n> @@ -138,6 +153,9 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n> \t\tstruct cache_entry *ce = istate->cache[i];\n> \t\tint changed;\n> \t\tunsigned dirty_submodule = 0;\n> +\t\tint defer_submodule_status = revs && revs->repo &&\n> +\t\t\t\t\t\t\t!strcmp(revs->repo->gitdir, \".git\");\n\nWhat is this overly deeply indented comparison with \".git\" doing?\nWhat are we checking and why? Is that something we need to do for\neach and every active_cache[] entry, or isn't it constant over the\nloop?\n\n> +\t\tint ignore_untracked_in_submodules;\n> \n> \t\tif (diff_can_quit_early(&revs->diffopt))\n> \t\t\tbreak;\n> @@ -269,11 +287,36 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n> \t\t\t}\n> \n> \t\t\tchanged = match_stat_with_submodule(&revs->diffopt, ce, &st,\n> +\t\t\t\t\t\t\tce_option, &dirty_submodule,\n> +\t\t\t\t\t\t\t&defer_submodule_status,\n> +\t\t\t\t\t\t\t&ignore_untracked_in_submodules);\n> \t\t\tnewmode = ce_mode_from_stat(ce, st.st_mode);\n> +\t\t\tif (defer_submodule_status) {\n> +\t\t\t\tstruct string_list_item *item =\n> +\t\t\t\t\t\t\t\tstring_list_append(&submodules, ce->name);\n> +\t\t\t\tstruct submodule_status_util *util = xmalloc(sizeof(*util));\n> +\t\t\t\tutil->changed = changed;\n> +\t\t\t\tutil->dirty_submodule = 0;\n> +\t\t\t\tutil->ignore_untracked = ignore_untracked_in_submodules;\n> +\t\t\t\tutil->newmode = newmode;\n> +\t\t\t\tutil->ce = ce;\n> +\t\t\t\titem->util = util;\n> +\t\t\t\tcontinue;\n\nThis makes me wonder if defer_submodule_status should be a string\nlist that receives the punted submodules---iow, don't these details\nbelong to match_stat_with_submodule() rather than its caller here?\n\nEven better may be to start a new child task for the submodule here,\nletting it work while the parent process moves on to the next entry\nin the active_cache[]. I do not know if you thought about doing it\nthat way.\n\nI dunno.\n\n> +\t\t\t}\n> \t\t}\n> \t\tfinish_run_diff_files(revs, ce, istate, changed, dirty_submodule, newmode);\n> \t}\n> +\tif (submodules.nr > 0) {\n> +\t\tif (get_submodules_status(revs->repo, &submodules,\n> +\t\t\t\t\t\tparallel_jobs > 0 ? parallel_jobs : 1))\n> +\t\t\tBUG(\"Submodule status failed\");\n> +\t\tfor (int i = 0; i < submodules.nr; i++) {\n\nWe still do not allow \"for (type var = init; ...)\" if I am not\nmistaken. Check the coding guidelines.\n"},{"id":"463560","messageId":"xmqqleq9snr8.fsf@gitster.g","threadId":"58499","inReplyTo":"20220922232947.631309-1-calvinwan@google.com","subject":"Re: [PATCH 0/4] submodule: parallelize status","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-23T22:56:59Z","receivedAt":"2022-09-23T22:57:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Calvin Wan writes:\n\n> Future work: The reason why status is much slower on a cold cache vs\n> warm cache is mainly due to refreshing the index. It is worth\n> investigating whether this is entirely necessary.\n\nI suspect that the CI breakage\n\n https://github.com/git/git/actions/runs/3115673002/jobs/5052804463\n\nis due to this topic. \n\nIn \"SANTIZE=leak GIT_TEST_PASSING_SANITIZE_LEAK=true make test\" run\nlocally, I see t7507 failing quite badly.\n"},{"id":"463583","messageId":"xmqqsfkgr0ht.fsf@gitster.g","threadId":"58499","inReplyTo":"220923.86o7v64ik0.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 4/4] diff-lib: parallelize run_diff_files for submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-24T20:17:02Z","receivedAt":"2022-09-24T20:17:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Thu, Sep 22 2022, Calvin Wan wrote:\n>\n>> +status.parallelSubmodules::\n>> +\tWhen linkgit:git-status[1] is run in a superproject with\n>> +\tsubmodules, a status subprocess is spawned for every submodule.\n>> +\tThis option specifies the number of submodule status subprocesses\n>> +\tto run in parallel. If unset, it defaults to 1.\n>\n> Why do we default to 1, instead of e.g. grep.threads defaulting to the\n> \"cores available\"?\n\nI would imagine we would want to be able to say:\n\n - I do not trust the parallel mode yet, just use the single process\n method that we have always been using.\n\n - I do not know how many cores I have, just use a reasonable\n default parallelism.\n\n - I want to use N processes because I know better than auto-scaling\n based on num_cpus.\n\nAnd the value of 1 would be a reasonable way to express the first\none.\n\n\n"},{"id":"463600","messageId":"88b194d9-7c78-c12e-1c29-95f768db7772@gmail.com","threadId":"58499","inReplyTo":"20220922232947.631309-5-calvinwan@google.com","subject":"Re: [PATCH 4/4] diff-lib: parallelize run_diff_files for submodules","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-09-25T13:59:15Z","receivedAt":"2022-09-25T13:59:25Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Calvin\n\nOn 23/09/2022 00:29, Calvin Wan wrote:\n> During the iteration of the index entries in run_diff_files, whenever\n> a submodule is found and needs its status checked, a subprocess is\n> spawned for it. Instead of spawning the subprocess immediately and\n> waiting for its completion to continue, hold onto all submodules and\n> relevant information in a list. Then use that list to create tasks for\n> run_processes_parallel. Finished subprocesses pipe their output to\n> status_finish which parses it and sets the relevant variables.\n> \n> Add config option status.parallelSubmodules to set the maximum number\n> of parallel jobs.\n\nI suspect in the future we may want to parallelize other commands for \nsubmodules in which case a more general name such as submodules.threads \nmight be a better choice. The speed up in the cover letter is \nimpressive, could this be safely enabled by default?\n\n> index fcf9c85947..c5147a7952 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1468,6 +1468,12 @@ static int git_status_config(const char *k, const char *v, void *cb)\n> \t\ts->detect_rename = git_config_rename(k, v);\n> \t\treturn 0;\n> \t}\n> +\tif (!strcmp(k, \"status.parallelsubmodules\")) {\n> +\t\ts->parallel_jobs_submodules = git_config_int(k, v);\n> +\t\tif (s->parallel_jobs_submodules < 0)\n> +\t\t\tdie(_(\"status.parallelsubmodules cannot be negative\"));\n\nWhat does a value of zero mean?\n\n> diff --git a/diff-lib.c b/diff-lib.c\n> index 2e148b79e6..ec745755fc 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n\n> -int run_diff_files(struct rev_info *revs, unsigned int option)\n> +int run_diff_files(struct rev_info *revs, unsigned int option, int parallel_jobs)\n\nAnother possibility would be to add a member to struct diff_opts, rather \nthan changing the function signature here, I'm wondering what the trade \noffs of the two approaches are. Also seeing all the callers from other \ncommands being changed made me wonder if they would benefit from \nparallelizing submodules as well. There aren't any tests - could we use \nGIT_TRACE2 to check that we are running the submodule diffs in parallel?\n\nBest Wishes\n\nPhillip\n"},{"id":"463640","messageId":"CAFySSZDDGiSsrcioa_JsgtoqH1aMJfKV2UyCxwkjxa3eX+-Oww@mail.gmail.com","threadId":"58499","inReplyTo":"xmqqleq9snr8.fsf@gitster.g","subject":"Re: [PATCH 0/4] submodule: parallelize status","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-09-26T16:33:13Z","receivedAt":"2022-09-26T17:19:51Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"I'm getting the following error running that command locally:\nBailout called. Further testing stopped:\nGIT_TEST_PASSING_SANITIZE_LEAK=true has no effect except when\ncompiled with SANITIZE=leak\n"},{"id":"463641","messageId":"CAFySSZB=bPWpK25Gwtm06cOyupGeUYo6qLC4PWPwaS2pNn22hA@mail.gmail.com","threadId":"58499","inReplyTo":"220923.86sfki4ize.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/4] run-command: add pipe_output to run_processes_parallel","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-09-26T16:59:08Z","receivedAt":"2022-09-26T17:37:36Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"> An earlier version of that series simply changed the API to pass an\n> \"opts\" struct instead:\n> https://lore.kernel.org/git/patch-v2-2.8-5f0a6e9925f-20220518T195858Z-avarab@gmail.com/\n>\n> I really should have submitted those post-release cleanup patches\n> already, and I'm not sure whether the right thing at this point is to\n> take this & do the cleanup for \"ungroup\" *and* this new argument later.\n>\n> But maybe you're interested in cherry-picking & adjusting the relevant\n> part of that series for this one? I.e. we're not in some post-release\n> regression hurry, so rather than extending the use of this odd interface\n> we could (and maybe should) just fix how we're doing it first.\n\nI'll go ahead and give this a try. I was also a little bit surprised that\n\"ungroup\" was set this way, but didn't realize it was for a quick fix.\n\n>\n> On the implementation:\n>\n> > + * If the \"pipe_output\" option is specified, the output will be piped\n> > + * to task_finished_fn in the \"struct strbuf *out\" variable. The output\n> > + * will still be printed unless the callback resets the strbuf. The\n> > + * \"pipe_output\" option can be enabled by setting the global\n> > + * \"run_processes_parallel_pipe_output\" to \"1\" before invoking\n> > + * run_processes_parallel(), it will be set back to \"0\" as soon as the\n> > + * API reads that setting.\n>\n> ...okey, but...\n>\n> > +static int task_finished_pipe_output(int result,\n> > + struct strbuf *err,\n> > + void *pp_cb,\n> > + void *pp_task_cb)\n> > +{\n> > + if (err && pipe_output) {\n> > + fprintf(stderr, \"%s\", err->buf);\n> > + strbuf_reset(err);\n>\n> ...my memory's hazy, and I haven't re-logged in any detail, but is it\n> really the API interface here that the \"output\" callback function is\n> responsible for resetting the strbuf that the API gives to it?\n>\n> That seems backwards to me, and e.g. a look at \"start_failure\" shows\n> that we strbuf_reset() the \"err\".\n>\n> What's the point of doing it in the API consumer? If it doesn't do it\n> we'll presumably keep accumulating output. Is there a use-case for that?\n>\n> Or perhaps it's not needed & this is really just misleading boilerplate?\n\nUltimately it is not needed -- I added it as an example to showcase that\nthe output is correctly being piped to \"task_finished_pipe_output\". The\nreset is necessary in this case to prevent the output from being printed\ntwice. I'm not sure how exactly else I would go about testing \"pipe_output\".\n\n>\n> > @@ -140,6 +140,11 @@ test_expect_success 'run_command runs ungrouped in parallel with more jobs avail\n> > test_line_count = 4 err\n> > '\n> >\n> > +test_expect_success 'run_command runs pipe_output in parallel with more jobs available than tasks' '\n> > + test-tool run-command --pipe-output run-command-parallel 5 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" 2>actual &&\n> > + test_cmp expect actual\n> > +'\n> > +\n>\n> Like the global argument, the copy/pasting for \"ungroup\" was mostly a\n> matter of expediency.\n>\n> But at least in that case we have a different assertion (test_cmp\n> v.s. test_line_count).\n>\n> But here this test case seems to be exactly the same as for the\n> \"vanilla\" version.\n>\n> So can't we make this some:\n>\n> for opt in '' '--pipe-output'\n> do\n> test_expect_success ...\n> done\n>\n> ?\n\nYes we can -- but I may need to rethink how instead I should be testing\nthis option?\n"},{"id":"463644","messageId":"xmqqedvyqcw6.fsf@gitster.g","threadId":"58499","inReplyTo":"88b194d9-7c78-c12e-1c29-95f768db7772@gmail.com","subject":"Re: [PATCH 4/4] diff-lib: parallelize run_diff_files for submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-26T17:11:21Z","receivedAt":"2022-09-26T17:43:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n>> +\tif (!strcmp(k, \"status.parallelsubmodules\")) {\n>> +\t\ts->parallel_jobs_submodules = git_config_int(k, v);\n>> +\t\tif (s->parallel_jobs_submodules < 0)\n>> +\t\t\tdie(_(\"status.parallelsubmodules cannot be negative\"));\n>\n> What does a value of zero mean?\n\nGood question. I don't remember what the code in the patch I read\nactually does, but I would imagine we would want to be able to say:\n\n - I do not trust the parallel mode yet, just use the single process\n method that we have always been using.\n\n - I do not know how many cores I have, just use a reasonable\n default parallelism.\n\n - I want to use N processes because I know better than auto-scaling\n based on num_cpus.\n\nAnd the value of 1 would be a reasonable way to express the first\none, and 0 would be a reasonable thing to do for the second one.\n\n>> -int run_diff_files(struct rev_info *revs, unsigned int option)\n>> +int run_diff_files(struct rev_info *revs, unsigned int option, int parallel_jobs)\n>\n> Another possibility would be to add a member to struct diff_opts,\n\nYes, absolutely. Somewhere that is reachable from rev_info\nstructure would be more appropriate.\n\nThanks.\n"},{"id":"463648","messageId":"CAFySSZA=tThoHdTY7+bMStvC=xeeyMiv4aVDYt-eNW2mQE10qg@mail.gmail.com","threadId":"58499","inReplyTo":"xmqqy1u9uddc.fsf@gitster.g","subject":"Re: [PATCH 1/4] run-command: add pipe_output to run_processes_parallel","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-09-26T17:31:05Z","receivedAt":"2022-09-26T17:55:28Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"> * Why are we configuring an API behaviour via a global variable in\n> 21st century?\n\nI was mimicking how \"ungroup\" worked, but now that Avar mentions\nthat pattern was for a quick regression fix, I can fix it to pass it in as a\nparameter.\n\n> * The name \"task_finished\" is mentioned, but it is unclear what it\n> is. Is it one of the parameters to run_process_parallel()?\n\nIt is one of the callback functions passed in as a parameter to\nrun_process_paraller(). I'll go ahead and clarify that.\n\n> * Is the effect of the new feature that task_finished callback is\n> called with the output, in addition to the normal output? I am\n> not sure why it is called \"pipe\". The task_finished callback may\n> be free to fork a child and send the received output from the\n> task to that child over the pipe, but that is what a client code\n> could do and is inappropriate to base the name of the mechanism,\n> isn't it?\n\nThe output in task_finished callback, before pipe_output, either\ncontains part of the output or the entire output of the child process,\nsince the output is periodically collected into stderr and then reset.\nThe intention of output I believe is for the caller to be able to add\nanything they would like to the end (this can be seen with functions\nlike fetch_finished() in builtin/fetch.c). My intention with pipe_output\nis to guarantee that output contains the entire output of the child\nprocess so task_finished can utilize it.\n\n>\n> > @@ -1770,10 +1771,12 @@ int run_processes_parallel(int n,\n> > int output_timeout = 100;\n> > int spawn_cap = 4;\n> > int ungroup = run_processes_parallel_ungroup;\n> > + int pipe_output = run_processes_parallel_pipe_output;\n> > struct parallel_processes pp;\n> >\n> > /* unset for the next API user */\n> > run_processes_parallel_ungroup = 0;\n> > + run_processes_parallel_pipe_output = 0;\n> >\n> > pp_init(&pp, n, get_next_task, start_failure, task_finished, pp_cb,\n> > ungroup);\n> > @@ -1800,7 +1803,8 @@ int run_processes_parallel(int n,\n> > pp.children[i].state = GIT_CP_WAIT_CLEANUP;\n> > } else {\n> > pp_buffer_stderr(&pp, output_timeout);\n> > - pp_output(&pp);\n> > + if (!pipe_output)\n> > + pp_output(&pp);\n>\n> So, we do not send the output from the child to the regular output\n> channel when pipe_output is in effect. OK.\n>\n> > }\n> > code = pp_collect_finished(&pp);\n> > if (code) {\n>\n> And no other code changes? This is quite different from what I\n> expected from reading the proposed log message.\n>\n> Am I correct to say that under this new mode, we no longer flush any\n> output while the child task is running (due to the change in the\n> above hunk to omit calls to pp_output() during the run) and instead\n> keep accumulating in the strbuf, until the child task finishes, at\n> which time pp_collect_finished() will call task_finished callback.\n>\n> Even though the callback usually consumes the last piece of the\n> output since the last pp_output() call made during the normal\n> execution of the run_processes_parallel() loop, because we omitted\n> these calls, we have full output from the child task accumulated in\n> the children[].err strbuf. We may still not output .err for real,\n> as we may not be the output_owner, in which case we may only append\n> to .buffered_output member.\n>\n> I am puzzled simply because, if the above summary is correct, I do\n> not see how a word \"pipe\" have a chance to come into the picture.\n\nAh I see what you mean here -- your summary is correct. Something\nlike \"buffer_output\" would make much more sense.\n\n> I can sort of see that in this mode, we would end up buffering the\n> entire output from each child task into one strbuf each, and can\n> avoid stalling the child tasks waiting for their turn to see their\n> output pipes drained. But is this a reasonable thing to do? How do\n> we control the memory consumption to avoid having to spool unbounded\n> amount of output from child tasks in core, or do we have a good\n> reason to believe that we do not have to bother?\n\nYou are correct that storing unbounded output doesn't seem like a good\nidea. One idea I have is to parse output during the periodic collection rather\nthan waiting till the end. The other idea I had was to add another\n\"git status --porcelain\" option that would only output the necessary\npieces of information so we wouldn't have to bother with worrying about\nunbounded output.\n\nAny other thoughts as to how I can workaround this?\n\nThanks!\n"},{"id":"463651","messageId":"CAFySSZBxwMHOD0c7T5cF3kkcej3+c2i97u=2t4jZthSCvuEhDg@mail.gmail.com","threadId":"58499","inReplyTo":"xmqqleq9u8u3.fsf@gitster.g","subject":"Re: [PATCH 3/4] diff-lib: refactor functions","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-09-26T17:35:56Z","receivedAt":"2022-09-26T17:58:24Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"> Unlike the original, this always makes two needless structure\n> assignments for anything that is not a submodule.\n>\n> Can we fix that regression before moving forward?\n>\n> Even when ce_mode is a gitlink, if .ignore_submodules bit is set,\n> the two structure assignments between diffopt->flags and orig_flags\n> become necessary, so the original was already doing extra copies but\n> we do not have to make it worse.\n\nack\n\n> Strange indentation. It is unclear why this bottom 1/3 of the loop\n> body of run_diff_files() need to be a separate helper function,\n> while the top 2/3 does not. The resulting loop (below) becomes very\n> hard to follow because the reader cannot tell when diff_change() is\n> called and when it is not.\n>\n> Overall, I see this change detrimental to diff-lib API at this step\n> in the series. Later steps may show something more rewarding than\n> the downsides we see here, hopefully.\n\nI'll see if I can come up with a way to rewrite this so it is less confusing.\nAlternatively, I could remove this refactor.\n"},{"id":"463653","messageId":"CAFySSZBf7iOkAMBJm3hAJHLA5cGMb0ZJj7xOfn8E3Tn7AU7ebg@mail.gmail.com","threadId":"58499","inReplyTo":"220923.86o7v64ik0.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 4/4] diff-lib: parallelize run_diff_files for submodules","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-09-26T17:50:25Z","receivedAt":"2022-09-26T18:07:02Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"> > +status.parallelSubmodules::\n> > + When linkgit:git-status[1] is run in a superproject with\n> > + submodules, a status subprocess is spawned for every submodule.\n> > + This option specifies the number of submodule status subprocesses\n> > + to run in parallel. If unset, it defaults to 1.\n>\n> Why do we default to 1, instead of e.g. grep.threads defaulting to the\n> \"cores available\"?\n\nIn my cover letter, I noted that with too many processes, status starts to\nslow down (but is still better than the baseline). This is because the\nexpensive part of status is IO, specifically reading objects from the index.\nMuch of the speedup of this patch comes from taking advantage of the\nability to do parallel reads on an SSD, rather than splitting up the work\nof status. However, this doesn't work with an HDD, so status may\nactually be slower than baseline with multiple processes since there is\nnow scheduling/switching overhead.\n\n>\n> > +struct submodule_status_util {\n> > + int changed, ignore_untracked;\n>\n> nit: we tend not to group by type, which also makes adding API docs\n> later easier.\n>\n> Except that we tend to do that if all the things are really boolean flags, which these are, so maybe:\n>\n> unsigned int changed:1,\n> ignore_untracked:1;\n>\n> ?\n\nack.\n"},{"id":"463667","messageId":"CAFySSZBCaAUGZuEdxSJEW_7tEcOVJfnkA_nY6BTkTQ1s3xGd5g@mail.gmail.com","threadId":"58499","inReplyTo":"xmqqy1u9sr3a.fsf@gitster.g","subject":"Re: [PATCH 4/4] diff-lib: parallelize run_diff_files for submodules","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-09-26T19:12:46Z","receivedAt":"2022-09-26T19:13:01Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"> You make it sound as if this is only for \"git status\", but shouldn't\n> it benefit the usual \"git diff\" the same way when you have\n> submodules that can be dirty?\n\nIt should also! I'll reword the commit message.\n\n> > Add config option status.parallelSubmodules to set the maximum number\n> > of parallel jobs.\n>\n> Configuration is fine, but I am having a hard time justifying the\n> addition of an extra parameter to run_diff_files(). It might be\n> more palatable to give a new bit (default off) in the rev structure\n> that tells it that it is OK to go into the \"defer_submodule_status\"\n> mode, if we absolutely want to change the behaviour of\n> run_diff_files() only for a single caller or something, but I doubt\n> it should even be needed.\n>\n> I cannot think of a sane user interface that says \"when\n> run_diff_files() is invoked as an implementation detail of 'status',\n> use this value, and when it is running for another command 'foo',\n> use this other value\". How would a user decide to pick a different\n> value for different commands?\n>\n> Letting a single configuration variable to decide the degree of\n> parallelism inside run_diff_files() would be sufficient, and we\n> shouldn't have to touch all the existing calling sites of\n> run_diff_files() all over the place. If you absolutely want to do\n> this, I'd rather see the new member for the configuration variable\n> not in wt_status but in rev_info.\n\nI agree the configuration variable should be in rev_info and not\nbe specific to status since other callers can take advantage of it.\n\n>\n> > +status.parallelSubmodules::\n> > + When linkgit:git-status[1] is run in a superproject with\n> > + submodules, a status subprocess is spawned for every submodule.\n> > + This option specifies the number of submodule status subprocesses\n> > + to run in parallel. If unset, it defaults to 1.\n>\n> As I said, I am not sure .parallelSubmodules per command makes\n> much sense. \"If unset, it defaults to\" sounds a bit redundant (if\n> it is explicitly set, the default value should not matter).\n\nack.\n\n> > @@ -138,6 +153,9 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n> > struct cache_entry *ce = istate->cache[i];\n> > int changed;\n> > unsigned dirty_submodule = 0;\n> > + int defer_submodule_status = revs && revs->repo &&\n> > + !strcmp(revs->repo->gitdir, \".git\");\n>\n> What is this overly deeply indented comparison with \".git\" doing?\n> What are we checking and why? Is that something we need to do for\n> each and every active_cache[] entry, or isn't it constant over the\n> loop?\n\nIt is checking to see whether we are in the superproject or not, since\nit doesn't make sense to parallelize status in a submodule subprocess.\nIt doesn't need to be in the loop though -- will move out.\n\n> > + int ignore_untracked_in_submodules;\n> >\n> > if (diff_can_quit_early(&revs->diffopt))\n> > break;\n> > @@ -269,11 +287,36 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n> > }\n> >\n> > changed = match_stat_with_submodule(&revs->diffopt, ce, &st,\n> > + ce_option, &dirty_submodule,\n> > + &defer_submodule_status,\n> > + &ignore_untracked_in_submodules);\n> > newmode = ce_mode_from_stat(ce, st.st_mode);\n> > + if (defer_submodule_status) {\n> > + struct string_list_item *item =\n> > + string_list_append(&submodules, ce->name);\n> > + struct submodule_status_util *util = xmalloc(sizeof(*util));\n> > + util->changed = changed;\n> > + util->dirty_submodule = 0;\n> > + util->ignore_untracked = ignore_untracked_in_submodules;\n> > + util->newmode = newmode;\n> > + util->ce = ce;\n> > + item->util = util;\n> > + continue;\n>\n> This makes me wonder if defer_submodule_status should be a string\n> list that receives the punted submodules---iow, don't these details\n> belong to match_stat_with_submodule() rather than its caller here?\n\nThat makes sense. I can definitely clean up this section and\nmatch_stat_with_submodules() more.\n\n>\n> Even better may be to start a new child task for the submodule here,\n> letting it work while the parent process moves on to the next entry\n> in the active_cache[]. I do not know if you thought about doing it\n> that way.\n\nI think the implementation complexity of doing it this way would be\nvery high -- basically reimplementing run_processes_parallel(), but\nwithin this loop. And the benefit of doing so I think is not worth it -- at\nthat point, I might as well parallelize the entire function.\n\n> > + for (int i = 0; i < submodules.nr; i++) {\n>\n> We still do not allow \"for (type var = init; ...)\" if I am not\n> mistaken. Check the coding guidelines.\n\nack.\n"},{"id":"463670","messageId":"CAFySSZAM4Ae2AoBMj1BLdpdaNkfTQkFdT--QBc6T=4VcrcnJDQ@mail.gmail.com","threadId":"58499","inReplyTo":"88b194d9-7c78-c12e-1c29-95f768db7772@gmail.com","subject":"Re: [PATCH 4/4] diff-lib: parallelize run_diff_files for submodules","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-09-26T19:22:05Z","receivedAt":"2022-09-26T19:22:20Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"> I suspect in the future we may want to parallelize other commands for\n> submodules in which case a more general name such as submodules.threads\n> might be a better choice. The speed up in the cover letter is\n> impressive, could this be safely enabled by default?\n\nUnfortunately not. To reiterate the answer I gave to Avar:\nIn my cover letter, I noted that with too many processes, status starts to\nslow down (but is still better than the baseline). This is because the\nexpensive part of status is IO, specifically reading objects from the index.\nMuch of the speedup of this patch comes from taking advantage of the\nability to do parallel reads on an SSD, rather than splitting up the work\nof status. However, this doesn't work with an HDD, so status may\nactually be slower than baseline with multiple processes since there is\nnow scheduling/switching overhead.\n\n>\n> > index fcf9c85947..c5147a7952 100644\n> > --- a/builtin/commit.c\n> > +++ b/builtin/commit.c\n> > @@ -1468,6 +1468,12 @@ static int git_status_config(const char *k, const char *v, void *cb)\n> > s->detect_rename = git_config_rename(k, v);\n> > return 0;\n> > }\n> > + if (!strcmp(k, \"status.parallelsubmodules\")) {\n> > + s->parallel_jobs_submodules = git_config_int(k, v);\n> > + if (s->parallel_jobs_submodules < 0)\n> > + die(_(\"status.parallelsubmodules cannot be negative\"));\n>\n> What does a value of zero mean?\n\nLooking at my code I set it to 1 if it's zero, but I should update the logic to\nsomething more reasonable as Junio suggested.\n\n>\n> > diff --git a/diff-lib.c b/diff-lib.c\n> > index 2e148b79e6..ec745755fc 100644\n> > --- a/diff-lib.c\n> > +++ b/diff-lib.c\n>\n> > -int run_diff_files(struct rev_info *revs, unsigned int option)\n> > +int run_diff_files(struct rev_info *revs, unsigned int option, int parallel_jobs)\n>\n> Another possibility would be to add a member to struct diff_opts, rather\n> than changing the function signature here, I'm wondering what the trade\n> offs of the two approaches are. Also seeing all the callers from other\n> commands being changed made me wonder if they would benefit from\n> parallelizing submodules as well. There aren't any tests - could we use\n> GIT_TRACE2 to check that we are running the submodule diffs in parallel?\n\nAdding the option to rev_info seems like the best way forward. I neglected to\nwrite tests for this patch since the parallelism comes from\nrun_processes_parallel() which already has tests for that. But maybe it is a\ngood idea to also add a test with GIT_TRACE2 for a sanity check.\n\nThanks!\n"},{"id":"463696","messageId":"xmqqpmfh8lxw.fsf@gitster.g","threadId":"58499","inReplyTo":"CAFySSZA=tThoHdTY7+bMStvC=xeeyMiv4aVDYt-eNW2mQE10qg@mail.gmail.com","subject":"Re: [PATCH 1/4] run-command: add pipe_output to run_processes_parallel","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-27T04:45:31Z","receivedAt":"2022-09-27T04:45:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Calvin Wan writes:\n\n> You are correct that storing unbounded output doesn't seem like a good\n> idea. One idea I have is to parse output during the periodic collection rather\n> than waiting till the end. The other idea I had was to add another\n> \"git status --porcelain\" option that would only output the necessary\n> pieces of information so we wouldn't have to bother with worrying about\n> unbounded output.\n>\n> Any other thoughts as to how I can workaround this?\n\nI wonder if you can arrange not to let them make unbounded progress?\n\nIn order to run diff-files with path A B C D E ... where B and D are\nsubmodules and others are not submodules, you do not have to run and\nfinish comparison for B and D before you can do the comparison for\nother paths, in order to preserve the proper output order. You can\nstart child task for B and D and arrange so that they will run for\nany other submodules, and then you\n\n - run comparison for A. The child task for B and D may be running\n and starting to talk back to you, in which case their write may\n get stuck waiting for you to read from them, but that is OK, as\n you will read from them shortly.\n\n - wait for the child task for B. This is done by reading from the\n pipe connected to it and waiting for its death synchronously.\n The child task for D is still running and may be making progress,\n but you are not obligated to read its output to the end. You can\n postpone reading to conserve memory and it will fill the pipe and\n stall automatically. Then accept the result for B.\n\n - run comparison for C.\n\n - wait for the child task for D. You may have some data read\n already while dealing with B, but you may still have some reading\n to do at this point. Let it finish synchronously.\n\n - run comparison for E.\n\netc., perhaps?\n\n"},{"id":"463703","messageId":"220927.86ill9yymv.gmgdl@evledraar.gmail.com","threadId":"58499","inReplyTo":"CAFySSZA=tThoHdTY7+bMStvC=xeeyMiv4aVDYt-eNW2mQE10qg@mail.gmail.com","subject":"Re: [PATCH 1/4] run-command: add pipe_output to run_processes_parallel","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-27T09:05:16Z","receivedAt":"2022-09-27T09:06:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Sep 26 2022, Calvin Wan wrote:\n\n>> * Why are we configuring an API behaviour via a global variable in\n>> 21st century?\n>\n> I was mimicking how \"ungroup\" worked, but now that Avar mentions\n> that pattern was for a quick regression fix, I can fix it to pass it in as a\n> parameter.\n\nI don't know if you started this work or not, but I had a WIP rebase of\nthose on-list patches lying around in my tree, which I'm rebasing on\n\"master\" currently.\n\nSome of it's a bit tricky with the in-tree activity since then, I'll\nsend them in when they're ready, maybe you already did your own rebasing\nof them, or maybe it helps & you'd like to base a re-submission on top\nof them.\n\n"},{"id":"463705","messageId":"220927.86edvxytla.gmgdl@evledraar.gmail.com","threadId":"58499","inReplyTo":"CAFySSZB=bPWpK25Gwtm06cOyupGeUYo6qLC4PWPwaS2pNn22hA@mail.gmail.com","subject":"Re: [PATCH 1/4] run-command: add pipe_output to run_processes_parallel","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-27T10:52:34Z","receivedAt":"2022-09-27T10:55:56Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Sep 26 2022, Calvin Wan wrote:\n\n>> On the implementation:\n>>\n>> > + * If the \"pipe_output\" option is specified, the output will be piped\n>> > + * to task_finished_fn in the \"struct strbuf *out\" variable. The output\n>> > + * will still be printed unless the callback resets the strbuf. The\n>> > + * \"pipe_output\" option can be enabled by setting the global\n>> > + * \"run_processes_parallel_pipe_output\" to \"1\" before invoking\n>> > + * run_processes_parallel(), it will be set back to \"0\" as soon as the\n>> > + * API reads that setting.\n>>\n>> ...okey, but...\n>>\n>> > +static int task_finished_pipe_output(int result,\n>> > + struct strbuf *err,\n>> > + void *pp_cb,\n>> > + void *pp_task_cb)\n>> > +{\n>> > + if (err && pipe_output) {\n>> > + fprintf(stderr, \"%s\", err->buf);\n>> > + strbuf_reset(err);\n>>\n>> ...my memory's hazy, and I haven't re-logged in any detail, but is it\n>> really the API interface here that the \"output\" callback function is\n>> responsible for resetting the strbuf that the API gives to it?\n>>\n>> That seems backwards to me, and e.g. a look at \"start_failure\" shows\n>> that we strbuf_reset() the \"err\".\n>>\n>> What's the point of doing it in the API consumer? If it doesn't do it\n>> we'll presumably keep accumulating output. Is there a use-case for that?\n>>\n>> Or perhaps it's not needed & this is really just misleading boilerplate?\n>\n> Ultimately it is not needed -- I added it as an example to showcase that\n> the output is correctly being piped to \"task_finished_pipe_output\". The\n> reset is necessary in this case to prevent the output from being printed\n> twice. I'm not sure how exactly else I would go about testing \"pipe_output\".\n\nIf that's the intent then having that reset there seems to me to be\ndoing the exact opposite of what you want.\n\nIf the API is broken and passing the output along twice without clearing\nit in-between the two calls your strbuf_reset() would be sweeping that\nissue under the rug, that API brokenness would be \"repaired\" by your\ntest.\n\nWhereas if you remove the strbuf_reset() it should behave as it does\nnow, and if it doesn't the API itself is broken, i.e. after calling the\ncallback it should be resetting the buffer.\n"},{"id":"463747","messageId":"CAFySSZBSAW=zea8UoMiaQkf8rdJUBGHDYZQFkPLW7mRSciS-FA@mail.gmail.com","threadId":"58499","inReplyTo":"220927.86ill9yymv.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/4] run-command: add pipe_output to run_processes_parallel","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-09-27T17:55:32Z","receivedAt":"2022-09-27T17:56:05Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"> I don't know if you started this work or not, but I had a WIP rebase of\n> those on-list patches lying around in my tree, which I'm rebasing on\n> \"master\" currently.\n>\n> Some of it's a bit tricky with the in-tree activity since then, I'll\n> send them in when they're ready, maybe you already did your own rebasing\n> of them, or maybe it helps & you'd like to base a re-submission on top\n> of them.\n\nI have not started on this part yet. Do you have an estimate as to when\nyou're planning on submitting your rebase? I'm also considering not using\nmy pipe_output option and going a different route since there is the\nissue of dealing with potentially unbounded output.\n"},{"id":"463748","messageId":"CAFySSZC1DMdnCpi9enDG0=NDb+OU8jufHsq4WkjoN3mp1SZT8w@mail.gmail.com","threadId":"58499","inReplyTo":"xmqqpmfh8lxw.fsf@gitster.g","subject":"Re: [PATCH 1/4] run-command: add pipe_output to run_processes_parallel","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-09-27T18:10:49Z","receivedAt":"2022-09-27T18:11:09Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"> I wonder if you can arrange not to let them make unbounded progress?\n>\n> In order to run diff-files with path A B C D E ... where B and D are\n> submodules and others are not submodules, you do not have to run and\n> finish comparison for B and D before you can do the comparison for\n> other paths, in order to preserve the proper output order. You can\n> start child task for B and D and arrange so that they will run for\n> any other submodules, and then you\n\nThere is no need to preserve proper output order, as the output is\nsorted at the end.\n\n> - run comparison for A. The child task for B and D may be running\n> and starting to talk back to you, in which case their write may\n> get stuck waiting for you to read from them, but that is OK, as\n> you will read from them shortly.\n>\n> - wait for the child task for B. This is done by reading from the\n> pipe connected to it and waiting for its death synchronously.\n> The child task for D is still running and may be making progress,\n> but you are not obligated to read its output to the end. You can\n> postpone reading to conserve memory and it will fill the pipe and\n> stall automatically. Then accept the result for B.\n>\n> - run comparison for C.\n>\n> - wait for the child task for D. You may have some data read\n> already while dealing with B, but you may still have some reading\n> to do at this point. Let it finish synchronously.\n>\n> - run comparison for E.\n>\n> etc., perhaps?\n\nI understand the idea you're suggesting and I think it would work,\nbut I'm worried about the overhead this would create. I would rather\nimplement a separate \"git status --porcelain\" output for just this\nsubmodule case so 1. we would not have to worry about unbounded\noutput and 2. both the output parsing and the command could be\noptimized. In parse_status_porcelain, the function returns early if\nthe submodule is found to have untracked and modified changes.\nThis early termination can happen on the command side, rather\nthan the parsing side.\n"},{"id":"463750","messageId":"YzNDj54cylRzu2Fj@google.com","threadId":"58499","inReplyTo":"20220922232947.631309-5-calvinwan@google.com","subject":"Re: [PATCH 4/4] diff-lib: parallelize run_diff_files for submodules","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2022-09-27T18:40:15Z","receivedAt":"2022-09-27T18:40:29Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Thu, Sep 22, 2022 at 11:29:47PM +0000, Calvin Wan wrote:\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index fcf9c85947..c5147a7952 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1468,6 +1468,12 @@ static int git_status_config(const char *k, const char *v, void *cb)\n> \t\ts->detect_rename = git_config_rename(k, v);\n> \t\treturn 0;\n> \t}\n> +\tif (!strcmp(k, \"status.parallelsubmodules\")) {\n> +\t\ts->parallel_jobs_submodules = git_config_int(k, v);\n> +\t\tif (s->parallel_jobs_submodules < 0)\n> +\t\t\tdie(_(\"status.parallelsubmodules cannot be negative\"));\n\nSeems odd to have this check when all through the rest of the code\nyou're happily setting parallel_jobs=-1.\n\nAlthough I don't think I disagree with the check ;) rather, I disagree\nwith the semantics used elsewhere for this magic -1 value.\n\n> +\t\treturn 0;\n> +\t}\n> \treturn git_diff_ui_config(k, v, NULL);\n> }\n> \n> diff --git a/diff-lib.c b/diff-lib.c\n> index 2e148b79e6..ec745755fc 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -65,14 +65,19 @@ static int check_removed(const struct index_state *istate, const struct cache_en\n> * Return 1 when changes are detected, 0 otherwise. If the DIRTY_SUBMODULES\n> * option is set, the caller does not only want to know if a submodule is\n> * modified at all but wants to know all the conditions that are met (new\n> - * commits, untracked content and/or modified content).\n> + * commits, untracked content and/or modified content). If\n> + * defer_submodule_status bit is set, dirty_submodule will be left to the\n> + * caller to set. defer_submodule_status can also be set to 0 in this\n> + * function if there is no need to check if the submodule is modified.\n> */\n\nShould the defer optimization be moved to its own pack? I think it is\nsomewhat orthogonal to deciding whether to parallelize or not, yes?\n\n> static int match_stat_with_submodule(struct diff_options *diffopt,\n> \t\t\t\t const struct cache_entry *ce,\n> \t\t\t\t struct stat *st, unsigned ce_option,\n> -\t\t\t\t unsigned *dirty_submodule)\n> +\t\t\t\t unsigned *dirty_submodule, int *defer_submodule_status,\n> +\t\t\t\t\t int *ignore_untracked_in_submodules)\n> {\n> \tint changed = ie_match_stat(diffopt->repo->index, ce, st, ce_option);\n> +\tint defer = 0;\n> \tstruct diff_flags orig_flags = diffopt->flags;\n> \tif (!S_ISGITLINK(ce->ce_mode))\n> \t\tgoto cleanup;\n> @@ -83,11 +88,20 @@ static int match_stat_with_submodule(struct diff_options *diffopt,\n> \t\tgoto cleanup;\n> \t}\n> \tif (!diffopt->flags.ignore_dirty_submodules &&\n> -\t\t(!changed || diffopt->flags.dirty_submodules))\n> +\t\t(!changed || diffopt->flags.dirty_submodules)) {\n> +\t\tif (defer_submodule_status && *defer_submodule_status) {\n> +\t\t\tdefer = 1;\n> +\t\t\t*ignore_untracked_in_submodules =\n> +\t\t\t\t\t\t\tdiffopt->flags.ignore_untracked_in_submodules;\n> +\t\t} else {\n> \t\t\t*dirty_submodule = is_submodule_modified(ce->name,\n> \t\t\t\t\t\t\tdiffopt->flags.ignore_untracked_in_submodules);\n> +\t\t}\n> +\t}\n> cleanup:\n> \tdiffopt->flags = orig_flags;\n> +\tif (defer_submodule_status)\n> +\t\t*defer_submodule_status = defer;\n> \treturn changed;\n> }\n> \n> @@ -117,7 +131,7 @@ static void finish_run_diff_files(struct rev_info *revs,\n> \t\t\t ce->name, 0, dirty_submodule);\n> }\n> \n> -int run_diff_files(struct rev_info *revs, unsigned int option)\n> +int run_diff_files(struct rev_info *revs, unsigned int option, int parallel_jobs)\n\nThe meaning of 'parallel_jobs' isn't documented anywhere in this patch\nthat I can see, but all over we're setting it to some magic -1 value,\nright? What's that about?\n\n> {\n> \tint entries, i;\n> \tint diff_unmerged_stage = revs->max_count;\n> @@ -125,6 +139,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n> \t\t\t ? CE_MATCH_RACY_IS_DIRTY : 0);\n> \tuint64_t start = getnanotime();\n> \tstruct index_state *istate = revs->diffopt.repo->index;\n> +\tstruct string_list submodules = STRING_LIST_INIT_NODUP;\n> \n> \tdiff_set_mnemonic_prefix(&revs->diffopt, \"i/\", \"w/\");\n> \n> @@ -138,6 +153,9 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n> \t\tstruct cache_entry *ce = istate->cache[i];\n> \t\tint changed;\n> \t\tunsigned dirty_submodule = 0;\n> +\t\tint defer_submodule_status = revs && revs->repo &&\n> +\t\t\t\t\t\t\t!strcmp(revs->repo->gitdir, \".git\");\n> +\t\tint ignore_untracked_in_submodules;\n\nDoes this actually compile with -Wall? I thought it is no good to do an\ninline function call in the middle of the allocation block?\n\n> \n> \t\tif (diff_can_quit_early(&revs->diffopt))\n> \t\t\tbreak;\n> @@ -269,11 +287,36 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n> \t\t\t}\n> \n> \t\t\tchanged = match_stat_with_submodule(&revs->diffopt, ce, &st,\n> -\t\t\t\t\t\t\t ce_option, &dirty_submodule);\n> +\t\t\t\t\t\t\tce_option, &dirty_submodule,\n> +\t\t\t\t\t\t\t&defer_submodule_status,\n> +\t\t\t\t\t\t\t&ignore_untracked_in_submodules);\n> \t\t\tnewmode = ce_mode_from_stat(ce, st.st_mode);\n> +\t\t\tif (defer_submodule_status) {\n> +\t\t\t\tstruct string_list_item *item =\n> +\t\t\t\t\t\t\t\tstring_list_append(&submodules, ce->name);\n> +\t\t\t\tstruct submodule_status_util *util = xmalloc(sizeof(*util));\n> +\t\t\t\tutil->changed = changed;\n> +\t\t\t\tutil->dirty_submodule = 0;\n> +\t\t\t\tutil->ignore_untracked = ignore_untracked_in_submodules;\n> +\t\t\t\tutil->newmode = newmode;\n> +\t\t\t\tutil->ce = ce;\n> +\t\t\t\titem->util = util;\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> \t\t}\n> \t\tfinish_run_diff_files(revs, ce, istate, changed, dirty_submodule, newmode);\n> \t}\n> +\tif (submodules.nr > 0) {\n> +\t\tif (get_submodules_status(revs->repo, &submodules,\n> +\t\t\t\t\t\tparallel_jobs > 0 ? parallel_jobs : 1))\n\nWe're not doing a lookup in case of parallel_jobs<0, right? So why even\nbother having this special meaning for -1? It's undocumented in the\nheader for run_diff_files anyway. In fact, I'd expect parallel_jobs=-1\nto mean \"as many jobs as possible\" - but here you're using it just to\nmean 1.\n\nWhy not let parallel_jobs be unsigned, do special casing for 0 if you\nwant to use that to mean \"recommended # of jobs\", and otherwise take it\nat face value? Am I missing something in my skim over the patch?\n\n> +\t\t\tBUG(\"Submodule status failed\");\n> +\t\tfor (int i = 0; i < submodules.nr; i++) {\n> +\t\t\tstruct submodule_status_util *util = submodules.items[i].util;\n> +\n> +\t\t\tfinish_run_diff_files(revs, util->ce, NULL, util->changed,\n> +\t\t\t\t\t\t\tutil->dirty_submodule, util->newmode);\n> +\t\t}\n> +\t}\n> \tdiffcore_std(&revs->diffopt);\n> \tdiff_flush(&revs->diffopt);\n> \ttrace_performance_since(start, \"diff-files\");\n> @@ -321,7 +364,7 @@ static int get_stat_data(const struct index_state *istate,\n> \t\t\treturn -1;\n> \t\t}\n> \t\tchanged = match_stat_with_submodule(diffopt, ce, &st,\n> -\t\t\t\t\t\t 0, dirty_submodule);\n> +\t\t\t\t\t\t 0, dirty_submodule, NULL, NULL);\n> \t\tif (changed) {\n> \t\t\tmode = ce_mode_from_stat(ce, st.st_mode);\n> \t\t\toid = null_oid();\n> diff --git a/diff.h b/diff.h\n> index 8ae18e5ab1..5a6a615381 100644\n> --- a/diff.h\n> +++ b/diff.h\n> @@ -627,7 +627,7 @@ void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb);\n> #define DIFF_SILENT_ON_REMOVED 01\n> /* report racily-clean paths as modified */\n> #define DIFF_RACY_IS_MODIFIED 02\n> -int run_diff_files(struct rev_info *revs, unsigned int option);\n> +int run_diff_files(struct rev_info *revs, unsigned int option, int parallel_jobs);\n\nSee above; probably documenting parallel_jobs should happen here.\n\n> \n> #define DIFF_INDEX_CACHED 01\n> #define DIFF_INDEX_MERGE_BASE 02\n> diff --git a/submodule.c b/submodule.c\n> index 91213ba83c..15729bb327 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1362,6 +1362,20 @@ int submodule_touches_in_range(struct repository *r,\n> \treturn ret;\n> }\n> \n\nI actually might like to see the conversion to using\nrun_processes_parallel happen in an earlier patch, with jobs=1. Might\nmake the review load easier.\n\nThat said, in previous series which took that approach, because it\nworked correctly with jobs=1 we missed a bug in the writing of the new\n*_task_finished() callback.... Maybe that doesn't apply here as the\njobs=N patch would follow on in the same series, so you'd test it right\naway?\n\n> +struct submodule_parallel_status {\n> +\tint index_count;\n> +\tint result;\n> +\n> +\tstruct string_list *submodule_names;\n> +\tstruct repository *r;\n> +\n> +\t/* Pending statuses by OIDs */\n> +\tstruct status_task **oid_status_tasks;\n> +\tint oid_status_tasks_nr, oid_status_tasks_alloc;\n> +};\n> +\n> +#define SPS_INIT { 0 }\n> +\n> struct submodule_parallel_fetch {\n> \t/*\n> \t * The index of the last index entry processed by\n> @@ -1444,6 +1458,12 @@ struct fetch_task {\n> \tstruct oid_array *commits; /* Ensure these commits are fetched */\n> };\n> \n> +struct status_task {\n> +\tstruct repository *repo;\n> +\tconst char *path;\n> +\tint ignore_untracked;\n> +};\n> +\n> /**\n> * When a submodule is not defined in .gitmodules, we cannot access it\n> * via the regular submodule-config. Create a fake submodule, which we can\n> @@ -1547,6 +1567,41 @@ static struct fetch_task *fetch_task_create(struct submodule_parallel_fetch *spf\n> \treturn NULL;\n> }\n> \n> +static struct status_task *\n> +get_status_task_from_index(struct submodule_parallel_status *sps,\n> +\t\t\t struct strbuf *err)\nThe spacing of these long function signatures feels odd; I'd much rather\nsee something like:\n\nstatic struct status_task* get_status_task_from_index(\n (args)\n\nPlus, as this get_status_task_from_index() exists only to be called by\nget_next_submodule_status(), I'd find it easier to read if this function\nwas closer to that one. But that's a pretty tiny nit ;)\n\n> +{\n> +\tfor (; sps->index_count < sps->submodule_names->nr; sps->index_count++) {\n> +\t\tstruct submodule_status_util *util = sps->submodule_names->items[sps->index_count].util;\n> +\t\tconst struct cache_entry *ce = util->ce;\n> +\t\tstruct status_task *task;\n> +\t\tstruct strbuf buf = STRBUF_INIT;\n> +\t\tconst char *git_dir;\n> +\n> +\t\tstrbuf_addf(&buf, \"%s/.git\", ce->name);\n> +\t\tgit_dir = read_gitfile(buf.buf);\n> +\t\tif (!git_dir)\n> +\t\t\tgit_dir = buf.buf;\n> +\t\tif (!is_git_directory(git_dir)) {\n> +\t\t\tif (is_directory(git_dir))\n> +\t\t\t\tdie(_(\"'%s' not recognized as a git repository\"), git_dir);\n> +\t\t\tstrbuf_release(&buf);\n> +\t\t\t/* The submodule is not checked out, so it is not modified */\n> +\t\t\tutil->dirty_submodule = 0;\n> +\t\t\tcontinue;\n> +\t\t}\n> +\t\tstrbuf_release(&buf);\n> +\n> +\t\ttask = xmalloc(sizeof(*task));\n> +\t\tmemset(task, 0, sizeof(*task));\n> +\t\ttask->path = ce->name;\n> +\t\ttask->ignore_untracked = util->ignore_untracked;\n> +\t\tsps->index_count++;\n> +\t\treturn task;\n> +\t}\n> +\treturn NULL;\n> +}\n> +\n> static struct fetch_task *\n> get_fetch_task_from_index(struct submodule_parallel_fetch *spf,\n> \t\t\t struct strbuf *err)\n> @@ -1744,6 +1799,16 @@ static int fetch_start_failure(struct strbuf *err,\n> \treturn 0;\n> }\n> \n> +static int status_start_failure(struct strbuf *err,\n> +\t\t\t void *cb, void *task_cb)\n> +{\n> +\tstruct submodule_parallel_status *sps = cb;\n> +\n> +\tsps->result = 1;\n> +\treturn 0;\n> +}\n> +\n> +\n> static int commit_missing_in_sub(const struct object_id *oid, void *data)\n> {\n> \tstruct repository *subrepo = data;\n> @@ -1790,6 +1855,30 @@ static int parse_status_porcelain(char *buf, unsigned *dirty_submodule, int igno\n> \treturn 0;\n> }\n> \n> +static int status_finish(int retvalue, struct strbuf *err,\n> +\t\t\tvoid *cb, void *task_cb)\n> +{\n> +\tstruct submodule_parallel_status *sps = cb;\n> +\tstruct status_task *task = task_cb;\n> +\tstruct string_list list = STRING_LIST_INIT_DUP;\n> +\tstruct string_list_item *it = string_list_lookup(sps->submodule_names,\n> +\t\t\t\t\t\t\t\t\t\t\ttask->path);\n> +\tstruct submodule_status_util *util = it->util;\n> +\n> +\tint i;\n> +\n> +\tstring_list_split(&list, err->buf, '\\n', -1);\n> +\n> +\tfor (i = 0; i < list.nr; i++) {\n> +\t\tif (parse_status_porcelain(list.items[i].string,\n> +\t\t\t\t\t\t\t&util->dirty_submodule, util->ignore_untracked))\n> +\t\t\tbreak;\n> +\t}\n> +\tstrbuf_reset(err);\n> +\n> +\treturn 0;\n> +}\n> +\n> static int fetch_finish(int retvalue, struct strbuf *err,\n> \t\t\tvoid *cb, void *task_cb)\n> {\n> @@ -1943,6 +2032,59 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n> \treturn dirty_submodule;\n> }\n> \n> +static int get_next_submodule_status(struct child_process *cp,\n> +\t\t\t\t\t\tstruct strbuf *err, void *data, void **task_cb)\n> +{\n> +\tstruct submodule_parallel_status *sps = data;\n> +\tstruct status_task *task = get_status_task_from_index(sps, err);\n> +\n> +\tint ignore_untracked;\n> +\n> +\tif (!task) {\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tignore_untracked = task->ignore_untracked;\n> +\n> +\tchild_process_init(cp);\n> +\tprepare_submodule_repo_env_in_gitdir(&cp->env);\n> +\n> +\tstrvec_init(&cp->args);\n> +\tstrvec_pushl(&cp->args, \"status\", \"--porcelain=2\", NULL);\n> +\tif (ignore_untracked)\n> +\t\tstrvec_push(&cp->args, \"-uno\");\n> +\n> +\tprepare_submodule_repo_env(&cp->env);\n> +\tcp->git_cmd = 1;\n> +\tcp->no_stdin = 1;\n> +\tcp->dir = task->path;\n> +\t*task_cb = task;\n> +\treturn 1;\n> +}\n> +\n> +int get_submodules_status(struct repository *r,\n> +\t\t\tstruct string_list *submodules,\n> +\t\t int max_parallel_jobs)\n> +{\n> +\tstruct submodule_parallel_status sps = SPS_INIT;\n> +\n> +\tsps.r = r;\n> +\n> +\tif (repo_read_index(r) < 0)\n> +\t\tdie(_(\"index file corrupt\"));\n> +\n> +\tsps.submodule_names = submodules;\n> +\tstring_list_sort(sps.submodule_names);\n> +\trun_processes_parallel_pipe_output = 1;\n> +\trun_processes_parallel_tr2(max_parallel_jobs,\n> +\t\t\t\tget_next_submodule_status,\n> +\t\t\t\tstatus_start_failure,\n> +\t\t\t\tstatus_finish,\n> +\t\t\t\t&sps,\n> +\t\t\t\t\"submodule\", \"parallel/status\");\n> +\treturn sps.result;\n> +}\n> +\n> int submodule_uses_gitfile(const char *path)\n> {\n> \tstruct child_process cp = CHILD_PROCESS_INIT;\n> diff --git a/submodule.h b/submodule.h\n> index bfaa9da186..18a42c64ce 100644\n> --- a/submodule.h\n> +++ b/submodule.h\n> @@ -41,6 +41,12 @@ struct submodule_update_strategy {\n> \t.type = SM_UPDATE_UNSPECIFIED, \\\n> }\n> \n> +struct submodule_status_util {\n> +\tint changed, ignore_untracked;\n> +\tunsigned dirty_submodule, newmode;\n> +\tstruct cache_entry *ce;\n> +};\n> +\n> int is_gitmodules_unmerged(struct index_state *istate);\n> int is_writing_gitmodules_ok(void);\n> int is_staging_gitmodules_ok(struct index_state *istate);\n> @@ -94,6 +100,9 @@ int fetch_submodules(struct repository *r,\n> \t\t int command_line_option,\n> \t\t int default_option,\n> \t\t int quiet, int max_parallel_jobs);\n> +int get_submodules_status(struct repository *r,\n> +\t\t\t struct string_list *submodules,\n> +\t\t int max_parallel_jobs);\n> unsigned is_submodule_modified(const char *path, int ignore_untracked);\n> int submodule_uses_gitfile(const char *path);\n> \n> diff --git a/wt-status.c b/wt-status.c\n> index 867e3e417e..9864484f81 100644\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -615,7 +615,7 @@ static void wt_status_collect_changes_worktree(struct wt_status *s)\n> \trev.diffopt.rename_limit = s->rename_limit >= 0 ? s->rename_limit : rev.diffopt.rename_limit;\n> \trev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : rev.diffopt.rename_score;\n> \tcopy_pathspec(&rev.prune_data, &s->pathspec);\n> -\trun_diff_files(&rev, 0);\n> +\trun_diff_files(&rev, 0, s->parallel_jobs_submodules);\n> \trelease_revisions(&rev);\n> }\n> \n> @@ -1149,7 +1149,7 @@ static void wt_longstatus_print_verbose(struct wt_status *s)\n> \t\tsetup_work_tree();\n> \t\trev.diffopt.a_prefix = \"i/\";\n> \t\trev.diffopt.b_prefix = \"w/\";\n> -\t\trun_diff_files(&rev, 0);\n> +\t\trun_diff_files(&rev, 0, -1);\n> \t}\n> \trelease_revisions(&rev);\n> }\n> @@ -2544,7 +2544,7 @@ int has_unstaged_changes(struct repository *r, int ignore_submodules)\n> \t}\n> \trev_info.diffopt.flags.quick = 1;\n> \tdiff_setup_done(&rev_info.diffopt);\n> -\tresult = run_diff_files(&rev_info, 0);\n> +\tresult = run_diff_files(&rev_info, 0, -1);\n> \tresult = diff_result_code(&rev_info.diffopt, result);\n> \trelease_revisions(&rev_info);\n> \treturn result;\n> diff --git a/wt-status.h b/wt-status.h\n> index ab9cc9d8f0..2ea2317715 100644\n> --- a/wt-status.h\n> +++ b/wt-status.h\n> @@ -131,6 +131,7 @@ struct wt_status {\n> \tenum wt_status_format status_format;\n> \tstruct wt_status_state state;\n> \tstruct object_id oid_commit; /* when not Initial */\n> +\tint parallel_jobs_submodules;\n> \n> \t/* These are computed during processing of the individual sections */\n> \tint committable;\n> -- \n> 2.37.3.998.g577e59143f-goog\n> \n\nI feel like we're missing tests exercising status.parallelSubmodules=N?\n\n - Emily\n"},{"id":"463756","messageId":"220927.86fsgcy5j9.gmgdl@evledraar.gmail.com","threadId":"58499","inReplyTo":"CAFySSZBSAW=zea8UoMiaQkf8rdJUBGHDYZQFkPLW7mRSciS-FA@mail.gmail.com","subject":"Re: [PATCH 1/4] run-command: add pipe_output to run_processes_parallel","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-27T19:34:22Z","receivedAt":"2022-09-27T19:35:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Sep 27 2022, Calvin Wan wrote:\n\n>> I don't know if you started this work or not, but I had a WIP rebase of\n>> those on-list patches lying around in my tree, which I'm rebasing on\n>> \"master\" currently.\n>>\n>> Some of it's a bit tricky with the in-tree activity since then, I'll\n>> send them in when they're ready, maybe you already did your own rebasing\n>> of them, or maybe it helps & you'd like to base a re-submission on top\n>> of them.\n>\n> I have not started on this part yet. Do you have an estimate as to when\n> you're planning on submitting your rebase?\n\nI hacked this up today, it passes CI at least:\n\n\thttps://github.com/git/git/compare/master...avar:avar/hook-run-process-parallel-tty-regression-2-argument-passing\n\n> I'm also considering not using\n> my pipe_output option and going a different route since there is the\n> issue of dealing with potentially unbounded output.\n\nOkey, if it's not blocking a re-submission of yours then I'll definitely\nwait until after the RC to submit the above, at least, but if you'd like\nit earlier...\n\n"},{"id":"463760","messageId":"CAFySSZB3hp2WWk0bL77FBR91ueJ1eJFtuVCoyE-ooVT77Vo_vw@mail.gmail.com","threadId":"58499","inReplyTo":"220927.86fsgcy5j9.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/4] run-command: add pipe_output to run_processes_parallel","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-09-27T20:45:11Z","receivedAt":"2022-09-27T20:45:31Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"> Okey, if it's not blocking a re-submission of yours then I'll definitely\n> wait until after the RC to submit the above, at least, but if you'd like\n> it earlier...\n\nI just tested a solution where I add pipe_output_fn to\nparallel_processes instead of having it as a variable. Not only does\nit work but it also solves my unbounded output problem, so this is\nno longer blocked resubmission.\n\nThanks!\n"},{"id":"463770","messageId":"xmqqo7v01onp.fsf@gitster.g","threadId":"58499","inReplyTo":"CAFySSZC1DMdnCpi9enDG0=NDb+OU8jufHsq4WkjoN3mp1SZT8w@mail.gmail.com","subject":"Re: [PATCH 1/4] run-command: add pipe_output to run_processes_parallel","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-27T21:40:58Z","receivedAt":"2022-09-27T21:41:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Calvin Wan writes:\n\n> ... I would rather\n> implement a separate \"git status --porcelain\" output for just this\n> submodule case so 1. we would not have to worry about unbounded\n> output and 2. both the output parsing and the command could be\n> optimized.\n\nSounds good.\n"},{"id":"463790","messageId":"220928.86bkr0xd9w.gmgdl@evledraar.gmail.com","threadId":"58499","inReplyTo":"CAFySSZB3hp2WWk0bL77FBR91ueJ1eJFtuVCoyE-ooVT77Vo_vw@mail.gmail.com","subject":"Re: [PATCH 1/4] run-command: add pipe_output to run_processes_parallel","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-28T05:40:02Z","receivedAt":"2022-09-28T05:45:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Sep 27 2022, Calvin Wan wrote:\n\n>> Okey, if it's not blocking a re-submission of yours then I'll definitely\n>> wait until after the RC to submit the above, at least, but if you'd like\n>> it earlier...\n>\n> I just tested a solution where I add pipe_output_fn to\n> parallel_processes instead of having it as a variable. Not only does\n> it work but it also solves my unbounded output problem, so this is\n> no longer blocked resubmission.\n\nYou mean the internal \"struct parallel_processes\"? How do you get the\nparameter there, presumably by passing it to\nrun_processes_parallel{,_tr2}() as a new parameter?\n\nThe reason for why the \"ungroup\" wasn't added as a parameter at the time\nwas to avoid the churn of changing every single caller of the API.\n\nBut it should really be a \"parameter\", and doing it via a struct means\nadding such parameters doesn't need to change every single caller.\n\nThen we have outstanding WIP patches for the hook.[ch] API which needed\nto add two other parameters...\n\nSo I think first ripping off the band-aid of making it painless to\nextend the interface is the right thing to do, unless I've missed some\nway of doing it that you've just discovered...\n\n\n\n\n"},{"id":"463919","messageId":"CAFySSZDF_nxGJCjhi38nb5R3jtPBVqZbMCqKxmTagUqCbwx7rw@mail.gmail.com","threadId":"58499","inReplyTo":"220928.86bkr0xd9w.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/4] run-command: add pipe_output to run_processes_parallel","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-09-29T20:52:09Z","receivedAt":"2022-09-29T20:52:26Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"> You mean the internal \"struct parallel_processes\"? How do you get the\n> parameter there, presumably by passing it to\n> run_processes_parallel{,_tr2}() as a new parameter?\n\nYea I added it as a new parameter...\n\n> The reason for why the \"ungroup\" wasn't added as a parameter at the time\n> was to avoid the churn of changing every single caller of the API.\n>\n> But it should really be a \"parameter\", and doing it via a struct means\n> adding such parameters doesn't need to change every single caller.\n>\n> Then we have outstanding WIP patches for the hook.[ch] API which needed\n> to add two other parameters...\n>\n> So I think first ripping off the band-aid of making it painless to\n> extend the interface is the right thing to do, unless I've missed some\n> way of doing it that you've just discovered...\n\nIn that case my patch does depend on yours for resubmission, so\nit sounds like if I want to quickly resubmit then I should cherry-pick\nthe relevant commits from your WIP branch.\n"}]}