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

From: Jeff King <peff@peff.net>
Date: Aug 21, 2020, 14:37 UTC
Message-ID: <20200821143727.GA3241139@coredump.intra.peff.net>
In-Reply-To: <0db8ad8cdb69afb9d6453bf60a808e8b82382a4e.1597998473.git.ps@pks.im>

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

Show 8 quoted lines

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

Show 10 quoted lines

>  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