threads / discuss / 34652

Huge possible memory leak while cherry-picking.

Subject: Huge possible memory leak while cherry-picking.

## tl;dr

9 messages between Aug 9, 2013 and Aug 13, 2013.

replies: 8people: 4as markdown or json

Лежанкин Иван· Aug 9, 2013, 12:13 UTC · lore
Hi,

I have tried to cherry-pick a range of ~200 commits from one branch to another. And you can't imagine how I was surprised when the git process ate 8 Gb of RAM and died - before cherry-picking was complete.

I downloaded git sources from master and built it with gperftools support (-ltcmalloc). After running `git cherry-pick <some hash>` with a heap-leak checker enabled I got this:

> Have memory regions w/o callers: might report false leaks
> Leak check _main_ detected leaks of 42838782 bytes in 257986 objects
These objects are allocated at
> read-cache.c:1340: struct cache_entry *ce = xmalloc(cache_entry_size(len));

After looking in the code, I found a comment in the function `static void remove_dir_entry(...)`:

/*
 * Release reference to the directory entry (and parents if 0).
 *
 * Note: we do not remove / free the entry because there's no
 * hash.[ch]::remove_hash and dir->next may point to other entries
 * that are still valid, so we must not free the memory.
 */

So, this objects are never freed - by design? Is it a real issue, or do I just misunderstand something?

Felipe Contreras· Aug 9, 2013, 20:39 UTC · re: Лежанкин Иван · lore

Re: Huge possible memory leak while cherry-picking.

On Fri, Aug 9, 2013 at 7:13 AM, Лежанкин Иван <abyss.7@gmail.com> wrote:
> I have tried to cherry-pick a range of ~200 commits from one branch to
> another. And you can't imagine how I was surprised when the git
> process ate 8 Gb of RAM and died - before cherry-picking was complete.

Try this: http://article.gmane.org/gmane.comp.version-control.git/226757

-- 
Felipe Contreras
Лежанкин Иван· Aug 12, 2013, 10:04 UTC · re: Felipe Contreras · lore

Re: Huge possible memory leak while cherry-picking.

Thank you, it works very well! Will this patch go to upstream?

Also, there is still some unexpected memory consumption - about 2Gb per ~200 commits, but it's bearable. I will do a futher investigation.

Felipe, should I exclude you from my futher reports on possible memory leaks?
On 10 August 2013 00:39, Felipe Contreras <felipe.contreras@gmail.com> wrote:
Show 10 quoted lines
> On Fri, Aug 9, 2013 at 7:13 AM, Лежанкин Иван <abyss.7@gmail.com> wrote:
>> I have tried to cherry-pick a range of ~200 commits from one branch to
>> another. And you can't imagine how I was surprised when the git
>> process ate 8 Gb of RAM and died - before cherry-picking was complete.
>
> Try this:
> http://article.gmane.org/gmane.comp.version-control.git/226757
>
> --
> Felipe Contreras
Felipe Contreras· Aug 12, 2013, 10:09 UTC · re: Лежанкин Иван · lore

Re: Huge possible memory leak while cherry-picking.

On Mon, Aug 12, 2013 at 5:04 AM, Лежанкин Иван <abyss.7@gmail.com> wrote:
> Thank you, it works very well!
> Will this patch go to upstream?
Ask Junio.
> Also, there is still some unexpected memory consumption - about 2Gb
> per ~200 commits, but it's bearable.
> I will do a futher investigation.
Can you post some valgrind log? Or even better, a way to reproduce?
> Felipe, should I exclude you from my futher reports on possible memory leaks?
Exclude me?
-- 
Felipe Contreras
René Scharfe· Aug 13, 2013, 18:27 UTC · re: Felipe Contreras · lore

[PATCH] unpack-trees: plug a memory leak

From: Felipe Contreras <felipe.contreras@gmail.com>

Before overwriting the destination index, first let's discard its contents.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Tested-by: Лежанкин Иван <abyss.7@gmail.com> wrote:
---
Felipe sent this patch as part of multiple series in June, but it can
stand on its own.  This version is trivially rebased against master.
The leak seems to have been introduced by 34110cd4 (2008-03-06,
"Make 'unpack_trees()' have a separate source and destination index").
 unpack-trees.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index bf01717..1a61e6f 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1154,8 +1154,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 
 	o->src_index = NULL;
 	ret = check_updates(o) ? (-2) : 0;
-	if (o->dst_index)
+	if (o->dst_index) {
+		discard_index(o->dst_index);
 		*o->dst_index = o->result;
+	}
 
 done:
 	clear_exclude_list(&el);

-- 
1.8.3.3
Junio C Hamano· Aug 13, 2013, 21:12 UTC · re: René Scharfe · lore

Re: [PATCH] unpack-trees: plug a memory leak

René Scharfe <l.s.r@web.de> writes:
Show 12 quoted lines
> From: Felipe Contreras <felipe.contreras@gmail.com>
>
> Before overwriting the destination index, first let's discard its
> contents.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> Tested-by: Лежанкин Иван <abyss.7@gmail.com> wrote:
> ---
> Felipe sent this patch as part of multiple series in June, but it can
> stand on its own.  This version is trivially rebased against master.
> The leak seems to have been introduced by 34110cd4 (2008-03-06,
> "Make 'unpack_trees()' have a separate source and destination index").
It was lost in the follow-up discussion and I missed it.

I assume that this is signed-off by you as a forwarder? I'd prefer to even mark it Reviewed-by: you.

Thanks.
Show 19 quoted lines
>  unpack-trees.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index bf01717..1a61e6f 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -1154,8 +1154,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>  
>  	o->src_index = NULL;
>  	ret = check_updates(o) ? (-2) : 0;
> -	if (o->dst_index)
> +	if (o->dst_index) {
> +		discard_index(o->dst_index);
>  		*o->dst_index = o->result;
> +	}
>  
>  done:
>  	clear_exclude_list(&el);
René Scharfe· Aug 13, 2013, 21:32 UTC · re: Junio C Hamano · lore

Re: [PATCH] unpack-trees: plug a memory leak

Am 13.08.2013 23:12, schrieb Junio C Hamano:
Show 16 quoted lines
> René Scharfe <l.s.r@web.de> writes:
>
>> From: Felipe Contreras <felipe.contreras@gmail.com>
>>
>> Before overwriting the destination index, first let's discard its
>> contents.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> Tested-by: Лежанкин Иван <abyss.7@gmail.com> wrote:
>> ---
>> Felipe sent this patch as part of multiple series in June, but it can
>> stand on its own.  This version is trivially rebased against master.
>> The leak seems to have been introduced by 34110cd4 (2008-03-06,
>> "Make 'unpack_trees()' have a separate source and destination index").
>
> It was lost in the follow-up discussion and I missed it.
I had forgotten about it as well, until Felipe mentioned it again.
> I assume that this is signed-off by you as a forwarder?  I'd prefer
> to even mark it Reviewed-by: you.
Right, I did review the patch and you can tag it as such.

Thanks, René

Junio C Hamano· Aug 13, 2013, 21:50 UTC · re: René Scharfe · lore

Re: [PATCH] unpack-trees: plug a memory leak

René Scharfe <l.s.r@web.de> writes:
Show 8 quoted lines
>> It was lost in the follow-up discussion and I missed it.
>
> I had forgotten about it as well, until Felipe mentioned it again.
>
>> I assume that this is signed-off by you as a forwarder?  I'd prefer
>> to even mark it Reviewed-by: you.
>
> Right, I did review the patch and you can tag it as such.
OK, will queue and soon merge to and cook in 'next'.
Thanks, everybody.
Felipe Contreras· Aug 12, 2013, 10:34 UTC · re: Лежанкин Иван · lore

Re: Huge possible memory leak while cherry-picking.

On Mon, Aug 12, 2013 at 5:04 AM, Лежанкин Иван <abyss.7@gmail.com> wrote:
> Thank you, it works very well!
> Will this patch go to upstream?
Ask Junio.
> Also, there is still some unexpected memory consumption - about 2Gb
> per ~200 commits, but it's bearable.
> I will do a futher investigation.
Can you post some valgrind log? Or even better, a way to reproduce?
> Felipe, should I exclude you from my futher reports on possible memory leaks?
Exclude me?
-- 
Felipe Contreras

← back to recent threads