Re: [PATCH] perf: amend the grep tests to test grep.threads

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jan 5, 2018, 21:36 UTC
Message-ID: <87o9m8asys.fsf@evledraar.gmail.com>
In-Reply-To: <xmqqinchfowj.fsf@gitster.mtv.corp.google.com>

On Thu, Jan 04 2018, Junio C. Hamano jotted:

Show 49 quoted lines

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Ever since 5b594f457a ("Threaded grep", 2010-01-25) the number of
>> threads git-grep uses under PTHREADS has been hardcoded to 8, but
>> there's no performance test to check whether this is an optimal
>> setting.
>>
>> Amend the existing tests for the grep engines to support a mode where
>> this can be tested, e.g.:
>>
>>     GIT_PERF_GREP_THREADS='1 8 16' GIT_PERF_LARGE_REPO=~/g/linux ./run p782*
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>
>> This looks less scary under diff -w.
>>
>>  t/perf/p7820-grep-engines.sh       | 52 ++++++++++++++++++++++++++++-------
>>  t/perf/p7821-grep-engines-fixed.sh | 55 ++++++++++++++++++++++++++++++--------
>>  2 files changed, 86 insertions(+), 21 deletions(-)
>>
>> diff --git a/t/perf/p7820-grep-engines.sh b/t/perf/p7820-grep-engines.sh
>> index 62aba19e76..8b09c5bf32 100755
>> --- a/t/perf/p7820-grep-engines.sh
>> +++ b/t/perf/p7820-grep-engines.sh
>> @@ -12,6 +12,9 @@ e.g. GIT_PERF_7820_GREP_OPTS=' -i'. Some options to try:
>> ...
>> +		if ! test_have_prereq PERF_GREP_ENGINES_THREADS
>>  		then
>> -			test_cmp out.basic out.perl
>> +			test_perf $prereq "$engine grep$GIT_PERF_7820_GREP_OPTS '$pattern'" "
>> +				git -c grep.patternType=$engine grep$GIT_PERF_7820_GREP_OPTS -- '$pattern' >'out.$engine' || :
>> +			"
>> +		else
>> +			for threads in $GIT_PERF_GREP_THREADS
>> +			do
>> +				test_perf PTHREADS,$prereq "$engine grep$GIT_PERF_7820_GREP_OPTS '$pattern' with $threads threads" "
>
> Is it guaranteed that $prereq is not empty at this point?
>
> Judging by the way the other side uses "test_perf $prereq ..."
> without quotes around it, I suspect you do expect it to be empty in
> some cases.  It means you expect test_have_prereq is prepared to
> skip an empty prerequisite in a prereq list, but I do not recall
> writing that helper in such a way, so...
>
> 	PTHREADS${prereq:+,}$prereq
>
> or something along that line, perhaps?

It's not, but a trailing comma at the end of the prereq list works since test_have_prereq() relies on setting the IFS to ",", so as long as this is portable:

    $ str="foo,bar,baz,"; export IFS=,; for word in $str; do echo "w:<$word>"; done
    w:<foo>
    w:<bar>
    w:<baz>

And I'm not 100% sure that it is, this should be fine. Works on both bash & dash for me.

We could use the ${...} pattern above if you prefer, but since it doesn't appear to be needed...

Show 28 quoted lines

>> diff --git a/t/perf/p7821-grep-engines-fixed.sh b/t/perf/p7821-grep-engines-fixed.sh
>> index c7ef1e198f..61e41b82cf 100755
>> --- a/t/perf/p7821-grep-engines-fixed.sh
>> +++ b/t/perf/p7821-grep-engines-fixed.sh
>> @@ -6,6 +6,9 @@ Set GIT_PERF_7821_GREP_OPTS in the environment to pass options to
>> ...
>>  for pattern in 'int' 'uncommon' 'æ'
>>  do
>>  	for engine in fixed basic extended perl
>> @@ -23,19 +31,44 @@ do
>> ...
>> +		if ! test_have_prereq PERF_GREP_ENGINES_THREADS
>>  		then
>> -			test_cmp out.fixed out.perl
>> +			test_perf $prereq "$engine grep$GIT_PERF_7821_GREP_OPTS $pattern" "
>> +				git -c grep.patternType=$engine grep$GIT_PERF_7821_GREP_OPTS $pattern >'out.$engine' || :
>> +			"
>> +		else
>> +			for threads in $GIT_PERF_GREP_THREADS
>> +			do
>> +				test_perf PTHREADS,$prereq "$engine grep$GIT_PERF_7821_GREP_OPTS $pattern with $threads threads" "
>> +					git -c grep.patternType=$engine -c grep.threads=$threads grep$GIT_PERF_7821_GREP_OPTS $pattern >'out.$engine.$threads' || :
>> +				"
>> +			done
>
> Same here, which means these two scripts share somewhat large body
> of text and makes me wonder if it is worth refactoring it to ease
> future updates to them.

Yeah, but I wanted to leave refactoring these for some other time.