From a083f94c2198e10ddb0c79acccd6137c24afce5b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 12 Oct 2022 23:02:20 +0200 Subject: [PATCH 01/15] run-command test helper: use "else if" pattern MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adjust the cmd__run_command() to use an "if/else if" chain rather than mutually exclusive "if" statements. This non-functional change makes a subsequent commit smaller. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- t/helper/test-run-command.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c index c9283b47af..390fa4fb72 100644 --- a/t/helper/test-run-command.c +++ b/t/helper/test-run-command.c @@ -427,18 +427,17 @@ int cmd__run_command(int argc, const char **argv) strvec_clear(&proc.args); strvec_pushv(&proc.args, (const char **)argv + 3); - if (!strcmp(argv[1], "run-command-parallel")) + if (!strcmp(argv[1], "run-command-parallel")) { exit(run_processes_parallel(jobs, parallel_next, NULL, NULL, &proc)); - - if (!strcmp(argv[1], "run-command-abort")) + } else if (!strcmp(argv[1], "run-command-abort")) { exit(run_processes_parallel(jobs, parallel_next, NULL, task_finished, &proc)); - - if (!strcmp(argv[1], "run-command-no-jobs")) + } else if (!strcmp(argv[1], "run-command-no-jobs")) { exit(run_processes_parallel(jobs, no_job, NULL, task_finished, &proc)); - - fprintf(stderr, "check usage\n"); - return 1; + } else { + fprintf(stderr, "check usage\n"); + return 1; + } } From 7dd5762d9f3980a85f96dd9f143184dc2fa07275 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 12 Oct 2022 23:02:21 +0200 Subject: [PATCH 02/15] run-command API: have "run_processes_parallel{,_tr2}()" return void MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change the "run_processes_parallel{,_tr2}()" functions to return void, instead of int. Ever since c553c72eed6 (run-command: add an asynchronous parallel child processor, 2015-12-15) they have unconditionally returned 0. To get a "real" return value out of this function the caller needs to get it via the "task_finished_fn" callback, see the example in hook.c added in 96e7225b310 (hook: add 'run' subcommand, 2021-12-22). So the "result = " and "if (!result)" code added to "builtin/fetch.c" d54dea77dba (fetch: let --jobs= parallelize --multiple, too, 2019-10-05) has always been redundant, we always took that "if" path. Likewise the "ret =" in "t/helper/test-run-command.c" added in be5d88e1128 (test-tool run-command: learn to run (parts of) the testsuite, 2019-10-04) wasn't used, instead we got the return value from the "if (suite.failed.nr > 0)" block seen in the context. Subsequent commits will alter this API interface, getting rid of this always-zero return value makes it easier to understand those changes. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- builtin/fetch.c | 15 +++++++-------- run-command.c | 27 +++++++++++---------------- run-command.h | 16 ++++++++-------- t/helper/test-run-command.c | 16 ++++++++-------- 4 files changed, 34 insertions(+), 40 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index a0fca93bb6..78043fb67e 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1953,15 +1953,14 @@ static int fetch_multiple(struct string_list *list, int max_children) struct parallel_fetch_state state = { argv.v, list, 0, 0 }; strvec_push(&argv, "--end-of-options"); - result = run_processes_parallel_tr2(max_children, - &fetch_next_remote, - &fetch_failed_to_start, - &fetch_finished, - &state, - "fetch", "parallel/fetch"); + run_processes_parallel_tr2(max_children, + &fetch_next_remote, + &fetch_failed_to_start, + &fetch_finished, + &state, + "fetch", "parallel/fetch"); - if (!result) - result = state.result; + result = state.result; } else for (i = 0; i < list->nr; i++) { const char *name = list->items[i].string; diff --git a/run-command.c b/run-command.c index 5ec3a46dcc..642e6b6e05 100644 --- a/run-command.c +++ b/run-command.c @@ -1783,11 +1783,11 @@ static int pp_collect_finished(struct parallel_processes *pp) return result; } -int run_processes_parallel(int n, - get_next_task_fn get_next_task, - start_failure_fn start_failure, - task_finished_fn task_finished, - void *pp_cb) +void run_processes_parallel(int n, + get_next_task_fn get_next_task, + start_failure_fn start_failure, + task_finished_fn task_finished, + void *pp_cb) { int i, code; int output_timeout = 100; @@ -1834,25 +1834,20 @@ int run_processes_parallel(int n, } pp_cleanup(&pp); - return 0; } -int run_processes_parallel_tr2(int n, get_next_task_fn get_next_task, - start_failure_fn start_failure, - task_finished_fn task_finished, void *pp_cb, - const char *tr2_category, const char *tr2_label) +void run_processes_parallel_tr2(int n, get_next_task_fn get_next_task, + start_failure_fn start_failure, + task_finished_fn task_finished, void *pp_cb, + const char *tr2_category, const char *tr2_label) { - int result; - trace2_region_enter_printf(tr2_category, tr2_label, NULL, "max:%d", ((n < 1) ? online_cpus() : n)); - result = run_processes_parallel(n, get_next_task, start_failure, - task_finished, pp_cb); + run_processes_parallel(n, get_next_task, start_failure, + task_finished, pp_cb); trace2_region_leave(tr2_category, tr2_label, NULL); - - return result; } int run_auto_maintenance(int quiet) diff --git a/run-command.h b/run-command.h index 0e85e5846a..e76a1b6b5b 100644 --- a/run-command.h +++ b/run-command.h @@ -485,14 +485,14 @@ typedef int (*task_finished_fn)(int result, * API reads that setting. */ extern int run_processes_parallel_ungroup; -int run_processes_parallel(int n, - get_next_task_fn, - start_failure_fn, - task_finished_fn, - void *pp_cb); -int run_processes_parallel_tr2(int n, get_next_task_fn, start_failure_fn, - task_finished_fn, void *pp_cb, - const char *tr2_category, const char *tr2_label); +void run_processes_parallel(int n, + get_next_task_fn, + start_failure_fn, + task_finished_fn, + void *pp_cb); +void run_processes_parallel_tr2(int n, get_next_task_fn, start_failure_fn, + task_finished_fn, void *pp_cb, + const char *tr2_category, const char *tr2_label); /** * Convenience function which prepares env for a command to be run in a diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c index 390fa4fb72..30c474f324 100644 --- a/t/helper/test-run-command.c +++ b/t/helper/test-run-command.c @@ -192,8 +192,8 @@ static int testsuite(int argc, const char **argv) fprintf(stderr, "Running %"PRIuMAX" tests (%d at a time)\n", (uintmax_t)suite.tests.nr, max_jobs); - ret = run_processes_parallel(max_jobs, next_test, test_failed, - test_finished, &suite); + run_processes_parallel(max_jobs, next_test, test_failed, + test_finished, &suite); if (suite.failed.nr > 0) { ret = 1; @@ -428,16 +428,16 @@ int cmd__run_command(int argc, const char **argv) strvec_pushv(&proc.args, (const char **)argv + 3); if (!strcmp(argv[1], "run-command-parallel")) { - exit(run_processes_parallel(jobs, parallel_next, - NULL, NULL, &proc)); + run_processes_parallel(jobs, parallel_next, NULL, NULL, &proc); } else if (!strcmp(argv[1], "run-command-abort")) { - exit(run_processes_parallel(jobs, parallel_next, - NULL, task_finished, &proc)); + run_processes_parallel(jobs, parallel_next, NULL, + task_finished, &proc); } else if (!strcmp(argv[1], "run-command-no-jobs")) { - exit(run_processes_parallel(jobs, no_job, - NULL, task_finished, &proc)); + run_processes_parallel(jobs, no_job, NULL, task_finished, + &proc); } else { fprintf(stderr, "check usage\n"); return 1; } + exit(0); } From 910e2b372f215263a79ae1d849274d2a212c16e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 12 Oct 2022 23:02:22 +0200 Subject: [PATCH 03/15] run-command tests: use "return", not "exit" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change the "run-command" test helper to "return" instead of calling "exit", see 338abb0f045 (builtins + test helpers: use return instead of exit() in cmd_*, 2021-06-08) Because we'd previously gotten past the SANITIZE=leak check by using exit() here we need to move to "goto cleanup" pattern. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- t/helper/test-run-command.c | 33 ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c index 30c474f324..ee509aefa2 100644 --- a/t/helper/test-run-command.c +++ b/t/helper/test-run-command.c @@ -381,13 +381,14 @@ int cmd__run_command(int argc, const char **argv) { struct child_process proc = CHILD_PROCESS_INIT; int jobs; + int ret; if (argc > 1 && !strcmp(argv[1], "testsuite")) - exit(testsuite(argc - 1, argv + 1)); + return testsuite(argc - 1, argv + 1); if (!strcmp(argv[1], "inherited-handle")) - exit(inherit_handle(argv[0])); + return inherit_handle(argv[0]); if (!strcmp(argv[1], "inherited-handle-child")) - exit(inherit_handle_child()); + return inherit_handle_child(); if (argc >= 2 && !strcmp(argv[1], "quote-stress-test")) return !!quote_stress_test(argc - 1, argv + 1); @@ -404,18 +405,24 @@ int cmd__run_command(int argc, const char **argv) argv += 2; argc -= 2; } - if (argc < 3) - return 1; + if (argc < 3) { + ret = 1; + goto cleanup; + } strvec_pushv(&proc.args, (const char **)argv + 2); if (!strcmp(argv[1], "start-command-ENOENT")) { - if (start_command(&proc) < 0 && errno == ENOENT) - return 0; + if (start_command(&proc) < 0 && errno == ENOENT) { + ret = 0; + goto cleanup; + } fprintf(stderr, "FAIL %s\n", argv[1]); return 1; } - if (!strcmp(argv[1], "run-command")) - exit(run_command(&proc)); + if (!strcmp(argv[1], "run-command")) { + ret = run_command(&proc); + goto cleanup; + } if (!strcmp(argv[1], "--ungroup")) { argv += 1; @@ -436,8 +443,12 @@ int cmd__run_command(int argc, const char **argv) run_processes_parallel(jobs, no_job, NULL, task_finished, &proc); } else { + ret = 1; fprintf(stderr, "check usage\n"); - return 1; + goto cleanup; } - exit(0); + ret = 0; +cleanup: + child_process_clear(&proc); + return ret; } From 6a48b428b4c36c4f96ccb64a189658887c78be3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 12 Oct 2022 23:02:23 +0200 Subject: [PATCH 04/15] run-command API: make "n" parameter a "size_t" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make the "n" variable added in c553c72eed6 (run-command: add an asynchronous parallel child processor, 2015-12-15) a "size_t". As we'll see in a subsequent commit we do pass "0" here, but never "jobs < 0". We could have made it an "unsigned int", but as we're having to change this let's not leave another case in the codebase where a size_t and "unsigned int" size differ on some platforms. In this case it's likely to never matter, but it's easier to not need to worry about it. After this and preceding changes: make run-command.o DEVOPTS=extra-all CFLAGS=-Wno-unused-parameter Only has one (and new) -Wsigned-compare warning relevant to a comparison about our "n" or "{nr,max}_processes": About using our "n" (size_t) in the same expression as online_cpus() (int). A subsequent commit will adjust & deal with online_cpus() and that warning. The only users of the "n" parameter are: * builtin/fetch.c: defaults to 1, reads from the "fetch.parallel" config. As seen in the code that parses the config added in d54dea77dba (fetch: let --jobs= parallelize --multiple, too, 2019-10-05) will die if the git_config_int() return value is < 0. It will however pass us n = 0, as we'll see in a subsequent commit. * submodule.c: defaults to 1, reads from "submodule.fetchJobs" config. Read via code originally added in a028a1930c6 (fetching submodules: respect `submodule.fetchJobs` config option, 2016-02-29). It now piggy-backs on the the submodule.fetchJobs code and validation added in f20e7c1ea24 (submodule: remove submodule.fetchjobs from submodule-config parsing, 2017-08-02). Like builtin/fetch.c it will die if the git_config_int() return value is < 0, but like builtin/fetch.c it will pass us n = 0. * builtin/submodule--helper.c: defaults to 1. Read via code originally added in 2335b870fa7 (submodule update: expose parallelism to the user, 2016-02-29). Since f20e7c1ea24 (submodule: remove submodule.fetchjobs from submodule-config parsing, 2017-08-02) it shares a config parser and semantics with the submodule.c caller. * hook.c: hardcoded to 1, see 96e7225b310 (hook: add 'run' subcommand, 2021-12-22). * t/helper/test-run-command.c: can be -1 after parsing the arguments, but will then be overridden to online_cpus() before passing it to this API. See be5d88e1128 (test-tool run-command: learn to run (parts of) the testsuite, 2019-10-04). Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- run-command.c | 42 ++++++++++++++++++------------------------ run-command.h | 4 ++-- 2 files changed, 20 insertions(+), 26 deletions(-) diff --git a/run-command.c b/run-command.c index 642e6b6e05..14a6e38e80 100644 --- a/run-command.c +++ b/run-command.c @@ -1500,8 +1500,8 @@ int run_processes_parallel_ungroup; struct parallel_processes { void *data; - int max_processes; - int nr_processes; + size_t max_processes; + size_t nr_processes; get_next_task_fn get_next_task; start_failure_fn start_failure; @@ -1522,7 +1522,7 @@ struct parallel_processes { unsigned shutdown : 1; unsigned ungroup : 1; - int output_owner; + size_t output_owner; struct strbuf buffered_output; /* of finished children */ }; @@ -1543,9 +1543,7 @@ static int default_task_finished(int result, static void kill_children(struct parallel_processes *pp, int signo) { - int i, n = pp->max_processes; - - for (i = 0; i < n; i++) + for (size_t i = 0; i < pp->max_processes; i++) if (pp->children[i].state == GIT_CP_WORKING) kill(pp->children[i].process.pid, signo); } @@ -1560,20 +1558,19 @@ static void handle_children_on_signal(int signo) } static void pp_init(struct parallel_processes *pp, - int n, + size_t n, get_next_task_fn get_next_task, start_failure_fn start_failure, task_finished_fn task_finished, void *data, int ungroup) { - int i; - if (n < 1) n = online_cpus(); pp->max_processes = n; - trace_printf("run_processes_parallel: preparing to run up to %d tasks", n); + trace_printf("run_processes_parallel: preparing to run up to %"PRIuMAX" tasks", + (uintmax_t)n); pp->data = data; if (!get_next_task) @@ -1594,7 +1591,7 @@ static void pp_init(struct parallel_processes *pp, CALLOC_ARRAY(pp->pfd, n); strbuf_init(&pp->buffered_output, 0); - for (i = 0; i < n; i++) { + for (size_t i = 0; i < n; i++) { strbuf_init(&pp->children[i].err, 0); child_process_init(&pp->children[i].process); if (pp->pfd) { @@ -1609,10 +1606,8 @@ static void pp_init(struct parallel_processes *pp, static void pp_cleanup(struct parallel_processes *pp) { - int i; - trace_printf("run_processes_parallel: done"); - for (i = 0; i < pp->max_processes; i++) { + for (size_t i = 0; i < pp->max_processes; i++) { strbuf_release(&pp->children[i].err); child_process_clear(&pp->children[i].process); } @@ -1639,7 +1634,8 @@ static void pp_cleanup(struct parallel_processes *pp) */ static int pp_start_one(struct parallel_processes *pp) { - int i, code; + size_t i; + int code; for (i = 0; i < pp->max_processes; i++) if (pp->children[i].state == GIT_CP_FREE) @@ -1697,7 +1693,7 @@ static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout) } /* Buffer output from all pipes. */ - for (i = 0; i < pp->max_processes; i++) { + for (size_t i = 0; i < pp->max_processes; i++) { if (pp->children[i].state == GIT_CP_WORKING && pp->pfd[i].revents & (POLLIN | POLLHUP)) { int n = strbuf_read_once(&pp->children[i].err, @@ -1714,7 +1710,7 @@ static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout) static void pp_output(struct parallel_processes *pp) { - int i = pp->output_owner; + size_t i = pp->output_owner; if (pp->children[i].state == GIT_CP_WORKING && pp->children[i].err.len) { @@ -1725,8 +1721,8 @@ static void pp_output(struct parallel_processes *pp) static int pp_collect_finished(struct parallel_processes *pp) { - int i, code; - int n = pp->max_processes; + int code; + size_t i, n = pp->max_processes; int result = 0; while (pp->nr_processes > 0) { @@ -1783,7 +1779,7 @@ static int pp_collect_finished(struct parallel_processes *pp) return result; } -void run_processes_parallel(int n, +void run_processes_parallel(size_t n, get_next_task_fn get_next_task, start_failure_fn start_failure, task_finished_fn task_finished, @@ -1817,9 +1813,7 @@ void run_processes_parallel(int n, if (!pp.nr_processes) break; if (ungroup) { - int i; - - for (i = 0; i < pp.max_processes; i++) + for (size_t i = 0; i < pp.max_processes; i++) pp.children[i].state = GIT_CP_WAIT_CLEANUP; } else { pp_buffer_stderr(&pp, output_timeout); @@ -1836,7 +1830,7 @@ void run_processes_parallel(int n, pp_cleanup(&pp); } -void run_processes_parallel_tr2(int n, get_next_task_fn get_next_task, +void run_processes_parallel_tr2(size_t n, get_next_task_fn get_next_task, start_failure_fn start_failure, task_finished_fn task_finished, void *pp_cb, const char *tr2_category, const char *tr2_label) diff --git a/run-command.h b/run-command.h index e76a1b6b5b..6f7604e114 100644 --- a/run-command.h +++ b/run-command.h @@ -485,12 +485,12 @@ typedef int (*task_finished_fn)(int result, * API reads that setting. */ extern int run_processes_parallel_ungroup; -void run_processes_parallel(int n, +void run_processes_parallel(size_t n, get_next_task_fn, start_failure_fn, task_finished_fn, void *pp_cb); -void run_processes_parallel_tr2(int n, get_next_task_fn, start_failure_fn, +void run_processes_parallel_tr2(size_t n, get_next_task_fn, start_failure_fn, task_finished_fn, void *pp_cb, const char *tr2_category, const char *tr2_label); From 51243f9f0f6a932ea579fd6f8014b348f8c2a523 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 12 Oct 2022 23:02:24 +0200 Subject: [PATCH 05/15] run-command API: don't fall back on online_cpus() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a "jobs = 0" is passed let's BUG() out rather than fall back on online_cpus(). The default behavior was added when this API was implemented in c553c72eed6 (run-command: add an asynchronous parallel child processor, 2015-12-15). Most of our code in-tree that scales up to "online_cpus()" by default calls that function by itself. Keeping this default behavior just for the sake of two callers means that we'd need to maintain this one spot where we're second-guessing the config passed down into pp_init(). The preceding commit has an overview of the API callers that passed "jobs = 0". There were only two of them (actually three, but they resolved to these two config parsing codepaths). The "fetch.parallel" caller already had a test for the "fetch.parallel=0" case added in 0353c688189 (fetch: do not run a redundant fetch from submodule, 2022-05-16), but there was no such test for "submodule.fetchJobs". Let's add one here. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- builtin/fetch.c | 2 ++ run-command.c | 7 +++---- submodule-config.c | 2 ++ t/t5526-fetch-submodules.sh | 8 +++++++- 4 files changed, 14 insertions(+), 5 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index 78043fb67e..82f1da14ec 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -122,6 +122,8 @@ static int git_fetch_config(const char *k, const char *v, void *cb) fetch_parallel_config = git_config_int(k, v); if (fetch_parallel_config < 0) die(_("fetch.parallel cannot be negative")); + if (!fetch_parallel_config) + fetch_parallel_config = online_cpus(); return 0; } diff --git a/run-command.c b/run-command.c index 14a6e38e80..14ea409375 100644 --- a/run-command.c +++ b/run-command.c @@ -1564,8 +1564,8 @@ static void pp_init(struct parallel_processes *pp, task_finished_fn task_finished, void *data, int ungroup) { - if (n < 1) - n = online_cpus(); + if (!n) + BUG("you must provide a non-zero number of processes!"); pp->max_processes = n; @@ -1835,8 +1835,7 @@ void run_processes_parallel_tr2(size_t n, get_next_task_fn get_next_task, task_finished_fn task_finished, void *pp_cb, const char *tr2_category, const char *tr2_label) { - trace2_region_enter_printf(tr2_category, tr2_label, NULL, "max:%d", - ((n < 1) ? online_cpus() : n)); + trace2_region_enter_printf(tr2_category, tr2_label, NULL, "max:%d", n); run_processes_parallel(n, get_next_task, start_failure, task_finished, pp_cb); diff --git a/submodule-config.c b/submodule-config.c index cd7ee236a1..4dc61b3a78 100644 --- a/submodule-config.c +++ b/submodule-config.c @@ -303,6 +303,8 @@ int parse_submodule_fetchjobs(const char *var, const char *value) int fetchjobs = git_config_int(var, value); if (fetchjobs < 0) die(_("negative values not allowed for submodule.fetchJobs")); + if (!fetchjobs) + fetchjobs = online_cpus(); return fetchjobs; } diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh index a301b56db8..a5f494dfcf 100755 --- a/t/t5526-fetch-submodules.sh +++ b/t/t5526-fetch-submodules.sh @@ -714,7 +714,13 @@ test_expect_success 'fetching submodules respects parallel settings' ' GIT_TRACE=$(pwd)/trace.out git fetch && grep "8 tasks" trace.out && GIT_TRACE=$(pwd)/trace.out git fetch --jobs 9 && - grep "9 tasks" trace.out + grep "9 tasks" trace.out && + >trace.out && + + GIT_TRACE=$(pwd)/trace.out git -c submodule.fetchJobs=0 fetch && + grep "preparing to run up to [0-9]* tasks" trace.out && + ! grep "up to 0 tasks" trace.out && + >trace.out ) ' From c333e6f3a86140c39797f8dedf6d078aec453dc3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 12 Oct 2022 23:02:25 +0200 Subject: [PATCH 06/15] run-command.c: use designated init for pp_init(), add "const" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use a designated initializer to initialize those parts of pp_init() that don't need any conditionals for their initialization, this sets us on a path to pp_init() itself into mostly a validation and allocation function. Since we're doing that we can add "const" to some of the members of the "struct parallel_processes", which helps to clarify and self-document this code. E.g. we never alter the "data" pointer we pass t user callbacks, nor (after the preceding change to stop invoking online_cpus()) do we change "max_processes", the same goes for the "ungroup" option. We can also do away with a call to strbuf_init() in favor of macro initialization, and to rely on other fields being NULL'd or zero'd. Making members of a struct "const" rather that the pointer to the struct itself is usually painful, as e.g. it precludes us from incrementally setting up the structure. In this case we only set it up with the assignment in run_process_parallel() and pp_init(), and don't pass the struct pointer around as "const", so making individual members "const" is worth the potential hassle for extra safety. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- run-command.c | 34 ++++++++++++++-------------------- 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/run-command.c b/run-command.c index 14ea409375..b69deb1cc5 100644 --- a/run-command.c +++ b/run-command.c @@ -1498,9 +1498,9 @@ enum child_state { int run_processes_parallel_ungroup; struct parallel_processes { - void *data; + void *const data; - size_t max_processes; + const size_t max_processes; size_t nr_processes; get_next_task_fn get_next_task; @@ -1520,7 +1520,7 @@ struct parallel_processes { struct pollfd *pfd; unsigned shutdown : 1; - unsigned ungroup : 1; + const unsigned ungroup : 1; size_t output_owner; struct strbuf buffered_output; /* of finished children */ @@ -1558,21 +1558,18 @@ static void handle_children_on_signal(int signo) } static void pp_init(struct parallel_processes *pp, - size_t n, get_next_task_fn get_next_task, start_failure_fn start_failure, - task_finished_fn task_finished, - void *data, int ungroup) + task_finished_fn task_finished) { + const size_t n = pp->max_processes; + if (!n) BUG("you must provide a non-zero number of processes!"); - pp->max_processes = n; - trace_printf("run_processes_parallel: preparing to run up to %"PRIuMAX" tasks", (uintmax_t)n); - pp->data = data; if (!get_next_task) BUG("you need to specify a get_next_task function"); pp->get_next_task = get_next_task; @@ -1580,16 +1577,9 @@ static void pp_init(struct parallel_processes *pp, pp->start_failure = start_failure ? start_failure : default_start_failure; pp->task_finished = task_finished ? task_finished : default_task_finished; - pp->nr_processes = 0; - pp->output_owner = 0; - pp->shutdown = 0; - pp->ungroup = ungroup; CALLOC_ARRAY(pp->children, n); - if (pp->ungroup) - pp->pfd = NULL; - else + if (!pp->ungroup) CALLOC_ARRAY(pp->pfd, n); - strbuf_init(&pp->buffered_output, 0); for (size_t i = 0; i < n; i++) { strbuf_init(&pp->children[i].err, 0); @@ -1789,13 +1779,17 @@ void run_processes_parallel(size_t n, int output_timeout = 100; int spawn_cap = 4; int ungroup = run_processes_parallel_ungroup; - struct parallel_processes pp; + struct parallel_processes pp = { + .max_processes = n, + .data = pp_cb, + .buffered_output = STRBUF_INIT, + .ungroup = ungroup, + }; /* unset for the next API user */ run_processes_parallel_ungroup = 0; - pp_init(&pp, n, get_next_task, start_failure, task_finished, pp_cb, - ungroup); + pp_init(&pp, get_next_task, start_failure, task_finished); while (1) { for (i = 0; i < spawn_cap && !pp.shutdown && From 6e5ba0bae447dba2adabea588b248b5fc6f59cd6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 12 Oct 2022 23:02:26 +0200 Subject: [PATCH 07/15] run-command API: have run_process_parallel() take an "opts" struct MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As noted in fd3aaf53f71 (run-command: add an "ungroup" option to run_process_parallel(), 2022-06-07) which added the "ungroup" passing it to "run_process_parallel()" via the global "run_processes_parallel_ungroup" variable was a compromise to get the smallest possible regression fix for "maint" at the time. This follow-up to that is a start at passing that parameter and others via a new "struct run_process_parallel_opts", as the earlier version[1] of what became fd3aaf53f71 did. Since we need to change all of the occurrences of "n" to "opt->SOMETHING" let's take the opportunity and rename the terse "n" to "processes". We could also have picked "max_processes", "jobs", "threads" etc., but as the API is named "run_processes_parallel()" let's go with "processes". Since the new "run_processes_parallel()" function is able to take an optional "tr2_category" and "tr2_label" via the struct we can at this point migrate all of the users of "run_processes_parallel_tr2()" over to it. But let's not migrate all the API users yet, only the two users that passed the "ungroup" parameter via the "run_processes_parallel_ungroup" global 1. https://lore.kernel.org/git/cover-v2-0.8-00000000000-20220518T195858Z-avarab@gmail.com/ Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- hook.c | 23 +++++++----- run-command.c | 54 ++++++++++++++++------------ run-command.h | 72 ++++++++++++++++++++++++++++--------- t/helper/test-run-command.c | 31 ++++++++++------ 4 files changed, 121 insertions(+), 59 deletions(-) diff --git a/hook.c b/hook.c index a493939a4f..a4fa1031f2 100644 --- a/hook.c +++ b/hook.c @@ -114,8 +114,20 @@ int run_hooks_opt(const char *hook_name, struct run_hooks_opt *options) .options = options, }; const char *const hook_path = find_hook(hook_name); - int jobs = 1; int ret = 0; + const struct run_process_parallel_opts opts = { + .tr2_category = "hook", + .tr2_label = hook_name, + + .processes = 1, + .ungroup = 1, + + .get_next_task = pick_next_hook, + .start_failure = notify_start_failure, + .task_finished = notify_hook_finished, + + .data = &cb_data, + }; if (!options) BUG("a struct run_hooks_opt must be provided to run_hooks"); @@ -137,14 +149,7 @@ int run_hooks_opt(const char *hook_name, struct run_hooks_opt *options) cb_data.hook_path = abs_path.buf; } - run_processes_parallel_ungroup = 1; - run_processes_parallel_tr2(jobs, - pick_next_hook, - notify_start_failure, - notify_hook_finished, - &cb_data, - "hook", - hook_name); + run_processes_parallel(&opts); ret = cb_data.rc; cleanup: strbuf_release(&abs_path); diff --git a/run-command.c b/run-command.c index b69deb1cc5..2858ec7bef 100644 --- a/run-command.c +++ b/run-command.c @@ -1496,7 +1496,6 @@ enum child_state { GIT_CP_WAIT_CLEANUP, }; -int run_processes_parallel_ungroup; struct parallel_processes { void *const data; @@ -1558,11 +1557,12 @@ static void handle_children_on_signal(int signo) } static void pp_init(struct parallel_processes *pp, - get_next_task_fn get_next_task, - start_failure_fn start_failure, - task_finished_fn task_finished) + const struct run_process_parallel_opts *opts) { - const size_t n = pp->max_processes; + const size_t n = opts->processes; + get_next_task_fn get_next_task = opts->get_next_task; + start_failure_fn start_failure = opts->start_failure; + task_finished_fn task_finished = opts->task_finished; if (!n) BUG("you must provide a non-zero number of processes!"); @@ -1769,27 +1769,27 @@ static int pp_collect_finished(struct parallel_processes *pp) return result; } -void run_processes_parallel(size_t n, - get_next_task_fn get_next_task, - start_failure_fn start_failure, - task_finished_fn task_finished, - void *pp_cb) +void run_processes_parallel(const struct run_process_parallel_opts *opts) { int i, code; int output_timeout = 100; int spawn_cap = 4; - int ungroup = run_processes_parallel_ungroup; struct parallel_processes pp = { - .max_processes = n, - .data = pp_cb, + .max_processes = opts->processes, + .data = opts->data, .buffered_output = STRBUF_INIT, - .ungroup = ungroup, + .ungroup = opts->ungroup, }; + /* options */ + const char *tr2_category = opts->tr2_category; + const char *tr2_label = opts->tr2_label; + const int do_trace2 = tr2_category && tr2_label; - /* unset for the next API user */ - run_processes_parallel_ungroup = 0; + if (do_trace2) + trace2_region_enter_printf(tr2_category, tr2_label, NULL, + "max:%d", opts->processes); - pp_init(&pp, get_next_task, start_failure, task_finished); + pp_init(&pp, opts); while (1) { for (i = 0; i < spawn_cap && !pp.shutdown && @@ -1806,7 +1806,7 @@ void run_processes_parallel(size_t n, } if (!pp.nr_processes) break; - if (ungroup) { + if (opts->ungroup) { for (size_t i = 0; i < pp.max_processes; i++) pp.children[i].state = GIT_CP_WAIT_CLEANUP; } else { @@ -1822,19 +1822,27 @@ void run_processes_parallel(size_t n, } pp_cleanup(&pp); + + if (do_trace2) + trace2_region_leave(tr2_category, tr2_label, NULL); } -void run_processes_parallel_tr2(size_t n, get_next_task_fn get_next_task, +void run_processes_parallel_tr2(size_t processes, get_next_task_fn get_next_task, start_failure_fn start_failure, task_finished_fn task_finished, void *pp_cb, const char *tr2_category, const char *tr2_label) { - trace2_region_enter_printf(tr2_category, tr2_label, NULL, "max:%d", n); + const struct run_process_parallel_opts opts = { + .tr2_category = tr2_category, + .tr2_label = tr2_label, + .processes = processes, - run_processes_parallel(n, get_next_task, start_failure, - task_finished, pp_cb); + .get_next_task = get_next_task, + .start_failure = start_failure, + .task_finished = task_finished, + }; - trace2_region_leave(tr2_category, tr2_label, NULL); + run_processes_parallel(&opts); } int run_auto_maintenance(int quiet) diff --git a/run-command.h b/run-command.h index 6f7604e114..aabdaf684d 100644 --- a/run-command.h +++ b/run-command.h @@ -459,17 +459,64 @@ typedef int (*task_finished_fn)(int result, void *pp_task_cb); /** - * Runs up to n processes at the same time. Whenever a process can be - * started, the callback get_next_task_fn is called to obtain the data + * Option used by run_processes_parallel(), { 0 }-initialized means no + * options. + */ +struct run_process_parallel_opts +{ + /** + * tr2_category & tr2_label: sets the trace2 category and label for + * logging. These must either be unset, or both of them must be set. + */ + const char *tr2_category; + const char *tr2_label; + + /** + * processes: see 'processes' in run_processes_parallel() below. + */ + size_t processes; + + /** + * ungroup: see 'ungroup' in run_processes_parallel() below. + */ + unsigned int ungroup:1; + + /** + * get_next_task: See get_next_task_fn() above. This must be + * specified. + */ + get_next_task_fn get_next_task; + + /** + * start_failure: See start_failure_fn() above. This can be + * NULL to omit any special handling. + */ + start_failure_fn start_failure; + + /** + * task_finished: See task_finished_fn() above. This can be + * NULL to omit any special handling. + */ + task_finished_fn task_finished; + + /** + * data: user data, will be passed as "pp_cb" to the callback + * parameters. + */ + void *data; +}; + +/** + * Options are passed via the "struct run_process_parallel_opts" above. + * + * Runs N 'processes' at the same time. Whenever a process can be + * started, the callback opts.get_next_task is called to obtain the data * required to start another child process. * * The children started via this function run in parallel. Their output * (both stdout and stderr) is routed to stderr in a manner that output * from different tasks does not interleave (but see "ungroup" below). * - * start_failure_fn and task_finished_fn can be NULL to omit any - * special handling. - * * If the "ungroup" option isn't specified, the API will set the * "stdout_to_stderr" parameter in "struct child_process" and provide * the callbacks with a "struct strbuf *out" parameter to write output @@ -479,19 +526,10 @@ typedef int (*task_finished_fn)(int result, * NULL "struct strbuf *out" parameter, and are responsible for * emitting their own output, including dealing with any race * conditions due to writing in parallel to stdout and stderr. - * The "ungroup" option can be enabled by setting the global - * "run_processes_parallel_ungroup" to "1" before invoking - * run_processes_parallel(), it will be set back to "0" as soon as the - * API reads that setting. */ -extern int run_processes_parallel_ungroup; -void run_processes_parallel(size_t n, - get_next_task_fn, - start_failure_fn, - task_finished_fn, - void *pp_cb); -void run_processes_parallel_tr2(size_t n, get_next_task_fn, start_failure_fn, - task_finished_fn, void *pp_cb, +void run_processes_parallel(const struct run_process_parallel_opts *opts); +void run_processes_parallel_tr2(size_t processes, get_next_task_fn, + start_failure_fn, task_finished_fn, void *pp_cb, const char *tr2_category, const char *tr2_label); /** diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c index ee509aefa2..3ecb830f4a 100644 --- a/t/helper/test-run-command.c +++ b/t/helper/test-run-command.c @@ -136,7 +136,7 @@ static const char * const testsuite_usage[] = { static int testsuite(int argc, const char **argv) { struct testsuite suite = TESTSUITE_INIT; - int max_jobs = 1, i, ret; + int max_jobs = 1, i, ret = 0; DIR *dir; struct dirent *d; struct option options[] = { @@ -152,6 +152,12 @@ static int testsuite(int argc, const char **argv) "write JUnit-style XML files"), OPT_END() }; + struct run_process_parallel_opts opts = { + .get_next_task = next_test, + .start_failure = test_failed, + .task_finished = test_finished, + .data = &suite, + }; argc = parse_options(argc, argv, NULL, options, testsuite_usage, PARSE_OPT_STOP_AT_NON_OPTION); @@ -192,8 +198,8 @@ static int testsuite(int argc, const char **argv) fprintf(stderr, "Running %"PRIuMAX" tests (%d at a time)\n", (uintmax_t)suite.tests.nr, max_jobs); - run_processes_parallel(max_jobs, next_test, test_failed, - test_finished, &suite); + opts.processes = max_jobs; + run_processes_parallel(&opts); if (suite.failed.nr > 0) { ret = 1; @@ -206,7 +212,7 @@ static int testsuite(int argc, const char **argv) string_list_clear(&suite.tests, 0); string_list_clear(&suite.failed, 0); - return !!ret; + return ret; } static uint64_t my_random_next = 1234; @@ -382,6 +388,9 @@ int cmd__run_command(int argc, const char **argv) struct child_process proc = CHILD_PROCESS_INIT; int jobs; int ret; + struct run_process_parallel_opts opts = { + .data = &proc, + }; if (argc > 1 && !strcmp(argv[1], "testsuite")) return testsuite(argc - 1, argv + 1); @@ -427,7 +436,7 @@ int cmd__run_command(int argc, const char **argv) if (!strcmp(argv[1], "--ungroup")) { argv += 1; argc -= 1; - run_processes_parallel_ungroup = 1; + opts.ungroup = 1; } jobs = atoi(argv[2]); @@ -435,18 +444,20 @@ int cmd__run_command(int argc, const char **argv) strvec_pushv(&proc.args, (const char **)argv + 3); if (!strcmp(argv[1], "run-command-parallel")) { - run_processes_parallel(jobs, parallel_next, NULL, NULL, &proc); + opts.get_next_task = parallel_next; } else if (!strcmp(argv[1], "run-command-abort")) { - run_processes_parallel(jobs, parallel_next, NULL, - task_finished, &proc); + opts.get_next_task = parallel_next; + opts.task_finished = task_finished; } else if (!strcmp(argv[1], "run-command-no-jobs")) { - run_processes_parallel(jobs, no_job, NULL, task_finished, - &proc); + opts.get_next_task = no_job; + opts.task_finished = task_finished; } else { ret = 1; fprintf(stderr, "check usage\n"); goto cleanup; } + opts.processes = jobs; + run_processes_parallel(&opts); ret = 0; cleanup: child_process_clear(&proc); From 36d69bf77e26793277d2708dd9ddad078e7e67a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 12 Oct 2022 23:02:27 +0200 Subject: [PATCH 08/15] run-command API: move *_tr2() users to "run_processes_parallel()" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Have the users of the "run_processes_parallel_tr2()" function use "run_processes_parallel()" instead. In preceding commits the latter was refactored to take a "struct run_process_parallel_opts" argument, since the only reason for "run_processes_parallel_tr2()" to exist was to take arguments that are now a part of that struct we can do away with it. See ee4512ed481 (trace2: create new combined trace facility, 2019-02-22) for the addition of the "*_tr2()" variant of the function, it was used by every caller except "t/helper/test-run-command.c".. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- builtin/fetch.c | 18 ++++++++++++------ builtin/submodule--helper.c | 16 ++++++++++++---- run-command.c | 18 ------------------ run-command.h | 3 --- submodule.c | 18 ++++++++++++------ 5 files changed, 36 insertions(+), 37 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index 82f1da14ec..b06e454cbd 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1953,15 +1953,21 @@ static int fetch_multiple(struct string_list *list, int max_children) if (max_children != 1 && list->nr != 1) { struct parallel_fetch_state state = { argv.v, list, 0, 0 }; + const struct run_process_parallel_opts opts = { + .tr2_category = "fetch", + .tr2_label = "parallel/fetch", + + .processes = max_children, + + .get_next_task = &fetch_next_remote, + .start_failure = &fetch_failed_to_start, + .task_finished = &fetch_finished, + .data = &state, + }; strvec_push(&argv, "--end-of-options"); - run_processes_parallel_tr2(max_children, - &fetch_next_remote, - &fetch_failed_to_start, - &fetch_finished, - &state, - "fetch", "parallel/fetch"); + run_processes_parallel(&opts); result = state.result; } else for (i = 0; i < list->nr; i++) { diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 0b4acb442b..df526525c1 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2567,12 +2567,20 @@ static int update_submodules(struct update_data *update_data) { int i, ret = 0; struct submodule_update_clone suc = SUBMODULE_UPDATE_CLONE_INIT; + const struct run_process_parallel_opts opts = { + .tr2_category = "submodule", + .tr2_label = "parallel/update", + + .processes = update_data->max_jobs, + + .get_next_task = update_clone_get_next_task, + .start_failure = update_clone_start_failure, + .task_finished = update_clone_task_finished, + .data = &suc, + }; suc.update_data = update_data; - run_processes_parallel_tr2(suc.update_data->max_jobs, update_clone_get_next_task, - update_clone_start_failure, - update_clone_task_finished, &suc, "submodule", - "parallel/update"); + run_processes_parallel(&opts); /* * We saved the output and put it out all at once now. diff --git a/run-command.c b/run-command.c index 2858ec7bef..646ea22e39 100644 --- a/run-command.c +++ b/run-command.c @@ -1827,24 +1827,6 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts) trace2_region_leave(tr2_category, tr2_label, NULL); } -void run_processes_parallel_tr2(size_t processes, get_next_task_fn get_next_task, - start_failure_fn start_failure, - task_finished_fn task_finished, void *pp_cb, - const char *tr2_category, const char *tr2_label) -{ - const struct run_process_parallel_opts opts = { - .tr2_category = tr2_category, - .tr2_label = tr2_label, - .processes = processes, - - .get_next_task = get_next_task, - .start_failure = start_failure, - .task_finished = task_finished, - }; - - run_processes_parallel(&opts); -} - int run_auto_maintenance(int quiet) { int enabled; diff --git a/run-command.h b/run-command.h index aabdaf684d..e3e1ea01ad 100644 --- a/run-command.h +++ b/run-command.h @@ -528,9 +528,6 @@ struct run_process_parallel_opts * conditions due to writing in parallel to stdout and stderr. */ void run_processes_parallel(const struct run_process_parallel_opts *opts); -void run_processes_parallel_tr2(size_t processes, get_next_task_fn, - start_failure_fn, task_finished_fn, void *pp_cb, - const char *tr2_category, const char *tr2_label); /** * Convenience function which prepares env for a command to be run in a diff --git a/submodule.c b/submodule.c index bf7a2c7918..f7c71f1f4b 100644 --- a/submodule.c +++ b/submodule.c @@ -1819,6 +1819,17 @@ int fetch_submodules(struct repository *r, { int i; struct submodule_parallel_fetch spf = SPF_INIT; + const struct run_process_parallel_opts opts = { + .tr2_category = "submodule", + .tr2_label = "parallel/fetch", + + .processes = max_parallel_jobs, + + .get_next_task = get_next_submodule, + .start_failure = fetch_start_failure, + .task_finished = fetch_finish, + .data = &spf, + }; spf.r = r; spf.command_line_option = command_line_option; @@ -1840,12 +1851,7 @@ int fetch_submodules(struct repository *r, calculate_changed_submodule_paths(r, &spf.changed_submodule_names); string_list_sort(&spf.changed_submodule_names); - run_processes_parallel_tr2(max_parallel_jobs, - get_next_submodule, - fetch_start_failure, - fetch_finish, - &spf, - "submodule", "parallel/fetch"); + run_processes_parallel(&opts); if (spf.submodules_with_errors.len > 0) fprintf(stderr, _("Errors during submodule fetch:\n%s"), From e39c9de8607cacee4cc5169c2e014bc59acbbcde Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 12 Oct 2022 23:02:28 +0200 Subject: [PATCH 09/15] run-command.c: make "struct parallel_processes" const if possible MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a "const" to two "struct parallel_processes" parameters where we're not modifying anything in "pp". For kill_children() we'll call it from both the signal handler, and from run_processes_parallel() itself. Adding a "const" there makes it clear that we don't need to modify any state when killing our children. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- run-command.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/run-command.c b/run-command.c index 646ea22e39..a72c4295ad 100644 --- a/run-command.c +++ b/run-command.c @@ -1540,7 +1540,7 @@ static int default_task_finished(int result, return 0; } -static void kill_children(struct parallel_processes *pp, int signo) +static void kill_children(const struct parallel_processes *pp, int signo) { for (size_t i = 0; i < pp->max_processes; i++) if (pp->children[i].state == GIT_CP_WORKING) @@ -1698,7 +1698,7 @@ static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout) } } -static void pp_output(struct parallel_processes *pp) +static void pp_output(const struct parallel_processes *pp) { size_t i = pp->output_owner; From fa93951d796a4837f6d24c54b638b976dbb17a8b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 12 Oct 2022 23:02:29 +0200 Subject: [PATCH 10/15] run-command.c: don't copy *_fn to "struct parallel_processes" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The only remaining reason for copying the callbacks in the "struct run_process_parallel_opts" over to the "struct parallel_processes" was to avoid two if/else statements in case the "start_failure" and "task_finished" callbacks were NULL. Let's handle those cases in pp_start_one() and pp_collect_finished() instead, and avoid the default_* stub functions, and the need to copy this data around. Organizing the code like this made more sense before the "struct run_parallel_parallel_opts" existed, as we'd have needed to pass each of these as a separate parameter. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- run-command.c | 67 +++++++++++++++++++-------------------------------- 1 file changed, 25 insertions(+), 42 deletions(-) diff --git a/run-command.c b/run-command.c index a72c4295ad..e10b1b9739 100644 --- a/run-command.c +++ b/run-command.c @@ -1502,10 +1502,6 @@ struct parallel_processes { const size_t max_processes; size_t nr_processes; - get_next_task_fn get_next_task; - start_failure_fn start_failure; - task_finished_fn task_finished; - struct { enum child_state state; struct child_process process; @@ -1525,21 +1521,6 @@ struct parallel_processes { struct strbuf buffered_output; /* of finished children */ }; -static int default_start_failure(struct strbuf *out, - void *pp_cb, - void *pp_task_cb) -{ - return 0; -} - -static int default_task_finished(int result, - struct strbuf *out, - void *pp_cb, - void *pp_task_cb) -{ - return 0; -} - static void kill_children(const struct parallel_processes *pp, int signo) { for (size_t i = 0; i < pp->max_processes; i++) @@ -1560,9 +1541,6 @@ static void pp_init(struct parallel_processes *pp, const struct run_process_parallel_opts *opts) { const size_t n = opts->processes; - get_next_task_fn get_next_task = opts->get_next_task; - start_failure_fn start_failure = opts->start_failure; - task_finished_fn task_finished = opts->task_finished; if (!n) BUG("you must provide a non-zero number of processes!"); @@ -1570,12 +1548,8 @@ static void pp_init(struct parallel_processes *pp, trace_printf("run_processes_parallel: preparing to run up to %"PRIuMAX" tasks", (uintmax_t)n); - if (!get_next_task) + if (!opts->get_next_task) BUG("you need to specify a get_next_task function"); - pp->get_next_task = get_next_task; - - pp->start_failure = start_failure ? start_failure : default_start_failure; - pp->task_finished = task_finished ? task_finished : default_task_finished; CALLOC_ARRAY(pp->children, n); if (!pp->ungroup) @@ -1622,7 +1596,8 @@ static void pp_cleanup(struct parallel_processes *pp) * <0 no new job was started, user wishes to shutdown early. Use negative code * to signal the children. */ -static int pp_start_one(struct parallel_processes *pp) +static int pp_start_one(struct parallel_processes *pp, + const struct run_process_parallel_opts *opts) { size_t i; int code; @@ -1633,10 +1608,10 @@ static int pp_start_one(struct parallel_processes *pp) if (i == pp->max_processes) BUG("bookkeeping is hard"); - code = pp->get_next_task(&pp->children[i].process, - pp->ungroup ? NULL : &pp->children[i].err, - pp->data, - &pp->children[i].data); + code = opts->get_next_task(&pp->children[i].process, + pp->ungroup ? NULL : &pp->children[i].err, + pp->data, + &pp->children[i].data); if (!code) { if (!pp->ungroup) { strbuf_addbuf(&pp->buffered_output, &pp->children[i].err); @@ -1651,10 +1626,14 @@ static int pp_start_one(struct parallel_processes *pp) pp->children[i].process.no_stdin = 1; if (start_command(&pp->children[i].process)) { - code = pp->start_failure(pp->ungroup ? NULL : - &pp->children[i].err, - pp->data, - pp->children[i].data); + if (opts->start_failure) + code = opts->start_failure(pp->ungroup ? NULL : + &pp->children[i].err, + pp->data, + pp->children[i].data); + else + code = 0; + if (!pp->ungroup) { strbuf_addbuf(&pp->buffered_output, &pp->children[i].err); strbuf_reset(&pp->children[i].err); @@ -1709,7 +1688,8 @@ static void pp_output(const struct parallel_processes *pp) } } -static int pp_collect_finished(struct parallel_processes *pp) +static int pp_collect_finished(struct parallel_processes *pp, + const struct run_process_parallel_opts *opts) { int code; size_t i, n = pp->max_processes; @@ -1724,9 +1704,12 @@ static int pp_collect_finished(struct parallel_processes *pp) code = finish_command(&pp->children[i].process); - code = pp->task_finished(code, pp->ungroup ? NULL : - &pp->children[i].err, pp->data, - pp->children[i].data); + if (opts->task_finished) + code = opts->task_finished(code, pp->ungroup ? NULL : + &pp->children[i].err, pp->data, + pp->children[i].data); + else + code = 0; if (code) result = code; @@ -1795,7 +1778,7 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts) i < spawn_cap && !pp.shutdown && pp.nr_processes < pp.max_processes; i++) { - code = pp_start_one(&pp); + code = pp_start_one(&pp, opts); if (!code) continue; if (code < 0) { @@ -1813,7 +1796,7 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts) pp_buffer_stderr(&pp, output_timeout); pp_output(&pp); } - code = pp_collect_finished(&pp); + code = pp_collect_finished(&pp, opts); if (code) { pp.shutdown = 1; if (code < 0) From 357f8e6e184e8e01c502ee5d7a81cca71d59f322 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 12 Oct 2022 23:02:30 +0200 Subject: [PATCH 11/15] run-command.c: don't copy "ungroup" to "struct parallel_processes" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As with the *_fn members removed in the preceding commit, let's not copy the "ungroup" member of the "struct run_process_parallel_opts" over to the "struct parallel_processes". Now that we're passing the "opts" down there's no reason to do so. This makes the code easier to follow, as we have a "const" attribute on the "struct run_process_parallel_opts", but not "struct parallel_processes". We do not alter the "ungroup" argument, so storing it in the non-const structure would make this control flow less obvious. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- run-command.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/run-command.c b/run-command.c index e10b1b9739..19d5cff8c6 100644 --- a/run-command.c +++ b/run-command.c @@ -1515,7 +1515,6 @@ struct parallel_processes { struct pollfd *pfd; unsigned shutdown : 1; - const unsigned ungroup : 1; size_t output_owner; struct strbuf buffered_output; /* of finished children */ @@ -1552,7 +1551,7 @@ static void pp_init(struct parallel_processes *pp, BUG("you need to specify a get_next_task function"); CALLOC_ARRAY(pp->children, n); - if (!pp->ungroup) + if (!opts->ungroup) CALLOC_ARRAY(pp->pfd, n); for (size_t i = 0; i < n; i++) { @@ -1609,17 +1608,17 @@ static int pp_start_one(struct parallel_processes *pp, BUG("bookkeeping is hard"); code = opts->get_next_task(&pp->children[i].process, - pp->ungroup ? NULL : &pp->children[i].err, + opts->ungroup ? NULL : &pp->children[i].err, pp->data, &pp->children[i].data); if (!code) { - if (!pp->ungroup) { + if (!opts->ungroup) { strbuf_addbuf(&pp->buffered_output, &pp->children[i].err); strbuf_reset(&pp->children[i].err); } return 1; } - if (!pp->ungroup) { + if (!opts->ungroup) { pp->children[i].process.err = -1; pp->children[i].process.stdout_to_stderr = 1; } @@ -1627,14 +1626,14 @@ static int pp_start_one(struct parallel_processes *pp, if (start_command(&pp->children[i].process)) { if (opts->start_failure) - code = opts->start_failure(pp->ungroup ? NULL : + code = opts->start_failure(opts->ungroup ? NULL : &pp->children[i].err, pp->data, pp->children[i].data); else code = 0; - if (!pp->ungroup) { + if (!opts->ungroup) { strbuf_addbuf(&pp->buffered_output, &pp->children[i].err); strbuf_reset(&pp->children[i].err); } @@ -1705,7 +1704,7 @@ static int pp_collect_finished(struct parallel_processes *pp, code = finish_command(&pp->children[i].process); if (opts->task_finished) - code = opts->task_finished(code, pp->ungroup ? NULL : + code = opts->task_finished(code, opts->ungroup ? NULL : &pp->children[i].err, pp->data, pp->children[i].data); else @@ -1722,7 +1721,7 @@ static int pp_collect_finished(struct parallel_processes *pp, pp->pfd[i].fd = -1; child_process_init(&pp->children[i].process); - if (pp->ungroup) { + if (opts->ungroup) { ; /* no strbuf_*() work to do here */ } else if (i != pp->output_owner) { strbuf_addbuf(&pp->buffered_output, &pp->children[i].err); @@ -1761,7 +1760,6 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts) .max_processes = opts->processes, .data = opts->data, .buffered_output = STRBUF_INIT, - .ungroup = opts->ungroup, }; /* options */ const char *tr2_category = opts->tr2_category; From 2aa8d2259ffd477b5d8ff090c90b7d38910b4741 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 12 Oct 2022 23:02:31 +0200 Subject: [PATCH 12/15] run-command.c: don't copy "data" to "struct parallel_processes" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As with the *_fn members removed in a preceding commit, let's not copy the "data" member of the "struct run_process_parallel_opts" over to the "struct parallel_processes". Now that we're passing the "opts" down there's no reason to do so. This makes the code easier to follow, as we have a "const" attribute on the "struct run_process_parallel_opts", but not "struct parallel_processes". We do not alter the "ungroup" argument, so storing it in the non-const structure would make this control flow less obvious. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- run-command.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/run-command.c b/run-command.c index 19d5cff8c6..d382c42f82 100644 --- a/run-command.c +++ b/run-command.c @@ -1497,8 +1497,6 @@ enum child_state { }; struct parallel_processes { - void *const data; - const size_t max_processes; size_t nr_processes; @@ -1609,7 +1607,7 @@ static int pp_start_one(struct parallel_processes *pp, code = opts->get_next_task(&pp->children[i].process, opts->ungroup ? NULL : &pp->children[i].err, - pp->data, + opts->data, &pp->children[i].data); if (!code) { if (!opts->ungroup) { @@ -1628,7 +1626,7 @@ static int pp_start_one(struct parallel_processes *pp, if (opts->start_failure) code = opts->start_failure(opts->ungroup ? NULL : &pp->children[i].err, - pp->data, + opts->data, pp->children[i].data); else code = 0; @@ -1705,7 +1703,7 @@ static int pp_collect_finished(struct parallel_processes *pp, if (opts->task_finished) code = opts->task_finished(code, opts->ungroup ? NULL : - &pp->children[i].err, pp->data, + &pp->children[i].err, opts->data, pp->children[i].data); else code = 0; @@ -1758,7 +1756,6 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts) int spawn_cap = 4; struct parallel_processes pp = { .max_processes = opts->processes, - .data = opts->data, .buffered_output = STRBUF_INIT, }; /* options */ From 9f3df6c0487bde80626ea2f367f9850e1758720b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 12 Oct 2022 23:02:32 +0200 Subject: [PATCH 13/15] run-command.c: use "opts->processes", not "pp->max_processes" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Neither the "processes" nor "max_processes" members ever change after their initialization, and they're always equivalent, but some existing code used "pp->max_processes" when we were already passing the "opts" to the function, let's use the "opts" directly instead. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- run-command.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/run-command.c b/run-command.c index d382c42f82..719a5b58e2 100644 --- a/run-command.c +++ b/run-command.c @@ -1599,10 +1599,10 @@ static int pp_start_one(struct parallel_processes *pp, size_t i; int code; - for (i = 0; i < pp->max_processes; i++) + for (i = 0; i < opts->processes; i++) if (pp->children[i].state == GIT_CP_FREE) break; - if (i == pp->max_processes) + if (i == opts->processes) BUG("bookkeeping is hard"); code = opts->get_next_task(&pp->children[i].process, @@ -1689,14 +1689,14 @@ static int pp_collect_finished(struct parallel_processes *pp, const struct run_process_parallel_opts *opts) { int code; - size_t i, n = pp->max_processes; + size_t i; int result = 0; while (pp->nr_processes > 0) { - for (i = 0; i < pp->max_processes; i++) + for (i = 0; i < opts->processes; i++) if (pp->children[i].state == GIT_CP_WAIT_CLEANUP) break; - if (i == pp->max_processes) + if (i == opts->processes) break; code = finish_command(&pp->children[i].process); @@ -1725,6 +1725,8 @@ static int pp_collect_finished(struct parallel_processes *pp, strbuf_addbuf(&pp->buffered_output, &pp->children[i].err); strbuf_reset(&pp->children[i].err); } else { + const size_t n = opts->processes; + strbuf_write(&pp->children[i].err, stderr); strbuf_reset(&pp->children[i].err); @@ -1771,7 +1773,7 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts) while (1) { for (i = 0; i < spawn_cap && !pp.shutdown && - pp.nr_processes < pp.max_processes; + pp.nr_processes < opts->processes; i++) { code = pp_start_one(&pp, opts); if (!code) @@ -1785,7 +1787,7 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts) if (!pp.nr_processes) break; if (opts->ungroup) { - for (size_t i = 0; i < pp.max_processes; i++) + for (size_t i = 0; i < opts->processes; i++) pp.children[i].state = GIT_CP_WAIT_CLEANUP; } else { pp_buffer_stderr(&pp, output_timeout); From d1610eef3f66d5735e087cde64bb4ab8cd5d9271 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 12 Oct 2022 23:02:33 +0200 Subject: [PATCH 14/15] run-command.c: pass "opts" further down, and use "opts->processes" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Continue the migration away from the "max_processes" member of "struct parallel_processes" to the "processes" member of the "struct run_process_parallel_opts", in this case we needed to pass the "opts" further down into pp_cleanup() and pp_buffer_stderr(). Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- run-command.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/run-command.c b/run-command.c index 719a5b58e2..ce7966394d 100644 --- a/run-command.c +++ b/run-command.c @@ -1565,10 +1565,11 @@ static void pp_init(struct parallel_processes *pp, sigchain_push_common(handle_children_on_signal); } -static void pp_cleanup(struct parallel_processes *pp) +static void pp_cleanup(struct parallel_processes *pp, + const struct run_process_parallel_opts *opts) { trace_printf("run_processes_parallel: done"); - for (size_t i = 0; i < pp->max_processes; i++) { + for (size_t i = 0; i < opts->processes; i++) { strbuf_release(&pp->children[i].err); child_process_clear(&pp->children[i].process); } @@ -1647,19 +1648,21 @@ static int pp_start_one(struct parallel_processes *pp, return 0; } -static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout) +static void pp_buffer_stderr(struct parallel_processes *pp, + const struct run_process_parallel_opts *opts, + int output_timeout) { int i; - while ((i = poll(pp->pfd, pp->max_processes, output_timeout)) < 0) { + while ((i = poll(pp->pfd, opts->processes, output_timeout) < 0)) { if (errno == EINTR) continue; - pp_cleanup(pp); + pp_cleanup(pp, opts); die_errno("poll"); } /* Buffer output from all pipes. */ - for (size_t i = 0; i < pp->max_processes; i++) { + for (size_t i = 0; i < opts->processes; i++) { if (pp->children[i].state == GIT_CP_WORKING && pp->pfd[i].revents & (POLLIN | POLLHUP)) { int n = strbuf_read_once(&pp->children[i].err, @@ -1790,7 +1793,7 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts) for (size_t i = 0; i < opts->processes; i++) pp.children[i].state = GIT_CP_WAIT_CLEANUP; } else { - pp_buffer_stderr(&pp, output_timeout); + pp_buffer_stderr(&pp, opts, output_timeout); pp_output(&pp); } code = pp_collect_finished(&pp, opts); @@ -1801,7 +1804,7 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts) } } - pp_cleanup(&pp); + pp_cleanup(&pp, opts); if (do_trace2) trace2_region_leave(tr2_category, tr2_label, NULL); From 0b0ab95f17d5eb71d2552e2c3b0806e2ed87beca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 12 Oct 2022 23:02:34 +0200 Subject: [PATCH 15/15] run-command.c: remove "max_processes", add "const" to signal() handler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As with the *_fn members removed in a preceding commit, let's not copy the "processes" member of the "struct run_process_parallel_opts" over to the "struct parallel_processes". In this case we need the number of processes for the kill_children() function, which will be called from a signal handler. To do that adjust this code added in c553c72eed6 (run-command: add an asynchronous parallel child processor, 2015-12-15) so that we use a dedicated "struct parallel_processes_for_signal" for passing data to the signal handler, in addition to the "struct parallel_process" it'll now have access to our "opts" variable. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- run-command.c | 37 ++++++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/run-command.c b/run-command.c index ce7966394d..c772acd743 100644 --- a/run-command.c +++ b/run-command.c @@ -1497,7 +1497,6 @@ enum child_state { }; struct parallel_processes { - const size_t max_processes; size_t nr_processes; struct { @@ -1518,24 +1517,38 @@ struct parallel_processes { struct strbuf buffered_output; /* of finished children */ }; -static void kill_children(const struct parallel_processes *pp, int signo) +struct parallel_processes_for_signal { + const struct run_process_parallel_opts *opts; + const struct parallel_processes *pp; +}; + +static void kill_children(const struct parallel_processes *pp, + const struct run_process_parallel_opts *opts, + int signo) { - for (size_t i = 0; i < pp->max_processes; i++) + for (size_t i = 0; i < opts->processes; i++) if (pp->children[i].state == GIT_CP_WORKING) kill(pp->children[i].process.pid, signo); } -static struct parallel_processes *pp_for_signal; +static void kill_children_signal(const struct parallel_processes_for_signal *pp_sig, + int signo) +{ + kill_children(pp_sig->pp, pp_sig->opts, signo); +} + +static struct parallel_processes_for_signal *pp_for_signal; static void handle_children_on_signal(int signo) { - kill_children(pp_for_signal, signo); + kill_children_signal(pp_for_signal, signo); sigchain_pop(signo); raise(signo); } static void pp_init(struct parallel_processes *pp, - const struct run_process_parallel_opts *opts) + const struct run_process_parallel_opts *opts, + struct parallel_processes_for_signal *pp_sig) { const size_t n = opts->processes; @@ -1561,7 +1574,9 @@ static void pp_init(struct parallel_processes *pp, } } - pp_for_signal = pp; + pp_sig->pp = pp; + pp_sig->opts = opts; + pp_for_signal = pp_sig; sigchain_push_common(handle_children_on_signal); } @@ -1759,8 +1774,8 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts) int i, code; int output_timeout = 100; int spawn_cap = 4; + struct parallel_processes_for_signal pp_sig; struct parallel_processes pp = { - .max_processes = opts->processes, .buffered_output = STRBUF_INIT, }; /* options */ @@ -1772,7 +1787,7 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts) trace2_region_enter_printf(tr2_category, tr2_label, NULL, "max:%d", opts->processes); - pp_init(&pp, opts); + pp_init(&pp, opts, &pp_sig); while (1) { for (i = 0; i < spawn_cap && !pp.shutdown && @@ -1783,7 +1798,7 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts) continue; if (code < 0) { pp.shutdown = 1; - kill_children(&pp, -code); + kill_children(&pp, opts, -code); } break; } @@ -1800,7 +1815,7 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts) if (code) { pp.shutdown = 1; if (code < 0) - kill_children(&pp, -code); + kill_children(&pp, opts,-code); } }