[PATCH] perf-lib.sh: make "./run <revisions>" use the correct gits

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: May 6, 2019, 19:16 UTC
Message-ID: <20190506191611.16770-1-avarab@gmail.com>
In-Reply-To: <20190502222409.GA15631@sigill.intra.peff.net>

Fix a really bad regression in 0baf78e7bc ("perf-lib.sh: rely on test-lib.sh for --tee handling", 2019-03-15). Since that change all runs of different <revisions> of git have used the git found in the user's $PATH, e.g. /usr/bin/git instead of the <revision> we just built and wanted to performance test.

The problem starts with GIT_TEST_INSTALLED not working like our non-perf tests with the "run" script. I.e. you can't run performance tests against a given installed git. Instead we expect to use it ourselves to point GIT_TEST_INSTALLED to the <revision> we just built.

However, we had been relying on '$(cd "$GIT_TEST_INSTALLED" && pwd)' to resolve that relative $GIT_TEST_INSTALLED to an absolute path *before* test-lib.sh was loaded, in cases where it was e.g. "build/<rev>/bin-wrappers" and we wanted "<abs_path>build/...".

Perhaps there's some better way to fix this, but it seems to me that the best solution is to just make this behavior less magical. We know in run_dirs_helper() that we're about to run performance tests on a given <revision>, so let's just set GIT_TEST_INSTALLED to an absolute path there, and then make getting logging target from a previously relative path less magical, we'll just explicitly pass down the relative path as a variable.

This makes e.g. these cases all work:

    ./run . $PWD/../../ origin/master origin/next HEAD -- <tests>

As well as just a plain one-off:

    ./run <tests>

And, since we're passing down the new GIT_PERF_DIR_MYDIR_REL we make sure the bug relating to aggregate.perl not finding our files as described in 0baf78e7bc doesn't happen again.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Fri, May 03 2019, Jeff King wrote:

Show 13 quoted lines

> On Thu, May 02, 2019 at 05:45:09PM -0400, Jeff King wrote:
>
>> Here's what I came up with. Note that there's a bug in 'master' right
>> now which causes perf to produce nonsense results. It's due to my
>> 0baf78e7bc (perf-lib.sh: rely on test-lib.sh for --tee handling,
>> 2019-03-15). I'll fix that separately (the timing below is done with
>> that commit reverted).
>
> And here's the fix for that. It's rather subtle, so I hope I explained
> it sufficiently. I didn't notice it while working on the original
> because everything _appears_ to run fine, but you just get timings from
> the wrong version of Git. Which is only noticeable if you're literally
> testing two versions that you expect to differ.

I ran into this today and it took me an embarrasingly long time to figure out why my code wasn't making things faster.

So I wrote this up before seeing your patch, since it wasn't queued in "pu" and my naïve ML search didn't include inline patches (again, *sigh*).

Anyway, I wonder if something closer to this patch, or some sort of merge of the two (e.g. to still get rid of the ABSOLUTE_GIT_TEST_INSTALLED variable) is better. I.e. why try to magically detect all of this in perf-lib.sh itself, we know we're going to invoke it like this in the "run" script, so we can just set the appropriate variables there instead of this hard-to-explain magic of $GIT_TEST_INSTALLED being one value the first time, but another one the second time around.

 t/perf/perf-lib.sh | 4 ++++
 t/perf/run         | 8 ++++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index 169f92eae3..b15ee1d262 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -32,6 +32,10 @@ TEST_NO_MALLOC_CHECK=t
 if test -z "$GIT_TEST_INSTALLED"; then
 	perf_results_prefix=
 else
+	if test -n "$GIT_PERF_DIR_MYDIR_REL"
+	then
+		GIT_TEST_INSTALLED=$GIT_PERF_DIR_MYDIR_REL
+	fi
 	perf_results_prefix=$(printf "%s" "${GIT_TEST_INSTALLED%/bin-wrappers}" | tr -c "[a-zA-Z0-9]" "[_*]")"."
 	GIT_TEST_INSTALLED=$ABSOLUTE_GIT_TEST_INSTALLED
 fi
diff --git a/t/perf/run b/t/perf/run
index 9aaa733c77..0a7c8744ab 100755
--- a/t/perf/run
+++ b/t/perf/run
@@ -91,10 +91,14 @@ run_dirs_helper () {
 	if test "$mydir" = .; then
 		unset GIT_TEST_INSTALLED
 	else
-		GIT_TEST_INSTALLED="$mydir/bin-wrappers"
+		GIT_PERF_DIR_MYDIR_REL=$mydir
+		GIT_PERF_DIR_MYDIR_ABS=$(cd $mydir && pwd)
+		export GIT_PERF_DIR_MYDIR_REL GIT_PERF_DIR_MYDIR_ABS
+
+		GIT_TEST_INSTALLED="$GIT_PERF_DIR_MYDIR_ABS/bin-wrappers"
 		# Older versions of git lacked bin-wrappers; fallback to the
 		# files in the root.
-		test -d "$GIT_TEST_INSTALLED" || GIT_TEST_INSTALLED=$mydir
+		test -d "$GIT_TEST_INSTALLED" || GIT_TEST_INSTALLED=$GIT_PERF_DIR_MYDIR_ABS
 		export GIT_TEST_INSTALLED
 	fi
 	run_one_dir "$@"

-- 
2.21.0.593.g511ec345e18