* [PATCH v2 3/3] Add a perf test for rebase -i
2016-05-11 8:31 ` [PATCH v2 0/3] Introduce a perf test for interactive rebase Johannes Schindelin
@ 2016-05-11 8:42 ` Johannes Schindelin
2016-05-11 21:17 ` Junio C Hamano
2016-05-11 8:42 ` [PATCH v2 2/3] perf: make the tests work in worktrees Johannes Schindelin
` (2 subsequent siblings)
3 siblings, 1 reply; 26+ messages in thread
From: Johannes Schindelin @ 2016-05-11 8:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
This developer spent a lot of time trying to speed up the interactive
rebase, in particular on Windows. And will continue to do so.
To make it easier to demonstrate the performance improvement, let's have
a reproducible performance test.
The topic branch we use to test performance was found using these shell
commands (essentially searching for a long-enough topic branch in Git's
own history that touched the same file multiple times):
git rev-list --parents origin/master |
grep ' .* ' |
while read commit rest
do
patch_count=$(git rev-list --count $commit^..$commit^2)
test $patch_count -gt 20 || continue
merges="$(git rev-list --parents $commit^..$commit^2 |
grep ' .* ')"
test -z "$merges" || continue
patches_per_file="$(git log --pretty=%H --name-only \
$commit^..$commit^2 |
grep -v '^$' |
sort |
uniq -c -d |
sort -n -r)"
test -n "$patches_per_file" &&
test 20 -lt $(echo "$patches_per_file" |
sed -n '1s/^ *\([0-9]*\).*/\1/p') || continue
printf 'commit %s\n%s\n' "$commit" "$patches_per_file"
done
Note that we can get away with *not* having to reset to the original
branch tip before rebasing: we switch the first two "pick" lines every
time, so we end up with the same patch order after two rebases, and the
complexity of both rebases is equivalent.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/perf/p3404-rebase-interactive.sh | 31 +++++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)
create mode 100755 t/perf/p3404-rebase-interactive.sh
diff --git a/t/perf/p3404-rebase-interactive.sh b/t/perf/p3404-rebase-interactive.sh
new file mode 100755
index 0000000..382163c
--- /dev/null
+++ b/t/perf/p3404-rebase-interactive.sh
@@ -0,0 +1,31 @@
+#!/bin/sh
+
+test_description='Tests rebase -i performance'
+. ./perf-lib.sh
+
+test_perf_default_repo
+
+# This commit merges a sufficiently long topic branch for reasonable
+# performance testing
+branch_merge=ba5312d
+export branch_merge
+
+write_script swap-first-two.sh <<\EOF
+case "$1" in
+*/COMMIT_EDITMSG)
+ mv "$1" "$1".bak &&
+ sed -e '1{h;d}' -e 2G <"$1".bak >"$1"
+ ;;
+esac
+EOF
+
+test_expect_success 'setup' '
+ git config core.editor "\"$PWD"/swap-first-two.sh\" &&
+ git checkout -f $branch_merge^2
+'
+
+test_perf 'rebase -i' '
+ git rebase -i $branch_merge^
+'
+
+test_done
--
2.8.2.465.gb077790
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v2 3/3] Add a perf test for rebase -i
2016-05-11 8:42 ` [PATCH v2 3/3] Add a perf test for rebase -i Johannes Schindelin
@ 2016-05-11 21:17 ` Junio C Hamano
2016-05-13 13:16 ` Johannes Schindelin
0 siblings, 1 reply; 26+ messages in thread
From: Junio C Hamano @ 2016-05-11 21:17 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> diff --git a/t/perf/p3404-rebase-interactive.sh b/t/perf/p3404-rebase-interactive.sh
> new file mode 100755
> index 0000000..382163c
> --- /dev/null
> +++ b/t/perf/p3404-rebase-interactive.sh
> @@ -0,0 +1,31 @@
> +#!/bin/sh
> +
> +test_description='Tests rebase -i performance'
> +. ./perf-lib.sh
> +
> +test_perf_default_repo
> +
> +# This commit merges a sufficiently long topic branch for reasonable
> +# performance testing
> +branch_merge=ba5312d
> +export branch_merge
t/perf/README mentions the possibility to use your own repository as
a test data via GIT_PERF_REPO, but doing so would obviously break
this test.
I wonder if there is a way to say "running this perf script with
custom GIT_PERF_REPO is not supported" and error out. That may
help other existing tests that (incorrectly) assume that their test
data is this project (if there is any).
> +
> +write_script swap-first-two.sh <<\EOF
> +case "$1" in
> +*/COMMIT_EDITMSG)
> + mv "$1" "$1".bak &&
> + sed -e '1{h;d}' -e 2G <"$1".bak >"$1"
> + ;;
> +esac
> +EOF
> +
> +test_expect_success 'setup' '
> + git config core.editor "\"$PWD"/swap-first-two.sh\" &&
> + git checkout -f $branch_merge^2
> +'
> +
> +test_perf 'rebase -i' '
> + git rebase -i $branch_merge^
> +'
> +
> +test_done
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 3/3] Add a perf test for rebase -i
2016-05-11 21:17 ` Junio C Hamano
@ 2016-05-13 13:16 ` Johannes Schindelin
0 siblings, 0 replies; 26+ messages in thread
From: Johannes Schindelin @ 2016-05-13 13:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Hi Junio,
On Wed, 11 May 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > diff --git a/t/perf/p3404-rebase-interactive.sh b/t/perf/p3404-rebase-interactive.sh
> > new file mode 100755
> > index 0000000..382163c
> > --- /dev/null
> > +++ b/t/perf/p3404-rebase-interactive.sh
> > @@ -0,0 +1,31 @@
> > +#!/bin/sh
> > +
> > +test_description='Tests rebase -i performance'
> > +. ./perf-lib.sh
> > +
> > +test_perf_default_repo
> > +
> > +# This commit merges a sufficiently long topic branch for reasonable
> > +# performance testing
> > +branch_merge=ba5312d
> > +export branch_merge
>
> t/perf/README mentions the possibility to use your own repository as
> a test data via GIT_PERF_REPO, but doing so would obviously break
> this test.
Right.
> I wonder if there is a way to say "running this perf script with
> custom GIT_PERF_REPO is not supported" and error out. That may
> help other existing tests that (incorrectly) assume that their test
> data is this project (if there is any).
Good point. I will change it so that the test is skipped if that commit is
not found.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 2/3] perf: make the tests work in worktrees
2016-05-11 8:31 ` [PATCH v2 0/3] Introduce a perf test for interactive rebase Johannes Schindelin
2016-05-11 8:42 ` [PATCH v2 3/3] Add a perf test for rebase -i Johannes Schindelin
@ 2016-05-11 8:42 ` Johannes Schindelin
2016-05-11 17:40 ` Eric Sunshine
2016-05-11 8:42 ` [PATCH v2 1/3] perf: let's disable symlinks when they are not available Johannes Schindelin
2016-05-13 13:25 ` [PATCH v3 0/3] Introduce a perf test for interactive rebase Johannes Schindelin
3 siblings, 1 reply; 26+ messages in thread
From: Johannes Schindelin @ 2016-05-11 8:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
This patch makes perf-lib.sh more robust so that it can run correctly
even inside a worktree. For example, it assumed that $GIT_DIR/objects is
the objects directory (which is not the case for worktrees) and it used
the commondir file verbatim, even if it contained a relative path.
Furthermore, the setup code expected `git rev-parse --git-dir` to spit
out a relative path, which is also not true for worktrees. Let's just
change the code to accept both relative and absolute paths, by avoiding
the `cd` into the copied working directory.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/perf/perf-lib.sh | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index 50c8c39..cb88b08 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -80,22 +80,22 @@ test_perf_create_repo_from () {
error "bug in the test script: not 2 parameters to test-create-repo"
repo="$1"
source="$2"
- source_git=$source/$(cd "$source" && git rev-parse --git-dir)
+ source_git="$(cd "$source" && git rev-parse --git-dir)"
+ objects_dir="$(cd "$source" && git rev-parse --git-path objects)"
mkdir -p "$repo/.git"
(
- cd "$repo/.git" &&
- { cp -Rl "$source_git/objects" . 2>/dev/null ||
- cp -R "$source_git/objects" .; } &&
+ { cp -Rl "$objects_dir" "$repo/.git/" 2>/dev/null ||
+ cp -R "$objects_dir" "$repo/.git/"; } &&
for stuff in "$source_git"/*; do
case "$stuff" in
- */objects|*/hooks|*/config)
+ */objects|*/hooks|*/config|*/commondir)
;;
*)
- cp -R "$stuff" . || exit 1
+ cp -R "$stuff" "$repo/.git/" || exit 1
;;
esac
done &&
- cd .. &&
+ cd "$repo" &&
git init -q &&
git init -q && {
test_have_prereq SYMLINKS ||
--
2.8.2.465.gb077790
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v2 2/3] perf: make the tests work in worktrees
2016-05-11 8:42 ` [PATCH v2 2/3] perf: make the tests work in worktrees Johannes Schindelin
@ 2016-05-11 17:40 ` Eric Sunshine
2016-05-13 13:14 ` Johannes Schindelin
0 siblings, 1 reply; 26+ messages in thread
From: Eric Sunshine @ 2016-05-11 17:40 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, Git List
On Wed, May 11, 2016 at 4:42 AM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> This patch makes perf-lib.sh more robust so that it can run correctly
> even inside a worktree. For example, it assumed that $GIT_DIR/objects is
> the objects directory (which is not the case for worktrees) and it used
> the commondir file verbatim, even if it contained a relative path.
>
> Furthermore, the setup code expected `git rev-parse --git-dir` to spit
> out a relative path, which is also not true for worktrees. Let's just
> change the code to accept both relative and absolute paths, by avoiding
> the `cd` into the copied working directory.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
> @@ -80,22 +80,22 @@ test_perf_create_repo_from () {
> - source_git=$source/$(cd "$source" && git rev-parse --git-dir)
> + source_git="$(cd "$source" && git rev-parse --git-dir)"
> + objects_dir="$(cd "$source" && git rev-parse --git-path objects)"
Would it be out of the scope of this patch to simplify these by using -C?
source_git=$(git -C "$source" rev-parse --git-dir)
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 2/3] perf: make the tests work in worktrees
2016-05-11 17:40 ` Eric Sunshine
@ 2016-05-13 13:14 ` Johannes Schindelin
0 siblings, 0 replies; 26+ messages in thread
From: Johannes Schindelin @ 2016-05-13 13:14 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Junio C Hamano, Git List
Hi Eric,
On Wed, 11 May 2016, Eric Sunshine wrote:
> On Wed, May 11, 2016 at 4:42 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > This patch makes perf-lib.sh more robust so that it can run correctly
> > even inside a worktree. For example, it assumed that $GIT_DIR/objects is
> > the objects directory (which is not the case for worktrees) and it used
> > the commondir file verbatim, even if it contained a relative path.
> >
> > Furthermore, the setup code expected `git rev-parse --git-dir` to spit
> > out a relative path, which is also not true for worktrees. Let's just
> > change the code to accept both relative and absolute paths, by avoiding
> > the `cd` into the copied working directory.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
> > @@ -80,22 +80,22 @@ test_perf_create_repo_from () {
> > - source_git=$source/$(cd "$source" && git rev-parse --git-dir)
> > + source_git="$(cd "$source" && git rev-parse --git-dir)"
> > + objects_dir="$(cd "$source" && git rev-parse --git-path objects)"
>
> Would it be out of the scope of this patch to simplify these by using -C?
>
> source_git=$(git -C "$source" rev-parse --git-dir)
Thanks for educating me: I had not known about this option.
Will send another iteration in a moment.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 1/3] perf: let's disable symlinks when they are not available
2016-05-11 8:31 ` [PATCH v2 0/3] Introduce a perf test for interactive rebase Johannes Schindelin
2016-05-11 8:42 ` [PATCH v2 3/3] Add a perf test for rebase -i Johannes Schindelin
2016-05-11 8:42 ` [PATCH v2 2/3] perf: make the tests work in worktrees Johannes Schindelin
@ 2016-05-11 8:42 ` Johannes Schindelin
2016-05-13 13:25 ` [PATCH v3 0/3] Introduce a perf test for interactive rebase Johannes Schindelin
3 siblings, 0 replies; 26+ messages in thread
From: Johannes Schindelin @ 2016-05-11 8:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
We already have a perfectly fine prereq to tell us whether it is safe to
use symlinks. So let's use it.
This fixes the performance tests in Git for Windows' SDK, where symlinks
are not really available ([*1*]). This is not an issue with Git for
Windows itself because it configures core.symlinks=false in its system
config. However, the system config is disabled for the performance
tests, for obvious reasons: we want them to be independent of the
vagaries of any local configuration.
Footnote *1*: Windows has symbolic links. Git for Windows disables them
by default, though (for example: in standard setups, non-admins lack the
privilege to create symbolic links). For details, see
https://github.com/git-for-windows/git/wiki/Symbolic-Links
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/perf/perf-lib.sh | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index 5cf74ed..50c8c39 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -97,6 +97,10 @@ test_perf_create_repo_from () {
done &&
cd .. &&
git init -q &&
+ git init -q && {
+ test_have_prereq SYMLINKS ||
+ git config core.symlinks false
+ } &&
mv .git/hooks .git/hooks-disabled 2>/dev/null
) || error "failed to copy repository '$source' to '$repo'"
}
--
2.8.2.465.gb077790
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v3 0/3] Introduce a perf test for interactive rebase
2016-05-11 8:31 ` [PATCH v2 0/3] Introduce a perf test for interactive rebase Johannes Schindelin
` (2 preceding siblings ...)
2016-05-11 8:42 ` [PATCH v2 1/3] perf: let's disable symlinks when they are not available Johannes Schindelin
@ 2016-05-13 13:25 ` Johannes Schindelin
2016-05-13 13:25 ` [PATCH v3 1/3] perf: let's disable symlinks when they are not available Johannes Schindelin
` (2 more replies)
3 siblings, 3 replies; 26+ messages in thread
From: Johannes Schindelin @ 2016-05-13 13:25 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
This is the second preparatory patch series for my rebase--helper work
(i.e. moving parts of the interactive rebase into a builtin).
It simply introduces a perf test (and ensures that it runs in my
environment) so as to better determine how much the performance changes,
really.
Johannes Schindelin (3):
perf: let's disable symlinks when they are not available
perf: make the tests work in worktrees
Add a perf test for rebase -i
t/perf/p3404-rebase-interactive.sh | 36 ++++++++++++++++++++++++++++++++++++
t/perf/perf-lib.sh | 19 +++++++++++--------
2 files changed, 47 insertions(+), 8 deletions(-)
create mode 100755 t/perf/p3404-rebase-interactive.sh
Published-As: https://github.com/dscho/git/releases/tag/perf-rebase-i-v3
Interdiff vs v2:
diff --git a/t/perf/p3404-rebase-interactive.sh b/t/perf/p3404-rebase-interactive.sh
index 382163c..88f47de 100755
--- a/t/perf/p3404-rebase-interactive.sh
+++ b/t/perf/p3404-rebase-interactive.sh
@@ -7,9 +7,14 @@ test_perf_default_repo
# This commit merges a sufficiently long topic branch for reasonable
# performance testing
-branch_merge=ba5312d
+branch_merge=ba5312da19c6fdb6c6747d479f58932aae6e900c^{commit}
export branch_merge
+git rev-parse --verify $branch_merge >/dev/null 2>&1 || {
+ skip_all='skipping because $branch_merge was not found'
+ test_done
+}
+
write_script swap-first-two.sh <<\EOF
case "$1" in
*/COMMIT_EDITMSG)
diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index cb88b08..5ef1744 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -80,8 +80,8 @@ test_perf_create_repo_from () {
error "bug in the test script: not 2 parameters to test-create-repo"
repo="$1"
source="$2"
- source_git="$(cd "$source" && git rev-parse --git-dir)"
- objects_dir="$(cd "$source" && git rev-parse --git-path objects)"
+ source_git="$(git -C "$source" rev-parse --git-dir)"
+ objects_dir="$(git -C "$source" rev-parse --git-path objects)"
mkdir -p "$repo/.git"
(
{ cp -Rl "$objects_dir" "$repo/.git/" 2>/dev/null ||
@@ -96,7 +96,6 @@ test_perf_create_repo_from () {
esac
done &&
cd "$repo" &&
- git init -q &&
git init -q && {
test_have_prereq SYMLINKS ||
git config core.symlinks false
--
2.8.2.465.gb077790
^ permalink raw reply [flat|nested] 26+ messages in thread* [PATCH v3 1/3] perf: let's disable symlinks when they are not available
2016-05-13 13:25 ` [PATCH v3 0/3] Introduce a perf test for interactive rebase Johannes Schindelin
@ 2016-05-13 13:25 ` Johannes Schindelin
2016-05-13 13:25 ` [PATCH v3 2/3] perf: make the tests work in worktrees Johannes Schindelin
2016-05-13 13:26 ` [PATCH v3 3/3] Add a perf test for rebase -i Johannes Schindelin
2 siblings, 0 replies; 26+ messages in thread
From: Johannes Schindelin @ 2016-05-13 13:25 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
We already have a perfectly fine prereq to tell us whether it is safe to
use symlinks. So let's use it.
This fixes the performance tests in Git for Windows' SDK, where symlinks
are not really available ([*1*]). This is not an issue with Git for
Windows itself because it configures core.symlinks=false in its system
config. However, the system config is disabled for the performance
tests, for obvious reasons: we want them to be independent of the
vagaries of any local configuration.
Footnote *1*: Windows has symbolic links. Git for Windows disables them
by default, though (for example: in standard setups, non-admins lack the
privilege to create symbolic links). For details, see
https://github.com/git-for-windows/git/wiki/Symbolic-Links
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/perf/perf-lib.sh | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index 5cf74ed..9fa0706 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -96,7 +96,10 @@ test_perf_create_repo_from () {
esac
done &&
cd .. &&
- git init -q &&
+ git init -q && {
+ test_have_prereq SYMLINKS ||
+ git config core.symlinks false
+ } &&
mv .git/hooks .git/hooks-disabled 2>/dev/null
) || error "failed to copy repository '$source' to '$repo'"
}
--
2.8.2.465.gb077790
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v3 2/3] perf: make the tests work in worktrees
2016-05-13 13:25 ` [PATCH v3 0/3] Introduce a perf test for interactive rebase Johannes Schindelin
2016-05-13 13:25 ` [PATCH v3 1/3] perf: let's disable symlinks when they are not available Johannes Schindelin
@ 2016-05-13 13:25 ` Johannes Schindelin
2016-05-29 16:43 ` René Scharfe
2016-06-21 19:25 ` Jeff King
2016-05-13 13:26 ` [PATCH v3 3/3] Add a perf test for rebase -i Johannes Schindelin
2 siblings, 2 replies; 26+ messages in thread
From: Johannes Schindelin @ 2016-05-13 13:25 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
This patch makes perf-lib.sh more robust so that it can run correctly
even inside a worktree. For example, it assumed that $GIT_DIR/objects is
the objects directory (which is not the case for worktrees) and it used
the commondir file verbatim, even if it contained a relative path.
Furthermore, the setup code expected `git rev-parse --git-dir` to spit
out a relative path, which is also not true for worktrees. Let's just
change the code to accept both relative and absolute paths, by avoiding
the `cd` into the copied working directory.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/perf/perf-lib.sh | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index 9fa0706..5ef1744 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -80,22 +80,22 @@ test_perf_create_repo_from () {
error "bug in the test script: not 2 parameters to test-create-repo"
repo="$1"
source="$2"
- source_git=$source/$(cd "$source" && git rev-parse --git-dir)
+ source_git="$(git -C "$source" rev-parse --git-dir)"
+ objects_dir="$(git -C "$source" rev-parse --git-path objects)"
mkdir -p "$repo/.git"
(
- cd "$repo/.git" &&
- { cp -Rl "$source_git/objects" . 2>/dev/null ||
- cp -R "$source_git/objects" .; } &&
+ { cp -Rl "$objects_dir" "$repo/.git/" 2>/dev/null ||
+ cp -R "$objects_dir" "$repo/.git/"; } &&
for stuff in "$source_git"/*; do
case "$stuff" in
- */objects|*/hooks|*/config)
+ */objects|*/hooks|*/config|*/commondir)
;;
*)
- cp -R "$stuff" . || exit 1
+ cp -R "$stuff" "$repo/.git/" || exit 1
;;
esac
done &&
- cd .. &&
+ cd "$repo" &&
git init -q && {
test_have_prereq SYMLINKS ||
git config core.symlinks false
--
2.8.2.465.gb077790
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v3 2/3] perf: make the tests work in worktrees
2016-05-13 13:25 ` [PATCH v3 2/3] perf: make the tests work in worktrees Johannes Schindelin
@ 2016-05-29 16:43 ` René Scharfe
2016-05-30 8:28 ` Johannes Schindelin
2016-06-21 19:25 ` Jeff King
1 sibling, 1 reply; 26+ messages in thread
From: René Scharfe @ 2016-05-29 16:43 UTC (permalink / raw)
To: Johannes Schindelin, git; +Cc: Junio C Hamano, Eric Sunshine
Am 13.05.2016 um 15:25 schrieb Johannes Schindelin:
> This patch makes perf-lib.sh more robust so that it can run correctly
> even inside a worktree. For example, it assumed that $GIT_DIR/objects is
> the objects directory (which is not the case for worktrees) and it used
> the commondir file verbatim, even if it contained a relative path.
>
> Furthermore, the setup code expected `git rev-parse --git-dir` to spit
> out a relative path, which is also not true for worktrees. Let's just
> change the code to accept both relative and absolute paths, by avoiding
> the `cd` into the copied working directory.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> t/perf/perf-lib.sh | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
> index 9fa0706..5ef1744 100644
> --- a/t/perf/perf-lib.sh
> +++ b/t/perf/perf-lib.sh
> @@ -80,22 +80,22 @@ test_perf_create_repo_from () {
> error "bug in the test script: not 2 parameters to test-create-repo"
> repo="$1"
> source="$2"
> - source_git=$source/$(cd "$source" && git rev-parse --git-dir)
> + source_git="$(git -C "$source" rev-parse --git-dir)"
> + objects_dir="$(git -C "$source" rev-parse --git-path objects)"
> mkdir -p "$repo/.git"
> (
> - cd "$repo/.git" &&
> - { cp -Rl "$source_git/objects" . 2>/dev/null ||
> - cp -R "$source_git/objects" .; } &&
> + { cp -Rl "$objects_dir" "$repo/.git/" 2>/dev/null ||
> + cp -R "$objects_dir" "$repo/.git/"; } &&
> for stuff in "$source_git"/*; do
> case "$stuff" in
> - */objects|*/hooks|*/config)
> + */objects|*/hooks|*/config|*/commondir)
> ;;
> *)
> - cp -R "$stuff" . || exit 1
> + cp -R "$stuff" "$repo/.git/" || exit 1
> ;;
> esac
> done &&
> - cd .. &&
> + cd "$repo" &&
> git init -q && {
> test_have_prereq SYMLINKS ||
> git config core.symlinks false
>
This breaks perf for the non-worktree case:
lsr@debian:~/src/git/t/perf$ make
rm -rf test-results
./run
=== Running 12 tests in this tree ===
cp: cannot stat '.git/objects': No such file or directory
error: failed to copy repository '/home/lsr/src/git/t/..' to '/tmp/trash directory.p0000-perf-lib-sanity'
cp: cannot stat '.git/objects': No such file or directory
error: failed to copy repository '/home/lsr/src/git/t/..' to '/tmp/trash directory.p0001-rev-list'
...
Here's a fix:
-- >8 --
Subject: perf: make the tests work without a worktree
In regular repositories $source_git and $objects_dir contain relative
paths based on $source. Go there to allow cp to resolve them.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
t/perf/perf-lib.sh | 1 +
1 file changed, 1 insertion(+)
diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index 5ef1744..1888790 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -84,6 +84,7 @@ test_perf_create_repo_from () {
objects_dir="$(git -C "$source" rev-parse --git-path objects)"
mkdir -p "$repo/.git"
(
+ cd "$source" &&
{ cp -Rl "$objects_dir" "$repo/.git/" 2>/dev/null ||
cp -R "$objects_dir" "$repo/.git/"; } &&
for stuff in "$source_git"/*; do
--
2.8.3
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v3 2/3] perf: make the tests work in worktrees
2016-05-29 16:43 ` René Scharfe
@ 2016-05-30 8:28 ` Johannes Schindelin
2016-05-30 18:03 ` Junio C Hamano
0 siblings, 1 reply; 26+ messages in thread
From: Johannes Schindelin @ 2016-05-30 8:28 UTC (permalink / raw)
To: René Scharfe; +Cc: git, Junio C Hamano, Eric Sunshine
[-- Attachment #1: Type: text/plain, Size: 3476 bytes --]
Hi René,
On Sun, 29 May 2016, René Scharfe wrote:
> Am 13.05.2016 um 15:25 schrieb Johannes Schindelin:
> > This patch makes perf-lib.sh more robust so that it can run correctly
> > even inside a worktree. For example, it assumed that $GIT_DIR/objects is
> > the objects directory (which is not the case for worktrees) and it used
> > the commondir file verbatim, even if it contained a relative path.
> >
> > Furthermore, the setup code expected `git rev-parse --git-dir` to spit
> > out a relative path, which is also not true for worktrees. Let's just
> > change the code to accept both relative and absolute paths, by avoiding
> > the `cd` into the copied working directory.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > t/perf/perf-lib.sh | 14 +++++++-------
> > 1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
> > index 9fa0706..5ef1744 100644
> > --- a/t/perf/perf-lib.sh
> > +++ b/t/perf/perf-lib.sh
> > @@ -80,22 +80,22 @@ test_perf_create_repo_from () {
> > error "bug in the test script: not 2 parameters to test-create-repo"
> > repo="$1"
> > source="$2"
> > - source_git=$source/$(cd "$source" && git rev-parse --git-dir)
> > + source_git="$(git -C "$source" rev-parse --git-dir)"
> > + objects_dir="$(git -C "$source" rev-parse --git-path objects)"
> > mkdir -p "$repo/.git"
> > (
> > - cd "$repo/.git" &&
> > - { cp -Rl "$source_git/objects" . 2>/dev/null ||
> > - cp -R "$source_git/objects" .; } &&
> > + { cp -Rl "$objects_dir" "$repo/.git/" 2>/dev/null ||
> > + cp -R "$objects_dir" "$repo/.git/"; } &&
> > for stuff in "$source_git"/*; do
> > case "$stuff" in
> > - */objects|*/hooks|*/config)
> > + */objects|*/hooks|*/config|*/commondir)
> > ;;
> > *)
> > - cp -R "$stuff" . || exit 1
> > + cp -R "$stuff" "$repo/.git/" || exit 1
> > ;;
> > esac
> > done &&
> > - cd .. &&
> > + cd "$repo" &&
> > git init -q && {
> > test_have_prereq SYMLINKS ||
> > git config core.symlinks false
> >
>
> This breaks perf for the non-worktree case:
Oh drats!
> lsr@debian:~/src/git/t/perf$ make
> rm -rf test-results
> ./run
> === Running 12 tests in this tree ===
> cp: cannot stat '.git/objects': No such file or directory
> error: failed to copy repository '/home/lsr/src/git/t/..' to '/tmp/trash directory.p0000-perf-lib-sanity'
> cp: cannot stat '.git/objects': No such file or directory
> error: failed to copy repository '/home/lsr/src/git/t/..' to '/tmp/trash directory.p0001-rev-list'
> ...
>
> Here's a fix:
>
> -- >8 --
> Subject: perf: make the tests work without a worktree
>
> In regular repositories $source_git and $objects_dir contain relative
> paths based on $source. Go there to allow cp to resolve them.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
> t/perf/perf-lib.sh | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
> index 5ef1744..1888790 100644
> --- a/t/perf/perf-lib.sh
> +++ b/t/perf/perf-lib.sh
> @@ -84,6 +84,7 @@ test_perf_create_repo_from () {
> objects_dir="$(git -C "$source" rev-parse --git-path objects)"
> mkdir -p "$repo/.git"
> (
> + cd "$source" &&
I fear that interacts badly with the `cd "$repo"` I introduced later
(replacing a `cd ..`)...
Ciao,
Dscho
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v3 2/3] perf: make the tests work in worktrees
2016-05-30 8:28 ` Johannes Schindelin
@ 2016-05-30 18:03 ` Junio C Hamano
2016-05-30 18:24 ` René Scharfe
0 siblings, 1 reply; 26+ messages in thread
From: Junio C Hamano @ 2016-05-30 18:03 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: René Scharfe, git, Eric Sunshine
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> This breaks perf for the non-worktree case:
>
> Oh drats!
>
>> lsr@debian:~/src/git/t/perf$ make
>> rm -rf test-results
>> ./run
>> === Running 12 tests in this tree ===
>> cp: cannot stat '.git/objects': No such file or directory
>> error: failed to copy repository '/home/lsr/src/git/t/..' to '/tmp/trash directory.p0000-perf-lib-sanity'
>> cp: cannot stat '.git/objects': No such file or directory
>> error: failed to copy repository '/home/lsr/src/git/t/..' to '/tmp/trash directory.p0001-rev-list'
>> ...
>>
>> Here's a fix:
>>
>> -- >8 --
>> Subject: perf: make the tests work without a worktree
>>
>> In regular repositories $source_git and $objects_dir contain relative
>> paths based on $source. Go there to allow cp to resolve them.
>>
>> Signed-off-by: Rene Scharfe <l.s.r@web.de>
>> ---
>> t/perf/perf-lib.sh | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
>> index 5ef1744..1888790 100644
>> --- a/t/perf/perf-lib.sh
>> +++ b/t/perf/perf-lib.sh
>> @@ -84,6 +84,7 @@ test_perf_create_repo_from () {
>> objects_dir="$(git -C "$source" rev-parse --git-path objects)"
>> mkdir -p "$repo/.git"
>> (
>> + cd "$source" &&
>
> I fear that interacts badly with the `cd "$repo"` I introduced later
> (replacing a `cd ..`)...
What do you want to do then? For now before -rc1 we can revert the
whole thing so that we can get a tested thing that works in both
layouts.
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v3 2/3] perf: make the tests work in worktrees
2016-05-30 18:03 ` Junio C Hamano
@ 2016-05-30 18:24 ` René Scharfe
2016-05-31 21:24 ` Junio C Hamano
0 siblings, 1 reply; 26+ messages in thread
From: René Scharfe @ 2016-05-30 18:24 UTC (permalink / raw)
To: Junio C Hamano, Johannes Schindelin; +Cc: git, Eric Sunshine
Am 30.05.2016 um 20:03 schrieb Junio C Hamano:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>>> This breaks perf for the non-worktree case:
>>
>> Oh drats!
>>
>>> lsr@debian:~/src/git/t/perf$ make
>>> rm -rf test-results
>>> ./run
>>> === Running 12 tests in this tree ===
>>> cp: cannot stat '.git/objects': No such file or directory
>>> error: failed to copy repository '/home/lsr/src/git/t/..' to '/tmp/trash directory.p0000-perf-lib-sanity'
>>> cp: cannot stat '.git/objects': No such file or directory
>>> error: failed to copy repository '/home/lsr/src/git/t/..' to '/tmp/trash directory.p0001-rev-list'
>>> ...
>>>
>>> Here's a fix:
>>>
>>> -- >8 --
>>> Subject: perf: make the tests work without a worktree
>>>
>>> In regular repositories $source_git and $objects_dir contain relative
>>> paths based on $source. Go there to allow cp to resolve them.
>>>
>>> Signed-off-by: Rene Scharfe <l.s.r@web.de>
>>> ---
>>> t/perf/perf-lib.sh | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
>>> index 5ef1744..1888790 100644
>>> --- a/t/perf/perf-lib.sh
>>> +++ b/t/perf/perf-lib.sh
>>> @@ -84,6 +84,7 @@ test_perf_create_repo_from () {
>>> objects_dir="$(git -C "$source" rev-parse --git-path objects)"
>>> mkdir -p "$repo/.git"
>>> (
>>> + cd "$source" &&
>>
>> I fear that interacts badly with the `cd "$repo"` I introduced later
>> (replacing a `cd ..`)...
Oh, right, it does if $repo is a relative path.
> What do you want to do then? For now before -rc1 we can revert the
> whole thing so that we can get a tested thing that works in both
> layouts.
Put it in its own subshell, e.g. like this?
---
t/perf/perf-lib.sh | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index 5ef1744..18c363e 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -84,6 +84,7 @@ test_perf_create_repo_from () {
objects_dir="$(git -C "$source" rev-parse --git-path objects)"
mkdir -p "$repo/.git"
(
+ cd "$source" &&
{ cp -Rl "$objects_dir" "$repo/.git/" 2>/dev/null ||
cp -R "$objects_dir" "$repo/.git/"; } &&
for stuff in "$source_git"/*; do
@@ -94,7 +95,9 @@ test_perf_create_repo_from () {
cp -R "$stuff" "$repo/.git/" || exit 1
;;
esac
- done &&
+ done
+ ) &&
+ (
cd "$repo" &&
git init -q && {
test_have_prereq SYMLINKS ||
--
2.8.3
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v3 2/3] perf: make the tests work in worktrees
2016-05-30 18:24 ` René Scharfe
@ 2016-05-31 21:24 ` Junio C Hamano
0 siblings, 0 replies; 26+ messages in thread
From: Junio C Hamano @ 2016-05-31 21:24 UTC (permalink / raw)
To: René Scharfe; +Cc: Johannes Schindelin, git, Eric Sunshine
René Scharfe <l.s.r@web.de> writes:
>>> I fear that interacts badly with the `cd "$repo"` I introduced later
>>> (replacing a `cd ..`)...
>
> Oh, right, it does if $repo is a relative path.
>
>> What do you want to do then? For now before -rc1 we can revert the
>> whole thing so that we can get a tested thing that works in both
>> layouts.
>
> Put it in its own subshell, e.g. like this?
OK. I've squashed it and merged the result to 'next'. We'll see if
anybody screams and merge it to 'master' before -rc2.
Thanks.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 2/3] perf: make the tests work in worktrees
2016-05-13 13:25 ` [PATCH v3 2/3] perf: make the tests work in worktrees Johannes Schindelin
2016-05-29 16:43 ` René Scharfe
@ 2016-06-21 19:25 ` Jeff King
1 sibling, 0 replies; 26+ messages in thread
From: Jeff King @ 2016-06-21 19:25 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano, Eric Sunshine
On Fri, May 13, 2016 at 03:25:58PM +0200, Johannes Schindelin wrote:
> This patch makes perf-lib.sh more robust so that it can run correctly
> even inside a worktree. For example, it assumed that $GIT_DIR/objects is
> the objects directory (which is not the case for worktrees) and it used
> the commondir file verbatim, even if it contained a relative path.
>
> Furthermore, the setup code expected `git rev-parse --git-dir` to spit
> out a relative path, which is also not true for worktrees. Let's just
> change the code to accept both relative and absolute paths, by avoiding
> the `cd` into the copied working directory.
I was trying to run the perf scripts today and got bit by this patch.
The problem is this line:
> + objects_dir="$(git -C "$source" rev-parse --git-path objects)"
When the perf script is running, the "git" in its PATH is the version of
git being perf-tested, not the most recent one. And --git-path wasn't
introduced until v2.5.0. So if you try to compare results against an
older git, it fails:
$ cd t/perf
$ ./run v2.4.0 v2.9.0 -- p0000-perf-lib-sanity.sh
[...]
=== Running 1 tests in build/67308bd628c6235dbc1bad60c9ad1f2d27d576cc/bin-wrappers ===
fatal: ambiguous argument 'objects': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
cp: unrecognized option '--git-path
objects'
Try 'cp --help' for more information.
error: failed to copy repository '/home/peff/compile/git/t/..' to '/var/ram/git-tests/trash directory.p0000-perf-lib-sanity'
[...]
Test v2.4.0 v2.9.0
--------------------------------------------------------------------------------------
0000.1: test_perf_default_repo works <missing> 0.00(0.00+0.00)
0000.2: test_checkout_worktree works <missing> 0.01(0.00+0.00)
0000.4: export a weird var <missing> 0.00(0.00+0.00)
0000.6: important variables available in subshells <missing> 0.00(0.00+0.00)
0000.7: test-lib-functions correctly loaded in subshells <missing> 0.00(0.00+0.00)
I know that running modern perf tests against older versions isn't
guaranteed to work (the tests might rely on newer options, for example),
but it at least generally worked up until this point.
IMHO the problem is in the design of t/perf, though. It should always
use your modern git for the harness setup, and only use the git-to-test
inside test_expect and test_perf blocks.
-Peff
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v3 3/3] Add a perf test for rebase -i
2016-05-13 13:25 ` [PATCH v3 0/3] Introduce a perf test for interactive rebase Johannes Schindelin
2016-05-13 13:25 ` [PATCH v3 1/3] perf: let's disable symlinks when they are not available Johannes Schindelin
2016-05-13 13:25 ` [PATCH v3 2/3] perf: make the tests work in worktrees Johannes Schindelin
@ 2016-05-13 13:26 ` Johannes Schindelin
2 siblings, 0 replies; 26+ messages in thread
From: Johannes Schindelin @ 2016-05-13 13:26 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
This developer spent a lot of time trying to speed up the interactive
rebase, in particular on Windows. And will continue to do so.
To make it easier to demonstrate the performance improvement, let's have
a reproducible performance test.
The topic branch we use to test performance was found using these shell
commands (essentially searching for a long-enough topic branch in Git's
own history that touched the same file multiple times):
git rev-list --parents origin/master |
grep ' .* ' |
while read commit rest
do
patch_count=$(git rev-list --count $commit^..$commit^2)
test $patch_count -gt 20 || continue
merges="$(git rev-list --parents $commit^..$commit^2 |
grep ' .* ')"
test -z "$merges" || continue
patches_per_file="$(git log --pretty=%H --name-only \
$commit^..$commit^2 |
grep -v '^$' |
sort |
uniq -c -d |
sort -n -r)"
test -n "$patches_per_file" &&
test 20 -lt $(echo "$patches_per_file" |
sed -n '1s/^ *\([0-9]*\).*/\1/p') || continue
printf 'commit %s\n%s\n' "$commit" "$patches_per_file"
done
Note that we can get away with *not* having to reset to the original
branch tip before rebasing: we switch the first two "pick" lines every
time, so we end up with the same patch order after two rebases, and the
complexity of both rebases is equivalent.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/perf/p3404-rebase-interactive.sh | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
create mode 100755 t/perf/p3404-rebase-interactive.sh
diff --git a/t/perf/p3404-rebase-interactive.sh b/t/perf/p3404-rebase-interactive.sh
new file mode 100755
index 0000000..88f47de
--- /dev/null
+++ b/t/perf/p3404-rebase-interactive.sh
@@ -0,0 +1,36 @@
+#!/bin/sh
+
+test_description='Tests rebase -i performance'
+. ./perf-lib.sh
+
+test_perf_default_repo
+
+# This commit merges a sufficiently long topic branch for reasonable
+# performance testing
+branch_merge=ba5312da19c6fdb6c6747d479f58932aae6e900c^{commit}
+export branch_merge
+
+git rev-parse --verify $branch_merge >/dev/null 2>&1 || {
+ skip_all='skipping because $branch_merge was not found'
+ test_done
+}
+
+write_script swap-first-two.sh <<\EOF
+case "$1" in
+*/COMMIT_EDITMSG)
+ mv "$1" "$1".bak &&
+ sed -e '1{h;d}' -e 2G <"$1".bak >"$1"
+ ;;
+esac
+EOF
+
+test_expect_success 'setup' '
+ git config core.editor "\"$PWD"/swap-first-two.sh\" &&
+ git checkout -f $branch_merge^2
+'
+
+test_perf 'rebase -i' '
+ git rebase -i $branch_merge^
+'
+
+test_done
--
2.8.2.465.gb077790
^ permalink raw reply related [flat|nested] 26+ messages in thread