Re: [PATCH 1/2] upload-pack: avoid parsing objects during ref advertisement
From: Jeff King <hidden>
Date: 2016-06-15 22:55:52
Possibly related (same subject, not in this thread)
- 2016-06-15 · Re: [PATCH 1/2] upload-pack: avoid parsing objects during ref advertisement · Shawn Pearce <hidden>
- 2016-06-15 · Re: [PATCH 1/2] upload-pack: avoid parsing objects during ref advertisement · Jeff King <hidden>
- 2016-06-15 · Re: [PATCH 1/2] upload-pack: avoid parsing objects during ref advertisement · Junio C Hamano <hidden>
- 2016-06-15 · [PATCH 1/2] upload-pack: avoid parsing objects during ref advertisement · Jeff King <hidden>
On Fri, Jan 18, 2013 at 03:12:52PM -0800, Junio C Hamano wrote:
Jeff King [off-list ref] writes:quoted
When we advertise a ref, the first thing we do is parse the pointed-to object. This gives us two things: 1. a "struct object" we can use to store flags 2. the type of the object, so we know whether we need to dereference it as a tag Instead, we can just use lookup_unknown_object to get an object struct, and then fill in just the type field using sha1_object_info (which, in the case of packed files, can find the information without actually inflating the object data). This can save time if you have a large number of refs, and the client isn't actually going to request those refs (e.g., because most of them are already up-to-date).This is an old news, but do you recall why this patch did not update the code in mark_our_ref() that gets "struct object *o" from parse_object() the same way and mark them with OUR_REF flag? I was wondering about code consolidation like this.
It was just because I did my measuring on raw upload-pack, so I didn't notice that mark_our_ref was doing the same potentially slow thing. We only call mark_our_ref during the second half of the stateless-rpc conversation, and I did not measure that (and it would be a pain to do so in isolation). But it should be able to get the exact same speedups that we get from send_ref. It probably matters less in the long run, because the advertising phase is going to be called more frequently (e.g., for every no-op fetch), and once we are calling mark_our_ref, we are presumably about to do do actual packing work. However, there's no reason not to get what speed we can there, too.
quoted hunk ↗ jump to hunk
diff --git a/upload-pack.c b/upload-pack.c index 95d8313..609cd6c 100644 --- a/upload-pack.c +++ b/upload-pack.c@@ -722,15 +722,18 @@ static void receive_needs(void) free(shallows.objects); } +static int mark_our_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data); + 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"; - struct object *o = lookup_unknown_object(sha1); const char *refname_nons = strip_namespace(refname); unsigned char peeled[20]; + mark_our_ref(refname, sha1, flag, cb_data); + if (capabilities) packet_write(1, "%s %s%c%s%s agent=%s\n", sha1_to_hex(sha1), refname_nons,@@ -740,10 +743,6 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo else packet_write(1, "%s %s\n", sha1_to_hex(sha1), refname_nons); capabilities = NULL; - if (!(o->flags & OUR_REF)) { - o->flags |= OUR_REF; - nr_our_refs++; - } if (!peel_ref(refname, peeled)) packet_write(1, "%s %s^{}\n", sha1_to_hex(peeled), refname_nons); return 0;
Right, I think this is a nice cleanup.
quoted hunk ↗ jump to hunk
@@ -751,7 +750,7 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo static int mark_our_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data) { - struct object *o = parse_object(sha1); + struct object *o = parse_object(sha1); /* lookup-unknown??? */ if (!o) die("git upload-pack: cannot find object %s:", sha1_to_hex(sha1)); if (!(o->flags & OUR_REF)) {
And yeah, this should use lookup_unknown_object to extend the optimization to mark_our_ref (and avoid removing it for the ref-advertisement case, of course). -Peff