{"thread":{"id":"29300","subject":"[PATCH] parse_object: try internal cache before reading object db","startedAt":"2012-01-05T21:00:01Z","lastAt":"2013-01-29T08:14:10Z","messageCount":17,"participants":["Jeff King","Junio C Hamano","Shawn Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"181990","messageId":"20120105210001.GA30549@sigill.intra.peff.net","threadId":"29300","inReplyTo":null,"subject":"[PATCH] parse_object: try internal cache before reading object db","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-05T21:00:01Z","receivedAt":"2012-01-05T21:00:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When parse_object is called, we do the following:\n\n  1. read the object data into a buffer via read_sha1_file\n\n  2. call parse_object_buffer, which then:\n\n     a. calls the appropriate lookup_{commit,tree,blob,tag}\n\tto either create a new \"struct object\", or to find\n\tan existing one. We know the appropriate type from\n\tthe lookup in step 1.\n\n     b. calls the appropriate parse_{commit,tree,blob,tag}\n        to parse the buffer for the new (or existing) object\n\nIn step 2b, all of the called functions are no-ops for\nobject \"X\" if \"X->object.parsed\" is set. I.e., when we have\nalready parsed an object, we end up going to a lot of work\njust to find out at a low level that there is nothing left\nfor us to do (and we throw away the data from read_sha1_file\nunread).\n\nWe can optimize this by moving the check for \"do we have an\nin-memory object\" from 2a before the expensive call to\nread_sha1_file in step 1.\n\nThis might seem circular, since step 2a uses the type\ninformation determined in step 1 to call the appropriate\nlookup function. However, we can notice that all of the\nlookup_* functions are backed by lookup_object. In other\nwords, all of the objects are kept in a master hash table,\nand we don't actually need the type to do the \"do we have\nit\" part of the lookup, only to do the \"and create it if it\ndoesn't exist\" part.\n\nThis can save time whenever we call parse_object on the same\nsha1 twice in a single program. Some code paths already\nperform this optimization manually, with either:\n\n  if (!obj->parsed)\n\t  obj = parse_object(obj->sha1);\n\nif you already have a \"struct object\", or:\n\n  struct object *obj = lookup_unknown_object(sha1);\n  if (!obj || !obj->parsed)\n\t  obj = parse_object(sha1);\n\nif you don't.  This patch moves the optimization into\nparse_object itself.\n\nMost git operations won't notice any impact. Either they\ndon't parse a lot of duplicate sha1s, or the calling code\ntakes special care not to re-parse objects. I timed two\ncode paths that do benefit (there may be more, but these two\nwere immediately obvious and easy to time).\n\nThe first is fast-export, which calls parse_object on each\nobject it outputs, like this:\n\n  object = parse_object(sha1);\n  if (!object)\n\t  die(...);\n  if (object->flags & SHOWN)\n\t  return;\n\nwhich means that just to realize we have already shown an\nobject, we will read the whole object from disk!\n\nWith this patch, my best-of-five time for \"fast-export --all\" on\ngit.git dropped from 26.3s to 21.3s.\n\nThe second case is upload-pack, which will call parse_object\nfor each advertised ref (because it needs to peel tags to\nshow \"^{}\" entries). This doesn't matter for most\nrepositories, because they don't have a lot of refs pointing\nto the same objects. However, if you have a big alternates\nrepository with a shared object db for a number of child\nrepositories, then the alternates repository will have\nduplicated refs representing each of its children.\n\nFor example, GitHub's alternates repository for git.git has\n~120,000 refs, of which only ~3200 are unique. The time for\nupload-pack to print its list of advertised refs dropped\nfrom 3.4s to 0.76s.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n object.c |    9 +++++++--\n 1 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/object.c b/object.c\nindex d8d09f9..6b06297 100644\n--- a/object.c\n+++ b/object.c\n@@ -191,10 +191,15 @@ struct object *parse_object(const unsigned char *sha1)\n \tenum object_type type;\n \tint eaten;\n \tconst unsigned char *repl = lookup_replace_object(sha1);\n-\tvoid *buffer = read_sha1_file(sha1, &type, &size);\n+\tvoid *buffer;\n+\tstruct object *obj;\n+\n+\tobj = lookup_object(sha1);\n+\tif (obj && obj->parsed)\n+\t\treturn obj;\n \n+\tbuffer = read_sha1_file(sha1, &type, &size);\n \tif (buffer) {\n-\t\tstruct object *obj;\n \t\tif (check_sha1_signature(repl, buffer, size, typename(type)) < 0) {\n \t\t\tfree(buffer);\n \t\t\terror(\"sha1 mismatch %s\\n\", sha1_to_hex(repl));\n-- \n1.7.6.5.6.ge6248\n"},{"id":"181992","messageId":"7vipkpn87d.fsf@alter.siamese.dyndns.org","threadId":"29300","inReplyTo":"20120105210001.GA30549@sigill.intra.peff.net","subject":"Re: [PATCH] parse_object: try internal cache before reading object db","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-05T21:35:50Z","receivedAt":"2012-01-05T21:35:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> This might seem circular, since step 2a uses the type\n> information determined in step 1 to call the appropriate\n> lookup function. However, we can notice that all of the\n> lookup_* functions are backed by lookup_object. In other\n> words, all of the objects are kept in a master hash table,\n> and we don't actually need the type to do the \"do we have\n> it\" part of the lookup,...\n\nThe only case that might matter is where you read one object, you have\nwritten another object of a different type but that happens to hash to the\nsame SHA-1 value. The other existing optimizations do not take that into\naccount, so I do not think there is any new issue here.\n\n> For example, GitHub's alternates repository for git.git has\n> ~120,000 refs, of which only ~3200 are unique. The time for\n> upload-pack to print its list of advertised refs dropped\n> from 3.4s to 0.76s.\n\nNice. I am more impressed by 120k/3.4 than 3.2k/0.76, though ;-)\n\nThanks.\n"},{"id":"181993","messageId":"20120105214941.GA31836@sigill.intra.peff.net","threadId":"29300","inReplyTo":"7vipkpn87d.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] parse_object: try internal cache before reading object db","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-05T21:49:41Z","receivedAt":"2012-01-05T21:49:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 05, 2012 at 01:35:50PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > This might seem circular, since step 2a uses the type\n> > information determined in step 1 to call the appropriate\n> > lookup function. However, we can notice that all of the\n> > lookup_* functions are backed by lookup_object. In other\n> > words, all of the objects are kept in a master hash table,\n> > and we don't actually need the type to do the \"do we have\n> > it\" part of the lookup,...\n> \n> The only case that might matter is where you read one object, you have\n> written another object of a different type but that happens to hash to the\n> same SHA-1 value. The other existing optimizations do not take that into\n> account, so I do not think there is any new issue here.\n\nYeah, I tried to think of issues like that. Even if you protected\nagainst that, you'd still have the issue of reading one object, then\nwriting another of the _same_ type but with different content. We\nwouldn't notice with the current code path (we'd just recreationally\nread the data from disk and then throw it away).\n\nThe worst potential problem I could come up with is if you somehow had\nan object whose \"parsed\" flag was set, but somehow didn't have its other\nfields set (like type). But I think you'd have to be abusing the lookup\nfunctions pretty hard to get into such a state (how would you be parsing\nif you didn't know the type?). The parsed flag only gets set by the\ntype-specific lookup functions.\n\nSo I think it is safe short of somebody doing some horrible manual\nmunging of a \"struct object\".\n\n> > For example, GitHub's alternates repository for git.git has\n> > ~120,000 refs, of which only ~3200 are unique. The time for\n> > upload-pack to print its list of advertised refs dropped\n> > from 3.4s to 0.76s.\n> \n> Nice. I am more impressed by 120k/3.4 than 3.2k/0.76, though ;-)\n\nYou can thank optimized zlib for that. We spent 60% of our time there.\n:)\n\n-Peff\n"},{"id":"181994","messageId":"7vehvdn7at.fsf@alter.siamese.dyndns.org","threadId":"29300","inReplyTo":"20120105214941.GA31836@sigill.intra.peff.net","subject":"Re: [PATCH] parse_object: try internal cache before reading object db","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-05T21:55:22Z","receivedAt":"2012-01-05T21:55:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The worst potential problem I could come up with is if you somehow had\n> an object whose \"parsed\" flag was set, but somehow didn't have its other\n> fields set (like type).\n> ...\n> So I think it is safe short of somebody doing some horrible manual\n> munging of a \"struct object\".\n\nYeah, I was worried about codepaths like commit-pretty-printing might be\nmucking with the contents of commit->buffer, perhaps reencoding the text\nand then calling parse_object() to get the unmodified original back, or\nsomething silly like that. But the lookup_object() call at the beginning\nof the parse_object() already prevents us from doing such a thing, so we\nshould be OK, I would think.\n"},{"id":"181996","messageId":"20120105221849.GA5459@sigill.intra.peff.net","threadId":"29300","inReplyTo":"7vehvdn7at.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] parse_object: try internal cache before reading object db","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-05T22:18:50Z","receivedAt":"2012-01-05T22:18:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 05, 2012 at 01:55:22PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > The worst potential problem I could come up with is if you somehow had\n> > an object whose \"parsed\" flag was set, but somehow didn't have its other\n> > fields set (like type).\n> > ...\n> > So I think it is safe short of somebody doing some horrible manual\n> > munging of a \"struct object\".\n> \n> Yeah, I was worried about codepaths like commit-pretty-printing might be\n> mucking with the contents of commit->buffer, perhaps reencoding the text\n> and then calling parse_object() to get the unmodified original back, or\n> something silly like that. But the lookup_object() call at the beginning\n> of the parse_object() already prevents us from doing such a thing, so we\n> should be OK, I would think.\n\nEr, without my patch there is no such lookup_object, is there?\n\nWhat saves you is that the parse_*_buffer functions all do nothing when\nthe object.parsed flag is set, and the code I added makes sure that\nobject.parsed is set in the object that lookup_object returns.\n\nSo yeah, anytime you tweak the contents of commit->buffer but don't\nunset the \"parsed\" flag, you are asking for trouble.\n\nHere's another possible code path where the behavior is changed:\n\n  1. You set the global save_commit_buffer to 0.\n\n  2. You call parse_commit(commit) on an unparsed commit object, which\n     does not save the commit buffer, but does set\n     commit->object.parsed.\n\n  3. You call parse_object(commit->object.sha1).\n\n     a. Without my patch, we read the file contents again, do _not_\n        re-parse them (because we look up the existing object and notice\n        that its \"parsed\" flag is set), but we _do_ assign the buffer to\n        commit->buffer.\n\n     b. With my patch, we see that there is an existing object that is\n        already parsed, and return early. commit->buffer remains NULL.\n\nI would argue that this doesn't matter, since \"parse_commit\" uses the\nexact same optimization (it returns early without setting commit->buffer\nif the parsed flag is set). So any program turning off\nsave_commit_buffer has to be ready to deal with a NULL commit->buffer in\nthe first place. The only exception would be a program that then tries\nto fill in the commit->buffer field by manually running parse_object on\nan already-parsed, buffer-less commit object. I don't think we do that.\n\nYou can verify that commit->buffer is the only place where these issues\ncan happen by following the logic in parse_object_buffer.\n\nSorry to belabor the discussion, but this is such a core piece of code,\nI want to make sure the optimization isn't hurting anybody (I don't\nthink it is, and certainly the tests are all happy, but I think talking\nthrough the cases is a good thing).\n\n-Peff\n"},{"id":"182034","messageId":"20120106191654.GA11022@sigill.intra.peff.net","threadId":"29300","inReplyTo":"7vipkpn87d.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] parse_object: try internal cache before reading object db","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-06T19:16:54Z","receivedAt":"2012-01-06T19:16:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 05, 2012 at 01:35:50PM -0800, Junio C Hamano wrote:\n\n> > For example, GitHub's alternates repository for git.git has\n> > ~120,000 refs, of which only ~3200 are unique. The time for\n> > upload-pack to print its list of advertised refs dropped\n> > from 3.4s to 0.76s.\n> \n> Nice. I am more impressed by 120k/3.4 than 3.2k/0.76, though ;-)\n\nActually, we can do much better than that. Here are a few patches that\navoid parsing objects when possible. They drop the 3.4s to 2.0s. If you\ncombine them with the parse_object optimization, my 120K case drops to\naround 0.68s.\n\nI don't know if it is really that worth it on top of the parse_object\noptimization. It's almost negligible for the normal case (though I get a\ntiny speedup on my ~900-ref git.git repo), and a minor speedup on the\ncrazy alternates case. OTOH, if you had some totally insane ref\nstructure, like 120K _unique_ refs (which would probably imply that\nyou're making one ref per commit or something silly like that. But hey,\npeople have suggested it in the past), then it could be a big\nimprovement.\n\n  [1/2]: upload-pack: avoid parsing objects during ref advertisement\n  [2/2]: upload-pack: avoid parsing tag destinations\n\n-Peff\n"},{"id":"182035","messageId":"20120106191740.GA12903@sigill.intra.peff.net","threadId":"29300","inReplyTo":"7vipkpn87d.fsf@alter.siamese.dyndns.org","subject":"[PATCH 1/2] upload-pack: avoid parsing objects during ref advertisement","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-06T19:17:40Z","receivedAt":"2012-01-06T19:17:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we advertise a ref, the first thing we do is parse the\npointed-to object. This gives us two things:\n\n  1. a \"struct object\" we can use to store flags\n\n  2. the type of the object, so we know whether we need to\n     dereference it as a tag\n\nInstead, we can just use lookup_unknown_object to get an\nobject struct, and then fill in just the type field using\nsha1_object_info (which, in the case of packed files, can\nfind the information without actually inflating the object\ndata).\n\nThis can save time if you have a large number of refs, and\nthe client isn't actually going to request those refs (e.g.,\nbecause most of them are already up-to-date).\n\nThe downside is that we are no longer verifying objects that\nwe advertise by fully parsing them (however, we do still\nknow we actually have them, because sha1_object_info must\nfind them to get the type). While we might fail to detect a\ncorrupt object here, if the client actually fetches the\nobject, we will parse (and verify) it then.\n\nOn a repository with 120K refs, the advertisement portion of\nupload-pack goes from ~3.4s to 3.2s (the failure to speed up\nmore is largely due to the fact that most of these refs are\ntags, which need dereferenced to find the tag destination\nanyway).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n upload-pack.c |   10 +++++++---\n 1 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 6f36f62..65cb0ff 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -720,11 +720,14 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo\n \tstatic const char *capabilities = \"multi_ack thin-pack side-band\"\n \t\t\" side-band-64k ofs-delta shallow no-progress\"\n \t\t\" include-tag multi_ack_detailed\";\n-\tstruct object *o = parse_object(sha1);\n+\tstruct object *o = lookup_unknown_object(sha1);\n \tconst char *refname_nons = strip_namespace(refname);\n \n-\tif (!o)\n-\t\tdie(\"git upload-pack: cannot find object %s:\", sha1_to_hex(sha1));\n+\tif (o->type == OBJ_NONE) {\n+\t\to->type = sha1_object_info(sha1, NULL);\n+\t\tif (o->type < 0)\n+\t\t    die(\"git upload-pack: cannot find object %s:\", sha1_to_hex(sha1));\n+\t}\n \n \tif (capabilities)\n \t\tpacket_write(1, \"%s %s%c%s%s\\n\", sha1_to_hex(sha1), refname_nons,\n@@ -738,6 +741,7 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo\n \t\tnr_our_refs++;\n \t}\n \tif (o->type == OBJ_TAG) {\n+\t\to = parse_object(o->sha1);\n \t\to = deref_tag(o, refname, 0);\n \t\tif (o)\n \t\t\tpacket_write(1, \"%s %s^{}\\n\", sha1_to_hex(o->sha1), refname_nons);\n-- \n1.7.6.5.14.g7b06f\n"},{"id":"182036","messageId":"20120106191801.GB12903@sigill.intra.peff.net","threadId":"29300","inReplyTo":"7vipkpn87d.fsf@alter.siamese.dyndns.org","subject":"[PATCH 2/2] upload-pack: avoid parsing tag destinations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-06T19:18:01Z","receivedAt":"2012-01-06T19:18:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When upload-pack advertises refs, it dereferences any tags\nit sees, and shows the resulting sha1 to the client. It does\nthis by calling deref_tag. That function must load and parse\neach tag object to find the sha1 of the tagged object.\nHowever, it also ends up parsing the tagged object itself,\nwhich is not strictly necessary for upload-pack's use.\n\nEach tag produces two object loads (assuming it is not a\nrecursive tag), when it could get away with only a single\none. Dropping the second load halves the effort we spend.\n\nThe downside is that we are no longer verifying the\nresulting object by loading it. In particular:\n\n  1. We never cross-check the \"type\" field given in the tag\n     object with the type of the pointed-to object.  If the\n     tag says it points to a tag but doesn't, then we will\n     keep peeling and realize the error.  If the tag says it\n     points to a non-tag but actually points to a tag, we\n     will stop peeling and just advertise the pointed-to\n     tag.\n\n  2. If we are missing the pointed-to object, we will not\n     realize (because we never even look it up in the object\n     db).\n\nHowever, both of these are errors in the object database,\nand both will be detected if a client actually requests the\nbroken objects in question. So we are simply pushing the\nverification away from the advertising stage, and down to\nthe actual fetching stage.\n\nOn my test repo with 120K refs, this drops the time to\nadvertise the refs from ~3.2s to ~2.0s.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n tag.c         |   12 ++++++++++++\n tag.h         |    1 +\n upload-pack.c |    3 +--\n 3 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/tag.c b/tag.c\nindex 3aa186d..78d272b 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -24,6 +24,18 @@ struct object *deref_tag(struct object *o, const char *warn, int warnlen)\n \treturn o;\n }\n \n+struct object *deref_tag_noverify(struct object *o)\n+{\n+\twhile (o && o->type == OBJ_TAG) {\n+\t\to = parse_object(o->sha1);\n+\t\tif (o && o->type == OBJ_TAG && ((struct tag *)o)->tagged)\n+\t\t\to = ((struct tag *)o)->tagged;\n+\t\telse\n+\t\t\to = NULL;\n+\t}\n+\treturn o;\n+}\n+\n struct tag *lookup_tag(const unsigned char *sha1)\n {\n \tstruct object *obj = lookup_object(sha1);\ndiff --git a/tag.h b/tag.h\nindex 5ee88e6..bc8a1e4 100644\n--- a/tag.h\n+++ b/tag.h\n@@ -16,6 +16,7 @@ extern struct tag *lookup_tag(const unsigned char *sha1);\n extern int parse_tag_buffer(struct tag *item, const void *data, unsigned long size);\n extern int parse_tag(struct tag *item);\n extern struct object *deref_tag(struct object *, const char *, int);\n+extern struct object *deref_tag_noverify(struct object *);\n extern size_t parse_signature(const char *buf, unsigned long size);\n \n #endif /* TAG_H */\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 65cb0ff..c01e161 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -741,8 +741,7 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo\n \t\tnr_our_refs++;\n \t}\n \tif (o->type == OBJ_TAG) {\n-\t\to = parse_object(o->sha1);\n-\t\to = deref_tag(o, refname, 0);\n+\t\to = deref_tag_noverify(o);\n \t\tif (o)\n \t\t\tpacket_write(1, \"%s %s^{}\\n\", sha1_to_hex(o->sha1), refname_nons);\n \t}\n-- \n1.7.6.5.14.g7b06f\n"},{"id":"182041","messageId":"7v8vlkjzcj.fsf@alter.siamese.dyndns.org","threadId":"29300","inReplyTo":"20120106191654.GA11022@sigill.intra.peff.net","subject":"Re: [PATCH] parse_object: try internal cache before reading object db","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-06T21:27:40Z","receivedAt":"2012-01-06T21:27:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Actually, we can do much better than that. Here are a few patches that\n> avoid parsing objects when possible. They drop the 3.4s to 2.0s. If you\n> combine them with the parse_object optimization, my 120K case drops to\n> around 0.68s.\n>\n> I don't know if it is really that worth it on top of the parse_object\n> optimization. It's almost negligible for the normal case...\n> ... OTOH, if you had some totally insane ref\n> structure, like 120K _unique_ refs (which would probably imply that\n> you're making one ref per commit or something silly like that. But hey,\n> people have suggested it in the past), then it could be a big\n> improvement.\n\nEven though it is a bit scary kind of loosening of sanity checks that I\nhesitate to take at this late in the cycle, I think it makes sense. Let's\nqueue them on 'pu' and aim for the next cycle.\n"},{"id":"182045","messageId":"20120106223324.GB13106@sigill.intra.peff.net","threadId":"29300","inReplyTo":"7v8vlkjzcj.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] parse_object: try internal cache before reading object db","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-06T22:33:24Z","receivedAt":"2012-01-06T22:33:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 06, 2012 at 01:27:40PM -0800, Junio C Hamano wrote:\n\n> > I don't know if it is really that worth it on top of the parse_object\n> > optimization. It's almost negligible for the normal case...\n> > ... OTOH, if you had some totally insane ref\n> > structure, like 120K _unique_ refs (which would probably imply that\n> > you're making one ref per commit or something silly like that. But hey,\n> > people have suggested it in the past), then it could be a big\n> > improvement.\n> \n> Even though it is a bit scary kind of loosening of sanity checks that I\n> hesitate to take at this late in the cycle, I think it makes sense. Let's\n> queue them on 'pu' and aim for the next cycle.\n\nDid you want to leave the parse_object optimization until next cycle,\ntoo? It's not loosening checks, but it's such a core piece of code that\nit makes me nervous somebody somewhere is abusing \"struct object\" in a\nway that will break it.\n\n-Peff\n"},{"id":"182046","messageId":"7vmxa0ih6s.fsf@alter.siamese.dyndns.org","threadId":"29300","inReplyTo":"20120106223324.GB13106@sigill.intra.peff.net","subject":"Re: [PATCH] parse_object: try internal cache before reading object db","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-06T22:45:15Z","receivedAt":"2012-01-06T22:45:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> Even though it is a bit scary kind of loosening of sanity checks that I\n>> hesitate to take at this late in the cycle, I think it makes sense. Let's\n>> queue them on 'pu' and aim for the next cycle.\n>\n> Did you want to leave the parse_object optimization until next cycle,\n> too? It's not loosening checks, but it's such a core piece of code that\n> it makes me nervous somebody somewhere is abusing \"struct object\" in a\n> way that will break it.\n\nI was just updating the \"What's cooking\" report and my current thinking is\nthat we should keep all three in \"next\" to give it a bit of exposure for\nnow, and merge them to \"master\" early in the 1.7.10 cycle.\n"},{"id":"182047","messageId":"20120106224619.GA13654@sigill.intra.peff.net","threadId":"29300","inReplyTo":"7vmxa0ih6s.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] parse_object: try internal cache before reading object db","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-06T22:46:19Z","receivedAt":"2012-01-06T22:46:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 06, 2012 at 02:45:15PM -0800, Junio C Hamano wrote:\n\n> > Did you want to leave the parse_object optimization until next cycle,\n> > too? It's not loosening checks, but it's such a core piece of code that\n> > it makes me nervous somebody somewhere is abusing \"struct object\" in a\n> > way that will break it.\n> \n> I was just updating the \"What's cooking\" report and my current thinking is\n> that we should keep all three in \"next\" to give it a bit of exposure for\n> now, and merge them to \"master\" early in the 1.7.10 cycle.\n\nThat sounds perfect. Thanks.\n\n-Peff\n"},{"id":"207236","messageId":"7vehhiozkb.fsf@alter.siamese.dyndns.org","threadId":"29300","inReplyTo":"20120106191740.GA12903@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] upload-pack: avoid parsing objects during ref advertisement","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-18T23:12:52Z","receivedAt":"2013-01-18T23:12:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> When we advertise a ref, the first thing we do is parse the\n> pointed-to object. This gives us two things:\n>\n>   1. a \"struct object\" we can use to store flags\n>\n>   2. the type of the object, so we know whether we need to\n>      dereference it as a tag\n>\n> Instead, we can just use lookup_unknown_object to get an\n> object struct, and then fill in just the type field using\n> sha1_object_info (which, in the case of packed files, can\n> find the information without actually inflating the object\n> data).\n>\n> This can save time if you have a large number of refs, and\n> the client isn't actually going to request those refs (e.g.,\n> because most of them are already up-to-date).\n>\n> The downside is that we are no longer verifying objects that\n> we advertise by fully parsing them (however, we do still\n> know we actually have them, because sha1_object_info must\n> find them to get the type). While we might fail to detect a\n> corrupt object here, if the client actually fetches the\n> object, we will parse (and verify) it then.\n>...\n\nThis is an old news, but do you recall why this patch did not update\nthe code in mark_our_ref() that gets \"struct object *o\" from parse_object()\nthe same way and mark them with OUR_REF flag?\n\nI was wondering about code consolidation like this.\n\n upload-pack.c | 11 +++++------\n 1 file changed, 5 insertions(+), 6 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 95d8313..609cd6c 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -722,15 +722,18 @@ static void receive_needs(void)\n \tfree(shallows.objects);\n }\n \n+static int mark_our_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data);\n+\n static int send_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)\n {\n \tstatic const char *capabilities = \"multi_ack thin-pack side-band\"\n \t\t\" side-band-64k ofs-delta shallow no-progress\"\n \t\t\" include-tag multi_ack_detailed\";\n-\tstruct object *o = lookup_unknown_object(sha1);\n \tconst char *refname_nons = strip_namespace(refname);\n \tunsigned char peeled[20];\n \n+\tmark_our_ref(refname, sha1, flag, cb_data);\n+\n \tif (capabilities)\n \t\tpacket_write(1, \"%s %s%c%s%s agent=%s\\n\",\n \t\t\t     sha1_to_hex(sha1), refname_nons,\n@@ -740,10 +743,6 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo\n \telse\n \t\tpacket_write(1, \"%s %s\\n\", sha1_to_hex(sha1), refname_nons);\n \tcapabilities = NULL;\n-\tif (!(o->flags & OUR_REF)) {\n-\t\to->flags |= OUR_REF;\n-\t\tnr_our_refs++;\n-\t}\n \tif (!peel_ref(refname, peeled))\n \t\tpacket_write(1, \"%s %s^{}\\n\", sha1_to_hex(peeled), refname_nons);\n \treturn 0;\n@@ -751,7 +750,7 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo\n \n static int mark_our_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)\n {\n-\tstruct object *o = parse_object(sha1);\n+\tstruct object *o = parse_object(sha1); /* lookup-unknown??? */\n \tif (!o)\n \t\tdie(\"git upload-pack: cannot find object %s:\", sha1_to_hex(sha1));\n \tif (!(o->flags & OUR_REF)) {\n"},{"id":"207673","messageId":"20130124075008.GA3249@sigill.intra.peff.net","threadId":"29300","inReplyTo":"7vehhiozkb.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] upload-pack: avoid parsing objects during ref advertisement","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-24T07:50:08Z","receivedAt":"2013-01-24T07:50:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 18, 2013 at 03:12:52PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > When we advertise a ref, the first thing we do is parse the\n> > pointed-to object. This gives us two things:\n> >\n> >   1. a \"struct object\" we can use to store flags\n> >\n> >   2. the type of the object, so we know whether we need to\n> >      dereference it as a tag\n> >\n> > Instead, we can just use lookup_unknown_object to get an\n> > object struct, and then fill in just the type field using\n> > sha1_object_info (which, in the case of packed files, can\n> > find the information without actually inflating the object\n> > data).\n> >\n> > This can save time if you have a large number of refs, and\n> > the client isn't actually going to request those refs (e.g.,\n> > because most of them are already up-to-date).\n> \n> This is an old news, but do you recall why this patch did not update\n> the code in mark_our_ref() that gets \"struct object *o\" from parse_object()\n> the same way and mark them with OUR_REF flag?\n> \n> I was wondering about code consolidation like this.\n\nIt was just because I did my measuring on raw upload-pack, so I didn't\nnotice that mark_our_ref was doing the same potentially slow thing. We\nonly call mark_our_ref during the second half of the stateless-rpc\nconversation, and I did not measure that (and it would be a pain to do\nso in isolation).\n\nBut it should be able to get the exact same speedups that we get from\nsend_ref. It probably matters less in the long run, because the\nadvertising phase is going to be called more frequently (e.g., for every\nno-op fetch), and once we are calling mark_our_ref, we are presumably\nabout to do do actual packing work. However, there's no reason not to\nget what speed we can there, too.\n\n> diff --git a/upload-pack.c b/upload-pack.c\n> index 95d8313..609cd6c 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -722,15 +722,18 @@ static void receive_needs(void)\n>  \tfree(shallows.objects);\n>  }\n>  \n> +static int mark_our_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data);\n> +\n>  static int send_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)\n>  {\n>  \tstatic const char *capabilities = \"multi_ack thin-pack side-band\"\n>  \t\t\" side-band-64k ofs-delta shallow no-progress\"\n>  \t\t\" include-tag multi_ack_detailed\";\n> -\tstruct object *o = lookup_unknown_object(sha1);\n>  \tconst char *refname_nons = strip_namespace(refname);\n>  \tunsigned char peeled[20];\n>  \n> +\tmark_our_ref(refname, sha1, flag, cb_data);\n> +\n>  \tif (capabilities)\n>  \t\tpacket_write(1, \"%s %s%c%s%s agent=%s\\n\",\n>  \t\t\t     sha1_to_hex(sha1), refname_nons,\n> @@ -740,10 +743,6 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo\n>  \telse\n>  \t\tpacket_write(1, \"%s %s\\n\", sha1_to_hex(sha1), refname_nons);\n>  \tcapabilities = NULL;\n> -\tif (!(o->flags & OUR_REF)) {\n> -\t\to->flags |= OUR_REF;\n> -\t\tnr_our_refs++;\n> -\t}\n>  \tif (!peel_ref(refname, peeled))\n>  \t\tpacket_write(1, \"%s %s^{}\\n\", sha1_to_hex(peeled), refname_nons);\n>  \treturn 0;\n\nRight, I think this is a nice cleanup.\n\n> @@ -751,7 +750,7 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo\n>  \n>  static int mark_our_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)\n>  {\n> -\tstruct object *o = parse_object(sha1);\n> +\tstruct object *o = parse_object(sha1); /* lookup-unknown??? */\n>  \tif (!o)\n>  \t\tdie(\"git upload-pack: cannot find object %s:\", sha1_to_hex(sha1));\n>  \tif (!(o->flags & OUR_REF)) {\n\nAnd yeah, this should use lookup_unknown_object to extend the\noptimization to mark_our_ref (and avoid removing it for the\nref-advertisement case, of course).\n\n-Peff\n"},{"id":"207696","messageId":"7vr4lacx2k.fsf@alter.siamese.dyndns.org","threadId":"29300","inReplyTo":"20130124075008.GA3249@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] upload-pack: avoid parsing objects during ref advertisement","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-24T17:25:39Z","receivedAt":"2013-01-24T17:25:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> And yeah, this should use lookup_unknown_object to extend the\n> optimization to mark_our_ref (and avoid removing it for the\n> ref-advertisement case, of course).\n\nThanks for sanity checking.  Here is what is queued at the bottom of\nthe hide-refs topic.\n\n-- >8 --\nDate: Fri, 18 Jan 2013 15:48:49 -0800\nSubject: [PATCH] upload-pack: share more code\n\nWe mark the objects pointed at our refs with \"OUR_REF\" flag in two\nfunctions (mark_our_ref() and send_ref()), but we can just use the\nformer as a helper for the latter.\n\nUpdate the way mark_our_ref() prepares in-core object to use\nlookup_unknown_object() to delay reading the actual object data,\njust like we did in 435c833 (upload-pack: use peel_ref for ref\nadvertisements, 2012-10-04).\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n upload-pack.c | 31 ++++++++++++++-----------------\n 1 file changed, 14 insertions(+), 17 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 95d8313..3dd220d 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -722,15 +722,28 @@ static void receive_needs(void)\n \tfree(shallows.objects);\n }\n \n+static int mark_our_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)\n+{\n+\tstruct object *o = lookup_unknown_object(sha1);\n+\tif (!o)\n+\t\tdie(\"git upload-pack: cannot find object %s:\", sha1_to_hex(sha1));\n+\tif (!(o->flags & OUR_REF)) {\n+\t\to->flags |= OUR_REF;\n+\t\tnr_our_refs++;\n+\t}\n+\treturn 0;\n+}\n+\n static int send_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)\n {\n \tstatic const char *capabilities = \"multi_ack thin-pack side-band\"\n \t\t\" side-band-64k ofs-delta shallow no-progress\"\n \t\t\" include-tag multi_ack_detailed\";\n-\tstruct object *o = lookup_unknown_object(sha1);\n \tconst char *refname_nons = strip_namespace(refname);\n \tunsigned char peeled[20];\n \n+\tmark_our_ref(refname, sha1, flag, cb_data);\n+\n \tif (capabilities)\n \t\tpacket_write(1, \"%s %s%c%s%s agent=%s\\n\",\n \t\t\t     sha1_to_hex(sha1), refname_nons,\n@@ -740,27 +753,11 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo\n \telse\n \t\tpacket_write(1, \"%s %s\\n\", sha1_to_hex(sha1), refname_nons);\n \tcapabilities = NULL;\n-\tif (!(o->flags & OUR_REF)) {\n-\t\to->flags |= OUR_REF;\n-\t\tnr_our_refs++;\n-\t}\n \tif (!peel_ref(refname, peeled))\n \t\tpacket_write(1, \"%s %s^{}\\n\", sha1_to_hex(peeled), refname_nons);\n \treturn 0;\n }\n \n-static int mark_our_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)\n-{\n-\tstruct object *o = parse_object(sha1);\n-\tif (!o)\n-\t\tdie(\"git upload-pack: cannot find object %s:\", sha1_to_hex(sha1));\n-\tif (!(o->flags & OUR_REF)) {\n-\t\to->flags |= OUR_REF;\n-\t\tnr_our_refs++;\n-\t}\n-\treturn 0;\n-}\n-\n static void upload_pack(void)\n {\n \tif (advertise_refs || !stateless_rpc) {\n-- \n1.8.1.1.525.gdace530\n"},{"id":"208160","messageId":"CAJo=hJubUF2mmkzXU77kQ5uj1cbBZrnLcw2u736wMqxhs39jdQ@mail.gmail.com","threadId":"29300","inReplyTo":"20120106191740.GA12903@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] upload-pack: avoid parsing objects during ref advertisement","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2013-01-29T08:10:59Z","receivedAt":"2013-01-29T08:10:59Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Fri, Jan 6, 2012 at 11:17 AM, Jeff King <peff@peff.net> wrote:\n> When we advertise a ref, the first thing we do is parse the\n> pointed-to object. This gives us two things:\n...\n> The downside is that we are no longer verifying objects that\n> we advertise by fully parsing them (however, we do still\n> know we actually have them, because sha1_object_info must\n> find them to get the type). While we might fail to detect a\n> corrupt object here, if the client actually fetches the\n> object, we will parse (and verify) it then.\n\nAs you explain, its not necessary to verify during the advertisement\nphase. Its fine to delay verification to when a client actually\n\"wants\" the object.\n\n> On a repository with 120K refs, the advertisement portion of\n> upload-pack goes from ~3.4s to 3.2s (the failure to speed up\n> more is largely due to the fact that most of these refs are\n> tags, which need dereferenced to find the tag destination\n> anyway).\n\nWhy aren't we using the peeled information from the packed-refs file?\nJGit does this and it saves a lot of time on advertisements from a\nwell packed repository.\n"},{"id":"208161","messageId":"20130129081409.GA5350@sigill.intra.peff.net","threadId":"29300","inReplyTo":"CAJo=hJubUF2mmkzXU77kQ5uj1cbBZrnLcw2u736wMqxhs39jdQ@mail.gmail.com","subject":"Re: [PATCH 1/2] upload-pack: avoid parsing objects during ref advertisement","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-29T08:14:10Z","receivedAt":"2013-01-29T08:14:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 29, 2013 at 12:10:59AM -0800, Shawn O. Pearce wrote:\n\n> > On a repository with 120K refs, the advertisement portion of\n> > upload-pack goes from ~3.4s to 3.2s (the failure to speed up\n> > more is largely due to the fact that most of these refs are\n> > tags, which need dereferenced to find the tag destination\n> > anyway).\n> \n> Why aren't we using the peeled information from the packed-refs file?\n> JGit does this and it saves a lot of time on advertisements from a\n> well packed repository.\n\nThe patch you are replying to is a year old. Since then, I did:\n\n  435c833 (upload-pack: use peel_ref for ref advertisements, 2012-10-04)\n\n-Peff\n"}]}