{"thread":{"id":"36147","subject":"No progress from push when using bitmaps","startedAt":"2014-03-13T00:21:21Z","lastAt":"2014-03-14T23:53:37Z","messageCount":10,"participants":["Shawn Pearce","Jeff King","Junio C Hamano","Michael Haggerty","Duy Nguyen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"236653","messageId":"CAJo=hJth1YB+u2Y9S0X3xk7s2-HcPP4_k03qprHBU4A9721i8g@mail.gmail.com","threadId":"36147","inReplyTo":null,"subject":"No progress from push when using bitmaps","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2014-03-13T00:21:21Z","receivedAt":"2014-03-13T00:21:21Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Today I tried pushing a copy of linux.git from a client that had\nbitmaps into a JGit server. The client stalled for a long time with no\nprogress, because it reused the existing pack. No progress appeared\nwhile it was sending the existing file on the wire:\n\n  $ git push git://localhost/linux.git master\n  Reusing existing pack: 2938117, done.\n  Total 2938117 (delta 0), reused 0 (delta 0)\n  remote: Resolving deltas:  66% (1637269/2455727)\n\nThis is not the best user experience. :-(\n"},{"id":"236689","messageId":"20140313212626.GA23618@sigill.intra.peff.net","threadId":"36147","inReplyTo":"CAJo=hJth1YB+u2Y9S0X3xk7s2-HcPP4_k03qprHBU4A9721i8g@mail.gmail.com","subject":"Re: No progress from push when using bitmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-13T21:26:26Z","receivedAt":"2014-03-13T21:26:26Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 12, 2014 at 05:21:21PM -0700, Shawn Pearce wrote:\n\n> Today I tried pushing a copy of linux.git from a client that had\n> bitmaps into a JGit server. The client stalled for a long time with no\n> progress, because it reused the existing pack. No progress appeared\n> while it was sending the existing file on the wire:\n> \n>   $ git push git://localhost/linux.git master\n>   Reusing existing pack: 2938117, done.\n>   Total 2938117 (delta 0), reused 0 (delta 0)\n>   remote: Resolving deltas:  66% (1637269/2455727)\n> \n> This is not the best user experience. :-(\n\nYeah, I agree that sucks. I hadn't noticed it, as I don't typically have\nmy client repos bitmapped (and on fetch, the interesting progress is\ncoming from the local index-pack).\n\nIt would definitely be good to have throughput measurements while\nwriting out the pack. However, I'm not sure we have anything useful to\ncount. We know the total number of objects we're reusing, but we're not\nactually parsing the data; we're just blitting it out as a stream. I\nthink the progress code will need some refactoring to handle a\nthroughput-only case.\n\n-Peff\n"},{"id":"236690","messageId":"CAJo=hJu1WscibwectWzrUcoOV4-9xk4hBm1JA+RjONmgvZTXwQ@mail.gmail.com","threadId":"36147","inReplyTo":"20140313212626.GA23618@sigill.intra.peff.net","subject":"Re: No progress from push when using bitmaps","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2014-03-13T22:01:09Z","receivedAt":"2014-03-13T22:01:09Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Thu, Mar 13, 2014 at 2:26 PM, Jeff King <peff@peff.net> wrote:\n> On Wed, Mar 12, 2014 at 05:21:21PM -0700, Shawn Pearce wrote:\n>\n>> Today I tried pushing a copy of linux.git from a client that had\n>> bitmaps into a JGit server. The client stalled for a long time with no\n>> progress, because it reused the existing pack. No progress appeared\n>> while it was sending the existing file on the wire:\n>>\n>>   $ git push git://localhost/linux.git master\n>>   Reusing existing pack: 2938117, done.\n>>   Total 2938117 (delta 0), reused 0 (delta 0)\n>>   remote: Resolving deltas:  66% (1637269/2455727)\n>>\n>> This is not the best user experience. :-(\n>\n> Yeah, I agree that sucks. I hadn't noticed it, as I don't typically have\n> my client repos bitmapped (and on fetch, the interesting progress is\n> coming from the local index-pack).\n>\n> It would definitely be good to have throughput measurements while\n> writing out the pack. However, I'm not sure we have anything useful to\n> count. We know the total number of objects we're reusing, but we're not\n> actually parsing the data; we're just blitting it out as a stream. I\n> think the progress code will need some refactoring to handle a\n> throughput-only case.\n\nYes. I think JGit suffers from this same bug, and again we never\nnoticed it because usually only the servers are bitmapped, not the\nclients.\n\npack-objects writes a throughput meter when its writing objects.\nReally just the bytes out/second would be enough to let the user know\nthe client is working. Unfortunately I think that is still tied to the\noverall progress system having some other counter?\n"},{"id":"236691","messageId":"20140313220754.GA20173@sigill.intra.peff.net","threadId":"36147","inReplyTo":"CAJo=hJu1WscibwectWzrUcoOV4-9xk4hBm1JA+RjONmgvZTXwQ@mail.gmail.com","subject":"Re: No progress from push when using bitmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-13T22:07:54Z","receivedAt":"2014-03-13T22:07:54Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 13, 2014 at 03:01:09PM -0700, Shawn Pearce wrote:\n\n> > It would definitely be good to have throughput measurements while\n> > writing out the pack. However, I'm not sure we have anything useful to\n> > count. We know the total number of objects we're reusing, but we're not\n> > actually parsing the data; we're just blitting it out as a stream. I\n> > think the progress code will need some refactoring to handle a\n> > throughput-only case.\n> \n> Yes. I think JGit suffers from this same bug, and again we never\n> noticed it because usually only the servers are bitmapped, not the\n> clients.\n> \n> pack-objects writes a throughput meter when its writing objects.\n> Really just the bytes out/second would be enough to let the user know\n> the client is working. Unfortunately I think that is still tied to the\n> overall progress system having some other counter?\n\nYes, I'm looking at it right now. The throughput meter is actually\nconnected to the sha1fd output. So really we just need to call\ndisplay_progress periodically as we loop through the data. It's a\none-liner fix.\n\n_But_ it still looks ugly, because, as you mention, it's tied to the\nprogress meter, which is counting up to N objects. So we basically sit\nthere at \"0\", pumping data, and then after the pack is done, we can say\nwe sent N. :)\n\nThere are a few ways around this:\n\n  1. Add a new phase \"Writing packs\" which counts from 0 to 1. Even\n     though it's more accurate, moving from 0 to 1 really isn't that\n     useful (the throughput is, but the 0/1 just looks like noise).\n\n  2. Add a new phase \"Writing reused objects\" that counts from 0 bytes\n     up to N bytes. This looks stupid, though, because we are repeating\n     the current byte count both here and in the throughput.\n\n  3. Use the regular \"Writing objects\" progress, but fake the object\n     count. We know we are writing M bytes with N objects. Bump the\n     counter by 1 for every M/N bytes we write.\n\nThe first two require some non-trivial surgery to the progress code. I\nam leaning towards the third. Not just because it's easy, but because I\nthink it actually shows the most intuitive display. Yes, it's fudging\nthe object numbers, but those are largely meaningless anyway (in fact,\nit makes them _better_ because now they're even, instead of getting 95%\ndone and then hitting some blob that is as big as the rest of the repo\ncombined).\n\n-Peff\n"},{"id":"236693","messageId":"xmqqr465hh64.fsf@gitster.dls.corp.google.com","threadId":"36147","inReplyTo":"20140313220754.GA20173@sigill.intra.peff.net","subject":"Re: No progress from push when using bitmaps","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-13T22:23:15Z","receivedAt":"2014-03-13T22:23:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> There are a few ways around this:\n>\n>   1. Add a new phase \"Writing packs\" which counts from 0 to 1. Even\n>      though it's more accurate, moving from 0 to 1 really isn't that\n>      useful (the throughput is, but the 0/1 just looks like noise).\n>\n>   2. Add a new phase \"Writing reused objects\" that counts from 0 bytes\n>      up to N bytes. This looks stupid, though, because we are repeating\n>      the current byte count both here and in the throughput.\n>\n>   3. Use the regular \"Writing objects\" progress, but fake the object\n>      count. We know we are writing M bytes with N objects. Bump the\n>      counter by 1 for every M/N bytes we write.\n>\n> The first two require some non-trivial surgery to the progress code. I\n> am leaning towards the third. Not just because it's easy, but because I\n> think it actually shows the most intuitive display. Yes, it's fudging\n> the object numbers, but those are largely meaningless anyway (in fact,\n> it makes them _better_ because now they're even, instead of getting 95%\n> done and then hitting some blob that is as big as the rest of the repo\n> combined).\n\nI think the above argument, especially the \"fudging but largely\nmeaningless anyway\" part, makes perfect sense.\n\nThanks for looking into this.\n"},{"id":"236694","messageId":"20140313222428.GA18406@sigill.intra.peff.net","threadId":"36147","inReplyTo":"20140313220754.GA20173@sigill.intra.peff.net","subject":"Re: No progress from push when using bitmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-13T22:24:28Z","receivedAt":"2014-03-13T22:24:28Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 13, 2014 at 06:07:54PM -0400, Jeff King wrote:\n\n>   3. Use the regular \"Writing objects\" progress, but fake the object\n>      count. We know we are writing M bytes with N objects. Bump the\n>      counter by 1 for every M/N bytes we write.\n\nHere is that strategy. I think it looks pretty nice, and it seamlessly\nhandles the case where you have extra objects to send on top of the\nreused pack (we just keep the same progress meter counting up).\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 831dd05..f187859 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -709,7 +709,7 @@ static struct object_entry **compute_write_order(void)\n static off_t write_reused_pack(struct sha1file *f)\n {\n \tunsigned char buffer[8192];\n-\toff_t to_write;\n+\toff_t to_write, total;\n \tint fd;\n \n \tif (!is_pack_valid(reuse_packfile))\n@@ -726,7 +726,7 @@ static off_t write_reused_pack(struct sha1file *f)\n \tif (reuse_packfile_offset < 0)\n \t\treuse_packfile_offset = reuse_packfile->pack_size - 20;\n \n-\tto_write = reuse_packfile_offset - sizeof(struct pack_header);\n+\ttotal = to_write = reuse_packfile_offset - sizeof(struct pack_header);\n \n \twhile (to_write) {\n \t\tint read_pack = xread(fd, buffer, sizeof(buffer));\n@@ -739,10 +739,23 @@ static off_t write_reused_pack(struct sha1file *f)\n \n \t\tsha1write(f, buffer, read_pack);\n \t\tto_write -= read_pack;\n+\n+\t\t/*\n+\t\t * We don't know the actual number of objects written,\n+\t\t * only how many bytes written, how many bytes total, and\n+\t\t * how many objects total. So we can fake it by pretending all\n+\t\t * objects we are writing are the same size. This gives us a\n+\t\t * smooth progress meter, and at the end it matches the true\n+\t\t * answer.\n+\t\t */\n+\t\twritten = reuse_packfile_objects *\n+\t\t\t\t(((double)(total - to_write)) / total);\n+\t\tdisplay_progress(progress_state, written);\n \t}\n \n \tclose(fd);\n-\twritten += reuse_packfile_objects;\n+\twritten = reuse_packfile_objects;\n+\tdisplay_progress(progress_state, written);\n \treturn reuse_packfile_offset - sizeof(struct pack_header);\n }\n \n"},{"id":"236722","messageId":"5322CF58.6060506@alum.mit.edu","threadId":"36147","inReplyTo":"20140313220754.GA20173@sigill.intra.peff.net","subject":"Re: No progress from push when using bitmaps","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-14T09:43:52Z","receivedAt":"2014-03-14T09:43:52Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/13/2014 11:07 PM, Jeff King wrote:\n> On Thu, Mar 13, 2014 at 03:01:09PM -0700, Shawn Pearce wrote:\n> \n>>> It would definitely be good to have throughput measurements while\n>>> writing out the pack. However, I'm not sure we have anything useful to\n>>> count. We know the total number of objects we're reusing, but we're not\n>>> actually parsing the data; we're just blitting it out as a stream. I\n>>> think the progress code will need some refactoring to handle a\n>>> throughput-only case.\n>>\n>> Yes. I think JGit suffers from this same bug, and again we never\n>> noticed it because usually only the servers are bitmapped, not the\n>> clients.\n>>\n>> pack-objects writes a throughput meter when its writing objects.\n>> Really just the bytes out/second would be enough to let the user know\n>> the client is working. Unfortunately I think that is still tied to the\n>> overall progress system having some other counter?\n> \n> Yes, I'm looking at it right now. The throughput meter is actually\n> connected to the sha1fd output. So really we just need to call\n> display_progress periodically as we loop through the data. It's a\n> one-liner fix.\n> \n> _But_ it still looks ugly, because, as you mention, it's tied to the\n> progress meter, which is counting up to N objects. So we basically sit\n> there at \"0\", pumping data, and then after the pack is done, we can say\n> we sent N. :)\n> \n> There are a few ways around this:\n> \n>   1. Add a new phase \"Writing packs\" which counts from 0 to 1. Even\n>      though it's more accurate, moving from 0 to 1 really isn't that\n>      useful (the throughput is, but the 0/1 just looks like noise).\n> \n>   2. Add a new phase \"Writing reused objects\" that counts from 0 bytes\n>      up to N bytes. This looks stupid, though, because we are repeating\n>      the current byte count both here and in the throughput.\n> \n>   3. Use the regular \"Writing objects\" progress, but fake the object\n>      count. We know we are writing M bytes with N objects. Bump the\n>      counter by 1 for every M/N bytes we write.\n\nWould it be practical to change it to a percentage of bytes written?\nThen we'd have progress info that is both convenient *and* truthful.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"236723","messageId":"CACsJy8BZd0uL2M8NHO+sFcWfkmoyuVnU1Mh2eKZ2OXm=Y-Ge5g@mail.gmail.com","threadId":"36147","inReplyTo":"5322CF58.6060506@alum.mit.edu","subject":"Re: No progress from push when using bitmaps","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-03-14T10:21:59Z","receivedAt":"2014-03-14T10:21:59Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Mar 14, 2014 at 4:43 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> Would it be practical to change it to a percentage of bytes written?\n> Then we'd have progress info that is both convenient *and* truthful.\n\nI agreed for a second, then remembered that we don't know the final\npack size until we finish writing it.. Not sure if we could estimate\n(cheaply) with a good accuracy though.\n\nIf an object is reused, we already know its compressed size. If it's\nnot reused and is a loose object, we could use on-disk size. It's a\nlot harder to estimate an not-reused, deltified object. All we have is\nthe uncompressed size, and the size of each delta in the delta chain..\nNeither gives a good hint of what the compressed size would be.\n-- \nDuy\n"},{"id":"236728","messageId":"20140314152939.GA6186@sigill.intra.peff.net","threadId":"36147","inReplyTo":"CACsJy8BZd0uL2M8NHO+sFcWfkmoyuVnU1Mh2eKZ2OXm=Y-Ge5g@mail.gmail.com","subject":"Re: No progress from push when using bitmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-14T15:29:40Z","receivedAt":"2014-03-14T15:29:40Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 14, 2014 at 05:21:59PM +0700, Duy Nguyen wrote:\n\n> On Fri, Mar 14, 2014 at 4:43 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> > Would it be practical to change it to a percentage of bytes written?\n> > Then we'd have progress info that is both convenient *and* truthful.\n> \n> I agreed for a second, then remembered that we don't know the final\n> pack size until we finish writing it.. Not sure if we could estimate\n> (cheaply) with a good accuracy though.\n\nRight. I'm not sure what Michael meant by \"it\". We can send a percentage\nof bytes written for the reused pack (my option 2), but we do not know\nthe total bytes for the rest of the objects. So we'd end up with two\nprogress meters (one for the reused pack, and one for everything else),\nboth counting up to different endpoints. And it would require quite a\nfew changes to the progress code.\n\n> If an object is reused, we already know its compressed size. If it's\n> not reused and is a loose object, we could use on-disk size. It's a\n> lot harder to estimate an not-reused, deltified object. All we have is\n> the uncompressed size, and the size of each delta in the delta chain..\n> Neither gives a good hint of what the compressed size would be.\n\nHmm. I think we do have the compressed delta size after having run the\ncompression phase (because that is ultimately what we compare to find\nthe best delta). Loose objects are probably the hardest here, as we\nactually recompress them (IIRC, because packfiles encode the type/size\ninfo outside of the compressed bit, whereas it is inside for loose\nobjects; the \"experimental loose\" format harmonized this, but it never\ncaught on).\n\nWithout doing that recompression, any value you came up with would be an\nestimate, though it would be pretty close (not off by more than a few\nbytes per object). However, you can't just run through the packing list\nand add up the object sizes; you'd need to do a real \"dry-run\" through\nthe writing phase. There are probably more I'm missing, but you need at\nleast to figure out:\n\n  1. The actual compressed size of a full loose object, as described\n     above.\n\n  2. The variable-length headers for each object based on its type and\n     size.\n\n  3. The final form that the object will take based on what has come\n     before. For example, if there is a max pack size, we may split an\n     object from its delta base, in which case we have to throw away the\n     delta. We don't know where those breaks will be until we walk\n     through the whole list.\n\n  4. If an object we attempt to reuse turns out to be corrupted, we\n     fall back to the non-reuse code path, which will have a different\n     size. So you'd need to actually check the reused object CRCs during\n     the dry-run (and for local repacks, not transfers, we actually\n     inflate and check the zlib, too, for safety).\n\nSo I think it's _possible_. But it's definitely not trivial. For now, I\nthink it makes sense to go with something like the patch I posted\nearlier (which I'll re-roll in a few minutes). That fixes what is IMHO a\nregression in the bitmaps case. And it does not make it any harder for\nsomebody to later convert us to a true byte-counter (i.e., it is the\neasy half already).\n\n-Peff\n"},{"id":"236761","messageId":"CACsJy8BrdN5sbuMEp+G-gUBDcSwFscX_4pYe-U5bOkuq6=J4gw@mail.gmail.com","threadId":"36147","inReplyTo":"20140314152939.GA6186@sigill.intra.peff.net","subject":"Re: No progress from push when using bitmaps","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-03-14T23:53:37Z","receivedAt":"2014-03-14T23:53:37Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Mar 14, 2014 at 10:29 PM, Jeff King <peff@peff.net> wrote:\n>> If an object is reused, we already know its compressed size. If it's\n>> not reused and is a loose object, we could use on-disk size. It's a\n>> lot harder to estimate an not-reused, deltified object. All we have is\n>> the uncompressed size, and the size of each delta in the delta chain..\n>> Neither gives a good hint of what the compressed size would be.\n>\n> Hmm. I think we do have the compressed delta size after having run the\n> compression phase (because that is ultimately what we compare to find\n> the best delta).\n\nThere are cases when we try not to find deltas (large blobs, file too\nsmall, or -delta attribute). The large blob case is especially\ninteresting because progress bar crawls slowly when they write these\nobjects.\n\n> Loose objects are probably the hardest here, as we\n> actually recompress them (IIRC, because packfiles encode the type/size\n> info outside of the compressed bit, whereas it is inside for loose\n> objects; the \"experimental loose\" format harmonized this, but it never\n> caught on).\n>\n> Without doing that recompression, any value you came up with would be an\n> estimate, though it would be pretty close (not off by more than a few\n> bytes per object).\n\nThat's my hope. Although if they tweak compression level then the\nestimation could be off (gzip -9 and gzip -1 produce big difference in\nsize)\n\n> However, you can't just run through the packing list\n> and add up the object sizes; you'd need to do a real \"dry-run\" through\n> the writing phase. There are probably more I'm missing, but you need at\n> least to figure out:\n>\n>   1. The actual compressed size of a full loose object, as described\n>      above.\n>\n>   2. The variable-length headers for each object based on its type and\n>      size.\n\nWe could run through a \"typical\" repo, calculate the average header\nlength then use it for all objects?\n\n>\n>   3. The final form that the object will take based on what has come\n>      before. For example, if there is a max pack size, we may split an\n>      object from its delta base, in which case we have to throw away the\n>      delta. We don't know where those breaks will be until we walk\n>      through the whole list.\n\nAh this could probably be avoided. max pack size does not apply to\nstreaming pack-objects, where progress bar is most shown. Falling back\nto object number in this case does not sound too bad.\n\n>\n>   4. If an object we attempt to reuse turns out to be corrupted, we\n>      fall back to the non-reuse code path, which will have a different\n>      size. So you'd need to actually check the reused object CRCs during\n>      the dry-run (and for local repacks, not transfers, we actually\n>      inflate and check the zlib, too, for safety).\n\nUgh..\n\n>\n> So I think it's _possible_. But it's definitely not trivial. For now, I\n> think it makes sense to go with something like the patch I posted\n> earlier (which I'll re-roll in a few minutes). That fixes what is IMHO a\n> regression in the bitmaps case. And it does not make it any harder for\n> somebody to later convert us to a true byte-counter (i.e., it is the\n> easy half already).\n\nAgreed.\n-- \nDuy\n"}]}