{"thread":{"id":"58603","subject":"[PATCH v2 0/4] submodule: parallelize diff","startedAt":"2022-10-11T23:26:22Z","lastAt":"2022-10-14T00:39:37Z","messageCount":11,"participants":["Calvin Wan","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":2,"patchTotal":4},"messages":[{"id":"464638","messageId":"20221011232604.839941-1-calvinwan@google.com","threadId":"58603","inReplyTo":"https://lore.kernel.org/git/20220922232947.631309-1-calvinwan@google.com/","subject":"[PATCH v2 0/4] submodule: parallelize diff","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-10-11T23:26:00Z","receivedAt":"2022-10-11T23:26:22Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"Changes since v1\n\nThis series is now rebased on top of Avar's run-command refactor\nseries[1], which allows new functions to be easily added to\nrun_parallel_processes without having to change other callers.\n\nThe config option has been renamed to submodule.diffJobs. This name\naccurately reflects what functionality is affected. While other APIs\nparse for config options from the initial command, this config option is\nparsed for in the diff-lib.c library. This is because there are so many\nentry points into run_diff_files resulting in signicant code changes\nrequired to pass in this config option.\n\nI also wanted to pose another question to list regarding defaults for\nparallel processes. For jobs that clearly scale with the number of\nprocesses (aka jobs that are mostly processor bound), it is obvious that\nsetting the default number of processes to the number of available cores\nis the most optimal option. However, this changes when the job is mostly\nI/O bound or has a combination of I/O and processing. Looking at my use\ncase for `status` on a cold cache (see below), we notice that increasing\nthe number of parallel processes speeds up status, but after a certain\nnumber, it actually starts slowing down. This is because `status` spends\nmost of its time reading from the index. While SSDs are able to do a\ncertain amount of reads in parallel, this limit can be significantly\nsmaller than the number of cores and going past that limit causes the\nread head to constantly swap, slowing down `status`. With an HDD,\nparallel reads aren't even possible and while I haven't tested my patch\non an HDD, I have a suspicion that adding multiple processes would\nprobably make `status` slower than the baseline. How should the default\nbe set then? In my case, the safe option would be to default it to 1,\nbut then many users wouldn't be able to discover this optimization.\nThere are also multiple places in the git documentation where we say\nthat setting a config option for a parallel process to 0 will result in\na \"reasonable amount\" of processes, which generally entails the number\nof available cores. Can this be problematic for other parallel processes\nthat spend a significant time in I/O? Should my option even have the\noption to set it to 0 given the pitalls?\n\nOriginal cover letter:\n\nWhen 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. While my initial\nintention was to speedup status, it turns out that much of the runtime\nspent on status is in diff-lib.c, resulting in speedups for many\ndifferent other commands that utilize this library.\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_opt 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\n[1] https://lore.kernel.org/git/cover-00.15-00000000000-20220930T111343Z-avarab@gmail.com/\n\nCalvin Wan (4):\n  run-command: add pipe_output_fn to run_processes_parallel_opts\n  submodule: move status parsing into function\n  diff-lib: refactor match_stat_with_submodule\n  diff-lib: parallelize run_diff_files for submodules\n\n Documentation/config/submodule.txt |  12 ++\n diff-lib.c                         | 108 ++++++++++++--\n run-command.c                      |  12 +-\n run-command.h                      |  22 +++\n submodule.c                        | 230 +++++++++++++++++++++++++----\n submodule.h                        |   9 ++\n t/helper/test-run-command.c        |  18 +++\n t/t0061-run-command.sh             |  65 ++++----\n t/t4027-diff-submodule.sh          |  19 +++\n t/t7506-status-submodule.sh        |  19 +++\n 10 files changed, 441 insertions(+), 73 deletions(-)\n\n-- \n2.38.0.rc1.362.ged0d419d3c-goog\n\n"},{"id":"464639","messageId":"20221011232604.839941-2-calvinwan@google.com","threadId":"58603","inReplyTo":"https://lore.kernel.org/git/20220922232947.631309-1-calvinwan@google.com/","subject":"[PATCH v2 1/4] run-command: add pipe_output_fn to run_processes_parallel_opts","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-10-11T23:26:01Z","receivedAt":"2022-10-11T23:26:35Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"Add pipe_output_fn as an optionally set function in\nrun_process_parallel_opts. If set, output from each child process is\npiped to the callback function to allow for separate parsing.\n\nSigned-off-by: Calvin Wan <calvinwan@google.com>\n---\n run-command.c               | 12 +++++--\n run-command.h               | 22 +++++++++++++\n t/helper/test-run-command.c | 18 ++++++++++\n t/t0061-run-command.sh      | 65 +++++++++++++++++++++++--------------\n 4 files changed, 90 insertions(+), 27 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex da02631933..c6090e4cb8 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1689,12 +1689,16 @@ static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout)\n \t}\n }\n \n-static void pp_output(struct parallel_processes *pp)\n+static void pp_output(struct parallel_processes *pp,\n+\t\t\tconst struct run_process_parallel_opts *opts)\n {\n \tint i = pp->output_owner;\n \n \tif (pp->children[i].state == GIT_CP_WORKING &&\n \t    pp->children[i].err.len) {\n+\t\tif (opts->pipe_output)\n+\t\t\topts->pipe_output(&pp->children[i].err, pp->data,\n+\t\t\t\t\t\tpp->children[i].data);\n \t\tstrbuf_write(&pp->children[i].err, stderr);\n \t\tstrbuf_reset(&pp->children[i].err);\n \t}\n@@ -1716,6 +1720,10 @@ static int pp_collect_finished(struct parallel_processes *pp,\n \n \t\tcode = finish_command(&pp->children[i].process);\n \n+\t\tif (opts->pipe_output)\n+\t\t\topts->pipe_output(&pp->children[i].err, pp->data,\n+\t\t\t\t\t\tpp->children[i].data);\n+\n \t\tif (opts->task_finished)\n \t\t\tcode = opts->task_finished(code, opts->ungroup ? NULL :\n \t\t\t\t\t\t   &pp->children[i].err, pp->data,\n@@ -1803,7 +1811,7 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts)\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\tpp_output(&pp, opts);\n \t\t}\n \t\tcode = pp_collect_finished(&pp, opts);\n \t\tif (code) {\ndiff --git a/run-command.h b/run-command.h\nindex 075bd9b9de..cb51c56ea6 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -440,6 +440,22 @@ typedef int (*start_failure_fn)(struct strbuf *out,\n \t\t\t\tvoid *pp_cb,\n \t\t\t\tvoid *pp_task_cb);\n \n+/**\n+ * This callback is periodically called while child processes are\n+ * running and also when a child process finishes.\n+ *\n+ * \"struct strbuf *out\" contains the output collected from pp_task_cb\n+ * since the last call of this function.\n+ *\n+ * pp_cb is the callback cookie as passed into run_processes_parallel,\n+ * pp_task_cb is the callback cookie as passed into get_next_task_fn.\n+ *\n+ * This function is incompatible with \"ungroup\"\n+ */\n+typedef void (*pipe_output_fn)(struct strbuf *out,\n+\t\t\t\tvoid *pp_cb,\n+\t\t\t\tvoid *pp_task_cb);\n+\n /**\n  * This callback is called on every child process that finished processing.\n  *\n@@ -493,6 +509,12 @@ struct run_process_parallel_opts\n \t */\n \tstart_failure_fn start_failure;\n \n+\t/**\n+\t * pipe_output: See pipe_output_fn() above. This should be\n+\t * NULL unless process specific output is needed\n+\t */\n+\tpipe_output_fn pipe_output;\n+\n \t/**\n \t * task_finished: See task_finished_fn() above. This can be\n \t * NULL to omit any special handling.\ndiff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c\nindex 46bac2bb70..d3c3df7960 100644\n--- a/t/helper/test-run-command.c\n+++ b/t/helper/test-run-command.c\n@@ -52,6 +52,18 @@ static int no_job(struct child_process *cp,\n \treturn 0;\n }\n \n+static void pipe_output(struct strbuf *out,\n+\t\t\t\tvoid *pp_cb,\n+\t\t\t\tvoid *pp_task_cb)\n+{\n+\tfprintf(stderr, \"%s\", out->buf);\n+\t/*\n+\t * Resetting output to show that piped output would print the\n+\t * same as other tests without the pipe_output() function set\n+\t */\n+\tstrbuf_reset(out);\n+}\n+\n static int task_finished(int result,\n \t\t\t struct strbuf *err,\n \t\t\t void *pp_cb,\n@@ -439,6 +451,12 @@ int cmd__run_command(int argc, const char **argv)\n \t\topts.ungroup = 1;\n \t}\n \n+\tif (!strcmp(argv[1], \"--pipe-output\")) {\n+\t\targv += 1;\n+\t\targc -= 1;\n+\t\topts.pipe_output = pipe_output;\n+\t}\n+\n \tjobs = atoi(argv[2]);\n \tstrvec_clear(&proc.args);\n \tstrvec_pushv(&proc.args, (const char **)argv + 3);\ndiff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\nindex 19af082750..feabb3717b 100755\n--- a/t/t0061-run-command.sh\n+++ b/t/t0061-run-command.sh\n@@ -129,11 +129,14 @@ Hello\n World\n EOF\n \n-test_expect_success 'run_command runs in parallel with more jobs available than tasks' '\n-\ttest-tool run-command run-command-parallel 5 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" >out 2>err &&\n-\ttest_must_be_empty out &&\n-\ttest_cmp expect err\n-'\n+for opt in '' '--pipe-output'\n+do\n+\ttest_expect_success \"run_command runs in parallel with more jobs available than tasks $opt\" '\n+\t\ttest-tool run-command run-command-parallel 5 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" >out 2>err &&\n+\t\ttest_must_be_empty out &&\n+\t\ttest_cmp expect err\n+\t'\n+done\n \n test_expect_success 'run_command runs ungrouped in parallel with more jobs available than tasks' '\n \ttest-tool run-command --ungroup run-command-parallel 5 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" >out 2>err &&\n@@ -141,11 +144,14 @@ 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 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\" >out 2>err &&\n-\ttest_must_be_empty out &&\n-\ttest_cmp expect err\n-'\n+for opt in '' '--pipe-output'\n+do\n+\ttest_expect_success \"run_command runs in parallel with as many jobs as tasks $opt\" '\n+\t\ttest-tool run-command run-command-parallel 4 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" >out 2>err &&\n+\t\ttest_must_be_empty out &&\n+\t\ttest_cmp expect err\n+\t'\n+done\n \n test_expect_success 'run_command runs ungrouped in parallel with as many jobs as tasks' '\n \ttest-tool run-command --ungroup run-command-parallel 4 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" >out 2>err &&\n@@ -153,11 +159,14 @@ 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 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\" >out 2>err &&\n-\ttest_must_be_empty out &&\n-\ttest_cmp expect err\n-'\n+for opt in '' '--pipe-output'\n+do\n+\ttest_expect_success \"run_command runs in parallel with more tasks than jobs available $opt\" '\n+\t\ttest-tool run-command run-command-parallel 3 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" >out 2>err &&\n+\t\ttest_must_be_empty out &&\n+\t\ttest_cmp expect err\n+\t'\n+done\n \n test_expect_success 'run_command runs ungrouped in parallel with more tasks than jobs available' '\n \ttest-tool run-command --ungroup run-command-parallel 3 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" >out 2>err &&\n@@ -174,11 +183,14 @@ preloaded output of a child\n asking for a quick stop\n EOF\n \n-test_expect_success 'run_command is asked to abort gracefully' '\n-\ttest-tool run-command run-command-abort 3 false >out 2>err &&\n-\ttest_must_be_empty out &&\n-\ttest_cmp expect err\n-'\n+for opt in '' '--pipe-output'\n+do\n+\ttest_expect_success \"run_command is asked to abort gracefully $opt\" '\n+\t\ttest-tool run-command run-command-abort 3 false >out 2>err &&\n+\t\ttest_must_be_empty out &&\n+\t\ttest_cmp expect err\n+\t'\n+done\n \n test_expect_success 'run_command is asked to abort gracefully (ungroup)' '\n \ttest-tool run-command --ungroup run-command-abort 3 false >out 2>err &&\n@@ -190,11 +202,14 @@ cat >expect <<-EOF\n no further jobs available\n EOF\n \n-test_expect_success 'run_command outputs ' '\n-\ttest-tool run-command run-command-no-jobs 3 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" >out 2>err &&\n-\ttest_must_be_empty out &&\n-\ttest_cmp expect err\n-'\n+for opt in '' '--pipe-output'\n+do\n+\ttest_expect_success \"run_command outputs $opt\" '\n+\t\ttest-tool run-command run-command-no-jobs 3 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" >out 2>err &&\n+\t\ttest_must_be_empty out &&\n+\t\ttest_cmp expect err\n+\t'\n+done\n \n test_expect_success 'run_command outputs (ungroup) ' '\n \ttest-tool run-command --ungroup run-command-no-jobs 3 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" >out 2>err &&\n-- \n2.38.0.rc1.362.ged0d419d3c-goog\n\n"},{"id":"464640","messageId":"20221011232604.839941-3-calvinwan@google.com","threadId":"58603","inReplyTo":"https://lore.kernel.org/git/20220922232947.631309-1-calvinwan@google.com/","subject":"[PATCH v2 2/4] submodule: move status parsing into function","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-10-11T23:26:02Z","receivedAt":"2022-10-11T23:26:37Z","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 <calvinwan@google.com>\n---\n submodule.c | 71 +++++++++++++++++++++++++++++------------------------\n 1 file changed, 39 insertions(+), 32 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 72b295b87b..a3410ed8f0 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1864,6 +1864,43 @@ int fetch_submodules(struct repository *r,\n \treturn spf.result;\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 unsigned is_submodule_modified(const char *path, int ignore_untracked)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n@@ -1900,39 +1937,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.38.0.rc1.362.ged0d419d3c-goog\n\n"},{"id":"464641","messageId":"20221011232604.839941-4-calvinwan@google.com","threadId":"58603","inReplyTo":"https://lore.kernel.org/git/20220922232947.631309-1-calvinwan@google.com/","subject":"[PATCH v2 3/4] diff-lib: refactor match_stat_with_submodule","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-10-11T23:26:03Z","receivedAt":"2022-10-11T23:26:42Z","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.\norig_flags didn't need to be set if the cache entry wasn't a\nGITLINK so defer setting it.\n\nSigned-off-by: Calvin Wan <calvinwan@google.com>\n---\n diff-lib.c | 27 +++++++++++++++++----------\n 1 file changed, 17 insertions(+), 10 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 2edea41a23..e249322141 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -73,18 +73,25 @@ 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+\tstruct diff_flags orig_flags;\n+\n+\tif (!S_ISGITLINK(ce->ce_mode))\n+\t\tgoto ret;\n+\n+\torig_flags = diffopt->flags;\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\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\t*dirty_submodule = is_submodule_modified(ce->name,\n+\t\t\t\t\tdiffopt->flags.ignore_untracked_in_submodules);\n+cleanup:\n \t\tdiffopt->flags = orig_flags;\n-\t}\n+ret:\n \treturn changed;\n }\n \n-- \n2.38.0.rc1.362.ged0d419d3c-goog\n\n"},{"id":"464642","messageId":"20221011232604.839941-5-calvinwan@google.com","threadId":"58603","inReplyTo":"https://lore.kernel.org/git/20220922232947.631309-1-calvinwan@google.com/","subject":"[PATCH v2 4/4] diff-lib: parallelize run_diff_files for submodules","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-10-11T23:26:04Z","receivedAt":"2022-10-11T23:26:45Z","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. Subprocess output is piped to\nstatus_pipe_output which parses it. \n\nAdd config option submodule.diffJobs to set the maximum number\nof parallel jobs. The option defaults to 1 if unset. If set to 0, the\nnumber of jobs is set to online_cpus(). I added a TODO here, regarding\ndefaults -- please see the cover letter for the discussion. \n\nSince run_diff_files is called from many different commands, I chose\nto grab the config option in the function rather than adding variables\nto every git command and then figuring out how to pass them all in.\n\nSigned-off-by: Calvin Wan <calvinwan@google.com>\n---\n Documentation/config/submodule.txt |  12 +++\n diff-lib.c                         |  89 ++++++++++++++--\n submodule.c                        | 159 +++++++++++++++++++++++++++++\n submodule.h                        |   9 ++\n t/t4027-diff-submodule.sh          |  19 ++++\n t/t7506-status-submodule.sh        |  19 ++++\n 6 files changed, 299 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/config/submodule.txt b/Documentation/config/submodule.txt\nindex 6490527b45..3209eb8117 100644\n--- a/Documentation/config/submodule.txt\n+++ b/Documentation/config/submodule.txt\n@@ -93,6 +93,18 @@ submodule.fetchJobs::\n \tin parallel. A value of 0 will give some reasonable default.\n \tIf unset, it defaults to 1.\n \n+submodule.diffJobs::\n+\tSpecifies how many submodules are diffed at the same time. A\n+\tpositive integer allows up to that number of submodules diffed\n+\tin parallel. A value of 0 will give some reasonable default.\n+\tIf unset, it defaults to 1. The diff operation is used by many\n+\tother git commands such as add, merge, diff, status, stash and\n+\tmore. Note that the expensive part of the diff operation is\n+\treading the index from cache or memory. Therefore multiple jobs\n+\tmay be detrimental to performance if your hardware does not\n+\tsupport parallel reads or if the number of jobs greatly exceeds\n+\tthe amount of supported reads.\n+\n submodule.alternateLocation::\n \tSpecifies how the submodules obtain alternates when submodules are\n \tcloned. Possible values are `no`, `superproject`.\ndiff --git a/diff-lib.c b/diff-lib.c\nindex e249322141..44bb5558b3 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -14,6 +14,7 @@\n #include \"dir.h\"\n #include \"fsmonitor.h\"\n #include \"commit-reach.h\"\n+#include \"config.h\"\n \n /*\n  * diff-files\n@@ -65,15 +66,20 @@ 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 \tstruct diff_flags orig_flags;\n+\tint defer = 0;\n \n \tif (!S_ISGITLINK(ce->ce_mode))\n \t\tgoto ret;\n@@ -86,12 +92,21 @@ 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\t (!changed || diffopt->flags.dirty_submodules))\n-\t\t*dirty_submodule = is_submodule_modified(ce->name,\n-\t\t\t\t\tdiffopt->flags.ignore_untracked_in_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-\t\tdiffopt->flags = orig_flags;\n+\tdiffopt->flags = orig_flags;\n ret:\n+\tif (defer_submodule_status)\n+\t\t*defer_submodule_status = defer;\n \treturn changed;\n }\n \n@@ -103,6 +118,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@@ -227,6 +243,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\tnewmode = ce->ce_mode;\n \t\t} else {\n \t\t\tstruct stat st;\n+\t\t\tint ignore_untracked_in_submodules = 0;\n+\t\t\tint defer_submodule_status = !!revs->repo;\n \n \t\t\tchanged = check_removed(istate, ce, &st);\n \t\t\tif (changed) {\n@@ -248,8 +266,22 @@ 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\t\tce_option, &dirty_submodule,\n+\t\t\t\t\t\t\t\t&defer_submodule_status,\n+\t\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 \n \t\tif (!changed && !dirty_submodule) {\n@@ -268,6 +300,47 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\t    ce->name, 0, dirty_submodule);\n \n \t}\n+\tif (submodules.nr > 0) {\n+\t\tint i;\n+\t\tint parallel_jobs = 1;\n+\t\tgit_config_get_int(\"submodule.diffjobs\", &parallel_jobs);\n+\t\tif (parallel_jobs < 0) {\n+\t\t\tdie(_(\"submodule.diffjobs cannot be negative\"));\n+\t\t}\n+\t\telse if (!parallel_jobs) {\n+\t\t\t/*\n+\t\t\t * TODO: Decide what a reasonable default for parallel_jobs\n+\t\t\t * is. Currently mimics what other parallel config options\n+\t\t\t * default to.\n+\t\t\t */\n+\t\t\tparallel_jobs = online_cpus();\n+\t\t}\n+\n+\t\tif (get_submodules_status(revs->repo, &submodules, parallel_jobs))\n+\t\t\tBUG(\"Submodule status failed\");\n+\t\tfor (i = 0; i < submodules.nr; i++) {\n+\t\t\tstruct submodule_status_util *util = submodules.items[i].util;\n+\t\t\tstruct cache_entry *ce = util->ce;\n+\t\t\tunsigned int oldmode;\n+\t\t\tconst struct object_id *old_oid, *new_oid;\n+\n+\t\t\tif (!util->changed && !util->dirty_submodule) {\n+\t\t\t\tce_mark_uptodate(ce);\n+\t\t\t\tmark_fsmonitor_valid(istate, ce);\n+\t\t\t\tif (!revs->diffopt.flags.find_copies_harder)\n+\t\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t\toldmode = ce->ce_mode;\n+\t\t\told_oid = &ce->oid;\n+\t\t\tnew_oid = util->changed ? null_oid() : &ce->oid;\n+\t\t\tdiff_change(&revs->diffopt, oldmode, util->newmode,\n+\t\t\t\t\told_oid, new_oid,\n+\t\t\t\t\t!is_null_oid(old_oid),\n+\t\t\t\t\t!is_null_oid(new_oid),\n+\t\t\t\t\tce->name, 0, util->dirty_submodule);\n+\t\t}\n+\t}\n+\tstring_list_clear(&submodules, 1);\n \tdiffcore_std(&revs->diffopt);\n \tdiff_flush(&revs->diffopt);\n \ttrace_performance_since(start, \"diff-files\");\n@@ -315,7 +388,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/submodule.c b/submodule.c\nindex a3410ed8f0..72051fda81 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1363,6 +1363,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@@ -1445,6 +1459,13 @@ 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+\tunsigned dirty_submodule;\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@@ -1950,6 +1971,144 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \treturn dirty_submodule;\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->dirty_submodule = util->dirty_submodule;\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+\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+\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+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+static void status_pipe_output(struct strbuf *out,\n+\t\t\tvoid *cb, void *task_cb)\n+{\n+\tstruct status_task *task = task_cb;\n+\tstruct string_list list = STRING_LIST_INIT_DUP;\n+\tint i;\n+\n+\tstring_list_split(&list, out->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&task->dirty_submodule, task->ignore_untracked))\n+\t\t\tbreak;\n+\t}\n+\tstring_list_clear(&list, 0);\n+\tstrbuf_reset(out);\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_item *it =\n+\t\tstring_list_lookup(sps->submodule_names, task->path);\n+\tstruct submodule_status_util *util = it->util;\n+\n+\tutil->dirty_submodule = task->dirty_submodule;\n+\tfree(task);\n+\n+\treturn 0;\n+}\n+\n+int get_submodules_status(struct repository *r,\n+\t\t\tstruct string_list *submodules,\n+\t\t\tint max_parallel_jobs)\n+{\n+\tstruct submodule_parallel_status sps = SPS_INIT;\n+\tconst struct run_process_parallel_opts opts = {\n+\t\t.tr2_category = \"submodule\",\n+\t\t.tr2_label = \"parallel/status\",\n+\n+\t\t.jobs = max_parallel_jobs,\n+\n+\t\t.get_next_task = get_next_submodule_status,\n+\t\t.start_failure = status_start_failure,\n+\t\t.pipe_output = status_pipe_output,\n+\t\t.task_finished = status_finish,\n+\t\t.data = &sps,\n+\t};\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(&opts);\n+\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 6a9fec6de1..c829d37b9f 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\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/t/t4027-diff-submodule.sh b/t/t4027-diff-submodule.sh\nindex 40164ae07d..e08ee315a7 100755\n--- a/t/t4027-diff-submodule.sh\n+++ b/t/t4027-diff-submodule.sh\n@@ -34,6 +34,25 @@ test_expect_success setup '\n \tsubtip=$3 subprev=$2\n '\n \n+test_expect_success 'diff in superproject with submodules respects parallel settings' '\n+\ttest_when_finished \"rm -f trace.out\" &&\n+\t(\n+\t\tGIT_TRACE=$(pwd)/trace.out git diff &&\n+\t\tgrep \"1 tasks\" trace.out &&\n+\t\t>trace.out &&\n+\n+\t\tgit config submodule.diffJobs 8 &&\n+\t\tGIT_TRACE=$(pwd)/trace.out git diff &&\n+\t\tgrep \"8 tasks\" trace.out &&\n+\t\t>trace.out &&\n+\n+\t\tGIT_TRACE=$(pwd)/trace.out git -c submodule.diffJobs=0 diff &&\n+\t\tgrep \"preparing to run up to [0-9]* tasks\" trace.out &&\n+\t\t! grep \"up to 0 tasks\" trace.out &&\n+\t\t>trace.out\n+\t)\n+'\n+\n test_expect_success 'git diff --raw HEAD' '\n \thexsz=$(test_oid hexsz) &&\n \tgit diff --raw --abbrev=$hexsz HEAD >actual &&\ndiff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\nindex f5426a8e58..dfdfb58bfc 100755\n--- a/t/t7506-status-submodule.sh\n+++ b/t/t7506-status-submodule.sh\n@@ -410,4 +410,23 @@ test_expect_success 'status with added file in nested submodule (short)' '\n \tEOF\n '\n \n+test_expect_success 'status in superproject with submodules respects parallel settings' '\n+\ttest_when_finished \"rm -f trace.out\" &&\n+\t(\n+\t\tGIT_TRACE=$(pwd)/trace.out git status &&\n+\t\tgrep \"1 tasks\" trace.out &&\n+\t\t>trace.out &&\n+\n+\t\tgit config submodule.diffJobs 8 &&\n+\t\tGIT_TRACE=$(pwd)/trace.out git status &&\n+\t\tgrep \"8 tasks\" trace.out &&\n+\t\t>trace.out &&\n+\n+\t\tGIT_TRACE=$(pwd)/trace.out git -c submodule.diffJobs=0 status &&\n+\t\tgrep \"preparing to run up to [0-9]* tasks\" trace.out &&\n+\t\t! grep \"up to 0 tasks\" trace.out &&\n+\t\t>trace.out\n+\t)\n+'\n+\n test_done\n-- \n2.38.0.rc1.362.ged0d419d3c-goog\n\n"},{"id":"464648","messageId":"xmqqczaxmvv9.fsf@gitster.g","threadId":"58603","inReplyTo":"20221011232604.839941-1-calvinwan@google.com","subject":"Re: [PATCH v2 0/4] submodule: parallelize diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-12T05:52:42Z","receivedAt":"2022-10-12T05:52:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Calvin Wan <calvinwan@google.com> writes:\n\n> I also wanted to pose another question to list regarding defaults for\n> parallel processes. For jobs that clearly scale with the number of\n> processes (aka jobs that are mostly processor bound), it is obvious that\n> setting the default number of processes to the number of available cores\n> is the most optimal option. However, this changes when the job is mostly\n> I/O bound or has a combination of I/O and processing. Looking at my use\n> case for `status` on a cold cache (see below), we notice that increasing\n> the number of parallel processes speeds up status, but after a certain\n> number, it actually starts slowing down.\n\nI do not offhand recall how the default parallelism is computed\nthere, but if I am correct to suspect that \"git grep\" has a similar\nscaling pattern, i.e. the threads all need to compete for I/O to\nread from the filesystem to find needles from the haystack, perhaps\nit would give us a precedent to model the behaviour of this part of\nthe code, too, hopefully?\n\n"},{"id":"464653","messageId":"221012.868rllo545.gmgdl@evledraar.gmail.com","threadId":"58603","inReplyTo":"20221011232604.839941-3-calvinwan@google.com","subject":"Re: [PATCH v2 2/4] submodule: move status parsing into function","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-12T07:41:21Z","receivedAt":"2022-10-12T07:47:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Oct 11 2022, Calvin Wan wrote:\n\n> A future patch requires the ability to parse the output of git\n> status --porcelain=2. Move parsing code from is_submodule_modified to\n> parse_status_porcelain.\n>\n> Signed-off-by: Calvin Wan <calvinwan@google.com>\n> ---\n>  submodule.c | 71 +++++++++++++++++++++++++++++------------------------\n>  1 file changed, 39 insertions(+), 32 deletions(-)\n>\n> diff --git a/submodule.c b/submodule.c\n> index 72b295b87b..a3410ed8f0 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1864,6 +1864,43 @@ int fetch_submodules(struct repository *r,\n>  \treturn spf.result;\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>  unsigned is_submodule_modified(const char *path, int ignore_untracked)\n>  {\n>  \tstruct child_process cp = CHILD_PROCESS_INIT;\n> @@ -1900,39 +1937,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\nThis isn't just a code move, but a rewrite of much of the code to accept\na \"char *buf\" rather than a \"struct strbuf buf\".\n\nI can see in a later commit that you'd like to change this to accept a\n.string from a string_list.\n\n\nWithout changing anything else, if you lead with a commit that does (in\nthe initial loop):\n\n\tchar *str = buf.buf;\n        const size_t len = buf.len;\n\nAnd then make it use \"buf\" and \"len\" instead you could follow with the\nmove commit, which at that ponit would benefit from the rename detection\nmore more than it does now.\n\nAlso: If we have a strbuf in this caller let's pass a \"len\", and not\nhere make it need to do a strlen() on every line when we've computed it\nalready, the other caller could just pass another strlen([...].string).\n\n"},{"id":"464654","messageId":"221012.864jw9o3ah.gmgdl@evledraar.gmail.com","threadId":"58603","inReplyTo":"20221011232604.839941-2-calvinwan@google.com","subject":"Re: [PATCH v2 1/4] run-command: add pipe_output_fn to run_processes_parallel_opts","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-12T07:58:32Z","receivedAt":"2022-10-12T08:27:11Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Oct 11 2022, Calvin Wan wrote:\n\n(re-arranging the diff a bit for discussion)\n\n> diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c\n> index 46bac2bb70..d3c3df7960 100644\n> --- a/t/helper/test-run-command.c\n> +++ b/t/helper/test-run-command.c\n> @@ -52,6 +52,18 @@ static int no_job(struct child_process *cp,\n>  \treturn 0;\n>  }\n>  \n> +static void pipe_output(struct strbuf *out,\n> +\t\t\t\tvoid *pp_cb,\n> +\t\t\t\tvoid *pp_task_cb)\n> +{\n> +\tfprintf(stderr, \"%s\", out->buf);\n> +\t/*\n> +\t * Resetting output to show that piped output would print the\n> +\t * same as other tests without the pipe_output() function set\n> +\t */\n> +\tstrbuf_reset(out);\n> +}\n> +\n\nThis is still unaddressed from the last round:\nhttps://lore.kernel.org/git/220927.86edvxytla.gmgdl@evledraar.gmail.com/\n\nOr well, it changed a bit, but the \"reset\" is still there.\n\nWhatever you're aiming for here, this is apparently doing nothing,\nbecause if I comment out the strbuf_reset() then all your tests pass,\nwhich...\n\n>  \tint i = pp->output_owner;\n>  \n>  \tif (pp->children[i].state == GIT_CP_WORKING &&\n>  \t    pp->children[i].err.len) {\n> +\t\tif (opts->pipe_output)\n> +\t\t\topts->pipe_output(&pp->children[i].err, pp->data,\n> +\t\t\t\t\t\tpp->children[i].data);\n>  \t\tstrbuf_write(&pp->children[i].err, stderr);\n>  \t\tstrbuf_reset(&pp->children[i].err);\n>  \t}\n\n...we can see from here...\n\n> @@ -1716,6 +1720,10 @@ static int pp_collect_finished(struct parallel_processes *pp,\n>  \n>  \t\tcode = finish_command(&pp->children[i].process);\n>  \n> +\t\tif (opts->pipe_output)\n> +\t\t\topts->pipe_output(&pp->children[i].err, pp->data,\n> +\t\t\t\t\t\tpp->children[i].data);\n> +\n>  \t\tif (opts->task_finished)\n>  \t\t\tcode = opts->task_finished(code, opts->ungroup ? NULL :\n>  \t\t\t\t\t\t   &pp->children[i].err, pp->data,\n\n...and in particular here, where the \"err\" is subsequently passed to\n\"task_finished\" *would* actually have an impact, but (again,\nre-arranging he diff a bit) ...\n\n> @@ -439,6 +451,12 @@ int cmd__run_command(int argc, const char **argv)\n>  \t\topts.ungroup = 1;\n>  \t}\n>  \n> +\tif (!strcmp(argv[1], \"--pipe-output\")) {\n> +\t\targv += 1;\n> +\t\targc -= 1;\n> +\t\topts.pipe_output = pipe_output;\n> +\t}\n> +\n>  \tjobs = atoi(argv[2]);\n>  \tstrvec_clear(&proc.args);\n>  \tstrvec_pushv(&proc.args, (const char **)argv + 3);\n\n...here to test that we'd need our tests to combine \"pipe_output\" with\n\"task_finished\".\n\nWhich we should do in either case, e.g. if you define both and append to\n\"err\" in what order do we call them, and does \"err\" accumulate? (I think\nit should), in any case...\n\n> +\t/**\n> +\t * pipe_output: See pipe_output_fn() above. This should be\n> +\t * NULL unless process specific output is needed\n> +\t */\n> +\tpipe_output_fn pipe_output;\n\n\n...since we're on the topic we really should document what the behavior\nis. My understanding of this API has been that the \"err\" is not \"const\"\nbecause the callback is supposed to *append errors*, and there's an\nimplicit contract to do nothing else than that. But looking at your new\nAPI docs:\n\n> + * This callback is periodically called while child processes are\n> + * running and also when a child process finishes.\n\nWhatever you think this should be doing isn't documented precisely,\ni.e. is it called before/after some other callbacks, should the user\nrely on that at all, and...\n\n> + * \"struct strbuf *out\" contains the output collected from pp_task_cb\n> + * since the last call of this function.\n\n...this just seems wrong. We don't collect outptu \"from pp_task_cb\", do\nyou mean to say something, well, I was about to put words in your mouth,\nbut I honestly don't know what you expect, so ...\n\n*I'd* this to be documented as something like (which every other\n callback does):\n\n\tSee run_processes_parallel() below for a discussion of the \"struct\n\tstrbuf *out\" parameter.\n\nNow *those* docs could be improved a bit, but how we handle \"err/out\"\nreally needs to be documented holistically, as it's shared betwene\ncallbacks.\n\n> + *\n> + * pp_cb is the callback cookie as passed into run_processes_parallel,\n> + * pp_task_cb is the callback cookie as passed into get_next_task_fn.\n> + *\n> + * This function is incompatible with \"ungroup\"\n\nI'll re-roll the base series, which has a good place to add a BUG() if\nincompatible options are combined, which would be good to add, i.e.:\n\n\tif (opts->pipe_output && opts->ungroup)\n\t\tBUG(...);\n\n> +for opt in '' '--pipe-output'\n> +do\n> +\ttest_expect_success \"run_command runs in parallel with more jobs available than tasks $opt\" '\n> +\t\ttest-tool run-command run-command-parallel 5 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" >out 2>err &&\n> +\t\ttest_must_be_empty out &&\n> +\t\ttest_cmp expect err\n> +\t'\n> +done\n\nI take an earlier comment back, you do have tests that would change if\nthat strbuf_reset() was removed, you're just not running them. You\nforgot to put an \"$opt\" into the \"test-tool\" invocations:\n\t\n\tdiff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\n\tindex feabb3717b0..19a674480cd 100755\n\t--- a/t/t0061-run-command.sh\n\t+++ b/t/t0061-run-command.sh\n\t@@ -162,7 +162,7 @@ test_expect_success 'run_command runs ungrouped in parallel with as many jobs as\n\t for opt in '' '--pipe-output'\n\t do\n\t \ttest_expect_success \"run_command runs in parallel with more tasks than jobs available $opt\" '\n\t-\t\ttest-tool run-command run-command-parallel 3 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" >out 2>err &&\n\t+\t\ttest-tool run-command $opt run-command-parallel 3 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" >out 2>err &&\n\t \t\ttest_must_be_empty out &&\n\t \t\ttest_cmp expect err\n\t \t'\n\t@@ -186,7 +186,7 @@ EOF\n\t for opt in '' '--pipe-output'\n\t do\n\t \ttest_expect_success \"run_command is asked to abort gracefully $opt\" '\n\t-\t\ttest-tool run-command run-command-abort 3 false >out 2>err &&\n\t+\t\ttest-tool run-command $opt run-command-abort 3 false >out 2>err &&\n\t \t\ttest_must_be_empty out &&\n\t \t\ttest_cmp expect err\n\t \t'\n\t@@ -205,7 +205,7 @@ EOF\n\t for opt in '' '--pipe-output'\n\t do\n\t \ttest_expect_success \"run_command outputs $opt\" '\n\t-\t\ttest-tool run-command run-command-no-jobs 3 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" >out 2>err &&\n\t+\t\ttest-tool run-command $opt run-command-no-jobs 3 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" >out 2>err &&\n\t \t\ttest_must_be_empty out &&\n\t \t\ttest_cmp expect err\n\t \t'\n\t\n\nWith that all except 1 of your tests pass, now if I remove\nstrbuf_reset() and tweak the test so we can see what output comes from\nwhat, then:\n\t\n\tdiff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c\n\tindex ce246dfa4bc..690467e75c0 100644\n\t--- a/t/helper/test-run-command.c\n\t+++ b/t/helper/test-run-command.c\n\t@@ -56,12 +56,11 @@ static void pipe_output(struct strbuf *out,\n\t \t\t\t\tvoid *pp_cb,\n\t \t\t\t\tvoid *pp_task_cb)\n\t {\n\t-\tfprintf(stderr, \"%s\", out->buf);\n\t-\t/*\n\t-\t * Resetting output to show that piped output would print the\n\t-\t * same as other tests without the pipe_output() function set\n\t-\t */\n\t-\tstrbuf_reset(out);\n\t+\tstruct strbuf sb = STRBUF_INIT;\n\t+\n\t+\tstrbuf_add_lines(&sb, \"pipe_output: \", out->buf, out->len);\n\t+\tfputs(sb.buf, stderr);\n\t+\tstrbuf_release(&sb);\n\t }\n\t \n\t static int task_finished(int result,\n\nThe first test we fail (but with the reset we don't fail this one) is:\n\t\n\t+ diff -u expect err\n\t--- expect      2022-10-12 08:22:58.032264828 +0000\n\t+++ err 2022-10-12 08:22:58.068264515 +0000\n\t@@ -1,12 +1,24 @@\n\t+pipe_output: preloaded output of a child\n\t+pipe_output: Hello\n\t+pipe_output: World\n\t preloaded output of a child\n\t Hello\n\t World\n\t+pipe_output: preloaded output of a child\n\t+pipe_output: Hello\n\t+pipe_output: World\n\t+pipe_output: preloaded output of a child\n\t+pipe_output: Hello\n\t+pipe_output: World\n\t preloaded output of a child\n\t Hello\n\t World\n\t preloaded output of a child\n\t Hello\n\t World\n\t+pipe_output: preloaded output of a child\n\t+pipe_output: Hello\n\t+pipe_output: World\n\t preloaded output of a child\n\t Hello\n\t World\n\nSo, the apparent reason for the strbuf_reset() is that at some point in\nthe past you passed $out, but wanted to \"reset\" it so that you could\nre-use the existing test_cmp.\n\nBut that's just wrong, because we'd be sweeping under the rug exactly\nwhat we want to be testing here. Instead of hiding this you should be\ntest_cmp-ing the full thing.\n"},{"id":"464656","messageId":"221012.86zge1moki.gmgdl@evledraar.gmail.com","threadId":"58603","inReplyTo":"20221011232604.839941-3-calvinwan@google.com","subject":"Re: [PATCH v2 2/4] submodule: move status parsing into function","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-12T08:27:45Z","receivedAt":"2022-10-12T08:30:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Oct 11 2022, Calvin Wan wrote:\n\nJust commenting on one case, but this is in other parts of the series\n(e.g. your additions in submodule.c):\n\n> +\t\t\tBUG(\"invalid status --porcelain=2 line %s\",\n> +\t\t\t\tbuf);\n> [...]\n> -\t\t\t\tBUG(\"invalid status --porcelain=2 line %s\",\n> -\t\t\t\t    buf.buf);\n\nYour editor's indentation settings need fixing. Arguments should align\nwith the opening \"(\", here you replaced 4 spaces with a \"\\t\".\n\nA \"\\t\" is == 8 spaces for the purposes of our identation, if you need 7\nspaces to align with the \"7\" you insert 7 spaces, if it's 8 a \"\\t\", then\nfor one more a \"\\t\" and one space etc.\n"},{"id":"464658","messageId":"221012.86v8opmntc.gmgdl@evledraar.gmail.com","threadId":"58603","inReplyTo":"20221011232604.839941-5-calvinwan@google.com","subject":"Re: [PATCH v2 4/4] diff-lib: parallelize run_diff_files for submodules","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-12T08:31:51Z","receivedAt":"2022-10-12T08:46:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Oct 11 2022, Calvin Wan wrote:\n\nMostly style comments, but also others.:\n\n> +\t\t\t\t     unsigned *dirty_submodule, int *defer_submodule_status,\n> +\t\t\t\t\t int *ignore_untracked_in_submodules)\n\nIf you can think of a (much) shorter name for this new paremeter, then...\n\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\n...code like this becomes a lot less verbose and doesn't need this much\nindentation...\n\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\nAnd e.g. here splittng up the two:\n\n\tstruct string_list_item *item;\n\n\titem = ...\n\nMakes for easier reading IMO.\n\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\nMaybe easier to read as:\n\n\tstruct submodule_status_util tmp = {\n\t\t.changed = ...,\n\t\t.dirty_submodule =...,\n\t};\n\nThen a single memcpy() to copy that data over to the malloc'd region.\n\n> +\t\t\t\titem->util = util;\n\nAnd you can also do this idiomatically as:\n\n\tstring_list_append(...)->util = util;\n\n> +\t\t\t\tcontinue;\n\n> +\t\tint parallel_jobs = 1;\n> +\t\tgit_config_get_int(\"submodule.diffjobs\", &parallel_jobs);\n> +\t\tif (parallel_jobs < 0) {\n> +\t\t\tdie(_(\"submodule.diffjobs cannot be negative\"));\n\nMaybe we want something that uses git_parse_unsigned()?\n\n> +\t\t}\n\nThis should be cuddled with the \"else if\"\n\n> +\t\telse if (!parallel_jobs) {\n> +\t\t\t/*\n> +\t\t\t * TODO: Decide what a reasonable default for parallel_jobs\n> +\t\t\t * is. Currently mimics what other parallel config options\n> +\t\t\t * default to.\n> +\t\t\t */\n\nIt's OK to just drop this comment IMO.\n\n\n> +\t\t\tparallel_jobs = online_cpus();\n> +\t\t}\n\nAnd as these are both one statement you can drop the {}'s altogether.\n\nI think this would probably be more idiomatic as (untested):\n\n\tif (git_config_get_int(..., &v) || !v)\n\t\tjobs = online_cpus();\n\telse if (v < 0) /* or some API checks it for us? */\n\t\tdie(...);\n\telse\n\t\tjobs = v;\n\nI.e. we'd be explicitly using the \"does the key exist\" return value.\n\n> +\n> +\t\tif (get_submodules_status(revs->repo, &submodules, parallel_jobs))\n> +\t\t\tBUG(\"Submodule status failed\");\n\ns/Sub/sub/, lower-case for bug(), die() etc.\n\n> +\t\tfor (i = 0; i < submodules.nr; i++) {\n\nYou're iterating a string_list, so that \"i\" earlier should be size_t,\nbut better yet I think you can use for_each_string_list_item() here\n\n> +struct submodule_parallel_status {\n> +\tint index_count;\n\nHere you have an int...\n> +\tint result;\n> +\n> +\tstruct string_list *submodule_names;\n\n..that'll be tracking the \"nr\" of this, which is size_t, let's have them\nmatch.\n\nAlso, can we remove the \"*\" there and just have submodule.c populate\nthis struct directly, maybe not worth making it public, just a\nthought...\n\n..I didn't read further than this, ran out of time..\n"},{"id":"464904","messageId":"CAFySSZAi_XtwAQeVDgbjXD7vnGuYgNLNo+6NNJP0QvH64KxPOA@mail.gmail.com","threadId":"58603","inReplyTo":"xmqqczaxmvv9.fsf@gitster.g","subject":"Re: [PATCH v2 0/4] submodule: parallelize diff","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-10-14T00:39:21Z","receivedAt":"2022-10-14T00:39:37Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"> Calvin Wan <calvinwan@google.com> writes:\n>\n> > I also wanted to pose another question to list regarding defaults for\n> > parallel processes. For jobs that clearly scale with the number of\n> > processes (aka jobs that are mostly processor bound), it is obvious that\n> > setting the default number of processes to the number of available cores\n> > is the most optimal option. However, this changes when the job is mostly\n> > I/O bound or has a combination of I/O and processing. Looking at my use\n> > case for `status` on a cold cache (see below), we notice that increasing\n> > the number of parallel processes speeds up status, but after a certain\n> > number, it actually starts slowing down.\n>\n> I do not offhand recall how the default parallelism is computed\n> there, but if I am correct to suspect that \"git grep\" has a similar\n> scaling pattern, i.e. the threads all need to compete for I/O to\n> read from the filesystem to find needles from the haystack, perhaps\n> it would give us a precedent to model the behaviour of this part of\n> the code, too, hopefully?\n\nSetting grep.threads=0 does default it to the number of available cores\n(at least the documentation is clear about this). I tested \"git grep\" on\nmy machine and found that it started slowing down after 4 threads --\nthis is most likely because my NVMe SSD uses 4 PCIe lanes aka it can at\nmost do 4 reads in parallel. AFAIK, there is no way to tell how many\nreads a disk can do in parallel. This coupled with the fact that other\ncommands have varying levels of IO requirements makes it impossible to\nset a \"reasonable\" amount of threads.\n"}]}