core.fsyncmethod: add 'batch' mode for faster fsyncing of multiple objects

From: Neeraj Singh <[email protected]>

Several tests use awk to parse OIDs from the output of 'git ls-files --stage' and 'git ls-tree'. Introduce helpers to centralize these uses of awk.

Update t5317-pack-objects-filter-objects.sh to use the new ls-files helper so that it has some usages to review. Other updates are left for the future.

---
 t/t5317-pack-objects-filter-objects.sh | 91 +++++++++++++-------------
 t/test-lib-functions.sh                | 10 +++
 2 files changed, 54 insertions(+), 47 deletions(-)

index 33b740ce628..bb633c9b099 100755
@@ -10,9 +10,6 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 # Test blob:none filter.
 
 test_expect_success 'setup r1' '
-	echo "{print \$1}" >print_1.awk &&
-	echo "{print \$2}" >print_2.awk &&
-
 	git init r1 &&
 	for n in 1 2 3 4 5
 	do

@@ -22,10 +19,13 @@ test_expect_success 'setup r1' '
 	done
 '
 
+parse_verify_pack_blob_oid () {
+	awk '{print $1}' -
+}
+
 test_expect_success 'verify blob count in normal packfile' '
-	git -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 \
-		>ls_files_result &&
-	awk -f print_2.awk ls_files_result |
+	git -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 |
+	test_parse_ls_files_stage_oids |
 	sort >expected &&
 
 	git -C r1 pack-objects --revs --stdout >all.pack <<-EOF &&

@@ -35,7 +35,7 @@ test_expect_success 'verify blob count in normal packfile' '
 
 	git -C r1 verify-pack -v ../all.pack >verify_result &&
 	grep blob verify_result |
-	awk -f print_1.awk |
+	parse_verify_pack_blob_oid |
 	sort >observed &&
 
 	test_cmp expected observed

@@ -54,12 +54,12 @@ test_expect_success 'verify blob:none packfile has no blobs' '
 test_expect_success 'verify normal and blob:none packfiles have same commits/trees' '
 	git -C r1 verify-pack -v ../all.pack >verify_result &&
 	grep -E "commit|tree" verify_result |
-	awk -f print_1.awk |
+	parse_verify_pack_blob_oid |
 	sort >expected &&
 
 	git -C r1 verify-pack -v ../filter.pack >verify_result &&
 	grep -E "commit|tree" verify_result |
-	awk -f print_1.awk |
+	parse_verify_pack_blob_oid |
 	sort >observed &&
 
 	test_cmp expected observed

@@ -123,8 +123,8 @@ test_expect_success 'setup r2' '
 '
 
 test_expect_success 'verify blob count in normal packfile' '
-	git -C r2 ls-files -s large.1000 large.10000 >ls_files_result &&
-	awk -f print_2.awk ls_files_result |
+	git -C r2 ls-files -s large.1000 large.10000 |
+	test_parse_ls_files_stage_oids |
 	sort >expected &&
 
 	git -C r2 pack-objects --revs --stdout >all.pack <<-EOF &&

@@ -134,7 +134,7 @@ test_expect_success 'verify blob count in normal packfile' '
 
 	git -C r2 verify-pack -v ../all.pack >verify_result &&
 	grep blob verify_result |
-	awk -f print_1.awk |
+	parse_verify_pack_blob_oid |
 	sort >observed &&
 
 	test_cmp expected observed

@@ -161,8 +161,8 @@ test_expect_success 'verify blob:limit=1000' '
 '
 
 test_expect_success 'verify blob:limit=1001' '
-	git -C r2 ls-files -s large.1000 >ls_files_result &&
-	awk -f print_2.awk ls_files_result |
+	git -C r2 ls-files -s large.1000 |
+	test_parse_ls_files_stage_oids |
 	sort >expected &&
 
 	git -C r2 pack-objects --revs --stdout --filter=blob:limit=1001 >filter.pack <<-EOF &&

@@ -172,15 +172,15 @@ test_expect_success 'verify blob:limit=1001' '
 
 	git -C r2 verify-pack -v ../filter.pack >verify_result &&
 	grep blob verify_result |
-	awk -f print_1.awk |
+	parse_verify_pack_blob_oid |
 	sort >observed &&
 
 	test_cmp expected observed
 '
 
 test_expect_success 'verify blob:limit=10001' '
-	git -C r2 ls-files -s large.1000 large.10000 >ls_files_result &&
-	awk -f print_2.awk ls_files_result |
+	git -C r2 ls-files -s large.1000 large.10000 |
+	test_parse_ls_files_stage_oids |
 	sort >expected &&
 
 	git -C r2 pack-objects --revs --stdout --filter=blob:limit=10001 >filter.pack <<-EOF &&

@@ -190,15 +190,15 @@ test_expect_success 'verify blob:limit=10001' '
 
 	git -C r2 verify-pack -v ../filter.pack >verify_result &&
 	grep blob verify_result |
-	awk -f print_1.awk |
+	parse_verify_pack_blob_oid |
 	sort >observed &&
 
 	test_cmp expected observed
 '
 
 test_expect_success 'verify blob:limit=1k' '
-	git -C r2 ls-files -s large.1000 >ls_files_result &&
-	awk -f print_2.awk ls_files_result |
+	git -C r2 ls-files -s large.1000 |
+	test_parse_ls_files_stage_oids |
 	sort >expected &&
 
 	git -C r2 pack-objects --revs --stdout --filter=blob:limit=1k >filter.pack <<-EOF &&

@@ -208,15 +208,15 @@ test_expect_success 'verify blob:limit=1k' '
 
 	git -C r2 verify-pack -v ../filter.pack >verify_result &&
 	grep blob verify_result |
-	awk -f print_1.awk |
+	parse_verify_pack_blob_oid |
 	sort >observed &&
 
 	test_cmp expected observed
 '
 
 test_expect_success 'verify explicitly specifying oversized blob in input' '
-	git -C r2 ls-files -s large.1000 large.10000 >ls_files_result &&
-	awk -f print_2.awk ls_files_result |
+	git -C r2 ls-files -s large.1000 large.10000 |
+	test_parse_ls_files_stage_oids |
 	sort >expected &&
 
 	echo HEAD >objects &&

@@ -226,15 +226,15 @@ test_expect_success 'verify explicitly specifying oversized blob in input' '
 
 	git -C r2 verify-pack -v ../filter.pack >verify_result &&
 	grep blob verify_result |
-	awk -f print_1.awk |
+	parse_verify_pack_blob_oid |
 	sort >observed &&
 
 	test_cmp expected observed
 '
 
 test_expect_success 'verify blob:limit=1m' '
-	git -C r2 ls-files -s large.1000 large.10000 >ls_files_result &&
-	awk -f print_2.awk ls_files_result |
+	git -C r2 ls-files -s large.1000 large.10000 |
+	test_parse_ls_files_stage_oids |
 	sort >expected &&
 
 	git -C r2 pack-objects --revs --stdout --filter=blob:limit=1m >filter.pack <<-EOF &&

@@ -244,7 +244,7 @@ test_expect_success 'verify blob:limit=1m' '
 
 	git -C r2 verify-pack -v ../filter.pack >verify_result &&
 	grep blob verify_result |
-	awk -f print_1.awk |
+	parse_verify_pack_blob_oid |
 	sort >observed &&
 
 	test_cmp expected observed

@@ -253,12 +253,12 @@ test_expect_success 'verify blob:limit=1m' '
 test_expect_success 'verify normal and blob:limit packfiles have same commits/trees' '
 	git -C r2 verify-pack -v ../all.pack >verify_result &&
 	grep -E "commit|tree" verify_result |
-	awk -f print_1.awk |
+	parse_verify_pack_blob_oid |
 	sort >expected &&
 
 	git -C r2 verify-pack -v ../filter.pack >verify_result &&
 	grep -E "commit|tree" verify_result |
-	awk -f print_1.awk |
+	parse_verify_pack_blob_oid |
 	sort >observed &&
 
 	test_cmp expected observed

@@ -289,9 +289,8 @@ test_expect_success 'setup r3' '
 '
 
 test_expect_success 'verify blob count in normal packfile' '
-	git -C r3 ls-files -s sparse1 sparse2 dir1/sparse1 dir1/sparse2 \
-		>ls_files_result &&
-	awk -f print_2.awk ls_files_result |
+	git -C r3 ls-files -s sparse1 sparse2 dir1/sparse1 dir1/sparse2 |
+	test_parse_ls_files_stage_oids |
 	sort >expected &&
 
 	git -C r3 pack-objects --revs --stdout >all.pack <<-EOF &&

@@ -301,7 +300,7 @@ test_expect_success 'verify blob count in normal packfile' '
 
 	git -C r3 verify-pack -v ../all.pack >verify_result &&
 	grep blob verify_result |
-	awk -f print_1.awk |
+	parse_verify_pack_blob_oid |
 	sort >observed &&
 
 	test_cmp expected observed

@@ -342,9 +341,8 @@ test_expect_success 'setup r4' '
 '
 
 test_expect_success 'verify blob count in normal packfile' '
-	git -C r4 ls-files -s pattern sparse1 sparse2 dir1/sparse1 dir1/sparse2 \
-		>ls_files_result &&
-	awk -f print_2.awk ls_files_result |
+	git -C r4 ls-files -s pattern sparse1 sparse2 dir1/sparse1 dir1/sparse2 |
+	test_parse_ls_files_stage_oids |
 	sort >expected &&
 
 	git -C r4 pack-objects --revs --stdout >all.pack <<-EOF &&

@@ -354,19 +352,19 @@ test_expect_success 'verify blob count in normal packfile' '
 
 	git -C r4 verify-pack -v ../all.pack >verify_result &&
 	grep blob verify_result |
-	awk -f print_1.awk |
+	parse_verify_pack_blob_oid |
 	sort >observed &&
 
 	test_cmp expected observed
 '
 
 test_expect_success 'verify sparse:oid=OID' '
-	git -C r4 ls-files -s dir1/sparse1 dir1/sparse2 >ls_files_result &&
-	awk -f print_2.awk ls_files_result |
+	git -C r4 ls-files -s dir1/sparse1 dir1/sparse2 |
+	test_parse_ls_files_stage_oids |
 	sort >expected &&
 
 	git -C r4 ls-files -s pattern >staged &&
-	oid=$(awk -f print_2.awk staged) &&
+	oid=$(test_parse_ls_files_stage_oids <staged) &&
 	git -C r4 pack-objects --revs --stdout --filter=sparse:oid=$oid >filter.pack <<-EOF &&
 	HEAD
 	EOF

@@ -374,15 +372,15 @@ test_expect_success 'verify sparse:oid=OID' '
 
 	git -C r4 verify-pack -v ../filter.pack >verify_result &&
 	grep blob verify_result |
-	awk -f print_1.awk |
+	parse_verify_pack_blob_oid |
 	sort >observed &&
 
 	test_cmp expected observed
 '
 
 test_expect_success 'verify sparse:oid=oid-ish' '
-	git -C r4 ls-files -s dir1/sparse1 dir1/sparse2 >ls_files_result &&
-	awk -f print_2.awk ls_files_result |
+	git -C r4 ls-files -s dir1/sparse1 dir1/sparse2 |
+	test_parse_ls_files_stage_oids |
 	sort >expected &&
 
 	git -C r4 pack-objects --revs --stdout --filter=sparse:oid=main:pattern >filter.pack <<-EOF &&

@@ -392,7 +390,7 @@ test_expect_success 'verify sparse:oid=oid-ish' '
 
 	git -C r4 verify-pack -v ../filter.pack >verify_result &&
 	grep blob verify_result |
-	awk -f print_1.awk |
+	parse_verify_pack_blob_oid |
 	sort >observed &&
 
 	test_cmp expected observed

@@ -402,9 +400,8 @@ test_expect_success 'verify sparse:oid=oid-ish' '
 # This models previously omitted objects that we did not receive.
 
 test_expect_success 'setup r1 - delete loose blobs' '
-	git -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 \
-		>ls_files_result &&
-	awk -f print_2.awk ls_files_result |
+	git -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 |
+	test_parse_ls_files_stage_oids |
 	sort >expected &&
 
 	for id in `cat expected | sed "s|..|&/|"`

index a027f0c409e..e6011409e2f 100644
@@ -1782,6 +1782,16 @@ test_oid_to_path () {
 	echo "${1%$basename}/$basename"
 }
 
+# Parse oids from git ls-files --staged output
+test_parse_ls_files_stage_oids () {
+	awk '{print $2}' -
+}
+
+# Parse oids from git ls-tree output
+test_parse_ls_tree_oids () {
+	awk '{print $3}' -
+}
+
 # Choose a port number based on the test script's number and store it in
 # the given variable name, unless that variable already contains a number.
 test_set_port () {

gitgitgadget

From: Neeraj Singh <[email protected]>

Add test cases to exercise batch mode for:
* 'git add'
* 'git stash'
* 'git update-index'
* 'git unpack-objects'

These tests ensure that the added data winds up in the object database.

In this change we introduce a new test helper lib-unique-files.sh. The goal of this library is to create a tree of files that have different oids from any other files that may have been created in the current test repo. This helps us avoid missing validation of an object being added due to it already being in the repo.

---
 t/lib-unique-files.sh  | 34 ++++++++++++++++++++++++++++++++++
 t/t3700-add.sh         | 28 ++++++++++++++++++++++++++++
 t/t3903-stash.sh       | 20 ++++++++++++++++++++
 t/t5300-pack-object.sh | 41 +++++++++++++++++++++++++++--------------
 4 files changed, 109 insertions(+), 14 deletions(-)
 create mode 100644 t/lib-unique-files.sh

new file mode 100644
index 00000000000..34c01a65256
@@ -0,0 +1,34 @@
+# Helper to create files with unique contents
+
+# Create multiple files with unique contents within this test run. Takes the
+# number of directories, the number of files in each directory, and the base
+# directory.
+#
+# test_create_unique_files 2 3 my_dir -- Creates 2 directories with 3 files
+#					 each in my_dir, all with contents
+#					 different from previous invocations
+#					 of this command in this run.
+
+test_create_unique_files () {
+	test "$#" -ne 3 && BUG "3 param"
+
+	local dirs="$1" &&
+	local files="$2" &&
+	local basedir="$3" &&
+	local counter=0 &&
+	local i &&
+	local j &&
+	test_tick &&
+	local basedata=$basedir$test_tick &&
+	rm -rf "$basedir" &&
+	for i in $(test_seq $dirs)
+	do
+		local dir=$basedir/dir$i &&
+		mkdir -p "$dir" &&
+		for j in $(test_seq $files)
+		do
+			counter=$((counter + 1)) &&
+			echo "$basedata.$counter">"$dir/file$j.txt"
+		done
+	done
+}

index b1f90ba3250..8979c8a5f03 100755
@@ -8,6 +8,8 @@ test_description='Test of git add, including the -- option.'
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
+. $TEST_DIRECTORY/lib-unique-files.sh
+
 # Test the file mode "$1" of the file "$2" in the index.
 test_mode_in_index () {
 	case "$(git ls-files -s "$2")" in

@@ -34,6 +36,32 @@ test_expect_success \
     'Test that "git add -- -q" works' \
     'touch -- -q && git add -- -q'
 
+BATCH_CONFIGURATION='-c core.fsync=loose-object -c core.fsyncmethod=batch'
+
+test_expect_success 'git add: core.fsyncmethod=batch' "
+	test_create_unique_files 2 4 files_base_dir1 &&
+	GIT_TEST_FSYNC=1 git $BATCH_CONFIGURATION add -- ./files_base_dir1/ &&
+	git ls-files --stage files_base_dir1/ |
+	test_parse_ls_files_stage_oids >added_files_oids &&
+
+	# We created 2 subdirs with 4 files each (8 files total) above
+	test_line_count = 8 added_files_oids &&
+	git cat-file --batch-check='%(objectname)' <added_files_oids >added_files_actual &&
+	test_cmp added_files_oids added_files_actual
+"
+
+test_expect_success 'git update-index: core.fsyncmethod=batch' "
+	test_create_unique_files 2 4 files_base_dir2 &&
+	find files_base_dir2 ! -type d -print | xargs git $BATCH_CONFIGURATION update-index --add -- &&
+	git ls-files --stage files_base_dir2 |
+	test_parse_ls_files_stage_oids >added_files2_oids &&
+
+	# We created 2 subdirs with 4 files each (8 files total) above
+	test_line_count = 8 added_files2_oids &&
+	git cat-file --batch-check='%(objectname)' <added_files2_oids >added_files2_actual &&
+	test_cmp added_files2_oids added_files2_actual
+"
+
 test_expect_success \
 	'git add: Test that executable bit is not used if core.filemode=0' \
 	'git config core.filemode 0 &&

index 4abbc8fccae..20e94881964 100755
@@ -9,6 +9,7 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
 . ./test-lib.sh
+. $TEST_DIRECTORY/lib-unique-files.sh
 
 test_expect_success 'usage on cmd and subcommand invalid option' '
 	test_expect_code 129 git stash --invalid-option 2>usage &&

@@ -1410,6 +1411,25 @@ test_expect_success 'stash handles skip-worktree entries nicely' '
 	git rev-parse --verify refs/stash:A.t
 '
 
+
+BATCH_CONFIGURATION='-c core.fsync=loose-object -c core.fsyncmethod=batch'
+
+test_expect_success 'stash with core.fsyncmethod=batch' "
+	test_create_unique_files 2 4 files_base_dir &&
+	GIT_TEST_FSYNC=1 git $BATCH_CONFIGURATION stash push -u -- ./files_base_dir/ &&
+
+	# The files were untracked, so use the third parent,
+	# which contains the untracked files
+	git ls-tree -r stash^3 -- ./files_base_dir/ |
+	test_parse_ls_tree_oids >stashed_files_oids &&
+
+	# We created 2 dirs with 4 files each (8 files total) above
+	test_line_count = 8 stashed_files_oids &&
+	git cat-file --batch-check='%(objectname)' <stashed_files_oids >stashed_files_actual &&
+	test_cmp stashed_files_oids stashed_files_actual
+"
+
+
 test_expect_success 'git stash succeeds despite directory/file change' '
 	test_create_repo directory_file_switch_v1 &&
 	(

gitgitgadget

From: Neeraj Singh <[email protected]>

Tests that affect the repo in stateful ways are easier to write if we can run setup steps outside of the measured portion of perf iteration.

This change adds a "--setup 'setup-script'" parameter to test_perf. To make invocations easier to understand, I also moved the prerequisites to a new --prereq parameter.

The setup facility will be used in the upcoming perf tests for batch mode, but it already helps in some existing tests, like t5302 and t7820.

---
 t/perf/p4220-log-grep-engines.sh       |  3 +-
 t/perf/p4221-log-grep-engines-fixed.sh |  3 +-
 t/perf/p5302-pack-index.sh             | 15 +++----
 t/perf/p7519-fsmonitor.sh              | 18 ++------
 t/perf/p7820-grep-engines.sh           |  6 ++-
 t/perf/perf-lib.sh                     | 62 +++++++++++++++++++++++---
 6 files changed, 73 insertions(+), 34 deletions(-)

index 2bc47ded4d1..03fbfbb85d3 100755
@@ -36,7 +36,8 @@ do
 		else
 			prereq=""
 		fi
-		test_perf $prereq "$engine log$GIT_PERF_4220_LOG_OPTS --grep='$pattern'" "
+		test_perf "$engine log$GIT_PERF_4220_LOG_OPTS --grep='$pattern'" \
+			--prereq "$prereq" "
 			git -c grep.patternType=$engine log --pretty=format:%h$GIT_PERF_4220_LOG_OPTS --grep='$pattern' >'out.$engine' || :
 		"
 	done

index 060971265a9..0a6d6dfc219 100755
@@ -26,7 +26,8 @@ do
 		else
 			prereq=""
 		fi
-		test_perf $prereq "$engine log$GIT_PERF_4221_LOG_OPTS --grep='$pattern'" "
+		test_perf "$engine log$GIT_PERF_4221_LOG_OPTS --grep='$pattern'" \
+			--prereq "$prereq" "
 			git -c grep.patternType=$engine log --pretty=format:%h$GIT_PERF_4221_LOG_OPTS --grep='$pattern' >'out.$engine' || :
 		"
 	done

index c16f6a3ff69..14c601bbf86 100755
@@ -26,9 +26,8 @@ test_expect_success 'set up thread-counting tests' '
 	done
 '
 
-test_perf PERF_EXTRA 'index-pack 0 threads' '
-	rm -rf repo.git &&
-	git init --bare repo.git &&
+test_perf 'index-pack 0 threads' --prereq PERF_EXTRA \
+	--setup 'rm -rf repo.git && git init --bare repo.git' '
 	GIT_DIR=repo.git git index-pack --threads=1 --stdin < $PACK
 '
 

@@ -36,17 +35,15 @@ for t in $threads
 do
 	THREADS=$t
 	export THREADS
-	test_perf PERF_EXTRA "index-pack $t threads" '
-		rm -rf repo.git &&
-		git init --bare repo.git &&
+	test_perf "index-pack $t threads" --prereq PERF_EXTRA \
+		--setup 'rm -rf repo.git && git init --bare repo.git' '
 		GIT_DIR=repo.git GIT_FORCE_THREADS=1 \
 		git index-pack --threads=$THREADS --stdin <$PACK
 	'
 done
 
-test_perf 'index-pack default number of threads' '
-	rm -rf repo.git &&
-	git init --bare repo.git &&
+test_perf 'index-pack default number of threads' \
+	--setup 'rm -rf repo.git && git init --bare repo.git' '
 	GIT_DIR=repo.git git index-pack --stdin < $PACK
 '
 

index c8be58f3c76..5b489c968b8 100755
@@ -60,18 +60,6 @@ then
 	esac
 fi
 
-if test -n "$GIT_PERF_7519_DROP_CACHE"
-then
-	# When using GIT_PERF_7519_DROP_CACHE, GIT_PERF_REPEAT_COUNT must be 1 to
-	# generate valid results. Otherwise the caching that happens for the nth
-	# run will negate the validity of the comparisons.
-	if test "$GIT_PERF_REPEAT_COUNT" -ne 1
-	then
-		echo "warning: Setting GIT_PERF_REPEAT_COUNT=1" >&2
-		GIT_PERF_REPEAT_COUNT=1
-	fi
-fi
-
 trace_start() {
 	if test -n "$GIT_PERF_7519_TRACE"
 	then

@@ -167,10 +155,10 @@ setup_for_fsmonitor() {
 
 test_perf_w_drop_caches () {
 	if test -n "$GIT_PERF_7519_DROP_CACHE"; then
-		test-tool drop-caches
+		test_perf "$1" --setup "test-tool drop-caches" "$2"
+	else
+		test_perf "$@"
 	fi
-
-	test_perf "$@"
 }
 
 test_fsmonitor_suite() {

index 8b09c5bf328..9bfb86842a9 100755
@@ -49,13 +49,15 @@ do
 		fi
 		if ! test_have_prereq PERF_GREP_ENGINES_THREADS
 		then
-			test_perf $prereq "$engine grep$GIT_PERF_7820_GREP_OPTS '$pattern'" "
+			test_perf "$engine grep$GIT_PERF_7820_GREP_OPTS '$pattern'" \
+				--prereq "$prereq" "
 				git -c grep.patternType=$engine grep$GIT_PERF_7820_GREP_OPTS -- '$pattern' >'out.$engine' || :
 			"
 		else
 			for threads in $GIT_PERF_GREP_THREADS
 			do
-				test_perf PTHREADS,$prereq "$engine grep$GIT_PERF_7820_GREP_OPTS '$pattern' with $threads threads" "
+				test_perf "$engine grep$GIT_PERF_7820_GREP_OPTS '$pattern' with $threads threads"
+					--prereq PTHREADS,$prereq "
 					git -c grep.patternType=$engine -c grep.threads=$threads grep$GIT_PERF_7820_GREP_OPTS -- '$pattern' >'out.$engine.$threads' || :
 				"
 			done

gitgitgadget

Thanks.

From: Neeraj Singh <[email protected]>

Add basic performance tests for "git add" and "git stash" of a lot of new objects with various fsync settings. This shows the benefit of batch mode relative to full fsync.

---
 t/perf/p3700-add.sh | 59 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)
 create mode 100755 t/perf/p3700-add.sh

new file mode 100755
index 00000000000..ef6024f9897
@@ -0,0 +1,59 @@
+#!/bin/sh
+#
+# This test measures the performance of adding new files to the object database
+# and index. The test was originally added to measure the effect of the
+# 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 adding things to the object database"
+
+# Fsync is normally turned off for the test suite.
+GIT_TEST_FSYNC=1
+export GIT_TEST_FSYNC
+
+. ./perf-lib.sh
+
+. $TEST_DIRECTORY/lib-unique-files.sh
+
+test_perf_fresh_repo
+test_checkout_worktree
+
+dir_count=10
+files_per_dir=50
+total_files=$((dir_count * files_per_dir))
+
+for mode in false true 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
+	"
+done
+
+test_done

gitgitgadget

Ævar suggested that it's a bit funny to have a stash test in a file named 'p3700-add.sh'. So I'm thinking that I'll move these tests to a p0008-odb-fsync.sh instead, and add a test for unpack-object and git-commit as well. I'm also going to adopt Ævar's scheme for iterating over the configs.

From: Neeraj Singh <[email protected]>

The warning for an unrecognized fsyncMethod was not camel-cased.

---
 config.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index e9cac5f4707..ae819dee20b 100644
@@ -1697,7 +1697,7 @@ static int git_default_core_config(const char *var, const char *value, void *cb)
 
 	if (!strcmp(var, "core.fsyncobjectfiles")) {
 		if (fsync_object_files < 0)
-			warning(_("core.fsyncobjectfiles is deprecated; use core.fsync instead"));
+			warning(_("core.fsyncObjectFiles is deprecated; use core.fsync instead"));
 		fsync_object_files = git_config_bool(var, value);
 		return 0;
 	}

gitgitgadget

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

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/[email protected]/ 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:

index ef6024f9897..60abd5ee076 100755
@@ -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

new file mode 100755
index 00000000000..12c489069ba
@@ -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

index a935ad622d3..24a5108f234 100644
@@ -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%

I was getting tired of editing two files that were nearly identical and thought that reviewers would be tired of reading them. At least in the main test suite, t/t3700-add.sh covers update-index in addition to git-add.

The advantage of the random data is that it's easy to scale the number of files and change the tree shape. I wanted the test_create_unique_files helper anyway, so using it here made sense. Also, I'm quite confident that I'm really getting new objects added to the repo with this test scheme.

Something is wrong with your data here. Or your repo is too small to highlight the differences.

I'd suggest that if you want to write a different perf test for this feature, that it be a follow-on change.

Thanks,
Neeraj

Despite my
https://lore.kernel.org/git/[email protected]/
I eventually gave up on trying to extract meaningful numbers from t/perf, I can never quite find out if they're because of its shellscripts shenanigans or actual code.

(And also; I realize I didn't follow-up on
https://lore.kernel.org/git/CANQDOdcFN5GgOPZ3hqCsjHDTiRfRpqoAKxjF1n9D6S8oD9--_A@mail.gmail.com/,
sorry):

But I came up with this (uses my thin
https://gitlab.com/avar/git-hyperfine/ wrapper, and you should be able to apt get hyperfine):

	#!/bin/sh
	set -xe
	
	if ! test -d /tmp/scalar.git
	then
		git clone --bare https://github.com/Microsoft/scalar.git /tmp/scalar.git
		mv /tmp/scalar.git/objects/pack/*.pack /tmp/scalar.git/my.pack
	fi
	git hyperfine \
	        --warmup 1 -r 3 \
		-L rev neeraj-v4,avar-RFC \
		-s 'make CFLAGS=-O3 && rm -rf repo && git init repo && cp -R t repo/ && git ls-files -- t >repo/.git/to-add.txt' \
		-p 'rm -rf repo/.git/objects/* repo/.git/index' \
		$@'./git -c core.fsync=loose-object -c core.fsyncMethod=batch -C repo update-index --add --stdin <repo/.git/to-add.txt'
	
	git hyperfine \
	        --warmup 1 -r 3 \
		-L rev neeraj-v4,avar-RFC \
		-s 'make CFLAGS=-O3 && rm -rf repo && git init repo && cp -R t repo/' \
		-p 'rm -rf repo/.git/objects/* repo/.git/index' \
		$@'./git -c core.fsync=loose-object -c core.fsyncMethod=batch -C repo add .'
	
	git hyperfine \
	        --warmup 1 -r 3 \
		-L rev neeraj-v4,avar-RFC \
	        -s 'make CFLAGS=-O3' \
	        -p 'git init --bare dest.git' \
	        -c 'rm -rf dest.git' \
	        $@'./git -C dest.git -c core.fsyncMethod=batch unpack-objects </tmp/scalar.git/my.pack'

Those tags are your v4 here & the v2 of the RFC I sent at
https://lore.kernel.org/git/[email protected]/

Which shows my RFC v2 is ~20% faster with:

    $ PFX='strace' ~/g/git.meta/benchmark.sh "strace "

1.22 ± 0.02 times faster than 'strace ./git -c core.fsync=loose-object -c core.fsyncMethod=batch -C repo update-index --add --stdin <repo/.git/to-add.txt' in 'neeraj-v4' 1.22 ± 0.01 times faster than 'strace ./git -c core.fsync=loose-object -c core.fsyncMethod=batch -C repo add .' in 'neeraj-v4'
1.00 ± 0.01 times faster than 'strace ./git -C dest.git -c core.fsyncMethod=batch unpack-objects </tmp/scalar.git/my.pack' in 'neeraj-v4'

But only for add/update-index, is the unpack-objects not using the tmp-objdir? (presumably yes).

As noted before I've found "strace" to be a handy way to "simulate" slower FS ops on a ramdisk (I get about the same numbers sometimes on the actual non-SSD disk, but due to load on the system (that I'm not in full control of[1]) I can't get hyperfine to be happy with the non-fuzzyness:

1.06 ± 0.02 times faster than './git -c core.fsync=loose-object -c core.fsyncMethod=batch -C repo update-index --add --stdin <repo/.git/to-add.txt' in 'neeraj-v4' 1.06 ± 0.03 times faster than './git -c core.fsync=loose-object -c core.fsyncMethod=batch -C repo add .' in 'neeraj-v4'
1.01 ± 0.01 times faster than './git -C dest.git -c core.fsyncMethod=batch unpack-objects </tmp/scalar.git/my.pack' in 'neeraj-v4'

FWIW these are my actual non-fuzzy-with-strace numbers on the not-ramdisk, as you can see the intervals overlap, but for the first two the "min" time is never close to the RFC v2:

	$ XDG_RUNTIME_DIR=/tmp/ghf ~/g/git.meta/benchmark.sh
	+ test -d /tmp/scalar.git
	+ git hyperfine --warmup 1 -r 3 -L rev neeraj-v4,avar-RFC -s make CFLAGS=-O3 && rm -rf repo && git init repo && cp -R t repo/ && git ls-files -- t >repo/.git/to-add.txt -p rm -rf repo/.git/objects/* repo/.git/index ./git -c core.fsync=loose-object -c core.fsyncMethod=batch -C repo update-index --add --stdin <repo/.git/to-add.txt
	Benchmark 1: ./git -c core.fsync=loose-object -c core.fsyncMethod=batch -C repo update-index --add --stdin <repo/.git/to-add.txt' in 'neeraj-v4
	  Time (mean ± σ):      1.043 s ±  0.143 s    [User: 0.184 s, System: 0.193 s]
	  Range (min … max):    0.943 s …  1.207 s    3 runs

	Benchmark 2: ./git -c core.fsync=loose-object -c core.fsyncMethod=batch -C repo update-index --add --stdin <repo/.git/to-add.txt' in 'avar-RFC
	  Time (mean ± σ):     877.6 ms ± 183.4 ms    [User: 197.9 ms, System: 149.4 ms]
	  Range (min … max):   697.8 ms … 1064.4 ms    3 runs
	
	Summary
	  './git -c core.fsync=loose-object -c core.fsyncMethod=batch -C repo update-index --add --stdin <repo/.git/to-add.txt' in 'avar-RFC' ran
	    1.19 ± 0.30 times faster than './git -c core.fsync=loose-object -c core.fsyncMethod=batch -C repo update-index --add --stdin <repo/.git/to-add.txt' in 'neeraj-v4'
	+ git hyperfine --warmup 1 -r 3 -L rev neeraj-v4,avar-RFC -s make CFLAGS=-O3 && rm -rf repo && git init repo && cp -R t repo/ -p rm -rf repo/.git/objects/* repo/.git/index ./git -c core.fsync=loose-object -c core.fsyncMethod=batch -C repo add .
	Benchmark 1: ./git -c core.fsync=loose-object -c core.fsyncMethod=batch -C repo add .' in 'neeraj-v4
	  Time (mean ± σ):      1.019 s ±  0.057 s    [User: 0.213 s, System: 0.194 s]
	  Range (min … max):    0.963 s …  1.076 s    3 runs
	
	Benchmark 2: ./git -c core.fsync=loose-object -c core.fsyncMethod=batch -C repo add .' in 'avar-RFC
	  Time (mean ± σ):     918.6 ms ±  34.4 ms    [User: 207.8 ms, System: 164.1 ms]
	  Range (min … max):   880.6 ms … 947.5 ms    3 runs
	
	Summary
	  './git -c core.fsync=loose-object -c core.fsyncMethod=batch -C repo add .' in 'avar-RFC' ran
	    1.11 ± 0.07 times faster than './git -c core.fsync=loose-object -c core.fsyncMethod=batch -C repo add .' in 'neeraj-v4'
	+ git hyperfine --warmup 1 -r 3 -L rev neeraj-v4,avar-RFC -s make CFLAGS=-O3 -p git init --bare dest.git -c rm -rf dest.git ./git -C dest.git -c core.fsyncMethod=batch unpack-objects </tmp/scalar.git/my.pack
	Benchmark 1: ./git -C dest.git -c core.fsyncMethod=batch unpack-objects </tmp/scalar.git/my.pack' in 'neeraj-v4
	  Time (mean ± σ):      1.362 s ±  0.285 s    [User: 1.021 s, System: 0.186 s]
	  Range (min … max):    1.192 s …  1.691 s    3 runs
	
	  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet PC without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.
	
	Benchmark 2: ./git -C dest.git -c core.fsyncMethod=batch unpack-objects </tmp/scalar.git/my.pack' in 'avar-RFC
	 ⠏ Performing warmup runs         ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ ⠙ Performing warmup runs         ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░  Time (mean ± σ):      1.188 s ±  0.009 s    [User: 1.025 s, System: 0.161 s]
	  Range (min … max):    1.180 s …  1.199 s    3 runs
	 
	Summary
	  './git -C dest.git -c core.fsyncMethod=batch unpack-objects </tmp/scalar.git/my.pack' in 'avar-RFC' ran
	    1.15 ± 0.24 times faster than './git -C dest.git -c core.fsyncMethod=batch unpack-objects </tmp/scalar.git/my.pack' in 'neeraj-v4'

1. I do my git hacking on a bare metal box I rent with some friends, and one of them is running one those persistent video game daemons written in Java. So I think all my non-RAM I/O numbers are continually fuzzed by what players are doing in Minecraft or whatever that thing is...

Looks like we aren't actually hitting fsync in the numbers you expressed there, if they're down in the 20ms range. Or we simply aren't adding enough files. Or if that's against a ramdisk, the ramdisk doesn't have enough cost to represent real disk hardware.

At least in this case, I don't think 'strace' is representative of what a real disk would behave like. Unless you can somehow make strace of sync_file_range cost less than strace of fsync.

Thanks for the numbers. So if I'm understanding correctly, the difference on a real disk between quarantine and non-quarantine is 20% or so on your system?

I did my own experiment by adding a 'batch-no-quarantine' method. No quarantine was slightly faster.
* For 'git add' I found a very small difference (.29s vs 30s).
* For 'git stash' it was a bit bigger (.35s vs.55s).

This is with perf-lib, so we're just looking at min-times. On the other hand, classic fsync is 1.04s for 'git add' and 1.21s for 'git stash', all with 500 tiny blobs. FYI, this is measured on my laptop running Ubuntu in WSL.

I don't think it's worth having a knob for no-quarantine for this
small delta. I believe a better use of time for a follow-on change would be to implement an appendable pack format for newly-added objects.

Thanks,
Neeraj

V5 changes:

* Remove 'local-with-assignment' from perf-lib.sh
* Add a new patch to put an ODB transaction around add_files_to_cache.
* Move the perf tests to t/perf/p0008-odb-fsync.sh and add more perf tests.

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.

V3 changes:

* Rebrand plug/unplug-bulk-checkin to "begin_odb_transaction" and "end_odb_transaction"
* Add a patch to pass filenames to fsync_or_die, rather than the string "loose object"
* Update the commit description for "core.fsyncmethod to explain why we do not directly expose objects until an fsync occurs.
* Also explain in the commit description why we're using a dummy file for the fsync.
* Create the bulk-fsync tmp-objdir lazily the first time a loose object is added. We now do fsync iff that objdir exists.
* Do batch fsync if core.fsyncMethod=batch and core.fsync contains loose-object, regardless of the core.fsyncObjectFiles setting.
* Mitigate the risk in update-index of an object not being visible due to bulk checkin.
* Add a perf comment to justify the unpack-objects usage of bulk-checkin.
* Add a new patch to create helpers for parsing OIDs from git commands.
* Add a comment to the lib-unique-files.sh helper about uniqueness only within a repo.
* Fix style and add '&&' chaining to test helpers.
* Comment on some magic numbers in tests.
* Take the object list as an argument in ./t5300-pack-object.sh:check_unpack ()
* Drop accidental change to t/perf/perf-lib.sh

V2 changes:

* Change doc to indicate that only some repo updates are batched
* Null and zero out control variables in do_batch_fsync under unplug_bulk_checkin
* Make batch mode default on Windows.
* Update the description for the initial patch that cleans up the bulk-checkin infrastructure.
* Rebase onto 'seen' at 0cac37f38f9.

--Original definition-- When core.fsync includes loose-object, we issue an fsync after every written object. For a 'git-add' or similar command that adds a lot of files to the repo, the costs of these fsyncs adds up. One major factor in this cost is the time it takes for the physical storage controller to flush its caches to durable media.

This series takes advantage of the writeout-only mode of git_fsync to issue OS cache writebacks for all of the objects being added to the repository followed by a single fsync to a dummy file, which should trigger a filesystem log flush and storage controller cache flush. This mechanism is known to be safe on common Windows filesystems and expected to be safe on macOS. Some linux filesystems, such as XFS, will probably do the right thing as well. See [1] for previous discussion on the predecessor of this patch series.

This series is important on Windows, where loose-objects are included in the fsync set by default in Git-For-Windows. In this series, I'm also setting the default mode for Windows to turn on loose object fsyncing with batch mode, so that we can get CI coverage of the actual git-for-windows configuration upstream. We still don't actually issue fsyncs for the test suite since GIT_TEST_FSYNC is set to 0, but we exercise all of the surrounding batch mode code.

This work is based on 'next' at c54b8eb302. It's dependent on ns/core-fsyncmethod.

[1]
https://lore.kernel.org/git/2c1ddef6057157d85da74a7274e03eacf0374e45.1629856293.git.gitgitgadget@gmail.com/

Neeraj Singh (14):
bulk-checkin: rename 'state' variable and separate 'plugged' boolean
bulk-checkin: rebrand plug/unplug APIs as 'odb transactions'
object-file: pass filename to fsync_or_die core.fsyncmethod: batched disk flushes for loose-objects
cache-tree: use ODB transaction around writing a tree builtin/add: add ODB transaction around add_files_to_cache
update-index: use the bulk-checkin infrastructure
unpack-objects: use the bulk-checkin infrastructure core.fsync: use batch mode and sync loose objects by default on
Windows
test-lib-functions: add parsing helpers for ls-files and ls-tree core.fsyncmethod: tests for batch mode
t/perf: add iteration setup mechanism to perf-lib core.fsyncmethod: performance tests for batch mode core.fsyncmethod: correctly camel-case warning message

 Documentation/config/core.txt          |   8 ++
 builtin/add.c                          |  13 +++-
 builtin/unpack-objects.c               |   3 +
 builtin/update-index.c                 |  24 ++++++
 bulk-checkin.c                         | 101 ++++++++++++++++++++++---
 bulk-checkin.h                         |  17 ++++-
 cache-tree.c                           |   3 +
 cache.h                                |  12 ++-
 compat/mingw.h                         |   3 +
 config.c                               |   6 +-
 git-compat-util.h                      |   2 +
 object-file.c                          |  15 ++--
 t/lib-unique-files.sh                  |  34 +++++++++
 t/perf/p0008-odb-fsync.sh              |  81 ++++++++++++++++++++
 t/perf/p4220-log-grep-engines.sh       |   3 +-
 t/perf/p4221-log-grep-engines-fixed.sh |   3 +-
 t/perf/p5302-pack-index.sh             |  15 ++--
 t/perf/p7519-fsmonitor.sh              |  18 +----
 t/perf/p7820-grep-engines.sh           |   6 +-
 t/perf/perf-lib.sh                     |  62 +++++++++++++--
 t/t3700-add.sh                         |  28 +++++++
 t/t3903-stash.sh                       |  20 +++++
 t/t5300-pack-object.sh                 |  41 ++++++----
 t/t5317-pack-objects-filter-objects.sh |  91 +++++++++++-----------
 t/test-lib-functions.sh                |  10 +++
 25 files changed, 501 insertions(+), 118 deletions(-)
 create mode 100644 t/lib-unique-files.sh
 create mode 100755 t/perf/p0008-odb-fsync.sh

base-commit: c54b8eb302ffb72f31e73a26044c8a864e2cb307
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1134%2Fneerajsi-msft%2Fns%2Fbatched-fsync-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1134/neerajsi-msft/ns/batched-fsync-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/1134

Range-diff vs v4:

  1:  c7a2a7efe6d =  1:  c7a2a7efe6d bulk-checkin: rename 'state' variable and separate 'plugged' boolean
  2:  d045b13795b =  2:  d045b13795b bulk-checkin: rebrand plug/unplug APIs as 'odb transactions'
  3:  2d1bc4568ac =  3:  2d1bc4568ac object-file: pass filename to fsync_or_die
  4:  9e7ae22fa4a =  4:  9e7ae22fa4a core.fsyncmethod: batched disk flushes for loose-objects
  5:  83fa4a5f3a5 =  5:  83fa4a5f3a5 cache-tree: use ODB transaction around writing a tree
  -:  ----------- >  6:  d514842ad49 builtin/add: add ODB transaction around add_files_to_cache
  6:  f03ebee695a =  7:  8cac94598a5 update-index: use the bulk-checkin infrastructure
  7:  d85013f7d2c =  8:  523e5fbd63e unpack-objects: use the bulk-checkin infrastructure
  8:  73e54f94c20 =  9:  faacc19aab2 core.fsync: use batch mode and sync loose objects by default on Windows
  9:  124450c86d9 = 10:  4de7300a7b0 test-lib-functions: add parsing helpers for ls-files and ls-tree
 10:  282fbdef792 = 11:  1a4aff8c350 core.fsyncmethod: tests for batch mode
 11:  ee7ecf4cabe ! 12:  47cc63e1dda t/perf: add iteration setup mechanism to perf-lib
     @@ t/perf/perf-lib.sh: exit $ret' >&3 2>&4
       }
       
       test_wrapper_ () {
     --	test_wrapper_func_=$1; shift
     -+	local test_wrapper_func_=$1; shift
     -+	local test_title_=$1; shift
     ++	local test_wrapper_func_ test_title_
     + 	test_wrapper_func_=$1; shift
     ++	test_title_=$1; shift
       	test_start_
      -	test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
      -	test "$#" = 2 ||
     @@ t/perf/perf-lib.sh: exit $ret' >&3 2>&4
       	export test_prereq
      -	if ! test_skip "$@"
      +	export test_perf_setup_
     ++
      +	if ! test_skip "$test_title_" "$@"
       	then
       		base=$(basename "$0" .sh)
 12:  fdf90d45f52 ! 13:  26be6ecb28b core.fsyncmethod: performance tests for add and stash
     @@ Metadata
      Author: Neeraj Singh <[email protected]>
      
       ## Commit message ##
     -    core.fsyncmethod: performance tests for add and stash
     +    core.fsyncmethod: performance tests for batch mode
      
     -    Add basic performance tests for "git add" and "git stash" of a lot of
     -    new objects with various fsync settings. This shows the benefit of batch
     -    mode relative to full fsync.
     +    Add basic performance tests for git commands that can add data to the
     +    object database. We cover:
     +    * git add
     +    * git stash
     +    * git update-index (via git stash)
     +    * git unpack-objects
     +    * git commit --all
     +
     +    We cover all currently available fsync methods as well.
      
          Signed-off-by: Neeraj Singh <[email protected]>
      
     - ## t/perf/p3700-add.sh (new) ##
     + ## t/perf/p0008-odb-fsync.sh (new) ##
      @@
      +#!/bin/sh
      +#
     -+# This test measures the performance of adding new files to the object database
     -+# and index. The test was originally added to measure the effect of the
     -+# core.fsyncMethod=batch mode, which is why we are testing different values
     -+# of that setting explicitly and creating a lot of unique objects.
     ++# This test measures the performance of adding new files to the object
     ++# database. The test was originally added to measure the effect of the
     ++# 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 adding things to the object database"
      +
     -+# Fsync is normally turned off for the test suite.
     -+GIT_TEST_FSYNC=1
     -+export GIT_TEST_FSYNC
     -+
      +. ./perf-lib.sh
      +
      +. $TEST_DIRECTORY/lib-unique-files.sh
     @@ t/perf/p3700-add.sh (new)
      +files_per_dir=50
      +total_files=$((dir_count * files_per_dir))
      +
     -+for mode in false true 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
     ++populate_files () {
     ++	test_create_unique_files $dir_count $files_per_dir files
     ++}
     ++
     ++setup_repo () {
     ++	(rm -rf .git || 1) &&
     ++	git init &&
     ++	test_commit first &&
     ++	populate_files
     ++}
     ++
     ++test_perf_fsync_cfgs () {
     ++	local method cfg &&
     ++	for method in none fsync batch writeout-only
     ++	do
     ++		case $method in
     ++		none)
     ++			cfg="-c core.fsync=none"
     ++			;;
     ++		*)
     ++			cfg="-c core.fsync=loose-object -c core.fsyncMethod=$method"
     ++		esac &&
     ++
     ++		# Set GIT_TEST_FSYNC=1 explicitly since fsync is normally
     ++		# disabled by t/test-lib.sh.
     ++		if ! test_perf "$1 (fsyncMethod=$method)" \
     ++						--setup "$2" \
     ++						"GIT_TEST_FSYNC=1 git $cfg $3"
     ++		then
     ++			break
     ++		fi
     ++	done
     ++}
     ++
     ++test_perf_fsync_cfgs "add $total_files files" \
     ++	"setup_repo" \
     ++	"add -- files"
     ++
     ++test_perf_fsync_cfgs "stash $total_files files" \
     ++	"setup_repo" \
     ++	"stash push -u -- files"
      +
     -+	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_fsync_cfgs "unpack $total_files files" \
      +	"
     ++	setup_repo &&
     ++	git -c core.fsync=none add -- files &&
     ++	git -c core.fsync=none commit -q -m second &&
     ++	echo HEAD | git pack-objects -q --stdout --revs >test_pack.pack &&
     ++	setup_repo
     ++	" \
     ++	"unpack-objects -q <test_pack.pack"
      +
     -+	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_fsync_cfgs "commit $total_files files" \
      +	"
     -+done
     ++	setup_repo &&
     ++	git -c core.fsync=none add -- files &&
     ++	populate_files
     ++	" \
     ++	"commit -q -a -m test"
      +
      +test_done

13: fb30bd02c8d = 14: 88c1f84d4c3 core.fsyncmethod: correctly camel-case warning message

gitgitgadget

From: Neeraj Singh <[email protected]>

This commit prepares for adding batch-fsync to the bulk-checkin infrastructure.

The bulk-checkin infrastructure is currently used to batch up addition of large blobs to a packfile. When a blob is larger than big_file_threshold, we unconditionally add it to a pack. If bulk checkins are 'plugged', we allow multiple large blobs to be added to a single pack until we reach the packfile size limit; otherwise, we simply make a new packfile for each large blob. The 'unplug' call tells us when the series of blob additions is done so that we can finish the packfiles and make their objects available to subsequent operations.

Stated another way, bulk-checkin allows callers to define a transaction that adds multiple objects to the object database, where the object database can optimize its internal operations within the transaction boundary.

Batched fsync will fit into bulk-checkin by taking advantage of the plug/unplug functionality to determine the appropriate time to fsync and make newly-added objects available in the primary object database.

* Rename 'state' variable to 'bulk_checkin_state', since we will later be adding 'bulk_fsync_objdir'. This also makes the variable easier to find in the debugger, since the name is more unique.

* Move the 'plugged' data member of 'bulk_checkin_state' into a separate static variable. Doing this avoids resetting the variable in finish_bulk_checkin when zeroing the 'bulk_checkin_state'. As-is, we seem to unintentionally disable the plugging functionality the first time a new packfile must be created due to packfile size limits. While disabling the plugging state only results in suboptimal behavior for the current code, it would be fatal for the bulk-fsync functionality later in this patch series.

---
 bulk-checkin.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

index 6d6c37171c9..577b135e39c 100644
@@ -10,9 +10,9 @@
 #include "packfile.h"
 #include "object-store.h"
 
-static struct bulk_checkin_state {
-	unsigned plugged:1;
+static int bulk_checkin_plugged;
 
+static struct bulk_checkin_state {
 	char *pack_tmp_name;
 	struct hashfile *f;
 	off_t offset;

@@ -21,7 +21,7 @@ static struct bulk_checkin_state {
 	struct pack_idx_entry **written;
 	uint32_t alloc_written;
 	uint32_t nr_written;
-} state;
+} bulk_checkin_state;
 
 static void finish_tmp_packfile(struct strbuf *basename,
 				const char *pack_tmp_name,

@@ -278,21 +278,23 @@ int index_bulk_checkin(struct object_id *oid,
 		       int fd, size_t size, enum object_type type,
 		       const char *path, unsigned flags)
 {
-	int status = deflate_to_pack(&state, oid, fd, size, type,
+	int status = deflate_to_pack(&bulk_checkin_state, oid, fd, size, type,
 				     path, flags);
-	if (!state.plugged)
-		finish_bulk_checkin(&state);
+	if (!bulk_checkin_plugged)
+		finish_bulk_checkin(&bulk_checkin_state);
 	return status;
 }
 
 void plug_bulk_checkin(void)
 {
-	state.plugged = 1;
+	assert(!bulk_checkin_plugged);
+	bulk_checkin_plugged = 1;
 }
 
 void unplug_bulk_checkin(void)
 {
-	state.plugged = 0;
-	if (state.f)
-		finish_bulk_checkin(&state);
+	assert(bulk_checkin_plugged);
+	bulk_checkin_plugged = 0;
+	if (bulk_checkin_state.f)
+		finish_bulk_checkin(&bulk_checkin_state);
 }

gitgitgadget

Paraphrasing to make sure I understand your reasoning here...

In the "plug and then perform as many changes to the repository and finally unplug" flow, before or after this series, the "perform" step in the middle is unaware of which "bulk_checkin_state" instance is being used to keep track of what is done to optimize by deferring some operations until the "unplug" time. So bulk_checkin_state is not there to allow us to create multiple instances of it, pass them around to different sequences of "plug, perform, unplug". Each of its members is inherently a singleton, so in the extreme, we could turn these members into separate file-scope global variables if we wanted to. The "plugged" bit happens to be the only one getting ejected by this patch, because it is inconvenient to "clear" other members otherwise.

Is that what is going on?

If it is, I am mildly opposed to the flow of thought, from at least two reasons. It makes it hard for the next developer to decide if the new members they are adding should be in or out of the struct.

More importantly, I think the call of finish_bulk_checkin() we make in deflate_to_pack() you found (and there may possibly other places that we do so; I didn't check) may not appear to be a bug in the original context, but it already is a bug. And when we change the semantics of plug-unplug to be more "transaction-like", it becomes a more serious bug, as you said.

There is NO reason to end the ongoing transaction there inside the while() loop that tries to limit the size of the packfile being used. We may want to flush the "packfile part", which may have been almost synonymous to the entirety of bulk_checkin_state, but as you found out, the "plugged" bit is *outside* the "packfile part", and that makes it a bug to call finish_bulk_checkin() from there.

We should add a new function, flush_bulk_checking_packfile(), to flush only the packfile part of the bulk_checkin_state without affecting other things---the "plugged" bit is the only one in the current code before this series, but it does not have to stay to be so. When you start plugging the loose ref transactions, you may find it handy (this is me handwaving) to have a list of refs that you may have to do something at "unplug" time kept in the struct, and you do not want deflate_to_pack() affecting the ongoing "plugged" ref operations by calling finish_bulk_checkin() and reinitializing that list, for example.

And then we should examine existing calls to finish_bulk_checkin() and replace the ones that should not be finishing, i.e. the ones that wanted "flush" but called "finish".

More or less. The current state is all about creating a single packfile for multiple large objects. That packfile is a singleton today (we could have an alternate implementation where there's a separate packfile per thread in the future, so it's not inherent to the API). We want to do this if the top-level caller is okay with the state being invisible until the "finish" call, and that is conveyed by the "plugged" flag.

I'm happy to rename the packfile related stuff to end with _packfile to make it clear that all of that state and functionality is related to batching of packfile additions.
So from this patch: s/bulk_checkin_state/bulk_checkin_packfile and s/finish_bulk_checkin/finish_bulk_checkin_packfile.

My new state will be bulk_fsync_* (as it is already). Any future ODB-related state can go here too (I'm imagining a future log-structured 'new objects pack' that we can append to for adding small objects, similar to the bulk_checkin_packfile but allowing appends from multiple git invocations).

I don't believe ref transactions will go through this part of the infrastructure. Refs already have a good transaction system (that partly inspired this rebranding, after I saw how Peter implemented batch ref fsync). I expect this area will remain all about the ODB as a subsystem that can enlist in a larger repo->transaction. So a top-level Git command might initiate a repo transaction, which would internally initiate an ODB transaction, index transaction, and ref transaction. The repo transaction would support flushing each of the subtransactions with an optimal number of fsyncs.

Sure. I can fix this, which will only change this file. The only case of "finishing" would be in unplug_bulk_checkin / end_odb_transaction.

Thanks,
Neeraj

I do not care about names, though. If you took that I hinted any such change, sorry about that. _state is fine as-is.

I do care about not ejecting plugged out of the structure and instead keeping them together, with proper way to flush the part that deflate_to_pack() wants to flush, instead of abusing the "finish".

Thanks.

Just to understand your feedback better, is it a problem to separate the state of each separate "thing" under ODB transactions into separate file-scope global(s)? In this series I declared the fsync state as completely separate from the packfile state. That's why I was thinking of it as more of a naming problem, since the remaining state aside from the plugged boolean is entirely packfile related.

My argument in favor of having separate file-scoped variables for each 'pluggable thing' would be that future implementations can evolve separately without authors first having to disentangle a single struct.

Thanks,
Neeraj

Ahh, sorry, my mistake.

I somehow thought that you would be making the existing "struct bulk_checkin_state" infrastructure to cover not just the object store but much more, perhaps because I partly mistook the motivation to rename the structure (thinking again about it, since "checkin" is the act of adding new objects to the object database from outside sources (either from the working tree using "git add" command, or from other sources using unpack-objects), the original name was already fine to signal that it was about the object database, and the need to rename it sounded like we were going to do much more than the object database behind my head).

That is fine. Would the trigger to "plug" and "unplug" also be independent? As you said elsewhere, the part to harden refs can piggyback on the existing ref-transaction infrastructure. I do not know offhand what things other than loose objects that want "plug" and "unplug" semantics, but if there are, are we going to have type specific begin- and end-transaction?

Thanks.

With regards to bulk-checkin.h, I believe for simplicity of interface to the callers, there should be a single pair of APIs for plug or unplug of the entire ODB regardless of what optimizations happen under the covers. For eventual repo-wide transactions, there should be a single API to initiate a transaction and a single one to commit/abort the transaction at the end. We may still also want a flush API so that we can make the repo state consistent prior to executing hooks or doing something else where an outside process needs consistent repo state.

Thanks,
Neeraj

From: Neeraj Singh <[email protected]>

Make it clearer in the naming and documentation of the plug_bulk_checkin and unplug_bulk_checkin APIs that they can be thought of as a "transaction" to optimize operations on the object database. These transactions may be nested so that subsystems like the cache-tree writing code can optimize their operations without caring whether the top-level code has a transaction active.

---
 builtin/add.c  |  4 ++--
 bulk-checkin.c | 20 ++++++++++++--------
 bulk-checkin.h | 14 ++++++++++++--
 3 files changed, 26 insertions(+), 12 deletions(-)

index 3ffb86a4338..9bf37ceae8e 100644
@@ -670,7 +670,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		string_list_clear(&only_match_skip_worktree, 0);
 	}
 
-	plug_bulk_checkin();
+	begin_odb_transaction();
 
 	if (add_renormalize)
 		exit_status |= renormalize_tracked_files(&pathspec, flags);

@@ -682,7 +682,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	if (chmod_arg && pathspec.nr)
 		exit_status |= chmod_pathspec(&pathspec, chmod_arg[0], show_only);
-	unplug_bulk_checkin();
+	end_odb_transaction();
 
 finish:
 	if (write_locked_index(&the_index, &lock_file,

index 577b135e39c..8b0fd5c7723 100644
@@ -10,7 +10,7 @@
 #include "packfile.h"
 #include "object-store.h"
 
-static int bulk_checkin_plugged;
+static int odb_transaction_nesting;
 
 static struct bulk_checkin_state {
 	char *pack_tmp_name;

@@ -280,21 +280,25 @@ int index_bulk_checkin(struct object_id *oid,
 {
 	int status = deflate_to_pack(&bulk_checkin_state, oid, fd, size, type,
 				     path, flags);
-	if (!bulk_checkin_plugged)
+	if (!odb_transaction_nesting)
 		finish_bulk_checkin(&bulk_checkin_state);
 	return status;
 }
 
-void plug_bulk_checkin(void)
+void begin_odb_transaction(void)
 {
-	assert(!bulk_checkin_plugged);
-	bulk_checkin_plugged = 1;
+	odb_transaction_nesting += 1;
 }
 
-void unplug_bulk_checkin(void)
+void end_odb_transaction(void)
 {
-	assert(bulk_checkin_plugged);
-	bulk_checkin_plugged = 0;
+	odb_transaction_nesting -= 1;
+	if (odb_transaction_nesting < 0)
+		BUG("Unbalanced ODB transaction nesting");
+
+	if (odb_transaction_nesting)
+		return;
+
 	if (bulk_checkin_state.f)
 		finish_bulk_checkin(&bulk_checkin_state);
 }

index b26f3dc3b74..69a94422ac7 100644
@@ -10,7 +10,17 @@ int index_bulk_checkin(struct object_id *oid,
 		       int fd, size_t size, enum object_type type,
 		       const char *path, unsigned flags);
 
-void plug_bulk_checkin(void);
-void unplug_bulk_checkin(void);
+/*
+ * Tell the object database to optimize for adding
+ * multiple objects. end_odb_transaction must be called
+ * to make new objects visible.
+ */
+void begin_odb_transaction(void);
+
+/*
+ * Tell the object database to make any objects from the
+ * current transaction visible.
+ */
+void end_odb_transaction(void);
 
 #endif

gitgitgadget

I can see that "checkin" part of the name is too limiting (you may want to do more than optimize checkin, e.g. fsync), and that you may prefer "begin/end" over "plug/unplug", but I am not sure if we want to limit ourselves to "odb". If we find our code doing things on many instances of something that are not objects (e.g. refs, config variables), don't we want to give them the same chance to be optimized by batching them?

{begin,end}_bulk_transaction perhaps? I dunno.

At least in the current code where the implementation of each 'database table' (odb, refs-db, config, index) is pretty separate, it seems better to keep bulk-checkin.c and its plugging scoped to the ODB. Patrick's (who I previously misnamed as 'Peter') older patch at
https://lore.kernel.org/git/d9aa96913b1730f1d0c238d7d52e27c20bc55390.1636544377.git.ps@pks.im/
showed a pretty nice and concise implementation for refs tied to the ref-transaction infrastructure.

Thanks,
Neeraj

From: Neeraj Singh <[email protected]>

If we die while trying to fsync a loose object file, pass the actual filename we're trying to sync. This is likely to be more helpful for a user trying to diagnose the cause of the failure than the former 'loose object file' string. It also sidesteps any concerns about translating the die message differently for loose objects versus something else that has a real path.

---
 object-file.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

index b254bc50d70..5ffbf3d4fd4 100644
@@ -1888,16 +1888,16 @@ void hash_object_file(const struct git_hash_algo *algo, const void *buf,
 }
 
 /* Finalize a file on disk, and close it. */
-static void close_loose_object(int fd)
+static void close_loose_object(int fd, const char *filename)
 {
 	if (the_repository->objects->odb->will_destroy)
 		goto out;
 
 	if (fsync_object_files > 0)
-		fsync_or_die(fd, "loose object file");
+		fsync_or_die(fd, filename);
 	else
 		fsync_component_or_die(FSYNC_COMPONENT_LOOSE_OBJECT, fd,
-				       "loose object file");
+				       filename);
 
 out:
 	if (close(fd) != 0)

@@ -2011,7 +2011,7 @@ static int write_loose_object(const struct object_id *oid, char *hdr,
 		die(_("confused by unstable object source data for %s"),
 		    oid_to_hex(oid));
 
-	close_loose_object(fd);
+	close_loose_object(fd, tmp_file.buf);
 
 	if (mtime) {
 		struct utimbuf utb;

gitgitgadget

Looks good and obviously can be done totally outside the series. Perhaps while the rest of the topic is still cooking, extract this (we may find others) and have them graduate sooner?

Ok. I'll create two new single-patch series with changes that are independent.

From: Neeraj Singh <[email protected]>

When adding many objects to a repo with core.fsync=loose-object, the cost of fsync'ing each object file can become prohibitive.

One major source of the cost of fsync is the implied flush of the hardware writeback cache within the disk drive. This commit introduces a new core.fsyncMethod=batch option that batches up hardware flushes. It hooks into the bulk-checkin odb-transaction functionality, takes advantage of tmp-objdir, and uses the writeout-only support code.

When the new mode is enabled, we do the following for each new object:
1a. Create the object in a tmp-objdir.
2a. Issue a pagecache writeback request and wait for it to complete.

At the end of the entire transaction when unplugging bulk checkin:
1b. Issue an fsync against a dummy file to flush the log and hardware
writeback cache, which should by now have seen the tmp-objdir writes.
2b. Rename all of the tmp-objdir files to their final names. 3b. When updating the index and/or refs, we assume that Git will issue
another fsync internal to that operation. This is not the default today, but the user now has the option of syncing the index and there is a separate patch series to implement syncing of refs.

On a filesystem with a singular journal that is updated during name operations (e.g. create, link, rename, etc), such as NTFS, HFS+, or XFS we would expect the fsync to trigger a journal writeout so that this sequence is enough to ensure that the user's data is durable by the time the git command returns. This sequence also ensures that no object files appear in the main object store unless they are fsync-durable.

Batch mode is only enabled if core.fsync includes loose-objects. If the legacy core.fsyncObjectFiles setting is enabled, but core.fsync does not include loose-objects, we will use file-by-file fsyncing.

In step (1a) of the sequence, the tmp-objdir is created lazily to avoid work if no loose objects are ever added to the ODB. We use a tmp-objdir to maintain the invariant that no loose-objects are visible in the main ODB unless they are properly fsync-durable. This is important since future ODB operations that try to create an object with specific contents will silently drop the new data if an object with the target hash exists without checking that the loose-object contents match the hash. Only a full git-fsck would restore the ODB to a functional state where dataloss doesn't occur.

In step (1b) of the sequence, we issue a fsync against a dummy file created specifically for the purpose. This method has a little higher cost than using one of the input object files, but makes adding new callers of this mechanism easier, since we don't need to figure out which object file is "last" or risk sharing violations by caching the fd of the last object file.

_Performance numbers_:

Linux - Hyper-V VM running Kernel 5.11 (Ubuntu 20.04) on a fast SSD. Mac - macOS 11.5.1 running on a Mac mini on a 1TB Apple SSD. Windows - Same host as Linux, a preview version of Windows 11.

Adding 500 files to the repo with 'git add' Times reported in seconds.

object file syncing | Linux | Mac   | Windows
--------------------|-------|-------|--------
           disabled | 0.06  |  0.35 | 0.61
              fsync | 1.88  | 11.18 | 2.47
              batch | 0.15  |  0.41 | 1.53

---
 Documentation/config/core.txt |  8 ++++
 bulk-checkin.c                | 71 +++++++++++++++++++++++++++++++++++
 bulk-checkin.h                |  3 ++
 cache.h                       |  8 +++-
 config.c                      |  2 +
 object-file.c                 |  7 +++-
 6 files changed, 97 insertions(+), 2 deletions(-)

index 9da3e5d88f6..3c90ba0b395 100644
@@ -596,6 +596,14 @@ core.fsyncMethod::
 * `writeout-only` issues pagecache writeback requests, but depending on the
   filesystem and storage hardware, data added to the repository may not be
   durable in the event of a system crash. This is the default mode on macOS.
+* `batch` enables a mode that uses writeout-only flushes to stage multiple
+  updates in the disk writeback cache and then does a single full fsync of
+  a dummy file to trigger the disk cache flush at the end of the operation.
++
+  Currently `batch` mode only applies to loose-object files. Other repository
+  data is made durable as if `fsync` was specified. This mode is expected to
+  be as safe as `fsync` on macOS for repos stored on HFS+ or APFS filesystems
+  and on Windows for repos stored on NTFS or ReFS filesystems.
 
 core.fsyncObjectFiles::
 	This boolean will enable 'fsync()' when writing object files.

index 69a94422ac7..70edf745be8 100644
@@ -6,6 +6,9 @@
 
 #include "cache.h"
 
+void prepare_loose_object_bulk_checkin(void);
+void fsync_loose_object_bulk_checkin(int fd, const char *filename);
+
 int index_bulk_checkin(struct object_id *oid,
 		       int fd, size_t size, enum object_type type,
 		       const char *path, unsigned flags);

index ef7d34b7a09..a5bf15a5131 100644
@@ -1040,7 +1040,8 @@ extern int use_fsync;
 
 enum fsync_method {
 	FSYNC_METHOD_FSYNC,
-	FSYNC_METHOD_WRITEOUT_ONLY
+	FSYNC_METHOD_WRITEOUT_ONLY,
+	FSYNC_METHOD_BATCH,
 };
 
 extern enum fsync_method fsync_method;

@@ -1767,6 +1768,11 @@ void fsync_or_die(int fd, const char *);
 int fsync_component(enum fsync_component component, int fd);
 void fsync_component_or_die(enum fsync_component component, int fd, const char *msg);
 
+static inline int batch_fsync_enabled(enum fsync_component component)
+{
+	return (fsync_components & component) && (fsync_method == FSYNC_METHOD_BATCH);
+}
+
 ssize_t read_in_full(int fd, void *buf, size_t count);
 ssize_t write_in_full(int fd, const void *buf, size_t count);
 ssize_t pread_in_full(int fd, void *buf, size_t count, off_t offset);

index 3c9b6b589ab..511f4584eeb 100644
@@ -1688,6 +1688,8 @@ static int git_default_core_config(const char *var, const char *value, void *cb)
 			fsync_method = FSYNC_METHOD_FSYNC;
 		else if (!strcmp(value, "writeout-only"))
 			fsync_method = FSYNC_METHOD_WRITEOUT_ONLY;
+		else if (!strcmp(value, "batch"))
+			fsync_method = FSYNC_METHOD_BATCH;
 		else
 			warning(_("ignoring unknown core.fsyncMethod value '%s'"), value);
 

index 5ffbf3d4fd4..d2e0c13198f 100644
@@ -1893,7 +1893,9 @@ static void close_loose_object(int fd, const char *filename)
 	if (the_repository->objects->odb->will_destroy)
 		goto out;
 
-	if (fsync_object_files > 0)
+	if (batch_fsync_enabled(FSYNC_COMPONENT_LOOSE_OBJECT))
+		fsync_loose_object_bulk_checkin(fd, filename);
+	else if (fsync_object_files > 0)
 		fsync_or_die(fd, filename);
 	else
 		fsync_component_or_die(FSYNC_COMPONENT_LOOSE_OBJECT, fd,

@@ -1961,6 +1963,9 @@ static int write_loose_object(const struct object_id *oid, char *hdr,
 	static struct strbuf tmp_file = STRBUF_INIT;
 	static struct strbuf filename = STRBUF_INIT;
 
+	if (batch_fsync_enabled(FSYNC_COMPONENT_LOOSE_OBJECT))
+		prepare_loose_object_bulk_checkin();
+
 	loose_object_path(the_repository, &filename, oid);
 
 	fd = create_tmpfile(&tmp_file, filename.buf);

gitgitgadget

Here's the man format (Not sure how this will render going through gmail):
• batch enables a mode that uses writeout-only flushes to
stage multiple updates in the disk writeback cache and then does a single full fsync of a dummy file to trigger the disk cache flush at the end of the
operation.

Currently batch mode only applies to loose-object
files. Other repository
data is made durable as if fsync was specified.
This mode is expected to
be as safe as fsync on macOS for repos stored on
HFS+ or APFS filesystems
and on Windows for repos stored on NTFS or ReFS filesystems.

To describe the above if it doesn't render correctly, we have a bulleted list where the batch after the * is bolded. Other instances of single backtick quoted text just appears as plaintext. The descriptive "Currently batch mode..." paragraph is under the bullet point and well-indented.

In HTML the output looks good as well, except that the descriptive paragraph is in monospace for some reason.

It seems like the only thing tying this to the bulk_checkin_state (which I've renamed in my local changes to bulk_checkin_packfile) is that they're both generally written when a transaction is active. Keeping fsync separate from packfile should help the reader see that the two sets of functions access only their respective global state. If we add another optimization strategy (e.g. appendable pack files), it would get its own separate state and functions that are independent of the large-blob packfile and loose-object fsync optimizations.

There is the possibility that we might create the objdir when calling prepare_loose_object_bulk_checkin but somehow fail to write the object and yet still make it to end_odb_transaction. In that case, we'd issue an extra dummy fsync. I don't think this case is worth extra code to track, since it's a single fsync in a weird failure case.

Thanks for looking at this.

The "except" part admits that it does not render well, no?

What happens if you modify the second and subsequent paragraph after the "+" continuation in the way suggested? Does it make it worse, or does it make it worse?

OK.

Yup.

Apologies, I misinterpreted your statement that the input "has to be flushed left without indentation". Now that I flushed it left I'm getting better output where the follow-on paragraph has a "normal" text style and interior backtick quoted things are bolded as expected.

This will be fixed in the next iteration.

I was reasonably but not absolutely sure if that works inside bulletted list. Thanks for experimenting it for us ;-)

From: Neeraj Singh <[email protected]>

Take advantage of the odb transaction infrastructure around writing the cached tree to the object database.

---
 cache-tree.c | 3 +++
 1 file changed, 3 insertions(+)

index 6752f69d515..8c5e8822716 100644
@@ -3,6 +3,7 @@
 #include "tree.h"
 #include "tree-walk.h"
 #include "cache-tree.h"
+#include "bulk-checkin.h"
 #include "object-store.h"
 #include "replace-object.h"
 #include "promisor-remote.h"

@@ -474,8 +475,10 @@ int cache_tree_update(struct index_state *istate, int flags)
 
 	trace_performance_enter();
 	trace2_region_enter("cache_tree", "update", the_repository);
+	begin_odb_transaction();
 	i = update_one(istate->cache_tree, istate->cache, istate->cache_nr,
 		       "", 0, &skip, flags);
+	end_odb_transaction();
 	trace2_region_leave("cache_tree", "update", the_repository);
 	trace_performance_leave("cache_tree_update");
 	if (i < 0)

gitgitgadget

Yes, with the current series, beginning and ending a transaction will just manipulate a few global variables in bulk-checkin.c unless there is something real to flush.

Thanks,
Neeraj

From: Neeraj Singh <[email protected]>

The add_files_to_cache function is invoked internally by builtin/commit.c and builtin/checkout.c for their flags that stage modified files before doing the larger operation. These commands can benefit from batched fsyncing.

---
 builtin/add.c | 9 +++++++++
 1 file changed, 9 insertions(+)

index 9bf37ceae8e..e39770e4746 100644
@@ -141,7 +141,16 @@ int add_files_to_cache(const char *prefix,
 	rev.diffopt.format_callback_data = &data;
 	rev.diffopt.flags.override_submodule_config = 1;
 	rev.max_count = 0; /* do not compare unmerged paths with stage #2 */
+
+	/*
+	 * Use an ODB transaction to optimize adding multiple objects.
+	 * This function is invoked from commands other than 'add', which
+	 * may not have their own transaction active.
+	 */
+	begin_odb_transaction();
 	run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);
+	end_odb_transaction();
+
 	clear_pathspec(&rev.prune_data);
 	return !!data.add_errors;
 }

gitgitgadget

From: Neeraj Singh <[email protected]>

The update-index functionality is used internally by 'git stash push' to setup the internal stashed commit.

This change enables odb-transactions for update-index infrastructure to speed up adding new objects to the object database by leveraging the batch fsync functionality.

There is some risk with this change, since under batch fsync, the object files will be in a tmp-objdir until update-index is complete, so callers using the --stdin option will not see them until update-index is done. This risk is mitigated by not keeping an ODB transaction open around --stdin processing if in --verbose mode. Without --verbose mode, a caller feeding update-index via --stdin wouldn't know when update-index adds an object, event without an ODB transaction.

---
 builtin/update-index.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

index aafe7eeac2a..50f9063e1c6 100644
@@ -5,6 +5,7 @@
  */
 #define USE_THE_INDEX_COMPATIBILITY_MACROS
 #include "cache.h"
+#include "bulk-checkin.h"
 #include "config.h"
 #include "lockfile.h"
 #include "quote.h"

@@ -1116,6 +1117,12 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 	 */
 	parse_options_start(&ctx, argc, argv, prefix,
 			    options, PARSE_OPT_STOP_AT_NON_OPTION);
+
+	/*
+	 * Allow the object layer to optimize adding multiple objects in
+	 * a batch.
+	 */
+	begin_odb_transaction();
 	while (ctx.argc) {
 		if (parseopt_state != PARSE_OPT_DONE)
 			parseopt_state = parse_options_step(&ctx, options,

@@ -1167,6 +1174,17 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 		the_index.version = preferred_index_format;
 	}
 
+	/*
+	 * It is possible, though unlikely, that a caller could use the verbose
+	 * output to synchronize with addition of objects to the object
+	 * database. The current implementation of ODB transactions leaves
+	 * objects invisible while a transaction is active, so end the
+	 * transaction here if verbose output is enabled.
+	 */
+
+	if (verbose)
+		end_odb_transaction();
+
 	if (read_from_stdin) {
 		struct strbuf buf = STRBUF_INIT;
 		struct strbuf unquoted = STRBUF_INIT;

@@ -1190,6 +1208,12 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 		strbuf_release(&buf);
 	}
 
+	/*
+	 * By now we have added all of the new objects
+	 */
+	if (!verbose)
+		end_odb_transaction();
+
 	if (split_index > 0) {
 		if (git_config_get_split_index() == 0)
 			warning(_("core.splitIndex is set to false; "

gitgitgadget

I thought about this, but I was somewhat worried about the extra cost of "flushing" versus just making things visible immediately if the top-level update-index command knows it's going to need objects to be visible. I'll go ahead and implement flush in the next iteration.

From: Neeraj Singh <[email protected]>

The unpack-objects functionality is used by fetch, push, and fast-import to turn the transfered data into object database entries when there are fewer objects than the 'unpacklimit' setting.

By enabling an odb-transaction when unpacking objects, we can take advantage of batched fsyncs.

Here are some performance numbers to justify batch mode for unpack-objects, collected on a WSL2 Ubuntu VM.

Fsync Mode | Time for 90 objects (ms)
-------------------------------------

       Off | 170
  On,fsync | 760
  On,batch | 230

Note that the default unpackLimit is 100 objects, so there's a 3x benefit in the worst case. The non-batch mode fsync scales linearly with the number of objects, so there are significant benefits even with smaller numbers of objects.

---
 builtin/unpack-objects.c | 3 +++
 1 file changed, 3 insertions(+)

index dbeb0680a58..56d05e2725d 100644
@@ -1,5 +1,6 @@
 #include "builtin.h"
 #include "cache.h"
+#include "bulk-checkin.h"
 #include "config.h"
 #include "object-store.h"
 #include "object.h"

@@ -503,10 +504,12 @@ static void unpack_all(void)
 	if (!quiet)
 		progress = start_progress(_("Unpacking objects"), nr_objects);
 	CALLOC_ARRAY(obj_list, nr_objects);
+	begin_odb_transaction();
 	for (i = 0; i < nr_objects; i++) {
 		unpack_one(i);
 		display_progress(progress, i + 1);
 	}
+	end_odb_transaction();
 	stop_progress(&progress);
 
 	if (delta_list)

gitgitgadget

From: Neeraj Singh <[email protected]>

Git for Windows has defaulted to core.fsyncObjectFiles=true since September 2017. We turn on syncing of loose object files with batch mode in upstream Git so that we can get broad coverage of the new code upstream.

We don't actually do fsyncs in the most of the test suite, since GIT_TEST_FSYNC is set to 0. However, we do exercise all of the surrounding batch mode code since GIT_TEST_FSYNC merely makes the maybe_fsync wrapper always appear to succeed.

---
 cache.h           | 4 ++++
 compat/mingw.h    | 3 +++
 config.c          | 2 +-
 git-compat-util.h | 2 ++
 4 files changed, 10 insertions(+), 1 deletion(-)

index a5bf15a5131..7f6cbb254b4 100644
@@ -1031,6 +1031,10 @@ enum fsync_component {
 			      FSYNC_COMPONENT_INDEX | \
 			      FSYNC_COMPONENT_REFERENCE)
 
+#ifndef FSYNC_COMPONENTS_PLATFORM_DEFAULT
+#define FSYNC_COMPONENTS_PLATFORM_DEFAULT FSYNC_COMPONENTS_DEFAULT
+#endif
+
 /*
  * A bitmask indicating which components of the repo should be fsynced.
  */

index 6074a3d3ced..afe30868c04 100644
@@ -332,6 +332,9 @@ int mingw_getpagesize(void);
 int win32_fsync_no_flush(int fd);
 #define fsync_no_flush win32_fsync_no_flush
 
+#define FSYNC_COMPONENTS_PLATFORM_DEFAULT (FSYNC_COMPONENTS_DEFAULT | FSYNC_COMPONENT_LOOSE_OBJECT)
+#define FSYNC_METHOD_DEFAULT (FSYNC_METHOD_BATCH)
+
 struct rlimit {
 	unsigned int rlim_cur;
 };

index 511f4584eeb..e9cac5f4707 100644
@@ -1342,7 +1342,7 @@ static const struct fsync_component_name {
 
 static enum fsync_component parse_fsync_components(const char *var, const char *string)
 {
-	enum fsync_component current = FSYNC_COMPONENTS_DEFAULT;
+	enum fsync_component current = FSYNC_COMPONENTS_PLATFORM_DEFAULT;
 	enum fsync_component positive = 0, negative = 0;
 
 	while (string) {

index 0892e209a2f..fffe42ce7c1 100644
@@ -1257,11 +1257,13 @@ __attribute__((format (printf, 3, 4))) NORETURN
 void BUG_fl(const char *file, int line, const char *fmt, ...);
 #define BUG(...) BUG_fl(__FILE__, __LINE__, __VA_ARGS__)
 
+#ifndef FSYNC_METHOD_DEFAULT
 #ifdef __APPLE__
 #define FSYNC_METHOD_DEFAULT FSYNC_METHOD_WRITEOUT_ONLY
 #else
 #define FSYNC_METHOD_DEFAULT FSYNC_METHOD_FSYNC
 #endif
+#endif
 
 enum fsync_action {
 	FSYNC_WRITEOUT_ONLY,

gitgitgadget

From: Neeraj Singh <[email protected]>

Several tests use awk to parse OIDs from the output of 'git ls-files --stage' and 'git ls-tree'. Introduce helpers to centralize these uses of awk.

Update t5317-pack-objects-filter-objects.sh to use the new ls-files helper so that it has some usages to review. Other updates are left for the future.

---
 t/t5317-pack-objects-filter-objects.sh | 91 +++++++++++++-------------
 t/test-lib-functions.sh                | 10 +++
 2 files changed, 54 insertions(+), 47 deletions(-)

index 33b740ce628..bb633c9b099 100755
@@ -10,9 +10,6 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 # Test blob:none filter.
 
 test_expect_success 'setup r1' '
-	echo "{print \$1}" >print_1.awk &&
-	echo "{print \$2}" >print_2.awk &&
-
 	git init r1 &&
 	for n in 1 2 3 4 5
 	do

@@ -22,10 +19,13 @@ test_expect_success 'setup r1' '
 	done
 '
 
+parse_verify_pack_blob_oid () {
+	awk '{print $1}' -
+}
+
 test_expect_success 'verify blob count in normal packfile' '
-	git -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 \
-		>ls_files_result &&
-	awk -f print_2.awk ls_files_result |
+	git -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 |
+	test_parse_ls_files_stage_oids |
 	sort >expected &&
 
 	git -C r1 pack-objects --revs --stdout >all.pack <<-EOF &&

@@ -35,7 +35,7 @@ test_expect_success 'verify blob count in normal packfile' '
 
 	git -C r1 verify-pack -v ../all.pack >verify_result &&
 	grep blob verify_result |
-	awk -f print_1.awk |
+	parse_verify_pack_blob_oid |
 	sort >observed &&
 
 	test_cmp expected observed

@@ -54,12 +54,12 @@ test_expect_success 'verify blob:none packfile has no blobs' '
 test_expect_success 'verify normal and blob:none packfiles have same commits/trees' '
 	git -C r1 verify-pack -v ../all.pack >verify_result &&
 	grep -E "commit|tree" verify_result |
-	awk -f print_1.awk |
+	parse_verify_pack_blob_oid |
 	sort >expected &&
 
 	git -C r1 verify-pack -v ../filter.pack >verify_result &&
 	grep -E "commit|tree" verify_result |
-	awk -f print_1.awk |
+	parse_verify_pack_blob_oid |
 	sort >observed &&
 
 	test_cmp expected observed

@@ -123,8 +123,8 @@ test_expect_success 'setup r2' '
 '
 
 test_expect_success 'verify blob count in normal packfile' '
-	git -C r2 ls-files -s large.1000 large.10000 >ls_files_result &&
-	awk -f print_2.awk ls_files_result |
+	git -C r2 ls-files -s large.1000 large.10000 |
+	test_parse_ls_files_stage_oids |
 	sort >expected &&
 
 	git -C r2 pack-objects --revs --stdout >all.pack <<-EOF &&

@@ -134,7 +134,7 @@ test_expect_success 'verify blob count in normal packfile' '
 
 	git -C r2 verify-pack -v ../all.pack >verify_result &&
 	grep blob verify_result |
-	awk -f print_1.awk |
+	parse_verify_pack_blob_oid |
 	sort >observed &&
 
 	test_cmp expected observed

@@ -161,8 +161,8 @@ test_expect_success 'verify blob:limit=1000' '
 '
 
 test_expect_success 'verify blob:limit=1001' '
-	git -C r2 ls-files -s large.1000 >ls_files_result &&
-	awk -f print_2.awk ls_files_result |
+	git -C r2 ls-files -s large.1000 |
+	test_parse_ls_files_stage_oids |
 	sort >expected &&
 
 	git -C r2 pack-objects --revs --stdout --filter=blob:limit=1001 >filter.pack <<-EOF &&

@@ -172,15 +172,15 @@ test_expect_success 'verify blob:limit=1001' '
 
 	git -C r2 verify-pack -v ../filter.pack >verify_result &&
 	grep blob verify_result |
-	awk -f print_1.awk |
+	parse_verify_pack_blob_oid |
 	sort >observed &&
 
 	test_cmp expected observed
 '
 
 test_expect_success 'verify blob:limit=10001' '
-	git -C r2 ls-files -s large.1000 large.10000 >ls_files_result &&
-	awk -f print_2.awk ls_files_result |
+	git -C r2 ls-files -s large.1000 large.10000 |
+	test_parse_ls_files_stage_oids |
 	sort >expected &&
 
 	git -C r2 pack-objects --revs --stdout --filter=blob:limit=10001 >filter.pack <<-EOF &&

@@ -190,15 +190,15 @@ test_expect_success 'verify blob:limit=10001' '
 
 	git -C r2 verify-pack -v ../filter.pack >verify_result &&
 	grep blob verify_result |
-	awk -f print_1.awk |
+	parse_verify_pack_blob_oid |
 	sort >observed &&
 
 	test_cmp expected observed
 '
 
 test_expect_success 'verify blob:limit=1k' '
-	git -C r2 ls-files -s large.1000 >ls_files_result &&
-	awk -f print_2.awk ls_files_result |
+	git -C r2 ls-files -s large.1000 |
+	test_parse_ls_files_stage_oids |
 	sort >expected &&
 
 	git -C r2 pack-objects --revs --stdout --filter=blob:limit=1k >filter.pack <<-EOF &&

@@ -208,15 +208,15 @@ test_expect_success 'verify blob:limit=1k' '
 
 	git -C r2 verify-pack -v ../filter.pack >verify_result &&
 	grep blob verify_result |
-	awk -f print_1.awk |
+	parse_verify_pack_blob_oid |
 	sort >observed &&
 
 	test_cmp expected observed
 '
 
 test_expect_success 'verify explicitly specifying oversized blob in input' '
-	git -C r2 ls-files -s large.1000 large.10000 >ls_files_result &&
-	awk -f print_2.awk ls_files_result |
+	git -C r2 ls-files -s large.1000 large.10000 |
+	test_parse_ls_files_stage_oids |
 	sort >expected &&
 
 	echo HEAD >objects &&

@@ -226,15 +226,15 @@ test_expect_success 'verify explicitly specifying oversized blob in input' '
 
 	git -C r2 verify-pack -v ../filter.pack >verify_result &&
 	grep blob verify_result |
-	awk -f print_1.awk |
+	parse_verify_pack_blob_oid |
 	sort >observed &&
 
 	test_cmp expected observed
 '
 
 test_expect_success 'verify blob:limit=1m' '
-	git -C r2 ls-files -s large.1000 large.10000 >ls_files_result &&
-	awk -f print_2.awk ls_files_result |
+	git -C r2 ls-files -s large.1000 large.10000 |
+	test_parse_ls_files_stage_oids |
 	sort >expected &&
 
 	git -C r2 pack-objects --revs --stdout --filter=blob:limit=1m >filter.pack <<-EOF &&

@@ -244,7 +244,7 @@ test_expect_success 'verify blob:limit=1m' '
 
 	git -C r2 verify-pack -v ../filter.pack >verify_result &&
 	grep blob verify_result |
-	awk -f print_1.awk |
+	parse_verify_pack_blob_oid |
 	sort >observed &&
 
 	test_cmp expected observed

@@ -253,12 +253,12 @@ test_expect_success 'verify blob:limit=1m' '
 test_expect_success 'verify normal and blob:limit packfiles have same commits/trees' '
 	git -C r2 verify-pack -v ../all.pack >verify_result &&
 	grep -E "commit|tree" verify_result |
-	awk -f print_1.awk |
+	parse_verify_pack_blob_oid |
 	sort >expected &&
 
 	git -C r2 verify-pack -v ../filter.pack >verify_result &&
 	grep -E "commit|tree" verify_result |
-	awk -f print_1.awk |
+	parse_verify_pack_blob_oid |
 	sort >observed &&
 
 	test_cmp expected observed

@@ -289,9 +289,8 @@ test_expect_success 'setup r3' '
 '
 
 test_expect_success 'verify blob count in normal packfile' '
-	git -C r3 ls-files -s sparse1 sparse2 dir1/sparse1 dir1/sparse2 \
-		>ls_files_result &&
-	awk -f print_2.awk ls_files_result |
+	git -C r3 ls-files -s sparse1 sparse2 dir1/sparse1 dir1/sparse2 |
+	test_parse_ls_files_stage_oids |
 	sort >expected &&
 
 	git -C r3 pack-objects --revs --stdout >all.pack <<-EOF &&

@@ -301,7 +300,7 @@ test_expect_success 'verify blob count in normal packfile' '
 
 	git -C r3 verify-pack -v ../all.pack >verify_result &&
 	grep blob verify_result |
-	awk -f print_1.awk |
+	parse_verify_pack_blob_oid |
 	sort >observed &&
 
 	test_cmp expected observed

@@ -342,9 +341,8 @@ test_expect_success 'setup r4' '
 '
 
 test_expect_success 'verify blob count in normal packfile' '
-	git -C r4 ls-files -s pattern sparse1 sparse2 dir1/sparse1 dir1/sparse2 \
-		>ls_files_result &&
-	awk -f print_2.awk ls_files_result |
+	git -C r4 ls-files -s pattern sparse1 sparse2 dir1/sparse1 dir1/sparse2 |
+	test_parse_ls_files_stage_oids |
 	sort >expected &&
 
 	git -C r4 pack-objects --revs --stdout >all.pack <<-EOF &&

@@ -354,19 +352,19 @@ test_expect_success 'verify blob count in normal packfile' '
 
 	git -C r4 verify-pack -v ../all.pack >verify_result &&
 	grep blob verify_result |
-	awk -f print_1.awk |
+	parse_verify_pack_blob_oid |
 	sort >observed &&
 
 	test_cmp expected observed
 '
 
 test_expect_success 'verify sparse:oid=OID' '
-	git -C r4 ls-files -s dir1/sparse1 dir1/sparse2 >ls_files_result &&
-	awk -f print_2.awk ls_files_result |
+	git -C r4 ls-files -s dir1/sparse1 dir1/sparse2 |
+	test_parse_ls_files_stage_oids |
 	sort >expected &&
 
 	git -C r4 ls-files -s pattern >staged &&
-	oid=$(awk -f print_2.awk staged) &&
+	oid=$(test_parse_ls_files_stage_oids <staged) &&
 	git -C r4 pack-objects --revs --stdout --filter=sparse:oid=$oid >filter.pack <<-EOF &&
 	HEAD
 	EOF

@@ -374,15 +372,15 @@ test_expect_success 'verify sparse:oid=OID' '
 
 	git -C r4 verify-pack -v ../filter.pack >verify_result &&
 	grep blob verify_result |
-	awk -f print_1.awk |
+	parse_verify_pack_blob_oid |
 	sort >observed &&
 
 	test_cmp expected observed
 '
 
 test_expect_success 'verify sparse:oid=oid-ish' '
-	git -C r4 ls-files -s dir1/sparse1 dir1/sparse2 >ls_files_result &&
-	awk -f print_2.awk ls_files_result |
+	git -C r4 ls-files -s dir1/sparse1 dir1/sparse2 |
+	test_parse_ls_files_stage_oids |
 	sort >expected &&
 
 	git -C r4 pack-objects --revs --stdout --filter=sparse:oid=main:pattern >filter.pack <<-EOF &&

@@ -392,7 +390,7 @@ test_expect_success 'verify sparse:oid=oid-ish' '
 
 	git -C r4 verify-pack -v ../filter.pack >verify_result &&
 	grep blob verify_result |
-	awk -f print_1.awk |
+	parse_verify_pack_blob_oid |
 	sort >observed &&
 
 	test_cmp expected observed

@@ -402,9 +400,8 @@ test_expect_success 'verify sparse:oid=oid-ish' '
 # This models previously omitted objects that we did not receive.
 
 test_expect_success 'setup r1 - delete loose blobs' '
-	git -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 \
-		>ls_files_result &&
-	awk -f print_2.awk ls_files_result |
+	git -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 |
+	test_parse_ls_files_stage_oids |
 	sort >expected &&
 
 	for id in `cat expected | sed "s|..|&/|"`

index a027f0c409e..e6011409e2f 100644
@@ -1782,6 +1782,16 @@ test_oid_to_path () {
 	echo "${1%$basename}/$basename"
 }
 
+# Parse oids from git ls-files --staged output
+test_parse_ls_files_stage_oids () {
+	awk '{print $2}' -
+}
+
+# Parse oids from git ls-tree output
+test_parse_ls_tree_oids () {
+	awk '{print $3}' -
+}
+
 # Choose a port number based on the test script's number and store it in
 # the given variable name, unless that variable already contains a number.
 test_set_port () {

gitgitgadget

From: Neeraj Singh <[email protected]>

Add test cases to exercise batch mode for:
* 'git add'
* 'git stash'
* 'git update-index'
* 'git unpack-objects'

These tests ensure that the added data winds up in the object database.

In this change we introduce a new test helper lib-unique-files.sh. The goal of this library is to create a tree of files that have different oids from any other files that may have been created in the current test repo. This helps us avoid missing validation of an object being added due to it already being in the repo.

---
 t/lib-unique-files.sh  | 34 ++++++++++++++++++++++++++++++++++
 t/t3700-add.sh         | 28 ++++++++++++++++++++++++++++
 t/t3903-stash.sh       | 20 ++++++++++++++++++++
 t/t5300-pack-object.sh | 41 +++++++++++++++++++++++++++--------------
 4 files changed, 109 insertions(+), 14 deletions(-)
 create mode 100644 t/lib-unique-files.sh

new file mode 100644
index 00000000000..34c01a65256
@@ -0,0 +1,34 @@
+# Helper to create files with unique contents
+
+# Create multiple files with unique contents within this test run. Takes the
+# number of directories, the number of files in each directory, and the base
+# directory.
+#
+# test_create_unique_files 2 3 my_dir -- Creates 2 directories with 3 files
+#					 each in my_dir, all with contents
+#					 different from previous invocations
+#					 of this command in this run.
+
+test_create_unique_files () {
+	test "$#" -ne 3 && BUG "3 param"
+
+	local dirs="$1" &&
+	local files="$2" &&
+	local basedir="$3" &&
+	local counter=0 &&
+	local i &&
+	local j &&
+	test_tick &&
+	local basedata=$basedir$test_tick &&
+	rm -rf "$basedir" &&
+	for i in $(test_seq $dirs)
+	do
+		local dir=$basedir/dir$i &&
+		mkdir -p "$dir" &&
+		for j in $(test_seq $files)
+		do
+			counter=$((counter + 1)) &&
+			echo "$basedata.$counter">"$dir/file$j.txt"
+		done
+	done
+}

index b1f90ba3250..8979c8a5f03 100755
@@ -8,6 +8,8 @@ test_description='Test of git add, including the -- option.'
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
+. $TEST_DIRECTORY/lib-unique-files.sh
+
 # Test the file mode "$1" of the file "$2" in the index.
 test_mode_in_index () {
 	case "$(git ls-files -s "$2")" in

@@ -34,6 +36,32 @@ test_expect_success \
     'Test that "git add -- -q" works' \
     'touch -- -q && git add -- -q'
 
+BATCH_CONFIGURATION='-c core.fsync=loose-object -c core.fsyncmethod=batch'
+
+test_expect_success 'git add: core.fsyncmethod=batch' "
+	test_create_unique_files 2 4 files_base_dir1 &&
+	GIT_TEST_FSYNC=1 git $BATCH_CONFIGURATION add -- ./files_base_dir1/ &&
+	git ls-files --stage files_base_dir1/ |
+	test_parse_ls_files_stage_oids >added_files_oids &&
+
+	# We created 2 subdirs with 4 files each (8 files total) above
+	test_line_count = 8 added_files_oids &&
+	git cat-file --batch-check='%(objectname)' <added_files_oids >added_files_actual &&
+	test_cmp added_files_oids added_files_actual
+"
+
+test_expect_success 'git update-index: core.fsyncmethod=batch' "
+	test_create_unique_files 2 4 files_base_dir2 &&
+	find files_base_dir2 ! -type d -print | xargs git $BATCH_CONFIGURATION update-index --add -- &&
+	git ls-files --stage files_base_dir2 |
+	test_parse_ls_files_stage_oids >added_files2_oids &&
+
+	# We created 2 subdirs with 4 files each (8 files total) above
+	test_line_count = 8 added_files2_oids &&
+	git cat-file --batch-check='%(objectname)' <added_files2_oids >added_files2_actual &&
+	test_cmp added_files2_oids added_files2_actual
+"
+
 test_expect_success \
 	'git add: Test that executable bit is not used if core.filemode=0' \
 	'git config core.filemode 0 &&

index 4abbc8fccae..20e94881964 100755
@@ -9,6 +9,7 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
 . ./test-lib.sh
+. $TEST_DIRECTORY/lib-unique-files.sh
 
 test_expect_success 'usage on cmd and subcommand invalid option' '
 	test_expect_code 129 git stash --invalid-option 2>usage &&

@@ -1410,6 +1411,25 @@ test_expect_success 'stash handles skip-worktree entries nicely' '
 	git rev-parse --verify refs/stash:A.t
 '
 
+
+BATCH_CONFIGURATION='-c core.fsync=loose-object -c core.fsyncmethod=batch'
+
+test_expect_success 'stash with core.fsyncmethod=batch' "
+	test_create_unique_files 2 4 files_base_dir &&
+	GIT_TEST_FSYNC=1 git $BATCH_CONFIGURATION stash push -u -- ./files_base_dir/ &&
+
+	# The files were untracked, so use the third parent,
+	# which contains the untracked files
+	git ls-tree -r stash^3 -- ./files_base_dir/ |
+	test_parse_ls_tree_oids >stashed_files_oids &&
+
+	# We created 2 dirs with 4 files each (8 files total) above
+	test_line_count = 8 stashed_files_oids &&
+	git cat-file --batch-check='%(objectname)' <stashed_files_oids >stashed_files_actual &&
+	test_cmp stashed_files_oids stashed_files_actual
+"
+
+
 test_expect_success 'git stash succeeds despite directory/file change' '
 	test_create_repo directory_file_switch_v1 &&
 	(

gitgitgadget

Whoops.. I guess I wasn't looking widely enough. I noticed failures on my Ubuntu system that uses dash.

Thanks for updating me on the documentation. I think I'll go back to
one-line assignments with the proper quotations and single variables, just to keep things short and consistent. I've been looking up everything as I go with these shell scripts, but there's a lot of subtlety. CMD batch files are even uglier, but I've had many years to learn the common practices there.

I'll ask a question on the perf test patch on a related shell topic.

Thanks,
Neeraj

From: Neeraj Singh <[email protected]>

Tests that affect the repo in stateful ways are easier to write if we can run setup steps outside of the measured portion of perf iteration.

This change adds a "--setup 'setup-script'" parameter to test_perf. To make invocations easier to understand, I also moved the prerequisites to a new --prereq parameter.

The setup facility will be used in the upcoming perf tests for batch mode, but it already helps in some existing tests, like t5302 and t7820.

---
 t/perf/p4220-log-grep-engines.sh       |  3 +-
 t/perf/p4221-log-grep-engines-fixed.sh |  3 +-
 t/perf/p5302-pack-index.sh             | 15 +++----
 t/perf/p7519-fsmonitor.sh              | 18 ++------
 t/perf/p7820-grep-engines.sh           |  6 ++-
 t/perf/perf-lib.sh                     | 62 +++++++++++++++++++++++---
 6 files changed, 74 insertions(+), 33 deletions(-)

index 2bc47ded4d1..03fbfbb85d3 100755
@@ -36,7 +36,8 @@ do
 		else
 			prereq=""
 		fi
-		test_perf $prereq "$engine log$GIT_PERF_4220_LOG_OPTS --grep='$pattern'" "
+		test_perf "$engine log$GIT_PERF_4220_LOG_OPTS --grep='$pattern'" \
+			--prereq "$prereq" "
 			git -c grep.patternType=$engine log --pretty=format:%h$GIT_PERF_4220_LOG_OPTS --grep='$pattern' >'out.$engine' || :
 		"
 	done

index 060971265a9..0a6d6dfc219 100755
@@ -26,7 +26,8 @@ do
 		else
 			prereq=""
 		fi
-		test_perf $prereq "$engine log$GIT_PERF_4221_LOG_OPTS --grep='$pattern'" "
+		test_perf "$engine log$GIT_PERF_4221_LOG_OPTS --grep='$pattern'" \
+			--prereq "$prereq" "
 			git -c grep.patternType=$engine log --pretty=format:%h$GIT_PERF_4221_LOG_OPTS --grep='$pattern' >'out.$engine' || :
 		"
 	done

index c16f6a3ff69..14c601bbf86 100755
@@ -26,9 +26,8 @@ test_expect_success 'set up thread-counting tests' '
 	done
 '
 
-test_perf PERF_EXTRA 'index-pack 0 threads' '
-	rm -rf repo.git &&
-	git init --bare repo.git &&
+test_perf 'index-pack 0 threads' --prereq PERF_EXTRA \
+	--setup 'rm -rf repo.git && git init --bare repo.git' '
 	GIT_DIR=repo.git git index-pack --threads=1 --stdin < $PACK
 '
 

@@ -36,17 +35,15 @@ for t in $threads
 do
 	THREADS=$t
 	export THREADS
-	test_perf PERF_EXTRA "index-pack $t threads" '
-		rm -rf repo.git &&
-		git init --bare repo.git &&
+	test_perf "index-pack $t threads" --prereq PERF_EXTRA \
+		--setup 'rm -rf repo.git && git init --bare repo.git' '
 		GIT_DIR=repo.git GIT_FORCE_THREADS=1 \
 		git index-pack --threads=$THREADS --stdin <$PACK
 	'
 done
 
-test_perf 'index-pack default number of threads' '
-	rm -rf repo.git &&
-	git init --bare repo.git &&
+test_perf 'index-pack default number of threads' \
+	--setup 'rm -rf repo.git && git init --bare repo.git' '
 	GIT_DIR=repo.git git index-pack --stdin < $PACK
 '
 

index c8be58f3c76..5b489c968b8 100755
@@ -60,18 +60,6 @@ then
 	esac
 fi
 
-if test -n "$GIT_PERF_7519_DROP_CACHE"
-then
-	# When using GIT_PERF_7519_DROP_CACHE, GIT_PERF_REPEAT_COUNT must be 1 to
-	# generate valid results. Otherwise the caching that happens for the nth
-	# run will negate the validity of the comparisons.
-	if test "$GIT_PERF_REPEAT_COUNT" -ne 1
-	then
-		echo "warning: Setting GIT_PERF_REPEAT_COUNT=1" >&2
-		GIT_PERF_REPEAT_COUNT=1
-	fi
-fi
-
 trace_start() {
 	if test -n "$GIT_PERF_7519_TRACE"
 	then

@@ -167,10 +155,10 @@ setup_for_fsmonitor() {
 
 test_perf_w_drop_caches () {
 	if test -n "$GIT_PERF_7519_DROP_CACHE"; then
-		test-tool drop-caches
+		test_perf "$1" --setup "test-tool drop-caches" "$2"
+	else
+		test_perf "$@"
 	fi
-
-	test_perf "$@"
 }
 
 test_fsmonitor_suite() {

index 8b09c5bf328..9bfb86842a9 100755
@@ -49,13 +49,15 @@ do
 		fi
 		if ! test_have_prereq PERF_GREP_ENGINES_THREADS
 		then
-			test_perf $prereq "$engine grep$GIT_PERF_7820_GREP_OPTS '$pattern'" "
+			test_perf "$engine grep$GIT_PERF_7820_GREP_OPTS '$pattern'" \
+				--prereq "$prereq" "
 				git -c grep.patternType=$engine grep$GIT_PERF_7820_GREP_OPTS -- '$pattern' >'out.$engine' || :
 			"
 		else
 			for threads in $GIT_PERF_GREP_THREADS
 			do
-				test_perf PTHREADS,$prereq "$engine grep$GIT_PERF_7820_GREP_OPTS '$pattern' with $threads threads" "
+				test_perf "$engine grep$GIT_PERF_7820_GREP_OPTS '$pattern' with $threads threads"
+					--prereq PTHREADS,$prereq "
 					git -c grep.patternType=$engine -c grep.threads=$threads grep$GIT_PERF_7820_GREP_OPTS -- '$pattern' >'out.$engine.$threads' || :
 				"
 			done

gitgitgadget

From: Neeraj Singh <[email protected]>

Add basic performance tests for git commands that can add data to the object database. We cover:
* git add
* git stash
* git update-index (via git stash)
* git unpack-objects
* git commit --all

We cover all currently available fsync methods as well.

---
 t/perf/p0008-odb-fsync.sh | 81 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)
 create mode 100755 t/perf/p0008-odb-fsync.sh

new file mode 100755
index 00000000000..87092c2627e
@@ -0,0 +1,81 @@
+#!/bin/sh
+#
+# This test measures the performance of adding new files to the object
+# database. The test was originally added to measure the effect of the
+# 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 adding things to the object database"
+
+. ./perf-lib.sh
+
+. $TEST_DIRECTORY/lib-unique-files.sh
+
+test_perf_fresh_repo
+test_checkout_worktree
+
+dir_count=10
+files_per_dir=50
+total_files=$((dir_count * files_per_dir))
+
+populate_files () {
+	test_create_unique_files $dir_count $files_per_dir files
+}
+
+setup_repo () {
+	(rm -rf .git || 1) &&
+	git init &&
+	test_commit first &&
+	populate_files
+}
+
+test_perf_fsync_cfgs () {
+	local method cfg &&
+	for method in none fsync batch writeout-only
+	do
+		case $method in
+		none)
+			cfg="-c core.fsync=none"
+			;;
+		*)
+			cfg="-c core.fsync=loose-object -c core.fsyncMethod=$method"
+		esac &&
+
+		# Set GIT_TEST_FSYNC=1 explicitly since fsync is normally
+		# disabled by t/test-lib.sh.
+		if ! test_perf "$1 (fsyncMethod=$method)" \
+						--setup "$2" \
+						"GIT_TEST_FSYNC=1 git $cfg $3"
+		then
+			break
+		fi
+	done
+}
+
+test_perf_fsync_cfgs "add $total_files files" \
+	"setup_repo" \
+	"add -- files"
+
+test_perf_fsync_cfgs "stash $total_files files" \
+	"setup_repo" \
+	"stash push -u -- files"
+
+test_perf_fsync_cfgs "unpack $total_files files" \
+	"
+	setup_repo &&
+	git -c core.fsync=none add -- files &&
+	git -c core.fsync=none commit -q -m second &&
+	echo HEAD | git pack-objects -q --stdout --revs >test_pack.pack &&
+	setup_repo
+	" \
+	"unpack-objects -q <test_pack.pack"
+
+test_perf_fsync_cfgs "commit $total_files files" \
+	"
+	setup_repo &&
+	git -c core.fsync=none add -- files &&
+	populate_files
+	" \
+	"commit -q -a -m test"
+
+test_done

gitgitgadget

From: Neeraj Singh <[email protected]>

The warning for an unrecognized fsyncMethod was not camel-cased.

---
 config.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index e9cac5f4707..ae819dee20b 100644
@@ -1697,7 +1697,7 @@ static int git_default_core_config(const char *var, const char *value, void *cb)
 
 	if (!strcmp(var, "core.fsyncobjectfiles")) {
 		if (fsync_object_files < 0)
-			warning(_("core.fsyncobjectfiles is deprecated; use core.fsync instead"));
+			warning(_("core.fsyncObjectFiles is deprecated; use core.fsync instead"));
 		fsync_object_files = git_config_bool(var, value);
 		return 0;
 	}

gitgitgadget

From: Neeraj Singh <[email protected]>

This commit prepares for adding batch-fsync to the bulk-checkin infrastructure.

The bulk-checkin infrastructure is currently used to batch up addition of large blobs to a packfile. When a blob is larger than big_file_threshold, we unconditionally add it to a pack. If bulk checkins are 'plugged', we allow multiple large blobs to be added to a single pack until we reach the packfile size limit; otherwise, we simply make a new packfile for each large blob. The 'unplug' call tells us when the series of blob additions is done so that we can finish the packfiles and make their objects available to subsequent operations.

Stated another way, bulk-checkin allows callers to define a transaction that adds multiple objects to the object database, where the object database can optimize its internal operations within the transaction boundary.

Batched fsync will fit into bulk-checkin by taking advantage of the plug/unplug functionality to determine the appropriate time to fsync and make newly-added objects available in the primary object database.

* Rename 'state' variable to 'bulk_checkin_packfile', since we will later be adding 'bulk_fsync_objdir'. This also makes the variable easier to find in the debugger, since the name is more unique.

* Rename finish_bulk_checkin to flush_bulk_checkin_packfile and call it unconditionally from unplug_bulk_checkin. Internally it will conditionally do a flush if there's any work to do.

* Move the 'plugged' data member of 'bulk_checkin_state' into a separate static variable. Doing this avoids resetting the variable in finish_bulk_checkin when zeroing the 'bulk_checkin_state'. As-is, we seem to unintentionally disable the plugging functionality the first time a new packfile must be created due to packfile size limits. While disabling the plugging state only results in suboptimal behavior for the current code, it would be fatal for the bulk-fsync functionality later in this patch series.

The net effect of these changes is to make a clear separation between the portion of the bulk-checkin infrastructure that is related to the packfile (nearly all of it at present) and the part that is related to other future optimizations of the ODB.

---
 bulk-checkin.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

index 6d6c37171c9..88d72178b2c 100644
@@ -10,9 +10,9 @@
 #include "packfile.h"
 #include "object-store.h"
 
-static struct bulk_checkin_state {
-	unsigned plugged:1;
+static int bulk_checkin_plugged;
 
+static struct bulk_checkin_packfile {
 	char *pack_tmp_name;
 	struct hashfile *f;
 	off_t offset;

@@ -21,7 +21,7 @@ static struct bulk_checkin_state {
 	struct pack_idx_entry **written;
 	uint32_t alloc_written;
 	uint32_t nr_written;
-} state;
+} bulk_checkin_packfile;
 
 static void finish_tmp_packfile(struct strbuf *basename,
 				const char *pack_tmp_name,

@@ -39,7 +39,7 @@ static void finish_tmp_packfile(struct strbuf *basename,
 	free(idx_tmp_name);
 }
 
-static void finish_bulk_checkin(struct bulk_checkin_state *state)
+static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)
 {
 	unsigned char hash[GIT_MAX_RAWSZ];
 	struct strbuf packname = STRBUF_INIT;

@@ -80,7 +80,7 @@ static void finish_bulk_checkin(struct bulk_checkin_state *state)
 	reprepare_packed_git(the_repository);
 }
 
-static int already_written(struct bulk_checkin_state *state, struct object_id *oid)
+static int already_written(struct bulk_checkin_packfile *state, struct object_id *oid)
 {
 	int i;
 

@@ -112,7 +112,7 @@ static int already_written(struct bulk_checkin_state *state, struct object_id *o
  * status before calling us just in case we ask it to call us again
  * with a new pack.
  */
-static int stream_to_pack(struct bulk_checkin_state *state,
+static int stream_to_pack(struct bulk_checkin_packfile *state,
 			  git_hash_ctx *ctx, off_t *already_hashed_to,
 			  int fd, size_t size, enum object_type type,
 			  const char *path, unsigned flags)

@@ -189,7 +189,7 @@ static int stream_to_pack(struct bulk_checkin_state *state,
 }
 
 /* Lazily create backing packfile for the state */
-static void prepare_to_stream(struct bulk_checkin_state *state,
+static void prepare_to_stream(struct bulk_checkin_packfile *state,
 			      unsigned flags)
 {
 	if (!(flags & HASH_WRITE_OBJECT) || state->f)

@@ -204,7 +204,7 @@ static void prepare_to_stream(struct bulk_checkin_state *state,
 		die_errno("unable to write pack header");
 }
 
-static int deflate_to_pack(struct bulk_checkin_state *state,
+static int deflate_to_pack(struct bulk_checkin_packfile *state,
 			   struct object_id *result_oid,
 			   int fd, size_t size,
 			   enum object_type type, const char *path,

@@ -251,7 +251,7 @@ static int deflate_to_pack(struct bulk_checkin_state *state,
 			BUG("should not happen");
 		hashfile_truncate(state->f, &checkpoint);
 		state->offset = checkpoint.offset;
-		finish_bulk_checkin(state);
+		flush_bulk_checkin_packfile(state);
 		if (lseek(fd, seekback, SEEK_SET) == (off_t) -1)
 			return error("cannot seek back");
 	}

@@ -278,21 +278,22 @@ int index_bulk_checkin(struct object_id *oid,
 		       int fd, size_t size, enum object_type type,
 		       const char *path, unsigned flags)
 {
-	int status = deflate_to_pack(&state, oid, fd, size, type,
+	int status = deflate_to_pack(&bulk_checkin_packfile, oid, fd, size, type,
 				     path, flags);
-	if (!state.plugged)
-		finish_bulk_checkin(&state);
+	if (!bulk_checkin_plugged)
+		flush_bulk_checkin_packfile(&bulk_checkin_packfile);
 	return status;
 }
 
 void plug_bulk_checkin(void)
 {
-	state.plugged = 1;
+	assert(!bulk_checkin_plugged);
+	bulk_checkin_plugged = 1;
 }
 
 void unplug_bulk_checkin(void)
 {
-	state.plugged = 0;
-	if (state.f)
-		finish_bulk_checkin(&state);
+	assert(bulk_checkin_plugged);
+	bulk_checkin_plugged = 0;
+	flush_bulk_checkin_packfile(&bulk_checkin_packfile);
 }

2.34.1.78.g86e39b8f8d

From: Neeraj Singh <[email protected]>

GGG closed this series erroneously, so I'm trying out git-send-email. Apologies for any mistakes.

This series is also available at https://github.com/neerajsi-msft/git/git.git ns/batched-fsync-v6.

V6 changes:

* Based on master at faa21c1 to pick up ns/fsync-or-die-message-fix. Also resolved a conflict with 8aa0209 in t/perf/p7519-fsmonitor.sh.

* Some independent patches were submitted separately on-list. This series is now dependent on ns/fsync-or-die-message-fix.

* Rename bulk_checkin_state to bulk_checkin_packfile to discourage future authors from adding any non-packfile related stuff to it. Each individual component of bulk_checkin should have its own state variable(s) going forward, and they should only be tied together by odb_transaction_nesting.

* Rename finish_bulk_checkin and do_batch_fsync to flush_bulk_checkin and flush_batch_fsync. The "finish" step is going to be the end_odb_transaction. The "flush" terminology should be consistently used for making changes visible.

* Add flush_odb_transaction and use it in update-index before printing verbose output to mitigate risk of missing objects for a tricky stdin feeder.

* Re-add shell "local with assignment": now these are all on a separate line with quotes around any values, to comply with dash. I'm running on ubuntu 20.04 LTS where I saw some of the dash issues before.

Neeraj Singh (12):
bulk-checkin: rename 'state' variable and separate 'plugged' boolean
bulk-checkin: rebrand plug/unplug APIs as 'odb transactions' core.fsyncmethod: batched disk flushes for loose-objects
cache-tree: use ODB transaction around writing a tree builtin/add: add ODB transaction around add_files_to_cache
update-index: use the bulk-checkin infrastructure
unpack-objects: use the bulk-checkin infrastructure core.fsync: use batch mode and sync loose objects by default on
Windows
test-lib-functions: add parsing helpers for ls-files and ls-tree core.fsyncmethod: tests for batch mode
t/perf: add iteration setup mechanism to perf-lib core.fsyncmethod: performance tests for batch mode

 Documentation/config/core.txt          |   8 ++
 builtin/add.c                          |  13 ++-
 builtin/unpack-objects.c               |   3 +
 builtin/update-index.c                 |  20 +++++
 bulk-checkin.c                         | 117 +++++++++++++++++++++----
 bulk-checkin.h                         |  27 +++++-
 cache-tree.c                           |   3 +
 cache.h                                |  12 ++-
 compat/mingw.h                         |   3 +
 config.c                               |   4 +-
 git-compat-util.h                      |   2 +
 object-file.c                          |   7 +-
 t/lib-unique-files.sh                  |  34 +++++++
 t/perf/p0008-odb-fsync.sh              |  82 +++++++++++++++++
 t/perf/p4220-log-grep-engines.sh       |   3 +-
 t/perf/p4221-log-grep-engines-fixed.sh |   3 +-
 t/perf/p5302-pack-index.sh             |  15 ++--
 t/perf/p7519-fsmonitor.sh              |  18 +---
 t/perf/p7820-grep-engines.sh           |   6 +-
 t/perf/perf-lib.sh                     |  63 +++++++++++--
 t/t3700-add.sh                         |  28 ++++++
 t/t3903-stash.sh                       |  20 +++++
 t/t5300-pack-object.sh                 |  41 ++++++---
 t/t5317-pack-objects-filter-objects.sh |  91 ++++++++++---------
 t/test-lib-functions.sh                |  10 +++
 25 files changed, 513 insertions(+), 120 deletions(-)
 create mode 100644 t/lib-unique-files.sh
 create mode 100755 t/perf/p0008-odb-fsync.sh

Range-diff against v5:

 1:  c7a2a7efe6d <  -:  ----------- bulk-checkin: rename 'state' variable and separate 'plugged' boolean
 -:  ----------- >  1:  adabdaa0290 bulk-checkin: rename 'state' variable and separate 'plugged' boolean
 2:  d045b13795b !  2:  72a6cd36c9c bulk-checkin: rebrand plug/unplug APIs as 'odb transactions'

@@ Commit message
writing code can optimize their operations without caring whether the top-level code has a transaction active.

+ Add a flush_odb_transaction API that will be used in update-index to
+ make objects visible even if a transaction is active. The flush call may
+ also be useful in future cases if we hold a transaction active around
+ calling hooks.
+

## builtin/add.c ##
@@ bulk-checkin.c
-static int bulk_checkin_plugged;
+static int odb_transaction_nesting;

- static struct bulk_checkin_state {
+ static struct bulk_checkin_packfile {
char *pack_tmp_name;
@@ bulk-checkin.c: int index_bulk_checkin(struct object_id *oid,
{
- int status = deflate_to_pack(&bulk_checkin_state, oid, fd, size, type,
+ int status = deflate_to_pack(&bulk_checkin_packfile, oid, fd, size, type,
path, flags);
- if (!bulk_checkin_plugged)
+ if (!odb_transaction_nesting)
- finish_bulk_checkin(&bulk_checkin_state);
+ flush_bulk_checkin_packfile(&bulk_checkin_packfile);
return status;
}

@@ bulk-checkin.c: int index_bulk_checkin(struct object_id *oid,
}

-void unplug_bulk_checkin(void)
-+void end_odb_transaction(void)
++void flush_odb_transaction(void)
{
- assert(bulk_checkin_plugged);
- bulk_checkin_plugged = 0;
+ flush_bulk_checkin_packfile(&bulk_checkin_packfile);
+ }
++
++void end_odb_transaction(void)
++{
+ odb_transaction_nesting -= 1;
+ if (odb_transaction_nesting < 0)
+ BUG("Unbalanced ODB transaction nesting");
@@ bulk-checkin.c: int index_bulk_checkin(struct object_id *oid,
+ if (odb_transaction_nesting)
+ return;
+
- if (bulk_checkin_state.f)
- finish_bulk_checkin(&bulk_checkin_state);
- }
++ flush_odb_transaction();
++}

## bulk-checkin.h ##
@@ bulk-checkin.h: int index_bulk_checkin(struct object_id *oid,
@@ bulk-checkin.h: int index_bulk_checkin(struct object_id *oid,
+/*
+ * Tell the object database to optimize for adding
+ * multiple objects. end_odb_transaction must be called
-+ * to make new objects visible.
++ * to make new objects visible. Transactions can be nested, ++ * and objects are only visible after the outermost transaction ++ * is complete or the transaction is flushed.
+ */
+void begin_odb_transaction(void);
+
+/*
++ * Make any objects that are currently part of a pending object ++ * database transaction visible. It is valid to call this function ++ * even if no transaction is active.
++ */
++void flush_odb_transaction(void);
++
++/*
+ * Tell the object database to make any objects from the
-+ * current transaction visible.
++ * current transaction visible if this is the final nested ++ * transaction.
+ */
+void end_odb_transaction(void);

 3:  2d1bc4568ac <  -:  ----------- object-file: pass filename to fsync_or_die
 4:  9e7ae22fa4a !  3:  57539f104ef core.fsyncmethod: batched disk flushes for loose-objects

@@ Documentation/config/core.txt: core.fsyncMethod::
+ updates in the disk writeback cache and then does a single full fsync of
+ a dummy file to trigger the disk cache flush at the end of the operation. ++
-+ Currently batch mode only applies to loose-object files. Other repository -+ data is made durable as if fsync was specified. This mode is expected to -+ be as safe as fsync on macOS for repos stored on HFS+ or APFS filesystems -+ and on Windows for repos stored on NTFS or ReFS filesystems. ++Currently batch mode only applies to loose-object files. Other repository ++data is made durable as if fsync was specified. This mode is expected to ++be as safe as fsync on macOS for repos stored on HFS+ or APFS filesystems ++and on Windows for repos stored on NTFS or ReFS filesystems.

core.fsyncObjectFiles::
This boolean will enable 'fsync()' when writing object files.
@@ bulk-checkin.c

+static struct tmp_objdir *bulk_fsync_objdir;
+
- static struct bulk_checkin_state {
+ static struct bulk_checkin_packfile {
char *pack_tmp_name;
struct hashfile *f;
-@@ bulk-checkin.c: static void finish_bulk_checkin(struct bulk_checkin_state *state) +@@ bulk-checkin.c: static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)
reprepare_packed_git(the_repository);
}

+/*
+ * Cleanup after batch-mode fsync_object_files.
+ */
-+static void do_batch_fsync(void)
++static void flush_batch_fsync(void)
+{
+ struct strbuf temp_path = STRBUF_INIT;
+ struct tempfile *temp;
@@ bulk-checkin.c: static void finish_bulk_checkin(struct bulk_checkin_state *state
+ bulk_fsync_objdir = NULL;
+}
+
- static int already_written(struct bulk_checkin_state *state, struct object_id *oid)
+ static int already_written(struct bulk_checkin_packfile *state, struct object_id *oid) {
int i;
-@@ bulk-checkin.c: static int deflate_to_pack(struct bulk_checkin_state *state, +@@ bulk-checkin.c: static int deflate_to_pack(struct bulk_checkin_packfile *state,
return 0;
}

@@ bulk-checkin.c: static int deflate_to_pack(struct bulk_checkin_state *state,
+ * If we have an active ODB transaction, we issue a call that
+ * cleans the filesystem page cache but avoids a hardware flush
+ * command. Later on we will issue a single hardware flush
-+ * before as part of do_batch_fsync.
++ * before renaming the objects to their final names as part of
++ * flush_batch_fsync.
+ */
+ if (!bulk_fsync_objdir ||
+ git_fsync(fd, FSYNC_WRITEOUT_ONLY) < 0) {
@@ bulk-checkin.c: static int deflate_to_pack(struct bulk_checkin_state *state,
int index_bulk_checkin(struct object_id *oid,
int fd, size_t size, enum object_type type, const char *path, unsigned flags)
-@@ bulk-checkin.c: void end_odb_transaction(void)
+@@ bulk-checkin.c: void begin_odb_transaction(void)

- if (bulk_checkin_state.f)
- finish_bulk_checkin(&bulk_checkin_state);
-+
-+ do_batch_fsync();
+ void flush_odb_transaction(void)
+ {
++ flush_batch_fsync();
+ flush_bulk_checkin_packfile(&bulk_checkin_packfile);
}
+

## bulk-checkin.h ##
@@

 5:  83fa4a5f3a5 =  4:  f47631e6a28 cache-tree: use ODB transaction around writing a tree
 6:  d514842ad49 =  5:  08c9b234942 builtin/add: add ODB transaction around add_files_to_cache
 7:  8cac94598a5 !  6:  bc37cdbd226 update-index: use the bulk-checkin infrastructure

@@ Commit message
There is some risk with this change, since under batch fsync, the object files will be in a tmp-objdir until update-index is complete, so callers using the --stdin option will not see them until update-index is done.
- This risk is mitigated by not keeping an ODB transaction open around
- --stdin processing if in --verbose mode. Without --verbose mode,
- a caller feeding update-index via --stdin wouldn't know when
- update-index adds an object, event without an ODB transaction.
+ This risk is mitigated by flushing the ODB transaction prior to
+ reporting any verbose output so that objects will be visible to callers
+ that are synchronizing with update-index by snooping its output.

@@ builtin/update-index.c
#include "config.h"
#include "lockfile.h"
#include "quote.h"
+@@ builtin/update-index.c: static void report(const char *fmt, ...)
+ if (!verbose)
+ return;
+
++ /*
++ * It is possible, though unlikely, that a caller could use the verbose
++ * output to synchronize with addition of objects to the object
++ * database. The current implementation of ODB transactions leaves
++ * objects invisible while a transaction is active, so flush the
++ * transaction here before reporting a change made by update-index.
++ */
++ flush_odb_transaction();
+ va_start(vp, fmt);
+ vprintf(fmt, vp);
+ putchar('\n');
@@ builtin/update-index.c: int cmd_update_index(int argc, const char **argv, const char *prefix)
*/
parse_options_start(&ctx, argc, argv, prefix,
@@ builtin/update-index.c: int cmd_update_index(int argc, const char **argv, const
while (ctx.argc) {
if (parseopt_state != PARSE_OPT_DONE)
parseopt_state = parse_options_step(&ctx, options,
-@@ builtin/update-index.c: int cmd_update_index(int argc, const char **argv, const char *prefix)
- the_index.version = preferred_index_format;
- }
-
-+ /*
-+ * It is possible, though unlikely, that a caller could use the verbose
-+ * output to synchronize with addition of objects to the object
-+ * database. The current implementation of ODB transactions leaves
-+ * objects invisible while a transaction is active, so end the
-+ * transaction here if verbose output is enabled.
-+ */
-+
-+ if (verbose)
-+ end_odb_transaction();
-+
- if (read_from_stdin) {
- struct strbuf buf = STRBUF_INIT;
- struct strbuf unquoted = STRBUF_INIT;
@@ builtin/update-index.c: int cmd_update_index(int argc, const char **argv, const char *prefix)
strbuf_release(&buf);
}
@@ builtin/update-index.c: int cmd_update_index(int argc, const char **argv, const
+ /*
+ * By now we have added all of the new objects
+ */
-+ if (!verbose)
-+ end_odb_transaction();
++ end_odb_transaction();
+
if (split_index > 0) {
if (git_config_get_split_index() == 0)

 8:  523e5fbd63e =  7:  9cf584cdb67 unpack-objects: use the bulk-checkin infrastructure
 9:  faacc19aab2 =  8:  5039b596064 core.fsync: use batch mode and sync loose objects by default on Windows
10:  4de7300a7b0 =  9:  67205b3ac25 test-lib-functions: add parsing helpers for ls-files and ls-tree
11:  1a4aff8c350 ! 10:  148e562ddb8 core.fsyncmethod: tests for batch mode
    @@ t/lib-unique-files.sh (new)
     +	local dirs="$1" &&
     +	local files="$2" &&
     +	local basedir="$3" &&
    -+	local counter=0 &&
    ++	local counter="0" &&
     +	local i &&
     +	local j &&
     +	test_tick &&
    -+	local basedata=$basedir$test_tick &&
    ++	local basedata="$basedir$test_tick" &&
     +	rm -rf "$basedir" &&
     +	for i in $(test_seq $dirs)
     +	do
    -+		local dir=$basedir/dir$i &&
    ++		local dir="$basedir/dir$i" &&
     +		mkdir -p "$dir" &&
     +		for j in $(test_seq $files)
     +		do
12:  47cc63e1dda ! 11:  1a8320828f7 t/perf: add iteration setup mechanism to perf-lib
    @@ t/perf/p7519-fsmonitor.sh: then
     -	fi
     -fi
     -
    - trace_start() {
    + trace_start () {
      	if test -n "$GIT_PERF_7519_TRACE"
      	then
    -@@ t/perf/p7519-fsmonitor.sh: setup_for_fsmonitor() {
    +@@ t/perf/p7519-fsmonitor.sh: setup_for_fsmonitor_hook () {
      
      test_perf_w_drop_caches () {
      	if test -n "$GIT_PERF_7519_DROP_CACHE"; then
    @@ t/perf/p7519-fsmonitor.sh: setup_for_fsmonitor() {
     -	test_perf "$@"
      }
      
    - test_fsmonitor_suite() {
    + test_fsmonitor_suite () {
     
      ## t/perf/p7820-grep-engines.sh ##
     @@ t/perf/p7820-grep-engines.sh: do
    @@ t/perf/perf-lib.sh: exit $ret' >&3 2>&4
      }
      
      test_wrapper_ () {
    -+	local test_wrapper_func_ test_title_
    - 	test_wrapper_func_=$1; shift
    -+	test_title_=$1; shift
    +-	test_wrapper_func_=$1; shift
    ++	local test_wrapper_func_="$1"; shift
    ++	local test_title_="$1"; shift
      	test_start_
     -	test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
     -	test "$#" = 2 ||
13:  26be6ecb28b ! 12:  3eb5a6720cb core.fsyncmethod: performance tests for batch mode
    @@ t/perf/p0008-odb-fsync.sh (new)
     +}
     +
     +test_perf_fsync_cfgs () {
    -+	local method cfg &&
    ++	local method &&
    ++	local cfg &&
     +	for method in none fsync batch writeout-only
     +	do
     +		case $method in

14: 88c1f84d4c3 < -: ----------- core.fsyncmethod: correctly camel-case warning message

2.34.1.78.g86e39b8f8d

Thanks. These looked all sensible.

Wil queue.

We haven't heard anything on this topic from anybody for this round. I am planning to merge it to 'next' soonish.

Please speak up if anybody has concerns.

Thanks.

No updates on my end. I'll keep my eyes out for any reports of regression.

Thanks,
Neeraj

Hi Neeraj,

I asked a colleague to have a go with these patches and the only concern I heard back was that with ext4's new fast_commit feature, the fsync seems not to actually flush all metadata. They indicated that they'd be happy with merely documenting this issue, and also pointed out that the fast_commit feature seems still not to be considered ready for production workloads.

So: 👍 from my side.

Ciao,
Dscho

From: Neeraj Singh <[email protected]>

Make it clearer in the naming and documentation of the plug_bulk_checkin and unplug_bulk_checkin APIs that they can be thought of as a "transaction" to optimize operations on the object database. These transactions may be nested so that subsystems like the cache-tree writing code can optimize their operations without caring whether the top-level code has a transaction active.

Add a flush_odb_transaction API that will be used in update-index to make objects visible even if a transaction is active. The flush call may also be useful in future cases if we hold a transaction active around calling hooks.

---
 builtin/add.c  |  4 ++--
 bulk-checkin.c | 25 +++++++++++++++++--------
 bulk-checkin.h | 24 ++++++++++++++++++++++--
 3 files changed, 41 insertions(+), 12 deletions(-)

index 3ffb86a4338..9bf37ceae8e 100644
@@ -670,7 +670,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		string_list_clear(&only_match_skip_worktree, 0);
 	}
 
-	plug_bulk_checkin();
+	begin_odb_transaction();
 
 	if (add_renormalize)
 		exit_status |= renormalize_tracked_files(&pathspec, flags);

@@ -682,7 +682,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	if (chmod_arg && pathspec.nr)
 		exit_status |= chmod_pathspec(&pathspec, chmod_arg[0], show_only);
-	unplug_bulk_checkin();
+	end_odb_transaction();
 
 finish:
 	if (write_locked_index(&the_index, &lock_file,

index 88d72178b2c..0fb032c7b69 100644
@@ -10,7 +10,7 @@
 #include "packfile.h"
 #include "object-store.h"
 
-static int bulk_checkin_plugged;
+static int odb_transaction_nesting;
 
 static struct bulk_checkin_packfile {
 	char *pack_tmp_name;

@@ -280,20 +280,29 @@ int index_bulk_checkin(struct object_id *oid,
 {
 	int status = deflate_to_pack(&bulk_checkin_packfile, oid, fd, size, type,
 				     path, flags);
-	if (!bulk_checkin_plugged)
+	if (!odb_transaction_nesting)
 		flush_bulk_checkin_packfile(&bulk_checkin_packfile);
 	return status;
 }
 
-void plug_bulk_checkin(void)
+void begin_odb_transaction(void)
 {
-	assert(!bulk_checkin_plugged);
-	bulk_checkin_plugged = 1;
+	odb_transaction_nesting += 1;
 }
 
-void unplug_bulk_checkin(void)
+void flush_odb_transaction(void)
 {
-	assert(bulk_checkin_plugged);
-	bulk_checkin_plugged = 0;
 	flush_bulk_checkin_packfile(&bulk_checkin_packfile);
 }
+
+void end_odb_transaction(void)
+{
+	odb_transaction_nesting -= 1;
+	if (odb_transaction_nesting < 0)
+		BUG("Unbalanced ODB transaction nesting");
+
+	if (odb_transaction_nesting)
+		return;
+
+	flush_odb_transaction();
+}

index b26f3dc3b74..ee0832989a8 100644
@@ -10,7 +10,27 @@ int index_bulk_checkin(struct object_id *oid,
 		       int fd, size_t size, enum object_type type,
 		       const char *path, unsigned flags);
 
-void plug_bulk_checkin(void);
-void unplug_bulk_checkin(void);
+/*
+ * Tell the object database to optimize for adding
+ * multiple objects. end_odb_transaction must be called
+ * to make new objects visible. Transactions can be nested,
+ * and objects are only visible after the outermost transaction
+ * is complete or the transaction is flushed.
+ */
+void begin_odb_transaction(void);
+
+/*
+ * Make any objects that are currently part of a pending object
+ * database transaction visible. It is valid to call this function
+ * even if no transaction is active.
+ */
+void flush_odb_transaction(void);
+
+/*
+ * Tell the object database to make any objects from the
+ * current transaction visible if this is the final nested
+ * transaction.
+ */
+void end_odb_transaction(void);
 
 #endif

2.34.1.78.g86e39b8f8d

From: Neeraj Singh <[email protected]>

When adding many objects to a repo with core.fsync=loose-object, the cost of fsync'ing each object file can become prohibitive.

One major source of the cost of fsync is the implied flush of the hardware writeback cache within the disk drive. This commit introduces a new core.fsyncMethod=batch option that batches up hardware flushes. It hooks into the bulk-checkin odb-transaction functionality, takes advantage of tmp-objdir, and uses the writeout-only support code.

When the new mode is enabled, we do the following for each new object:
1a. Create the object in a tmp-objdir.
2a. Issue a pagecache writeback request and wait for it to complete.

At the end of the entire transaction when unplugging bulk checkin:
1b. Issue an fsync against a dummy file to flush the log and hardware
writeback cache, which should by now have seen the tmp-objdir writes.
2b. Rename all of the tmp-objdir files to their final names. 3b. When updating the index and/or refs, we assume that Git will issue
another fsync internal to that operation. This is not the default today, but the user now has the option of syncing the index and there is a separate patch series to implement syncing of refs.

On a filesystem with a singular journal that is updated during name operations (e.g. create, link, rename, etc), such as NTFS, HFS+, or XFS we would expect the fsync to trigger a journal writeout so that this sequence is enough to ensure that the user's data is durable by the time the git command returns. This sequence also ensures that no object files appear in the main object store unless they are fsync-durable.

Batch mode is only enabled if core.fsync includes loose-objects. If the legacy core.fsyncObjectFiles setting is enabled, but core.fsync does not include loose-objects, we will use file-by-file fsyncing.

In step (1a) of the sequence, the tmp-objdir is created lazily to avoid work if no loose objects are ever added to the ODB. We use a tmp-objdir to maintain the invariant that no loose-objects are visible in the main ODB unless they are properly fsync-durable. This is important since future ODB operations that try to create an object with specific contents will silently drop the new data if an object with the target hash exists without checking that the loose-object contents match the hash. Only a full git-fsck would restore the ODB to a functional state where dataloss doesn't occur.

In step (1b) of the sequence, we issue a fsync against a dummy file created specifically for the purpose. This method has a little higher cost than using one of the input object files, but makes adding new callers of this mechanism easier, since we don't need to figure out which object file is "last" or risk sharing violations by caching the fd of the last object file.

_Performance numbers_:

Linux - Hyper-V VM running Kernel 5.11 (Ubuntu 20.04) on a fast SSD. Mac - macOS 11.5.1 running on a Mac mini on a 1TB Apple SSD. Windows - Same host as Linux, a preview version of Windows 11.

Adding 500 files to the repo with 'git add' Times reported in seconds.

object file syncing | Linux | Mac   | Windows
--------------------|-------|-------|--------
           disabled | 0.06  |  0.35 | 0.61
              fsync | 1.88  | 11.18 | 2.47
              batch | 0.15  |  0.41 | 1.53

---
 Documentation/config/core.txt |  8 ++++
 bulk-checkin.c                | 71 +++++++++++++++++++++++++++++++++++
 bulk-checkin.h                |  3 ++
 cache.h                       |  8 +++-
 config.c                      |  2 +
 object-file.c                 |  7 +++-
 6 files changed, 97 insertions(+), 2 deletions(-)

index 889522956e4..d543bf12824 100644
@@ -628,6 +628,14 @@ core.fsyncMethod::
 * `writeout-only` issues pagecache writeback requests, but depending on the
   filesystem and storage hardware, data added to the repository may not be
   durable in the event of a system crash. This is the default mode on macOS.
+* `batch` enables a mode that uses writeout-only flushes to stage multiple
+  updates in the disk writeback cache and then does a single full fsync of
+  a dummy file to trigger the disk cache flush at the end of the operation.
++
+Currently `batch` mode only applies to loose-object files. Other repository
+data is made durable as if `fsync` was specified. This mode is expected to
+be as safe as `fsync` on macOS for repos stored on HFS+ or APFS filesystems
+and on Windows for repos stored on NTFS or ReFS filesystems.
 
 core.fsyncObjectFiles::
 	This boolean will enable 'fsync()' when writing object files.

index ee0832989a8..8281b9cb159 100644
@@ -6,6 +6,9 @@
 
 #include "cache.h"
 
+void prepare_loose_object_bulk_checkin(void);
+void fsync_loose_object_bulk_checkin(int fd, const char *filename);
+
 int index_bulk_checkin(struct object_id *oid,
 		       int fd, size_t size, enum object_type type,
 		       const char *path, unsigned flags);

index 6226f6a8a53..ea1466340d7 100644
@@ -1040,7 +1040,8 @@ extern int use_fsync;
 
 enum fsync_method {
 	FSYNC_METHOD_FSYNC,
-	FSYNC_METHOD_WRITEOUT_ONLY
+	FSYNC_METHOD_WRITEOUT_ONLY,
+	FSYNC_METHOD_BATCH,
 };
 
 extern enum fsync_method fsync_method;

@@ -1766,6 +1767,11 @@ void fsync_or_die(int fd, const char *);
 int fsync_component(enum fsync_component component, int fd);
 void fsync_component_or_die(enum fsync_component component, int fd, const char *msg);
 
+static inline int batch_fsync_enabled(enum fsync_component component)
+{
+	return (fsync_components & component) && (fsync_method == FSYNC_METHOD_BATCH);
+}
+
 ssize_t read_in_full(int fd, void *buf, size_t count);
 ssize_t write_in_full(int fd, const void *buf, size_t count);
 ssize_t pread_in_full(int fd, void *buf, size_t count, off_t offset);

index a5e11aad7fe..8ff25642906 100644
@@ -1688,6 +1688,8 @@ static int git_default_core_config(const char *var, const char *value, void *cb)
 			fsync_method = FSYNC_METHOD_FSYNC;
 		else if (!strcmp(value, "writeout-only"))
 			fsync_method = FSYNC_METHOD_WRITEOUT_ONLY;
+		else if (!strcmp(value, "batch"))
+			fsync_method = FSYNC_METHOD_BATCH;
 		else
 			warning(_("ignoring unknown core.fsyncMethod value '%s'"), value);