{"thread":{"id":"50294","subject":"git status OOM on mmap of large file","startedAt":"2019-01-22T22:16:48Z","lastAt":"2019-01-24T21:18:47Z","messageCount":12,"participants":["Joey Hess","brian m. carlson","Jeff King","Duy Nguyen","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"367385","messageId":"20190122220714.GA6176@kitenet.net","threadId":"50294","inReplyTo":null,"subject":"git status OOM on mmap of large file","fromName":"Joey Hess","fromEmail":"id@joeyh.name","sentAt":"2019-01-22T22:07:14Z","receivedAt":"2019-01-22T22:16:48Z","isPatch":false,"sender":{"key":"id@joeyh.name","avatar":"https://avatars.githubusercontent.com/u/16392?v=4"},"body":"joey@darkstar:~/tmp/t> ls -l big-file\n-rw-r--r-- 1 joey joey 11811160064 Jan 22 17:48 big-file\njoey@darkstar:~/tmp/t> git status\nfatal: Out of memory, realloc failed\n\nThis file is checked into git, but using a smudge/clean filter, so the actual\ndata checked into git is a hash. I did so using git-annex v7 mode, but I\nsuppose git lfs would cause the same problem.\n\n[pid  6573] mmap(NULL, 11811164160, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = -1 ENOMEM (Cannot allocate memory)\n\nWhy status needs to mmap a large file that is not modified\nand that is configured to pass through smudge/clean, I don't know.\nIt seems like it should be possible for status work in this situation.\n\n-- \nsee shy jo\n"},{"id":"367511","messageId":"20190124003948.GS423984@genre.crustytoothpaste.net","threadId":"50294","inReplyTo":"20190122220714.GA6176@kitenet.net","subject":"Re: git status OOM on mmap of large file","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-01-24T00:39:49Z","receivedAt":"2019-01-24T00:39:57Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Tue, Jan 22, 2019 at 06:07:14PM -0400, Joey Hess wrote:\n> joey@darkstar:~/tmp/t> ls -l big-file\n> -rw-r--r-- 1 joey joey 11811160064 Jan 22 17:48 big-file\n> joey@darkstar:~/tmp/t> git status\n> fatal: Out of memory, realloc failed\n> \n> This file is checked into git, but using a smudge/clean filter, so the actual\n> data checked into git is a hash. I did so using git-annex v7 mode, but I\n> suppose git lfs would cause the same problem.\n> \n> [pid  6573] mmap(NULL, 11811164160, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = -1 ENOMEM (Cannot allocate memory)\n> \n> Why status needs to mmap a large file that is not modified\n> and that is configured to pass through smudge/clean, I don't know.\n\nI believe that currently, Git stores the smudge/clean output in memory\nuntil it writes it out. When using the persistent filter process, it's\npossible for the process to choose to abort the operation, so we store\nthe data in memory until we get the status.\n\nTheoretically, it should be possible for us to write this to a temporary\nfile, and if necessary, rename into place, although I'm not sure how\nwell that will work on Windows. File modes may also be tricky here.\nPatches are of course welcome.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"367533","messageId":"20190124121037.GA4949@sigill.intra.peff.net","threadId":"50294","inReplyTo":"20190122220714.GA6176@kitenet.net","subject":"Re: git status OOM on mmap of large file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-24T12:10:38Z","receivedAt":"2019-01-24T12:10:41Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 22, 2019 at 06:07:14PM -0400, Joey Hess wrote:\n\n> joey@darkstar:~/tmp/t> ls -l big-file\n> -rw-r--r-- 1 joey joey 11811160064 Jan 22 17:48 big-file\n> joey@darkstar:~/tmp/t> git status\n> fatal: Out of memory, realloc failed\n> \n> This file is checked into git, but using a smudge/clean filter, so the actual\n> data checked into git is a hash. I did so using git-annex v7 mode, but I\n> suppose git lfs would cause the same problem.\n> \n> [pid  6573] mmap(NULL, 11811164160, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = -1 ENOMEM (Cannot allocate memory)\n> \n> Why status needs to mmap a large file that is not modified\n> and that is configured to pass through smudge/clean, I don't know.\n> It seems like it should be possible for status work in this situation.\n\nOne minor point: I don't think this is us mmap-ing the file. The\ndescriptor is -1, and Git never uses PROT_WRITE. This is likely your\nlibc using mmap to fulfill a malloc() request.\n\nThat said, it just turns the question into: why did Git try to malloc\nthat many bytes?  If I reproduce your example (using a 100MB file) and\nset GIT_ALLOC_LIMIT in the environment, the backtrace to die() is:\n\n  #1  0x0000555555786d65 in memory_limit_check (size=104857601, gentle=0) at wrapper.c:27\n  #2  0x0000555555787084 in xrealloc (ptr=0x0, size=104857601) at wrapper.c:137\n  #3  0x000055555575612e in strbuf_grow (sb=0x7fffffffdbf0, extra=104857600) at strbuf.c:98\n  #4  0x000055555575731a in strbuf_read (sb=0x7fffffffdbf0, fd=4, hint=104857600) at strbuf.c:429\n  #5  0x0000555555664a1f in apply_single_file_filter (path=0x5555558c787c \"foo.rand\", ...)\n  #6  0x0000555555665321 in apply_filter (path=0x5555558c787c \"foo.rand\", ...)\n  ...\n\nLooking at apply_single_file_filter(), it's not the _original_ file that\nit's trying to store, but rather the data coming back from the filter.\nIt's just that we use the original file size as a hint!\n\nIn this case (and I'd venture to say in most gigantic-file cases) it's\nmuch larger than we need, to the point of causing a problem.\n\nIn other words, I think this patch fixes your problem:\n\ndiff --git a/convert.c b/convert.c\nindex 0d89ae7c23..85aebe2ed3 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -732,7 +732,7 @@ static int apply_single_file_filter(const char *path, const char *src, size_t le\n \tif (start_async(&async))\n \t\treturn 0;\t/* error was already reported */\n \n-\tif (strbuf_read(&nbuf, async.out, len) < 0) {\n+\tif (strbuf_read(&nbuf, async.out, 0) < 0) {\n \t\terr = error(_(\"read from external filter '%s' failed\"), cmd);\n \t}\n \tif (close(async.out)) {\n\nthough possibly we should actually continue to use the file size as a\nhint up to a certain point, which avoids reallocations for more \"normal\"\nfilters where the input and output sizes are in the same ballpark.\n\nJust off the top of my head, something like:\n\n  /* guess that the filtered output will be the same size as the original */\n  hint = len;\n\n  /* allocate 10% extra in case the clean size is slightly larger */\n  hint *= 1.1;\n\n  /*\n   * in any case, never go higher than half of core.bigfileThreshold.\n   * We'd like to avoid allocating more bytes than that, and that still\n   * gives us room for our strbuf to preemptively double if our guess is\n   * just a little on the low side.\n   */\n  if (hint > big_file_threshold / 2)\n\thint = big_file_threshold / 2;\n\nBut to be honest, I have no idea if that would even produce measurable\nbenefits over simply growing the strbuf from scratch (i.e., hint==0).\n\n-Peff\n"},{"id":"367534","messageId":"20190124121427.GB4949@sigill.intra.peff.net","threadId":"50294","inReplyTo":"20190124003948.GS423984@genre.crustytoothpaste.net","subject":"Re: git status OOM on mmap of large file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-24T12:14:28Z","receivedAt":"2019-01-24T12:14:31Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 24, 2019 at 12:39:49AM +0000, brian m. carlson wrote:\n\n> > [pid  6573] mmap(NULL, 11811164160, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = -1 ENOMEM (Cannot allocate memory)\n> > \n> > Why status needs to mmap a large file that is not modified\n> > and that is configured to pass through smudge/clean, I don't know.\n> \n> I believe that currently, Git stores the smudge/clean output in memory\n> until it writes it out. When using the persistent filter process, it's\n> possible for the process to choose to abort the operation, so we store\n> the data in memory until we get the status.\n\nFor the clean step, that should be OK, since that filter output is tiny\n(but see my other message for a silly heuristic and an easy fix). And\nthat should be all that \"git status\" needs. But...\n\n> Theoretically, it should be possible for us to write this to a temporary\n> file, and if necessary, rename into place, although I'm not sure how\n> well that will work on Windows. File modes may also be tricky here.\n> Patches are of course welcome.\n\nI didn't experiment with the smudge side, but I think it uses the same\napply_filter() code. Which means that yes, it would try to store the\n11GB in memory before writing it out. And I agree writing it out to a\nfile and moving it directly into place is the sanest option there. If\nthat doesn't work, spooling to a tempfile and then streaming it into\nplace would also work.\n\n-Peff\n"},{"id":"367542","messageId":"CACsJy8A3JbO775qaR1PBAsosh031JWmDPv5Y40JzUP=fuT9hRA@mail.gmail.com","threadId":"50294","inReplyTo":"20190124121037.GA4949@sigill.intra.peff.net","subject":"Re: git status OOM on mmap of large file","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-01-24T12:54:01Z","receivedAt":"2019-01-24T12:54:30Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Jan 24, 2019 at 7:11 PM Jeff King <peff@peff.net> wrote:\n>\n> On Tue, Jan 22, 2019 at 06:07:14PM -0400, Joey Hess wrote:\n>\n> > joey@darkstar:~/tmp/t> ls -l big-file\n> > -rw-r--r-- 1 joey joey 11811160064 Jan 22 17:48 big-file\n> > joey@darkstar:~/tmp/t> git status\n> > fatal: Out of memory, realloc failed\n> >\n> > This file is checked into git, but using a smudge/clean filter, so the actual\n> > data checked into git is a hash. I did so using git-annex v7 mode, but I\n> > suppose git lfs would cause the same problem.\n> >\n> > [pid  6573] mmap(NULL, 11811164160, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = -1 ENOMEM (Cannot allocate memory)\n> >\n> > Why status needs to mmap a large file that is not modified\n> > and that is configured to pass through smudge/clean, I don't know.\n> > It seems like it should be possible for status work in this situation.\n>\n> One minor point: I don't think this is us mmap-ing the file. The\n> descriptor is -1, and Git never uses PROT_WRITE. This is likely your\n> libc using mmap to fulfill a malloc() request.\n>\n> That said, it just turns the question into: why did Git try to malloc\n> that many bytes?  If I reproduce your example (using a 100MB file) and\n> set GIT_ALLOC_LIMIT in the environment, the backtrace to die() is:\n>\n>   #1  0x0000555555786d65 in memory_limit_check (size=104857601, gentle=0) at wrapper.c:27\n>   #2  0x0000555555787084 in xrealloc (ptr=0x0, size=104857601) at wrapper.c:137\n>   #3  0x000055555575612e in strbuf_grow (sb=0x7fffffffdbf0, extra=104857600) at strbuf.c:98\n>   #4  0x000055555575731a in strbuf_read (sb=0x7fffffffdbf0, fd=4, hint=104857600) at strbuf.c:429\n>   #5  0x0000555555664a1f in apply_single_file_filter (path=0x5555558c787c \"foo.rand\", ...)\n>   #6  0x0000555555665321 in apply_filter (path=0x5555558c787c \"foo.rand\", ...)\n>   ...\n>\n> Looking at apply_single_file_filter(), it's not the _original_ file that\n> it's trying to store, but rather the data coming back from the filter.\n> It's just that we use the original file size as a hint!\n\nReally cool! I guessed as far as malloc() but did not actually test\nit, let alone examine the problem closely like this.\n-- \nDuy\n"},{"id":"367562","messageId":"20190124170544.GB29200@kitenet.net","threadId":"50294","inReplyTo":"20190124121427.GB4949@sigill.intra.peff.net","subject":"Re: git status OOM on mmap of large file","fromName":"Joey Hess","fromEmail":"id@joeyh.name","sentAt":"2019-01-24T17:05:44Z","receivedAt":"2019-01-24T17:05:54Z","isPatch":false,"sender":{"key":"id@joeyh.name","avatar":"https://avatars.githubusercontent.com/u/16392?v=4"},"body":"Jeff King wrote:\n> I didn't experiment with the smudge side, but I think it uses the same\n> apply_filter() code. Which means that yes, it would try to store the\n> 11GB in memory before writing it out. And I agree writing it out to a\n> file and moving it directly into place is the sanest option there. If\n> that doesn't work, spooling to a tempfile and then streaming it into\n> place would also work.\n\nI have seen that buffering on the smudge side, yes. In git-annex \nI happen to use the smudge filter in an unusual way that avoids\nthat being a problem but I think it would affect git-lfs.\n\n-- \nsee shy jo\n"},{"id":"367574","messageId":"20190124183810.GC29200@kitenet.net","threadId":"50294","inReplyTo":"20190124121037.GA4949@sigill.intra.peff.net","subject":"Re: git status OOM on mmap of large file","fromName":"Joey Hess","fromEmail":"id@joeyh.name","sentAt":"2019-01-24T18:38:10Z","receivedAt":"2019-01-24T18:38:24Z","isPatch":false,"sender":{"key":"id@joeyh.name","avatar":"https://avatars.githubusercontent.com/u/16392?v=4"},"body":"Jeff King wrote:\n> Looking at apply_single_file_filter(), it's not the _original_ file that\n> it's trying to store, but rather the data coming back from the filter.\n> It's just that we use the original file size as a hint!\n\nThanks much for working that out!\n\n> In other words, I think this patch fixes your problem:\n> \n> diff --git a/convert.c b/convert.c\n> index 0d89ae7c23..85aebe2ed3 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -732,7 +732,7 @@ static int apply_single_file_filter(const char *path, const char *src, size_t le\n>  \tif (start_async(&async))\n>  \t\treturn 0;\t/* error was already reported */\n>  \n> -\tif (strbuf_read(&nbuf, async.out, len) < 0) {\n> +\tif (strbuf_read(&nbuf, async.out, 0) < 0) {\n>  \t\terr = error(_(\"read from external filter '%s' failed\"), cmd);\n>  \t}\n>  \tif (close(async.out)) {\n\nYes, confirmed that does fix it.\n\n> though possibly we should actually continue to use the file size as a\n> hint up to a certain point, which avoids reallocations for more \"normal\"\n> filters where the input and output sizes are in the same ballpark.\n> \n> Just off the top of my head, something like:\n> \n>   /* guess that the filtered output will be the same size as the original */\n>   hint = len;\n> \n>   /* allocate 10% extra in case the clean size is slightly larger */\n>   hint *= 1.1;\n> \n>   /*\n>    * in any case, never go higher than half of core.bigfileThreshold.\n>    * We'd like to avoid allocating more bytes than that, and that still\n>    * gives us room for our strbuf to preemptively double if our guess is\n>    * just a little on the low side.\n>    */\n>   if (hint > big_file_threshold / 2)\n> \thint = big_file_threshold / 2;\n> \n> But to be honest, I have no idea if that would even produce measurable\n> benefits over simply growing the strbuf from scratch (i.e., hint==0).\n\nHalf of 512 MB is still quite a lot of memory to default to using in\nthis situation. Eg smaller VPS's still often only have a GB or two of ram.\n\nWhen the clean filter is being used in a way that doesn't involve hashes\nof large files, it will mostly be operating on typically sized source\ncode files. So capping the maximum hint size around the size of a typical\nsource code file would be plenty for both common cases for the clean filter.\n\nI did some benchmarking, using cat as the clean filter:\n\ngit status 32 kb file, hint == len\n   time                 3.865 ms   (3.829 ms .. 3.943 ms)\n                        0.994 R²   (0.987 R² .. 0.999 R²)\n   mean                 3.934 ms   (3.889 ms .. 4.021 ms)\n   std dev              191.8 μs   (106.8 μs .. 291.8 μs)\ngit status 32 kb file, hint == 0\n   time                 3.887 ms   (3.751 ms .. 4.064 ms)\n                        0.992 R²   (0.986 R² .. 0.998 R²)\n   mean                 4.002 ms   (3.931 ms .. 4.138 ms)\n   std dev              292.2 μs   (189.0 μs .. 498.3 μs)\ngit status 1 mb file, hint == len\n   time                 3.942 ms   (3.816 ms .. 4.078 ms)\n                        0.995 R²   (0.991 R² .. 0.999 R²)\n   mean                 3.969 ms   (3.916 ms .. 4.054 ms)\n   std dev              220.1 μs   (155.1 μs .. 304.3 μs)\ngit status 1 mb file, hint == 0\n   time                 3.869 ms   (3.836 ms .. 3.911 ms)\n                        0.998 R²   (0.995 R² .. 1.000 R²)\n   mean                 3.895 ms   (3.868 ms .. 3.947 ms)\n   std dev              112.3 μs   (47.93 μs .. 182.7 μs)\ngit status 1 gb file, hint == len\n   time                 7.173 s    (6.834 s .. 7.564 s)\n                        0.999 R²   (0.999 R² .. 1.000 R²)\n   mean                 7.560 s    (7.369 s .. 7.903 s)\n   std dev              333.2 ms   (27.65 ms .. 412.2 ms)\ngit status 1 gb file, hint == 0\n   time                 7.652 s    (6.307 s .. 8.263 s)\n                        0.996 R²   (0.992 R² .. 1.000 R²)\n   mean                 8.082 s    (7.843 s .. 8.202 s)\n   std dev              232.3 ms   (2.362 ms .. 277.1 ms)\n\nFrom this, it looks like the file has to be quite large before the\npreallocation makes a sizable improvement to runtime, and the\nsmudge/clean filters have to be used for actual content filtering\n(not for hash generation purposes as git-annex and git-lfs use it).\nAn unusual edge case I think. So hint == 0 seems fine.\n\n-- \nsee shy jo\n"},{"id":"367582","messageId":"20190124191836.GA31073@sigill.intra.peff.net","threadId":"50294","inReplyTo":"20190124183810.GC29200@kitenet.net","subject":"Re: git status OOM on mmap of large file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-24T19:18:36Z","receivedAt":"2019-01-24T19:18:39Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 24, 2019 at 02:38:10PM -0400, Joey Hess wrote:\n\n> > Just off the top of my head, something like:\n> > \n> >   /* guess that the filtered output will be the same size as the original */\n> >   hint = len;\n> > \n> >   /* allocate 10% extra in case the clean size is slightly larger */\n> >   hint *= 1.1;\n> > \n> >   /*\n> >    * in any case, never go higher than half of core.bigfileThreshold.\n> >    * We'd like to avoid allocating more bytes than that, and that still\n> >    * gives us room for our strbuf to preemptively double if our guess is\n> >    * just a little on the low side.\n> >    */\n> >   if (hint > big_file_threshold / 2)\n> > \thint = big_file_threshold / 2;\n> > \n> > But to be honest, I have no idea if that would even produce measurable\n> > benefits over simply growing the strbuf from scratch (i.e., hint==0).\n> \n> Half of 512 MB is still quite a lot of memory to default to using in\n> this situation. Eg smaller VPS's still often only have a GB or two of ram.\n\nI think you'd want to drop core.bigFileThreshold on such a server, just\nbecause Git will happily keep 2*(bigFileThreshold-1) in memory to do a\ndiff. But that nit aside...\n\n> I did some benchmarking, using cat as the clean filter:\n> [...]\n> From this, it looks like the file has to be quite large before the\n> preallocation makes a sizable improvement to runtime, and the\n> smudge/clean filters have to be used for actual content filtering\n> (not for hash generation purposes as git-annex and git-lfs use it).\n> An unusual edge case I think. So hint == 0 seems fine.\n\nThanks for these timings! I agree that \"hint == 0\" is probably\nreasonable, then.\n\nI suppose there's no reason not to proceed with a patch around this.\nFor most cases it's really only half the solution (since smudging is\ngoing to run into the same problem). But fixing that is quite a bit more\ninvolved, and the change itself will be largely orthogonal.\n\n-Peff\n"},{"id":"367591","messageId":"20190124192831.GA14201@sigill.intra.peff.net","threadId":"50294","inReplyTo":"20190124191836.GA31073@sigill.intra.peff.net","subject":"Re: git status OOM on mmap of large file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-24T19:28:31Z","receivedAt":"2019-01-24T20:09:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 24, 2019 at 02:18:36PM -0500, Jeff King wrote:\n\n> > I did some benchmarking, using cat as the clean filter:\n> > [...]\n> > From this, it looks like the file has to be quite large before the\n> > preallocation makes a sizable improvement to runtime, and the\n> > smudge/clean filters have to be used for actual content filtering\n> > (not for hash generation purposes as git-annex and git-lfs use it).\n> > An unusual edge case I think. So hint == 0 seems fine.\n> \n> Thanks for these timings! I agree that \"hint == 0\" is probably\n> reasonable, then.\n\nOne other minor point to consider: on some systems over-allocating\nactually isn't that expensive, because pages are actually allocated\nuntil we write to them, and malloc() is perfectly happy to overcommit\nmemory.  Your case would only run into problems on Linux when malloc()\nactually refuses the allocation (so limiting ourselves to \"too large but\nstill reasonable\" is a valid strategy there).\n\nBut I doubt that's something we should be relying on in general. There\nare many systems that don't overcommit.\n\n-Peff\n"},{"id":"367596","messageId":"20190124203639.GA17595@kitenet.net","threadId":"50294","inReplyTo":"20190124191836.GA31073@sigill.intra.peff.net","subject":"[PATCH] avoid unncessary malloc of whole file size","fromName":"Joey Hess","fromEmail":"id@joeyh.name","sentAt":"2019-01-24T20:36:39Z","receivedAt":"2019-01-24T20:36:50Z","isPatch":true,"sender":{"key":"id@joeyh.name","avatar":"https://avatars.githubusercontent.com/u/16392?v=4"},"body":"When a worktree file is larger than the available memory, and a clean\nfilter is in use, this avoids mallocing a buffer the whole size of the\nfile when reading from the clean filter, which caused commands like git\nstatus and git commit to OOM.\n\nOften in this situation the clean filter will produce a short identifier\nfor the file, so such a large buffer is not needed.\n\nWhen the clean filter does output something around the same size as the\nworktree file, the buffer will need to be reallocated until it fits,\nstarting at 8192 and doubling in size. Benchmarking indicates that\nreallocation is not a significant overhead for outputs up to a\nfew MB in size.\n\nSigned-off-by: Joey Hess <id@joeyh.name>\n---\n convert.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/convert.c b/convert.c\nindex 0d89ae7c23..85aebe2ed3 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -732,7 +732,7 @@ static int apply_single_file_filter(const char *path, const char *src, size_t le\n \tif (start_async(&async))\n \t\treturn 0;\t/* error was already reported */\n \n-\tif (strbuf_read(&nbuf, async.out, len) < 0) {\n+\tif (strbuf_read(&nbuf, async.out, 0) < 0) {\n \t\terr = error(_(\"read from external filter '%s' failed\"), cmd);\n \t}\n \tif (close(async.out)) {\n-- \n2.20.1\n\n"},{"id":"367600","messageId":"xmqqlg39hia8.fsf@gitster-ct.c.googlers.com","threadId":"50294","inReplyTo":"20190124203639.GA17595@kitenet.net","subject":"Re: [PATCH] avoid unncessary malloc of whole file size","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-24T21:12:15Z","receivedAt":"2019-01-24T21:12:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joey Hess <id@joeyh.name> writes:\n\n> When a worktree file is larger than the available memory, and a clean\n> filter is in use, this avoids mallocing a buffer the whole size of the\n> file when reading from the clean filter, which caused commands like git\n> status and git commit to OOM.\n>\n> Often in this situation the clean filter will produce a short identifier\n> for the file, so such a large buffer is not needed.\n>\n> When the clean filter does output something around the same size as the\n> worktree file, the buffer will need to be reallocated until it fits,\n> starting at 8192 and doubling in size. Benchmarking indicates that\n> reallocation is not a significant overhead for outputs up to a\n> few MB in size.\n\nProblem description first, then solultion.  \"... this avoids ...\" is\nalready talking about solution while forcing the readers to know\nwhat the problem is.\n\n    When a worktree file is ... filter is in use, we allocate a\n    buffer for the whole size of the file when reading from the\n    clean filter.  This can force us to overallocate if the clean\n    filter is used to radically shrink a huge file and replace it\n    with a small token (e.g. git-annex or git-lfs) and lead to OOM\n    at the worst case.  Reading from the filter and growing the\n    buffer as we go would avoid such an unnecessary OOM.\n\n    When the clean filter does output ...\n    ... few MB in size.\n\nperhaps.\n\n> Signed-off-by: Joey Hess <id@joeyh.name>\n> ---\n>  convert.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/convert.c b/convert.c\n> index 0d89ae7c23..85aebe2ed3 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -732,7 +732,7 @@ static int apply_single_file_filter(const char *path, const char *src, size_t le\n>  \tif (start_async(&async))\n>  \t\treturn 0;\t/* error was already reported */\n>  \n> -\tif (strbuf_read(&nbuf, async.out, len) < 0) {\n> +\tif (strbuf_read(&nbuf, async.out, 0) < 0) {\n>  \t\terr = error(_(\"read from external filter '%s' failed\"), cmd);\n>  \t}\n>  \tif (close(async.out)) {\n"},{"id":"367605","messageId":"20190124211844.GC16114@sigill.intra.peff.net","threadId":"50294","inReplyTo":"xmqqlg39hia8.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] avoid unncessary malloc of whole file size","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-24T21:18:44Z","receivedAt":"2019-01-24T21:18:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 24, 2019 at 01:12:15PM -0800, Junio C Hamano wrote:\n\n> Joey Hess <id@joeyh.name> writes:\n> \n> > When a worktree file is larger than the available memory, and a clean\n> > filter is in use, this avoids mallocing a buffer the whole size of the\n> > file when reading from the clean filter, which caused commands like git\n> > status and git commit to OOM.\n> >\n> > Often in this situation the clean filter will produce a short identifier\n> > for the file, so such a large buffer is not needed.\n> >\n> > When the clean filter does output something around the same size as the\n> > worktree file, the buffer will need to be reallocated until it fits,\n> > starting at 8192 and doubling in size. Benchmarking indicates that\n> > reallocation is not a significant overhead for outputs up to a\n> > few MB in size.\n> \n> Problem description first, then solultion.  \"... this avoids ...\" is\n> already talking about solution while forcing the readers to know\n> what the problem is.\n> \n>     When a worktree file is ... filter is in use, we allocate a\n>     buffer for the whole size of the file when reading from the\n>     clean filter.  This can force us to overallocate if the clean\n>     filter is used to radically shrink a huge file and replace it\n>     with a small token (e.g. git-annex or git-lfs) and lead to OOM\n>     at the worst case.  Reading from the filter and growing the\n>     buffer as we go would avoid such an unnecessary OOM.\n> \n>     When the clean filter does output ...\n>     ... few MB in size.\n> \n> perhaps.\n\nYeah, I agree that organization is nicer. Other than that, the patch\nlooks good to me.\n\n-Peff\n"}]}