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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 4, 2018, 18:40 UTC
Message-ID: <xmqqinchfowj.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20171229225903.19688-1-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 35 quoted lines

> 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?

Show 24 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.

Thanks.