A local shared clone is now much slower

A local shared clone is now much slower

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 853 bytes --]

Hi guys,

So commit 0433ad128c59 ("clone: run check_everything_connected") (which
turned up with v1.8.3) added a large traversal to clone which (as the
comment said) makes a clone much slower.  It is especially noticeable on
"git clone -s -l -n" which I use every day and used to be almost
instant.  Is there any thought to making it fast again, please?

The above clone is very useful for working with different branches in one
tree without touching every file in the main branch you are working
with (and consequent issues with rebuilding at least).  As linux-next
maintainer, you can imagine that I do this a bit.

I am sure one of Linus' points about branches was that being able to make
a fast local clone of a tree to use more than one branch was a feature.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: A local shared clone is now much slower

[Duy Nguyen]

On Mon, Jul 8, 2013 at 10:03 AM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> Hi guys,
>
> So commit 0433ad128c59 ("clone: run check_everything_connected") (which
> turned up with v1.8.3) added a large traversal to clone which (as the
> comment said) makes a clone much slower.  It is especially noticeable on
> "git clone -s -l -n" which I use every day and used to be almost
> instant.  Is there any thought to making it fast again, please?

It's done that way as a security measure against repo corruption.
Although I wonder if we could do connectivity check in background
instead (reports are stored in .git and picked up by git-status). The
same mechanism could be used for "git gc --auto". If the repo turns
out corrupted, the user may lose the last ~10 minutes of work, not
really bad for the speed trade off. This mode is not the default, of
course. The user has to be aware of the risk when choosing this route.
--
Duy

Re: A local shared clone is now much slower

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1327 bytes --]

Hi Duy,

On Mon, 8 Jul 2013 10:20:22 +0700 Duy Nguyen <pclouds@gmail.com> wrote:
>
> On Mon, Jul 8, 2013 at 10:03 AM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> >
> > So commit 0433ad128c59 ("clone: run check_everything_connected") (which
> > turned up with v1.8.3) added a large traversal to clone which (as the
> > comment said) makes a clone much slower.  It is especially noticeable on
> > "git clone -s -l -n" which I use every day and used to be almost
> > instant.  Is there any thought to making it fast again, please?
> 
> It's done that way as a security measure against repo corruption.
> Although I wonder if we could do connectivity check in background
> instead (reports are stored in .git and picked up by git-status). The
> same mechanism could be used for "git gc --auto". If the repo turns
> out corrupted, the user may lose the last ~10 minutes of work, not
> really bad for the speed trade off. This mode is not the default, of
> course. The user has to be aware of the risk when choosing this route.

Thanks for the explanation.  Now, is there some way I can turn it off
just for the local shared case.   In my case, I check my repo regularly,
so don't need or want this going on while I am working ...

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: A local shared clone is now much slower

[Jeff King]

On Mon, Jul 08, 2013 at 01:03:55PM +1000, Stephen Rothwell wrote:

> So commit 0433ad128c59 ("clone: run check_everything_connected") (which
> turned up with v1.8.3) added a large traversal to clone which (as the
> comment said) makes a clone much slower.  It is especially noticeable on
> "git clone -s -l -n" which I use every day and used to be almost
> instant.  Is there any thought to making it fast again, please?
> 
> The above clone is very useful for working with different branches in one
> tree without touching every file in the main branch you are working
> with (and consequent issues with rebuilding at least).  As linux-next
> maintainer, you can imagine that I do this a bit.

Yeah, I have noticed it is somewhat annoying, as well, because the
proportion of time taken for the check is so much larger compared to the
relatively instant time taken for the local shared clone.

The point of that commit is to add the same safety checks to clone that
we do for fetching. But in the local shared-repo case, I really feel
like all safety bets are off anyway. You are not creating a verified
redundant copy at all, and there are still corruptions that can sneak
through (e.g., bit corruptions of blob objects).

So maybe this:

-- >8 --
Subject: [PATCH] clone: drop connectivity check for local clones

Commit 0433ad1 (clone: run check_everything_connected,
2013-03-25) added the same connectivity check to clone that
we use for fetching. The intent was to provide enough safety
checks that "git clone git://..." could be counted on to
detect bit errors and other repo corruption, and not
silently propagate them to the clone.

For local clones, this turns out to be a bad idea, for two
reasons:

  1. Local clones use hard linking (or even shared object
     stores), and so complete far more quickly. The time
     spent on the connectivity check is therefore
     proportionally much more painful.

  2. Local clones do not actually meet our safety guarantee
     anyway. The connectivity check makes sure we have all
     of the objects we claim to, but it does not check for
     bit errors. We will notice bit errors in commits and
     trees, but we do not load blob objects at all. Whereas
     over the pack transport, we actually recompute the sha1
     of each object in the incoming packfile; bit errors
     change the sha1 of the object, which is then caught by
     the connectivity check.

This patch drops the connectivity check in the local case.
Note that we have to revert the changes from 0433ad1 to
t5710, as we no longer notice the corruption during clone.

We could go a step further and provide a "verify even local
clones" option, but it is probably not worthwhile. You can
already spell that as "cd foo.git && git fsck && git clone ."
or as "git clone --no-local foo.git".

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/clone.c           | 22 +++++++++++++---------
 t/t5710-info-alternate.sh |  8 +++++++-
 2 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 14b1323..dafb6b5 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -545,17 +545,20 @@ static void update_remote_refs(const struct ref *refs,
 			       const struct ref *remote_head_points_at,
 			       const char *branch_top,
 			       const char *msg,
-			       struct transport *transport)
+			       struct transport *transport,
+			       int check_connectivity)
 {
 	const struct ref *rm = mapped_refs;
 
-	if (0 <= option_verbosity)
-		printf(_("Checking connectivity... "));
-	if (check_everything_connected_with_transport(iterate_ref_map,
-						      0, &rm, transport))
-		die(_("remote did not send all necessary objects"));
-	if (0 <= option_verbosity)
-		printf(_("done\n"));
+	if (check_connectivity) {
+		if (0 <= option_verbosity)
+			printf(_("Checking connectivity... "));
+		if (check_everything_connected_with_transport(iterate_ref_map,
+							      0, &rm, transport))
+			die(_("remote did not send all necessary objects"));
+		if (0 <= option_verbosity)
+			printf(_("done\n"));
+	}
 
 	if (refs) {
 		write_remote_refs(mapped_refs);
@@ -963,7 +966,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		transport_fetch_refs(transport, mapped_refs);
 
 	update_remote_refs(refs, mapped_refs, remote_head_points_at,
-			   branch_top.buf, reflog_msg.buf, transport);
+			   branch_top.buf, reflog_msg.buf, transport,
+			   !is_local);
 
 	update_head(our_head_points_at, remote_head, reflog_msg.buf);
 
diff --git a/t/t5710-info-alternate.sh b/t/t5710-info-alternate.sh
index 8956c21..5a6e49d 100755
--- a/t/t5710-info-alternate.sh
+++ b/t/t5710-info-alternate.sh
@@ -58,7 +58,13 @@ git clone -l -s F G &&
 git clone -l -s D E &&
 git clone -l -s E F &&
 git clone -l -s F G &&
-test_must_fail git clone --bare -l -s G H'
+git clone --bare -l -s G H'
+
+test_expect_success 'invalidity of deepest repository' \
+'cd H && {
+	test_valid_repo
+	test $? -ne 0
+}'
 
 cd "$base_dir"
 
-- 
1.8.3.rc3.24.gec82cb9

Re: A local shared clone is now much slower

[Duy Nguyen]

On Mon, Jul 8, 2013 at 2:30 PM, Jeff King <peff@peff.net> wrote:
> Subject: [PATCH] clone: drop connectivity check for local clones
>
> Commit 0433ad1 (clone: run check_everything_connected,
> 2013-03-25) added the same connectivity check to clone that
> we use for fetching. The intent was to provide enough safety
> checks that "git clone git://..." could be counted on to
> detect bit errors and other repo corruption, and not
> silently propagate them to the clone.
>
> For local clones, this turns out to be a bad idea, for two
> reasons:
>
>   1. Local clones use hard linking (or even shared object
>      stores), and so complete far more quickly. The time
>      spent on the connectivity check is therefore
>      proportionally much more painful.

There's also byte-to-byte copy when system does not support hardlinks
(or the user does not want it) but I guess it's safe to trust the OS
to copy correctly in most cases.

>   2. Local clones do not actually meet our safety guarantee
>      anyway. The connectivity check makes sure we have all
>      of the objects we claim to, but it does not check for
>      bit errors. We will notice bit errors in commits and
>      trees, but we do not load blob objects at all. Whereas
>      over the pack transport, we actually recompute the sha1
>      of each object in the incoming packfile; bit errors
>      change the sha1 of the object, which is then caught by
>      the connectivity check.

We used to, before d21c463 (fetch/receive: remove over-pessimistic
connectivity check - 2012-03-15). But back then we did not even do
connectivity check in clone.

> This patch drops the connectivity check in the local case.
> Note that we have to revert the changes from 0433ad1 to
> t5710, as we no longer notice the corruption during clone.
>
> We could go a step further and provide a "verify even local
> clones" option, but it is probably not worthwhile. You can
> already spell that as "cd foo.git && git fsck && git clone ."
> or as "git clone --no-local foo.git".

Faster clones make everybody happy :-)
--
Duy

Re: A local shared clone is now much slower

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Mon, Jul 8, 2013 at 2:30 PM, Jeff King <peff@peff.net> wrote:
>> Subject: [PATCH] clone: drop connectivity check for local clones
>>
>> Commit 0433ad1 (clone: run check_everything_connected,
>> 2013-03-25) added the same connectivity check to clone that
>> we use for fetching. The intent was to provide enough safety
>> checks that "git clone git://..." could be counted on to
>> detect bit errors and other repo corruption, and not
>> silently propagate them to the clone.
>>
>> For local clones, this turns out to be a bad idea, for two
>> reasons:
>>
>>   1. Local clones use hard linking (or even shared object
>>      stores), and so complete far more quickly. The time
>>      spent on the connectivity check is therefore
>>      proportionally much more painful.
>
> There's also byte-to-byte copy when system does not support hardlinks
> (or the user does not want it) but I guess it's safe to trust the OS
> to copy correctly in most cases.

While that may be true, I do not think it matters that much.  The
check during transport is meant to guard against not just corruption
during the object transfer, but also against a corrupt source
repository, and your trust on "cp -R" only covers the "transfer"
part.  And that makes 2. below very relevant.

>>   2. Local clones do not actually meet our safety guarantee
>>      anyway.
>>      ...
>
> Faster clones make everybody happy :-)

Yup.

I think this deserves to be backported to 'maint' track for
1.8.3.x.  Here is an attempt to do so.

 builtin/clone.c           | 11 +++++++----
 t/t5710-info-alternate.sh |  8 +++++++-
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 035ab64..38a0a64 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -542,12 +542,15 @@ static void update_remote_refs(const struct ref *refs,
 			       const struct ref *mapped_refs,
 			       const struct ref *remote_head_points_at,
 			       const char *branch_top,
-			       const char *msg)
+			       const char *msg,
+			       int check_connectivity)
 {
 	const struct ref *rm = mapped_refs;
 
-	if (check_everything_connected(iterate_ref_map, 0, &rm))
-		die(_("remote did not send all necessary objects"));
+	if (check_connectivity) {
+		if (check_everything_connected(iterate_ref_map, 0, &rm))
+			die(_("remote did not send all necessary objects"));
+	}
 
 	if (refs) {
 		write_remote_refs(mapped_refs);
@@ -950,7 +953,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		transport_fetch_refs(transport, mapped_refs);
 
 	update_remote_refs(refs, mapped_refs, remote_head_points_at,
-			   branch_top.buf, reflog_msg.buf);
+			   branch_top.buf, reflog_msg.buf, !is_local);
 
 	update_head(our_head_points_at, remote_head, reflog_msg.buf);
 
diff --git a/t/t5710-info-alternate.sh b/t/t5710-info-alternate.sh
index 8956c21..5a6e49d 100755
--- a/t/t5710-info-alternate.sh
+++ b/t/t5710-info-alternate.sh
@@ -58,7 +58,13 @@ test_expect_success 'creating too deep nesting' \
 git clone -l -s D E &&
 git clone -l -s E F &&
 git clone -l -s F G &&
-test_must_fail git clone --bare -l -s G H'
+git clone --bare -l -s G H'
+
+test_expect_success 'invalidity of deepest repository' \
+'cd H && {
+	test_valid_repo
+	test $? -ne 0
+}'
 
 cd "$base_dir"
 

Re: A local shared clone is now much slower

[Jeff King]

On Mon, Jul 08, 2013 at 08:00:09AM -0700, Junio C Hamano wrote:

> I think this deserves to be backported to 'maint' track for
> 1.8.3.x.  Here is an attempt to do so.

Agreed. As it makes certain local-clone workflows really painful, I
think my original can be considered a performance regression for those
cases.

Your back-port looks good to me. Thanks.

-Peff

Priming git clone with a local repo?

[Andreas Krey]

Hi, dear listers,

I'm wondering if there is (or will be) a way of doing almost

  git clone --reference localrepo host:canonrep

Basically, I don't want the implications of --reference but still the
performance advantages of reusing local objects/pack files. I probably
have to go and first do a

  git clone -s -n localrepo canonrepo

and then go into canonrepo, fix up the remote, fetch, and properly
reset the local branch. Is there an easier way of doing a
--reference-but-hardlink-objects-instead-of-setting-alternate
within git? Or implementing it?

Andreas

-- 
"Totally trivial. Famous last words."
From: Linus Torvalds <torvalds@*.org>
Date: Fri, 22 Jan 2010 07:29:21 -0800

Re: Priming git clone with a local repo?

[Junio C Hamano]

Andreas Krey <a.krey@gmx.de> writes:

> I'm wondering if there is (or will be) a way of doing almost
>
>   git clone --reference localrepo host:canonrep
>
> Basically, I don't want the implications of --reference but still the
> performance advantages of reusing local objects/pack files.

I think the standard operating procedure for that is to still clone
with the "--reference" option first, and then do "repack -a -d" in
the resulting clone (note the lack of "-l" option in "repack").

After that repack, you should be able to discard the alternates
file.

    $ git clone --reference ../there otherhost:repo ours.git
    $ cd ours.git
    $ git repack -a -d ;# no "-l"!!!
    $ mv .git/objects/info/alternates .git/objects/info/alter-nates
    $ git fsck
    $ rm .git/objects/info/alter-nates

Re: A local shared clone is now much slower

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Jul 08, 2013 at 01:03:55PM +1000, Stephen Rothwell wrote:
>
>> So commit 0433ad128c59 ("clone: run check_everything_connected") (which
>> turned up with v1.8.3) added a large traversal to clone which (as the
>> comment said) makes a clone much slower.  It is especially noticeable on
>> "git clone -s -l -n" which I use every day and used to be almost
>> instant.  Is there any thought to making it fast again, please?
>> 
>> The above clone is very useful for working with different branches in one
>> tree without touching every file in the main branch you are working
>> with (and consequent issues with rebuilding at least).  As linux-next
>> maintainer, you can imagine that I do this a bit.
>
> Yeah, I have noticed it is somewhat annoying, as well, because the
> proportion of time taken for the check is so much larger compared to the
> relatively instant time taken for the local shared clone.
>
> The point of that commit is to add the same safety checks to clone that
> we do for fetching. But in the local shared-repo case, I really feel
> like all safety bets are off anyway. You are not creating a verified
> redundant copy at all, and there are still corruptions that can sneak
> through (e.g., bit corruptions of blob objects).

Yeah, I was thinking the same when I saw that report, so obviously I
think the approacch makes sense ;-)

Thanks.

>
> So maybe this:
>
> -- >8 --
> Subject: [PATCH] clone: drop connectivity check for local clones
>
> Commit 0433ad1 (clone: run check_everything_connected,
> 2013-03-25) added the same connectivity check to clone that
> we use for fetching. The intent was to provide enough safety
> checks that "git clone git://..." could be counted on to
> detect bit errors and other repo corruption, and not
> silently propagate them to the clone.
>
> For local clones, this turns out to be a bad idea, for two
> reasons:
>
>   1. Local clones use hard linking (or even shared object
>      stores), and so complete far more quickly. The time
>      spent on the connectivity check is therefore
>      proportionally much more painful.
>
>   2. Local clones do not actually meet our safety guarantee
>      anyway. The connectivity check makes sure we have all
>      of the objects we claim to, but it does not check for
>      bit errors. We will notice bit errors in commits and
>      trees, but we do not load blob objects at all. Whereas
>      over the pack transport, we actually recompute the sha1
>      of each object in the incoming packfile; bit errors
>      change the sha1 of the object, which is then caught by
>      the connectivity check.
>
> This patch drops the connectivity check in the local case.
> Note that we have to revert the changes from 0433ad1 to
> t5710, as we no longer notice the corruption during clone.
>
> We could go a step further and provide a "verify even local
> clones" option, but it is probably not worthwhile. You can
> already spell that as "cd foo.git && git fsck && git clone ."
> or as "git clone --no-local foo.git".
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/clone.c           | 22 +++++++++++++---------
>  t/t5710-info-alternate.sh |  8 +++++++-
>  2 files changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 14b1323..dafb6b5 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -545,17 +545,20 @@ static void update_remote_refs(const struct ref *refs,
>  			       const struct ref *remote_head_points_at,
>  			       const char *branch_top,
>  			       const char *msg,
> -			       struct transport *transport)
> +			       struct transport *transport,
> +			       int check_connectivity)
>  {
>  	const struct ref *rm = mapped_refs;
>  
> -	if (0 <= option_verbosity)
> -		printf(_("Checking connectivity... "));
> -	if (check_everything_connected_with_transport(iterate_ref_map,
> -						      0, &rm, transport))
> -		die(_("remote did not send all necessary objects"));
> -	if (0 <= option_verbosity)
> -		printf(_("done\n"));
> +	if (check_connectivity) {
> +		if (0 <= option_verbosity)
> +			printf(_("Checking connectivity... "));
> +		if (check_everything_connected_with_transport(iterate_ref_map,
> +							      0, &rm, transport))
> +			die(_("remote did not send all necessary objects"));
> +		if (0 <= option_verbosity)
> +			printf(_("done\n"));
> +	}
>  
>  	if (refs) {
>  		write_remote_refs(mapped_refs);
> @@ -963,7 +966,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  		transport_fetch_refs(transport, mapped_refs);
>  
>  	update_remote_refs(refs, mapped_refs, remote_head_points_at,
> -			   branch_top.buf, reflog_msg.buf, transport);
> +			   branch_top.buf, reflog_msg.buf, transport,
> +			   !is_local);
>  
>  	update_head(our_head_points_at, remote_head, reflog_msg.buf);
>  
> diff --git a/t/t5710-info-alternate.sh b/t/t5710-info-alternate.sh
> index 8956c21..5a6e49d 100755
> --- a/t/t5710-info-alternate.sh
> +++ b/t/t5710-info-alternate.sh
> @@ -58,7 +58,13 @@ git clone -l -s F G &&
>  git clone -l -s D E &&
>  git clone -l -s E F &&
>  git clone -l -s F G &&
> -test_must_fail git clone --bare -l -s G H'
> +git clone --bare -l -s G H'
> +
> +test_expect_success 'invalidity of deepest repository' \
> +'cd H && {
> +	test_valid_repo
> +	test $? -ne 0
> +}'
>  
>  cd "$base_dir"