Re: [PATCH 1/2] upload-pack: avoid parsing objects during ref advertisement
On Fri, Jan 6, 2012 at 11:17 AM, Jeff King p...@peff.net wrote: When we advertise a ref, the first thing we do is parse the pointed-to object. This gives us two things: ... The downside is that we are no longer verifying objects that we advertise by fully parsing them (however, we do still know we actually have them, because sha1_object_info must find them to get the type). While we might fail to detect a corrupt object here, if the client actually fetches the object, we will parse (and verify) it then. As you explain, its not necessary to verify during the advertisement phase. Its fine to delay verification to when a client actually wants the object. On a repository with 120K refs, the advertisement portion of upload-pack goes from ~3.4s to 3.2s (the failure to speed up more is largely due to the fact that most of these refs are tags, which need dereferenced to find the tag destination anyway). Why aren't we using the peeled information from the packed-refs file? JGit does this and it saves a lot of time on advertisements from a well packed repository. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] upload-pack: avoid parsing objects during ref advertisement
Jeff King p...@peff.net writes:
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).
Thanks for sanity checking. Here is what is queued at the bottom of
the hide-refs topic.
-- 8 --
Date: Fri, 18 Jan 2013 15:48:49 -0800
Subject: [PATCH] upload-pack: share more code
We mark the objects pointed at our refs with OUR_REF flag in two
functions (mark_our_ref() and send_ref()), but we can just use the
former as a helper for the latter.
Update the way mark_our_ref() prepares in-core object to use
lookup_unknown_object() to delay reading the actual object data,
just like we did in 435c833 (upload-pack: use peel_ref for ref
advertisements, 2012-10-04).
Signed-off-by: Junio C Hamano gits...@pobox.com
---
upload-pack.c | 31 ++-
1 file changed, 14 insertions(+), 17 deletions(-)
diff --git a/upload-pack.c b/upload-pack.c
index 95d8313..3dd220d 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -722,15 +722,28 @@ 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)
+{
+ struct object *o = lookup_unknown_object(sha1);
+ if (!o)
+ die(git upload-pack: cannot find object %s:,
sha1_to_hex(sha1));
+ if (!(o-flags OUR_REF)) {
+ o-flags |= OUR_REF;
+ nr_our_refs++;
+ }
+ return 0;
+}
+
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,27 +753,11 @@ 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;
}
-static int mark_our_ref(const char *refname, const unsigned char *sha1, int
flag, void *cb_data)
-{
- struct object *o = parse_object(sha1);
- if (!o)
- die(git upload-pack: cannot find object %s:,
sha1_to_hex(sha1));
- if (!(o-flags OUR_REF)) {
- o-flags |= OUR_REF;
- nr_our_refs++;
- }
- return 0;
-}
-
static void upload_pack(void)
{
if (advertise_refs || !stateless_rpc) {
--
1.8.1.1.525.gdace530
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] upload-pack: avoid parsing objects during ref advertisement
Jeff King p...@peff.net writes:
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).
The downside is that we are no longer verifying objects that
we advertise by fully parsing them (however, we do still
know we actually have them, because sha1_object_info must
find them to get the type). While we might fail to detect a
corrupt object here, if the client actually fetches the
object, we will parse (and verify) it then.
...
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.
upload-pack.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
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;
@@ -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)) {
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
