From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>, "Jeff King" <peff@peff.net>,
"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: [PATCH] perf-lib.sh: make "./run <revisions>" use the correct gits
Date: Mon, 6 May 2019 21:16:11 +0200 [thread overview]
Message-ID: <20190506191611.16770-1-avarab@gmail.com> (raw)
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:
> 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
next prev parent reply other threads:[~2019-05-06 19:16 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-25 10:15 What's cooking in git.git (Apr 2019, #05; Thu, 25) Junio C Hamano
2019-04-25 14:50 ` Elijah Newren
2019-04-25 22:16 ` js/partial-clone-connectivity-check (was: What's cooking in git.git (Apr 2019, #05; Thu, 25)) Josh Steadmon
2019-05-02 2:52 ` Jeff King
2019-05-02 16:48 ` Josh Steadmon
2019-05-02 21:45 ` Jeff King
2019-05-02 22:24 ` Jeff King
2019-05-06 19:16 ` Ævar Arnfjörð Bjarmason [this message]
2019-05-06 20:24 ` [PATCH] perf-lib.sh: make "./run <revisions>" use the correct gits Jeff King
2019-05-06 23:23 ` [PATCH v2 0/2] perf-lib.sh: fix 0baf78e7bc regression, refactor & fix bugs Ævar Arnfjörð Bjarmason
2019-05-07 7:06 ` Jeff King
2019-05-07 10:54 ` [PATCH v3 0/6] " Ævar Arnfjörð Bjarmason
2019-05-07 21:36 ` Jeff King
2019-05-08 1:59 ` Junio C Hamano
2019-05-07 10:54 ` [PATCH v3 1/6] perf README: correct docs for 3c8f12c96c regression Ævar Arnfjörð Bjarmason
2019-05-07 10:54 ` [PATCH v3 2/6] perf aggregate: remove GIT_TEST_INSTALLED from --codespeed Ævar Arnfjörð Bjarmason
2019-05-07 10:54 ` [PATCH v3 3/6] perf-lib.sh: make "./run <revisions>" use the correct gits Ævar Arnfjörð Bjarmason
2019-05-07 10:54 ` [PATCH v3 4/6] perf-lib.sh: remove GIT_TEST_INSTALLED from perf-lib.sh Ævar Arnfjörð Bjarmason
2019-05-07 10:54 ` [PATCH v3 5/6] perf tests: add "bindir" prefix to git tree test results Ævar Arnfjörð Bjarmason
2019-05-07 10:54 ` [PATCH v3 6/6] perf-lib.sh: forbid the use of GIT_TEST_INSTALLED Ævar Arnfjörð Bjarmason
2019-05-06 23:23 ` [PATCH v2 1/2] perf-lib.sh: make "./run <revisions>" use the correct gits Ævar Arnfjörð Bjarmason
2019-05-07 7:19 ` Jeff King
2019-05-06 23:23 ` [PATCH v2 2/2] perf-lib.sh: remove GIT_TEST_INSTALLED from perf-lib.sh Ævar Arnfjörð Bjarmason
2019-05-07 7:16 ` Jeff King
2019-05-07 8:31 ` Ævar Arnfjörð Bjarmason
2019-05-07 21:23 ` Jeff King
2019-04-26 5:05 ` What's cooking in git.git (Apr 2019, #05; Thu, 25) Taylor Blau
2019-04-26 5:41 ` Junio C Hamano
2019-04-26 5:53 ` Taylor Blau
2019-04-26 6:01 ` Junio C Hamano
2019-04-26 17:58 ` Kaartic Sivaraam
2019-04-27 5:06 ` Junio C Hamano
2019-04-27 5:57 ` Kaartic Sivaraam
2019-05-02 3:09 ` Jeff King
2019-04-29 22:20 ` js/macos-gettext-build, was " Johannes Schindelin
2019-05-05 5:13 ` Junio C Hamano
2019-05-06 9:11 ` Duy Nguyen
2019-05-07 2:36 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190506191611.16770-1-avarab@gmail.com \
--to=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).