threads / discuss / 33677

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

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

## tl;dr

7 messages between Apr 30, 2013 and May 9, 2013.

replies: 6people: 3as markdown or json

Stephen Boyd· Apr 30, 2013, 00:11 UTC · lore

(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

This is against recent git.git (89740333e8d398f1da701e9023675321bbb9a85b). A workaround is to limit the amount of commits per cherry-pick invocation, but can we somehow fix the leaks?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Junio C Hamano· Apr 30, 2013, 18:31 UTC · re: Stephen Boyd · lore

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

Stephen Boyd <sboyd@codeaurora.org> writes:

Show 8 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

Unfortunately, I am not very surprised.

The merge-recursive machinery was designed in the "run once and let exit() clean up after ourselves" manner, which lets it not even having to worry about keeping track of what needs to be cleaned up. Reusing it inside cherry-pick and revert without updating it was OK, but extending cherry-pick and revert to take more than one change without addressing its resource management was a large mistake.

I vaguely recall suggesting to fork and perform a three-way merge in a separate process when operating on more than one commit when this feature was first discussed. We may have to do something like that.

René Scharfe· Apr 30, 2013, 22:47 UTC · re: Stephen Boyd · lore

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

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)

Stephen Boyd· May 1, 2013, 18:27 UTC · re: René Scharfe · lore

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

On 04/30/13 15:47, René Scharfe wrote:

Show 47 quoted lines

> Am 30.04.2013 02:11, schrieb Stephen Boyd:
>> (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?

Ok I think I will make a copy of my .git first before I try out your patch. In case you're curious here are the next big leaks.

==7986== 433,116,790 (432,950,308 direct, 166,482 indirect) bytes in 4,146,402 blocks are definitely lost in loss record 253 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 0x4F6FAE: unpack_trees (unpack-trees.c:1074) ==7986== by 0x4AD095: git_merge_trees (merge-recursive.c:241) ==7986== by 0x4AF3D5: merge_trees (merge-recursive.c:1811) ==7986== by 0x4D9A7A: do_pick_commit (sequencer.c:311) ==7986== ==7986== 482,083,201 (46,465,928 direct, 435,617,273 indirect) bytes in 93 blocks are definitely lost in loss record 255 of 257 ==7986== at 0x4C267CC: calloc (vg_replace_malloc.c:467) ==7986== by 0x4FAF57: xcalloc (wrapper.c:119) ==7986== by 0x4C4AE4: read_index_from (read-cache.c:1452) ==7986== by 0x4D99BC: do_pick_commit (sequencer.c:297) ==7986== by 0x4DA750: pick_commits (sequencer.c:995) ==7986== by 0x4DAFD6: sequencer_pick_revisions (sequencer.c:1124) ==7986== by 0x463E7C: cmd_cherry_pick (revert.c:236) ==7986== by 0x404C86: handle_internal_command (git.c:284) ==7986== by 0x40541C: main (git.c:492) ==7986== ==7986== 557,706,880 (548,062,684 direct, 9,644,196 indirect) bytes in 4,931,819 blocks are definitely lost in loss record 256 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 0x4F6FAE: unpack_trees (unpack-trees.c:1074) ==7986== by 0x4AD095: git_merge_trees (merge-recursive.c:241) ==7986==

Full report can be found at https://gist.github.com/bebarino/5497181

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Stephen Boyd· May 6, 2013, 20:16 UTC · re: René Scharfe · lore

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

On 04/30/13 15:47, René Scharfe wrote:

Show 47 quoted lines

> Am 30.04.2013 02:11, schrieb Stephen Boyd:
>> (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?

Ok. I tested it and it definitely helps.

==10728== LEAK SUMMARY: ==10728== definitely lost: 316,355,458 bytes in 8,652 blocks ==10728== indirectly lost: 1,327,251,588 bytes in 16,180,628 blocks ==10728== possibly lost: 677,049,918 bytes in 7,381,801 blocks ==10728== still reachable: 9,238,039 bytes in 63,947 blocks ==10728== suppressed: 0 bytes in 0 blocks

vs.

==27614== LEAK SUMMARY: ==27614== definitely lost: 2,369,692,222 bytes in 20,005,707 blocks ==27614== indirectly lost: 829,151,786 bytes in 9,594,715 blocks ==27614== possibly lost: 658,069,373 bytes in 6,345,172 blocks ==27614== still reachable: 8,806,386 bytes in 50,199 blocks ==27614== suppressed: 0 bytes in 0 blocks

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

René Scharfe· May 9, 2013, 15:10 UTC · re: Stephen Boyd · lore

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

Am 06.05.2013 22:16, schrieb Stephen Boyd:

Show 17 quoted lines

> Ok. I tested it and it definitely helps.
>
> ==10728== LEAK SUMMARY:
> ==10728==    definitely lost: 316,355,458 bytes in 8,652 blocks
> ==10728==    indirectly lost: 1,327,251,588 bytes in 16,180,628 blocks
> ==10728==      possibly lost: 677,049,918 bytes in 7,381,801 blocks
> ==10728==    still reachable: 9,238,039 bytes in 63,947 blocks
> ==10728==         suppressed: 0 bytes in 0 blocks
>
> vs.
>
> ==27614== LEAK SUMMARY:
> ==27614==    definitely lost: 2,369,692,222 bytes in 20,005,707 blocks
> ==27614==    indirectly lost: 829,151,786 bytes in 9,594,715 blocks
> ==27614==      possibly lost: 658,069,373 bytes in 6,345,172 blocks
> ==27614==    still reachable: 8,806,386 bytes in 50,199 blocks
> ==27614==         suppressed: 0 bytes in 0 blocks

Thanks, Stephen. I'm going to prepare a series around that patch which will (hopefully) show that freeing these entries is safe by passing only const pointers down to the callbacks. It's too late for 1.8.3, of course, but it shouldn't take another year as most of that series is done already. :)

We still have an impressive amount of leakage here. I wonder why "indirectly lost" increased so much. Do you perhaps still have the full output of valgrind for the run with the patch applied?

Thanks, René

Stephen Boyd· May 9, 2013, 17:24 UTC · re: René Scharfe · lore

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

On 05/09/13 08:10, René Scharfe wrote:

Show 24 quoted lines

> Am 06.05.2013 22:16, schrieb Stephen Boyd:
>> Ok. I tested it and it definitely helps.
>>
>> ==10728== LEAK SUMMARY:
>> ==10728==    definitely lost: 316,355,458 bytes in 8,652 blocks
>> ==10728==    indirectly lost: 1,327,251,588 bytes in 16,180,628 blocks
>> ==10728==      possibly lost: 677,049,918 bytes in 7,381,801 blocks
>> ==10728==    still reachable: 9,238,039 bytes in 63,947 blocks
>> ==10728==         suppressed: 0 bytes in 0 blocks
>>
>> vs.
>>
>> ==27614== LEAK SUMMARY:
>> ==27614==    definitely lost: 2,369,692,222 bytes in 20,005,707 blocks
>> ==27614==    indirectly lost: 829,151,786 bytes in 9,594,715 blocks
>> ==27614==      possibly lost: 658,069,373 bytes in 6,345,172 blocks
>> ==27614==    still reachable: 8,806,386 bytes in 50,199 blocks
>> ==27614==         suppressed: 0 bytes in 0 blocks
>
> Thanks, Stephen.  I'm going to prepare a series around that patch
> which will (hopefully) show that freeing these entries is safe by
> passing only const pointers down to the callbacks.  It's too late for
> 1.8.3, of course, but it shouldn't take another year as most of that
> series is done already. :)

Yes I started trying to throw const around everywhere but then I just sent out the email in hopes someone had already solved this problem.

>
> We still have an impressive amount of leakage here.  I wonder why
> "indirectly lost" increased so much.  Do you perhaps still have the
> full output of valgrind for the run with the patch applied?

I think it increased because the first one ran to completion while the second one failed half way through so it's not an apples to apples comparison. I can re-run with a certain range that is known not to fail if you're interested, but I think you got the idea that the patch helps.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation