Re: [patch] possible memory leak in diff.c::diff_free_filepair()
- From
Junio C Hamano <junkio@cox.net>
- Date
- Aug 13, 2005, 21:31 UTC
- Message-ID
- <7viry9le0g.fsf@assigned-by-dhcp.cox.net>
- In-Reply-To
- <877jepo87m.wl@mail2.atmark-techno.com>
Yasushi SHOJI <yashi@atmark-techno.com> writes:
> oops. probably my english wasn't clear. my patch fixes > diff_free_filepair().
No, my reading of your patch when I wrote that message was wrong. The attached is what I ended up doing based on your patch. It does not seem to barf with the following test on either git repository itself nor recent linux-2.6 repository, which is a good sign.
$ export MALLOC_CHECK_=2
$ ./git-rev-list HEAD |
./git-diff-tree --stdin -r -B -C --find-copies-harder |
sed -ne '/^[^:]/p;/ [MRCDA][0-9][0-9]* /p'When the command is run on linux-2.6 repository, virtual memory consumption of git-diff-tree command skyrockets to about half a gig, because it maps all files in two adjacent revisions of the entire kernel tree. But it seems to reclaim things reasonably well and goes back down to less than 10m when it starts to process the next commit pair.
------------ From: Yasushi SHOJI <yashi@atmark-techno.com> Date: 1123930736 +0900 [PATCH] plug memory leak in diff.c::diff_free_filepair()
When I run git-diff-tree on big change, it seems the command eats so much memory. so I just put git under valgrind to see what's going on. diff_free_filespec_data() doesn't free diff_filespec itself.
[jc: I ended up doing things slightly differently from Yasushi's patch. The original idea was to use free_filespec_data() only to free the data portion and keep useing the filespec itself, but no existing code seems to do things that way, so I just yanked that part out.]
Signed-off-by: Yasushi SHOJI <yashi@atmark-techno.com> Signed-off-by: Junio C Hamano <junkio@cox.net> ---
diff.c | 9 ++++----- diffcore-break.c | 4 ++-- diffcore.h | 2 +- 3 files changed, 7 insertions(+), 8 deletions(-)
20226fa40d48069b55cf165c9e197a003e1608a8
diff --git a/diff.c b/diff.c --- a/diff.c +++ b/diff.c @@ -405,14 +405,13 @@ int diff_populate_filespec(struct diff_f return 0; } -void diff_free_filespec_data(struct diff_filespec *s) +void diff_free_filespec(struct diff_filespec *s) { if (s->should_free) free(s->data); else if (s->should_munmap) munmap(s->data, s->size); - s->should_free = s->should_munmap = 0; - s->data = NULL; + free(s); } static void prep_temp_blob(struct diff_tempfile *temp, @@ -769,8 +768,8 @@ struct diff_filepair *diff_queue(struct void diff_free_filepair(struct diff_filepair *p) { - diff_free_filespec_data(p->one); - diff_free_filespec_data(p->two); + diff_free_filespec(p->one); + diff_free_filespec(p->two); free(p); } diff --git a/diffcore-break.c b/diffcore-break.c --- a/diffcore-break.c +++ b/diffcore-break.c @@ -231,8 +231,8 @@ static void merge_broken(struct diff_fil dp = diff_queue(outq, d->one, c->two); dp->score = p->score; - diff_free_filespec_data(d->two); - diff_free_filespec_data(c->one); + diff_free_filespec(d->two); + diff_free_filespec(c->one); free(d); free(c); } diff --git a/diffcore.h b/diffcore.h --- a/diffcore.h +++ b/diffcore.h @@ -43,7 +43,7 @@ extern void fill_filespec(struct diff_fi unsigned short); extern int diff_populate_filespec(struct diff_filespec *, int); -extern void diff_free_filespec_data(struct diff_filespec *); +extern void diff_free_filespec(struct diff_filespec *); struct diff_filepair { struct diff_filespec *one;