[PATCH 0/2] Two perf test fixes

[PATCH 0/2] Two perf test fixes

[Philippe Blain via GitGitGadget]

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

[PATCH 1/2] p7821: fix test_perf invocation for prereqs

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

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 <levraiphilippeblain@gmail.com>
---
 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

Re: [PATCH 1/2] p7821: fix test_perf invocation for prereqs

[Patrick Steinhardt]

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

Re: [PATCH 1/2] p7821: fix test_perf invocation for prereqs

[Philippe Blain]

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.

[PATCH 2/2] p9210: fix 'scalar clone' when running from a detached HEAD

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

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 <levraiphilippeblain@gmail.com>
---
 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

Re: [PATCH 2/2] p9210: fix 'scalar clone' when running from a detached HEAD

[Patrick Steinhardt]

On Fri, Mar 28, 2025 at 05:07:49PM +0000, Philippe Blain via GitGitGadget wrote:
> From: Philippe Blain <levraiphilippeblain@gmail.com>
> 
> 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 <levraiphilippeblain@gmail.com>
> ---
>  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

[PATCH v2 0/3] Two perf test fixes

[Philippe Blain via GitGitGadget]

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

[PATCH v2 1/3] p7821: fix test_perf invocation for prereqs

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

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 <levraiphilippeblain@gmail.com>
---
 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

[PATCH v2 2/3] p9210: fix 'scalar clone' when running from a detached HEAD

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

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 <levraiphilippeblain@gmail.com>
---
 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

[PATCH v2 3/3] p7821: fix instructions for testing with threads

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

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 <levraiphilippeblain@gmail.com>
---
 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

Re: [PATCH v2 3/3] p7821: fix instructions for testing with threads

[Junio C Hamano]

"Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Philippe Blain <levraiphilippeblain@gmail.com>
>
> 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 <levraiphilippeblain@gmail.com>
> ---
>  t/perf/p7821-grep-engines-fixed.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Thanks.

Re: [PATCH v2 0/3] Two perf test fixes

[Philippe Blain]

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
> 

Re: [PATCH v2 0/3] Two perf test fixes

[Junio C Hamano]

Philippe Blain <levraiphilippeblain@gmail.com> writes:

> Sorry, I forgot to mention that this v2 only adds a third commit
> with a small comment fix.

Thanks!