Re: git rev-list | git cherry-pick --stdin is leaky

From: René Scharfe <rene.scharfe@lsrfire.ath.cx>
Date: Apr 30, 2013, 22:47 UTC
Message-ID: <51804A02.6080301@lsrfire.ath.cx>
In-Reply-To: <517F0C18.8060703@codeaurora.org>

Am 30.04.2013 02:11, schrieb Stephen Boyd:

Show 35 quoted lines

> (resending since the attachment seems to make vger sad)
> 
> Hi,
> 
> I'm running git rev-list | git cherry-pick --stdin on a range of about
> 300 commits. Eventually the chery-pick dies with:
> 
>      error: cannot fork() for commit: Cannot allocate memory
> 
> Running valgrind shows me that the tree traversal code is leaking
> gigabytes of memory (particularly unpack_callback). Since cherry-pick is
> a very long running process all these allocations are never freed and
> eventually I run out of memory. The worst offender and summary is:
> 
> ==7986== 938,956,692 (929,961,582 direct, 8,995,110 indirect) bytes in
> 7,765,439 blocks are definitely lost in loss record 257 of 257
> ==7986==    at 0x4C267CC: calloc (vg_replace_malloc.c:467)
> ==7986==    by 0x4FAF57: xcalloc (wrapper.c:119)
> ==7986==    by 0x4F5281: unpack_callback (unpack-trees.c:539)
> ==7986==    by 0x4F40E5: traverse_trees (tree-walk.c:407)
> ==7986==    by 0x4F586C: unpack_callback (unpack-trees.c:467)
> ==7986==    by 0x4F40E5: traverse_trees (tree-walk.c:407)
> ==7986==    by 0x4F586C: unpack_callback (unpack-trees.c:467)
> ==7986==    by 0x4F40E5: traverse_trees (tree-walk.c:407)
> ==7986==    by 0x4F586C: unpack_callback (unpack-trees.c:467)
> ==7986==    by 0x4F40E5: traverse_trees (tree-walk.c:407)
> ==7986==    by 0x4F586C: unpack_callback (unpack-trees.c:467)
> ==7986==    by 0x4F40E5: traverse_trees (tree-walk.c:407)
> ==7986==
> ==7986== LEAK SUMMARY:
> ==7986==    definitely lost: 2,514,117,692 bytes in 21,210,861 blocks
> ==7986==    indirectly lost: 885,481,947 bytes in 10,165,801 blocks
> ==7986==      possibly lost: 650,712,395 bytes in 6,014,309 blocks
> ==7986==    still reachable: 7,734,870 bytes in 47,794 blocks
> ==7986==         suppressed: 0 bytes in 0 blocks

I looked at that particular leak a year ago but couldn't convince myself to submit the patch below. If the callback function we call through call_unpack_fn does something strange like free()ing entries itself or adding them to some list without duplication then the added free() can cause trouble.

Looking at it again today I don't understand that concern any more. The current callback functions don't do something like that, in any case. Maybe I'm missing something.

Anyway, could you please check if the patch helps with your use case?

Thanks, René

diff --git a/unpack-trees.c b/unpack-trees.c
index ede4299..67bc698 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -590,8 +590,13 @@ static int unpack_nondirectories(int n, unsigned long mask,
 		src[i + o->merge] = create_ce_entry(info, names + i, stage);
 	}
 
-	if (o->merge)
-		return call_unpack_fn(src, o);
+	if (o->merge) {
+		int ret = call_unpack_fn(src, o);
+		for (i = 1; i < n + 1; i++)
+			if (src[i] && src[i] != o->df_conflict_entry)
+				free(src[i]);
+		return ret;
+	}
 
 	for (i = 0; i < n; i++)
 		if (src[i] && src[i] != o->df_conflict_entry)