Re: [PATCH] refs: remove lookup cache for reference-transaction hook

[Jeff King <peff@peff.net>]

On Fri, Aug 21, 2020 at 10:29:18AM +0200, Patrick Steinhardt wrote:

> 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:

Ah, right, I forgot that update-ref would use one single transaction. So
what we were testing in our earlier discussion was not even useful. :)

>  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 &&
> +	printf "create refs/heads/branch-%d PRE\n" $(test_seq 1000) | git update-ref --stdin
>  '

OK, we need these new branches to have something to push into and delete
from the remote. They might impact the timings of the other tests,
though (since we now have 1000 entries in .git/refs/heads/, which might
affect filesystem performance). But it should do so uniformly, so I
don't think it invalidates their results.

However, I wondered...

> +test_perf "nonatomic push" '
> +	git push ./target-repo.git branch-{1..1000} &&
> +	git push --delete ./target-repo.git branch-{1..1000}
> +'

...if it might make the test more consistent (not to mention isolated
from the cost of other parts of the push) if we used update-ref here, as
well. You added the code necessary to control individual transactions,
so I thought that:

  printf 'start\ncreate refs/heads/%d PRE\ncommit\n' \
    $(test_seq 1000) >create-transaction

might work. But it doesn't, because after the first transaction is
closed, we refuse to accept any other commands. That makes sense for
"prepare", etc, but there's no reason we couldn't start a new one.

Is that worth supporting? It would allow a caller to use a single
update-ref to make a series of non-atomic updates, which is something
that can't currently be done. And we're so close.

Even if it is, though, that's definitely outside the scope of this
patch, and I think we should take it as-is with "push".

-Peff