{"thread":{"id":"50685","subject":"[PATCH] convert: avoid malloc of original file size","startedAt":"2019-03-07T19:57:00Z","lastAt":"2019-03-08T02:52:40Z","messageCount":3,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"370924","messageId":"20190307195657.GA29776@sigill.intra.peff.net","threadId":"50685","inReplyTo":null,"subject":"[PATCH] convert: avoid malloc of original file size","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-07T19:56:57Z","receivedAt":"2019-03-07T19:57:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Joey Hess \n\nWe write the output of a \"clean\" filter into a strbuf. Rather than\ngrowing the strbuf dynamically as we read its output, we make the\ninitial allocation as large as the original input file. This is a good\nguess when the filter is just tweaking a few bytes, but it's disastrous\nwhen the point of the filter is to condense a very large file into a\nshort identifier (e.g., the way git-lfs and git-annex do). We may ask to\nallocate many gigabytes, causing the allocation to fail and Git to\ndie().\n\nInstead, let's just let strbuf do its usual growth.\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 \nSigned-off-by: Jeff King \n---\nThis is a resurrection of the patch from:\n\n https://public-inbox.org/git/20190122220714.GA6176@kitenet.net/\n\nIt got stalled on discussion of the commit message, which I've rewritten\nhere to match the suggestions in the thread.\n\nAs discussed there, I do think this only solves half the problem, as the\nsmudge filter has the same issue in reverse. That's more complicated to\nfix, and AFAIK nobody is working on it. But I don't think there's any\nreason not to pick up this part in the meantime.\n\n convert.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/convert.c b/convert.c\nindex 5d0307fc10..94ff837649 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -731,7 +731,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.21.0.787.g929e938557\n"},{"id":"370935","messageId":"xmqqa7i6kvgf.fsf@gitster-ct.c.googlers.com","threadId":"50685","inReplyTo":"20190307195657.GA29776@sigill.intra.peff.net","subject":"Re: [PATCH] convert: avoid malloc of original file size","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-08T01:26:24Z","receivedAt":"2019-03-08T01:26:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> As discussed there, I do think this only solves half the problem, as the\n> smudge filter has the same issue in reverse. That's more complicated to\n> fix, and AFAIK nobody is working on it. But I don't think there's any\n> reason not to pick up this part in the meantime.\n\nYeah, I agree that the reverse direction shares the same issue.\n\nI am not sure 0 is a good initial value in this direction, either;\nI'd rather clip to min(len, core.bigfilethreshold) or something like\nthat, to avoid regressing the more normal use cases. \n\nBut let's queue this and see what happens.\n\nThanks.\n\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 5d0307fc10..94ff837649 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -731,7 +731,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":"370938","messageId":"20190308025237.GA12304@sigill.intra.peff.net","threadId":"50685","inReplyTo":"xmqqa7i6kvgf.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] convert: avoid malloc of original file size","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-08T02:52:37Z","receivedAt":"2019-03-08T02:52:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 08, 2019 at 10:26:24AM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > As discussed there, I do think this only solves half the problem, as the\n> > smudge filter has the same issue in reverse. That's more complicated to\n> > fix, and AFAIK nobody is working on it. But I don't think there's any\n> > reason not to pick up this part in the meantime.\n> \n> Yeah, I agree that the reverse direction shares the same issue.\n> \n> I am not sure 0 is a good initial value in this direction, either;\n> I'd rather clip to min(len, core.bigfilethreshold) or something like\n> that, to avoid regressing the more normal use cases.\n\nThat was my initial thought, too, but Joey's benchmarks show that it\ndoesn't seem to make a big difference either way. In his numbers it did\nget measurable for a 1GB file, but we'd still not use \"hint == len\" in\nthat case (we'd probably do one or two doublings to get there).\n\nI also think running a real (non-condensing) filter on a 1GB file is\nalready a pretty unlikely corner case.\n\n> But let's queue this and see what happens.\n\nSounds good to me.\n\n-Peff\n"}]}