Re: [PATCH v4 00/13] core.fsyncmethod: add 'batch' mode for faster fsyncing of multiple objects

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Mar 29 2022, Neeraj K. Singh via GitGitGadget wrote:

> V4 changes:
>
>  * Make ODB transactions nestable.
>  * Add an ODB transaction around writing out the cached tree.
>  * Change update-index to use a more straightforward way of managing ODB
>    transactions.
>  * Fix missing 'local's in lib-unique-files
>  * Add a per-iteration setup mechanism to test_perf.
>  * Fix camelCasing in warning message.

I haven't looked at the bulk of this in any detail, but:

>  10:  b99b32a469c ! 12:  fdf90d45f52 core.fsyncmethod: performance tests for add and stash
>      @@ t/perf/p3700-add.sh (new)
>       +# core.fsyncMethod=batch mode, which is why we are testing different values
>       +# of that setting explicitly and creating a lot of unique objects.
>       +
>      -+test_description="Tests performance of add"
>      ++test_description="Tests performance of adding things to the object database"

Now having both tests for "add" and "stash" in a test named p3700-add.sh
isn't better, the rest of the perf tests are split up by command,
perhaps just add a helper library and have both use it?

And re the unaddressed feedback I ad of "why the random data"
inhttps://lore.kernel.org/git/220326.86o81sk9ao.gmgdl@evledraar.gmail.com/
I tried patching it on top to do what I suggested there, allowing us to
run these against any arbitrary repository and came up with this:

diff --git a/t/perf/p3700-add.sh b/t/perf/p3700-add.sh
index ef6024f9897..60abd5ee076 100755
--- a/t/perf/p3700-add.sh
+++ b/t/perf/p3700-add.sh
@@ -13,47 +13,26 @@ export GIT_TEST_FSYNC
 
 . ./perf-lib.sh
 
-. $TEST_DIRECTORY/lib-unique-files.sh
-
-test_perf_fresh_repo
+test_perf_default_repo
 test_checkout_worktree
 
-dir_count=10
-files_per_dir=50
-total_files=$((dir_count * files_per_dir))
-
-for mode in false true batch
+for cfg in \
+	'-c core.fsync=-loose-object -c core.fsyncmethod=fsync' \
+	'-c core.fsync=loose-object -c core.fsyncmethod=fsync' \
+	'-c core.fsync=loose-object -c core.fsyncmethod=batch' \
+	'-c core.fsyncmethod=batch'
 do
-	case $mode in
-	false)
-		FSYNC_CONFIG='-c core.fsync=-loose-object -c core.fsyncmethod=fsync'
-		;;
-	true)
-		FSYNC_CONFIG='-c core.fsync=loose-object -c core.fsyncmethod=fsync'
-		;;
-	batch)
-		FSYNC_CONFIG='-c core.fsync=loose-object -c core.fsyncmethod=batch'
-		;;
-	esac
-
-	test_perf "add $total_files files (object_fsyncing=$mode)" \
-		--setup "
-		(rm -rf .git || 1) &&
-		git init &&
-		test_create_unique_files $dir_count $files_per_dir files_$mode
-	" "
-		git $FSYNC_CONFIG add files_$mode
-	"
-
-	test_perf "stash $total_files files (object_fsyncing=$mode)" \
-		--setup "
-		(rm -rf .git || 1) &&
-		git init &&
-		test_commit first &&
-		test_create_unique_files $dir_count $files_per_dir stash_files_$mode
-	" "
-		git $FSYNC_CONFIG stash push -u -- stash_files_$mode
-	"
+	test_perf "'git add' with '$cfg'" \
+		--setup '
+			mv -v .git .git.old &&
+			git init .
+		' \
+		--cleanup '
+			rm -rf .git &&
+			mv .git.old .git
+		' '
+		git $cfg add -f -- ":!.git.old/"
+	'
 done
 
 test_done
diff --git a/t/perf/p3900-stash.sh b/t/perf/p3900-stash.sh
new file mode 100755
index 00000000000..12c489069ba
--- /dev/null
+++ b/t/perf/p3900-stash.sh
@@ -0,0 +1,34 @@
+#!/bin/sh
+
+test_description='performance of "git stash" with different fsync settings'
+
+# Fsync is normally turned off for the test suite.
+GIT_TEST_FSYNC=1
+export GIT_TEST_FSYNC
+
+. ./perf-lib.sh
+
+test_perf_default_repo
+test_checkout_worktree
+
+for cfg in \
+	'-c core.fsync=-loose-object -c core.fsyncmethod=fsync' \
+	'-c core.fsync=loose-object -c core.fsyncmethod=fsync' \
+	'-c core.fsync=loose-object -c core.fsyncmethod=batch' \
+	'-c core.fsyncmethod=batch'
+do
+	test_perf "'stash push -u' with '$cfg'" \
+		--setup '
+			mv -v .git .git.old &&
+			git init . &&
+			test_commit dummy
+		' \
+		--cleanup '
+			rm -rf .git &&
+			mv .git.old .git
+		' '
+		git $cfg stash push -a -u ":!.git.old/" ":!test*" "."
+	'
+done
+
+test_done
diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index a935ad622d3..24a5108f234 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -194,6 +194,7 @@ test_wrapper_ () {
 	test_start_
 	test_prereq=
 	test_perf_setup_=
+	test_perf_cleanup_=
 	while test $# != 0
 	do
 		case $1 in
@@ -205,6 +206,10 @@ test_wrapper_ () {
 			test_perf_setup_=$2
 			shift
 			;;
+		--cleanup)
+			test_perf_cleanup_=$2
+			shift
+			;;
 		*)
 			break
 			;;
@@ -214,6 +219,7 @@ test_wrapper_ () {
 	test "$#" = 1 || BUG "test_wrapper_ needs 2 positional parameters"
 	export test_prereq
 	export test_perf_setup_
+	export test_perf_cleanup_
 	if ! test_skip "$test_title_" "$@"
 	then
 		base=$(basename "$0" .sh)
@@ -256,6 +262,16 @@ test_perf_ () {
 			test_failure_ "$@"
 			break
 		fi
+		if test -n "$test_perf_cleanup_"
+		then
+			say >&3 "cleanup: $test_perf_cleanup_"
+			if ! test_eval_ $test_perf_cleanup_
+			then
+				test_failure_ "$test_perf_cleanup_"
+				break
+			fi
+
+		fi
 	done
 	if test -z "$verbose"; then
 		echo " ok"


Here it is against Cor.git (a random small-ish repo I had laying around):
	
	$ GIT_SKIP_TESTS='p3[79]00.[12]' GIT_PERF_MAKE_OPTS='CFLAGS=-O3' GIT_PERF_REPO=~/g/Cor/ ./run origin/master HEAD -- p3900-stash.sh
	=== Building abf474a5dd901f28013c52155411a48fd4c09922 (origin/master) ===
	    GEN git-add--interactive
	    GEN git-archimport
	    GEN git-cvsexportcommit
	    GEN git-cvsimport
	    GEN git-cvsserver
	    GEN git-send-email
	    GEN git-svn
	    GEN git-p4
	    SUBDIR templates
	=== Running 1 tests in /home/avar/g/git/t/perf/build/abf474a5dd901f28013c52155411a48fd4c09922/bin-wrappers ===
	ok 1 # skip 'stash push -u' with '-c core.fsync=-loose-object -c core.fsyncmethod=fsync' (GIT_SKIP_TESTS)
	ok 2 # skip 'stash push -u' with '-c core.fsync=loose-object -c core.fsyncmethod=fsync' (GIT_SKIP_TESTS)
	perf 3 - 'stash push -u' with '-c core.fsync=loose-object -c core.fsyncmethod=batch': 1 2 3 ok
	perf 4 - 'stash push -u' with '-c core.fsyncmethod=batch': 1 2 3 ok
	# passed all 4 test(s)
	1..4
	=== Building ecda9c2b029e35d239e369b875b245f45fd2a097 (HEAD) ===
	    GEN git-add--interactive
	    GEN git-archimport
	    GEN git-cvsexportcommit
	    GEN git-cvsimport
	    GEN git-cvsserver
	    GEN git-send-email
	    GEN git-svn
	    GEN git-p4
	    SUBDIR templates
	=== Running 1 tests in /home/avar/g/git/t/perf/build/ecda9c2b029e35d239e369b875b245f45fd2a097/bin-wrappers ===
	ok 1 # skip 'stash push -u' with '-c core.fsync=-loose-object -c core.fsyncmethod=fsync' (GIT_SKIP_TESTS)
	ok 2 # skip 'stash push -u' with '-c core.fsync=loose-object -c core.fsyncmethod=fsync' (GIT_SKIP_TESTS)
	perf 3 - 'stash push -u' with '-c core.fsync=loose-object -c core.fsyncmethod=batch': 1 2 3 ok
	perf 4 - 'stash push -u' with '-c core.fsyncmethod=batch': 1 2 3 ok
	# passed all 4 test(s)
	1..4
	Test       origin/master     HEAD
	---------------------------------------------------
	3900.3:    0.03(0.00+0.00)   0.02(0.00+0.00) -33.3%
	3900.4:    0.02(0.00+0.00)   0.03(0.00+0.00) +50.0%