Re: [PATCH v3 09/11] core.fsyncmethod: tests for batch mode

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

On Thu, Mar 24 2022, Neeraj Singh via GitGitGadget wrote:

> From: Neeraj Singh <neerajsi@microsoft.com>
>
> 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.
>
> Signed-off-by: Neeraj Singh <neerajsi@microsoft.com>
> ---
>  t/lib-unique-files.sh  | 32 ++++++++++++++++++++++++++++++++
>  t/t3700-add.sh         | 28 ++++++++++++++++++++++++++++
>  t/t3903-stash.sh       | 20 ++++++++++++++++++++
>  t/t5300-pack-object.sh | 41 +++++++++++++++++++++++++++--------------
>  4 files changed, 107 insertions(+), 14 deletions(-)
>  create mode 100644 t/lib-unique-files.sh
>
> diff --git a/t/lib-unique-files.sh b/t/lib-unique-files.sh
> new file mode 100644
> index 00000000000..74efca91dd7
> --- /dev/null
> +++ b/t/lib-unique-files.sh
> @@ -0,0 +1,32 @@
> +# 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 &&
> +	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
> +}

Having written my own perf tests for this series, I still don't get why
this is needed, at all.

tl;dr: the below: I think this whole workaround is because you missed
that "test_when_finished" exists, and how it excludes perf timings.

I.e. I get that if we ran this N times we'd want to wipe our repo
between tests, as for e.g. "git add" you want it to actually add the
objects.

It's what I do with the "hyperfine" command in
https://lore.kernel.org/git/RFC-patch-v2-4.7-61f4f3d7ef4-20220323T140753Z-avarab@gmail.com/
with the "-p" option.

I.e. hyperfine has a way to say "this is setup, but don't measure the
time", which is 1/2 of what you're working around here and in 10/11.

But as 10/11 shows you're limited to one run with t/perf because you
want to not include those "setup" numbers, and "test_perf" has no easy
way to avoid that (but more on that later).

Which b.t.w. I'm really skeptical of as an approach here in any case
(even if we couldn't exclude it from the numbers).

I.e. yes what "hyperfine" does would be preferrable, but in exchange for
avoiding that you're comparing samples of 1 runs.

Surely we're better off with N run (even if noisy). Given enough of them
the difference will shake out, and our estimated +/- will narrow..

But aside from that, why isn't this just:
	
	for cfg in true false blah
	done
		test_expect_success "setup for $cfg" '
			git init repo-$cfg &&
			for f in $(test_seq 1 100)
			do
				>repo-$cfg/$f
			done
		'
	
		test_perf "perf test for $cfg" '
			git -C repo-$cfg
		'
	done

Which surely is going to be more accurate in the context of our limited
t/perf environment because creating unique files is not sufficient at
all to ensure that your tests don't interfere with each other.

That's because in the first iteration we'll create N objects in
.git/objects/aa/* or whatever, which will *still be there* for your
second test, which will impact performance.

Whereas if you just make N repos you don't need unique files, and you
won't be introducing that as a conflating variable.

But anyway, reading perf-lib.sh again I haven't tested, but this whole
workaround seems truly unnecessary. I.e. in test_run_perf_ we do:
	
	test_run_perf_ () {
	        test_cleanup=:
	        test_export_="test_cleanup"
	        export test_cleanup test_export_
	        "$GTIME" -f "%E %U %S" -o test_time.$i "$TEST_SHELL_PATH" -c ' 
                	[... code we run and time ...]
		'
                [... later ...]
                test_eval_ "$test_cleanup"
	}

So can't you just avoid this whole glorious workaround for the low low
cost of approximately one shellscript string assignment? :)

I.e. if you do:

	setup_clean () {
		rm -rf repo
	}

	setup_first () {
		git init repo &&
		[make a bunch of files or whatever in repo]
	}

	setup_next () {
		test_when_finished "setup_clean" &&
		setup_first
	}

	test_expect_success 'setup initial stuff' '
		setup_first
	'

	test_perf 'my perf test' '
		test_when_finished "setup_next" &&
		[your perf test here]
	'

	test_expect_success 'cleanup' '
		# Not really needed, but just for completeness, we are
                # about to nuke the trash dir anyway...
		setup_clean
	'

I haven't tested (and need to run), but i'm pretty sure that does
exactly what you want without these workarounds, i.e. you'll get
"trampoline setup" without that setup being included in the perf
numbers.

Is it pretty? No, but it's a lot less complex than this unique file
business & workarounds, and will give you just the numbers you want, and
most importantly you car run it N times now for better samples.

I.e. "what you want" sans a *tiny* bit of noise that we use to just call
a function to do:

    test_cleanup=setup_next

Which we'll then eval *after* we measure your numbers to setup the next
test.