{"thread":{"id":"34652","subject":"Huge possible memory leak while cherry-picking.","startedAt":"2013-08-09T12:13:17Z","lastAt":"2013-08-13T21:50:06Z","messageCount":9,"participants":["Лежанкин Иван","Felipe Contreras","René Scharfe","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"224894","messageId":"CAJc7LbpRuqug9pLFVVg=XMvJ9u_P0ZVSy2MVBDaCVkjvfKnfJw@mail.gmail.com","threadId":"34652","inReplyTo":null,"subject":"Huge possible memory leak while cherry-picking.","fromName":"Лежанкин Иван","fromEmail":"abyss.7@gmail.com","sentAt":"2013-08-09T12:13:17Z","receivedAt":"2013-08-09T12:13:17Z","isPatch":false,"sender":{"key":"abyss.7@gmail.com","avatar":"https://gravatar.com/avatar/7bf07a992c48466713f8d8bdb589ee968bfd768559731e349aac5889079b348e?d=mp&s=160"},"body":"Hi,\n\nI have tried to cherry-pick a range of ~200 commits from one branch to\nanother. And you can't imagine how I was surprised when the git\nprocess ate 8 Gb of RAM and died - before cherry-picking was complete.\n\nI downloaded git sources from master and built it with gperftools\nsupport (-ltcmalloc). After running `git cherry-pick ` with\na heap-leak checker enabled I got this:\n\n> Have memory regions w/o callers: might report false leaks\n> Leak check _main_ detected leaks of 42838782 bytes in 257986 objects\n\nThese objects are allocated at\n\n> read-cache.c:1340: struct cache_entry *ce = xmalloc(cache_entry_size(len));\n\nAfter looking in the code, I found a comment in the function `static\nvoid remove_dir_entry(...)`:\n\n/*\n * Release reference to the directory entry (and parents if 0).\n *\n * Note: we do not remove / free the entry because there's no\n * hash.[ch]::remove_hash and dir->next may point to other entries\n * that are still valid, so we must not free the memory.\n */\n\nSo, this objects are never freed - by design?\nIs it a real issue, or do I just misunderstand something?\n"},{"id":"224950","messageId":"CAMP44s282DD+tQUgVHawdRDJayjTxMjOu_R3robbCVhkbksEtQ@mail.gmail.com","threadId":"34652","inReplyTo":"CAJc7LbpRuqug9pLFVVg=XMvJ9u_P0ZVSy2MVBDaCVkjvfKnfJw@mail.gmail.com","subject":"Re: Huge possible memory leak while cherry-picking.","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-08-09T20:39:04Z","receivedAt":"2013-08-09T20:39:04Z","isPatch":false,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Fri, Aug 9, 2013 at 7:13 AM, Лежанкин Иван wrote:\n> I have tried to cherry-pick a range of ~200 commits from one branch to\n> another. And you can't imagine how I was surprised when the git\n> process ate 8 Gb of RAM and died - before cherry-picking was complete.\n\nTry this:\nhttp://article.gmane.org/gmane.comp.version-control.git/226757\n\n-- \nFelipe Contreras\n"},{"id":"225098","messageId":"CAJc7Lbrmsna4u4s+fdCGZ7jn9HzgZkinL3tbjbjcuw40Of5umg@mail.gmail.com","threadId":"34652","inReplyTo":"CAMP44s282DD+tQUgVHawdRDJayjTxMjOu_R3robbCVhkbksEtQ@mail.gmail.com","subject":"Re: Huge possible memory leak while cherry-picking.","fromName":"Лежанкин Иван","fromEmail":"abyss.7@gmail.com","sentAt":"2013-08-12T10:04:41Z","receivedAt":"2013-08-12T10:04:41Z","isPatch":false,"sender":{"key":"abyss.7@gmail.com","avatar":"https://gravatar.com/avatar/7bf07a992c48466713f8d8bdb589ee968bfd768559731e349aac5889079b348e?d=mp&s=160"},"body":"Thank you, it works very well!\nWill this patch go to upstream?\n\nAlso, there is still some unexpected memory consumption - about 2Gb\nper ~200 commits, but it's bearable.\nI will do a futher investigation.\n\nFelipe, should I exclude you from my futher reports on possible memory leaks?\n\nOn 10 August 2013 00:39, Felipe Contreras wrote:\n> On Fri, Aug 9, 2013 at 7:13 AM, Лежанкин Иван wrote:\n>> I have tried to cherry-pick a range of ~200 commits from one branch to\n>> another. And you can't imagine how I was surprised when the git\n>> process ate 8 Gb of RAM and died - before cherry-picking was complete.\n>\n> Try this:\n> http://article.gmane.org/gmane.comp.version-control.git/226757\n>\n> --\n> Felipe Contreras\n"},{"id":"225099","messageId":"CAMP44s1CAMPWXDSAc7WHahmrKRrB8aG_H9fnXAMi2LFOGy5EdA@mail.gmail.com","threadId":"34652","inReplyTo":"CAJc7Lbrmsna4u4s+fdCGZ7jn9HzgZkinL3tbjbjcuw40Of5umg@mail.gmail.com","subject":"Re: Huge possible memory leak while cherry-picking.","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-08-12T10:09:44Z","receivedAt":"2013-08-12T10:09:44Z","isPatch":false,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, Aug 12, 2013 at 5:04 AM, Лежанкин Иван wrote:\n> Thank you, it works very well!\n> Will this patch go to upstream?\n\nAsk Junio.\n\n> Also, there is still some unexpected memory consumption - about 2Gb\n> per ~200 commits, but it's bearable.\n> I will do a futher investigation.\n\nCan you post some valgrind log? Or even better, a way to reproduce?\n\n> Felipe, should I exclude you from my futher reports on possible memory leaks?\n\nExclude me?\n\n-- \nFelipe Contreras\n"},{"id":"225100","messageId":"CAMP44s3SMwQseKbQ8dvYKK41WCWwejRhTqBmJQPCJP2XNG1mLQ@mail.gmail.com","threadId":"34652","inReplyTo":"CAJc7Lbrmsna4u4s+fdCGZ7jn9HzgZkinL3tbjbjcuw40Of5umg@mail.gmail.com","subject":"Re: Huge possible memory leak while cherry-picking.","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-08-12T10:34:16Z","receivedAt":"2013-08-12T10:34:16Z","isPatch":false,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, Aug 12, 2013 at 5:04 AM, Лежанкин Иван wrote:\n> Thank you, it works very well!\n> Will this patch go to upstream?\n\nAsk Junio.\n\n> Also, there is still some unexpected memory consumption - about 2Gb\n> per ~200 commits, but it's bearable.\n> I will do a futher investigation.\n\nCan you post some valgrind log? Or even better, a way to reproduce?\n\n> Felipe, should I exclude you from my futher reports on possible memory leaks?\n\nExclude me?\n\n-- \nFelipe Contreras\n"},{"id":"225161","messageId":"520A7AAE.6010309@web.de","threadId":"34652","inReplyTo":"CAMP44s1CAMPWXDSAc7WHahmrKRrB8aG_H9fnXAMi2LFOGy5EdA@mail.gmail.com","subject":"[PATCH] unpack-trees: plug a memory leak","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2013-08-13T18:27:58Z","receivedAt":"2013-08-13T18:27:58Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"From: Felipe Contreras \n\nBefore overwriting the destination index, first let's discard its\ncontents.\n\nSigned-off-by: Felipe Contreras \nTested-by: Лежанкин Иван wrote:\n---\nFelipe sent this patch as part of multiple series in June, but it can\nstand on its own. This version is trivially rebased against master.\nThe leak seems to have been introduced by 34110cd4 (2008-03-06,\n\"Make 'unpack_trees()' have a separate source and destination index\").\n\n unpack-trees.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex bf01717..1a61e6f 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1154,8 +1154,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \n \to->src_index = NULL;\n \tret = check_updates(o) ? (-2) : 0;\n-\tif (o->dst_index)\n+\tif (o->dst_index) {\n+\t\tdiscard_index(o->dst_index);\n \t\t*o->dst_index = o->result;\n+\t}\n \n done:\n \tclear_exclude_list(&el);\n-- \n1.8.3.3\n"},{"id":"225166","messageId":"7va9klwb03.fsf@alter.siamese.dyndns.org","threadId":"34652","inReplyTo":"520A7AAE.6010309@web.de","subject":"Re: [PATCH] unpack-trees: plug a memory leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-13T21:12:44Z","receivedAt":"2013-08-13T21:12:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> From: Felipe Contreras \n>\n> Before overwriting the destination index, first let's discard its\n> contents.\n>\n> Signed-off-by: Felipe Contreras \n> Tested-by: Лежанкин Иван wrote:\n> ---\n> Felipe sent this patch as part of multiple series in June, but it can\n> stand on its own. This version is trivially rebased against master.\n> The leak seems to have been introduced by 34110cd4 (2008-03-06,\n> \"Make 'unpack_trees()' have a separate source and destination index\").\n\nIt was lost in the follow-up discussion and I missed it.\n\nI assume that this is signed-off by you as a forwarder? I'd prefer\nto even mark it Reviewed-by: you.\n\nThanks.\n\n> unpack-trees.c | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index bf01717..1a61e6f 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -1154,8 +1154,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n> \n> \to->src_index = NULL;\n> \tret = check_updates(o) ? (-2) : 0;\n> -\tif (o->dst_index)\n> +\tif (o->dst_index) {\n> +\t\tdiscard_index(o->dst_index);\n> \t\t*o->dst_index = o->result;\n> +\t}\n> \n> done:\n> \tclear_exclude_list(&el);\n"},{"id":"225167","messageId":"520AA5FE.1090208@web.de","threadId":"34652","inReplyTo":"7va9klwb03.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] unpack-trees: plug a memory leak","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2013-08-13T21:32:46Z","receivedAt":"2013-08-13T21:32:46Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 13.08.2013 23:12, schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n>> From: Felipe Contreras \n>>\n>> Before overwriting the destination index, first let's discard its\n>> contents.\n>>\n>> Signed-off-by: Felipe Contreras \n>> Tested-by: Лежанкин Иван wrote:\n>> ---\n>> Felipe sent this patch as part of multiple series in June, but it can\n>> stand on its own. This version is trivially rebased against master.\n>> The leak seems to have been introduced by 34110cd4 (2008-03-06,\n>> \"Make 'unpack_trees()' have a separate source and destination index\").\n>\n> It was lost in the follow-up discussion and I missed it.\n\nI had forgotten about it as well, until Felipe mentioned it again.\n\n> I assume that this is signed-off by you as a forwarder? I'd prefer\n> to even mark it Reviewed-by: you.\n\nRight, I did review the patch and you can tag it as such.\n\nThanks,\nRené\n"},{"id":"225169","messageId":"7v1u5xw99t.fsf@alter.siamese.dyndns.org","threadId":"34652","inReplyTo":"520AA5FE.1090208@web.de","subject":"Re: [PATCH] unpack-trees: plug a memory leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-13T21:50:06Z","receivedAt":"2013-08-13T21:50:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n>> It was lost in the follow-up discussion and I missed it.\n>\n> I had forgotten about it as well, until Felipe mentioned it again.\n>\n>> I assume that this is signed-off by you as a forwarder? I'd prefer\n>> to even mark it Reviewed-by: you.\n>\n> Right, I did review the patch and you can tag it as such.\n\nOK, will queue and soon merge to and cook in 'next'.\n\nThanks, everybody.\n"}]}