| Message ID | cover-v3-00.15-00000000000-20221012T205712Z-avarab@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | run-command API: pass functions & opts via struct | expand |
Hi Ævar Thank you for condensing the patch series into something more palatable for reviewers. The general consensus from the review club yesterday (we looked at v2) was that it was difficult to follow what patches were relevant to your original intention and what patches were auxiliary QOL changes. Also having too many intermediary patches where you add variables/functions that were later deleted made it hard to visualize which parts of the patch would end up making it into the final state. I appreciate the "show your work" approach you take, but I think that approach is better suited for more difficult patch series where there are significant/difficult to understand functionality changes. In this case, the end state of a refactor is clear and there are no functionality changes so I believe a more condensed series would be easier to review. That being said, I don't believe there is a need to spend more time trying to condense this series so future reviewers have an easier time -- the end result and intentions are enough. Reviewed-by: Calvin Wan <calvinwan@google.com> On Wed, Oct 12, 2022 at 2:03 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > For a general overview see the v2 CL: > https://lore.kernel.org/git/cover-v2-00.22-00000000000-20221012T084850Z-avarab@gmail.com/ > > Changes since v2: > > * Ejected various not-to-the-point of converting to the "opts struct" > per feedback about attempting to make this leaner. > * I kept the size_t change & the online_cpus() fallback, and updated > a commit message for the latter. For "int" v.s. "size_t" once we're > not handling "-1" to mean "use the default" it makes sense to be > unsigned, and if we're re-doing that at this point we'd need > rewrites for "!processes" assumptions. > * Squashed the multi-step introduction of the "opts" struct, per > Phillip's suggestion. > * Fixed a segfault in the v2's 22/22 (this 15/15). > * Got rid of superfluous unsigned conversions of code related to the > "env" member. > > Ævar Arnfjörð Bjarmason (15): > run-command test helper: use "else if" pattern > run-command API: have "run_processes_parallel{,_tr2}()" return void > run-command tests: use "return", not "exit" > run-command API: make "n" parameter a "size_t" > run-command API: don't fall back on online_cpus() > run-command.c: use designated init for pp_init(), add "const" > run-command API: have run_process_parallel() take an "opts" struct > run-command API: move *_tr2() users to "run_processes_parallel()" > run-command.c: make "struct parallel_processes" const if possible > run-command.c: don't copy *_fn to "struct parallel_processes" > run-command.c: don't copy "ungroup" to "struct parallel_processes" > run-command.c: don't copy "data" to "struct parallel_processes" > run-command.c: use "opts->processes", not "pp->max_processes" > run-command.c: pass "opts" further down, and use "opts->processes" > run-command.c: remove "max_processes", add "const" to signal() handler > > builtin/fetch.c | 25 ++-- > builtin/submodule--helper.c | 16 ++- > hook.c | 23 ++-- > run-command.c | 236 ++++++++++++++++-------------------- > run-command.h | 71 ++++++++--- > submodule-config.c | 2 + > submodule.c | 18 ++- > t/helper/test-run-command.c | 77 +++++++----- > t/t5526-fetch-submodules.sh | 8 +- > 9 files changed, 268 insertions(+), 208 deletions(-) > > Range-diff against v2: > 1: bc51dfcb1be < -: ----------- hook tests: fix redirection logic error in 96e7225b310 > 2: 3027f5587a7 < -: ----------- submodule tests: reset "trace.out" between "grep" invocations > 3: c4923358bbd < -: ----------- run-command tests: test stdout of run_command_parallel() > 4: 26e28086252 = 1: d3a2489d9b2 run-command test helper: use "else if" pattern > 5: 5e09dc68fd9 = 2: a2e4fd652c1 run-command API: have "run_processes_parallel{,_tr2}()" return void > 6: e4e91dbbf9e = 3: 4a19de65783 run-command tests: use "return", not "exit" > 7: b90961ae76d < -: ----------- run-command.c: remove dead assignment in while-loop > 8: 279b0430c5d < -: ----------- run-command.c: use C99 "for (TYPE VAR = ..." syntax where useful > 9: a900711270c ! 4: 58018a79b2f run-command API: make "n" parameter a "size_t" > @@ Commit message > > make run-command.o DEVOPTS=extra-all CFLAGS=-Wno-unused-parameter > > - Only has one (and new) -Wsigned-compare warning, about a comparison of > - "i" to online_cpus(), a subsequent commit will adjust & deal with > - online_cpus() and that warning. > + 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: > > @@ run-command.c: static void pp_cleanup(struct parallel_processes *pp) > > for (i = 0; i < pp->max_processes; i++) > if (pp->children[i].state == GIT_CP_FREE) > -@@ run-command.c: static int pp_start_one(struct parallel_processes *pp) > - > - static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout) > - { > -- int i; > -- > - while (poll(pp->pfd, pp->max_processes, output_timeout) < 0) { > - if (errno == EINTR) > - continue; > @@ run-command.c: static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout) > } > > 10: eb9d672b0d8 ! 5: e230701dff6 run-command API: don't fall back on online_cpus() > @@ Commit message > child processor, 2015-12-15). > > Most of our code in-tree that scales up to "online_cpus()" by default > - calls that function by itself. By having these callers of the > - "run_processes_parallel()" API do the same we can in subsequent > - commits pass all arguments down as a "const struct". > + 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 > @@ submodule-config.c: int parse_submodule_fetchjobs(const char *var, const char *v > > ## t/t5526-fetch-submodules.sh ## > @@ t/t5526-fetch-submodules.sh: 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 && > - >trace.out && > +- 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 && > 11: aedda10d8e1 ! 6: df2ca5dd097 run-command.c: use designated init for pp_init(), add "const" > @@ run-command.c: void run_processes_parallel(size_t n, > - ungroup); > + pp_init(&pp, get_next_task, start_failure, task_finished); > while (1) { > - for (int i = 0; > + for (i = 0; > i < spawn_cap && !pp.shutdown && > 12: fde2af11579 < -: ----------- run-command API: add nascent "struct run_process_parallel_opts" > 13: 01e894bed90 < -: ----------- run-command API: make run_process_parallel{,_tr2}() thin wrappers > 14: 41c2886b44b ! 7: eaed3d8838d run-command API: have run_process_parallel() take an "opts" struct > @@ Metadata > ## Commit message ## > run-command API: have run_process_parallel() take an "opts" struct > > - Have the "run_process_parallel()" function take an "opts" struct, > - which allows us to eliminate it as a wrapper for > - "run_processes_parallel_1()". > + 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, only the two users that > + 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, allowing us to delete that > - global in favor of passing "ungroup" via the "opts" struct. As noted > - in fd3aaf53f71 (run-command: add an "ungroup" option to > - run_process_parallel(), 2022-06-07) which added > - "run_processes_parallel_ungroup" passing this as a global was a hack > - to produce a small regression fix for "maint". > + "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 <avarab@gmail.com> > > @@ run-command.c: enum child_state { > struct parallel_processes { > void *const data; > > +@@ run-command.c: 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!"); > @@ run-command.c: static int pp_collect_finished(struct parallel_processes *pp) > return result; > } > > --static void run_processes_parallel_1(const struct run_process_parallel_opts *opts, void *pp_cb) > +-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 code; > + int i, code; > int output_timeout = 100; > int spawn_cap = 4; > +- int ungroup = run_processes_parallel_ungroup; > struct parallel_processes pp = { > - .max_processes = opts->processes, > +- .max_processes = n, > - .data = pp_cb, > ++ .max_processes = opts->processes, > + .data = opts->data, > .buffered_output = STRBUF_INIT, > -- .ungroup = run_processes_parallel_ungroup, > +- .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; > ++ /* 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); > -@@ run-command.c: static void run_processes_parallel_1(const struct run_process_parallel_opts *opt > - trace2_region_leave(tr2_category, tr2_label, NULL); > ++ 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 && > +@@ run-command.c: 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 { > +@@ run-command.c: 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(size_t processes, > -- get_next_task_fn get_next_task, > -- start_failure_fn start_failure, > -- task_finished_fn task_finished, > -- void *pp_cb) > --{ > -- const struct run_process_parallel_opts opts = { > -- .processes = processes, > -- .ungroup = run_processes_parallel_ungroup, > -- > -- .get_next_task = get_next_task, > -- .start_failure = start_failure, > -- .task_finished = task_finished, > -- }; > -- > -- run_processes_parallel_1(&opts, pp_cb); > --} > -- > - void run_processes_parallel_tr2(size_t processes, get_next_task_fn get_next_task, > +-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, > -@@ run-command.c: void run_processes_parallel_tr2(size_t processes, get_next_task_fn get_next_task > - const struct run_process_parallel_opts opts = { > - .tr2_category = tr2_category, > - .tr2_label = tr2_label, > -- > - .processes = processes, > -- .ungroup = run_processes_parallel_ungroup, > + 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, > > - .get_next_task = get_next_task, > - .start_failure = start_failure, > - .task_finished = task_finished, > - }; > +- 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, > ++ }; > > -- run_processes_parallel_1(&opts, pp_cb); > +- trace2_region_leave(tr2_category, tr2_label, NULL); > + run_processes_parallel(&opts); > } > > int run_auto_maintenance(int quiet) > > ## run-command.h ## > -@@ run-command.h: struct run_process_parallel_opts > - * NULL to omit any special handling. > - */ > - task_finished_fn task_finished; > +@@ run-command.h: 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 get_next_task_fn is called to obtain the data > ++ * 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 > -@@ run-command.h: struct run_process_parallel_opts > + * (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 > +@@ run-command.h: 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. > @@ run-command.h: struct run_process_parallel_opts > - * API reads that setting. > */ > -extern int run_processes_parallel_ungroup; > --void run_processes_parallel(size_t processes, > +-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, > ++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); > + > + /** > > ## t/helper/test-run-command.c ## > @@ t/helper/test-run-command.c: static const char * const testsuite_usage[] = { > 15: 391d1d99d91 = 8: c19c60b2e95 run-command API: move *_tr2() users to "run_processes_parallel()" > 16: acac50cc1a5 = 9: 99a4f4f6b9c run-command.c: make "struct parallel_processes" const if possible > 17: fdd64236985 = 10: bf67e24bcc5 run-command.c: don't copy *_fn to "struct parallel_processes" > 18: 17f34d81ecd = 11: 06de42adc2e run-command.c: don't copy "ungroup" to "struct parallel_processes" > 19: 9cbee2dfe76 = 12: 3081dfc49d1 run-command.c: don't copy "data" to "struct parallel_processes" > 20: 2dabed9e155 ! 13: b7c10f6a23f run-command.c: use "opts->processes", not "pp->max_processes" > @@ run-command.c: static int pp_collect_finished(struct parallel_processes *pp, > > @@ run-command.c: void run_processes_parallel(const struct run_process_parallel_opts *opts) > while (1) { > - for (int i = 0; > + for (i = 0; > i < spawn_cap && !pp.shutdown && > - pp.nr_processes < pp.max_processes; > + pp.nr_processes < opts->processes; > 21: c1a286a8ebb ! 14: 4856d6a4674 run-command.c: pass "opts" further down, and use "opts->processes" > @@ run-command.c: static int pp_start_one(struct parallel_processes *pp, > + const struct run_process_parallel_opts *opts, > + int output_timeout) > { > -- while (poll(pp->pfd, pp->max_processes, output_timeout) < 0) { > -+ while (poll(pp->pfd, opts->processes, output_timeout) < 0) { > + 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); > 22: 541f41566e7 ! 15: 39a20be0cbb run-command.c: remove "pp->max_processes", add "const" to signal() handler > @@ Metadata > Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > > ## Commit message ## > - run-command.c: remove "pp->max_processes", add "const" to signal() handler > + run-command.c: remove "max_processes", add "const" to signal() handler > > 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 > @@ run-command.c: 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); > } > > @@ run-command.c: void run_processes_parallel(const struct run_process_parallel_opts *opts) > - int code; > + int i, code; > int output_timeout = 100; > int spawn_cap = 4; > + struct parallel_processes_for_signal pp_sig; > @@ run-command.c: void run_processes_parallel(const struct run_process_parallel_opt > - pp_init(&pp, opts); > + pp_init(&pp, opts, &pp_sig); > while (1) { > - for (int i = 0; > + for (i = 0; > i < spawn_cap && !pp.shutdown && > @@ run-command.c: void run_processes_parallel(const struct run_process_parallel_opts *opts) > continue; > -- > 2.38.0.971.ge79ff6d20e7 >
Calvin Wan <calvinwan@google.com> writes: > Hi Ævar > > Thank you for condensing the patch series into something more palatable > for reviewers. The general consensus from the review club yesterday (we > looked at v2) was that it was difficult to follow what patches were > relevant to your original intention and what patches were auxiliary QOL > changes. Also having too many intermediary patches where you add > variables/functions that were later deleted made it hard to visualize > which parts of the patch would end up making it into the final state. I > appreciate the "show your work" approach you take, but I think that > approach is better suited for more difficult patch series where there > are significant/difficult to understand functionality changes. In this > case, the end state of a refactor is clear and there are no > functionality changes so I believe a more condensed series would be > easier to review. That being said, I don't believe there is a need to > spend more time trying to condense this series so future reviewers have > an easier time -- the end result and intentions are enough. > > Reviewed-by: Calvin Wan <calvinwan@google.com> Thanks, both.
Hi Ævar On 12/10/2022 22:02, Ævar Arnfjörð Bjarmason wrote: > For a general overview see the v2 CL: > https://lore.kernel.org/git/cover-v2-00.22-00000000000-20221012T084850Z-avarab@gmail.com/ > > Changes since v2: > > * Ejected various not-to-the-point of converting to the "opts struct" > per feedback about attempting to make this leaner. We're back to the same number of patches as v1 but you've removed three test cleanups and squashed three patches together which means there are five new patches in this version - what are they doing? > * I kept the size_t change & the online_cpus() fallback, and updated > a commit message for the latter. For "int" v.s. "size_t" once we're > not handling "-1" to mean "use the default" it makes sense to be > unsigned, and if we're re-doing that at this point we'd need > rewrites for "!processes" assumptions. I left some comments about this, I think the size_t change is taking us in the wrong direction as it is introducing a number of new implicit signed->unsigned conversions. I'm still not sure why you need the online_cups() changes c.f. https://lore.kernel.org/git/8f95fbdb-b211-56af-8693-0e5a84afebac@gmail.com/ which has never had a reply > * Squashed the multi-step introduction of the "opts" struct, per > Phillip's suggestion. That's most welcome, thanks > * Fixed a segfault in the v2's 22/22 (this 15/15). > * Got rid of superfluous unsigned conversions of code related to the > "env" member. I've read through to 'API: move *_tr2() users to "run_processes_parallel()"' and apart from my comments above they seem reasonable (I'm not that taken with the const struct members but it is a fairly small change) Best Wishes Phillip > Ævar Arnfjörð Bjarmason (15): > run-command test helper: use "else if" pattern > run-command API: have "run_processes_parallel{,_tr2}()" return void > run-command tests: use "return", not "exit" > run-command API: make "n" parameter a "size_t" > run-command API: don't fall back on online_cpus() > run-command.c: use designated init for pp_init(), add "const" > run-command API: have run_process_parallel() take an "opts" struct > run-command API: move *_tr2() users to "run_processes_parallel()" > run-command.c: make "struct parallel_processes" const if possible > run-command.c: don't copy *_fn to "struct parallel_processes" > run-command.c: don't copy "ungroup" to "struct parallel_processes" > run-command.c: don't copy "data" to "struct parallel_processes" > run-command.c: use "opts->processes", not "pp->max_processes" > run-command.c: pass "opts" further down, and use "opts->processes" > run-command.c: remove "max_processes", add "const" to signal() handler > > builtin/fetch.c | 25 ++-- > builtin/submodule--helper.c | 16 ++- > hook.c | 23 ++-- > run-command.c | 236 ++++++++++++++++-------------------- > run-command.h | 71 ++++++++--- > submodule-config.c | 2 + > submodule.c | 18 ++- > t/helper/test-run-command.c | 77 +++++++----- > t/t5526-fetch-submodules.sh | 8 +- > 9 files changed, 268 insertions(+), 208 deletions(-) > > Range-diff against v2: > 1: bc51dfcb1be < -: ----------- hook tests: fix redirection logic error in 96e7225b310 > 2: 3027f5587a7 < -: ----------- submodule tests: reset "trace.out" between "grep" invocations > 3: c4923358bbd < -: ----------- run-command tests: test stdout of run_command_parallel() > 4: 26e28086252 = 1: d3a2489d9b2 run-command test helper: use "else if" pattern > 5: 5e09dc68fd9 = 2: a2e4fd652c1 run-command API: have "run_processes_parallel{,_tr2}()" return void > 6: e4e91dbbf9e = 3: 4a19de65783 run-command tests: use "return", not "exit" > 7: b90961ae76d < -: ----------- run-command.c: remove dead assignment in while-loop > 8: 279b0430c5d < -: ----------- run-command.c: use C99 "for (TYPE VAR = ..." syntax where useful > 9: a900711270c ! 4: 58018a79b2f run-command API: make "n" parameter a "size_t" > @@ Commit message > > make run-command.o DEVOPTS=extra-all CFLAGS=-Wno-unused-parameter > > - Only has one (and new) -Wsigned-compare warning, about a comparison of > - "i" to online_cpus(), a subsequent commit will adjust & deal with > - online_cpus() and that warning. > + 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: > > @@ run-command.c: static void pp_cleanup(struct parallel_processes *pp) > > for (i = 0; i < pp->max_processes; i++) > if (pp->children[i].state == GIT_CP_FREE) > -@@ run-command.c: static int pp_start_one(struct parallel_processes *pp) > - > - static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout) > - { > -- int i; > -- > - while (poll(pp->pfd, pp->max_processes, output_timeout) < 0) { > - if (errno == EINTR) > - continue; > @@ run-command.c: static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout) > } > > 10: eb9d672b0d8 ! 5: e230701dff6 run-command API: don't fall back on online_cpus() > @@ Commit message > child processor, 2015-12-15). > > Most of our code in-tree that scales up to "online_cpus()" by default > - calls that function by itself. By having these callers of the > - "run_processes_parallel()" API do the same we can in subsequent > - commits pass all arguments down as a "const struct". > + 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 > @@ submodule-config.c: int parse_submodule_fetchjobs(const char *var, const char *v > > ## t/t5526-fetch-submodules.sh ## > @@ t/t5526-fetch-submodules.sh: 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 && > - >trace.out && > +- 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 && > 11: aedda10d8e1 ! 6: df2ca5dd097 run-command.c: use designated init for pp_init(), add "const" > @@ run-command.c: void run_processes_parallel(size_t n, > - ungroup); > + pp_init(&pp, get_next_task, start_failure, task_finished); > while (1) { > - for (int i = 0; > + for (i = 0; > i < spawn_cap && !pp.shutdown && > 12: fde2af11579 < -: ----------- run-command API: add nascent "struct run_process_parallel_opts" > 13: 01e894bed90 < -: ----------- run-command API: make run_process_parallel{,_tr2}() thin wrappers > 14: 41c2886b44b ! 7: eaed3d8838d run-command API: have run_process_parallel() take an "opts" struct > @@ Metadata > ## Commit message ## > run-command API: have run_process_parallel() take an "opts" struct > > - Have the "run_process_parallel()" function take an "opts" struct, > - which allows us to eliminate it as a wrapper for > - "run_processes_parallel_1()". > + 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, only the two users that > + 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, allowing us to delete that > - global in favor of passing "ungroup" via the "opts" struct. As noted > - in fd3aaf53f71 (run-command: add an "ungroup" option to > - run_process_parallel(), 2022-06-07) which added > - "run_processes_parallel_ungroup" passing this as a global was a hack > - to produce a small regression fix for "maint". > + "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 <avarab@gmail.com> > > @@ run-command.c: enum child_state { > struct parallel_processes { > void *const data; > > +@@ run-command.c: 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!"); > @@ run-command.c: static int pp_collect_finished(struct parallel_processes *pp) > return result; > } > > --static void run_processes_parallel_1(const struct run_process_parallel_opts *opts, void *pp_cb) > +-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 code; > + int i, code; > int output_timeout = 100; > int spawn_cap = 4; > +- int ungroup = run_processes_parallel_ungroup; > struct parallel_processes pp = { > - .max_processes = opts->processes, > +- .max_processes = n, > - .data = pp_cb, > ++ .max_processes = opts->processes, > + .data = opts->data, > .buffered_output = STRBUF_INIT, > -- .ungroup = run_processes_parallel_ungroup, > +- .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; > ++ /* 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); > -@@ run-command.c: static void run_processes_parallel_1(const struct run_process_parallel_opts *opt > - trace2_region_leave(tr2_category, tr2_label, NULL); > ++ 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 && > +@@ run-command.c: 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 { > +@@ run-command.c: 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(size_t processes, > -- get_next_task_fn get_next_task, > -- start_failure_fn start_failure, > -- task_finished_fn task_finished, > -- void *pp_cb) > --{ > -- const struct run_process_parallel_opts opts = { > -- .processes = processes, > -- .ungroup = run_processes_parallel_ungroup, > -- > -- .get_next_task = get_next_task, > -- .start_failure = start_failure, > -- .task_finished = task_finished, > -- }; > -- > -- run_processes_parallel_1(&opts, pp_cb); > --} > -- > - void run_processes_parallel_tr2(size_t processes, get_next_task_fn get_next_task, > +-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, > -@@ run-command.c: void run_processes_parallel_tr2(size_t processes, get_next_task_fn get_next_task > - const struct run_process_parallel_opts opts = { > - .tr2_category = tr2_category, > - .tr2_label = tr2_label, > -- > - .processes = processes, > -- .ungroup = run_processes_parallel_ungroup, > + 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, > > - .get_next_task = get_next_task, > - .start_failure = start_failure, > - .task_finished = task_finished, > - }; > +- 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, > ++ }; > > -- run_processes_parallel_1(&opts, pp_cb); > +- trace2_region_leave(tr2_category, tr2_label, NULL); > + run_processes_parallel(&opts); > } > > int run_auto_maintenance(int quiet) > > ## run-command.h ## > -@@ run-command.h: struct run_process_parallel_opts > - * NULL to omit any special handling. > - */ > - task_finished_fn task_finished; > +@@ run-command.h: 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 get_next_task_fn is called to obtain the data > ++ * 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 > -@@ run-command.h: struct run_process_parallel_opts > + * (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 > +@@ run-command.h: 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. > @@ run-command.h: struct run_process_parallel_opts > - * API reads that setting. > */ > -extern int run_processes_parallel_ungroup; > --void run_processes_parallel(size_t processes, > +-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, > ++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); > + > + /** > > ## t/helper/test-run-command.c ## > @@ t/helper/test-run-command.c: static const char * const testsuite_usage[] = { > 15: 391d1d99d91 = 8: c19c60b2e95 run-command API: move *_tr2() users to "run_processes_parallel()" > 16: acac50cc1a5 = 9: 99a4f4f6b9c run-command.c: make "struct parallel_processes" const if possible > 17: fdd64236985 = 10: bf67e24bcc5 run-command.c: don't copy *_fn to "struct parallel_processes" > 18: 17f34d81ecd = 11: 06de42adc2e run-command.c: don't copy "ungroup" to "struct parallel_processes" > 19: 9cbee2dfe76 = 12: 3081dfc49d1 run-command.c: don't copy "data" to "struct parallel_processes" > 20: 2dabed9e155 ! 13: b7c10f6a23f run-command.c: use "opts->processes", not "pp->max_processes" > @@ run-command.c: static int pp_collect_finished(struct parallel_processes *pp, > > @@ run-command.c: void run_processes_parallel(const struct run_process_parallel_opts *opts) > while (1) { > - for (int i = 0; > + for (i = 0; > i < spawn_cap && !pp.shutdown && > - pp.nr_processes < pp.max_processes; > + pp.nr_processes < opts->processes; > 21: c1a286a8ebb ! 14: 4856d6a4674 run-command.c: pass "opts" further down, and use "opts->processes" > @@ run-command.c: static int pp_start_one(struct parallel_processes *pp, > + const struct run_process_parallel_opts *opts, > + int output_timeout) > { > -- while (poll(pp->pfd, pp->max_processes, output_timeout) < 0) { > -+ while (poll(pp->pfd, opts->processes, output_timeout) < 0) { > + 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); > 22: 541f41566e7 ! 15: 39a20be0cbb run-command.c: remove "pp->max_processes", add "const" to signal() handler > @@ Metadata > Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > > ## Commit message ## > - run-command.c: remove "pp->max_processes", add "const" to signal() handler > + run-command.c: remove "max_processes", add "const" to signal() handler > > 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 > @@ run-command.c: 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); > } > > @@ run-command.c: void run_processes_parallel(const struct run_process_parallel_opts *opts) > - int code; > + int i, code; > int output_timeout = 100; > int spawn_cap = 4; > + struct parallel_processes_for_signal pp_sig; > @@ run-command.c: void run_processes_parallel(const struct run_process_parallel_opt > - pp_init(&pp, opts); > + pp_init(&pp, opts, &pp_sig); > while (1) { > - for (int i = 0; > + for (i = 0; > i < spawn_cap && !pp.shutdown && > @@ run-command.c: void run_processes_parallel(const struct run_process_parallel_opts *opts) > continue;
On Fri, Oct 14 2022, Phillip Wood wrote: > Hi Ævar Hi, thanks for taking a look again. > On 12/10/2022 22:02, Ævar Arnfjörð Bjarmason wrote: >> For a general overview see the v2 CL: >> https://lore.kernel.org/git/cover-v2-00.22-00000000000-20221012T084850Z-avarab@gmail.com/ >> Changes since v2: >> * Ejected various not-to-the-point of converting to the "opts >> struct" >> per feedback about attempting to make this leaner. > > We're back to the same number of patches as v1 but you've removed > three test cleanups and squashed three patches together which means > there are five new patches in this version - what are they doing? There's nothing *new* in this version, but per the range-diff of v2 some of the new ones in that version were kept here. I think that's what you're taling about. Mainly it's the 'Only the "ungroup" was here in v1[...]' part of the v2 CL: 1. https://lore.kernel.org/git/cover-v2-00.22-00000000000-20221012T084850Z-avarab@gmail.com/ >> * I kept the size_t change & the online_cpus() fallback, and updated >> a commit message for the latter. For "int" v.s. "size_t" once we're >> not handling "-1" to mean "use the default" it makes sense to be >> unsigned, and if we're re-doing that at this point we'd need >> rewrites for "!processes" assumptions. > > I left some comments about this, I think the size_t change is taking > us in the wrong direction as it is introducing a number of new > implicit signed->unsigned conversions. I'm still not sure why you need > the online_cups() changes c.f. > https://lore.kernel.org/git/8f95fbdb-b211-56af-8693-0e5a84afebac@gmail.com/ > which has never had a reply Sorry about the non-reply there, between that & later discussion I tried to address that in the "Keeping this default behavior just for[...]" commit message update in this v3 (see range-diff). But no, those changes are not strictly needed. But it's a trade-off I decided to take. In the v1 (linked above) you pointed out that we could simply copy this field to the "struct pp" (a shorthand for "struct parallel_processes", I assume). That's true, but for maintaining & understanding this API I think it's much easier to reason about it when all of our user options are "const", and we don't second guess any of those, and the "struct parallel_processes" For the v1 I can see what that was easy to miss, as we still kept the copy of the number of processes in the "struct parallel_processes". In the v2 and this v3 we get to the point where we can remove that, and "ungroup", the copies of the callbacks etc. So, leaving out the "provide a default" seemed worth it in the end, it's just 4 additional lines in the callers per the 05/15 (most of them had those already). You also had a related concern in 04/05 (which I'm taking the liberty of replying ot here): https://lore.kernel.org/git/a7463bc5-9a92-8f0f-c0ee-e72fbbeedc09@dunelm.org.uk/ So, first I disagree with it "going in the wrong direction". We've been converting more things to size_t. For e.g. an "int nr_processes" we can expect that we'll want to e.g. have a corresponding "struct string_list" whose "nr" is a "size_t" (or similar aggregate types). By mixing the two we're mixing types forever with associated warnings (& suppressions). We've been changing these sort of things to a "size_t" elsewhere, e.g. dd38e9e510c (trace2: use size_t alloc,nr_open_regions in tr2tls_thread_ctx, 2022-10-12) is one recent example.. But yes, we do incur warnings under DEVOPTS=extra-all now because things outside of the API are still "int" in some cases, just as we do with e.g. "struct string_list" and its users. As to your: Before this patch we could have caught a negative value of n with an assertion. After this patch a negative value passed will become a large positive value (you have audited the current callers but that does not protect us from future mistakes). I that's a good point. In this case I thought the likelihood that anyone would accidentally pass ".processes = -1" or whatever wasn't worth worrying about. If you think it's worth worrying about I think that concern would be easily addressed e.g. with: if (n == SIZE_MAX) BUG("overflow?"); Or whatever. A much likelier edge case IMO is that you'd have some pattern where you init a "processes" with "{ 0 }" or whatever, so it's zero, and then we'd interpret that zero as online_cpus(), which e.g. for the in-flight paralellizing of certain "git submodule" operations is probably too high a number. Since we don't interpret !n or n < 1 as online_cpus() anymore we can BUG() out on it, which I think is an improvement. The 4 lines of additions to the relevant callers to call online_cpus() themselves are better than having those or a new caller potentially have this DWYMery from the low-level API. >> * Squashed the multi-step introduction of the "opts" struct, per >> Phillip's suggestion. > That's most welcome, thanks Glad you like it!
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > You also had a related concern in 04/05 (which I'm taking the liberty of > replying ot here): > > https://lore.kernel.org/git/a7463bc5-9a92-8f0f-c0ee-e72fbbeedc09@dunelm.org.uk/ > > So, first I disagree with it "going in the wrong direction". We've been > converting more things to size_t. For e.g. an "int nr_processes" we can > expect that we'll want to e.g. have a corresponding "struct string_list" > whose "nr" is a "size_t" (or similar aggregate types). I do not quite see why that is relevant. We may create list of textual descriptions from list of processes, so we expect to be able to loop "for (int i = 0; i < nr_processes; i++)" and access i-th element of the corresponding string_list. As long as int is narrower than size_t (and it is wide enough to count the processes we are going to ever spawn) there is no issue, no? Also using signed type is so much more convenient to signal an error (imagine asking for a process's i that is between 0..nr_processes with some other key in your database, and having to answer "no such process known to me"). > By mixing the two we're mixing types forever with associated warnings (& > suppressions). That is a complaint about shortcomings of the tool that gives irrelevant warning, no? It is not a good reason to make the code worse. Compilers, editors, and linters are to serve the code, not the other way around.
For a general overview see the v2 CL: https://lore.kernel.org/git/cover-v2-00.22-00000000000-20221012T084850Z-avarab@gmail.com/ Changes since v2: * Ejected various not-to-the-point of converting to the "opts struct" per feedback about attempting to make this leaner. * I kept the size_t change & the online_cpus() fallback, and updated a commit message for the latter. For "int" v.s. "size_t" once we're not handling "-1" to mean "use the default" it makes sense to be unsigned, and if we're re-doing that at this point we'd need rewrites for "!processes" assumptions. * Squashed the multi-step introduction of the "opts" struct, per Phillip's suggestion. * Fixed a segfault in the v2's 22/22 (this 15/15). * Got rid of superfluous unsigned conversions of code related to the "env" member. Ævar Arnfjörð Bjarmason (15): run-command test helper: use "else if" pattern run-command API: have "run_processes_parallel{,_tr2}()" return void run-command tests: use "return", not "exit" run-command API: make "n" parameter a "size_t" run-command API: don't fall back on online_cpus() run-command.c: use designated init for pp_init(), add "const" run-command API: have run_process_parallel() take an "opts" struct run-command API: move *_tr2() users to "run_processes_parallel()" run-command.c: make "struct parallel_processes" const if possible run-command.c: don't copy *_fn to "struct parallel_processes" run-command.c: don't copy "ungroup" to "struct parallel_processes" run-command.c: don't copy "data" to "struct parallel_processes" run-command.c: use "opts->processes", not "pp->max_processes" run-command.c: pass "opts" further down, and use "opts->processes" run-command.c: remove "max_processes", add "const" to signal() handler builtin/fetch.c | 25 ++-- builtin/submodule--helper.c | 16 ++- hook.c | 23 ++-- run-command.c | 236 ++++++++++++++++-------------------- run-command.h | 71 ++++++++--- submodule-config.c | 2 + submodule.c | 18 ++- t/helper/test-run-command.c | 77 +++++++----- t/t5526-fetch-submodules.sh | 8 +- 9 files changed, 268 insertions(+), 208 deletions(-) Range-diff against v2: 1: bc51dfcb1be < -: ----------- hook tests: fix redirection logic error in 96e7225b310 2: 3027f5587a7 < -: ----------- submodule tests: reset "trace.out" between "grep" invocations 3: c4923358bbd < -: ----------- run-command tests: test stdout of run_command_parallel() 4: 26e28086252 = 1: d3a2489d9b2 run-command test helper: use "else if" pattern 5: 5e09dc68fd9 = 2: a2e4fd652c1 run-command API: have "run_processes_parallel{,_tr2}()" return void 6: e4e91dbbf9e = 3: 4a19de65783 run-command tests: use "return", not "exit" 7: b90961ae76d < -: ----------- run-command.c: remove dead assignment in while-loop 8: 279b0430c5d < -: ----------- run-command.c: use C99 "for (TYPE VAR = ..." syntax where useful 9: a900711270c ! 4: 58018a79b2f run-command API: make "n" parameter a "size_t" @@ Commit message make run-command.o DEVOPTS=extra-all CFLAGS=-Wno-unused-parameter - Only has one (and new) -Wsigned-compare warning, about a comparison of - "i" to online_cpus(), a subsequent commit will adjust & deal with - online_cpus() and that warning. + 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: @@ run-command.c: static void pp_cleanup(struct parallel_processes *pp) for (i = 0; i < pp->max_processes; i++) if (pp->children[i].state == GIT_CP_FREE) -@@ run-command.c: static int pp_start_one(struct parallel_processes *pp) - - static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout) - { -- int i; -- - while (poll(pp->pfd, pp->max_processes, output_timeout) < 0) { - if (errno == EINTR) - continue; @@ run-command.c: static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout) } 10: eb9d672b0d8 ! 5: e230701dff6 run-command API: don't fall back on online_cpus() @@ Commit message child processor, 2015-12-15). Most of our code in-tree that scales up to "online_cpus()" by default - calls that function by itself. By having these callers of the - "run_processes_parallel()" API do the same we can in subsequent - commits pass all arguments down as a "const struct". + 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 @@ submodule-config.c: int parse_submodule_fetchjobs(const char *var, const char *v ## t/t5526-fetch-submodules.sh ## @@ t/t5526-fetch-submodules.sh: 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 && - >trace.out && +- 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 && 11: aedda10d8e1 ! 6: df2ca5dd097 run-command.c: use designated init for pp_init(), add "const" @@ run-command.c: void run_processes_parallel(size_t n, - ungroup); + pp_init(&pp, get_next_task, start_failure, task_finished); while (1) { - for (int i = 0; + for (i = 0; i < spawn_cap && !pp.shutdown && 12: fde2af11579 < -: ----------- run-command API: add nascent "struct run_process_parallel_opts" 13: 01e894bed90 < -: ----------- run-command API: make run_process_parallel{,_tr2}() thin wrappers 14: 41c2886b44b ! 7: eaed3d8838d run-command API: have run_process_parallel() take an "opts" struct @@ Metadata ## Commit message ## run-command API: have run_process_parallel() take an "opts" struct - Have the "run_process_parallel()" function take an "opts" struct, - which allows us to eliminate it as a wrapper for - "run_processes_parallel_1()". + 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, only the two users that + 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, allowing us to delete that - global in favor of passing "ungroup" via the "opts" struct. As noted - in fd3aaf53f71 (run-command: add an "ungroup" option to - run_process_parallel(), 2022-06-07) which added - "run_processes_parallel_ungroup" passing this as a global was a hack - to produce a small regression fix for "maint". + "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 <avarab@gmail.com> @@ run-command.c: enum child_state { struct parallel_processes { void *const data; +@@ run-command.c: 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!"); @@ run-command.c: static int pp_collect_finished(struct parallel_processes *pp) return result; } --static void run_processes_parallel_1(const struct run_process_parallel_opts *opts, void *pp_cb) +-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 code; + int i, code; int output_timeout = 100; int spawn_cap = 4; +- int ungroup = run_processes_parallel_ungroup; struct parallel_processes pp = { - .max_processes = opts->processes, +- .max_processes = n, - .data = pp_cb, ++ .max_processes = opts->processes, + .data = opts->data, .buffered_output = STRBUF_INIT, -- .ungroup = run_processes_parallel_ungroup, +- .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; ++ /* 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); -@@ run-command.c: static void run_processes_parallel_1(const struct run_process_parallel_opts *opt - trace2_region_leave(tr2_category, tr2_label, NULL); ++ 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 && +@@ run-command.c: 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 { +@@ run-command.c: 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(size_t processes, -- get_next_task_fn get_next_task, -- start_failure_fn start_failure, -- task_finished_fn task_finished, -- void *pp_cb) --{ -- const struct run_process_parallel_opts opts = { -- .processes = processes, -- .ungroup = run_processes_parallel_ungroup, -- -- .get_next_task = get_next_task, -- .start_failure = start_failure, -- .task_finished = task_finished, -- }; -- -- run_processes_parallel_1(&opts, pp_cb); --} -- - void run_processes_parallel_tr2(size_t processes, get_next_task_fn get_next_task, +-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, -@@ run-command.c: void run_processes_parallel_tr2(size_t processes, get_next_task_fn get_next_task - const struct run_process_parallel_opts opts = { - .tr2_category = tr2_category, - .tr2_label = tr2_label, -- - .processes = processes, -- .ungroup = run_processes_parallel_ungroup, + 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, - .get_next_task = get_next_task, - .start_failure = start_failure, - .task_finished = task_finished, - }; +- 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, ++ }; -- run_processes_parallel_1(&opts, pp_cb); +- trace2_region_leave(tr2_category, tr2_label, NULL); + run_processes_parallel(&opts); } int run_auto_maintenance(int quiet) ## run-command.h ## -@@ run-command.h: struct run_process_parallel_opts - * NULL to omit any special handling. - */ - task_finished_fn task_finished; +@@ run-command.h: 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 get_next_task_fn is called to obtain the data ++ * 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 -@@ run-command.h: struct run_process_parallel_opts + * (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 +@@ run-command.h: 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. @@ run-command.h: struct run_process_parallel_opts - * API reads that setting. */ -extern int run_processes_parallel_ungroup; --void run_processes_parallel(size_t processes, +-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, ++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); + + /** ## t/helper/test-run-command.c ## @@ t/helper/test-run-command.c: static const char * const testsuite_usage[] = { 15: 391d1d99d91 = 8: c19c60b2e95 run-command API: move *_tr2() users to "run_processes_parallel()" 16: acac50cc1a5 = 9: 99a4f4f6b9c run-command.c: make "struct parallel_processes" const if possible 17: fdd64236985 = 10: bf67e24bcc5 run-command.c: don't copy *_fn to "struct parallel_processes" 18: 17f34d81ecd = 11: 06de42adc2e run-command.c: don't copy "ungroup" to "struct parallel_processes" 19: 9cbee2dfe76 = 12: 3081dfc49d1 run-command.c: don't copy "data" to "struct parallel_processes" 20: 2dabed9e155 ! 13: b7c10f6a23f run-command.c: use "opts->processes", not "pp->max_processes" @@ run-command.c: static int pp_collect_finished(struct parallel_processes *pp, @@ run-command.c: void run_processes_parallel(const struct run_process_parallel_opts *opts) while (1) { - for (int i = 0; + for (i = 0; i < spawn_cap && !pp.shutdown && - pp.nr_processes < pp.max_processes; + pp.nr_processes < opts->processes; 21: c1a286a8ebb ! 14: 4856d6a4674 run-command.c: pass "opts" further down, and use "opts->processes" @@ run-command.c: static int pp_start_one(struct parallel_processes *pp, + const struct run_process_parallel_opts *opts, + int output_timeout) { -- while (poll(pp->pfd, pp->max_processes, output_timeout) < 0) { -+ while (poll(pp->pfd, opts->processes, output_timeout) < 0) { + 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); 22: 541f41566e7 ! 15: 39a20be0cbb run-command.c: remove "pp->max_processes", add "const" to signal() handler @@ Metadata Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com> ## Commit message ## - run-command.c: remove "pp->max_processes", add "const" to signal() handler + run-command.c: remove "max_processes", add "const" to signal() handler 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 @@ run-command.c: 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); } @@ run-command.c: void run_processes_parallel(const struct run_process_parallel_opts *opts) - int code; + int i, code; int output_timeout = 100; int spawn_cap = 4; + struct parallel_processes_for_signal pp_sig; @@ run-command.c: void run_processes_parallel(const struct run_process_parallel_opt - pp_init(&pp, opts); + pp_init(&pp, opts, &pp_sig); while (1) { - for (int i = 0; + for (i = 0; i < spawn_cap && !pp.shutdown && @@ run-command.c: void run_processes_parallel(const struct run_process_parallel_opts *opts) continue;