# [PATCH 0/2] Two perf test fixes 13 messages from 2025-03-28 to 2025-04-14. Participants: Philippe Blain via GitGitGadget, Patrick Steinhardt, Philippe Blain, Junio C Hamano. Thread: https://gitlist.dev/t/63213 ## Philippe Blain via GitGitGadget, 2025-03-28 17:07 Subject: [PATCH 0/2] Two perf test fixes Message-ID: URL: https://gitlist.dev/e/pull.1936.git.git.1743181669.gitgitgadget%40gmail.com ``` Here a two fixes for failures I noticed while running the perf tests. Philippe Blain (2): p7821: fix test_perf invocation for prereqs p9210: fix 'scalar clone' when running from a detached HEAD t/perf/p7821-grep-engines-fixed.sh | 4 ++-- t/perf/p9210-scalar.sh | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1936%2Fphil-blain%2Fperf-test-fixes-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1936/phil-blain/perf-test-fixes-v1 Pull-Request: https://github.com/git/git/pull/1936 -- gitgitgadget ``` ## Philippe Blain via GitGitGadget, 2025-03-28 17:07 Subject: [PATCH 1/2] p7821: fix test_perf invocation for prereqs Message-ID: <41a093d570a5756f730b069980edafbcedf5c8bc.1743181669.git.gitgitgadget@gmail.com> URL: https://gitlist.dev/e/41a093d570a5756f730b069980edafbcedf5c8bc.1743181669.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Philippe Blain Since 5dccd9155f (t/perf: add iteration setup mechanism to perf-lib, 2022-04-04), perf tests need to declare their prerequisites with '--prereq', after the test title. p7821 was forgotten in that commit, such that running that test on a machine where the PCRE prereq is not satisfied aborts the test with: error: bug in the test script: test_wrapper_ needs 2 positional parameters Fix this by correcting the two 'test_perf' invocations in that test suite. Signed-off-by: Philippe Blain --- t/perf/p7821-grep-engines-fixed.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/perf/p7821-grep-engines-fixed.sh b/t/perf/p7821-grep-engines-fixed.sh index 61e41b82cff..1d126c7b039 100755 --- a/t/perf/p7821-grep-engines-fixed.sh +++ b/t/perf/p7821-grep-engines-fixed.sh @@ -33,13 +33,13 @@ do fi if ! test_have_prereq PERF_GREP_ENGINES_THREADS then - test_perf $prereq "$engine grep$GIT_PERF_7821_GREP_OPTS $pattern" " + test_perf "$engine grep$GIT_PERF_7821_GREP_OPTS $pattern" --prereq "$prereq" " 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" " + test_perf "$engine grep$GIT_PERF_7821_GREP_OPTS $pattern with $threads threads" --prereq "PTHREADS,$prereq" " git -c grep.patternType=$engine -c grep.threads=$threads grep$GIT_PERF_7821_GREP_OPTS $pattern >'out.$engine.$threads' || : " done -- gitgitgadget ``` ## Philippe Blain via GitGitGadget, 2025-03-28 17:07 Subject: [PATCH 2/2] p9210: fix 'scalar clone' when running from a detached HEAD Message-ID: <1092c32609f249839453052ca802cb10256cb48f.1743181669.git.gitgitgadget@gmail.com> URL: https://gitlist.dev/e/1092c32609f249839453052ca802cb10256cb48f.1743181669.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Philippe Blain In p9210-scalar-clone.sh, we test using 'scalar clone' to clone $GIT_PERF_LARGE_REPO (copied locally as 'to-clone'), which defaults to the git.git checkout we are running the test from. When --branch is not specified (as in this test), 'scalar clone' tries to get the default branch of the remote repository by parsing the output of 'git ls-remote --symref $URL HEAD', as implemented in scalar.c:remote_default_branch. When the git.git checkout we are running the test from is in detached HEAD, this fails and we fall back to using the name of the currently checked out branch in the newly initialized repository, which in this case is the value returned earlier in cmd_clone by repo_default_branch_name. We then invoke 'git checkout -t origin/$branch', with $branch being the name we got from remote_default_branch. This invocation fails if '$branch' does not exist as a branch in the current git.git checkout. Fix this by creating a local branch in 'to-clone' in the setup test "enable server-side partial clone", making sure to use '-B' in case a branch named 'test-branch' already exists. Signed-off-by: Philippe Blain --- t/perf/p9210-scalar.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/t/perf/p9210-scalar.sh b/t/perf/p9210-scalar.sh index 265f7cd1fe2..56b075e906e 100755 --- a/t/perf/p9210-scalar.sh +++ b/t/perf/p9210-scalar.sh @@ -7,7 +7,8 @@ test_perf_large_repo "$TRASH_DIRECTORY/to-clone" test_expect_success 'enable server-side partial clone' ' git -C to-clone config uploadpack.allowFilter true && - git -C to-clone config uploadpack.allowAnySHA1InWant true + git -C to-clone config uploadpack.allowAnySHA1InWant true && + git -C to-clone checkout -B test-branch ' test_perf 'scalar clone' ' -- gitgitgadget ``` ## Patrick Steinhardt, 2025-03-31 07:27 Subject: Re: [PATCH 1/2] p7821: fix test_perf invocation for prereqs Message-ID: URL: https://gitlist.dev/e/Z-pD1puYT87YKAd4%40pks.im In-Reply-To: <41a093d570a5756f730b069980edafbcedf5c8bc.1743181669.git.gitgitgadget@gmail.com> ``` On Fri, Mar 28, 2025 at 05:07:48PM +0000, Philippe Blain via GitGitGadget wrote: > diff --git a/t/perf/p7821-grep-engines-fixed.sh b/t/perf/p7821-grep-engines-fixed.sh > index 61e41b82cff..1d126c7b039 100755 > --- a/t/perf/p7821-grep-engines-fixed.sh > +++ b/t/perf/p7821-grep-engines-fixed.sh > @@ -33,13 +33,13 @@ do > fi > if ! test_have_prereq PERF_GREP_ENGINES_THREADS > then > - test_perf $prereq "$engine grep$GIT_PERF_7821_GREP_OPTS $pattern" " > + test_perf "$engine grep$GIT_PERF_7821_GREP_OPTS $pattern" --prereq "$prereq" " > 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" " > + test_perf "$engine grep$GIT_PERF_7821_GREP_OPTS $pattern with $threads threads" --prereq "PTHREADS,$prereq" " > git -c grep.patternType=$engine -c grep.threads=$threads grep$GIT_PERF_7821_GREP_OPTS $pattern >'out.$engine.$threads' || : > " > done "$prereq" can be empty here as it depends on which regexp engine we're using. The second case you adapt already looked weird before because we potentially checked for "PTHREADS,", but the first case was correct before but is now potentially checking for the empty prerequisite. Does that actually work as expected? Patrick ``` ## Patrick Steinhardt, 2025-03-31 07:27 Subject: Re: [PATCH 2/2] p9210: fix 'scalar clone' when running from a detached HEAD Message-ID: URL: https://gitlist.dev/e/Z-pD2aeCJ6yp9XBN%40pks.im In-Reply-To: <1092c32609f249839453052ca802cb10256cb48f.1743181669.git.gitgitgadget@gmail.com> ``` On Fri, Mar 28, 2025 at 05:07:49PM +0000, Philippe Blain via GitGitGadget wrote: > From: Philippe Blain > > In p9210-scalar-clone.sh, we test using 'scalar clone' to clone > $GIT_PERF_LARGE_REPO (copied locally as 'to-clone'), which defaults to > the git.git checkout we are running the test from. > > When --branch is not specified (as in this test), 'scalar clone' tries > to get the default branch of the remote repository by parsing the output > of 'git ls-remote --symref $URL HEAD', as implemented in > scalar.c:remote_default_branch. When the git.git checkout we are running > the test from is in detached HEAD, this fails and we fall back to using > the name of the currently checked out branch in the newly initialized > repository, which in this case is the value returned earlier in > cmd_clone by repo_default_branch_name. > > We then invoke 'git checkout -t origin/$branch', with $branch being the > name we got from remote_default_branch. This invocation fails if > '$branch' does not exist as a branch in the current git.git checkout. > > Fix this by creating a local branch in 'to-clone' in the setup test > "enable server-side partial clone", making sure to use '-B' in case a > branch named 'test-branch' already exists. > > Signed-off-by: Philippe Blain > --- > t/perf/p9210-scalar.sh | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/t/perf/p9210-scalar.sh b/t/perf/p9210-scalar.sh > index 265f7cd1fe2..56b075e906e 100755 > --- a/t/perf/p9210-scalar.sh > +++ b/t/perf/p9210-scalar.sh > @@ -7,7 +7,8 @@ test_perf_large_repo "$TRASH_DIRECTORY/to-clone" > > test_expect_success 'enable server-side partial clone' ' > git -C to-clone config uploadpack.allowFilter true && > - git -C to-clone config uploadpack.allowAnySHA1InWant true > + git -C to-clone config uploadpack.allowAnySHA1InWant true && > + git -C to-clone checkout -B test-branch > ' This feels like an easy and pragmatic fix. Thanks! Patrick ``` ## Philippe Blain via GitGitGadget, 2025-04-12 18:15 Subject: [PATCH v2 0/3] Two perf test fixes Message-ID: URL: https://gitlist.dev/e/pull.1936.v2.git.git.1744481732.gitgitgadget%40gmail.com In-Reply-To: ``` Here a two fixes for failures I noticed while running the perf tests. Philippe Blain (3): p7821: fix test_perf invocation for prereqs p9210: fix 'scalar clone' when running from a detached HEAD p7821: fix instructions for testing with threads t/perf/p7821-grep-engines-fixed.sh | 6 +++--- t/perf/p9210-scalar.sh | 3 ++- 2 files changed, 5 insertions(+), 4 deletions(-) base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1936%2Fphil-blain%2Fperf-test-fixes-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1936/phil-blain/perf-test-fixes-v2 Pull-Request: https://github.com/git/git/pull/1936 Range-diff vs v1: 1: 41a093d570a = 1: 41a093d570a p7821: fix test_perf invocation for prereqs 2: 1092c32609f = 2: 1092c32609f p9210: fix 'scalar clone' when running from a detached HEAD -: ----------- > 3: abd146b7c2a p7821: fix instructions for testing with threads -- gitgitgadget ``` ## Philippe Blain via GitGitGadget, 2025-04-12 18:15 Subject: [PATCH v2 1/3] p7821: fix test_perf invocation for prereqs Message-ID: <41a093d570a5756f730b069980edafbcedf5c8bc.1744481732.git.gitgitgadget@gmail.com> URL: https://gitlist.dev/e/41a093d570a5756f730b069980edafbcedf5c8bc.1744481732.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Philippe Blain Since 5dccd9155f (t/perf: add iteration setup mechanism to perf-lib, 2022-04-04), perf tests need to declare their prerequisites with '--prereq', after the test title. p7821 was forgotten in that commit, such that running that test on a machine where the PCRE prereq is not satisfied aborts the test with: error: bug in the test script: test_wrapper_ needs 2 positional parameters Fix this by correcting the two 'test_perf' invocations in that test suite. Signed-off-by: Philippe Blain --- t/perf/p7821-grep-engines-fixed.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/perf/p7821-grep-engines-fixed.sh b/t/perf/p7821-grep-engines-fixed.sh index 61e41b82cff..1d126c7b039 100755 --- a/t/perf/p7821-grep-engines-fixed.sh +++ b/t/perf/p7821-grep-engines-fixed.sh @@ -33,13 +33,13 @@ do fi if ! test_have_prereq PERF_GREP_ENGINES_THREADS then - test_perf $prereq "$engine grep$GIT_PERF_7821_GREP_OPTS $pattern" " + test_perf "$engine grep$GIT_PERF_7821_GREP_OPTS $pattern" --prereq "$prereq" " 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" " + test_perf "$engine grep$GIT_PERF_7821_GREP_OPTS $pattern with $threads threads" --prereq "PTHREADS,$prereq" " git -c grep.patternType=$engine -c grep.threads=$threads grep$GIT_PERF_7821_GREP_OPTS $pattern >'out.$engine.$threads' || : " done -- gitgitgadget ``` ## Philippe Blain via GitGitGadget, 2025-04-12 18:15 Subject: [PATCH v2 2/3] p9210: fix 'scalar clone' when running from a detached HEAD Message-ID: <1092c32609f249839453052ca802cb10256cb48f.1744481732.git.gitgitgadget@gmail.com> URL: https://gitlist.dev/e/1092c32609f249839453052ca802cb10256cb48f.1744481732.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Philippe Blain In p9210-scalar-clone.sh, we test using 'scalar clone' to clone $GIT_PERF_LARGE_REPO (copied locally as 'to-clone'), which defaults to the git.git checkout we are running the test from. When --branch is not specified (as in this test), 'scalar clone' tries to get the default branch of the remote repository by parsing the output of 'git ls-remote --symref $URL HEAD', as implemented in scalar.c:remote_default_branch. When the git.git checkout we are running the test from is in detached HEAD, this fails and we fall back to using the name of the currently checked out branch in the newly initialized repository, which in this case is the value returned earlier in cmd_clone by repo_default_branch_name. We then invoke 'git checkout -t origin/$branch', with $branch being the name we got from remote_default_branch. This invocation fails if '$branch' does not exist as a branch in the current git.git checkout. Fix this by creating a local branch in 'to-clone' in the setup test "enable server-side partial clone", making sure to use '-B' in case a branch named 'test-branch' already exists. Signed-off-by: Philippe Blain --- t/perf/p9210-scalar.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/t/perf/p9210-scalar.sh b/t/perf/p9210-scalar.sh index 265f7cd1fe2..56b075e906e 100755 --- a/t/perf/p9210-scalar.sh +++ b/t/perf/p9210-scalar.sh @@ -7,7 +7,8 @@ test_perf_large_repo "$TRASH_DIRECTORY/to-clone" test_expect_success 'enable server-side partial clone' ' git -C to-clone config uploadpack.allowFilter true && - git -C to-clone config uploadpack.allowAnySHA1InWant true + git -C to-clone config uploadpack.allowAnySHA1InWant true && + git -C to-clone checkout -B test-branch ' test_perf 'scalar clone' ' -- gitgitgadget ``` ## Philippe Blain via GitGitGadget, 2025-04-12 18:15 Subject: [PATCH v2 3/3] p7821: fix instructions for testing with threads Message-ID: URL: https://gitlist.dev/e/abd146b7c2a62aaef5c22269cff155387f33fe32.1744481732.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Philippe Blain In 7b31b55db1 (perf: amend the grep tests to test grep.threads, 2017-12-29), p7821 was tweaked to test the performance of 'git grep' under different number of threads. These tests are run if GIT_PERF_GREP_THREADS is set to a list of thread numbers, but the comment at the top of the file instead mentions GIT_PERF_7821_THREADS. Fix the comment. Signed-off-by: Philippe Blain --- t/perf/p7821-grep-engines-fixed.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/perf/p7821-grep-engines-fixed.sh b/t/perf/p7821-grep-engines-fixed.sh index 1d126c7b039..66bec284e3b 100755 --- a/t/perf/p7821-grep-engines-fixed.sh +++ b/t/perf/p7821-grep-engines-fixed.sh @@ -7,7 +7,7 @@ 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' +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. " -- gitgitgadget ``` ## Philippe Blain, 2025-04-13 02:50 Subject: Re: [PATCH v2 0/3] Two perf test fixes Message-ID: <54864a66-c399-ac2e-e223-affd6a493989@gmail.com> URL: https://gitlist.dev/e/54864a66-c399-ac2e-e223-affd6a493989%40gmail.com In-Reply-To: ``` Sorry, I forgot to mention that this v2 only adds a third commit with a small comment fix. Philippe. Le 2025-04-12 à 14:15, Philippe Blain via GitGitGadget a écrit : > Here a two fixes for failures I noticed while running the perf tests. > > Philippe Blain (3): > p7821: fix test_perf invocation for prereqs > p9210: fix 'scalar clone' when running from a detached HEAD > p7821: fix instructions for testing with threads > > t/perf/p7821-grep-engines-fixed.sh | 6 +++--- > t/perf/p9210-scalar.sh | 3 ++- > 2 files changed, 5 insertions(+), 4 deletions(-) > > > base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1936%2Fphil-blain%2Fperf-test-fixes-v2 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1936/phil-blain/perf-test-fixes-v2 > Pull-Request: https://github.com/git/git/pull/1936 > > Range-diff vs v1: > > 1: 41a093d570a = 1: 41a093d570a p7821: fix test_perf invocation for prereqs > 2: 1092c32609f = 2: 1092c32609f p9210: fix 'scalar clone' when running from a detached HEAD > -: ----------- > 3: abd146b7c2a p7821: fix instructions for testing with threads > ``` ## Philippe Blain, 2025-04-13 19:00 Subject: Re: [PATCH 1/2] p7821: fix test_perf invocation for prereqs Message-ID: <90b3f122-8b94-3b45-08e0-32af95f9cea5@gmail.com> URL: https://gitlist.dev/e/90b3f122-8b94-3b45-08e0-32af95f9cea5%40gmail.com In-Reply-To: ``` Hi Patrick, Le 2025-03-31 à 03:27, Patrick Steinhardt a écrit : > On Fri, Mar 28, 2025 at 05:07:48PM +0000, Philippe Blain via GitGitGadget wrote: >> diff --git a/t/perf/p7821-grep-engines-fixed.sh b/t/perf/p7821-grep-engines-fixed.sh >> index 61e41b82cff..1d126c7b039 100755 >> --- a/t/perf/p7821-grep-engines-fixed.sh >> +++ b/t/perf/p7821-grep-engines-fixed.sh >> @@ -33,13 +33,13 @@ do >> fi >> if ! test_have_prereq PERF_GREP_ENGINES_THREADS >> then >> - test_perf $prereq "$engine grep$GIT_PERF_7821_GREP_OPTS $pattern" " >> + test_perf "$engine grep$GIT_PERF_7821_GREP_OPTS $pattern" --prereq "$prereq" " >> 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" " >> + test_perf "$engine grep$GIT_PERF_7821_GREP_OPTS $pattern with $threads threads" --prereq "PTHREADS,$prereq" " >> git -c grep.patternType=$engine -c grep.threads=$threads grep$GIT_PERF_7821_GREP_OPTS $pattern >'out.$engine.$threads' || : >> " >> done > > "$prereq" can be empty here as it depends on which regexp engine we're > using. The second case you adapt already looked weird before because we > potentially checked for "PTHREADS,", Indeed, the reason why the second case did not fail even when built with PCRE (which would not fail the first 'test_perf' since '$prereq' would be empty) is that this second test (in fact the whole loop) is only reached if GIT_PERF_GREP_THREADS is set in the environment, which sets the PERF_GREP_ENGINES_THREADS prereq. So just running 'make perf' or './p7821-*' would not enter this part of the test. > but the first case was correct > before but is now potentially checking for the empty prerequisite. Does > that actually work as expected? Yes, it was correct when built with PCRE, but not without, as then $prereq would not be empty. I did check that it works correctly before sending the patch, both when built with and without PCRE. Thank you for the review and also checking that the patch works correctly. I just checked 'test_skip' which is the function that checks the prereq and indeed and empty 'test_prereq' is treated as no prereq. Cheers, Philippe. ``` ## Junio C Hamano, 2025-04-14 16:04 Subject: Re: [PATCH v2 0/3] Two perf test fixes Message-ID: URL: https://gitlist.dev/e/xmqqlds2rapw.fsf%40gitster.g In-Reply-To: <54864a66-c399-ac2e-e223-affd6a493989@gmail.com> ``` Philippe Blain writes: > Sorry, I forgot to mention that this v2 only adds a third commit > with a small comment fix. Thanks! ``` ## Junio C Hamano, 2025-04-14 21:48 Subject: Re: [PATCH v2 3/3] p7821: fix instructions for testing with threads Message-ID: URL: https://gitlist.dev/e/xmqq7c3mpg7m.fsf%40gitster.g In-Reply-To: ``` "Philippe Blain via GitGitGadget" writes: > From: Philippe Blain > > In 7b31b55db1 (perf: amend the grep tests to test grep.threads, > 2017-12-29), p7821 was tweaked to test the performance of 'git grep' > under different number of threads. These tests are run if > GIT_PERF_GREP_THREADS is set to a list of thread numbers, but the > comment at the top of the file instead mentions GIT_PERF_7821_THREADS. > Fix the comment. > > Signed-off-by: Philippe Blain > --- > t/perf/p7821-grep-engines-fixed.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Thanks. ```