Re: [PATCH 06/18] chainlint.pl: validate test scripts in parallel

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 22, 2022, 00:11 UTC
Message-ID: <221122.86cz9fbyln.gmgdl@evledraar.gmail.com>
In-Reply-To: <CAPig+cQEdidB4YHm9OiyOUe8mbTPBajjX5t-_6ZJVwRykXkqmg@mail.gmail.com>

On Mon, Nov 21 2022, Eric Sunshine wrote:

Show 26 quoted lines

> On Mon, Nov 21, 2022 at 1:52 PM Jeff King <peff@peff.net> wrote:
>> On Mon, Nov 21, 2022 at 01:47:42PM -0500, Eric Sunshine wrote:
>> > I think Ævar's use-case for `make` parallelization was to speed up
>> > git-bisect runs. But thinking about it now, the likelihood of "lint"
>> > problems cropping up during a git-bisect run is effectively nil, in
>> > which case setting GIT_TEST_CHAIN_LINT=1 should be a perfectly
>> > appropriate way to take linting out of the equation when bisecting.
>>
>> Yes. It's also dumb to run a straight "make test" while bisecting in the
>> first place, because you are going to run a zillion tests that aren't
>> relevant to your bisection. Bisecting on "cd t && ./test-that-fails" is
>> faster, at which point you're only running the one lint process (and if
>> it really bothers you, you can disable chain lint as you suggest).
>
> I think I misspoke. Dredging up old memories, I think Ævar's use-case
> is that he now runs:
>
>     git rebase -i --exec 'make test' ...
>
> in order to ensure that the entire test suite passes for _every_ patch
> in a series. (This is due to him having missed a runtime breakage by
> only running "make test" after the final patch in a series was
> applied, when the breakage was only temporary -- added by one patch,
> but resolved by some other later patch.)
>
> Even so, GIT_TEST_CHAIN_LINT=0 should be appropriate here too.

I'd like to make "make" fast in terms of avoiding its own overhead before it gets to actual work mainly because of that use-case, but it helps in general. E.g. if you switch branches we don't compile a file we don't need to, we shouldn't re-run test checks we don't need either.

For t/ this is:

 - Running chainlint.pl on the file, even if it didn't change
 - Ditto check-non-portable-shell.pl
 - Ditto "non-portable file name(s)" check
 - Ditto "test -x" on all test files

I have a branch where these are all checked using dependencies instead, e.g. we run a "test -x" on t0071-sort.sh and create a ".build/check-executable/t0071-sort.sh.ok" if that passed, we don't need to shell out in the common case.

The results of that are, and this is a best case in picking one where
the test itself is cheap:
	
	$ git hyperfine -L rev @{u},HEAD~,HEAD -s 'make CFLAGS=-O3' 'make test T=t0071-sort.sh' -w 1
	Benchmark 1: make test T=t0071-sort.sh' in '@{u}
	  Time (mean ± σ):      1.168 s ±  0.074 s    [User: 1.534 s, System: 0.082 s]
	  Range (min … max):    1.096 s …  1.316 s    10 runs
	
	Benchmark 2: make test T=t0071-sort.sh' in 'HEAD~
	  Time (mean ± σ):     719.1 ms ±  46.1 ms    [User: 910.6 ms, System: 79.7 ms]
	  Range (min … max):   682.0 ms … 828.2 ms    10 runs
	
	Benchmark 3: make test T=t0071-sort.sh' in 'HEAD
	  Time (mean ± σ):     685.0 ms ±  34.2 ms    [User: 645.0 ms, System: 56.8 ms]
	  Range (min … max):   657.6 ms … 773.6 ms    10 runs
	
	Summary
	  'make test T=t0071-sort.sh' in 'HEAD' ran
	    1.05 ± 0.09 times faster than 'make test T=t0071-sort.sh' in 'HEAD~'
	    1.71 ± 0.14 times faster than 'make test T=t0071-sort.sh' in '@{u}'

The @{u} being "master", HEAD~ is "incremant without chainlint.pl", and "HEAD" is where it's all incremental.

It's very WIP-quality, but I pushed the chainlint.pl part of it as a POC just now, I did the others a while ago: https://github.com/avar/git/tree/avar/t-Makefile-break-T-to-file-association