refs: remove lookup cache for reference-transaction hook

When adding the reference-transaction hook, there were concerns about
the performance impact it may have on setups which do not make use of
the new hook at all. After all, it gets executed every time a reftx is
prepared, committed or aborted, which linearly scales with the number of
reference-transactions created per session. And as there are code paths
like `git push` which create a new transaction for each reference to be
updated, this may translate to calling `find_hook()` quite a lot.

To address this concern, a cache was added with the intention to not
repeatedly do negative hook lookups. Turns out this cache caused a
regression, which was fixed via e5256c82e5 (refs: fix interleaving hook
calls with reference-transaction hook, 2020-08-07). In the process of
discussing the fix, we realized that the cache doesn't really help even
in the negative-lookup case. While performance tests added to benchmark
this did show a slight improvement in the 1% range, this really doesn't
warrent having a cache. Furthermore, it's quite flaky, too. E.g. running
it twice in succession produces the following results:

Test                         master            pks-reftx-hook-remove-cache
--------------------------------------------------------------------------
1400.2: update-ref           2.79(2.16+0.74)   2.73(2.12+0.71) -2.2%
1400.3: update-ref --stdin   0.22(0.08+0.14)   0.21(0.08+0.12) -4.5%

Test                         master            pks-reftx-hook-remove-cache
--------------------------------------------------------------------------
1400.2: update-ref           2.70(2.09+0.72)   2.74(2.13+0.71) +1.5%
1400.3: update-ref --stdin   0.21(0.10+0.10)   0.21(0.08+0.13) +0.0%

One case notably absent from those benchmarks is a single executable
searching for the hook hundreds of times, which is exactly the case for
which the negative cache was added. p1400.2 will spawn a new update-ref
for each transaction and p1400.3 only has a single reference-transaction
for all reference updates. So this commit adds a third benchmark, which
performs an non-atomic push of a thousand references. This will create a
new reference transaction per reference. But even for this case, the
negative cache doesn't consistently improve performance:

Test                         master            pks-reftx-hook-remove-cache
--------------------------------------------------------------------------
1400.4: nonatomic push       6.63(6.50+0.13)   6.81(6.67+0.14) +2.7%
1400.4: nonatomic push       6.35(6.21+0.14)   6.39(6.23+0.16) +0.6%
1400.4: nonatomic push       6.43(6.31+0.13)   6.42(6.28+0.15) -0.2%

So let's just remove the cache altogether to simplify the code.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/refs.c b/refs.c
index 836d49b9588dfc5e7eafe908654f02b4aaf93607..d4ec7d30c126d408ca088f6bdd4e67a227e99dbd 100644 (file)
--- a/refs.c
+++ b/refs.c
@@ -1988,24 +1988,17 @@ int ref_update_reject_duplicates(struct string_list *refnames,
        return 0;
 }
 
-static const char hook_not_found;
-static const char *hook;
-
 static int run_transaction_hook(struct ref_transaction *transaction,
                                const char *state)
 {
        struct child_process proc = CHILD_PROCESS_INIT;
        struct strbuf buf = STRBUF_INIT;
+       const char *hook;
        int ret = 0, i;
 
-       if (hook == &hook_not_found)
-               return ret;
+       hook = find_hook("reference-transaction");
        if (!hook)
-               hook = xstrdup_or_null(find_hook("reference-transaction"));
-       if (!hook) {
-               hook = &hook_not_found;
                return ret;
-       }
 
        argv_array_pushl(&proc.args, hook, state, NULL);
        proc.in = -1;

diff --git a/t/perf/p1400-update-ref.sh b/t/perf/p1400-update-ref.sh
index d275a81248b04adaab5e37ea20504a82d572e649..ce5ac3ed85e10efc2b4dd784e5d8949d8f269a75 100755 (executable)
--- a/t/perf/p1400-update-ref.sh
+++ b/t/perf/p1400-update-ref.sh
@@ -7,11 +7,13 @@ test_description="Tests performance of update-ref"
 test_perf_fresh_repo
 
 test_expect_success "setup" '
+       git init --bare target-repo.git &&
        test_commit PRE &&
        test_commit POST &&
        printf "create refs/heads/%d PRE\n" $(test_seq 1000) >create &&
        printf "update refs/heads/%d POST PRE\n" $(test_seq 1000) >update &&
-       printf "delete refs/heads/%d POST\n" $(test_seq 1000) >delete
+       printf "delete refs/heads/%d POST\n" $(test_seq 1000) >delete &&
+       git update-ref --stdin <create
 '
 
 test_perf "update-ref" '
@@ -24,9 +26,14 @@ test_perf "update-ref" '
 '
 
 test_perf "update-ref --stdin" '
-       git update-ref --stdin <create &&
        git update-ref --stdin <update &&
-       git update-ref --stdin <delete
+       git update-ref --stdin <delete &&
+       git update-ref --stdin <create
+'
+
+test_perf "nonatomic push" '
+       git push ./target-repo.git $(test_seq 1000) &&
+       git push --delete ./target-repo.git $(test_seq 1000)
 '
 
 test_done