* [PATCH] perf: amend the grep tests to test grep.threads
@ 2017-12-29 22:59 Ævar Arnfjörð Bjarmason
2018-01-04 18:40 ` Junio C Hamano
0 siblings, 1 reply; 4+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-12-29 22:59 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason
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:
-vi
-vw
-viw
+
+If GIT_PERF_GREP_THREADS is set to a list of threads (e.g. '1 4 8'
+etc.) we will test the patterns under those numbers of threads.
"
. ./perf-lib.sh
@@ -19,6 +22,11 @@ e.g. GIT_PERF_7820_GREP_OPTS=' -i'. Some options to try:
test_perf_large_repo
test_checkout_worktree
+if test -n "$GIT_PERF_GREP_THREADS"
+then
+ test_set_prereq PERF_GREP_ENGINES_THREADS
+fi
+
for pattern in \
'how.to' \
'^how to' \
@@ -39,18 +47,42 @@ do
else
prereq=""
fi
- 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' || :
- "
- done
-
- test_expect_success "assert that all engines found the same for$GIT_PERF_7820_GREP_OPTS '$pattern'" '
- test_cmp out.basic out.extended &&
- if test_have_prereq PCRE
+ 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" "
+ git -c grep.patternType=$engine -c grep.threads=$threads grep$GIT_PERF_7820_GREP_OPTS -- '$pattern' >'out.$engine.$threads' || :
+ "
+ done
fi
- '
+ done
+
+ if ! test_have_prereq PERF_GREP_ENGINES_THREADS
+ then
+ test_expect_success "assert that all engines found the same for$GIT_PERF_7820_GREP_OPTS '$pattern'" '
+ test_cmp out.basic out.extended &&
+ if test_have_prereq PCRE
+ then
+ test_cmp out.basic out.perl
+ fi
+ '
+ else
+ for threads in $GIT_PERF_GREP_THREADS
+ do
+ test_expect_success PTHREADS "assert that all engines found the same for$GIT_PERF_7820_GREP_OPTS '$pattern' under threading" "
+ test_cmp out.basic.$threads out.extended.$threads &&
+ if test_have_prereq PCRE
+ then
+ test_cmp out.basic.$threads out.perl.$threads
+ fi
+ "
+ done
+ fi
done
test_done
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
git-grep. Make sure to include a leading space,
e.g. GIT_PERF_7821_GREP_OPTS=' -w'. See p7820-grep-engines.sh for more
options to try.
+
+If GIT_PERF_7821_THREADS is set to a list of threads (e.g. '1 4 8'
+etc.) we will test the patterns under those numbers of threads.
"
. ./perf-lib.sh
@@ -13,6 +16,11 @@ options to try.
test_perf_large_repo
test_checkout_worktree
+if test -n "$GIT_PERF_GREP_THREADS"
+then
+ test_set_prereq PERF_GREP_ENGINES_THREADS
+fi
+
for pattern in 'int' 'uncommon' 'æ'
do
for engine in fixed basic extended perl
@@ -23,19 +31,44 @@ do
else
prereq=""
fi
- 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' || :
- "
- done
-
- test_expect_success "assert that all engines found the same for$GIT_PERF_7821_GREP_OPTS $pattern" '
- test_cmp out.fixed out.basic &&
- test_cmp out.fixed out.extended &&
- if test_have_prereq PCRE
+ 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
fi
- '
+ done
+
+ if ! test_have_prereq PERF_GREP_ENGINES_THREADS
+ then
+ test_expect_success "assert that all engines found the same for$GIT_PERF_7821_GREP_OPTS $pattern" '
+ test_cmp out.fixed out.basic &&
+ test_cmp out.fixed out.extended &&
+ if test_have_prereq PCRE
+ then
+ test_cmp out.fixed out.perl
+ fi
+ '
+ else
+ for threads in $GIT_PERF_GREP_THREADS
+ do
+ test_expect_success PTHREADS "assert that all engines found the same for$GIT_PERF_7821_GREP_OPTS $pattern under threading" "
+ test_cmp out.fixed.$threads out.basic.$threads &&
+ test_cmp out.fixed.$threads out.extended.$threads &&
+ if test_have_prereq PCRE
+ then
+ test_cmp out.fixed.$threads out.perl.$threads
+ fi
+ "
+ done
+ fi
done
test_done
--
2.15.1.424.g9478a66081
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] perf: amend the grep tests to test grep.threads
2017-12-29 22:59 [PATCH] perf: amend the grep tests to test grep.threads Ævar Arnfjörð Bjarmason
@ 2018-01-04 18:40 ` Junio C Hamano
2018-01-05 21:36 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 4+ messages in thread
From: Junio C Hamano @ 2018-01-04 18:40 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git
Æ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?
> 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.
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] perf: amend the grep tests to test grep.threads
2018-01-04 18:40 ` Junio C Hamano
@ 2018-01-05 21:36 ` Ævar Arnfjörð Bjarmason
2018-01-05 22:28 ` Junio C Hamano
0 siblings, 1 reply; 4+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-01-05 21:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Thu, Jan 04 2018, Junio C. Hamano jotted:
> Æ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...
>> 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.
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] perf: amend the grep tests to test grep.threads
2018-01-05 21:36 ` Ævar Arnfjörð Bjarmason
@ 2018-01-05 22:28 ` Junio C Hamano
0 siblings, 0 replies; 4+ messages in thread
From: Junio C Hamano @ 2018-01-05 22:28 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>> 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:
> ...
> We could use the ${...} pattern above if you prefer, but since it
> doesn't appear to be needed...
I view that reasoning as depending more on how the helper happens to
be implemented right now than working by design, though.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-01-05 22:28 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-12-29 22:59 [PATCH] perf: amend the grep tests to test grep.threads Ævar Arnfjörð Bjarmason
2018-01-04 18:40 ` Junio C Hamano
2018-01-05 21:36 ` Ævar Arnfjörð Bjarmason
2018-01-05 22:28 ` Junio C Hamano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).