[7/8] t/perf: add 'GIT_PERF_USE_SCALAR' run option

Commit Message

Victoria Dye Aug. 31, 2022, 4:02 p.m. UTC

From: Victoria Dye <[e-mail]>

Add a 'GIT_PERF_USE_SCALAR' environment variable (and corresponding perf
config 'useScalar') to register a repository created with any of:

* test_perf_fresh_repo
* test_perf_default_repo
* test_perf_large_repo

as a Scalar enlistment. This is intended to allow a developer to test the
impact of Scalar on already-defined performance scenarios.

Suggested-by: Derrick Stolee <[e-mail]>
Signed-off-by: Victoria Dye <[e-mail]>
---
 t/perf/README      |  4 ++++
 t/perf/perf-lib.sh | 13 ++++++++++++-
 t/perf/run         |  3 +++
 3 files changed, 19 insertions(+), 1 deletion(-)

Comments

Johannes Schindelin Sept. 1, 2022, 9:43 a.m. UTC | #1

Hi Victoria,

On Wed, 31 Aug 2022, Victoria Dye via GitGitGadget wrote:

> From: Victoria Dye <[e-mail]>
>
> Add a 'GIT_PERF_USE_SCALAR' environment variable (and corresponding perf
> config 'useScalar') to register a repository created with any of:
>
> * test_perf_fresh_repo
> * test_perf_default_repo
> * test_perf_large_repo
>
> as a Scalar enlistment. This is intended to allow a developer to test the
> impact of Scalar on already-defined performance scenarios.

Great idea!

> [...]
> @@ -130,7 +137,11 @@ test_perf_fresh_repo () {
>  	"$MODERN_GIT" init -q "$repo" &&
>  	(
>  		cd "$repo" &&
> -		test_perf_do_repo_symlink_config_
> +		test_perf_do_repo_symlink_config_ &&
> +		if test_bool_env "$GIT_PERF_USE_SCALAR" false
> +		then
> +			"$MODERN_SCALAR" register

Do we need to unregister anything here? My guess is that no, the "global"
config we're using in tests is "$TRASH_DIRECTORY/.gitconfig", and the side
effect of scheduling the maintenance task won't matter in practice. But I
might have missed something and we may want to have an explicit
`unregister` step.

What's your take on this?

Ciao,
Dscho

> +		fi
>  	)
>  }

Victoria Dye Sept. 2, 2022, 4 a.m. UTC | #2

Johannes Schindelin wrote:
> Hi Victoria,
> 
> On Wed, 31 Aug 2022, Victoria Dye via GitGitGadget wrote:
> 
>> From: Victoria Dye <[e-mail]>
>>
>> Add a 'GIT_PERF_USE_SCALAR' environment variable (and corresponding perf
>> config 'useScalar') to register a repository created with any of:
>>
>> * test_perf_fresh_repo
>> * test_perf_default_repo
>> * test_perf_large_repo
>>
>> as a Scalar enlistment. This is intended to allow a developer to test the
>> impact of Scalar on already-defined performance scenarios.
> 
> Great idea!
> 
>> [...]
>> @@ -130,7 +137,11 @@ test_perf_fresh_repo () {
>>  	"$MODERN_GIT" init -q "$repo" &&
>>  	(
>>  		cd "$repo" &&
>> -		test_perf_do_repo_symlink_config_
>> +		test_perf_do_repo_symlink_config_ &&
>> +		if test_bool_env "$GIT_PERF_USE_SCALAR" false
>> +		then
>> +			"$MODERN_SCALAR" register
> 
> Do we need to unregister anything here? My guess is that no, the "global"
> config we're using in tests is "$TRASH_DIRECTORY/.gitconfig", and the side
> effect of scheduling the maintenance task won't matter in practice. But I
> might have missed something and we may want to have an explicit
> `unregister` step.
> 
> What's your take on this?

As you guessed, a '.gitconfig' is created in the trash directory of each
test containing the Scalar registration and I haven't seen any issues
arising from the scheduled maintenance, so I don't think an 'unregister' is
necessary. However, while verifying that, I noticed that the registration
wasn't happening *at all* because 'test_bool_env' is currently being used
incorrectly. The fix is straightforward - I'll make sure to correct it in
the next version.

Thanks!

> 
> Ciao,
> Dscho
> 
>> +		fi
>>  	)
>>  }

Johannes Schindelin Sept. 2, 2022, 9:17 a.m. UTC | #3

Hi Victoria,

On Thu, 1 Sep 2022, Victoria Dye wrote:

> Johannes Schindelin wrote:
> >
> > On Wed, 31 Aug 2022, Victoria Dye via GitGitGadget wrote:
> >
> >> From: Victoria Dye <[e-mail]>
> >>
> >> Add a 'GIT_PERF_USE_SCALAR' environment variable (and corresponding perf
> >> config 'useScalar') to register a repository created with any of:
> >>
> >> * test_perf_fresh_repo
> >> * test_perf_default_repo
> >> * test_perf_large_repo
> >>
> >> as a Scalar enlistment. This is intended to allow a developer to test the
> >> impact of Scalar on already-defined performance scenarios.
> >
> > Great idea!
> >
> >> [...]
> >> @@ -130,7 +137,11 @@ test_perf_fresh_repo () {
> >>  	"$MODERN_GIT" init -q "$repo" &&
> >>  	(
> >>  		cd "$repo" &&
> >> -		test_perf_do_repo_symlink_config_
> >> +		test_perf_do_repo_symlink_config_ &&
> >> +		if test_bool_env "$GIT_PERF_USE_SCALAR" false
> >> +		then
> >> +			"$MODERN_SCALAR" register
> >
> > Do we need to unregister anything here? My guess is that no, the "global"
> > config we're using in tests is "$TRASH_DIRECTORY/.gitconfig", and the side
> > effect of scheduling the maintenance task won't matter in practice. But I
> > might have missed something and we may want to have an explicit
> > `unregister` step.
> >
> > What's your take on this?
>
> As you guessed, a '.gitconfig' is created in the trash directory of each
> test containing the Scalar registration and I haven't seen any issues
> arising from the scheduled maintenance, so I don't think an 'unregister' is
> necessary.

Thank you for checking!

> However, while verifying that, I noticed that the registration wasn't
> happening *at all* because 'test_bool_env' is currently being used
> incorrectly. The fix is straightforward - I'll make sure to correct it
> in the next version.

Oh, great, then my feedback was at least _somewhat_ helpful... ;-)

Ciao,
Dscho

Patch

diff --git a/t/perf/README b/t/perf/README
index fb9127a66f7..8f217d7be7d 100644
--- a/t/perf/README
+++ b/t/perf/README
@@ -95,6 +95,10 @@  You can set the following variables (also in your config.mak):
 	Git (e.g., performance of index-pack as the number of threads
 	changes). These can be enabled with GIT_PERF_EXTRA.
 
+    GIT_PERF_USE_SCALAR
+	Boolean indicating whether to register test repo(s) with Scalar
+	before executing tests.
+
 You can also pass the options taken by ordinary git tests; the most
 useful one is:
 
diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index 27c28017921..b960b0f6301 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -49,6 +49,9 @@  export TEST_DIRECTORY TRASH_DIRECTORY GIT_BUILD_DIR GIT_TEST_CMP
 MODERN_GIT=$GIT_BUILD_DIR/bin-wrappers/git
 export MODERN_GIT
 
+MODERN_SCALAR=$GIT_BUILD_DIR/bin-wrappers/scalar
+export MODERN_SCALAR
+
 perf_results_dir=$TEST_RESULTS_DIR
 test -n "$GIT_PERF_SUBSECTION" && perf_results_dir="$perf_results_dir/$GIT_PERF_SUBSECTION"
 mkdir -p "$perf_results_dir"
@@ -120,6 +123,10 @@  test_perf_create_repo_from () {
 			# status" due to a locked index. Since we have
 			# a copy it's fine to remove the lock.
 			rm .git/index.lock
+		fi &&
+		if test_bool_env "$GIT_PERF_USE_SCALAR" false
+		then
+			"$MODERN_SCALAR" register
 		fi
 	) || error "failed to copy repository '$source' to '$repo'"
 }
@@ -130,7 +137,11 @@  test_perf_fresh_repo () {
 	"$MODERN_GIT" init -q "$repo" &&
 	(
 		cd "$repo" &&
-		test_perf_do_repo_symlink_config_
+		test_perf_do_repo_symlink_config_ &&
+		if test_bool_env "$GIT_PERF_USE_SCALAR" false
+		then
+			"$MODERN_SCALAR" register
+		fi
 	)
 }
 
diff --git a/t/perf/run b/t/perf/run
index 55219aa4056..33da4d2aba2 100755
--- a/t/perf/run
+++ b/t/perf/run
@@ -171,6 +171,9 @@  run_subsection () {
 	get_var_from_env_or_config "GIT_PERF_MAKE_COMMAND" "perf" "makeCommand"
 	get_var_from_env_or_config "GIT_PERF_MAKE_OPTS" "perf" "makeOpts"
 
+	get_var_from_env_or_config "GIT_PERF_USE_SCALAR" "perf" "useScalar" "--bool"
+	export GIT_PERF_USE_SCALAR
+
 	get_var_from_env_or_config "GIT_PERF_REPO_NAME" "perf" "repoName"
 	export GIT_PERF_REPO_NAME