{"thread":{"id":"47513","subject":"[PATCH] perf: amend the grep tests to test grep.threads","startedAt":"2017-12-29T22:59:18Z","lastAt":"2018-01-05T22:28:22Z","messageCount":4,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"335549","messageId":"20171229225903.19688-1-avarab@gmail.com","threadId":"47513","inReplyTo":null,"subject":"[PATCH] perf: amend the grep tests to test grep.threads","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-12-29T22:59:03Z","receivedAt":"2017-12-29T22:59:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Ever since 5b594f457a (\"Threaded grep\", 2010-01-25) the number of\nthreads git-grep uses under PTHREADS has been hardcoded to 8, but\nthere's no performance test to check whether this is an optimal\nsetting.\n\nAmend the existing tests for the grep engines to support a mode where\nthis can be tested, e.g.:\n\n    GIT_PERF_GREP_THREADS='1 8 16' GIT_PERF_LARGE_REPO=~/g/linux ./run p782*\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nThis looks less scary under diff -w.\n\n t/perf/p7820-grep-engines.sh       | 52 ++++++++++++++++++++++++++++-------\n t/perf/p7821-grep-engines-fixed.sh | 55 ++++++++++++++++++++++++++++++--------\n 2 files changed, 86 insertions(+), 21 deletions(-)\n\ndiff --git a/t/perf/p7820-grep-engines.sh b/t/perf/p7820-grep-engines.sh\nindex 62aba19e76..8b09c5bf32 100755\n--- a/t/perf/p7820-grep-engines.sh\n+++ b/t/perf/p7820-grep-engines.sh\n@@ -12,6 +12,9 @@ e.g. GIT_PERF_7820_GREP_OPTS=' -i'. Some options to try:\n \t-vi\n \t-vw\n \t-viw\n+\n+If GIT_PERF_GREP_THREADS is set to a list of threads (e.g. '1 4 8'\n+etc.) we will test the patterns under those numbers of threads.\n \"\n \n . ./perf-lib.sh\n@@ -19,6 +22,11 @@ e.g. GIT_PERF_7820_GREP_OPTS=' -i'. Some options to try:\n test_perf_large_repo\n test_checkout_worktree\n \n+if test -n \"$GIT_PERF_GREP_THREADS\"\n+then\n+\ttest_set_prereq PERF_GREP_ENGINES_THREADS\n+fi\n+\n for pattern in \\\n \t'how.to' \\\n \t'^how to' \\\n@@ -39,18 +47,42 @@ do\n \t\telse\n \t\t\tprereq=\"\"\n \t\tfi\n-\t\ttest_perf $prereq \"$engine grep$GIT_PERF_7820_GREP_OPTS '$pattern'\" \"\n-\t\t\tgit -c grep.patternType=$engine grep$GIT_PERF_7820_GREP_OPTS -- '$pattern' >'out.$engine' || :\n-\t\t\"\n-\tdone\n-\n-\ttest_expect_success \"assert that all engines found the same for$GIT_PERF_7820_GREP_OPTS '$pattern'\" '\n-\t\ttest_cmp out.basic out.extended &&\n-\t\tif test_have_prereq PCRE\n+\t\tif ! test_have_prereq PERF_GREP_ENGINES_THREADS\n \t\tthen\n-\t\t\ttest_cmp out.basic out.perl\n+\t\t\ttest_perf $prereq \"$engine grep$GIT_PERF_7820_GREP_OPTS '$pattern'\" \"\n+\t\t\t\tgit -c grep.patternType=$engine grep$GIT_PERF_7820_GREP_OPTS -- '$pattern' >'out.$engine' || :\n+\t\t\t\"\n+\t\telse\n+\t\t\tfor threads in $GIT_PERF_GREP_THREADS\n+\t\t\tdo\n+\t\t\t\ttest_perf PTHREADS,$prereq \"$engine grep$GIT_PERF_7820_GREP_OPTS '$pattern' with $threads threads\" \"\n+\t\t\t\t\tgit -c grep.patternType=$engine -c grep.threads=$threads grep$GIT_PERF_7820_GREP_OPTS -- '$pattern' >'out.$engine.$threads' || :\n+\t\t\t\t\"\n+\t\t\tdone\n \t\tfi\n-\t'\n+\tdone\n+\n+\tif ! test_have_prereq PERF_GREP_ENGINES_THREADS\n+\tthen\n+\t\ttest_expect_success \"assert that all engines found the same for$GIT_PERF_7820_GREP_OPTS '$pattern'\" '\n+\t\t\ttest_cmp out.basic out.extended &&\n+\t\t\tif test_have_prereq PCRE\n+\t\t\tthen\n+\t\t\t\ttest_cmp out.basic out.perl\n+\t\t\tfi\n+\t\t'\n+\telse\n+\t\tfor threads in $GIT_PERF_GREP_THREADS\n+\t\tdo\n+\t\t\ttest_expect_success PTHREADS \"assert that all engines found the same for$GIT_PERF_7820_GREP_OPTS '$pattern' under threading\" \"\n+\t\t\t\ttest_cmp out.basic.$threads out.extended.$threads &&\n+\t\t\t\tif test_have_prereq PCRE\n+\t\t\t\tthen\n+\t\t\t\t\ttest_cmp out.basic.$threads out.perl.$threads\n+\t\t\t\tfi\n+\t\t\t\"\n+\t\tdone\n+\tfi\n done\n \n test_done\ndiff --git a/t/perf/p7821-grep-engines-fixed.sh b/t/perf/p7821-grep-engines-fixed.sh\nindex c7ef1e198f..61e41b82cf 100755\n--- a/t/perf/p7821-grep-engines-fixed.sh\n+++ b/t/perf/p7821-grep-engines-fixed.sh\n@@ -6,6 +6,9 @@ Set GIT_PERF_7821_GREP_OPTS in the environment to pass options to\n git-grep. Make sure to include a leading space,\n e.g. GIT_PERF_7821_GREP_OPTS=' -w'. See p7820-grep-engines.sh for more\n options to try.\n+\n+If GIT_PERF_7821_THREADS is set to a list of threads (e.g. '1 4 8'\n+etc.) we will test the patterns under those numbers of threads.\n \"\n \n . ./perf-lib.sh\n@@ -13,6 +16,11 @@ options to try.\n test_perf_large_repo\n test_checkout_worktree\n \n+if test -n \"$GIT_PERF_GREP_THREADS\"\n+then\n+\ttest_set_prereq PERF_GREP_ENGINES_THREADS\n+fi\n+\n for pattern in 'int' 'uncommon' 'æ'\n do\n \tfor engine in fixed basic extended perl\n@@ -23,19 +31,44 @@ do\n \t\telse\n \t\t\tprereq=\"\"\n \t\tfi\n-\t\ttest_perf $prereq \"$engine grep$GIT_PERF_7821_GREP_OPTS $pattern\" \"\n-\t\t\tgit -c grep.patternType=$engine grep$GIT_PERF_7821_GREP_OPTS $pattern >'out.$engine' || :\n-\t\t\"\n-\tdone\n-\n-\ttest_expect_success \"assert that all engines found the same for$GIT_PERF_7821_GREP_OPTS $pattern\" '\n-\t\ttest_cmp out.fixed out.basic &&\n-\t\ttest_cmp out.fixed out.extended &&\n-\t\tif test_have_prereq PCRE\n+\t\tif ! test_have_prereq PERF_GREP_ENGINES_THREADS\n \t\tthen\n-\t\t\ttest_cmp out.fixed out.perl\n+\t\t\ttest_perf $prereq \"$engine grep$GIT_PERF_7821_GREP_OPTS $pattern\" \"\n+\t\t\t\tgit -c grep.patternType=$engine grep$GIT_PERF_7821_GREP_OPTS $pattern >'out.$engine' || :\n+\t\t\t\"\n+\t\telse\n+\t\t\tfor threads in $GIT_PERF_GREP_THREADS\n+\t\t\tdo\n+\t\t\t\ttest_perf PTHREADS,$prereq \"$engine grep$GIT_PERF_7821_GREP_OPTS $pattern with $threads threads\" \"\n+\t\t\t\t\tgit -c grep.patternType=$engine -c grep.threads=$threads grep$GIT_PERF_7821_GREP_OPTS $pattern >'out.$engine.$threads' || :\n+\t\t\t\t\"\n+\t\t\tdone\n \t\tfi\n-\t'\n+\tdone\n+\n+\tif ! test_have_prereq PERF_GREP_ENGINES_THREADS\n+\tthen\n+\t\ttest_expect_success \"assert that all engines found the same for$GIT_PERF_7821_GREP_OPTS $pattern\" '\n+\t\t\ttest_cmp out.fixed out.basic &&\n+\t\t\ttest_cmp out.fixed out.extended &&\n+\t\t\tif test_have_prereq PCRE\n+\t\t\tthen\n+\t\t\t\ttest_cmp out.fixed out.perl\n+\t\t\tfi\n+\t\t'\n+\telse\n+\t\tfor threads in $GIT_PERF_GREP_THREADS\n+\t\tdo\n+\t\t\ttest_expect_success PTHREADS \"assert that all engines found the same for$GIT_PERF_7821_GREP_OPTS $pattern under threading\" \"\n+\t\t\t\ttest_cmp out.fixed.$threads out.basic.$threads &&\n+\t\t\t\ttest_cmp out.fixed.$threads out.extended.$threads &&\n+\t\t\t\tif test_have_prereq PCRE\n+\t\t\t\tthen\n+\t\t\t\t\ttest_cmp out.fixed.$threads out.perl.$threads\n+\t\t\t\tfi\n+\t\t\t\"\n+\t\tdone\n+\tfi\n done\n \n test_done\n-- \n2.15.1.424.g9478a66081\n\n"},{"id":"335833","messageId":"xmqqinchfowj.fsf@gitster.mtv.corp.google.com","threadId":"47513","inReplyTo":"20171229225903.19688-1-avarab@gmail.com","subject":"Re: [PATCH] perf: amend the grep tests to test grep.threads","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-04T18:40:44Z","receivedAt":"2018-01-04T18:40:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Ever since 5b594f457a (\"Threaded grep\", 2010-01-25) the number of\n> threads git-grep uses under PTHREADS has been hardcoded to 8, but\n> there's no performance test to check whether this is an optimal\n> setting.\n>\n> Amend the existing tests for the grep engines to support a mode where\n> this can be tested, e.g.:\n>\n>     GIT_PERF_GREP_THREADS='1 8 16' GIT_PERF_LARGE_REPO=~/g/linux ./run p782*\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>\n> This looks less scary under diff -w.\n>\n>  t/perf/p7820-grep-engines.sh       | 52 ++++++++++++++++++++++++++++-------\n>  t/perf/p7821-grep-engines-fixed.sh | 55 ++++++++++++++++++++++++++++++--------\n>  2 files changed, 86 insertions(+), 21 deletions(-)\n>\n> diff --git a/t/perf/p7820-grep-engines.sh b/t/perf/p7820-grep-engines.sh\n> index 62aba19e76..8b09c5bf32 100755\n> --- a/t/perf/p7820-grep-engines.sh\n> +++ b/t/perf/p7820-grep-engines.sh\n> @@ -12,6 +12,9 @@ e.g. GIT_PERF_7820_GREP_OPTS=' -i'. Some options to try:\n> ...\n> +\t\tif ! test_have_prereq PERF_GREP_ENGINES_THREADS\n>  \t\tthen\n> -\t\t\ttest_cmp out.basic out.perl\n> +\t\t\ttest_perf $prereq \"$engine grep$GIT_PERF_7820_GREP_OPTS '$pattern'\" \"\n> +\t\t\t\tgit -c grep.patternType=$engine grep$GIT_PERF_7820_GREP_OPTS -- '$pattern' >'out.$engine' || :\n> +\t\t\t\"\n> +\t\telse\n> +\t\t\tfor threads in $GIT_PERF_GREP_THREADS\n> +\t\t\tdo\n> +\t\t\t\ttest_perf PTHREADS,$prereq \"$engine grep$GIT_PERF_7820_GREP_OPTS '$pattern' with $threads threads\" \"\n\nIs it guaranteed that $prereq is not empty at this point?  \n\nJudging by the way the other side uses \"test_perf $prereq ...\"\nwithout quotes around it, I suspect you do expect it to be empty in\nsome cases.  It means you expect test_have_prereq is prepared to\nskip an empty prerequisite in a prereq list, but I do not recall\nwriting that helper in such a way, so...\n\n\tPTHREADS${prereq:+,}$prereq\n\nor something along that line, perhaps?\n\n> diff --git a/t/perf/p7821-grep-engines-fixed.sh b/t/perf/p7821-grep-engines-fixed.sh\n> index c7ef1e198f..61e41b82cf 100755\n> --- a/t/perf/p7821-grep-engines-fixed.sh\n> +++ b/t/perf/p7821-grep-engines-fixed.sh\n> @@ -6,6 +6,9 @@ Set GIT_PERF_7821_GREP_OPTS in the environment to pass options to\n> ...\n>  for pattern in 'int' 'uncommon' 'æ'\n>  do\n>  \tfor engine in fixed basic extended perl\n> @@ -23,19 +31,44 @@ do\n> ...\n> +\t\tif ! test_have_prereq PERF_GREP_ENGINES_THREADS\n>  \t\tthen\n> -\t\t\ttest_cmp out.fixed out.perl\n> +\t\t\ttest_perf $prereq \"$engine grep$GIT_PERF_7821_GREP_OPTS $pattern\" \"\n> +\t\t\t\tgit -c grep.patternType=$engine grep$GIT_PERF_7821_GREP_OPTS $pattern >'out.$engine' || :\n> +\t\t\t\"\n> +\t\telse\n> +\t\t\tfor threads in $GIT_PERF_GREP_THREADS\n> +\t\t\tdo\n> +\t\t\t\ttest_perf PTHREADS,$prereq \"$engine grep$GIT_PERF_7821_GREP_OPTS $pattern with $threads threads\" \"\n> +\t\t\t\t\tgit -c grep.patternType=$engine -c grep.threads=$threads grep$GIT_PERF_7821_GREP_OPTS $pattern >'out.$engine.$threads' || :\n> +\t\t\t\t\"\n> +\t\t\tdone\n\nSame here, which means these two scripts share somewhat large body\nof text and makes me wonder if it is worth refactoring it to ease\nfuture updates to them.\n\nThanks.\n"},{"id":"336017","messageId":"87o9m8asys.fsf@evledraar.gmail.com","threadId":"47513","inReplyTo":"xmqqinchfowj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] perf: amend the grep tests to test grep.threads","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-01-05T21:36:27Z","receivedAt":"2018-01-05T21:36:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jan 04 2018, Junio C. Hamano jotted:\n\n> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n>\n>> Ever since 5b594f457a (\"Threaded grep\", 2010-01-25) the number of\n>> threads git-grep uses under PTHREADS has been hardcoded to 8, but\n>> there's no performance test to check whether this is an optimal\n>> setting.\n>>\n>> Amend the existing tests for the grep engines to support a mode where\n>> this can be tested, e.g.:\n>>\n>>     GIT_PERF_GREP_THREADS='1 8 16' GIT_PERF_LARGE_REPO=~/g/linux ./run p782*\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n>> ---\n>>\n>> This looks less scary under diff -w.\n>>\n>>  t/perf/p7820-grep-engines.sh       | 52 ++++++++++++++++++++++++++++-------\n>>  t/perf/p7821-grep-engines-fixed.sh | 55 ++++++++++++++++++++++++++++++--------\n>>  2 files changed, 86 insertions(+), 21 deletions(-)\n>>\n>> diff --git a/t/perf/p7820-grep-engines.sh b/t/perf/p7820-grep-engines.sh\n>> index 62aba19e76..8b09c5bf32 100755\n>> --- a/t/perf/p7820-grep-engines.sh\n>> +++ b/t/perf/p7820-grep-engines.sh\n>> @@ -12,6 +12,9 @@ e.g. GIT_PERF_7820_GREP_OPTS=' -i'. Some options to try:\n>> ...\n>> +\t\tif ! test_have_prereq PERF_GREP_ENGINES_THREADS\n>>  \t\tthen\n>> -\t\t\ttest_cmp out.basic out.perl\n>> +\t\t\ttest_perf $prereq \"$engine grep$GIT_PERF_7820_GREP_OPTS '$pattern'\" \"\n>> +\t\t\t\tgit -c grep.patternType=$engine grep$GIT_PERF_7820_GREP_OPTS -- '$pattern' >'out.$engine' || :\n>> +\t\t\t\"\n>> +\t\telse\n>> +\t\t\tfor threads in $GIT_PERF_GREP_THREADS\n>> +\t\t\tdo\n>> +\t\t\t\ttest_perf PTHREADS,$prereq \"$engine grep$GIT_PERF_7820_GREP_OPTS '$pattern' with $threads threads\" \"\n>\n> Is it guaranteed that $prereq is not empty at this point?\n>\n> Judging by the way the other side uses \"test_perf $prereq ...\"\n> without quotes around it, I suspect you do expect it to be empty in\n> some cases.  It means you expect test_have_prereq is prepared to\n> skip an empty prerequisite in a prereq list, but I do not recall\n> writing that helper in such a way, so...\n>\n> \tPTHREADS${prereq:+,}$prereq\n>\n> or something along that line, perhaps?\n\nIt's not, but a trailing comma at the end of the prereq list works since\ntest_have_prereq() relies on setting the IFS to \",\", so as long as this\nis portable:\n\n    $ str=\"foo,bar,baz,\"; export IFS=,; for word in $str; do echo \"w:<$word>\"; done\n    w:<foo>\n    w:<bar>\n    w:<baz>\n\nAnd I'm not 100% sure that it is, this should be fine. Works on both\nbash & dash for me.\n\nWe could use the ${...} pattern above if you prefer, but since it\ndoesn't appear to be needed...\n\n>> diff --git a/t/perf/p7821-grep-engines-fixed.sh b/t/perf/p7821-grep-engines-fixed.sh\n>> index c7ef1e198f..61e41b82cf 100755\n>> --- a/t/perf/p7821-grep-engines-fixed.sh\n>> +++ b/t/perf/p7821-grep-engines-fixed.sh\n>> @@ -6,6 +6,9 @@ Set GIT_PERF_7821_GREP_OPTS in the environment to pass options to\n>> ...\n>>  for pattern in 'int' 'uncommon' 'æ'\n>>  do\n>>  \tfor engine in fixed basic extended perl\n>> @@ -23,19 +31,44 @@ do\n>> ...\n>> +\t\tif ! test_have_prereq PERF_GREP_ENGINES_THREADS\n>>  \t\tthen\n>> -\t\t\ttest_cmp out.fixed out.perl\n>> +\t\t\ttest_perf $prereq \"$engine grep$GIT_PERF_7821_GREP_OPTS $pattern\" \"\n>> +\t\t\t\tgit -c grep.patternType=$engine grep$GIT_PERF_7821_GREP_OPTS $pattern >'out.$engine' || :\n>> +\t\t\t\"\n>> +\t\telse\n>> +\t\t\tfor threads in $GIT_PERF_GREP_THREADS\n>> +\t\t\tdo\n>> +\t\t\t\ttest_perf PTHREADS,$prereq \"$engine grep$GIT_PERF_7821_GREP_OPTS $pattern with $threads threads\" \"\n>> +\t\t\t\t\tgit -c grep.patternType=$engine -c grep.threads=$threads grep$GIT_PERF_7821_GREP_OPTS $pattern >'out.$engine.$threads' || :\n>> +\t\t\t\t\"\n>> +\t\t\tdone\n>\n> Same here, which means these two scripts share somewhat large body\n> of text and makes me wonder if it is worth refactoring it to ease\n> future updates to them.\n\nYeah, but I wanted to leave refactoring these for some other time.\n"},{"id":"336024","messageId":"xmqqa7xsaqki.fsf@gitster.mtv.corp.google.com","threadId":"47513","inReplyTo":"87o9m8asys.fsf@evledraar.gmail.com","subject":"Re: [PATCH] perf: amend the grep tests to test grep.threads","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-05T22:28:13Z","receivedAt":"2018-01-05T22:28:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n>> Judging by the way the other side uses \"test_perf $prereq ...\"\n>> without quotes around it, I suspect you do expect it to be empty in\n>> some cases.  It means you expect test_have_prereq is prepared to\n>> skip an empty prerequisite in a prereq list, but I do not recall\n>> writing that helper in such a way, so...\n>>\n>> \tPTHREADS${prereq:+,}$prereq\n>>\n>> or something along that line, perhaps?\n>\n> It's not, but a trailing comma at the end of the prereq list works since\n> test_have_prereq() relies on setting the IFS to \",\", so as long as this\n> is portable:\n> ...\n> We could use the ${...} pattern above if you prefer, but since it\n> doesn't appear to be needed...\n\nI view that reasoning as depending more on how the helper happens to\nbe implemented right now than working by design, though.\n\n"}]}