[PATCH 0/1] Fix update hook perf regression in next

[PATCH 0/1] Fix update hook perf regression in next

[Adrian Ratiu]

Hello everyone,

This fixes a performance regression I introduced in next by
removing the "exit early" check for hooks which output over
a sideband, during the conversion to the new hook API.

That was unintentional and these hooks should continue to exit
early if no hook is found, to avoid unnecessarily spinning
un/down async threads which no-op and just add overhead.

Reported by Patrick at [1] and independently root caused and
confirmed by Peff who fixed it in a very similar manner [2].

Pushed to GitHub [3] and succesfully ran the CI [4].

1: https://lore.kernel.org/git/aaWeSu-d1FMz_sW8@pks.im/T/#m4a1e62b3149825ef03f9b5b48f478933abc521cd
2: https://lore.kernel.org/git/aaWeSu-d1FMz_sW8@pks.im/T/#me0d2655bb53f5ca8fc8f31e5726ecf4d2971fa11
3: https://github.com/10ne1/git/tree/refs/heads/dev/aratiu/update-regression-fix
4: https://github.com/10ne1/git/actions/runs/22590151068

Adrian Ratiu (1):
  builtin/receive-pack: avoid spinning no-op sideband_async threads

 builtin/receive-pack.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

-- 
2.52.0.732.gb351b5166d.dirty

[PATCH 1/1] builtin/receive-pack: avoid spinning no-op sideband async threads

[Adrian Ratiu]

Exit early if the hooks do not exist, to avoid spinning up/down
sideband async threads which no-op.

It is important to call the hook_exists() API provided by hook.[ch]
because it covers both config-defined hooks and the "traditional"
hooks from the hookdir. find_hook() only covers the hookdir hooks.

The regression happened because the no-op async threads add some
additional overhead which can be measured with the receive-refs test
of the benchmarks suite [1].

Reproduced using:
cd benchmarks/receive-refs && \
./run --revisions /path/to/git \
fc148b146ad41be71a7852c4867f0773cbfe1ff9~,fc148b146ad41be71a7852c4867f0773cbfe1ff9 \
--parameter-list refformat reftable --parameter-list refcount 10000

1: https://gitlab.com/gitlab-org/data-access/git/benchmarks

Fixes: fc148b146ad4 ("receive-pack: convert update hooks to new API")
Reported-by: Patrick Steinhardt <ps@pks.im>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
---
 builtin/receive-pack.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 139a227e71..6376c191c7 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -934,6 +934,9 @@ static int run_receive_hook(struct command *commands,
 	int saved_stderr = -1;
 	int ret;
 
+	if (!hook_exists(the_repository, hook_name))
+		return 0;
+
 	/* if there are no valid commands, don't invoke the hook at all. */
 	while (iter && skip_broken && (iter->error_string || iter->did_not_exist))
 		iter = iter->next;
@@ -980,6 +983,9 @@ static int run_update_hook(struct command *cmd)
 	int saved_stderr = -1;
 	int code;
 
+	if (!hook_exists(the_repository, "update"))
+		return 0;
+
 	strvec_pushl(&opt.args,
 		     cmd->ref_name,
 		     oid_to_hex(&cmd->old_oid),
@@ -1674,6 +1680,9 @@ static void run_update_post_hook(struct command *commands)
 	int sideband_async_started = 0;
 	int saved_stderr = -1;
 
+	if (!hook_exists(the_repository, "post-update"))
+		return;
+
 	for (cmd = commands; cmd; cmd = cmd->next) {
 		if (cmd->error_string || cmd->did_not_exist)
 			continue;
-- 
2.52.0.732.gb351b5166d.dirty

Re: [PATCH 1/1] builtin/receive-pack: avoid spinning no-op sideband async threads

[Junio C Hamano]

Adrian Ratiu <adrian.ratiu@collabora.com> writes:

> @@ -980,6 +983,9 @@ static int run_update_hook(struct command *cmd)
>  	int saved_stderr = -1;
>  	int code;
>  
> +	if (!hook_exists(the_repository, "update"))
> +		return 0;
> +
>  	strvec_pushl(&opt.args,
>  		     cmd->ref_name,
>  		     oid_to_hex(&cmd->old_oid),

Shouldn't we consolidate the two instances of hardcoded string
"update" in this function by introducing

	static const char hook_name[] = "update";

in the function scope and using it?

> @@ -1674,6 +1680,9 @@ static void run_update_post_hook(struct command *commands)
>  	int sideband_async_started = 0;
>  	int saved_stderr = -1;
>  
> +	if (!hook_exists(the_repository, "post-update"))
> +		return;
> +
>  	for (cmd = commands; cmd; cmd = cmd->next) {
>  		if (cmd->error_string || cmd->did_not_exist)
>  			continue;

Ditto for "post-update".

Will queue with the following change squashed in.

 builtin/receive-pack.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git c/builtin/receive-pack.c w/builtin/receive-pack.c
index 62c576c247..bf5d7e6dd0 100644
--- c/builtin/receive-pack.c
+++ w/builtin/receive-pack.c
@@ -977,13 +977,14 @@ static int run_receive_hook(struct command *commands,
 
 static int run_update_hook(struct command *cmd)
 {
+	static const char hook_name[] = "update";
 	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
 	struct async sideband_async;
 	int sideband_async_started = 0;
 	int saved_stderr = -1;
 	int code;
 
-	if (!hook_exists(the_repository, "update"))
+	if (!hook_exists(the_repository, hook_name))
 		return 0;
 
 	strvec_pushl(&opt.args,
@@ -994,7 +995,7 @@ static int run_update_hook(struct command *cmd)
 
 	prepare_sideband_async(&sideband_async, &saved_stderr, &sideband_async_started);
 
-	code = run_hooks_opt(the_repository, "update", &opt);
+	code = run_hooks_opt(the_repository, hook_name, &opt);
 
 	finish_sideband_async(&sideband_async, saved_stderr, sideband_async_started);
 
@@ -1674,13 +1675,14 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 
 static void run_update_post_hook(struct command *commands)
 {
+	static const char hook_name[] = "post-update";
 	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
 	struct async sideband_async;
 	struct command *cmd;
 	int sideband_async_started = 0;
 	int saved_stderr = -1;
 
-	if (!hook_exists(the_repository, "post-update"))
+	if (!hook_exists(the_repository, hook_name))
 		return;
 
 	for (cmd = commands; cmd; cmd = cmd->next) {
@@ -1693,7 +1695,7 @@ static void run_update_post_hook(struct command *commands)
 
 	prepare_sideband_async(&sideband_async, &saved_stderr, &sideband_async_started);
 
-	run_hooks_opt(the_repository, "post-update", &opt);
+	run_hooks_opt(the_repository, hook_name, &opt);
 
 	finish_sideband_async(&sideband_async, saved_stderr, sideband_async_started);
 }

Re: [PATCH 1/1] builtin/receive-pack: avoid spinning no-op sideband async threads

[Patrick Steinhardt]

On Mon, Mar 02, 2026 at 09:17:04PM +0200, Adrian Ratiu wrote:
> Exit early if the hooks do not exist, to avoid spinning up/down
> sideband async threads which no-op.
> 
> It is important to call the hook_exists() API provided by hook.[ch]
> because it covers both config-defined hooks and the "traditional"
> hooks from the hookdir. find_hook() only covers the hookdir hooks.

Just out of curiosity: will `find_hook()` eventually be removed? I saw
that we still use it for the "proc-receive" hook in git-receive-pack(1)
for example, which feels a bit fishy to me.

In any case, if this is an oversight then this can be handled in a
subsequent patch series, if you ask me.

> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 139a227e71..6376c191c7 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -934,6 +934,9 @@ static int run_receive_hook(struct command *commands,
>  	int saved_stderr = -1;
>  	int ret;
>  
> +	if (!hook_exists(the_repository, hook_name))
> +		return 0;
> +
>  	/* if there are no valid commands, don't invoke the hook at all. */
>  	while (iter && skip_broken && (iter->error_string || iter->did_not_exist))
>  		iter = iter->next;

That fix is delightfully simple -- I was fearing for a deeper issue. I
can confirm that this restores original performance:

  Benchmark 1: receive: many refs (refformat = reftable, refcount = 10000, revision = fc148b146ad41be71a7852c4867f0773cbfe1ff9~)
    Time (mean ± σ):     177.4 ms ±   3.0 ms    [User: 92.0 ms, System: 84.2 ms]
    Range (min … max):   172.1 ms … 182.6 ms    15 runs

  Benchmark 2: receive: many refs (refformat = reftable, refcount = 10000, revision = fc148b146ad41be71a7852c4867f0773cbfe1ff9)
    Time (mean ± σ):     485.0 ms ±   7.1 ms    [User: 180.0 ms, System: 375.0 ms]
    Range (min … max):   466.9 ms … 491.0 ms    10 runs

  Benchmark 3: receive: many refs (refformat = reftable, refcount = 10000, revision = 005f3fbe07a20dd5f7dea57f6f46cd797387e56a)
    Time (mean ± σ):     178.1 ms ±   2.4 ms    [User: 91.8 ms, System: 85.1 ms]
    Range (min … max):   172.2 ms … 181.3 ms    15 runs

  Summary
    receive: many refs (refformat = reftable, refcount = 10000, revision = fc148b146ad41be71a7852c4867f0773cbfe1ff9~) ran
      1.00 ± 0.02 times faster than receive: many refs (refformat = reftable, refcount = 10000, revision = 005f3fbe07a20dd5f7dea57f6f46cd797387e56a)
      2.73 ± 0.06 times faster than receive: many refs (refformat = reftable, refcount = 10000, revision = fc148b146ad41be71a7852c4867f0773cbfe1ff9)

And Bencher has already picked up those changes, too, and graphs have
dropped back to previous levels. Awesome.

Thanks a lot for the quick turnaround!

Patrick

Re: [PATCH 1/1] builtin/receive-pack: avoid spinning no-op sideband async threads

[Adrian Ratiu]

On Tue, 03 Mar 2026, Patrick Steinhardt <ps@pks.im> wrote:
> On Mon, Mar 02, 2026 at 09:17:04PM +0200, Adrian Ratiu wrote:
>> Exit early if the hooks do not exist, to avoid spinning up/down
>> sideband async threads which no-op.
>> 
>> It is important to call the hook_exists() API provided by hook.[ch]
>> because it covers both config-defined hooks and the "traditional"
>> hooks from the hookdir. find_hook() only covers the hookdir hooks.
>
> Just out of curiosity: will `find_hook()` eventually be removed? I saw
> that we still use it for the "proc-receive" hook in git-receive-pack(1)
> for example, which feels a bit fishy to me.

The answer is a big YES and I actually thought about this while fixing
the regression yesterday (unrelated to proc-receive).

All hooks should use the new hook.[ch] APIs which provide clearer
functions like hook_exists() and all direct find_hook() / run-command 
invocations should be removed.

> In any case, if this is an oversight then this can be handled in a
> subsequent patch series, if you ask me.

Yes, this can be done incrementally in a subsequent patch so that
proc-receive can also benefit from hook.[ch] features like being able to
specify it via configs.

It was out of scope for the initial patches, so I didn't pay too much
attention to it, but it should be rather simple to convert. I do plan to
convert it as well.

The end goal is to make find_hook() static (not exported outside hook.c)
once all its external uses have been converted.

Re: [PATCH 1/1] builtin/receive-pack: avoid spinning no-op sideband async threads

[Adrian Ratiu]

On Mon, 02 Mar 2026, Junio C Hamano <gitster@pobox.com> wrote:
>> @@ -1674,6 +1680,9 @@ static void run_update_post_hook(struct command *commands)
>>  	int sideband_async_started = 0;
>>  	int saved_stderr = -1;
>>  
>> +	if (!hook_exists(the_repository, "post-update"))
>> +		return;
>> +
>>  	for (cmd = commands; cmd; cmd = cmd->next) {
>>  		if (cmd->error_string || cmd->did_not_exist)
>>  			continue;
>
> Ditto for "post-update".
>
> Will queue with the following change squashed in.

Thank you Junio, much appreciated!

Re: [PATCH 1/1] builtin/receive-pack: avoid spinning no-op sideband async threads

[Jeff King]

On Mon, Mar 02, 2026 at 09:17:04PM +0200, Adrian Ratiu wrote:

> It is important to call the hook_exists() API provided by hook.[ch]
> because it covers both config-defined hooks and the "traditional"
> hooks from the hookdir. find_hook() only covers the hookdir hooks.

Ah, OK. Traditionally hook_exists() was just a thin wrapper over
find_hook(), but it looks like that changed in your series. But either
way, it much more clearly expresses the intent to use hook_exists().

So this obviously looks good, but just some random thoughts below.

> @@ -934,6 +934,9 @@ static int run_receive_hook(struct command *commands,
>  	int saved_stderr = -1;
>  	int ret;
>  
> +	if (!hook_exists(the_repository, hook_name))
> +		return 0;

It is a little inelegant that we have to look up the hook data
separately ourselves here, and then it will be done again in
run_hooks_opt(). But I don't think there is an easy way to reorganize
it, short of something like:

  struct hook myhook = HOOK_INIT;

  load_hooks(&myhook, hook_name);
  if (myhook.nr)
	return 0; /* no hooks of this type */

  ...other prep work...

  run_hooks_opt(repo, &myhook, &opt);

I doubt that is worth it, as the lookup process should not be too
expensive. It looks like we cache the config parts of the lookup
already. We call access() to find the traditional hooks on each lookup,
but that is also true of the code before your series.

It would not matter at all for pre-receive, for example, but for
something like update, I guess we are doing a bunch of pointless
access() calls that could be cached. But again, not new in your series,
and nobody has really noticed. So we can either treat it as an
optimization for later, or just leave it be forever.

-Peff