Huge possible memory leak while cherry-picking.

Huge possible memory leak while cherry-picking.

[Лежанкин Иван]

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?

Re: Huge possible memory leak while cherry-picking.

[Felipe Contreras]

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

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:
> 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

Re: Huge possible memory leak while cherry-picking.

[Felipe Contreras]

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

Re: Huge possible memory leak while cherry-picking.

[Felipe Contreras]

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

[PATCH] unpack-trees: plug a memory leak

[René Scharfe]

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

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

[Junio C Hamano]

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 assume that this is signed-off by you as a forwarder?  I'd prefer
to even mark it Reviewed-by: you.

Thanks.

>  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);

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

[René Scharfe]

Am 13.08.2013 23:12, schrieb Junio C Hamano:
> 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é

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

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

>> 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.