Re: [PATCH v3 02/20] t/perf: add performance test for sparse operations

On Wed, Mar 17 2021, Derrick Stolee wrote:

On 3/17/2021 4:41 AM, Ævar Arnfjörð Bjarmason wrote:

quoted

On Tue, Mar 16 2021, Derrick Stolee via GitGitGadget wrote:

quoted

+test_expect_success 'setup repo and indexes' '
+	git reset --hard HEAD &&
+	# Remove submodules from the example repo, because our
+	# duplication of the entire repo creates an unlikly data shape.
+	git config --file .gitmodules --get-regexp "submodule.*.path" >modules &&
+	git rm -f .gitmodules &&
+	for module in $(awk "{print \$2}" modules)
+	do
+		git rm $module || return 1
+	done &&
+	git commit -m "remove submodules" &&

Paradoxically with this you can no longer use a repo that's not git.git
or another repo that has submodules, since we'll die in trying to remove
them.

Good point.

quoted

Also you don't have to "git rm .gitmodules", the "git rm" command
removes submodule entries.

Sure.

quoted

Perhaps just:

    for module in $(git ls-files --stage | grep ^160000 | awk -F '\t' '{ print $2 }')
    do
        git rm "$module"
    done

Or another way of guarding against rm getting the empty list && commit?

But it seems odd to be doing this at all, the point of the perf
framework is that you can point it at any repo, and some repos you want
to test will have submodules.

You're right that it should handle all repos. However, the point of
the test is to have many copies of the repo, but most of them are
excluded by sparse-directory entries. We don't collapse sparse-directory
entries if there is a submodule inside, so the data shape is wrong after
making all the copies.

So, I disagree with your approach in your suggested diff, and instead
offer this one. I've tested this with git.git and another local repo
without submodules and checked that everything works as expected.

What's got me confused here is that there's two uses for the perf
framework in this context.

It's to use an empty/git.git as a test repo to demonstrate something,
but then also that you can run it in your arbitrary repo, and e.g. see
how much a given feature might benefit you.

Hence suggesting that maybe test_perf_fresh_repois better here, because
by using test_perf_default_repo you're creating the expectation that you
can run the perf test, observe an %X difference, and that'll be
give-or-take what you'll get for that use case if you enable the feature.

Except it won't because the repo has submodules, which we deleted for
the perf test...

A subshell isn't needed here.

FWIW the reason I got this out of ls-files is because you can have
submodules without .gitmodules entries, rare and broken, but seemed more
direct to grep the mode bits.

+	then
+		for module in $(awk "{print \$2}" modules)
+		do
+			git rm $module || return 1
+		done &&

Once we know we have submodules we can just do this without the loop.

    git rm $(awk "{print \$2}" modules)

+		git commit -m "remove submodules" || return 1
+	fi &&
 
 	echo bogus >a &&
 	cp a b &&

quoted

Seems like something like the WIP patch at the end on top would be
better.

quoted

+	echo bogus >a &&
+	cp a b &&
+	git add a b &&
+	git commit -m "level 0" &&
+	BLOB=$(git rev-parse HEAD:a) &&

Isn't the way we're getting this $BLOB equivalent to just 'echo bogus |
git hash-object --stdin -w' why commit it?

We are committing it so we can add commits that deepen the copies,
but within those copies we have these known file paths.

quoted

This whole thing makes me think you just wanted a test_perf_fresh_repo
all along, but I think this would be much more useful if you took the
default repo and multiplied the size in its tree by some multiple.

E.g. take the files we have in git.git, write a copy at prefix-1/,
prefix-2/ etc.

That is essentially what is happening here, but using multiple levels
of directories. Using these multiple levels presents extra tree
lookups and parsing in the event of expanding a sparse index to a
full one.

*nod*

Anyway, this thread's a bit of a bikeshed on my part, I was just
wondering if & what part of the test relied on the existing repo if it
was mostly setting up its own test data.