On Wed, May 18, 2022 at 10:05:18PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> Add a new "struct run_process_parallel_opts" to replace the growing
> run_processes_parallel() and run_processes_parallel_tr2() argument
> lists. This refactoring makes it easier to add new options and
> parameters easier.
>
> The *_tr2() variant of the function was added in ee4512ed481 (trace2:
> create new combined trace facility, 2019-02-22), and has subsequently
> been used by every caller except t/helper/test-run-command.c.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
> ---
[...]
> diff --git a/run-command.h b/run-command.h
> index 5bd0c933e80..b0268ed3db1 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -458,6 +458,32 @@ typedef int (*task_finished_fn)(int result,
> void *pp_task_cb);
>
> /**
> + * Options to pass to run_processes_parallel(), { 0 }-initialized
> + * means no options. Fields:
> + *
> + * tr2_category & tr2_label: sets the trace2 category and label for
> + * logging. These must either be unset, or both of them must be set.
I see this comment...
> + *
> + * jobs: see 'n' in run_processes_parallel() below.
> + *
> + * *_fn & data: see run_processes_parallel() below.
> + */
> +struct run_process_parallel_opts
> +{
> + const char *tr2_category;
> + const char *tr2_label;
> +
> + int jobs;
> +
> + get_next_task_fn get_next_task;
> + start_failure_fn start_failure;
> + task_finished_fn task_finished;
> + void *data;
> +};
[moved snippet]
> -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)
> +int run_processes_parallel(struct run_process_parallel_opts *opts)
> {
> int i, code;
> int output_timeout = 100;
> int spawn_cap = 4;
> struct parallel_processes pp;
> + const char *tr2_category = opts->tr2_category;
> + const char *tr2_label = opts->tr2_label;
> + const int do_trace2 = tr2_category && tr2_label;
...but it's not actually very well enforced here. That is, it seems I
can set one or the other but not both, and nothing bad will happen,
except that I am wasting my time setting one. If you want to enforce
them both to be set, then why not use a BUG()? But otherwise the comment
could be reworded, I think.
> + const int n = opts->jobs;
>
> - pp_init(&pp, n, get_next_task, start_failure, task_finished, pp_cb);
> + if (do_trace2)
> + trace2_region_enter_printf(tr2_category, tr2_label, NULL,
> + "max:%d", ((n < 1) ? online_cpus()
> + : n));
> +
> + pp_init(&pp, opts);
> while (1) {
> for (i = 0;
> i < spawn_cap && !pp.shutdown &&
[/moved snippet]
Otherwise, although the number of lines of code is often higher, I find
the named initializers in the struct much easier to read at the
callsites, so I like this change.
Reviewed-by: Emily Shaffer <emilyshaffer@xxxxxxxxxx>
