{"thread":{"id":"39098","subject":"[BUG] Performance regression due to #33d4221: write_sha1_file: freshen existing objects","startedAt":"2015-04-17T07:30:22Z","lastAt":"2015-04-22T22:00:06Z","messageCount":19,"participants":["Stefan Saasen","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"259556","messageId":"CADoxLGPYOkgzb4bkdHq5tK0aJS2M=nWGzO=YYXPDcy-gh45q-g@mail.gmail.com","threadId":"39098","inReplyTo":null,"subject":"[BUG] Performance regression due to #33d4221: write_sha1_file: freshen existing objects","fromName":"Stefan Saasen","fromEmail":"ssaasen@atlassian.com","sentAt":"2015-04-17T07:30:22Z","receivedAt":"2015-04-17T07:30:22Z","isPatch":false,"sender":{"key":"ssaasen@atlassian.com","avatar":null},"body":"We became aware of slow merge times with the following setup:\n\nThe merge is created in a temporary location that uses alternates. The\ntemporary repository is on a local disk, the alternate object database\non an NFS mount.\n\nAfter some investigation we believe that #33d4221 (present in git\n2.2.0, absent in 2.1.4) is causing this regression in merge time.\n\nThe following are merge times (in seconds) with git@33d4221~\n(2.1.2-393-gabcb865) (before the change)\n\n Elapsed System User\n Min. :0.3700 Min. :0.04000 Min. :0.3000\n 1st Qu.:0.3800 1st Qu.:0.05000 1st Qu.:0.3100\n Median :0.4000 Median :0.06000 Median :0.3300\n Mean :0.4295 Mean :0.05905 Mean :0.3519\n 3rd Qu.:0.4600 3rd Qu.:0.07000 3rd Qu.:0.3600\n Max. :0.5900 Max. :0.09000 Max. :0.4900\n\n\nThe following are merge times with git@33d4221 (2.1.2-394-g33d4221):\n\n Elapsed System User\n Min. : 8.58 Min. :1.46 Min. :0.4400\n 1st Qu.: 9.63 1st Qu.:1.60 1st Qu.:0.4400\n Median :10.64 Median :1.66 Median :0.4800\n Mean :10.50 Mean :1.69 Mean :0.4986\n 3rd Qu.:11.13 3rd Qu.:1.81 3rd Qu.:0.5000\n Max. :13.96 Max. :2.05 Max. :0.6700\n\n\nAs you can see the merge times are an order of magnitude slower after\nthe change.\n\nThe effect of #33d4221 can be seen when strace'ing the merge:\n\nRunning strace on git@33d4221 yields\n% time seconds usecs/call calls errors syscall\n------ ----------- ----------- --------- --------- ----------------\n 70.79 0.714852 178 4018 utime\n 14.73 0.148789 3 50141 50123 lstat\n 13.88 0.140198 17 8074 8067 access\n 0.24 0.002455 614 4 rename\n 0.15 0.001493 3 577 write\n 0.06 0.000618 10 65 close\n 0.04 0.000453 3 152 brk\n 0.04 0.000433 27 16 mkdir\n 0.03 0.000310 8 41 fstat\n\n\nRunning strace on git@33d4221~ yields\n\n% time seconds usecs/call calls errors syscall\n------ ----------- ----------- --------- --------- ----------------\n 98.37 0.138516 3 50141 50123 lstat\n 0.92 0.001292 2 577 write\n 0.37 0.000520 14 38 31 access\n 0.18 0.000252 36 7 getcwd\n 0.17 0.000237 7 36 20 stat\n 0.00 0.000000 0 40 read\n 0.00 0.000000 0 87 30 open\n\n\nMy current hypothesis is that the additional `access`, but more\nimportantly the additional `utime` calls are responsible in the\nincreased merge times that we see.\nNFS stats on the server for the tests seem to confirm this (see\nnfsstat-{after,before}-change.txt on\nhttps://bitbucket.org/snippets/ssaasen/oend).\nThis is certainly due to the fact that this will all happen over NFS\nbut in 2.1.4 this worked fine and starting with 2.2 this has become\nvery slow.\n\nLooking at the detailed strace shows that utime will be called\nrepeatedly in same cases (e.g.\nhttps://bitbucket.org/snippets/ssaasen/oend shows an example where the\nsame packfile will be updated more than 4000 times in a single merge).\n\nhttp://www.spinics.net/lists/git/msg240106.html discusses a potential\nimprovement for this case. Would that be an acceptable avenue to\nimprove this situation?\n\nBest regards,\nStefan Saasen\n"},{"id":"259562","messageId":"20150417140315.GA13506@peff.net","threadId":"39098","inReplyTo":"CADoxLGPYOkgzb4bkdHq5tK0aJS2M=nWGzO=YYXPDcy-gh45q-g@mail.gmail.com","subject":"Re: [BUG] Performance regression due to #33d4221: write_sha1_file: freshen existing objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-17T14:03:15Z","receivedAt":"2015-04-17T14:03:15Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 17, 2015 at 05:30:22PM +1000, Stefan Saasen wrote:\n\n> The merge is created in a temporary location that uses alternates. The\n> temporary repository is on a local disk, the alternate object database\n> on an NFS mount.\n\nIs the alternate writeable? If we can't freshen the object, we fall back\nto storing the object locally, which could have a performance impact.\nBut it looks from your tables below like the utime() call is succeeding,\nso that is probably not what is happening here.\n\n> My current hypothesis is that the additional `access`, but more\n> importantly the additional `utime` calls are responsible in the\n> increased merge times that we see.\n\nYeah, that makes sense from your tables. The commit in question flips\nthe order of the loose/packed check, and the packed check should be much\nfaster on your NFS mount. Can you try:\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 88f06ba..822aaef 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -3014,7 +3014,7 @@ int write_sha1_file(const void *buf, unsigned long len, const char *type, unsign\n \twrite_sha1_file_prepare(buf, len, type, sha1, hdr, &hdrlen);\n \tif (returnsha1)\n \t\thashcpy(returnsha1, sha1);\n-\tif (freshen_loose_object(sha1) || freshen_packed_object(sha1))\n+\tif (freshen_packed_object(sha1) || freshen_loose_object(sha1))\n \t\treturn 0;\n \treturn write_loose_object(sha1, hdr, hdrlen, buf, len, 0);\n }\n\nI think that should clear up the access() calls, but leave the utime()\nones.\n\n> Looking at the detailed strace shows that utime will be called\n> repeatedly in same cases (e.g.\n> https://bitbucket.org/snippets/ssaasen/oend shows an example where the\n> same packfile will be updated more than 4000 times in a single merge).\n> \n> http://www.spinics.net/lists/git/msg240106.html discusses a potential\n> improvement for this case. Would that be an acceptable avenue to\n> improve this situation?\n\nI think so. Here's a tentative patch:\n\ndiff --git a/cache.h b/cache.h\nindex 3d3244b..72c6888 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1174,6 +1174,7 @@ extern struct packed_git {\n \tint pack_fd;\n \tunsigned pack_local:1,\n \t\t pack_keep:1,\n+\t\t freshened:1,\n \t\t do_not_close:1;\n \tunsigned char sha1[20];\n \t/* something like \".git/objects/pack/xxxxx.pack\" */\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 822aaef..f27cbf1 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2999,7 +2999,11 @@ static int freshen_loose_object(const unsigned char *sha1)\n static int freshen_packed_object(const unsigned char *sha1)\n {\n \tstruct pack_entry e;\n-\treturn find_pack_entry(sha1, &e) && freshen_file(e.p->pack_name);\n+\tif (!find_pack_entry(sha1, &e))\n+\t\treturn 0;\n+\tif (e.p->freshened)\n+\t\treturn 1;\n+\treturn freshen_file(e.p->pack_name);\n }\n \n int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *returnsha1)\n\n\nIf it's not a problem, I'd love to see timings for your case with just\nthe first patch, and then with both.\n\nYou may also be interested in:\n\n http://thread.gmane.org/gmane.comp.version-control.git/266370\n\nwhich addresses another performance problem related to the\nfreshen/recent code in v2.2.\n\n-Peff\n"},{"id":"259571","messageId":"xmqqlhhqhhy4.fsf@gitster.dls.corp.google.com","threadId":"39098","inReplyTo":"20150417140315.GA13506@peff.net","subject":"Re: [BUG] Performance regression due to #33d4221: write_sha1_file: freshen existing objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-17T15:50:59Z","receivedAt":"2015-04-17T15:50:59Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> If it's not a problem, I'd love to see timings for your case with just\n> the first patch, and then with both.\n\nThanks for two quick progress patches.\n\n> You may also be interested in:\n>\n> http://thread.gmane.org/gmane.comp.version-control.git/266370\n>\n> which addresses another performance problem related to the\n> freshen/recent code in v2.2.\n\nThat, too.\n"},{"id":"259612","messageId":"CADoxLGOPXDgb0LBcSBm+xRDhbnGV_y-TXENyPV7oK_+KZzPKRQ@mail.gmail.com","threadId":"39098","inReplyTo":"20150417140315.GA13506@peff.net","subject":"Re: [BUG] Performance regression due to #33d4221: write_sha1_file: freshen existing objects","fromName":"Stefan Saasen","fromEmail":"ssaasen@atlassian.com","sentAt":"2015-04-18T03:35:51Z","receivedAt":"2015-04-18T03:35:51Z","isPatch":false,"sender":{"key":"ssaasen@atlassian.com","avatar":null},"body":"> If it's not a problem, I'd love to see timings for your case with just\n> the first patch, and then with both.\n\nThanks for the swift response, much appreciated Jeff!\n\nHere are the timings for the two patches:\n\nPatch 1 on top of 33d4221c79\n\n Elapsed System User\n Min. :6.110 Min. :0.6700 Min. :0.3600\n 1st Qu.:6.580 1st Qu.:0.6900 1st Qu.:0.3900\n Median :7.260 Median :0.7100 Median :0.4100\n Mean :7.347 Mean :0.7248 Mean :0.4214\n 3rd Qu.:8.000 3rd Qu.:0.7400 3rd Qu.:0.4600\n Max. :8.860 Max. :0.8700 Max. :0.5100\n\nI've had to slightly tweak your second patch (`freshened` was never\nset) but applying the modified patch yielded even better results\ncompared to patch 1:\n\n Elapsed System User\n Min. :0.38 Min. :0.03000 Min. :0.2900\n 1st Qu.:0.38 1st Qu.:0.04000 1st Qu.:0.3100\n Median :0.39 Median :0.06000 Median :0.3200\n Mean :0.43 Mean :0.05667 Mean :0.3519\n 3rd Qu.:0.42 3rd Qu.:0.07000 3rd Qu.:0.3600\n Max. :0.68 Max. :0.08000 Max. :0.5700\n\nThis is pretty much back to the \"before\" state.\nThe graph really tells the whole story:\nhttps://bytebucket.org/snippets/ssaasen/GeRE/raw/7367353a58c50ccd7c493af40ffb6ba1533e1490/git-merge-timing-patched.png\n(After is the change in #33d4221, Before the parent of #33d4221 and so on)\nThe graph and the NFS stats can be found here:\nhttps://bitbucket.org/snippets/ssaasen/GeRE\n\nMy tweaked version of your second patch is:\n\ndiff --git a/cache.h b/cache.h\nindex 51ee856..8982055 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1168,6 +1168,7 @@ extern struct packed_git {\n int pack_fd;\n unsigned pack_local:1,\n pack_keep:1,\n+ freshened:1,\n do_not_close:1;\n unsigned char sha1[20];\n /* something like \".git/objects/pack/xxxxx.pack\" */\ndiff --git a/sha1_file.c b/sha1_file.c\nindex bc6322e..c0ccd4b 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2999,7 +2999,11 @@ static int freshen_loose_object(const unsigned\nchar *sha1)\n static int freshen_packed_object(const unsigned char *sha1)\n {\n struct pack_entry e;\n- return find_pack_entry(sha1, &e) && freshen_file(e.p->pack_name);\n+ if (!find_pack_entry(sha1, &e))\n+ return 0;\n+ if (e.p->freshened)\n+ return 1;\n+ return e.p->freshened = freshen_file(e.p->pack_name);\n }\n\n int write_sha1_file(const void *buf, unsigned long len, const char\n*type, unsigned char *returnsha1)\n\n\n\nThe only change is that I assign the result of `freshen_file` to the\n`freshened` flag.\n\nI've only ran this with the test case I was using before but it looks\nlike this is pretty much fixing the merge time changes we observed.\n\nThanks again for the swift response. I've got my test setup sitting\nhere, happy to rerun the tests if that'd be useful.\n\nIs there a chance to backport those changes to the 2.2+ branches?\n\n> You may also be interested in:\n>\n> http://thread.gmane.org/gmane.comp.version-control.git/266370\n>\n> which addresses another performance problem related to the\n> freshen/recent code in v2.2.\n\nThanks for the pointer, I'll have a look at that as well.\n\nCheers,\nStefan\n"},{"id":"259700","messageId":"20150420195337.GA15447@peff.net","threadId":"39098","inReplyTo":"CADoxLGOPXDgb0LBcSBm+xRDhbnGV_y-TXENyPV7oK_+KZzPKRQ@mail.gmail.com","subject":"Re: [BUG] Performance regression due to #33d4221: write_sha1_file: freshen existing objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-20T19:53:38Z","receivedAt":"2015-04-20T19:53:38Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 18, 2015 at 01:35:51PM +1000, Stefan Saasen wrote:\n\n> Here are the timings for the two patches:\n> [...]\n\nThanks, that matches what I was hoping for.\n\n> My tweaked version of your second patch is:\n> [...]\n> - return find_pack_entry(sha1, &e) && freshen_file(e.p->pack_name);\n> + if (!find_pack_entry(sha1, &e))\n> + return 0;\n> + if (e.p->freshened)\n> + return 1;\n> + return e.p->freshened = freshen_file(e.p->pack_name);\n> }\n\nWhooops, yeah, setting the flag is probably helpful. :)\n\nWe usually try to avoid assignments in a return like this, so I've\nwritten it out a little more verbosely in my final version. I'll send\nthose patches in a moment.\n\n [1/2]: sha1_file: freshen pack objects before loose\n [2/2]: sha1_file: only freshen packs once per run\n\n> Is there a chance to backport those changes to the 2.2+ branches?\n\nThat's up to Junio. These patches can be applied straight to the\njk/prune-mtime topic. Usually he would then merge the topic up to\n\"maint\", which at this would potentially become the next v2.3.x. If an\nissue is critical (e.g., a security vulnerability), he'll sometimes\nmerge and roll maintenance releases for older versions. But I don't know\nif this counts as critical (it is for you, certainly, but I don't think\nthat many people are affected, as the crucial factor here is really the\nslow NFS filesystem operations).\n\n-Peff\n"},{"id":"259701","messageId":"20150420195403.GA15760@peff.net","threadId":"39098","inReplyTo":"20150420195337.GA15447@peff.net","subject":"[PATCH 1/2] sha1_file: freshen pack objects before loose","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-20T19:54:03Z","receivedAt":"2015-04-20T19:54:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When writing out an object file, we first check whether it\nalready exists and if so optimize out the write. Prior to\n33d4221, we did this by calling has_sha1_file(), which will\ncheck for packed objects followed by loose. Since that\ncommit, we check loose objects first.\n\nFor the common case of a repository whose objects are mostly\npacked, this means we will make a lot of extra access()\nsystem calls checking for loose objects. We should follow\nthe same packed-then-loose order that all of our other\nlookups use.\n\nReported-by: Stefan Saasen \nSigned-off-by: Jeff King \n---\n sha1_file.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 88f06ba..822aaef 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -3014,7 +3014,7 @@ int write_sha1_file(const void *buf, unsigned long len, const char *type, unsign\n \twrite_sha1_file_prepare(buf, len, type, sha1, hdr, &hdrlen);\n \tif (returnsha1)\n \t\thashcpy(returnsha1, sha1);\n-\tif (freshen_loose_object(sha1) || freshen_packed_object(sha1))\n+\tif (freshen_packed_object(sha1) || freshen_loose_object(sha1))\n \t\treturn 0;\n \treturn write_loose_object(sha1, hdr, hdrlen, buf, len, 0);\n }\n-- \n2.4.0.rc2.384.g7297a4a\n"},{"id":"259702","messageId":"20150420195500.GB15760@peff.net","threadId":"39098","inReplyTo":"20150420195337.GA15447@peff.net","subject":"[PATCH 2/2] sha1_file: only freshen packs once per run","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-20T19:55:00Z","receivedAt":"2015-04-20T19:55:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since 33d4221 (write_sha1_file: freshen existing objects,\n2014-10-15), we update the mtime of existing objects that we\nwould have written out (had they not existed). For the\ncommon case in which many objects are packed, we may update\nthe mtime on a single packfile repeatedly. This can result\nin a noticeable performance problem if calling utime() is\nexpensive (e.g., because your storage is on NFS).\n\nWe can fix this by keeping a per-pack flag that lets us\nfreshen only once per program invocation.\n\nAn alternative would be to keep the packed_git.mtime flag up\nto date as we freshen, and freshen only once every N\nseconds. In practice, it's not worth the complexity. We are\nracing against prune expiration times here, which inherently\nmust be set to accomodate reasonable program running times\n(because they really care about the time between an object\nbeing written and it becoming referenced, and the latter is\ntypically the last step a program takes).\n\nSigned-off-by: Jeff King \n---\nHopefully I didn't botch the flag logic again. :) I tested with \"strace\n-c\" myself this time, so I think it is good.\n\n cache.h | 1 +\n sha1_file.c | 9 ++++++++-\n 2 files changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/cache.h b/cache.h\nindex 3d3244b..72c6888 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1174,6 +1174,7 @@ extern struct packed_git {\n \tint pack_fd;\n \tunsigned pack_local:1,\n \t\t pack_keep:1,\n+\t\t freshened:1,\n \t\t do_not_close:1;\n \tunsigned char sha1[20];\n \t/* something like \".git/objects/pack/xxxxx.pack\" */\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 822aaef..26b9b2b 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2999,7 +2999,14 @@ static int freshen_loose_object(const unsigned char *sha1)\n static int freshen_packed_object(const unsigned char *sha1)\n {\n \tstruct pack_entry e;\n-\treturn find_pack_entry(sha1, &e) && freshen_file(e.p->pack_name);\n+\tif (!find_pack_entry(sha1, &e))\n+\t\treturn 0;\n+\tif (e.p->freshened)\n+\t\treturn 1;\n+\tif (!freshen_file(e.p->pack_name))\n+\t\treturn 0;\n+\te.p->freshened = 1;\n+\treturn 1;\n }\n \n int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *returnsha1)\n-- \n2.4.0.rc2.384.g7297a4a\n"},{"id":"259703","messageId":"xmqq1tjelg78.fsf@gitster.dls.corp.google.com","threadId":"39098","inReplyTo":"20150420195337.GA15447@peff.net","subject":"Re: [BUG] Performance regression due to #33d4221: write_sha1_file: freshen existing objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-20T20:04:11Z","receivedAt":"2015-04-20T20:04:11Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ... But I don't know\n> if this counts as critical (it is for you, certainly, but I don't think\n> that many people are affected, as the crucial factor here is really the\n> slow NFS filesystem operations).\n\nIf it is critical to some people, they can downmerge to their custom\nold installations of Git they maintain with ease, of course, and\nthat \"with ease\" part is the reason why I try to apply fixes to tip\nof the original topic branch even though they were merged to the\nmainline eons ago ;-).\n\nThanks. The patches look good from cursory reading.\n"},{"id":"259704","messageId":"20150420200956.GA16249@peff.net","threadId":"39098","inReplyTo":"xmqq1tjelg78.fsf@gitster.dls.corp.google.com","subject":"Re: [BUG] Performance regression due to #33d4221: write_sha1_file: freshen existing objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-20T20:09:56Z","receivedAt":"2015-04-20T20:09:56Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 20, 2015 at 01:04:11PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > ... But I don't know\n> > if this counts as critical (it is for you, certainly, but I don't think\n> > that many people are affected, as the crucial factor here is really the\n> > slow NFS filesystem operations).\n> \n> If it is critical to some people, they can downmerge to their custom\n> old installations of Git they maintain with ease, of course, and\n> that \"with ease\" part is the reason why I try to apply fixes to tip\n> of the original topic branch even though they were merged to the\n> mainline eons ago ;-).\n\nI think it is a bigger deal for folks who do not ship a custom\ninstallation, but expect to ship a third-party system that interacts\nwith whatever version of git their customers happen to have (in which\ncase they can only recommend their customers to upgrade).\n\nI don't know how Stash or GitLab installations work. GitHub ships our\nown custom git (which I maintain), though we are already on 2.3.x.\n\nEither way, though, I do not think it is the upstream Git project's\nproblem.\n\n-Peff\n"},{"id":"259705","messageId":"xmqqwq16k189.fsf@gitster.dls.corp.google.com","threadId":"39098","inReplyTo":"20150420200956.GA16249@peff.net","subject":"Re: [BUG] Performance regression due to #33d4221: write_sha1_file: freshen existing objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-20T20:12:54Z","receivedAt":"2015-04-20T20:12:54Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Either way, though, I do not think it is the upstream Git project's\n> problem.\n\nThe commit to pick where to queue the fixes actually is my problem,\nas I have this illusion that I'd be helping these derived works by\nmaking it easier for them to merge, not cherry-pick.\n\nBut I would imagine that they may go the cherry-pick route anyway,\nin which case I may be wasting my time worrying about them X-<.\n"},{"id":"259706","messageId":"20150420202822.GA16367@peff.net","threadId":"39098","inReplyTo":"xmqqwq16k189.fsf@gitster.dls.corp.google.com","subject":"Re: [BUG] Performance regression due to #33d4221: write_sha1_file: freshen existing objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-20T20:28:23Z","receivedAt":"2015-04-20T20:28:23Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 20, 2015 at 01:12:54PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Either way, though, I do not think it is the upstream Git project's\n> > problem.\n> \n> The commit to pick where to queue the fixes actually is my problem,\n> as I have this illusion that I'd be helping these derived works by\n> making it easier for them to merge, not cherry-pick.\n\nTrue, I had just meant the actual rolling of the releases.\n\n> But I would imagine that they may go the cherry-pick route anyway,\n> in which case I may be wasting my time worrying about them X-<.\n\nFWIW, I typically cherry-pick rather than merge. The resulting history\nis not as nice, but it means I don't have to think as hard about the\nhistory when doing so. It also means that topics may not be as well\ntested (e.g., they may have been implicitly relying on some other thing\nthat happened upstream that I did _not_ cherry-pick). But we treat even\ncherry-picked upstream topics as their own feature branches, and do our\nnormal internal testing and review.\n\n-Peff\n"},{"id":"259722","messageId":"CADoxLGMrWXn+3qsGr2dYaDMmzLTKDW6wDthH6z1a_whOch9qPw@mail.gmail.com","threadId":"39098","inReplyTo":"20150420195500.GB15760@peff.net","subject":"Re: [PATCH 2/2] sha1_file: only freshen packs once per run","fromName":"Stefan Saasen","fromEmail":"ssaasen@atlassian.com","sentAt":"2015-04-21T00:45:01Z","receivedAt":"2015-04-21T00:45:01Z","isPatch":true,"sender":{"key":"ssaasen@atlassian.com","avatar":null},"body":"I can confirm that this patch is equivalent to the previous one.\n\nhttps://bitbucket.org/snippets/ssaasen/9AXg shows both the timing and\nthe NFS stats showing the effect of applying this patch.\n\nThanks for the fix Jeff!\n\nCheers,\nStefan\n\nOn 21 April 2015 at 05:55, Jeff King wrote:\n> Since 33d4221 (write_sha1_file: freshen existing objects,\n> 2014-10-15), we update the mtime of existing objects that we\n> would have written out (had they not existed). For the\n> common case in which many objects are packed, we may update\n> the mtime on a single packfile repeatedly. This can result\n> in a noticeable performance problem if calling utime() is\n> expensive (e.g., because your storage is on NFS).\n>\n> We can fix this by keeping a per-pack flag that lets us\n> freshen only once per program invocation.\n>\n> An alternative would be to keep the packed_git.mtime flag up\n> to date as we freshen, and freshen only once every N\n> seconds. In practice, it's not worth the complexity. We are\n> racing against prune expiration times here, which inherently\n> must be set to accomodate reasonable program running times\n> (because they really care about the time between an object\n> being written and it becoming referenced, and the latter is\n> typically the last step a program takes).\n>\n> Signed-off-by: Jeff King \n> ---\n> Hopefully I didn't botch the flag logic again. :) I tested with \"strace\n> -c\" myself this time, so I think it is good.\n>\n> cache.h | 1 +\n> sha1_file.c | 9 ++++++++-\n> 2 files changed, 9 insertions(+), 1 deletion(-)\n>\n> diff --git a/cache.h b/cache.h\n> index 3d3244b..72c6888 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1174,6 +1174,7 @@ extern struct packed_git {\n> int pack_fd;\n> unsigned pack_local:1,\n> pack_keep:1,\n> + freshened:1,\n> do_not_close:1;\n> unsigned char sha1[20];\n> /* something like \".git/objects/pack/xxxxx.pack\" */\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 822aaef..26b9b2b 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -2999,7 +2999,14 @@ static int freshen_loose_object(const unsigned char *sha1)\n> static int freshen_packed_object(const unsigned char *sha1)\n> {\n> struct pack_entry e;\n> - return find_pack_entry(sha1, &e) && freshen_file(e.p->pack_name);\n> + if (!find_pack_entry(sha1, &e))\n> + return 0;\n> + if (e.p->freshened)\n> + return 1;\n> + if (!freshen_file(e.p->pack_name))\n> + return 0;\n> + e.p->freshened = 1;\n> + return 1;\n> }\n>\n> int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *returnsha1)\n> --\n> 2.4.0.rc2.384.g7297a4a\n"},{"id":"259723","messageId":"CADoxLGPNEjDWBjsYn30acapCUj6TfMw5z34W6f_9OjhXySFMLQ@mail.gmail.com","threadId":"39098","inReplyTo":"20150420195403.GA15760@peff.net","subject":"Re: [PATCH 1/2] sha1_file: freshen pack objects before loose","fromName":"Stefan Saasen","fromEmail":"ssaasen@atlassian.com","sentAt":"2015-04-21T00:46:25Z","receivedAt":"2015-04-21T00:46:25Z","isPatch":true,"sender":{"key":"ssaasen@atlassian.com","avatar":null},"body":"I didn't expect anything else (as the patch is the same as the\nprevious one) but I verified that applying this patch has the desired\neffect (https://bitbucket.org/snippets/ssaasen/9AXg).\n\nThanks for the fix Jeff.\n\nOn 21 April 2015 at 05:54, Jeff King wrote:\n> When writing out an object file, we first check whether it\n> already exists and if so optimize out the write. Prior to\n> 33d4221, we did this by calling has_sha1_file(), which will\n> check for packed objects followed by loose. Since that\n> commit, we check loose objects first.\n>\n> For the common case of a repository whose objects are mostly\n> packed, this means we will make a lot of extra access()\n> system calls checking for loose objects. We should follow\n> the same packed-then-loose order that all of our other\n> lookups use.\n>\n> Reported-by: Stefan Saasen \n> Signed-off-by: Jeff King \n> ---\n> sha1_file.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 88f06ba..822aaef 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -3014,7 +3014,7 @@ int write_sha1_file(const void *buf, unsigned long len, const char *type, unsign\n> write_sha1_file_prepare(buf, len, type, sha1, hdr, &hdrlen);\n> if (returnsha1)\n> hashcpy(returnsha1, sha1);\n> - if (freshen_loose_object(sha1) || freshen_packed_object(sha1))\n> + if (freshen_packed_object(sha1) || freshen_loose_object(sha1))\n> return 0;\n> return write_loose_object(sha1, hdr, hdrlen, buf, len, 0);\n> }\n> --\n> 2.4.0.rc2.384.g7297a4a\n>\n"},{"id":"259724","messageId":"CADoxLGOdvJVgjRFrC81nM6A4=PRABSiL_EGOUtN7d-MAKXrzzg@mail.gmail.com","threadId":"39098","inReplyTo":"20150420200956.GA16249@peff.net","subject":"Re: [BUG] Performance regression due to #33d4221: write_sha1_file: freshen existing objects","fromName":"Stefan Saasen","fromEmail":"ssaasen@atlassian.com","sentAt":"2015-04-21T01:49:33Z","receivedAt":"2015-04-21T01:49:33Z","isPatch":false,"sender":{"key":"ssaasen@atlassian.com","avatar":null},"body":">> If it is critical to some people, they can downmerge to their custom\n>> old installations of Git they maintain with ease, of course, and\n>> that \"with ease\" part is the reason why I try to apply fixes to tip\n>> of the original topic branch even though they were merged to the\n>> mainline eons ago ;-).\n>\n> I think it is a bigger deal for folks who do not ship a custom\n> installation, but expect to ship a third-party system that interacts\n> with whatever version of git their customers happen to have (in which\n> case they can only recommend their customers to upgrade).\n\nYes, this is the situation we are facing. We allow our customers to\nuse the git version that is supported/available on their OS (within a\ncertain range of supported versions) so our customers usually don't\ncompile from source.\n\n> Either way, though, I do not think it is the upstream Git project's\n> problem.\n\nThat's fair enough, I was mostly enquiring about the official git\nversions this will land in so that we can advise customers what git\nversion to use (or not to use).\n\nI've noticed Peff's patches on pu which suggest they will be available\nin git 2.5?\nDo you Junio, have plans to merge them to maint (2.3.x) and/or next (2.4)?\n\nWhile I certainly agree that this is specific to Git on NFS and not a\nmore widespread git performance problem, I'd love to be able to\nmessage something other than \"skip all the git version between and\nincluding git 2.2 - 2.4\".\n\nI appreciate your consideration and thanks again for the swift response on this.\n\nCheers,\nStefan\n"},{"id":"259740","messageId":"xmqqiocpif8p.fsf@gitster.dls.corp.google.com","threadId":"39098","inReplyTo":"CADoxLGOdvJVgjRFrC81nM6A4=PRABSiL_EGOUtN7d-MAKXrzzg@mail.gmail.com","subject":"Re: [BUG] Performance regression due to #33d4221: write_sha1_file: freshen existing objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-21T17:05:26Z","receivedAt":"2015-04-21T17:05:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Saasen writes:\n\n> I've noticed Peff's patches on pu which suggest they will be available\n> in git 2.5?\n\nBeing on 'pu' (or 'next' for that matter) is not a suggestion for a\nchange to appear in any future version at all, even though it often\nmeans that it would soon be merged to 'master' and will be in the\nupcoming release to be on 'next' in early part of a development\ncycle. Some larger topics would stay on 'next' for a few cycles.\n\n> Do you Junio, have plans to merge them to maint (2.3.x) and/or next (2.4)?\n\nThe topic will hopefully be merged to 'master' after 2.4 final is\nreleased end of this month, down to 'maint' early May and will ship\nwith 2.4.1, unless there is unforeseen issues discovered in the\nchange while people try it out while it is in 'next' (which will\nhappen today, hopefully).\n"},{"id":"259765","messageId":"xmqq4mo9gnq3.fsf@gitster.dls.corp.google.com","threadId":"39098","inReplyTo":"xmqqiocpif8p.fsf@gitster.dls.corp.google.com","subject":"Re: [BUG] Performance regression due to #33d4221: write_sha1_file: freshen existing objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-21T21:45:08Z","receivedAt":"2015-04-21T21:45:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Stefan Saasen writes:\n>\n>> I've noticed Peff's patches on pu which suggest they will be available\n>> in git 2.5?\n>\n> Being on 'pu' (or 'next' for that matter) is not a suggestion for a\n> change to appear in any future version at all, even though it often\n> means that it would soon be merged to 'master' and will be in the\n> upcoming release to be on 'next' in early part of a development\n> cycle. Some larger topics would stay on 'next' for a few cycles.\n>\n>> Do you Junio, have plans to merge them to maint (2.3.x) and/or next (2.4)?\n>\n> The topic will hopefully be merged to 'master' after 2.4 final is\n> released end of this month, down to 'maint' early May and will ship\n> with 2.4.1, unless there is unforeseen issues discovered in the\n> change while people try it out while it is in 'next' (which will\n> happen today, hopefully).\n\n... and then if I do not forget and if the topic is really important\nfor real-world users, I am OK to merge it down to 2.3 and even 2.2\nmaintenance tracks later. But that will happen only after the topic\nhits 'maint', which will happen only after the topic hits 'master'.\n\nWhat you _can_ help is the \"if I do not forget\" part ;-) Also see a\nsimilar discussion we had recently\n\n http://thread.gmane.org/gmane.comp.version-control.git/264365\n\nThe key sentence from my part in the thread is \n\n> When I say \"the tip of 'master' is meant to be more stable than\n> any tagged versions\", I do mean it.\n\nand the reasoning behind it that is given in the paragraph before\nthat, though.\n\nPerhaps companies like Atlassian that rely on the stability of the\nopen source Git can spare some resources and join forces with like\nminded folks on LTS of older maintenance tracks, if they are truly\ninterested in.\n"},{"id":"259781","messageId":"CADoxLGNgWy7o4Kz2Zw-H=k9Dcrmff3_hd1pu1yrotZjKNqFENQ@mail.gmail.com","threadId":"39098","inReplyTo":"xmqqiocpif8p.fsf@gitster.dls.corp.google.com","subject":"Re: [BUG] Performance regression due to #33d4221: write_sha1_file: freshen existing objects","fromName":"Stefan Saasen","fromEmail":"ssaasen@atlassian.com","sentAt":"2015-04-22T00:04:12Z","receivedAt":"2015-04-22T00:04:12Z","isPatch":false,"sender":{"key":"ssaasen@atlassian.com","avatar":null},"body":">> I've noticed Peff's patches on pu which suggest they will be available\n>> in git 2.5?\n>\n> Being on 'pu' (or 'next' for that matter) is not a suggestion for a\n> change to appear in any future version at all, even though it often\n> means that it would soon be merged to 'master' and will be in the\n> upcoming release to be on 'next' in early part of a development\n> cycle. Some larger topics would stay on 'next' for a few cycles.\n>\n>> Do you Junio, have plans to merge them to maint (2.3.x) and/or next (2.4)?\n>\n> The topic will hopefully be merged to 'master' after 2.4 final is\n> released end of this month, down to 'maint' early May and will ship\n> with 2.4.1, unless there is unforeseen issues discovered in the\n> change while people try it out while it is in 'next' (which will\n> happen today, hopefully).\n\nThanks for the clarification Junio.\n"},{"id":"259784","messageId":"CADoxLGOEE8rT7SS1n+wxmBbWWsLY+a5QstM=WPC=c5EajqfVkA@mail.gmail.com","threadId":"39098","inReplyTo":"xmqq4mo9gnq3.fsf@gitster.dls.corp.google.com","subject":"Re: [BUG] Performance regression due to #33d4221: write_sha1_file: freshen existing objects","fromName":"Stefan Saasen","fromEmail":"ssaasen@atlassian.com","sentAt":"2015-04-22T01:46:17Z","receivedAt":"2015-04-22T01:46:17Z","isPatch":false,"sender":{"key":"ssaasen@atlassian.com","avatar":null},"body":"> Perhaps companies like Atlassian that rely on the stability of the\n> open source Git can spare some resources and join forces with like\n> minded folks on LTS of older maintenance tracks, if they are truly\n> interested in.\n\nWe certainly can and would like to. I'm not entirely sure what that\nwould entail though?\n\n>From reading through $gmane/264365 I've identified the following\nresponsibilities/opportunities to help:\n\n> - Monitor \"git log --first-parent maint-lts..master\" and find\n> the tip of topic branches that need to be down-merged;\n>\n> - Down-merge such topics to maint-lts; this might involve\n> cherry-picking instead of merge, as the bugfix topics may\n> originally be done on the codebase newer than maint-lts;\n\nand more importantly testing the maint-lts version to ensure\nbackported changes don't introduce regressions and the maint-lts\nbranch is stable.\n\nThis suggests specific, spaced LTS versions but in the same thread you\nmention maint-2.1or maint-2.2.\nSo a different model could be maintaining old versions in a sliding\nwindow fashion (e.g. critical issues would be backported to the last 6\nmonths worth of git releases).\n\nMaybe I'm getting ahead of myself here :)\n\nAnyway, long story short. We're interested to help but I'm not\nentirely sure what that would look like at the moment. Are there\nformed ideas floating around or would you be looking for some form of\nproposal instead?\n\nCheers,\nStefan\n"},{"id":"259853","messageId":"xmqqoamfddsp.fsf@gitster.dls.corp.google.com","threadId":"39098","inReplyTo":"CADoxLGOEE8rT7SS1n+wxmBbWWsLY+a5QstM=WPC=c5EajqfVkA@mail.gmail.com","subject":"Re: [BUG] Performance regression due to #33d4221: write_sha1_file: freshen existing objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-22T22:00:06Z","receivedAt":"2015-04-22T22:00:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Saasen writes:\n\n> Anyway, long story short. We're interested to help but I'm not\n> entirely sure what that would look like at the moment. Are there\n> formed ideas floating around or would you be looking for some form of\n> proposal instead?\n\nI am not proposing anything or looking for proposals myself,\nactually. It is just somebody expressed interest in having tested\nolder maintenance track that is kept alive in the past, so I was\nmerely trying to help connect you with that old thread.\n\nIf those who are interested in having such LTS track(s) need\nsomething specific from me, and if it will not be unrealistic\nmaintenance burden, I am willing to help. That's all.\n\nFor example, LTS group for whatever reason may nominate 2.2.x track\nas a base that they want to keep alive longer than other maintenance\ntracks and promise to test changes to them to keep it stable. Then\nI can help the effort by making sure people's bugfix patches would\napply down to 2.2.x track (often people make mistake of using newer\nfacility to fix or test the fix for an ancient bug, and bugfix topic\nbranch ends up forked at a point much newer than where it should\nbe).\n"}]}