Re: [PATCH] t/perf/perf-lib.sh: remove test_times.* at the end test_perf_()

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> What I'd propose instead is that we ought to have:
>
>   test_perf 'checkout'
>             --prepare '
> 	        git reset --hard the_original_state
> 	    ' '
> 	        git checkout
> 	    '
>
> Having two multi-line snippets is a bit ugly (check out that awful
> middle line), but I think this could be added without breaking existing
> tests (they just wouldn't have a --prepare option).
>
> If that syntax is too horrendous, we could have:
>
>   # this saves the snippet in a variable internally, and runs
>   # it before each trial of the next test_perf(), after which
>   # it is discarded
>   test_perf_prepare '
>           git reset --hard the_original_state
>   '
>
>   test_perf 'checkout' '
>           git checkout
>   '
>
> I think that would be pretty easy to implement, and would solve the most
> common form of this problem. And there's plenty of prior art; just about
> every decent benchmarking system has a "do this before each trial"
> mechanism. Our t/perf suite (as you probably noticed) is rather more
> ad-hoc and less mature.

Nice.

> There are cases it doesn't help, though. For instance, in one of the
> scripts we measure the time to run "git repack -adb" to generate
> bitmaps. But the first run has to do more work, because we can reuse
> results for subsequent ones! It would help to "rm -f
> objects/pack/*.bitmap", but even that's not entirely fair, as it will be
> repacking from a single pack, versus whatever state we started with.

You need a "do this too for each iteration but do not time it", i.e.

    test_perf 'repack performance' --prepare '
	make a messy original repository
    ' --per-iteration-prepare '
	prepare a test repository from the messy original
    ' --time-this-part-only '
        git repack -adb
    '

Syntactically, eh, Yuck.