[PATCH 0/3] Introduce a perf test for interactive rebase

classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 0/3] Introduce a perf test for interactive rebase

Johannes Schindelin
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 on Windows
  perf: make the tests work in worktrees
  Add a perf test for rebase -i

 t/perf/p3404-rebase-interactive.sh | 31 +++++++++++++++++++++++++++++++
 t/perf/perf-lib.sh                 | 18 +++++++++++-------
 2 files changed, 42 insertions(+), 7 deletions(-)
 create mode 100755 t/perf/p3404-rebase-interactive.sh

Published-As: https://github.com/dscho/git/releases/tag/perf-rebase-i-v1
--
2.8.2.465.gb077790

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH 1/3] perf: let's disable symlinks on Windows

Johannes Schindelin
In Git for Windows' SDK, Git's source code is always checked out
with symlinks disabled. The reason is that POSIX symlinks have no
accurate equivalent on Windows [*1*]. More precisely, though, it is
not just Git's source code but *all* source code that is checked
out with symlinks disabled: core.symlinks is set to false in the
system-wide gitconfig.

Since the perf tests are run with the system-wide gitconfig *disabled*,
we have to make sure that the Git repository is initialized correctly
by configuring core.symlinks explicitly.

Footnote *1*:
https://github.com/git-for-windows/git/wiki/Symbolic-Links

Signed-off-by: Johannes Schindelin <[hidden email]>
---
 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..e9020d0 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 &&
+ if test_have_prereq MINGW
+ then
+ git config core.symlinks false
+ fi &&
  mv .git/hooks .git/hooks-disabled 2>/dev/null
  ) || error "failed to copy repository '$source' to '$repo'"
 }
--
2.8.2.465.gb077790


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH 2/3] perf: make the tests work in worktrees

Johannes Schindelin
In reply to this post by 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.

Signed-off-by: Johannes Schindelin <[hidden email]>
---
 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 e9020d0..e5682f7 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="$(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 &&
  if test_have_prereq MINGW
  then
--
2.8.2.465.gb077790


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH 3/3] Add a perf test for rebase -i

Johannes Schindelin
In reply to this post by Johannes Schindelin
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 <[hidden email]>
---
 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/3] perf: let's disable symlinks on Windows

Junio C Hamano
In reply to this post by Johannes Schindelin
Johannes Schindelin <[hidden email]> writes:

> In Git for Windows' SDK, Git's source code is always checked out
> with symlinks disabled. The reason is that POSIX symlinks have no
> accurate equivalent on Windows [*1*]. More precisely, though, it is
> not just Git's source code but *all* source code that is checked
> out with symlinks disabled: core.symlinks is set to false in the
> system-wide gitconfig.
>
> Since the perf tests are run with the system-wide gitconfig *disabled*,
> we have to make sure that the Git repository is initialized correctly
> by configuring core.symlinks explicitly.

Is MINGW the right prerequisite to use here, or is SIMLINKS more
appropriate?

>
> Footnote *1*:
> https://github.com/git-for-windows/git/wiki/Symbolic-Links
>
> Signed-off-by: Johannes Schindelin <[hidden email]>
> ---
>  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..e9020d0 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 &&
> + if test_have_prereq MINGW
> + then
> + git config core.symlinks false
> + fi &&
>   mv .git/hooks .git/hooks-disabled 2>/dev/null
>   ) || error "failed to copy repository '$source' to '$repo'"
>  }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/3] perf: make the tests work in worktrees

Junio C Hamano
In reply to this post by Johannes Schindelin
Johannes Schindelin <[hidden email]> writes:

> 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.
>
> Signed-off-by: Johannes Schindelin <[hidden email]>
> ---
>  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 e9020d0..e5682f7 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="$(git rev-parse --git-path objects)"

I do not quite understand this change.  Whose object_dir is this
looking into?  The original wanted to peek into $source/.git/objects/
which may have been wrong when $source is borrowing from some other
repository, but the new invocation of rev-parse --git-path objects
is done inside what repository?  It does not seem to pay any attention
to $source and the change below just copies from there into $repo.

Confused.

>   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 &&
>   if test_have_prereq MINGW
>   then
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/3] perf: make the tests work in worktrees

Johannes Schindelin
Hi Junio,

On Tue, 10 May 2016, Junio C Hamano wrote:

> Johannes Schindelin <[hidden email]> writes:
>
> > diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
> > index e9020d0..e5682f7 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="$(git rev-parse --git-path objects)"
>
> I do not quite understand this change.  Whose object_dir is this
> looking into?  The original wanted to peek into $source/.git/objects/
> which may have been wrong when $source is borrowing from some other
> repository, but the new invocation of rev-parse --git-path objects
> is done inside what repository?  It does not seem to pay any attention
> to $source and the change below just copies from there into $repo.

Bah. This got messed up in one of my interactive rebases. And then I
missed it in my final look-over before sending. Sorry. Fixed in v2.

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/3] perf: let's disable symlinks on Windows

Johannes Schindelin
In reply to this post by Junio C Hamano
Hi Junio,

On Tue, 10 May 2016, Junio C Hamano wrote:

> Johannes Schindelin <[hidden email]> writes:
>
> > In Git for Windows' SDK, Git's source code is always checked out
> > with symlinks disabled. The reason is that POSIX symlinks have no
> > accurate equivalent on Windows [*1*]. More precisely, though, it is
> > not just Git's source code but *all* source code that is checked
> > out with symlinks disabled: core.symlinks is set to false in the
> > system-wide gitconfig.
> >
> > Since the perf tests are run with the system-wide gitconfig *disabled*,
> > we have to make sure that the Git repository is initialized correctly
> > by configuring core.symlinks explicitly.
>
> Is MINGW the right prerequisite to use here, or is SIMLINKS more
> appropriate?

Oh, you're absolutely correct! It has nothing to do with MINGW itself, of
course.

Fixed in v2,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 0/3] Introduce a perf test for interactive rebase

Johannes Schindelin
In reply to this post by Johannes Schindelin
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 | 31 +++++++++++++++++++++++++++++++
 t/perf/perf-lib.sh                 | 18 +++++++++++-------
 2 files changed, 42 insertions(+), 7 deletions(-)
 create mode 100755 t/perf/p3404-rebase-interactive.sh

Published-As: https://github.com/dscho/git/releases/tag/perf-rebase-i-v2
Interdiff vs v1:

 diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
 index e5682f7..cb88b08 100644
 --- a/t/perf/perf-lib.sh
 +++ b/t/perf/perf-lib.sh
 @@ -81,7 +81,7 @@ test_perf_create_repo_from () {
  repo="$1"
  source="$2"
  source_git="$(cd "$source" && git rev-parse --git-dir)"
 - objects_dir="$(git rev-parse --git-path objects)"
 + objects_dir="$(cd "$source" && git rev-parse --git-path objects)"
  mkdir -p "$repo/.git"
  (
  { cp -Rl "$objects_dir" "$repo/.git/" 2>/dev/null ||
 @@ -97,10 +97,10 @@ test_perf_create_repo_from () {
  done &&
  cd "$repo" &&
  git init -q &&
 - if test_have_prereq MINGW
 - then
 + git init -q && {
 + test_have_prereq SYMLINKS ||
  git config core.symlinks false
 - fi &&
 + } &&
  mv .git/hooks .git/hooks-disabled 2>/dev/null
  ) || error "failed to copy repository '$source' to '$repo'"
  }

--
2.8.2.465.gb077790

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 3/3] Add a perf test for rebase -i

Johannes Schindelin
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 <[hidden email]>
---
 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 2/3] perf: make the tests work in worktrees

Johannes Schindelin
In reply to this post by 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 <[hidden email]>
---
 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


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 1/3] perf: let's disable symlinks when they are not available

Johannes Schindelin
In reply to this post by Johannes Schindelin
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 <[hidden email]>
---
 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


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 2/3] perf: make the tests work in worktrees

Eric Sunshine
In reply to this post by Johannes Schindelin
On Wed, May 11, 2016 at 4:42 AM, Johannes Schindelin
<[hidden email]> 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 <[hidden email]>
> ---
> 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)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 3/3] Add a perf test for rebase -i

Junio C Hamano
In reply to this post by Johannes Schindelin
Johannes Schindelin <[hidden email]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 2/3] perf: make the tests work in worktrees

Johannes Schindelin
In reply to this post by Eric Sunshine
Hi Eric,

On Wed, 11 May 2016, Eric Sunshine wrote:

> On Wed, May 11, 2016 at 4:42 AM, Johannes Schindelin
> <[hidden email]> 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 <[hidden email]>
> > ---
> > 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 3/3] Add a perf test for rebase -i

Johannes Schindelin
In reply to this post by Junio C Hamano
Hi Junio,

On Wed, 11 May 2016, Junio C Hamano wrote:

> Johannes Schindelin <[hidden email]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v3 0/3] Introduce a perf test for interactive rebase

Johannes Schindelin
In reply to this post by Johannes Schindelin
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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v3 1/3] perf: let's disable symlinks when they are not available

Johannes Schindelin
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 <[hidden email]>
---
 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


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v3 2/3] perf: make the tests work in worktrees

Johannes Schindelin
In reply to this post by 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 <[hidden email]>
---
 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


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v3 3/3] Add a perf test for rebase -i

Johannes Schindelin
In reply to this post by Johannes Schindelin
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 <[hidden email]>
---
 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html