@matheusbsbs / matheus-bagliano / commits / 0a0fbbe3ff

Fork 0

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>

Patrick Steinhardt committed Aug 25, 2020 at 12:35 UTC 0a0fbbe3ff2090fb3eb0003744c045dc38670fa2

2 files changed +12 -12

refs.c

+2 -9

@@ -1988,24 +1988,17 @@ int ref_update_reject_duplicates(struct string_list *refnames,

1988 return 0;

1989 }

1990

1991 -static const char hook_not_found;

1992 -static const char *hook;

1993 -

1991 static int run_transaction_hook(struct ref_transaction *transaction,

1992 const char *state)

1993 {

1994 struct child_process proc = CHILD_PROCESS_INIT;

1995 struct strbuf buf = STRBUF_INIT;

1996 + const char *hook;

1997 int ret = 0, i;

1998

2001 - if (hook == &hook_not_found)

2002 - return ret;

1999 + hook = find_hook("reference-transaction");

2000 if (!hook)

2004 - hook = xstrdup_or_null(find_hook("reference-transaction"));

2005 - if (!hook) {

2006 - hook = &hook_not_found;

2001 return ret;

2008 - }

2002

2003 argv_array_pushl(&proc.args, hook, state, NULL);

2004 proc.in = -1;

t/perf/p1400-update-ref.sh

+10 -3

@@ -7,11 +7,13 @@ test_description="Tests performance of update-ref"

7 test_perf_fresh_repo

9 test_expect_success "setup" '

10 + git init --bare target-repo.git &&

11 test_commit PRE &&

12 test_commit POST &&

13 printf "create refs/heads/%d PRE\n" $(test_seq 1000) >create &&

14 printf "update refs/heads/%d POST PRE\n" $(test_seq 1000) >update &&

14 - printf "delete refs/heads/%d POST\n" $(test_seq 1000) >delete

15 + printf "delete refs/heads/%d POST\n" $(test_seq 1000) >delete &&

16 + git update-ref --stdin <create

17 '

19 test_perf "update-ref" '

@@ -24,9 +26,14 @@ test_perf "update-ref" '

26 '

28 test_perf "update-ref --stdin" '

27 - git update-ref --stdin <create &&

29 git update-ref --stdin <update &&

29 - git update-ref --stdin <delete

30 + git update-ref --stdin <delete &&

31 + git update-ref --stdin <create

32 +'

33 +

34 +test_perf "nonatomic push" '

35 + git push ./target-repo.git $(test_seq 1000) &&

36 + git push --delete ./target-repo.git $(test_seq 1000)

37 '

39 test_done