Revert/delay performance regression in 'git checkout -b'

RFC PATCH In review Derrick Stolee via GitGitGadget 4 participants 11 messages Aug 21, 2019 – Aug 22, 2019 Documentation

lore

Share & tools

lore

Derrick Stolee via GitGitGadget

author

2019-08-21 19:18 UTC #0 lore raw

cover Revert/delay performance regression in 'git checkout -b'

As we were integrating Git 2.23.0 into VFS for Git, we discovered that "git checkout -b new-branch" went from 0.3s to 10+s on the Windows OS repo. This was an intentional change when writing the "git switch" builtin. Here is the commit message for 65f099b ("switch: no worktree status unless real branch switch happens" 2019-03-29):

When we switch from one branch to another, it makes sense to show a summary of local changes since there could be conflicts, or some files left modified.... When switch is used solely for creating a new branch (and "switch" to the same commit) or detaching, we don't really need to show anything.

"git checkout" does it anyway for historical reasons. But we can start with a clean slate with switch and don't have to.

This essentially reverts fa655d8411 (checkout: optimize "git checkout -b <new_branch>" - 2018-08-16) and make it default for switch, but also for -B and --detach. Users of big repos are encouraged to move to switch.

I was considering doing a full, long-term revert of this change to get the performance back to normal, but I also saw this feedback on the list for this patch:

I like this last bit. The skip_merge_working_tree() function which this removes was ugly, difficult to maintain, and difficult to get just right (and easy to break -- even by changing parts of the system which one might not expect to impact it).

So, the goal is to reduce the complication given by skip_merge_working_tree() by recommending that users use 'git switch -c'. The only problem is: users will take a while to move, unless prompted.

This series does the following:

1. Reverts the change that makes 'git checkout -b' slow again.
2. Creates a warning that recommends users start using 'git switch -c' instead.

This allows us to strip out this performance feature after users have had time to adopt the new way of doing things.

Derrick Stolee (2):
Revert "switch: no worktree status unless real branch switch happens"
DEPRECATION: warn about 'git checkout -b'

 Documentation/config/checkout.txt |   8 ++
 builtin/checkout.c                | 130 +++++++++++++++++++++++++++++-
 t/t1090-sparse-checkout-scope.sh  |  14 ++++
 3 files changed, 150 insertions(+), 2 deletions(-)

base-commit: 5fa0f5238b0cd46cfe7f6fa76c3f526ea98148d9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-317%2Fderrickstolee%2Frevert-switch-slow-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-317/derrickstolee/revert-switch-slow-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/317

Signature · 1 line

gitgitgadget

Derrick Stolee via GitGitGadget

author

2019-08-21 19:18 UTC #1 lore raw

PATCH 1/2 Revert "switch: no worktree status unless real branch switch happens"

From: Derrick Stolee <[email protected]>

This reverts commit 65f099b3988198f0fdf3ef7a21dc01c556d21fff, which removed logic for avoiding extra cost in "git checkout -b" in favor of the new "git switch -c". This will cause a performance issue for users in large repos.

Signed-off-by Derrick Stolee <[email protected]>

---
 Documentation/config/checkout.txt |   8 ++
 builtin/.checkout.c.swp           | Bin 0 -> 77824 bytes
 builtin/checkout.c                | 133 ++++++++++++++++++++++++++++--
 t/t1090-sparse-checkout-scope.sh  |  14 ++++
 4 files changed, 148 insertions(+), 7 deletions(-)
 create mode 100644 builtin/.checkout.c.swp

Documentation/config/checkout.txt +8 -0

index 6b646813ab..d6872ffa83 100644
@@ -16,3 +16,11 @@ will checkout the '<something>' branch on another remote,
 and by linkgit:git-worktree[1] when 'git worktree add' refers to a
 remote branch. This setting might be used for other checkout-like
 commands or functionality in the future.
+
+checkout.optimizeNewBranch::
+	Optimizes the performance of "git checkout -b <new_branch>" when
+	using sparse-checkout.  When set to true, git will not update the
+	repo based on the current sparse-checkout settings.  This means it
+	will not update the skip-worktree bit in the index nor add/remove
+	files in the working directory to reflect the current sparse checkout
+	settings nor will it show the local changes.

builtin/.checkout.c.swp new file binary +0 -0

new file mode 100644
index 0000000000000000000000000000000000000000..f6dad4abb02c265ee66b3f6f76d00d59b9b524a4
GIT binary patch

builtin/checkout.c +126 -7

183 lines not on this page · raw message

t/t1090-sparse-checkout-scope.sh +14 -0

index 40cc004326..090b7fc3d3 100755
@@ -31,6 +31,20 @@ test_expect_success 'perform sparse checkout of master' '
 	test_path_is_file c
 '
 
+test_expect_success 'checkout -b checkout.optimizeNewBranch interaction' '
+	cp .git/info/sparse-checkout .git/info/sparse-checkout.bak &&
+	test_when_finished "
+		mv -f .git/info/sparse-checkout.bak .git/info/sparse-checkout
+		git checkout master
+	" &&
+	echo "/b" >>.git/info/sparse-checkout &&
+	test "$(git ls-files -t b)" = "S b" &&
+	git -c checkout.optimizeNewBranch=true checkout -b fast &&
+	test "$(git ls-files -t b)" = "S b" &&
+	git checkout -b slow &&
+	test "$(git ls-files -t b)" = "H b"
+'
+
 test_expect_success 'merge feature branch into sparse checkout of master' '
 	git merge feature &&
 	test_path_is_file a &&

Signature · 1 line

gitgitgadget

SZEDER Gábor

2019-08-21 22:15 UTC #2 lore raw

Re: [PATCH 1/2] Revert "switch: no worktree status unless real branch switch happens"

Quoted 4 lines · Derrick Stolee via GitGitGadget

This reverts commit 65f099b3988198f0fdf3ef7a21dc01c556d21fff, which removed logic for avoiding extra cost in "git checkout -b" in favor of the new "git switch -c". This will cause a performance issue for users in large repos.

It always makes me sad when I see a pull request, where all the relevant information is only included in the PR, which will not be part of the history, but not in the commit messages...

builtin/.checkout.c.swp

Quoted 9 lines

 builtin/.checkout.c.swp           | Bin 0 -> 77824 bytes

builtin/.checkout.c.swp new file binary +0 -0

new file mode 100644
index 0000000000000000000000000000000000000000..f6dad4abb02c265ee66b3f6f76d00d59b9b524a4
GIT binary patch

Uh-oh.

Derrick Stolee

2019-08-22 13:41 UTC #3 lore raw

Re: [PATCH 1/2] Revert "switch: no worktree status unless real branch switch happens"

Quoted 9 lines · SZEDER Gábor

Quoted 4 lines · Derrick Stolee via GitGitGadget
This reverts commit 65f099b3988198f0fdf3ef7a21dc01c556d21fff, which removed logic for avoiding extra cost in "git checkout -b" in favor of the new "git switch -c". This will cause a performance issue for users in large repos.
It always makes me sad when I see a pull request, where all the relevant information is only included in the PR, which will not be part of the history, but not in the commit messages...

An issue with the RFC quality at the moment. I'll do a better job if we decide to move this direction.

Quoted 11 lines

builtin/.checkout.c.swp
Quoted 9 lines
 builtin/.checkout.c.swp           | Bin 0 -> 77824 bytes
builtin/.checkout.c.swp new file binary +0 -0
new file mode 100644
index 0000000000000000000000000000000000000000..f6dad4abb02c265ee66b3f6f76d00d59b9b524a4
GIT binary patch
Uh-oh.

Whoops! I thought I removed this in time to submit, but I guess I did not.

-Stolee

Derrick Stolee via GitGitGadget

author

2019-08-21 19:18 UTC #4 lore raw

PATCH 2/2 DEPRECATION: warn about 'git checkout -b'

In reply to Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

Recommend that users use 'git switch -c' instead.

Signed-off-by Derrick Stolee <[email protected]>

---
 builtin/.checkout.c.swp | Bin 77824 -> 0 bytes
 builtin/checkout.c      |   9 ++++++++-
 2 files changed, 8 insertions(+), 1 deletion(-)
 delete mode 100644 builtin/.checkout.c.swp

builtin/.checkout.c.swp deleted binary +0 -0

deleted file mode 100644
index f6dad4abb02c265ee66b3f6f76d00d59b9b524a4..0000000000000000000000000000000000000000
GIT binary patch

builtin/checkout.c +8 -1

index f894b9de1a..32c8edccaf 100644
@@ -1133,6 +1133,7 @@ static int switch_branches(const struct checkout_opts *opts,
 	void *path_to_free;
 	struct object_id rev;
 	int flag, writeout_error = 0;
+	int do_merge = 1;
 
 	trace2_cmd_mode("branch");

@@ -1159,15 +1160,20 @@ static int switch_branches(const struct checkout_opts *opts,
 		if (!new_branch_info->commit)
 			die(_("You are on a branch yet to be born"));
 		parse_commit_or_die(new_branch_info->commit);
+
+		if (opts->only_merge_on_switching_branches)
+			do_merge = 0;
 	}
 
 	/* optimize the "checkout -b <new_branch> path */
-	if (skip_merge_working_tree(opts, &old_branch_info, new_branch_info)) {
+	if (!do_merge || skip_merge_working_tree(opts, &old_branch_info, new_branch_info)) {
 		if (!checkout_optimize_new_branch && !opts->quiet) {
 			if (read_cache_preload(NULL) < 0)
 				return error(_("index file corrupt"));
 			show_local_changes(&new_branch_info->commit->object, &opts->diff_options);
 		}
+		if (!opts->only_merge_on_switching_branches)
+			warning(_("'git checkout -b <new-branch>' is being deprecated in favor of 'git switch -c <new-branch>'. Please adjust your workflow accordingly."));
 	} else {
 		ret = merge_working_tree(opts, &old_branch_info, new_branch_info, &writeout_error);
 		if (ret) {

@@ -1867,6 +1873,7 @@ int cmd_switch(int argc, const char **argv, const char *prefix)
 	opts.accept_ref = 1;
 	opts.accept_pathspec = 0;
 	opts.switch_branch_doing_nothing_is_ok = 0;
+	opts.only_merge_on_switching_branches = 1;
 	opts.implicit_detach = 0;
 	opts.can_switch_when_in_progress = 0;
 	opts.orphan_from_empty_tree = 1;

Signature · 1 line

gitgitgadget

Elijah Newren

2019-08-22 03:03 UTC #5 lore raw

Re: [PATCH 2/2] DEPRECATION: warn about 'git checkout -b'

builtin/.checkout.c.swp

Quoted 23 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>
Recommend that users use 'git switch -c' instead.
Signed-off-by Derrick Stolee <[email protected]>
---
 builtin/.checkout.c.swp | Bin 77824 -> 0 bytes
 builtin/checkout.c      |   9 ++++++++-
 2 files changed, 8 insertions(+), 1 deletion(-)
 delete mode 100644 builtin/.checkout.c.swp
builtin/.checkout.c.swp deleted binary +0 -0
deleted file mode 100644
index f6dad4abb02c265ee66b3f6f76d00d59b9b524a4..0000000000000000000000000000000000000000
GIT binary patch

<snip>

You may want to add a local commit hook that prevents commiting vim swp files, or adding them to a global excludes file or something. :-)

SZEDER Gábor

2019-08-21 22:32 UTC #6 lore raw

In reply to Derrick Stolee via GitGitGadget

Quoted 2 lines · Derrick Stolee via GitGitGadget

As we were integrating Git 2.23.0 into VFS for Git, we discovered that "git checkout -b new-branch" went from 0.3s to 10+s on the Windows OS repo.

Does this slowdown only affect the Windows OS repo with VFS for Git, or other biggish repos without VFS for Git as well?

Quoted 5 lines

This series does the following:
1. Reverts the change that makes 'git checkout -b' slow again.
2. Creates a warning that recommends users start using 'git switch -c' instead.

'git help switch' says loudly and clearly that "THIS COMMAND IS EXPERIMENTAL. THE BEHAVIOR MAY CHANGE." It's too early to recommend it this aggressively, and to deprecate any parts of 'git checkout'.

Elijah Newren

2019-08-22 03:18 UTC #7 lore raw

Quoted 7 lines · SZEDER Gábor

Quoted 2 lines · Derrick Stolee via GitGitGadget
As we were integrating Git 2.23.0 into VFS for Git, we discovered that "git checkout -b new-branch" went from 0.3s to 10+s on the Windows OS repo.
Does this slowdown only affect the Windows OS repo with VFS for Git, or other biggish repos without VFS for Git as well?

It will also affect other biggish repos without VFS for Git; see the "does not seem to be GFVS-specific in any way" paragraph of
https://public-inbox.org/git/CABPp-BGir_5xyqEfwytDog0rZDydPHXjuqXCpNKk67dVPXjUjA@mail.gmail.com/

Derrick Stolee

2019-08-22 13:43 UTC #8 lore raw

In reply to SZEDER Gábor

Quoted 9 lines · SZEDER Gábor

Quoted 2 lines · Derrick Stolee via GitGitGadget
As we were integrating Git 2.23.0 into VFS for Git, we discovered that "git checkout -b new-branch" went from 0.3s to 10+s on the Windows OS repo.
Does this slowdown only affect the Windows OS repo with VFS for Git, or other biggish repos without VFS for Git as well?
Quoted 2 lines
This series does the following:

Quoted 7 lines

Quoted 3 lines
1. Reverts the change that makes 'git checkout -b' slow again.
2. Creates a warning that recommends users start using 'git switch -c' instead.
'git help switch' says loudly and clearly that "THIS COMMAND IS EXPERIMENTAL. THE BEHAVIOR MAY CHANGE." It's too early to recommend it this aggressively, and to deprecate any parts of 'git checkout'.

Even more reason why the performance boost should not have been removed from 'git checkout -b' so quickly.

Thanks,
-Stolee

Elijah Newren

2019-08-22 03:16 UTC #9 lore raw

In reply to Derrick Stolee via GitGitGadget

Hi,

Quoted 21 lines · Derrick Stolee via GitGitGadget

As we were integrating Git 2.23.0 into VFS for Git, we discovered that "git checkout -b new-branch" went from 0.3s to 10+s on the Windows OS repo. This was an intentional change when writing the "git switch" builtin. Here is the commit message for 65f099b ("switch: no worktree status unless real branch switch happens" 2019-03-29):
When we switch from one branch to another, it makes sense to show a summary of local changes since there could be conflicts, or some files left modified.... When switch is used solely for creating a new branch (and "switch" to the same commit) or detaching, we don't really need to show anything.
"git checkout" does it anyway for historical reasons. But we can start with a clean slate with switch and don't have to.
This essentially reverts fa655d8411 (checkout: optimize "git checkout -b <new_branch>" - 2018-08-16) and make it default for switch, but also for -B and --detach. Users of big repos are encouraged to move to switch.

Quoted 8 lines

I was considering doing a full, long-term revert of this change to get the performance back to normal, but I also saw this feedback on the list for this patch:
I like this last bit. The skip_merge_working_tree() function which this removes was ugly, difficult to maintain, and difficult to get just right (and easy to break -- even by changing parts of the system which one might not expect to impact it).

Instead of restoring this easy-to-break code, could we instead simplify it and make it more robust? As per the original commit message, the whole point of the patch is that when you have a huge index, operations take a while. But in the special case of "git checkout -b <new_branch>", there's no need to even check the index. So, we could:

* Check if the user is running "git checkout -b <new_branch>"
* If so, use the performance hack to skip touching the index at all.

This would be much better than what the patch currently does:

* Check if the user has specified -m, if so they clearly didn't just
specify "git checkout -b <new_branch>"
* Check if the user has specified -f, if so they clearly didn't just
specify "git checkout -b <new_branch>"
* Check if the user has specified --detach, if so they clearly
didn't just specify "git checkout -b <new_branch>"
* ...<lots of other similar steps>...
* If we got here, since we've checked all other cases (assuming
other people who have touched checkout remembered to add the necessary checks for each and every new flag), then by deduction the user must have specified "git checkout -b <new_branch>", so...
* Use the performance hack to skip touching the index at all.

Derrick Stolee

2019-08-22 13:45 UTC #10 lore raw

Quoted 46 lines · Elijah Newren

Hi,
Quoted 21 lines · Derrick Stolee via GitGitGadget
As we were integrating Git 2.23.0 into VFS for Git, we discovered that "git checkout -b new-branch" went from 0.3s to 10+s on the Windows OS repo. This was an intentional change when writing the "git switch" builtin. Here is the commit message for 65f099b ("switch: no worktree status unless real branch switch happens" 2019-03-29):
When we switch from one branch to another, it makes sense to show a summary of local changes since there could be conflicts, or some files left modified.... When switch is used solely for creating a new branch (and "switch" to the same commit) or detaching, we don't really need to show anything.
"git checkout" does it anyway for historical reasons. But we can start with a clean slate with switch and don't have to.
This essentially reverts fa655d8411 (checkout: optimize "git checkout -b <new_branch>" - 2018-08-16) and make it default for switch, but also for -B and --detach. Users of big repos are encouraged to move to switch.
Quoted 8 lines
I was considering doing a full, long-term revert of this change to get the performance back to normal, but I also saw this feedback on the list for this patch:
I like this last bit. The skip_merge_working_tree() function which this removes was ugly, difficult to maintain, and difficult to get just right (and easy to break -- even by changing parts of the system which one might not expect to impact it).
Instead of restoring this easy-to-break code, could we instead simplify it and make it more robust? As per the original commit message, the whole point of the patch is that when you have a huge index, operations take a while. But in the special case of "git checkout -b <new_branch>", there's no need to even check the index. So, we could:
* Check if the user is running "git checkout -b <new_branch>"
* If so, use the performance hack to skip touching the index at all.
This would be much better than what the patch currently does:

Quoted 12 lines

* Check if the user has specified -m, if so they clearly didn't just
specify "git checkout -b <new_branch>"
* Check if the user has specified -f, if so they clearly didn't just
specify "git checkout -b <new_branch>"
* Check if the user has specified --detach, if so they clearly
didn't just specify "git checkout -b <new_branch>"
* ...<lots of other similar steps>...
* If we got here, since we've checked all other cases (assuming
other people who have touched checkout remembered to add the necessary checks for each and every new flag), then by deduction the user must have specified "git checkout -b <new_branch>", so...
* Use the performance hack to skip touching the index at all.

I can look into a simpler implementation of this direction. I'm getting the feeling that this immediate recommendation of "git switch -c" is too hasty, so the best thing to do is to create a simpler way to detect "git checkout -b" and use the fast method.

Thanks,
-Stolee