Re: [PATCH 2/9] hook: allow parallel hook execution

On Thu, Jul 22 2021, Emily Shaffer wrote:

On Fri, Jul 16, 2021 at 10:36:10AM +0200, Ævar Arnfjörð Bjarmason wrote:

quoted

On Thu, Jul 15 2021, Emily Shaffer wrote:

quoted

In many cases, there's no reason not to allow hooks to execute in
parallel. run_processes_parallel() is well-suited - it's a task queue
that runs its housekeeping in series, which means users don't
need to worry about thread safety on their callback data. True
multithreaded execution with the async_* functions isn't necessary here.
Synchronous hook execution can be achieved by only allowing 1 job to run
at a time.

Teach run_hooks() to use that function for simple hooks which don't
require stdin or capture of stderr.

This doesn't mention...

quoted

 	int ret;
-	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
+	struct run_hooks_opt opt;
 
+	run_hooks_opt_init_sync(&opt);

...why we need to bring the s/macro/func/ init pattern, back, but looking ahead...

quoted

+int configured_hook_jobs(void)a
+{
+	int n = online_cpus();
+	git_config_get_int("hook.jobs", &n);
+
+	return n;
+}
+
 int hook_exists(const char *name)
 {
 	return !!find_hook(name);

@@ -117,6 +125,26 @@ struct list_head* hook_list(const char* hookname)
 	return hook_head;
 }
 
+void run_hooks_opt_init_sync(struct run_hooks_opt *o)
+{
+	strvec_init(&o->env);
+	strvec_init(&o->args);
+	o->path_to_stdin = NULL;
+	o->jobs = 1;
+	o->dir = NULL;
+	o->feed_pipe = NULL;
+	o->feed_pipe_ctx = NULL;
+	o->consume_sideband = NULL;
+	o->invoked_hook = NULL;
+	o->absolute_path = 0;
+}
+
+void run_hooks_opt_init_async(struct run_hooks_opt *o)
+{
+	run_hooks_opt_init_sync(o);
+	o->jobs = configured_hook_jobs();
+}

...okey, so it's because you brought back the "call jobs function" in
one of the init functions.

I had a comment in a previous round, I found
https://lore.kernel.org/git/87lf7bzbrk.fsf@evledraar.gmail.com/ (local), but I
think there was a later one where I commented on the "jobs" field
specifically.

Anyway, it seems much easier to me to just keep the simpler macro init
and then:

quoted

-	if (options->jobs != 1)
-		BUG("we do not handle %d or any other != 1 job number yet", options->jobs);
-
 	run_processes_parallel_tr2(options->jobs,
 				   pick_next_hook,
 				   notify_start_failure,

There's this one place where we use the "jobs" parameter, just do
something like this there:
        
        int configured_hook_jobs(void)
        {
                static int jobs;
                if (!jobs)
                    return jobs;
                if (git_config_get_int("hook.jobs", &jobs))
                    jobs = online_cpus();
                return jobs;
        }

I.e. you also needlessly call online_cpus() when we're about to override
it in the config. The git_config_get_int()'s return value indicates
whether you need to do that. Then just:

    int jobs = options->jobs ? options->jobs : configured_hook_jobs();
    run_processes_parallel_tr2(jobs, [...]);

Ahh, and then let RUN_HOOKS_OPT_INIT_ASYNC set jobs to 0 ("go look it
up"). Yeah, that makes sense.

Shout if somehow you meant to leave just one initializer macro;
otherwise, I'll do it this way - with RUN_HOOKS_OPT_INIT_ASYNC and
RUN_HOOKS_OPT_INIT_SYNC. I think it's valuable for hook callers to make
it very plain at the callsite whether they're parallelizable or not, and
I think

 struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
 opt.jobs = 0;

doesn't make that as obvious.

Yes agreed, sorry about the ambiguity, I meant we should have two init
macros, just like e.g. STRING_LIST_INIT_NODUP and STRING_LIST_INIT_DUP.