{"thread":{"id":"27886","subject":"[REGRESSION, BISECTED] `git checkout ` started to be memory hog","startedAt":"2011-07-22T13:05:18Z","lastAt":"2011-07-23T18:04:11Z","messageCount":8,"participants":["Kirill Smelkov","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"171840","messageId":"20110722130518.GA9873@tugrik.mns.mnsspb.ru","threadId":"27886","inReplyTo":null,"subject":"[REGRESSION, BISECTED] `git checkout ` started to be memory hog","fromName":"Kirill Smelkov","fromEmail":"kirr@mns.spb.ru","sentAt":"2011-07-22T13:05:18Z","receivedAt":"2011-07-22T13:05:18Z","isPatch":false,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"Hello up there,\n\nToday I've tried to upgrade to linux-3.0 on my netbook with 1GB of RAM,\nwith today's Git master (v1.7.6-233-gd79bcd6) and Git was killed by an OOM:\n\n $ git new-workdir linux linux-3.0 linux-3.0.y\n Checking out files: ~80% (XXXXX/36783)\n Aborted\n (with OOM messages in dmesg).\n\ngit new-workdir boils down to (uninteresting to the issue) symlinks\nsetup + final `git checkout -f ` and that final git checkout is\nfailing.\n\n\nIt turned out that with Git v1.7.6 memory usage for git-checkout\nlinux-3.0.y as seen in top is\n\n VIRTmax RESmax\n\n ~338M ~247M\n\nand for master\n\n VIRTmax RESmax\n (both till not killed)\n ~2200M ~1000M\n\n\ni.e. it looks like when residential memory usage approaches the amount of\nphysical RAM, the OOM killer comes into play.\n\n\nAnd I've bisected this to b6691092 (\"Add streaming filter API\"; Junio C\nHamano, May 20 2011; merged to next on Jun 30 2011):\n\n commit b6691092d707860019bbab80eaaf9173ada10586\n Author: Junio C Hamano \n Date: Fri May 20 14:33:31 2011 -0700\n\n Add streaming filter API\n\n This introduces an API to plug custom filters to an input stream.\n\n The caller gets get_stream_filter(\"path\") to obtain an appropriate\n filter for the path, and then uses it when opening an input stream\n via open_istream(). After that, the caller can read from the stream\n with read_istream(), and close it with close_istream(), just like an\n unfiltered stream.\n\n This only adds a \"null\" filter that is a pass-thru filter, but later\n changes can add LF-to-CRLF and other filters, and the callers of the\n streaming API do not have to change.\n\n Signed-off-by: Junio C Hamano \n\n\n\nHope Git stays usable for us, with not enormous amount of RAM onboard,\nand thanks beforehand for fixing,\n\nKirill\n"},{"id":"171845","messageId":"20110722170001.GB20700@sigill.intra.peff.net","threadId":"27886","inReplyTo":"20110722130518.GA9873@tugrik.mns.mnsspb.ru","subject":"Re: [REGRESSION, BISECTED] `git checkout ` started to be memory hog","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-07-22T17:00:03Z","receivedAt":"2011-07-22T17:00:03Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 22, 2011 at 05:05:18PM +0400, Kirill Smelkov wrote:\n\n> It turned out that with Git v1.7.6 memory usage for git-checkout\n> linux-3.0.y as seen in top is\n> \n> VIRTmax RESmax\n> \n> ~338M ~247M\n> \n> and for master\n> \n> VIRTmax RESmax\n> (both till not killed)\n> ~2200M ~1000M\n> \n> \n> i.e. it looks like when residential memory usage approaches the amount of\n> physical RAM, the OOM killer comes into play.\n> \n> \n> And I've bisected this to b6691092 (\"Add streaming filter API\"; Junio C\n> Hamano, May 20 2011; merged to next on Jun 30 2011):\n\nHmm, that series was supposed to _reduce_ memory usage. :)\n\nAccording to valgrind, we are leaking gigabytes of memory allocated in\ngit_istream buffers:\n\n $ cd linux-2.6\n $ rm -vrf *\n $ valgrind --leak-check=full git checkout -f\n [...]\n 2,418,940,448 (1,163,660,832 direct, 1,255,279,616 indirect) bytes in 35,271\n blocks are definitely lost in loss record 81 of 81\n at 0x4C2780D: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)\n by 0x517C62: xmalloc (wrapper.c:35)\n by 0x503112: attach_stream_filter (streaming.c:255)\n by 0x502D61: open_istream (streaming.c:152)\n by 0x4B1B7A: streaming_write_entry (entry.c:130)\n by 0x4B1E0B: write_entry (entry.c:193)\n by 0x4B23F4: checkout_entry (entry.c:318)\n by 0x511DA4: check_updates (unpack-trees.c:223)\n by 0x513D72: unpack_trees (unpack-trees.c:1125)\n by 0x41CCB4: reset_tree (checkout.c:333)\n by 0x41CE32: merge_working_tree (checkout.c:378)\n by 0x41DE6A: switch_branches (checkout.c:737)\n\nThis malloc is for the actual git_istream struct. It seems that we never\nactually free it when calling close_istream(). And these structs are\nquite big; they contain 32K of filter buffers inside a union.\n\n>From my quick look, I came up with the fix below. It removes the leak\nand doesn't trigger any memory errors according to valgrind. So it\n_must_ be right. :)\n\n-Peff\n\n---\ndiff --git a/streaming.c b/streaming.c\nindex 565f000..f3acc5d 100644\n--- a/streaming.c\n+++ b/streaming.c\n@@ -93,7 +93,9 @@ struct git_istream {\n \n int close_istream(struct git_istream *st)\n {\n-\treturn st->vtbl->close(st);\n+\tint r = st->vtbl->close(st);\n+\tfree(st);\n+\treturn r;\n }\n \n ssize_t read_istream(struct git_istream *st, char *buf, size_t sz)\n"},{"id":"171846","messageId":"7vipquz0d0.fsf@alter.siamese.dyndns.org","threadId":"27886","inReplyTo":"20110722170001.GB20700@sigill.intra.peff.net","subject":"Re: [REGRESSION, BISECTED] `git checkout ` started to be memory hog","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-07-22T17:33:31Z","receivedAt":"2011-07-22T17:33:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> From my quick look, I came up with the fix below. It removes the leak\n> and doesn't trigger any memory errors according to valgrind. So it\n> _must_ be right. :)\n>\n> -Peff\n>\n> ---\n> diff --git a/streaming.c b/streaming.c\n> index 565f000..f3acc5d 100644\n> --- a/streaming.c\n> +++ b/streaming.c\n> @@ -93,7 +93,9 @@ struct git_istream {\n> \n> int close_istream(struct git_istream *st)\n> {\n> -\treturn st->vtbl->close(st);\n> +\tint r = st->vtbl->close(st);\n> +\tfree(st);\n> +\treturn r;\n> }\n\nI do not think of a reason any future callers would want to close the\nstream first and then inspect some attribute of the stream afterwards (if\nthis were an output stream, there might be things like output stats that\nmay want such an interface, but even in such a case, the caller should say\n\"flush\" first to drain whatever is pending, grab stats and then close), so\nfreeing the resource at close time seems sensible to me.\n\nBoth the external caller in entry.c and the internal caller (a filtered\nistream reads from its underlying istream, and when it is getting closed,\ncloses the underlying istream) were leaking the istream exactly the same\nway, so this fix should plug both of them.\n\nThanks.\n"},{"id":"171847","messageId":"20110722173601.GA9422@sigill.intra.peff.net","threadId":"27886","inReplyTo":"7vipquz0d0.fsf@alter.siamese.dyndns.org","subject":"Re: [REGRESSION, BISECTED] `git checkout ` started to be memory hog","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-07-22T17:36:02Z","receivedAt":"2011-07-22T17:36:02Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 22, 2011 at 10:33:31AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > From my quick look, I came up with the fix below. It removes the leak\n> > and doesn't trigger any memory errors according to valgrind. So it\n> > _must_ be right. :)\n> >\n> > -Peff\n> >\n> > ---\n> > diff --git a/streaming.c b/streaming.c\n> > index 565f000..f3acc5d 100644\n> > --- a/streaming.c\n> > +++ b/streaming.c\n> > @@ -93,7 +93,9 @@ struct git_istream {\n> > \n> > int close_istream(struct git_istream *st)\n> > {\n> > -\treturn st->vtbl->close(st);\n> > +\tint r = st->vtbl->close(st);\n> > +\tfree(st);\n> > +\treturn r;\n> > }\n> \n> I do not think of a reason any future callers would want to close the\n> stream first and then inspect some attribute of the stream afterwards (if\n> this were an output stream, there might be things like output stats that\n> may want such an interface, but even in such a case, the caller should say\n> \"flush\" first to drain whatever is pending, grab stats and then close), so\n> freeing the resource at close time seems sensible to me.\n\nYeah. My impression was that these istreams would operate much like\n\"FILE *\" streams. And closing those frees all resources, and you can\nthrow away the pointer afterwards.\n\n> Both the external caller in entry.c and the internal caller (a filtered\n> istream reads from its underlying istream, and when it is getting closed,\n> closes the underlying istream) were leaking the istream exactly the same\n> way, so this fix should plug both of them.\n> \n> Thanks.\n\nDid you want me to write a commit message? I think you might do a better\njob of writing a coherent one for this particular patch.\n\n-Peff\n"},{"id":"171860","messageId":"7vaac6ysd2.fsf@alter.siamese.dyndns.org","threadId":"27886","inReplyTo":"20110722173601.GA9422@sigill.intra.peff.net","subject":"Re: [REGRESSION, BISECTED] `git checkout ` started to be memory hog","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-07-22T20:26:17Z","receivedAt":"2011-07-22T20:26:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Did you want me to write a commit message? I think you might do a better\n> job of writing a coherent one for this particular patch.\n\nI didn't do a very good job.\n\ncommit 06a21454b424d5bc0a36ed76ce46bb16ed156774\nAuthor: Jeff King \nDate: Fri Jul 22 11:00:03 2011 -0600\n\n streaming: free git_istream upon closing\n \n Kirill Smelkov noticed that post 1.7.6 \"git checkout\" started leaking\n tons of memory. close_istream() forgot to release its buffer and the\n caller was not freeing it, either.\n \n Signed-off-by: Junio C Hamano \n\nAt least it needs your S-o-b ;-).\n"},{"id":"171862","messageId":"20110722203614.GA13232@sigill.intra.peff.net","threadId":"27886","inReplyTo":"7vaac6ysd2.fsf@alter.siamese.dyndns.org","subject":"Re: [REGRESSION, BISECTED] `git checkout ` started to be memory hog","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-07-22T20:36:14Z","receivedAt":"2011-07-22T20:36:14Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 22, 2011 at 01:26:17PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Did you want me to write a commit message? I think you might do a better\n> > job of writing a coherent one for this particular patch.\n> \n> I didn't do a very good job.\n\nHow about:\n\n-- >8 --\nSubject: [PATCH] streaming: free git_istream upon closing\n\nKirill Smelkov noticed that post-1.7.6 \"git checkout\"\nstarted leaking tons of memory. The streaming_write_entry\nfunction properly calls close_istream(), but that function\ndid not actually free() the allocated git_istream struct.\n\nThe git_istream struct is totally opaque to calling code,\nand must be heap-allocated by open_istream. Therefore it's\nnot appropriate for callers to have to free it.\n\nThis patch makes close_istream() into \"close and de-allocate\nall associated resources\". We could add a new \"free_istream\"\ncall, but there's not much point in letting callers inspect\nthe istream after close. And this patch's semantics make us\nmatch fopen/fclose, which is well-known and understood.\n\nSigned-off-by: Jeff King \n---\n streaming.c | 4 +++-\n 1 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/streaming.c b/streaming.c\nindex 565f000..f3acc5d 100644\n--- a/streaming.c\n+++ b/streaming.c\n@@ -93,7 +93,9 @@ struct git_istream {\n \n int close_istream(struct git_istream *st)\n {\n-\treturn st->vtbl->close(st);\n+\tint r = st->vtbl->close(st);\n+\tfree(st);\n+\treturn r;\n }\n \n ssize_t read_istream(struct git_istream *st, char *buf, size_t sz)\n-- \n1.7.6.rc1.12.g65e2\n"},{"id":"171866","messageId":"7vk4baxas5.fsf@alter.siamese.dyndns.org","threadId":"27886","inReplyTo":"20110722203614.GA13232@sigill.intra.peff.net","subject":"Re: [REGRESSION, BISECTED] `git checkout ` started to be memory hog","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-07-22T21:31:22Z","receivedAt":"2011-07-22T21:31:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> How about:\n>\n> -- >8 --\n> Subject: [PATCH] streaming: free git_istream upon closing\n>\n> Kirill Smelkov noticed that post-1.7.6 \"git checkout\"\n> started leaking tons of memory. The streaming_write_entry\n> function properly calls close_istream(), but that function\n> did not actually free() the allocated git_istream struct.\n>\n> The git_istream struct is totally opaque to calling code,\n> and must be heap-allocated by open_istream. Therefore it's\n> not appropriate for callers to have to free it.\n>\n> This patch makes close_istream() into \"close and de-allocate\n> all associated resources\". We could add a new \"free_istream\"\n> call, but there's not much point in letting callers inspect\n> the istream after close. And this patch's semantics make us\n> match fopen/fclose, which is well-known and understood.\n>\n> Signed-off-by: Jeff King \n\nMuch nicer ;-) Thanks.\n"},{"id":"171895","messageId":"20110723180411.GA16463@tugrik.mns.mnsspb.ru","threadId":"27886","inReplyTo":"7vk4baxas5.fsf@alter.siamese.dyndns.org","subject":"Re: [REGRESSION, BISECTED] `git checkout ` started to be memory hog","fromName":"Kirill Smelkov","fromEmail":"kirr@mns.spb.ru","sentAt":"2011-07-23T18:04:11Z","receivedAt":"2011-07-23T18:04:11Z","isPatch":false,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"On Fri, Jul 22, 2011 at 02:31:22PM -0700, Junio C Hamano wrote:\n> Jeff King writes:\n> \n> > How about:\n> >\n> > -- >8 --\n> > Subject: [PATCH] streaming: free git_istream upon closing\n> >\n> > Kirill Smelkov noticed that post-1.7.6 \"git checkout\"\n> > started leaking tons of memory. The streaming_write_entry\n> > function properly calls close_istream(), but that function\n> > did not actually free() the allocated git_istream struct.\n> >\n> > The git_istream struct is totally opaque to calling code,\n> > and must be heap-allocated by open_istream. Therefore it's\n> > not appropriate for callers to have to free it.\n> >\n> > This patch makes close_istream() into \"close and de-allocate\n> > all associated resources\". We could add a new \"free_istream\"\n> > call, but there's not much point in letting callers inspect\n> > the istream after close. And this patch's semantics make us\n> > match fopen/fclose, which is well-known and understood.\n> >\n> > Signed-off-by: Jeff King \n> \n> Much nicer ;-) Thanks.\n\nJeff, Junio,\n\nThanks for fixing this.\n\n\nKirill\n"}]}