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

> On Fri, Jul 16, 2021 at 10:36:10AM +0200, Ævar Arnfjörð Bjarmason wrote:
>> 
>> 
>> On Thu, Jul 15 2021, Emily Shaffer wrote:
>> 
>> > 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...
>> 
>> >  	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...
>> 
>> > +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/, 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:
>> 
>> > -	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.