{"thread":{"id":"4229","subject":"[RFC] send-pack: allow skipping delta when sending pack","startedAt":"2006-05-21T05:48:27Z","lastAt":"2006-05-21T09:40:46Z","messageCount":10,"participants":["Jeff King","Junio C Hamano","Jakub Narebski"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"20375","messageId":"20060521054827.GA18530@coredump.intra.peff.net","threadId":"4229","inReplyTo":null,"subject":"[RFC] send-pack: allow skipping delta when sending pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2006-05-21T05:48:27Z","receivedAt":"2006-05-21T05:48:27Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I have a git repo where I keep relatively large files (digital photos).\nI have a local repo and a \"master\" repo on a server which I access over\nssh.  Deltifying a bunch of large images takes a relatively long time. I\ncan live with this while packing (though it is slightly annoying to have\nto pack separately on both repos, I understand why it might be hard to\nreuse the deltification).\n\nHowever, it is extremely annoying to add a large set of images and then\npush them to the server. The server is on a 100Mbit LAN, so the\ndeltification part of the process takes up most of the time (and\ntypically ends up making no deltas, since the files are unrelated\nimages). The patch below causes the GIT_NODELTA environment variable to\nset the window depth to 0 when sending a pack, preventing deltification.\n\nThe result is much better performance in my case. However, the method\nseems quite hack-ish, so I wanted to get comments on how this should be\ndone. Possibilities I considered:\n  1. A command line option to git-send-pack. The problem with this is\n     that support is required from git-push and cg-push to pass the\n     option through.\n  2. A repo config variable that says not to deltify on sending (or\n     potentially, not to deltify at all, which makes packing in general\n     much nicer -- however, I don't think this is a good idea, as I do\n     still want deltification rarely, it's just that it mostly will\n     fail). This should probably be per-remote for the obvious reason\n     that one might push to local and remote repos. One drawback is that\n     sometimes deltification may be a win; it's just that I sometimes\n     know that it won't be (because I added a bunch of unrelated large\n     files). It's nice to selectively turn this option on for a given\n     push.\n  3. Ideally, we could do some heuristic to see if deltification will\n     yield helpful results. In particular, we may already have a pack\n     with these commits in it (especially if we just repack before a\n     push). If we can re-use this information, it at least saves\n     deltifying twice (once to pack, once to push). In theory, I would\n     think the fact that we don't pass --no-reuse-delta to pack-objects\n     means that this would happen automatically, but it clearly doesn't.\n\nComments?\n\n---\n\nf1cf653120dd492d1c86ee2a92a9c8221023cef1\n send-pack.c |    6 ++++++\n 1 files changed, 6 insertions(+), 0 deletions(-)\n\nf1cf653120dd492d1c86ee2a92a9c8221023cef1\ndiff --git a/send-pack.c b/send-pack.c\nindex 409f188..4ad6489 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -30,8 +30,14 @@ static void exec_pack_objects(void)\n \tstatic const char *args[] = {\n \t\t\"pack-objects\",\n \t\t\"--stdout\",\n+\t\tNULL,\n \t\tNULL\n \t};\n+\tconst char *nodelta;\n+\n+\tnodelta = getenv(\"GIT_NODELTA\");\n+\tif(nodelta && !strcmp(nodelta, \"1\"))\n+\t\targs[2] = \"--depth=0\";\n \texecv_git_cmd(args);\n \tdie(\"git-pack-objects exec failed (%s)\", strerror(errno));\n }\n-- \n1.3.3.g288c-dirty\n"},{"id":"20376","messageId":"7vy7wvx5o9.fsf@assigned-by-dhcp.cox.net","threadId":"4229","inReplyTo":"20060521054827.GA18530@coredump.intra.peff.net","subject":"Re: [RFC] send-pack: allow skipping delta when sending pack","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-05-21T06:17:42Z","receivedAt":"2006-05-21T06:17:42Z","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> The result is much better performance in my case. However, the method\n> seems quite hack-ish, so I wanted to get comments on how this should be\n> done. Possibilities I considered:\n\n>   1. A command line option to git-send-pack. The problem with this is\n>      that support is required from git-push and cg-push to pass the\n>      option through.\n\nWhen you pull from such a repository you would also need to be\nable to control this.  The repository owner knows what's in the\nrepository a lot more than the downloader, so some repository\nconfiguration that tells upload-pack to use such-and-such delta\nwindow is also needed.  But as you say below:\n\n>   3. Ideally, we could do some heuristic to see if deltification will\n>      yield helpful results. In particular, we may already have a pack\n>      with these commits in it (especially if we just repack before a\n>      push). If we can re-use this information, it at least saves\n>      deltifying twice (once to pack, once to push). In theory, I would\n>      think the fact that we don't pass --no-reuse-delta to pack-objects\n>      means that this would happen automatically, but it clearly doesn't.\n\nThe lack of --no-reuse-delta just means \"if the object we are\ngoing to send is a delta in the source, and its delta base is\nalso something we are going to send, then pretend that it is the\nbase delta for that object to skip computation\".  What you want\nhere is \"if the object we are going to send is not a delta in\nthe source, and there are sufficient number of other objects the\nobject could have been deltified against, then it is very likely\nthat it was not worth deltifying when it was packed; so it is\nprobably not worth deltifying it now\".\n"},{"id":"20379","messageId":"20060521081435.GA4526@coredump.intra.peff.net","threadId":"4229","inReplyTo":"7vy7wvx5o9.fsf@assigned-by-dhcp.cox.net","subject":"Re: [RFC] send-pack: allow skipping delta when sending pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2006-05-21T08:14:35Z","receivedAt":"2006-05-21T08:14:35Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 20, 2006 at 11:17:42PM -0700, Junio C Hamano wrote:\n\n> base delta for that object to skip computation\".  What you want\n> here is \"if the object we are going to send is not a delta in\n> the source, and there are sufficient number of other objects the\n> object could have been deltified against, then it is very likely\n> that it was not worth deltifying when it was packed; so it is\n> probably not worth deltifying it now\".\n\nI think we can make a stronger statement in many cases: \"if the object\nwe are going to send is not a delta in the source, and there are no\nother objects it could be deltified against, then it is not worth\ndeltifying.\" That is, in the case that we just packed we KNOW that it's\nnot worth it, since we're not sending anything that isn't already\npacked.\n\nFollowing this logic, I believe we could always turn off deltification\nif there are no loose objects. It seems a bit special case, but it\noptimizes the \"repack -d then push\" workflow which I suspect may be\nrelatively common.\n\nThe patch below runs git-rev-list with --unpacked; if there are no\nobjects returned, it tells pack-objects to set depth=0 (is that really\nwhat we want? It's OK to use existing deltas; we just don't want to\ncompute any new ones. I'm not sure how --depth behaves in that respect).\n\n-Peff\n\n---\n\nf3e7fb4a5025cc8157557f3da6f9dc7d0a89395f\n send-pack.c |   72 ++++++++++++++++++++++++++++++++++++++++++++++++-----------\n 1 files changed, 58 insertions(+), 14 deletions(-)\n\nf3e7fb4a5025cc8157557f3da6f9dc7d0a89395f\ndiff --git a/send-pack.c b/send-pack.c\nindex 409f188..0bddc0a 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -25,18 +25,7 @@ static int is_zero_sha1(const unsigned c\n \treturn 1;\n }\n \n-static void exec_pack_objects(void)\n-{\n-\tstatic const char *args[] = {\n-\t\t\"pack-objects\",\n-\t\t\"--stdout\",\n-\t\tNULL\n-\t};\n-\texecv_git_cmd(args);\n-\tdie(\"git-pack-objects exec failed (%s)\", strerror(errno));\n-}\n-\n-static void exec_rev_list(struct ref *refs)\n+static void exec_rev_list(struct ref *refs, int unpacked)\n {\n \tstruct ref *ref;\n \tstatic const char *args[1000];\n@@ -47,6 +36,8 @@ static void exec_rev_list(struct ref *re\n \t\targs[i++] = \"--objects-edge\";\n \telse\n \t\targs[i++] = \"--objects\";\n+\tif(unpacked)\n+\t\targs[i++] = \"--unpacked\";\n \n \t/* First send the ones we care about most */\n \tfor (ref = refs; ref; ref = ref->next) {\n@@ -85,6 +76,59 @@ static void exec_rev_list(struct ref *re\n \tdie(\"git-rev-list exec failed (%s)\", strerror(errno));\n }\n \n+static int rev_list_loose_objects(struct ref *refs)\n+{\n+\tint fd[2];\n+\tpid_t pid;\n+\n+\tif(pipe(fd) < 0)\n+\t\tdie(\"rev-list setup: pipe failed\");\n+\tpid = fork();\n+\tif(pid < 0)\n+\t\tdie(\"rev-list setup: fork failed\");\n+\tif(!pid) {\n+\t\tdup2(fd[1], 1);\n+\t\tclose(fd[0]);\n+\t\tclose(fd[1]);\n+\t\texec_rev_list(refs, 1);\n+\t\tdie(\"rev-list failed\");\n+\t}\n+\tclose(fd[1]);\n+\treturn fd[0];\n+}\n+\n+static int count_loose_objects(struct ref *refs)\n+{\n+\tint fd = rev_list_loose_objects(refs);\n+\tunsigned count = 0;\n+\tchar buf[4096];\n+\tint len;\n+\n+\twhile((len = read(fd, buf, sizeof buf)) > 0) {\n+\t\tint i;\n+\t\tfor(i = 0; i < len; i++)\n+\t\t\tif(buf[i] == '\\n')\n+\t\t\t\tcount++;\n+\t}\n+\tif(len < 0)\n+\t\tdie(\"rev-list read failed\");\n+\treturn count;\n+}\n+\n+static void exec_pack_objects(struct ref *refs)\n+{\n+\tstatic const char *args[] = {\n+\t\t\"pack-objects\",\n+\t\t\"--stdout\",\n+\t\tNULL,\n+\t\tNULL\n+\t};\n+\tif(count_loose_objects(refs) == 0)\n+\t\targs[2] = \"--depth=0\";\n+\texecv_git_cmd(args);\n+\tdie(\"git-pack-objects exec failed (%s)\", strerror(errno));\n+}\n+\n static void rev_list(int fd, struct ref *refs)\n {\n \tint pipe_fd[2];\n@@ -99,7 +143,7 @@ static void rev_list(int fd, struct ref \n \t\tclose(pipe_fd[0]);\n \t\tclose(pipe_fd[1]);\n \t\tclose(fd);\n-\t\texec_pack_objects();\n+\t\texec_pack_objects(refs);\n \t\tdie(\"pack-objects setup failed\");\n \t}\n \tif (pack_objects_pid < 0)\n@@ -108,7 +152,7 @@ static void rev_list(int fd, struct ref \n \tclose(pipe_fd[0]);\n \tclose(pipe_fd[1]);\n \tclose(fd);\n-\texec_rev_list(refs);\n+\texec_rev_list(refs, 0);\n }\n \n static int pack_objects(int fd, struct ref *refs)\n-- \n1.3.3.gf1cf-dirty\n"},{"id":"20381","messageId":"e4p83e$uqt$1@sea.gmane.org","threadId":"4229","inReplyTo":"20060521081435.GA4526@coredump.intra.peff.net","subject":"Re: [RFC] send-pack: allow skipping delta when sending pack","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-05-21T08:24:37Z","receivedAt":"2006-05-21T08:24:37Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Jeff King wrote:\n\nHmmm... isn't the patch slightly against git coding style?\n> +     if(unpacked)\n[...]\n>+                for(i = 0; i < len; i++)\n\ncompare to:\n>          for (ref = refs; ref; ref = ref->next) {\n\n-- \nJakub Narebski\nWarsaw, Poland\n"},{"id":"20383","messageId":"7vlksvzsmd.fsf@assigned-by-dhcp.cox.net","threadId":"4229","inReplyTo":"20060521081435.GA4526@coredump.intra.peff.net","subject":"Re: [RFC] send-pack: allow skipping delta when sending pack","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-05-21T08:31:22Z","receivedAt":"2006-05-21T08:31:22Z","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> On Sat, May 20, 2006 at 11:17:42PM -0700, Junio C Hamano wrote:\n>\n>> base delta for that object to skip computation\".  What you want\n>> here is \"if the object we are going to send is not a delta in\n>> the source, and there are sufficient number of other objects the\n>> object could have been deltified against, then it is very likely\n>> that it was not worth deltifying when it was packed; so it is\n>> probably not worth deltifying it now\".\n>\n> I think we can make a stronger statement in many cases: \"if the object\n> we are going to send is not a delta in the source, and there are no\n> other objects it could be deltified against, then it is not worth\n> deltifying.\" That is, in the case that we just packed we KNOW that it's\n> not worth it, since we're not sending anything that isn't already\n> packed.\n\nCareful.\n\nWe do not delta an otherwise perfectly deltifiable object if its\ndelta base happens to be at the depth edge in the original pack.\nSo no, we do _NOT_ know if it is not worth it merely from the\nfact that it is not deltified in the existing pack.  And the\nlatter part of your test \"there are no other objects it could be\ndeltified against\" is either expensive (you have to try first to\nsee if that is the case to really see it) or stupid (you just\nassume there is no suitable delta base without looking at other\nobjects like we currently do).\n"},{"id":"20384","messageId":"20060521084313.GA12825@coredump.intra.peff.net","threadId":"4229","inReplyTo":"7vlksvzsmd.fsf@assigned-by-dhcp.cox.net","subject":"Re: [RFC] send-pack: allow skipping delta when sending pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2006-05-21T08:43:13Z","receivedAt":"2006-05-21T08:43:13Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 21, 2006 at 01:31:22AM -0700, Junio C Hamano wrote:\n\n> We do not delta an otherwise perfectly deltifiable object if its\n> delta base happens to be at the depth edge in the original pack.\n> So no, we do _NOT_ know if it is not worth it merely from the\n> fact that it is not deltified in the existing pack.  And the\n\nYes, you're right. What do you suggest for this performance issue, then?\nA manual no-delta trigger, or just going to get a cup of coffee while\npushing (my tests showed 5-6x slowdown from deltifying)?\n\n-Peff\n"},{"id":"20385","messageId":"20060521084403.GB12825@coredump.intra.peff.net","threadId":"4229","inReplyTo":"e4p83e$uqt$1@sea.gmane.org","subject":"Re: [RFC] send-pack: allow skipping delta when sending pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2006-05-21T08:44:03Z","receivedAt":"2006-05-21T08:44:03Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 21, 2006 at 10:24:37AM +0200, Jakub Narebski wrote:\n\n> Hmmm... isn't the patch slightly against git coding style?\n\nOops, yes (though the point is moot since the patch is conceptually\nwrong). Is there a git coding style document somewhere?\n\n-Peff\n"},{"id":"20387","messageId":"7v4pzjzqmf.fsf@assigned-by-dhcp.cox.net","threadId":"4229","inReplyTo":"20060521084403.GB12825@coredump.intra.peff.net","subject":"Re: [RFC] send-pack: allow skipping delta when sending pack","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-05-21T09:14:32Z","receivedAt":"2006-05-21T09:14:32Z","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> On Sun, May 21, 2006 at 10:24:37AM +0200, Jakub Narebski wrote:\n>\n>> Hmmm... isn't the patch slightly against git coding style?\n>\n> Oops, yes (though the point is moot since the patch is conceptually\n> wrong). Is there a git coding style document somewhere?\n\nWhen in doubt, please follow the kernel coding style ;-).\n"},{"id":"20388","messageId":"e4pbk3$6au$1@sea.gmane.org","threadId":"4229","inReplyTo":"20060521084403.GB12825@coredump.intra.peff.net","subject":"Re: [RFC] send-pack: allow skipping delta when sending pack","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-05-21T09:24:42Z","receivedAt":"2006-05-21T09:24:42Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Jeff King wrote:\n\n> On Sun, May 21, 2006 at 10:24:37AM +0200, Jakub Narebski wrote:\n> \n>> Hmmm... isn't the patch slightly against git coding style?\n> \n> Oops, yes (though the point is moot since the patch is conceptually\n> wrong). Is there a git coding style document somewhere?\n\nNot that I know of. It's better to follow the coding style used\nin the rest of files.\n\nBesides, differentiating between\n\n   if (condition)\n\nand\n\n   function(argument);\n\nmakes IMVHO sense.\n\n-- \nJakub Narebski\nWarsaw, Poland\n"},{"id":"20389","messageId":"7vpsi7yau9.fsf@assigned-by-dhcp.cox.net","threadId":"4229","inReplyTo":"20060521084313.GA12825@coredump.intra.peff.net","subject":"Re: [RFC] send-pack: allow skipping delta when sending pack","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-05-21T09:40:46Z","receivedAt":"2006-05-21T09:40:46Z","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> What do you suggest for this performance issue, then?\n> A manual no-delta trigger, or just going to get a cup of coffee while\n> pushing (my tests showed 5-6x slowdown from deltifying)?\n\nIn the short term, give --depth to send-pack, would be better\nthan nothing.\n\nI agree we should be able to do better.  But let me grab some\nsleep now first ;-).\n"}]}