On Thu, Jan 04 2018, Junio C. Hamano jotted:
> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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@xxxxxxxxx>
>> ---
>>
>> 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...
>> 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.
