Poor push performance with large number of refs

> I have a repository that's just under 2 GiB in size and contains over
> 20000 refs, with a copy of it on a server.  Both sides are using Git
> 2.1.2.  If I push a branch that contains a single commit, it takes about
> 15 seconds to push.  However, if everything is up-to-date, it completes
> within 2 seconds.  Notably, HTTPS performs the same as SSH.
>
> Most of the time is spent between the "Pushing to remote machine" and
> "Counting objects", running git pack-objects:
>
>   git pack-objects --all-progress-implied --revs --stdout --thin --delta-base-offset --progress
>
> Unfortunately, -vvv doesn't provide any helpful output.  I have some
> suspicions what's going on here, but no hard data.  Where should I
> be looking to determine the bottleneck?

> On Tue, Dec 9, 2014 at 4:37 PM, brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> > Most of the time is spent between the "Pushing to remote machine" and
> > "Counting objects", running git pack-objects:
> >
> >   git pack-objects --all-progress-implied --revs --stdout --thin --delta-base-offset --progress
> >
> > Unfortunately, -vvv doesn't provide any helpful output.  I have some
> > suspicions what's going on here, but no hard data.  Where should I
> > be looking to determine the bottleneck?
> 
> My guess is the revision queue is struggling to insert 20,000 commits
> that the remote side "has", are uninteresting, and should not be
> transmitted. This queue insertion usually requires parsing the commit
> object out of the local object store to get the commit timestamp, then
> bubble sort inserting that commit into the queue.

> From: "brian m. carlson" <brian.carlson@cpanel.net>
> 
> Thin packs are enabled when pushing by default, but with a large number
> of refs and a fast network, git may spend more time trying to find a
> good delta than it would spend simply sending data over the network.
> Add a per-remote option, pushThin, that controls the default for pushes
> on that remote.

> Signed-off-by: brian m. carlson <brian.carlson@cpanel.net>
> ---
>  SOURCES/git/Documentation/config.txt | 6 ++++++
>  SOURCES/git/builtin/push.c           | 6 ++++--
>  SOURCES/git/remote.c                 | 3 +++
>  SOURCES/git/remote.h                 | 1 +
>  SOURCES/git/transport.c              | 2 ++
>  5 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/SOURCES/git/Documentation/config.txt b/SOURCES/git/Documentation/config.txt
> index 9220725..7fededd 100644
> --- a/SOURCES/git/Documentation/config.txt
> +++ b/SOURCES/git/Documentation/config.txt
> @@ -2178,6 +2178,12 @@ remote.<name>.push::
>  	The default set of "refspec" for linkgit:git-push[1]. See
>  	linkgit:git-push[1].
>  
> +remote.<name>.pushThin::
> +	If true (the default), pass the \--thin option to
> +	linkgit:git-pack-objects[1] during push.  This results in a smaller
> +	pack being sent and can improve push time over slow networks.  Over
> +	fast networks, setting this value to false may improve performance.
> +
>  remote.<name>.mirror::
>  	If true, pushing to this remote will automatically behave
>  	as if the `--mirror` option was given on the command line.
> diff --git a/SOURCES/git/builtin/push.c b/SOURCES/git/builtin/push.c
> index a076b19..ae39677 100644
> --- a/SOURCES/git/builtin/push.c
> +++ b/SOURCES/git/builtin/push.c
> @@ -15,7 +15,7 @@ static const char * const push_usage[] = {
>  	NULL,
>  };
>  
> -static int thin = 1;
> +static int thin = -1;
>  static int deleterefs;
>  static const char *receivepack;
>  static int verbosity;
> @@ -347,7 +347,9 @@ static int push_with_options(struct transport *transport, int flags)
>  	if (receivepack)
>  		transport_set_option(transport,
>  				     TRANS_OPT_RECEIVEPACK, receivepack);
> -	transport_set_option(transport, TRANS_OPT_THIN, thin ? "yes" : NULL);
> +
> +	if (thin != -1)
> +		transport_set_option(transport, TRANS_OPT_THIN, thin ? "yes" : NULL);
>  
>  	if (!is_empty_cas(&cas)) {
>  		if (!transport->smart_options)
> diff --git a/SOURCES/git/remote.c b/SOURCES/git/remote.c
> index f624217..54777cb 100644
> --- a/SOURCES/git/remote.c
> +++ b/SOURCES/git/remote.c
> @@ -175,6 +175,7 @@ static struct remote *make_remote(const char *name, int len)
>  
>  	ret = xcalloc(1, sizeof(struct remote));
>  	ret->prune = -1;  /* unspecified */
> +	ret->push_thin = 1; /* default on */
>  	ALLOC_GROW(remotes, remotes_nr + 1, remotes_alloc);
>  	remotes[remotes_nr++] = ret;
>  	ret->name = xstrndup(name, len);
> @@ -445,6 +446,8 @@ static int handle_config(const char *key, const char *value, void *cb)
>  		if (git_config_string(&v, key, value))
>  			return -1;
>  		add_push_refspec(remote, v);
> +	} else if (!strcmp(subkey, ".pushthin")) {
> +		remote->push_thin = git_config_bool(key, value);
>  	} else if (!strcmp(subkey, ".fetch")) {
>  		const char *v;
>  		if (git_config_string(&v, key, value))
> diff --git a/SOURCES/git/remote.h b/SOURCES/git/remote.h
> index 8b62efd..badf266 100644
> --- a/SOURCES/git/remote.h
> +++ b/SOURCES/git/remote.h
> @@ -46,6 +46,7 @@ struct remote {
>  	int skip_default_update;
>  	int mirror;
>  	int prune;
> +	int push_thin;
>  
>  	const char *receivepack;
>  	const char *uploadpack;
> diff --git a/SOURCES/git/transport.c b/SOURCES/git/transport.c
> index 70d38e4..2f495fa 100644
> --- a/SOURCES/git/transport.c
> +++ b/SOURCES/git/transport.c
> @@ -987,6 +987,8 @@ struct transport *transport_get(struct remote *remote, const char *url)
>  			ret->smart_options->receivepack = remote->receivepack;
>  	}
>  
> +	transport_set_option(ret, TRANS_OPT_THIN, remote->push_thin ? "yes" : NULL);
> +
>  	return ret;
>  }
>  
> -- 
> 2.2.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

> I looked at this more in depth today and I found that the bottleneck is
> --thin.  I tried git send-pack, which does not use --thin by default,
> which led me to further testing.  A particular push went from 24 seconds
> with --thin to 4 seconds without.
>
> I agree that the large number of refs is at least part of the problem,
> because reducing the number of refs has a slight but noticeable impact.
> It's also the factor I can least control.
>
> I have a patch which allows per-remote configuration of whether to use
> thin packs (which I will send shortly), but I'm wondering if we can do
> better, especially since --thin is the default.  It looks like --thin
> forces pack-objects to do its own lookup (essentially a rev-list)
> instead of using the values provided on stdin.

> It could be a regression by fbd4a70 (list-objects: mark more commits
> as edges in mark_edges_uninteresting - 2013-08-16). That commit makes
> --thin a lot more agressive (reading lots of trees). You can try to
> revert that commit (or use a git version without that commit) and see
> if it improves performance. If so, we probably want to enable that
> code for shallow repos only.

> In commit fbd4a70 (list-objects: mark more commits as edges in
> mark_edges_uninteresting - 2013-08-16), we made --thin much more
> aggressive by reading lots more trees.  This produces much smaller packs
> for shallow clones; however, it causes a significant performance
> regression for non-shallow clones with lots of refs (23.322 seconds vs.
> 4.785 seconds with 22400 refs).  Limit this extra edge-marking to
> shallow clones to avoid poor performance.

> In commit fbd4a70 (list-objects: mark more commits as edges in
> mark_edges_uninteresting - 2013-08-16), we made --thin much more
> aggressive by reading lots more trees.  This produces much smaller packs
> for shallow clones; however, it causes a significant performance
> regression for non-shallow clones with lots of refs (23.322 seconds vs.
> 4.785 seconds with 22400 refs).  Limit this extra edge-marking to
> shallow clones to avoid poor performance.

> This change affects non-clone/fetch uses of object listing depending
> on the shallowness of the repository, and does not even care if it
> is driven as part of the pack-object codepath, if I am reading it
> correctly.  It smells wrong.
>
> The problematic fbd4a70 already had unintended fallout that needed
> to be corrected with 200abe74 (list-objects: only look at cmdline
> trees with edge_hint, 2014-01-20).  The current code with the fix,
> the decision to use the more expensive marking is tied to
> "edge_hint". I notice that edge_hint is turned on only if the caller
> of rev-list passes the "--objects-edge" option, and currently that
> only happens in the pack-objects codepath when "thin" is given.
> Perhaps that part should decide if it really wants to do edge_hint
> depending on the shallowness of the repository perhaps?
>
> That is, something like this instead?

>
>  builtin/pack-objects.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 3f9f5c7..a9ebf56 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -2709,7 +2709,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
>  		usage_with_options(pack_usage, pack_objects_options);
>  
>  	argv_array_push(&rp, "pack-objects");
> -	if (thin) {
> +	if (thin && is_repository_shallow()) {
>  		use_internal_rev_list = 1;
>  		argv_array_push(&rp, "--objects-edge");
>  	} else

> The right approach would be more like allocating one more bit in
> struct rev_info (call that edge_hint_aggressive), give a new option
> "--objects-edge-aggressive", and do something like
> 
> 	if (thin) {
>         	use_internal_rev_list = 1;
> 		argv_array_push(&rp, is_repository_shallow()
>                 	? "--objects-edge-aggressive"
>                         : "--objects-edge");
> 	}
> 
> in this codepath?  I'd actually suggest is_repository_shallow()
> detection to happen one level even higher (i.e. make decision at the
> caller of pack-objects) and decide to pass either "--thin" or
> "--thin-aggressive", so that we can make sure that the damage caused
> by fbd4a70 to be limited only to fetches into shallow repository
> with stronger confidence.

> I have a repository that's just under 2 GiB in size and contains over
> 20000 refs, with a copy of it on a server.  Both sides are using Git
> 2.1.2.  If I push a branch that contains a single commit, it takes about
> 15 seconds to push.  However, if everything is up-to-date, it completes
> within 2 seconds.  Notably, HTTPS performs the same as SSH.
>
> Most of the time is spent between the "Pushing to remote machine" and
> "Counting objects", running git pack-objects:
>
>   git pack-objects --all-progress-implied --revs --stdout --thin --delta-base-offset --progress
>
> Unfortunately, -vvv doesn't provide any helpful output.  I have some
> suspicions what's going on here, but no hard data.  Where should I
> be looking to determine the bottleneck?