Re: upload-pack is slow with lots of refs

Jeff King [off-list ref] writes:

quoted

Has there been any work on extending the protocol so that the client
tells the server what refs it's interested in?

I don't think so. It would be hard to do in a backwards-compatible way,
because the advertisement is the first thing the server says, before it
has negotiated any capabilities with the client at all.

That is being discussed but hasn't surfaced on the list.

On Wed, Oct 03, 2012 at 11:53:35AM -0700, Junio C Hamano wrote:

Jeff King [off-list ref] writes:

quoted

quoted

Has there been any work on extending the protocol so that the client
tells the server what refs it's interested in?

I don't think so. It would be hard to do in a backwards-compatible way,
because the advertisement is the first thing the server says, before it
has negotiated any capabilities with the client at all.

That is being discussed but hasn't surfaced on the list.

Out of curiosity, how are you thinking about triggering such a new
behavior in a backwards-compatible way? Invoke git-upload-pack2, and
fall back to reconnecting to start git-upload-pack if it fails?

-Peff

On Wed, Oct 03, 2012 at 12:41:38PM -0700, Shawn O. Pearce wrote:

quoted

Out of curiosity, how are you thinking about triggering such a new
behavior in a backwards-compatible way? Invoke git-upload-pack2, and
fall back to reconnecting to start git-upload-pack if it fails?

Basically, yes. New clients connect for git-upload-pack2. Over git://
the remote peer will just close the TCP socket with no messages. The
client can fallback to git-upload-pack and try again. Over SSH a
similar thing will happen in the sense there is no data output from
the remote side, so the client can try again. This has the downside of
authentication twice over SSH, which may prompt for a password twice.
But the user can get out of this by setting remote.NAME.uploadpack =
git-upload-pack and thus force the Git client to use the current
protocol if they have a new client and must continue to work over SSH
with an old server, and don't use an ssh-agent.

It's a shame that we have to reestablish the TCP or ssh connection to do
the retry. The password thing is annoying, but also it just wastes a
round-trip. It means we'd probably want to default the v2 probe to off
(and let the user turn it on for a specific remote) until v2 is much
more common than v1. Otherwise everyone pays the price.

It may also be worth designing v2 to handle more graceful capability
negotiation so this doesn't come up again.

Another alternative would be to tweak git-daemon to allow more graceful
fallback. That wouldn't help us now, but it would if we ever wanted a
v3. For stock ssh, you could send:

  sh -c 'git upload-pack2; test $? = 127 && git-upload-pack'

which would work if you have an unrestricted shell on the other side.
But it would break for a restricted shell or other "fake" ssh
environment. It's probably too ugly to have restricted shells recognize
that as a magic token (well, I could maybe even live with the ugliness,
but it is not strictly backwards compatible).

I was hoping we could do something like "git upload-pack --v2", but I'm
pretty sure current git-daemon would reject that.

Over HTTP we can request ?service=git-upload-pack2 and retry just like
git:// would, or be a bit smarter and say
?service=git-upload-pack&v=2, and determine the protocol support of
the remote peer based on the response we get. If we see an immediate
advertisement its still the "v1" protocol, if we get back the "yes I
speak v2" response like git:// would see, we can continue the
conversation from there.

Yeah, I would think "&v=2" would be better simply to avoid the
round-trip if we fail. It should be safe to turn the new protocol on by
default for http, then.

-Peff

On Wed, Oct 3, 2012 at 11:55 AM, Jeff King [off-list ref] wrote:

On Wed, Oct 03, 2012 at 11:53:35AM -0700, Junio C Hamano wrote:

quoted

Jeff King [off-list ref] writes:

quoted

quoted

Has there been any work on extending the protocol so that the client
tells the server what refs it's interested in?

I don't think so. It would be hard to do in a backwards-compatible way,
because the advertisement is the first thing the server says, before it
has negotiated any capabilities with the client at all.

That is being discussed but hasn't surfaced on the list.

Out of curiosity, how are you thinking about triggering such a new
behavior in a backwards-compatible way? Invoke git-upload-pack2, and
fall back to reconnecting to start git-upload-pack if it fails?

Basically, yes. New clients connect for git-upload-pack2. Over git://
the remote peer will just close the TCP socket with no messages. The
client can fallback to git-upload-pack and try again. Over SSH a
similar thing will happen in the sense there is no data output from
the remote side, so the client can try again. This has the downside of
authentication twice over SSH, which may prompt for a password twice.
But the user can get out of this by setting remote.NAME.uploadpack =
git-upload-pack and thus force the Git client to use the current
protocol if they have a new client and must continue to work over SSH
with an old server, and don't use an ssh-agent.

Over HTTP we can request ?service=git-upload-pack2 and retry just like
git:// would, or be a bit smarter and say
?service=git-upload-pack&v=2, and determine the protocol support of
the remote peer based on the response we get. If we see an immediate
advertisement its still the "v1" protocol, if we get back the "yes I
speak v2" response like git:// would see, we can continue the
conversation from there.

Am Mittwoch, 3. Oktober 2012, 16:13:16 schrieb Jeff King:

On Wed, Oct 03, 2012 at 12:41:38PM -0700, Shawn O. Pearce wrote:

quoted

quoted

Out of curiosity, how are you thinking about triggering such a new
behavior in a backwards-compatible way? Invoke git-upload-pack2, and
fall back to reconnecting to start git-upload-pack if it fails?

Basically, yes. New clients connect for git-upload-pack2. Over git://
the remote peer will just close the TCP socket with no messages. The
client can fallback to git-upload-pack and try again. Over SSH a
similar thing will happen in the sense there is no data output from
the remote side, so the client can try again. This has the downside of
authentication twice over SSH, which may prompt for a password twice.
But the user can get out of this by setting remote.NAME.uploadpack =
git-upload-pack and thus force the Git client to use the current
protocol if they have a new client and must continue to work over SSH
with an old server, and don't use an ssh-agent.

It's a shame that we have to reestablish the TCP or ssh connection to do
the retry. The password thing is annoying, but also it just wastes a
round-trip. It means we'd probably want to default the v2 probe to off
(and let the user turn it on for a specific remote) until v2 is much
more common than v1. Otherwise everyone pays the price.

Would it be possible to use this workflow:

- Every client connects per default to v1

- If server is capable of v2, it sends a flag along with the usual response
  (A v1 server will obviously not send that flag)

- If client is also capable of v2 and gets the flag, it enables v2 for
  just that remote (probably unless the user said, "i never want to")

- Next time the client connects to that remote it will use v2.

I'm not sure, if this is possible, since I think to remember that I have read 
in the Documentation folder something along the line: Capabilities announced 
from the server mean "I want you to use exactly these flags".

Sascha

On Thu, Oct 04, 2012 at 11:52:13PM +0200, Sascha Cunz wrote:

Would it be possible to use this workflow:

- Every client connects per default to v1

- If server is capable of v2, it sends a flag along with the usual response
  (A v1 server will obviously not send that flag)

That is more or less the strategy we use for existing extensions (your
"flag" is a space-separated list of capability strings). But in this
case, the idea would be to change what the "usual response" is. Since a
v1 client would be expecting the response, we must send it, but at that
point it is too late to make the change. So we need to see some flag
from the client before the server says anything.

And the problem is that the client sending that flag will break v1
servers, and the client would need to waste time doing a retry when
connecting to the (initially more common) v1 servers.

- If client is also capable of v2 and gets the flag, it enables v2 for
  just that remote (probably unless the user said, "i never want to")

- Next time the client connects to that remote it will use v2.

So yeah, that would work to help with the wasted time. We'd have
git-upload-pack2 to do the v2 protocol, but the v1 git-upload-pack for
the server would say "by the way, next time you connect, try v2 first".
So the client would have to store a version number for each remote.
Which is not too onerous.

Another way to think of it is phasing it in like this:

  1. Add v2 support to client and server. Initially, clients try only
     v1.

  2. Add a remote.*.preferProtocol config option, defaulting to v1. This
     lets people turn on v2 for remotes they know support it. If v2
     fails, still fall back to v1.

  3. Add a server upload-pack capability that says "by the way, try v2
     next time".  Have the client set the preferProtocol config option
     for a remote if we see that capability.

  4. Wait a while until v2 is very popular.

  5. Switch the default for preferProtocol to v2 (but still fall back to
     v1).

So always fall back and remain compatible, and let the config option
just be an optimization to avoid extra failed requests.

I'm not sure, if this is possible, since I think to remember that I have read 
in the Documentation folder something along the line: Capabilities announced 
from the server mean "I want you to use exactly these flags".

No, the server capability says "I can do this", and the client should
respond with "I want you to do this". Because the server might be
talking to an older client that does not know what "this" is, it must
handle the case that the capability does not come back.

-Peff

Am 10/3/2012 21:41, schrieb Shawn Pearce:

On Wed, Oct 3, 2012 at 11:55 AM, Jeff King [off-list ref] wrote:

quoted

On Wed, Oct 03, 2012 at 11:53:35AM -0700, Junio C Hamano wrote:

quoted

Jeff King [off-list ref] writes:

quoted

quoted

Has there been any work on extending the protocol so that the client
tells the server what refs it's interested in?

I don't think so. It would be hard to do in a backwards-compatible way,
because the advertisement is the first thing the server says, before it
has negotiated any capabilities with the client at all.

That is being discussed but hasn't surfaced on the list.

Out of curiosity, how are you thinking about triggering such a new
behavior in a backwards-compatible way? Invoke git-upload-pack2, and
fall back to reconnecting to start git-upload-pack if it fails?

Basically, yes. New clients connect for git-upload-pack2. Over git://
the remote peer will just close the TCP socket with no messages. The
client can fallback to git-upload-pack and try again. Over SSH a
similar thing will happen in the sense there is no data output from
the remote side, so the client can try again.

These connections are bidirectional. Upload-pack can just start
advertising refs in the "v1" way and announce a "v2" capability and listen
for response in parallel. A v2 capable client can start sending "wants" or
some other signal as soon as it sees the "v2" capability. Upload-pack,
which was listening for responses in parallel, can interrupt its
advertisements and continue with v2 protocol from here.

This sounds so simple (not the implementation, of course) - I must be
missing something.

-- Hannes

On Thu, Oct 4, 2012 at 11:24 PM, Johannes Sixt [off-list ref] wrote:

Am 10/3/2012 21:41, schrieb Shawn Pearce:

quoted

On Wed, Oct 3, 2012 at 11:55 AM, Jeff King [off-list ref] wrote:

quoted

On Wed, Oct 03, 2012 at 11:53:35AM -0700, Junio C Hamano wrote:

quoted

Jeff King [off-list ref] writes:

quoted

quoted

Has there been any work on extending the protocol so that the client
tells the server what refs it's interested in?

I don't think so. It would be hard to do in a backwards-compatible way,
because the advertisement is the first thing the server says, before it
has negotiated any capabilities with the client at all.

That is being discussed but hasn't surfaced on the list.

Out of curiosity, how are you thinking about triggering such a new
behavior in a backwards-compatible way? Invoke git-upload-pack2, and
fall back to reconnecting to start git-upload-pack if it fails?

Basically, yes. New clients connect for git-upload-pack2. Over git://
the remote peer will just close the TCP socket with no messages. The
client can fallback to git-upload-pack and try again. Over SSH a
similar thing will happen in the sense there is no data output from
the remote side, so the client can try again.

These connections are bidirectional.

Smart HTTP is not bidirectional.

Upload-pack can just start
advertising refs in the "v1" way and announce a "v2" capability and listen
for response in parallel. A v2 capable client can start sending "wants" or
some other signal as soon as it sees the "v2" capability. Upload-pack,
which was listening for responses in parallel, can interrupt its
advertisements and continue with v2 protocol from here.

This sounds so simple (not the implementation, of course) - I must be
missing something.

Smart HTTP is not bidirectional. The client can't cut off the server.
Its also more complex to code the server to listen for a stop command
from the client at the same time the server is blasting out useless
references to the client.

Am 05.10.2012 18:57, schrieb Shawn Pearce:

On Thu, Oct 4, 2012 at 11:24 PM, Johannes Sixt [off-list ref] wrote:

quoted

Upload-pack can just start
advertising refs in the "v1" way and announce a "v2" capability and listen
for response in parallel. A v2 capable client can start sending "wants" or
some other signal as soon as it sees the "v2" capability. Upload-pack,
which was listening for responses in parallel, can interrupt its
advertisements and continue with v2 protocol from here.

This sounds so simple (not the implementation, of course) - I must be
missing something.

Smart HTTP is not bidirectional. The client can't cut off the server.

Smart HTTP does not need it: you already posted a better solution (I'm
refering to "&v=2").

Its also more complex to code the server to listen for a stop command
from the client at the same time the server is blasting out useless
references to the client.

At least the server side does not seem to be that complex. See below.
Of course, the server blasted out some refs, but I'm confident that in
practice the client will be able to signal v2 capability after a few packets
of advertisements. You can switch on TCP_NODELAY for the first line with
the capabilities to ensure it goes out on the wire ASAP.

diff --git a/upload-pack.c b/upload-pack.c
index 2e90ccb..c29ae04 100644
--- a/upload-pack.c
+++ b/upload-pack.c

@@ -720,11 +720,20 @@ static void receive_needs(void)
 	free(shallows.objects);
 }
 
+static int client_spoke(void)
+{
+	struct pollfd pfd;
+	pfd.fd = 0;
+	pfd.events = POLLIN;
+	return poll(&pfd, 1, 0) > 0 &&
+		(pfd.revents & (POLLIN|POLLHUP));
+}
+
 static int send_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
 {
 	static const char *capabilities = "multi_ack thin-pack side-band"
 		" side-band-64k ofs-delta shallow no-progress"
-		" include-tag multi_ack_detailed";
+		" include-tag multi_ack_detailed version2";
 	struct object *o = lookup_unknown_object(sha1);
 	const char *refname_nons = strip_namespace(refname);
 

@@ -752,7 +761,8 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo
 		if (o)
 			packet_write(1, "%s %s^{}\n", sha1_to_hex(o->sha1), refname_nons);
 	}
-	return 0;
+
+	return client_spoke();
 }
 
 static int mark_our_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)

@@ -771,8 +781,14 @@ static void upload_pack(void)
 {
 	if (advertise_refs || !stateless_rpc) {
 		reset_timeout();
-		head_ref_namespaced(send_ref, NULL);
-		for_each_namespaced_ref(send_ref, NULL);
+		if (head_ref_namespaced(send_ref, NULL) ||
+		    for_each_namespaced_ref(send_ref, NULL)) {
+			/*
+			 * TODO: continue with protocol version 2
+			 * optimization: do not send refs
+			 * that were already sent
+			 */
+		}
 		packet_flush(1);
 	} else {
 		head_ref_namespaced(mark_our_ref, NULL);

On Mon, Oct 8, 2012 at 8:05 AM, Johannes Sixt [off-list ref] wrote:

Am 05.10.2012 18:57, schrieb Shawn Pearce:

quoted

On Thu, Oct 4, 2012 at 11:24 PM, Johannes Sixt [off-list ref] wrote:

quoted

Upload-pack can just start
advertising refs in the "v1" way and announce a "v2" capability and listen
for response in parallel. A v2 capable client can start sending "wants" or
some other signal as soon as it sees the "v2" capability. Upload-pack,
which was listening for responses in parallel, can interrupt its
advertisements and continue with v2 protocol from here.

This sounds so simple (not the implementation, of course) - I must be
missing something.

Smart HTTP is not bidirectional. The client can't cut off the server.

Smart HTTP does not need it: you already posted a better solution (I'm
refering to "&v=2").

Yes but then it diverges even further from the native bidirectional protocol.

quoted

Its also more complex to code the server to listen for a stop command
from the client at the same time the server is blasting out useless
references to the client.

At least the server side does not seem to be that complex. See below.
Of course, the server blasted out some refs, but I'm confident that in
practice the client will be able to signal v2 capability after a few packets
of advertisements. You can switch on TCP_NODELAY for the first line with
the capabilities to ensure it goes out on the wire ASAP.

...

+static int client_spoke(void)
+{
+       struct pollfd pfd;
+       pfd.fd = 0;
+       pfd.events = POLLIN;
+       return poll(&pfd, 1, 0) > 0 &&
+               (pfd.revents & (POLLIN|POLLHUP));

Except doing this in Java is harder on an arbitrary InputStream type.
I guess we really only care about basic TCP, in which case we can use
NIO to implement an emulation of poll, and SSH, where MINA SSHD
probably doesn't provide a way to see if the client has given us data
without blocking. That makes supporting v2 really hard in e.g. Gerrit
Code Review. You could argue that its improper to attempt to implement
a network protocol in a language whose standard libraries have gone
out of their way to prevent you from polling to see if data is
immediately available, but I prefer to ignore such arguments.

As it turns out we don't really have this problem with git://. Clients
can bury a v2 request in the extended headers where the host line
appears today. Its a bit tricky because of that \0 bug causing
infinite looping, but IIRC using \0\0 is safe even against ancient
servers. So git:// and http:// both have a way where the client can
ask for v2 support before the server speaks, and have it transparently
be ignored by ancient servers.


The only place we have a problem is SSH. That exec of the remote
binary is just super-strict. Its good to be paranoid, but its also
locked out any chance we have at doing the upgrade over SSH without
having to run two SSH commands in the worst case. I guess the best
approach is to try the v1 protocol by default, have the remote
advertise it supports v2, and remember this on a per-host basis in
~/.gitconfig for future requests. Users could always force a specific
preference with remote.NAME.uploadpack variable or --uploadpack
command line flag.

Am 09.10.2012 08:46, schrieb Shawn Pearce:

On Mon, Oct 8, 2012 at 8:05 AM, Johannes Sixt [off-list ref] wrote:

quoted

Am 05.10.2012 18:57, schrieb Shawn Pearce:

quoted

Smart HTTP is not bidirectional. The client can't cut off the server.

Smart HTTP does not need it: you already posted a better solution (I'm
refering to "&v=2").

Yes but then it diverges even further from the native bidirectional protocol.

I won't argue here because I know next to nothing about Smart HTTP. But
it sounds like you either have compatibility, but a diverging protocol
or at least implementation, or no compatibility.

quoted

+static int client_spoke(void)
+{
+       struct pollfd pfd;
+       pfd.fd = 0;
+       pfd.events = POLLIN;
+       return poll(&pfd, 1, 0) > 0 &&
+               (pfd.revents & (POLLIN|POLLHUP));

Except doing this in Java is harder on an arbitrary InputStream type.
I guess we really only care about basic TCP, in which case we can use
NIO to implement an emulation of poll, and SSH, where MINA SSHD
probably doesn't provide a way to see if the client has given us data
without blocking. That makes supporting v2 really hard in e.g. Gerrit
Code Review. You could argue that its improper to attempt to implement
a network protocol in a language whose standard libraries have gone
out of their way to prevent you from polling to see if data is
immediately available, but I prefer to ignore such arguments.

Can't you read the inbound stream in a second thread while the first
thread writes the advertisements to the outbound stream? Then you don't
even need to poll; you can just read the 4-byte length header, stash it
away and set a flag. The implementation of client_spoke() would only
amount to check that flag.

As it turns out we don't really have this problem with git://. Clients
can bury a v2 request in the extended headers where the host line
appears today.

I tried, but it seems that todays git-daemons are too strict and accept
only \0host=foo\0, nothing else :-(

-- Hannes

Am 09.10.2012 22:30, schrieb Johannes Sixt:

Am 09.10.2012 08:46, schrieb Shawn Pearce:

quoted

As it turns out we don't really have this problem with git://. Clients
can bury a v2 request in the extended headers where the host line
appears today.

I tried, but it seems that todays git-daemons are too strict and accept
only \0host=foo\0, nothing else :-(

I take that back: Modern git-daemons accept "\0host=foo\0\0version=2\0",
as you said.

It looks like SSH is the only stubborn protocol.

-- Hannes