patch, 3 partsIntroduce a perf test for interactive rebase

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

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 <johannes.schindelin@gmx.de>
---
 t/perf/perf-lib.sh | 4 ++++
 1 file changed, 4 insertions(+)

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

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

Hi Junio,

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

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

Fixed in v2, Dscho

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 <johannes.schindelin@gmx.de>
---
 t/perf/perf-lib.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

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.

Hi Junio,

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

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

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

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'"
  }

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

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

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).

Hi Junio,

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

Right.

Good point. I will change it so that the test is skipped if that commit is not found.

Ciao, Dscho

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(-)

On Wed, May 11, 2016 at 4:42 AM, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

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)

Hi Eric,

On Wed, 11 May 2016, Eric Sunshine wrote:

Thanks for educating me: I had not known about this option.

Will send another iteration in a moment.

Ciao, Dscho

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(+)

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

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(-)

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(-)

Am 13.05.2016 um 15:25 schrieb Johannes Schindelin:

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(+)

Hi René,

On Sun, 29 May 2016, René Scharfe wrote:

Oh drats!

I fear that interacts badly with the `cd "$repo"` I introduced later (replacing a `cd ..`)...

Ciao, Dscho

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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.

Am 30.05.2016 um 20:03 schrieb Junio C Hamano:

Oh, right, it does if $repo is a relative path.

Put it in its own subshell, e.g. like this?

---
 t/perf/perf-lib.sh | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

René Scharfe <l.s.r@web.de> writes:

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.

On Fri, May 13, 2016 at 03:25:58PM +0200, Johannes Schindelin wrote:

I was trying to run the perf scripts today and got bit by this patch. The problem is this line:

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

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