{"thread":{"id":"65122","subject":"[PATCH 0/1] Fix update hook perf regression in next","startedAt":"2026-03-02T19:17:43Z","lastAt":"2026-03-03T13:28:58Z","messageCount":7,"participants":["Adrian Ratiu","Junio C Hamano","Patrick Steinhardt","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"537589","messageId":"20260302191704.1814567-1-adrian.ratiu@collabora.com","threadId":"65122","inReplyTo":null,"subject":"[PATCH 0/1] Fix update hook perf regression in next","fromName":"Adrian Ratiu","fromEmail":"adrian.ratiu@collabora.com","sentAt":"2026-03-02T19:17:03Z","receivedAt":"2026-03-02T19:17:43Z","isPatch":true,"sender":{"key":"adrian.ratiu@collabora.com","avatar":"https://avatars.githubusercontent.com/u/12472556?v=4"},"body":"Hello everyone,\n\nThis fixes a performance regression I introduced in next by\nremoving the \"exit early\" check for hooks which output over\na sideband, during the conversion to the new hook API.\n\nThat was unintentional and these hooks should continue to exit\nearly if no hook is found, to avoid unnecessarily spinning\nun/down async threads which no-op and just add overhead.\n\nReported by Patrick at [1] and independently root caused and\nconfirmed by Peff who fixed it in a very similar manner [2].\n\nPushed to GitHub [3] and succesfully ran the CI [4].\n\n1: https://lore.kernel.org/git/aaWeSu-d1FMz_sW8@pks.im/T/#m4a1e62b3149825ef03f9b5b48f478933abc521cd\n2: https://lore.kernel.org/git/aaWeSu-d1FMz_sW8@pks.im/T/#me0d2655bb53f5ca8fc8f31e5726ecf4d2971fa11\n3: https://github.com/10ne1/git/tree/refs/heads/dev/aratiu/update-regression-fix\n4: https://github.com/10ne1/git/actions/runs/22590151068\n\nAdrian Ratiu (1):\n builtin/receive-pack: avoid spinning no-op sideband_async threads\n\n builtin/receive-pack.c | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\n-- \n2.52.0.732.gb351b5166d.dirty\n"},{"id":"537590","messageId":"20260302191704.1814567-2-adrian.ratiu@collabora.com","threadId":"65122","inReplyTo":"20260302191704.1814567-1-adrian.ratiu@collabora.com","subject":"[PATCH 1/1] builtin/receive-pack: avoid spinning no-op sideband async threads","fromName":"Adrian Ratiu","fromEmail":"adrian.ratiu@collabora.com","sentAt":"2026-03-02T19:17:04Z","receivedAt":"2026-03-02T19:17:43Z","isPatch":true,"sender":{"key":"adrian.ratiu@collabora.com","avatar":"https://avatars.githubusercontent.com/u/12472556?v=4"},"body":"Exit early if the hooks do not exist, to avoid spinning up/down\nsideband async threads which no-op.\n\nIt is important to call the hook_exists() API provided by hook.[ch]\nbecause it covers both config-defined hooks and the \"traditional\"\nhooks from the hookdir. find_hook() only covers the hookdir hooks.\n\nThe regression happened because the no-op async threads add some\nadditional overhead which can be measured with the receive-refs test\nof the benchmarks suite [1].\n\nReproduced using:\ncd benchmarks/receive-refs && \\\n./run --revisions /path/to/git \\\nfc148b146ad41be71a7852c4867f0773cbfe1ff9~,fc148b146ad41be71a7852c4867f0773cbfe1ff9 \\\n--parameter-list refformat reftable --parameter-list refcount 10000\n\n1: https://gitlab.com/gitlab-org/data-access/git/benchmarks\n\nFixes: fc148b146ad4 (\"receive-pack: convert update hooks to new API\")\nReported-by: Patrick Steinhardt \nHelped-by: Jeff King \nSigned-off-by: Adrian Ratiu \n---\n builtin/receive-pack.c | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 139a227e71..6376c191c7 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -934,6 +934,9 @@ static int run_receive_hook(struct command *commands,\n \tint saved_stderr = -1;\n \tint ret;\n \n+\tif (!hook_exists(the_repository, hook_name))\n+\t\treturn 0;\n+\n \t/* if there are no valid commands, don't invoke the hook at all. */\n \twhile (iter && skip_broken && (iter->error_string || iter->did_not_exist))\n \t\titer = iter->next;\n@@ -980,6 +983,9 @@ static int run_update_hook(struct command *cmd)\n \tint saved_stderr = -1;\n \tint code;\n \n+\tif (!hook_exists(the_repository, \"update\"))\n+\t\treturn 0;\n+\n \tstrvec_pushl(&opt.args,\n \t\t cmd->ref_name,\n \t\t oid_to_hex(&cmd->old_oid),\n@@ -1674,6 +1680,9 @@ static void run_update_post_hook(struct command *commands)\n \tint sideband_async_started = 0;\n \tint saved_stderr = -1;\n \n+\tif (!hook_exists(the_repository, \"post-update\"))\n+\t\treturn;\n+\n \tfor (cmd = commands; cmd; cmd = cmd->next) {\n \t\tif (cmd->error_string || cmd->did_not_exist)\n \t\t\tcontinue;\n-- \n2.52.0.732.gb351b5166d.dirty\n\n"},{"id":"537606","messageId":"xmqq4imxzz90.fsf@gitster.g","threadId":"65122","inReplyTo":"20260302191704.1814567-2-adrian.ratiu@collabora.com","subject":"Re: [PATCH 1/1] builtin/receive-pack: avoid spinning no-op sideband async threads","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-02T21:40:27Z","receivedAt":"2026-03-02T21:40:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adrian Ratiu writes:\n\n> @@ -980,6 +983,9 @@ static int run_update_hook(struct command *cmd)\n> \tint saved_stderr = -1;\n> \tint code;\n> \n> +\tif (!hook_exists(the_repository, \"update\"))\n> +\t\treturn 0;\n> +\n> \tstrvec_pushl(&opt.args,\n> \t\t cmd->ref_name,\n> \t\t oid_to_hex(&cmd->old_oid),\n\nShouldn't we consolidate the two instances of hardcoded string\n\"update\" in this function by introducing\n\n\tstatic const char hook_name[] = \"update\";\n\nin the function scope and using it?\n\n> @@ -1674,6 +1680,9 @@ static void run_update_post_hook(struct command *commands)\n> \tint sideband_async_started = 0;\n> \tint saved_stderr = -1;\n> \n> +\tif (!hook_exists(the_repository, \"post-update\"))\n> +\t\treturn;\n> +\n> \tfor (cmd = commands; cmd; cmd = cmd->next) {\n> \t\tif (cmd->error_string || cmd->did_not_exist)\n> \t\t\tcontinue;\n\nDitto for \"post-update\".\n\nWill queue with the following change squashed in.\n\n builtin/receive-pack.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git c/builtin/receive-pack.c w/builtin/receive-pack.c\nindex 62c576c247..bf5d7e6dd0 100644\n--- c/builtin/receive-pack.c\n+++ w/builtin/receive-pack.c\n@@ -977,13 +977,14 @@ static int run_receive_hook(struct command *commands,\n \n static int run_update_hook(struct command *cmd)\n {\n+\tstatic const char hook_name[] = \"update\";\n \tstruct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;\n \tstruct async sideband_async;\n \tint sideband_async_started = 0;\n \tint saved_stderr = -1;\n \tint code;\n \n-\tif (!hook_exists(the_repository, \"update\"))\n+\tif (!hook_exists(the_repository, hook_name))\n \t\treturn 0;\n \n \tstrvec_pushl(&opt.args,\n@@ -994,7 +995,7 @@ static int run_update_hook(struct command *cmd)\n \n \tprepare_sideband_async(&sideband_async, &saved_stderr, &sideband_async_started);\n \n-\tcode = run_hooks_opt(the_repository, \"update\", &opt);\n+\tcode = run_hooks_opt(the_repository, hook_name, &opt);\n \n \tfinish_sideband_async(&sideband_async, saved_stderr, sideband_async_started);\n \n@@ -1674,13 +1675,14 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \n static void run_update_post_hook(struct command *commands)\n {\n+\tstatic const char hook_name[] = \"post-update\";\n \tstruct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;\n \tstruct async sideband_async;\n \tstruct command *cmd;\n \tint sideband_async_started = 0;\n \tint saved_stderr = -1;\n \n-\tif (!hook_exists(the_repository, \"post-update\"))\n+\tif (!hook_exists(the_repository, hook_name))\n \t\treturn;\n \n \tfor (cmd = commands; cmd; cmd = cmd->next) {\n@@ -1693,7 +1695,7 @@ static void run_update_post_hook(struct command *commands)\n \n \tprepare_sideband_async(&sideband_async, &saved_stderr, &sideband_async_started);\n \n-\trun_hooks_opt(the_repository, \"post-update\", &opt);\n+\trun_hooks_opt(the_repository, hook_name, &opt);\n \n \tfinish_sideband_async(&sideband_async, saved_stderr, sideband_async_started);\n }\n"},{"id":"537645","messageId":"aaZ7eXtUSWSS_igX@pks.im","threadId":"65122","inReplyTo":"20260302191704.1814567-2-adrian.ratiu@collabora.com","subject":"Re: [PATCH 1/1] builtin/receive-pack: avoid spinning no-op sideband async threads","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-03T06:11:05Z","receivedAt":"2026-03-03T06:11:21Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Mar 02, 2026 at 09:17:04PM +0200, Adrian Ratiu wrote:\n> Exit early if the hooks do not exist, to avoid spinning up/down\n> sideband async threads which no-op.\n> \n> It is important to call the hook_exists() API provided by hook.[ch]\n> because it covers both config-defined hooks and the \"traditional\"\n> hooks from the hookdir. find_hook() only covers the hookdir hooks.\n\nJust out of curiosity: will `find_hook()` eventually be removed? I saw\nthat we still use it for the \"proc-receive\" hook in git-receive-pack(1)\nfor example, which feels a bit fishy to me.\n\nIn any case, if this is an oversight then this can be handled in a\nsubsequent patch series, if you ask me.\n\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 139a227e71..6376c191c7 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -934,6 +934,9 @@ static int run_receive_hook(struct command *commands,\n> \tint saved_stderr = -1;\n> \tint ret;\n> \n> +\tif (!hook_exists(the_repository, hook_name))\n> +\t\treturn 0;\n> +\n> \t/* if there are no valid commands, don't invoke the hook at all. */\n> \twhile (iter && skip_broken && (iter->error_string || iter->did_not_exist))\n> \t\titer = iter->next;\n\nThat fix is delightfully simple -- I was fearing for a deeper issue. I\ncan confirm that this restores original performance:\n\n Benchmark 1: receive: many refs (refformat = reftable, refcount = 10000, revision = fc148b146ad41be71a7852c4867f0773cbfe1ff9~)\n Time (mean ± σ): 177.4 ms ± 3.0 ms [User: 92.0 ms, System: 84.2 ms]\n Range (min … max): 172.1 ms … 182.6 ms 15 runs\n\n Benchmark 2: receive: many refs (refformat = reftable, refcount = 10000, revision = fc148b146ad41be71a7852c4867f0773cbfe1ff9)\n Time (mean ± σ): 485.0 ms ± 7.1 ms [User: 180.0 ms, System: 375.0 ms]\n Range (min … max): 466.9 ms … 491.0 ms 10 runs\n\n Benchmark 3: receive: many refs (refformat = reftable, refcount = 10000, revision = 005f3fbe07a20dd5f7dea57f6f46cd797387e56a)\n Time (mean ± σ): 178.1 ms ± 2.4 ms [User: 91.8 ms, System: 85.1 ms]\n Range (min … max): 172.2 ms … 181.3 ms 15 runs\n\n Summary\n receive: many refs (refformat = reftable, refcount = 10000, revision = fc148b146ad41be71a7852c4867f0773cbfe1ff9~) ran\n 1.00 ± 0.02 times faster than receive: many refs (refformat = reftable, refcount = 10000, revision = 005f3fbe07a20dd5f7dea57f6f46cd797387e56a)\n 2.73 ± 0.06 times faster than receive: many refs (refformat = reftable, refcount = 10000, revision = fc148b146ad41be71a7852c4867f0773cbfe1ff9)\n\nAnd Bencher has already picked up those changes, too, and graphs have\ndropped back to previous levels. Awesome.\n\nThanks a lot for the quick turnaround!\n\nPatrick\n"},{"id":"537658","messageId":"875x7dozdd.fsf@gentoo.mail-host-address-is-not-set","threadId":"65122","inReplyTo":"aaZ7eXtUSWSS_igX@pks.im","subject":"Re: [PATCH 1/1] builtin/receive-pack: avoid spinning no-op sideband async threads","fromName":"Adrian Ratiu","fromEmail":"adrian.ratiu@collabora.com","sentAt":"2026-03-03T12:45:34Z","receivedAt":"2026-03-03T12:45:53Z","isPatch":true,"sender":{"key":"adrian.ratiu@collabora.com","avatar":"https://avatars.githubusercontent.com/u/12472556?v=4"},"body":"On Tue, 03 Mar 2026, Patrick Steinhardt wrote:\n> On Mon, Mar 02, 2026 at 09:17:04PM +0200, Adrian Ratiu wrote:\n>> Exit early if the hooks do not exist, to avoid spinning up/down\n>> sideband async threads which no-op.\n>> \n>> It is important to call the hook_exists() API provided by hook.[ch]\n>> because it covers both config-defined hooks and the \"traditional\"\n>> hooks from the hookdir. find_hook() only covers the hookdir hooks.\n>\n> Just out of curiosity: will `find_hook()` eventually be removed? I saw\n> that we still use it for the \"proc-receive\" hook in git-receive-pack(1)\n> for example, which feels a bit fishy to me.\n\nThe answer is a big YES and I actually thought about this while fixing\nthe regression yesterday (unrelated to proc-receive).\n\nAll hooks should use the new hook.[ch] APIs which provide clearer\nfunctions like hook_exists() and all direct find_hook() / run-command \ninvocations should be removed.\n\n> In any case, if this is an oversight then this can be handled in a\n> subsequent patch series, if you ask me.\n\nYes, this can be done incrementally in a subsequent patch so that\nproc-receive can also benefit from hook.[ch] features like being able to\nspecify it via configs.\n\nIt was out of scope for the initial patches, so I didn't pay too much\nattention to it, but it should be rather simple to convert. I do plan to\nconvert it as well.\n\nThe end goal is to make find_hook() static (not exported outside hook.c)\nonce all its external uses have been converted.\n"},{"id":"537659","messageId":"87342hoz9l.fsf@collabora.com","threadId":"65122","inReplyTo":"xmqq4imxzz90.fsf@gitster.g","subject":"Re: [PATCH 1/1] builtin/receive-pack: avoid spinning no-op sideband async threads","fromName":"Adrian Ratiu","fromEmail":"adrian.ratiu@collabora.com","sentAt":"2026-03-03T12:47:50Z","receivedAt":"2026-03-03T12:48:09Z","isPatch":true,"sender":{"key":"adrian.ratiu@collabora.com","avatar":"https://avatars.githubusercontent.com/u/12472556?v=4"},"body":"On Mon, 02 Mar 2026, Junio C Hamano wrote:\n>> @@ -1674,6 +1680,9 @@ static void run_update_post_hook(struct command *commands)\n>> \tint sideband_async_started = 0;\n>> \tint saved_stderr = -1;\n>> \n>> +\tif (!hook_exists(the_repository, \"post-update\"))\n>> +\t\treturn;\n>> +\n>> \tfor (cmd = commands; cmd; cmd = cmd->next) {\n>> \t\tif (cmd->error_string || cmd->did_not_exist)\n>> \t\t\tcontinue;\n>\n> Ditto for \"post-update\".\n>\n> Will queue with the following change squashed in.\n\nThank you Junio, much appreciated!\n"},{"id":"537662","messageId":"20260303132855.GA748945@coredump.intra.peff.net","threadId":"65122","inReplyTo":"20260302191704.1814567-2-adrian.ratiu@collabora.com","subject":"Re: [PATCH 1/1] builtin/receive-pack: avoid spinning no-op sideband async threads","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-03T13:28:55Z","receivedAt":"2026-03-03T13:28:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 02, 2026 at 09:17:04PM +0200, Adrian Ratiu wrote:\n\n> It is important to call the hook_exists() API provided by hook.[ch]\n> because it covers both config-defined hooks and the \"traditional\"\n> hooks from the hookdir. find_hook() only covers the hookdir hooks.\n\nAh, OK. Traditionally hook_exists() was just a thin wrapper over\nfind_hook(), but it looks like that changed in your series. But either\nway, it much more clearly expresses the intent to use hook_exists().\n\nSo this obviously looks good, but just some random thoughts below.\n\n> @@ -934,6 +934,9 @@ static int run_receive_hook(struct command *commands,\n> \tint saved_stderr = -1;\n> \tint ret;\n> \n> +\tif (!hook_exists(the_repository, hook_name))\n> +\t\treturn 0;\n\nIt is a little inelegant that we have to look up the hook data\nseparately ourselves here, and then it will be done again in\nrun_hooks_opt(). But I don't think there is an easy way to reorganize\nit, short of something like:\n\n struct hook myhook = HOOK_INIT;\n\n load_hooks(&myhook, hook_name);\n if (myhook.nr)\n\treturn 0; /* no hooks of this type */\n\n ...other prep work...\n\n run_hooks_opt(repo, &myhook, &opt);\n\nI doubt that is worth it, as the lookup process should not be too\nexpensive. It looks like we cache the config parts of the lookup\nalready. We call access() to find the traditional hooks on each lookup,\nbut that is also true of the code before your series.\n\nIt would not matter at all for pre-receive, for example, but for\nsomething like update, I guess we are doing a bunch of pointless\naccess() calls that could be cached. But again, not new in your series,\nand nobody has really noticed. So we can either treat it as an\noptimization for later, or just leave it be forever.\n\n-Peff\n"}]}