Re: [PATCH] p5302: create the repo in each index-pack test

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 23, 2019, 01:09 UTC
Message-ID: <xmqqef5t7cil.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190422211952.GA4728@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 16 quoted lines

> Subject: [PATCH] p5302: create the repo in each index-pack test
>
> The p5302 script runs "index-pack --stdin" in each timing test. It does
> two things to try to get good timings:
>
>   1. we do the repo creation in a separate (non-timed) setup test, so
>      that our timing is purely the index-pack run
>
>   2. we use a separate repo for each test; this is important because the
>      presence of existing objects in the repo influences the result
>      (because we'll end up doing collision checks against them)
>
> But this forgets one thing: we generally run each timed test multiple
> times to reduce the impact of noise. Which means that repeats of each
> test after the first will be subject to the collision slowdown from
> point 2, and we'll generally just end up taking the first time anyway.

The above is very cleanly written to convince anybody that what the current test does contradicts with wish #2 above, and that the two wishes #1 and #2 are probably mutually incompatible.

But isn't the collision check a part of the real-life workload that Git users are made waiting for and care about the performance of? Or are we purely interested in the cost of resolving delta, computing the object name, and writing the result out to the disk in this test and the "overall experience" benchmark is left elsewhere?

The reason why I got confused is because the test_description of the script leaves "the actual effects we're interested in measuring" unsaid, I think. The log message of b8a2486f ("index-pack: support multithreaded delta resolving", 2012-05-06) that created this test does not help that much, either.

In any case, the above "this forgets one thing" makes it clear that we at this point in time declare what we are interested in very clearly, and I agree that the solution described in the paragraph below clearly matches the goal. Looks good.

> Instead, let's create the repo in the test (effectively undoing point
> 1). That does add a constant amount of extra work to each iteration, but
> it's quite small compared to the actual effects we're interested in
> measuring.

> Signed-off-by: Jeff King <peff@peff.net>
> ---
> The very first 0-thread one will run faster because it has less to "rm
> -rf", but I think we can ignore that.

OK.

> -	GIT_DIR=t1 git index-pack --threads=1 --stdin < $PACK
> +	rm -rf repo.git &&
> +	git init --bare repo.git &&
> +	GIT_DIR=repo.git git index-pack --threads=1 --stdin < $PACK

This is obviously inherited from the original, but do we get scolded by some versions of bash for this line, without quoting the source path of the redirection, i.e.

	... --stdin <"$PACK"