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 <[email protected]>
---
On Fri, May 03 2019, Jeff King wrote:
> 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
