Two perf test fixes

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

From: Philippe Blain <[email protected]>

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.

---
 t/perf/p7821-grep-engines-fixed.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 61e41b82cff..1d126c7b039 100755
@@ -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

From: Philippe Blain <[email protected]>

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.

---
 t/perf/p9210-scalar.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 265f7cd1fe2..56b075e906e 100755
@@ -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

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

From: Philippe Blain <[email protected]>

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.

---
 t/perf/p7821-grep-engines-fixed.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 61e41b82cff..1d126c7b039 100755
@@ -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

From: Philippe Blain <[email protected]>

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.

---
 t/perf/p9210-scalar.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 265f7cd1fe2..56b075e906e 100755
@@ -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

From: Philippe Blain <[email protected]>

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.

---
 t/perf/p7821-grep-engines-fixed.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 1d126c7b039..66bec284e3b 100755
@@ -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

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 :

Hi Patrick,

Le 2025-03-31 à 03:27, Patrick Steinhardt a écrit :

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.

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.

Thanks!

Thanks.