threads / patch / 65122

patchFix update hook perf regression in next

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

## tl;dr

7 messages between Mar 2, 2026 and Mar 3, 2026. Diffs are folded; open one to read it.

replies: 6people: 4as markdown or json

Adrian Ratiu· Mar 2, 2026, 19:17 UTC · lore
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
Adrian Ratiu· Mar 2, 2026, 19:17 UTC · re: Adrian Ratiu · lore

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

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(+)
Show changes to builtin/receive-pack.c +9 −0
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
Junio C Hamano· Mar 2, 2026, 21:40 UTC · re: Adrian Ratiu · lore

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

Adrian Ratiu <adrian.ratiu@collabora.com> writes:
Show 10 quoted lines
> @@ -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?
Show 10 quoted lines
> @@ -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(-)
Show changes to diff +6 −4
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);
 }
Adrian Ratiu· Mar 3, 2026, 12:47 UTC · re: Junio C Hamano · lore

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

On Mon, 02 Mar 2026, Junio C Hamano <gitster@pobox.com> wrote:
Show 14 quoted lines
>> @@ -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!
Patrick Steinhardt· Mar 3, 2026, 06:11 UTC · re: Adrian Ratiu · lore

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

On Mon, Mar 02, 2026 at 09:17:04PM +0200, Adrian Ratiu wrote:
Show 6 quoted lines
> 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.

Show 14 quoted lines
> 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
Adrian Ratiu· Mar 3, 2026, 12:45 UTC · re: Patrick Steinhardt · lore

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

On Tue, 03 Mar 2026, Patrick Steinhardt <ps@pks.im> wrote:
Show 11 quoted lines
> 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.

Jeff King· Mar 3, 2026, 13:28 UTC · re: Adrian Ratiu · lore

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

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.
Show 6 quoted lines
> @@ -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

← back to recent threads