{"thread":{"id":"36340","subject":"Re: [PATCH] pack-objects: do not reuse packfiles without --delta-base-offset","startedAt":"2014-04-02T06:39:17Z","lastAt":"2014-04-07T17:15:43Z","messageCount":6,"participants":["Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"238341","messageId":"20140402063916.GA1437@sigill.intra.peff.net","threadId":"36340","inReplyTo":null,"subject":"[PATCH] pack-objects: do not reuse packfiles without --delta-base-offset","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-02T06:39:17Z","receivedAt":"2014-04-02T06:39:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we are sending a packfile to a remote, we currently try\nto reuse a whole chunk of packfile without bothering to look\nat the individual objects. This can make things like initial\nclones much lighter on the server, as we can just dump the\npackfile bytes.\n\nHowever, it's possible that the other side cannot read our\npackfile verbatim. For example, we may have objects stored\nas OFS_DELTA, but the client is an antique version of git\nthat only understands REF_DELTA. We negotiate this\ncapability over the fetch protocol. A normal pack-objects\nrun will convert OFS_DELTA into REF_DELTA on the fly, but\nthe \"reuse pack\" code path never even looks at the objects.\n\nThis patch disables packfile reuse if the other side is\nmissing any capabilities that we might have used in the\non-disk pack. Right now the only one is OFS_DELTA, but we\nmay need to expand in the future (e.g., if packv4 introduces\nnew object types).\n\nWe could be more thorough and only disable reuse in this\ncase when we actually have an OFS_DELTA to send, but:\n\n 1. We almost always will have one, since we prefer\n OFS_DELTA to REF_DELTA when possible. So this case\n would almost never come up.\n\n 2. Looking through the objects defeats the purpose of the\n optimization, which is to do as little work as possible\n to get the bytes to the remote.\n\nSigned-off-by: Jeff King \n---\nI happened to be fooling around with git v1.4.0 today, and noticed a\nproblem fetching from GitHub. Pre-OFS_DELTA git versions are ancient by\ntoday's standard, but it's quite easy to remain compatible here, so I\ndon't see why not. And in theory, alternate implementations might not\nunderstand OFS_DELTA, though in practice I would consider such an\nimplementation to be pretty crappy.\n\n builtin/pack-objects.c | 13 ++++++++++++-\n 1 file changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 7950c43..1503632 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2439,12 +2439,23 @@ static void loosen_unused_packed_objects(struct rev_info *revs)\n \t}\n }\n \n+/*\n+ * This tracks any options which a reader of the pack might\n+ * not understand, and which would therefore prevent blind reuse\n+ * of what we have on disk.\n+ */\n+static int pack_options_allow_reuse(void)\n+{\n+\treturn allow_ofs_delta;\n+}\n+\n static int get_object_list_from_bitmap(struct rev_info *revs)\n {\n \tif (prepare_bitmap_walk(revs) < 0)\n \t\treturn -1;\n \n-\tif (!reuse_partial_packfile_from_bitmap(\n+\tif (pack_options_allow_reuse() &&\n+\t !reuse_partial_packfile_from_bitmap(\n \t\t\t&reuse_packfile,\n \t\t\t&reuse_packfile_objects,\n \t\t\t&reuse_packfile_offset)) {\n-- \n1.9.1.656.ge8a0637\n"},{"id":"238305","messageId":"xmqqfvlvvdfi.fsf@gitster.dls.corp.google.com","threadId":"36340","inReplyTo":"20140402063916.GA1437@sigill.intra.peff.net","subject":"Re: [PATCH] pack-objects: do not reuse packfiles without --delta-base-offset","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-02T17:39:13Z","receivedAt":"2014-04-02T17:39:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> When we are sending a packfile to a remote, we currently try\n> to reuse a whole chunk of packfile without bothering to look\n> at the individual objects. This can make things like initial\n> clones much lighter on the server, as we can just dump the\n> packfile bytes.\n>\n> However, it's possible that the other side cannot read our\n> packfile verbatim. For example, we may have objects stored\n> as OFS_DELTA, but the client is an antique version of git\n> that only understands REF_DELTA. We negotiate this\n> capability over the fetch protocol. A normal pack-objects\n> run will convert OFS_DELTA into REF_DELTA on the fly, but\n> the \"reuse pack\" code path never even looks at the objects.\n\nThe above makes it sound like \"reuse pack\" codepath is broken. Is it\ntoo much hassle to peek at the initial bytes of each object to see\nhow they are encoded? Would it be possible to convert OFS_DELTA to\nREF_DELTA on the fly on that codepath as well, instead of disabling\nthe reuse altogether?\n\n> This patch disables packfile reuse if the other side is\n> missing any capabilities that we might have used in the\n> on-disk pack. Right now the only one is OFS_DELTA, but we\n> may need to expand in the future (e.g., if packv4 introduces\n> new object types).\n>\n> We could be more thorough and only disable reuse in this\n> case when we actually have an OFS_DELTA to send, but:\n>\n> 1. We almost always will have one, since we prefer\n> OFS_DELTA to REF_DELTA when possible. So this case\n> would almost never come up.\n>\n> 2. Looking through the objects defeats the purpose of the\n> optimization, which is to do as little work as possible\n> to get the bytes to the remote.\n>\n> Signed-off-by: Jeff King \n> ---\n> I happened to be fooling around with git v1.4.0 today, and noticed a\n> problem fetching from GitHub. Pre-OFS_DELTA git versions are ancient by\n> today's standard, but it's quite easy to remain compatible here, so I\n> don't see why not.\n\n\n\n\n And in theory, alternate implementations might not\n> understand OFS_DELTA, though in practice I would consider such an\n> implementation to be pretty crappy.\n>\n> builtin/pack-objects.c | 13 ++++++++++++-\n> 1 file changed, 12 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 7950c43..1503632 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -2439,12 +2439,23 @@ static void loosen_unused_packed_objects(struct rev_info *revs)\n> \t}\n> }\n> \n> +/*\n> + * This tracks any options which a reader of the pack might\n> + * not understand, and which would therefore prevent blind reuse\n> + * of what we have on disk.\n> + */\n> +static int pack_options_allow_reuse(void)\n> +{\n> +\treturn allow_ofs_delta;\n> +}\n> +\n> static int get_object_list_from_bitmap(struct rev_info *revs)\n> {\n> \tif (prepare_bitmap_walk(revs) < 0)\n> \t\treturn -1;\n> \n> -\tif (!reuse_partial_packfile_from_bitmap(\n> +\tif (pack_options_allow_reuse() &&\n> +\t !reuse_partial_packfile_from_bitmap(\n> \t\t\t&reuse_packfile,\n> \t\t\t&reuse_packfile_objects,\n> \t\t\t&reuse_packfile_offset)) {\n"},{"id":"238382","messageId":"20140404214848.GA23666@sigill.intra.peff.net","threadId":"36340","inReplyTo":"xmqqfvlvvdfi.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] pack-objects: do not reuse packfiles without --delta-base-offset","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-04T21:48:49Z","receivedAt":"2014-04-04T21:48:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 02, 2014 at 10:39:13AM -0700, Junio C Hamano wrote:\n\n> > However, it's possible that the other side cannot read our\n> > packfile verbatim. For example, we may have objects stored\n> > as OFS_DELTA, but the client is an antique version of git\n> > that only understands REF_DELTA. We negotiate this\n> > capability over the fetch protocol. A normal pack-objects\n> > run will convert OFS_DELTA into REF_DELTA on the fly, but\n> > the \"reuse pack\" code path never even looks at the objects.\n> \n> The above makes it sound like \"reuse pack\" codepath is broken.\n\nIt is broken (without this patch), though in practice only for ancient\n(pre-1.4.x) clients.\n\n> Is it too much hassle to peek at the initial bytes of each object to\n> see how they are encoded? Would it be possible to convert OFS_DELTA to\n> REF_DELTA on the fly on that codepath as well, instead of disabling\n> the reuse altogether?\n\nIt's a mistake to peek ahead of time. Part of the point of the\npack-reuse optimization is to start sending out bytes as soon as\npossible, since the network is quite often the bottleneck. So we would\nnot want to look through all of the to-be-sent data before sending out\nthe first byte.\n\nWe could convert OFS_DELTA to REF_DELTA on the fly. That _may_ have a\nperformance impact. Right now, we are basically doing the equivalent of\nsendfile(), and conversion would involve iterating through each object\nand examining the header. I think that's probably not too bad, though.\nThe most expensive part of that, stepping to the next object, requires\nscanning through the zlib packets, but we should be able to use the\nrevidx to avoid that.\n\nI'm not sure it's even worth the code complexity, though. The non-reuse\ncodepath is not that much slower, and it should be extremely rare for a\nclient not to support OFS_DELTA these days.\n\n-Peff\n"},{"id":"238383","messageId":"xmqqsipsohjz.fsf@gitster.dls.corp.google.com","threadId":"36340","inReplyTo":"20140404214848.GA23666@sigill.intra.peff.net","subject":"Re: [PATCH] pack-objects: do not reuse packfiles without --delta-base-offset","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-04T22:28:48Z","receivedAt":"2014-04-04T22:28:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> We could convert OFS_DELTA to REF_DELTA on the fly. That _may_ have a\n> performance impact. Right now, we are basically doing the equivalent of\n> sendfile(), and conversion would involve iterating through each object\n> and examining the header. I think that's probably not too bad, though.\n> The most expensive part of that, stepping to the next object, requires\n> scanning through the zlib packets, but we should be able to use the\n> revidx to avoid that.\n>\n> I'm not sure it's even worth the code complexity, though. The non-reuse\n> codepath is not that much slower, and it should be extremely rare for a\n> client not to support OFS_DELTA these days.\n\nOK, together with the fact that only ancient versions of fetcher\nwould trigger this \"do not reuse\" codepath, I agree that we should\ngo the simplest route this patch takes.\n\nThanks.\n"},{"id":"238386","messageId":"20140404231301.GA2528@sigill.intra.peff.net","threadId":"36340","inReplyTo":"xmqqsipsohjz.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] pack-objects: do not reuse packfiles without --delta-base-offset","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-04T23:13:01Z","receivedAt":"2014-04-04T23:13:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 04, 2014 at 03:28:48PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > We could convert OFS_DELTA to REF_DELTA on the fly. That _may_ have a\n> > performance impact. Right now, we are basically doing the equivalent of\n> > sendfile(), and conversion would involve iterating through each object\n> > and examining the header. I think that's probably not too bad, though.\n> > The most expensive part of that, stepping to the next object, requires\n> > scanning through the zlib packets, but we should be able to use the\n> > revidx to avoid that.\n> >\n> > I'm not sure it's even worth the code complexity, though. The non-reuse\n> > codepath is not that much slower, and it should be extremely rare for a\n> > client not to support OFS_DELTA these days.\n> \n> OK, together with the fact that only ancient versions of fetcher\n> would trigger this \"do not reuse\" codepath, I agree that we should\n> go the simplest route this patch takes.\n\nBy the way, we may want to revisit this if we grow more features that do\nnot allow straight byte-for-byte reuse. I am thinking specifically if we\ngrow a packv4-like representation for an object, and we plan to convert\non-the-fly to existing packv2 clients. But I think the sensible steps\nfor that are:\n\n 1. If we have v4 on disk and are outputting v2, add this case to the\n \"can_reuse\" function I just added. I.e., start out correct, and\n turn off the optimization.\n\n 2. Experiment with on-the-fly conversion. It may be that the\n conversion is so expensive that the reuse optimization gets lost in\n the noise. Or maybe we can reclaim most of the advantage of the\n reuse code path, and it is worth going object-by-object and\n converting. But we won't know until we can measure.\n\n-Peff\n"},{"id":"238475","messageId":"xmqqvbulm56o.fsf@gitster.dls.corp.google.com","threadId":"36340","inReplyTo":"20140404231301.GA2528@sigill.intra.peff.net","subject":"Re: [PATCH] pack-objects: do not reuse packfiles without --delta-base-offset","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-07T17:15:43Z","receivedAt":"2014-04-07T17:15:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Apr 04, 2014 at 03:28:48PM -0700, Junio C Hamano wrote:\n> ...\n>> OK, together with the fact that only ancient versions of fetcher\n>> would trigger this \"do not reuse\" codepath, I agree that we should\n>> go the simplest route this patch takes.\n>\n> By the way, we may want to revisit this if we grow more features that do\n> not allow straight byte-for-byte reuse. \n\nTrue.\n\n> I am thinking specifically if we\n> grow a packv4-like representation for an object, and we plan to convert\n> on-the-fly to existing packv2 clients. But I think the sensible steps\n> for that are:\n>\n> 1. If we have v4 on disk and are outputting v2, add this case to the\n> \"can_reuse\" function I just added. I.e., start out correct, and\n> turn off the optimization.\n>\n> 2. Experiment with on-the-fly conversion. It may be that the\n> conversion is so expensive that the reuse optimization gets lost in\n> the noise. Or maybe we can reclaim most of the advantage of the\n> reuse code path, and it is worth going object-by-object and\n> converting. But we won't know until we can measure.\n\nYeah; I think these are sensible steps in the future direction.\n\nThanks.\n"}]}