{"thread":{"id":"1496","subject":"[patch] possible memory leak in diff.c::diff_free_filepair()","startedAt":"2005-08-13T10:58:56Z","lastAt":"2005-08-21T07:14:16Z","messageCount":7,"participants":["Yasushi SHOJI","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"7185","messageId":"87y876gl1r.wl@mail2.atmark-techno.com","threadId":"1496","inReplyTo":null,"subject":"[patch] possible memory leak in diff.c::diff_free_filepair()","fromName":"Yasushi SHOJI","fromEmail":"yashi@atmark-techno.com","sentAt":"2005-08-13T10:58:56Z","receivedAt":"2005-08-13T10:58:56Z","isPatch":true,"sender":{"key":"yashi@atmark-techno.com","avatar":"https://gravatar.com/avatar/4817e8703ac4379935834d87453faa9d0c94b9dc19d83fcc54c67875eb133e59?d=mp&s=160"},"body":"Hi all,\n\nWhen I run git-diff-tree on big change, it seems the command eats so\nmuch memory. so I just put git under valgrind to see what's going on.\n\nhere is the output:\n\n==26475== 63816 bytes in 766 blocks are definitely lost in loss record 7 of 7\n==26475== at 0x1B8FF896: malloc (vg_replace_malloc.c:149)\n==26475== by 0x805203B: alloc_filespec (diff.c:214)\n==26475== by 0x80528C5: diff_addremove (diff.c:1141)\n==26475== by 0x8049C7A: show_file (diff-tree.c:97)\n==26475== by 0x8049D63: show_file (diff-tree.c:206)\n==26475== by 0x8049D63: show_file (diff-tree.c:206)\n==26475== by 0x8049D63: show_file (diff-tree.c:206)\n==26475== by 0x8049EB3: diff_tree (diff-tree.c:118)\n==26475== by 0x804A12E: diff_tree_sha1 (diff-tree.c:260)\n==26475== by 0x804A06E: diff_tree (diff-tree.c:139)\n==26475== by 0x804A12E: diff_tree_sha1 (diff-tree.c:260)\n==26475== by 0x804A06E: diff_tree (diff-tree.c:139)\n==26475== \n==26475== LEAK SUMMARY:\n==26475== definitely lost: 63816 bytes in 766 blocks.\n==26475== possibly lost: 0 bytes in 0 blocks.\n==26475== still reachable: 351 bytes in 6 blocks.\n==26475== suppressed: 0 bytes in 0 blocks.\n\ndiff_free_filespec_data() doesn't free diff_filespec itself. is this\nbecause in merge_broken() filespec itself is used but fliespec data\nneed to be freed?\n\nso I've put one more function, diff_free_filespec(), between\ndiff_free_filepare() and diff_free_filespec_data() call-chain.\n\nresult is:\n\n==27983== LEAK SUMMARY:\n==27983== definitely lost: 0 bytes in 0 blocks.\n==27983== possibly lost: 0 bytes in 0 blocks.\n==27983== still reachable: 276 bytes in 6 blocks.\n==27983== suppressed: 0 bytes in 0 blocks.\n\nSigned-off-by: Yasushi SHOJI \n\n---\ndiff --git a/diff.c b/diff.c\n--- a/diff.c\n+++ b/diff.c\n@@ -767,10 +767,16 @@ struct diff_filepair *diff_queue(struct \n \treturn dp;\n }\n \n+void diff_free_filespec(struct diff_filespec *s)\n+{\n+\tdiff_free_filespec_data(s);\n+ free(s);\n+}\n+\n void diff_free_filepair(struct diff_filepair *p)\n {\n-\tdiff_free_filespec_data(p->one);\n-\tdiff_free_filespec_data(p->two);\n+\tdiff_free_filespec(p->one);\n+\tdiff_free_filespec(p->two);\n \tfree(p);\n }\n \n"},{"id":"7211","messageId":"7viry9my6k.fsf@assigned-by-dhcp.cox.net","threadId":"1496","inReplyTo":"87y876gl1r.wl@mail2.atmark-techno.com","subject":"Re: [patch] possible memory leak in diff.c::diff_free_filepair()","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-08-13T19:30:59Z","receivedAt":"2005-08-13T19:30:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yasushi SHOJI writes:\n\n> When I run git-diff-tree on big change, it seems the command eats so\n> much memory. so I just put git under valgrind to see what's going on.\n>\n> diff_free_filespec_data() doesn't free diff_filespec itself. is this\n> because in merge_broken() filespec itself is used but fliespec data\n> need to be freed?\n\nThanks for the patch. I am wondering if the same leak exists in\ndiff_free_filepair(), which frees the filespec data without\nfreeing filespec itself for both sides. If this is something\nyou can trap easily with valgrind I would really appreciate it.\n"},{"id":"7218","messageId":"877jepo87m.wl@mail2.atmark-techno.com","threadId":"1496","inReplyTo":"7viry9my6k.fsf@assigned-by-dhcp.cox.net","subject":"Re: [patch] possible memory leak in diff.c::diff_free_filepair()","fromName":"Yasushi SHOJI","fromEmail":"yashi@atmark-techno.com","sentAt":"2005-08-13T21:09:01Z","receivedAt":"2005-08-13T21:09:01Z","isPatch":true,"sender":{"key":"yashi@atmark-techno.com","avatar":"https://gravatar.com/avatar/4817e8703ac4379935834d87453faa9d0c94b9dc19d83fcc54c67875eb133e59?d=mp&s=160"},"body":"At Sat, 13 Aug 2005 12:30:59 -0700,\nJunio C Hamano wrote:\n> \n> Yasushi SHOJI writes:\n> \n> > When I run git-diff-tree on big change, it seems the command eats so\n> > much memory. so I just put git under valgrind to see what's going on.\n> >\n> > diff_free_filespec_data() doesn't free diff_filespec itself. is this\n> > because in merge_broken() filespec itself is used but fliespec data\n> > need to be freed?\n> \n> Thanks for the patch. I am wondering if the same leak exists in\n> diff_free_filepair(), which frees the filespec data without\n> freeing filespec itself for both sides. If this is something\n> you can trap easily with valgrind I would really appreciate it.\n\noops. probably my english wasn't clear. my patch fixes\ndiff_free_filepair().\n\nthe reason I asked about merge_broken() was that those two functions\nare the only functions calling diff_free_filespec_data(). it's first\ntime reading git source code and still learning ;)\n--\n yashi\n"},{"id":"7219","messageId":"7viry9le0g.fsf@assigned-by-dhcp.cox.net","threadId":"1496","inReplyTo":"877jepo87m.wl@mail2.atmark-techno.com","subject":"Re: [patch] possible memory leak in diff.c::diff_free_filepair()","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-08-13T21:31:59Z","receivedAt":"2005-08-13T21:31:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yasushi SHOJI writes:\n\n> oops. probably my english wasn't clear. my patch fixes\n> diff_free_filepair().\n\nNo, my reading of your patch when I wrote that message was\nwrong. The attached is what I ended up doing based on your\npatch. It does not seem to barf with the following test on\neither git repository itself nor recent linux-2.6 repository,\nwhich is a good sign.\n\n $ export MALLOC_CHECK_=2\n $ ./git-rev-list HEAD |\n ./git-diff-tree --stdin -r -B -C --find-copies-harder |\n sed -ne '/^[^:]/p;/ [MRCDA][0-9][0-9]*\t/p'\n\nWhen the command is run on linux-2.6 repository, virtual memory\nconsumption of git-diff-tree command skyrockets to about half a\ngig, because it maps all files in two adjacent revisions of the\nentire kernel tree. But it seems to reclaim things reasonably\nwell and goes back down to less than 10m when it starts to\nprocess the next commit pair.\n\n------------\nFrom: Yasushi SHOJI \nDate: 1123930736 +0900\n[PATCH] plug memory leak in diff.c::diff_free_filepair()\n\nWhen I run git-diff-tree on big change, it seems the command eats so\nmuch memory. so I just put git under valgrind to see what's going on.\ndiff_free_filespec_data() doesn't free diff_filespec itself.\n\n[jc: I ended up doing things slightly differently from Yasushi's\npatch. The original idea was to use free_filespec_data() only to\nfree the data portion and keep useing the filespec itself, but\nno existing code seems to do things that way, so I just yanked\nthat part out.]\n\nSigned-off-by: Yasushi SHOJI \nSigned-off-by: Junio C Hamano \n---\n\n diff.c | 9 ++++-----\n diffcore-break.c | 4 ++--\n diffcore.h | 2 +-\n 3 files changed, 7 insertions(+), 8 deletions(-)\n\n20226fa40d48069b55cf165c9e197a003e1608a8\ndiff --git a/diff.c b/diff.c\n--- a/diff.c\n+++ b/diff.c\n@@ -405,14 +405,13 @@ int diff_populate_filespec(struct diff_f\n \treturn 0;\n }\n \n-void diff_free_filespec_data(struct diff_filespec *s)\n+void diff_free_filespec(struct diff_filespec *s)\n {\n \tif (s->should_free)\n \t\tfree(s->data);\n \telse if (s->should_munmap)\n \t\tmunmap(s->data, s->size);\n-\ts->should_free = s->should_munmap = 0;\n-\ts->data = NULL;\n+\tfree(s);\n }\n \n static void prep_temp_blob(struct diff_tempfile *temp,\n@@ -769,8 +768,8 @@ struct diff_filepair *diff_queue(struct \n \n void diff_free_filepair(struct diff_filepair *p)\n {\n-\tdiff_free_filespec_data(p->one);\n-\tdiff_free_filespec_data(p->two);\n+\tdiff_free_filespec(p->one);\n+\tdiff_free_filespec(p->two);\n \tfree(p);\n }\n \ndiff --git a/diffcore-break.c b/diffcore-break.c\n--- a/diffcore-break.c\n+++ b/diffcore-break.c\n@@ -231,8 +231,8 @@ static void merge_broken(struct diff_fil\n \n \tdp = diff_queue(outq, d->one, c->two);\n \tdp->score = p->score;\n-\tdiff_free_filespec_data(d->two);\n-\tdiff_free_filespec_data(c->one);\n+\tdiff_free_filespec(d->two);\n+\tdiff_free_filespec(c->one);\n \tfree(d);\n \tfree(c);\n }\ndiff --git a/diffcore.h b/diffcore.h\n--- a/diffcore.h\n+++ b/diffcore.h\n@@ -43,7 +43,7 @@ extern void fill_filespec(struct diff_fi\n \t\t\t unsigned short);\n \n extern int diff_populate_filespec(struct diff_filespec *, int);\n-extern void diff_free_filespec_data(struct diff_filespec *);\n+extern void diff_free_filespec(struct diff_filespec *);\n \n struct diff_filepair {\n \tstruct diff_filespec *one;\n"},{"id":"7342","messageId":"8764u6ponn.wl@mail2.atmark-techno.com","threadId":"1496","inReplyTo":"7viry9le0g.fsf@assigned-by-dhcp.cox.net","subject":"Re: [patch] possible memory leak in diff.c::diff_free_filepair()","fromName":"Yasushi SHOJI","fromEmail":"yashi@atmark-techno.com","sentAt":"2005-08-16T03:05:16Z","receivedAt":"2005-08-16T03:05:16Z","isPatch":true,"sender":{"key":"yashi@atmark-techno.com","avatar":"https://gravatar.com/avatar/4817e8703ac4379935834d87453faa9d0c94b9dc19d83fcc54c67875eb133e59?d=mp&s=160"},"body":"At Sat, 13 Aug 2005 14:31:59 -0700,\nJunio C Hamano wrote:\n> \n> Yasushi SHOJI writes:\n> \n> > oops. probably my english wasn't clear. my patch fixes\n> > diff_free_filepair().\n> \n> When the command is run on linux-2.6 repository, virtual memory\n> consumption of git-diff-tree command skyrockets to about half a\n> gig, because it maps all files in two adjacent revisions of the\n> entire kernel tree. But it seems to reclaim things reasonably\n> well and goes back down to less than 10m when it starts to\n> process the next commit pair.\n\nit tunes out that, at least for my problem is to populating filespec\ndata in parepare_temp_file() and not freeing it after creating temp\nfile with prep_temp_blob().\n\nparepare_temp_file() and diff_populate_filespec() has a lot in\nsimilarity. so it'd be nice to refactor some. and re-introduce\ndiff_free_filespec_data() and call right after prep_temp_blob() in\nprepare_temp_file().\n\nJunio, did you also mean to clean-up these functions when you said in\nthe thread of \"Re: gitweb - option to disable rename detection\"?\n\nregards,\n--\n yashi\n"},{"id":"7348","messageId":"7v3bpapkmr.fsf@assigned-by-dhcp.cox.net","threadId":"1496","inReplyTo":"8764u6ponn.wl@mail2.atmark-techno.com","subject":"Re: [patch] possible memory leak in diff.c::diff_free_filepair()","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-08-16T04:32:12Z","receivedAt":"2005-08-16T04:32:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yasushi SHOJI writes:\n\n> parepare_temp_file() and diff_populate_filespec() has a lot in\n> similarity. so it'd be nice to refactor some. and re-introduce\n> diff_free_filespec_data() and call right after prep_temp_blob() in\n> prepare_temp_file().\n\nAnother thing that may (or may not) help would be to move that\nprepare_temp_file() and stuff to happen in the forked child\nprocess instead of the parent, so that we do not have to worry\nabout freeing the buffer; it has been some time since I worked\non that part of the code so this may not be an option to make\nthings easier.\n\n> Junio, did you also mean to clean-up these functions when you said in\n> the thread of \"Re: gitweb - option to disable rename detection\"?\n\nNo. I was talking about cleaning up the similar option parsing\ncode, and the call into diffcore_std() which takes more\narguments every time a new option is added to the family.\n"},{"id":"7599","messageId":"87irxzhid3.wl@mail2.atmark-techno.com","threadId":"1496","inReplyTo":"7v3bpapkmr.fsf@assigned-by-dhcp.cox.net","subject":"Re: [patch] possible memory leak in diff.c::diff_free_filepair()","fromName":"Yasushi SHOJI","fromEmail":"yashi@atmark-techno.com","sentAt":"2005-08-21T07:14:16Z","receivedAt":"2005-08-21T07:14:16Z","isPatch":true,"sender":{"key":"yashi@atmark-techno.com","avatar":"https://gravatar.com/avatar/4817e8703ac4379935834d87453faa9d0c94b9dc19d83fcc54c67875eb133e59?d=mp&s=160"},"body":"At Mon, 15 Aug 2005 21:32:12 -0700,\nJunio C Hamano wrote:\n> \n> Yasushi SHOJI writes:\n> \n> > parepare_temp_file() and diff_populate_filespec() has a lot in\n> > similarity. so it'd be nice to refactor some. and re-introduce\n> > diff_free_filespec_data() and call right after prep_temp_blob() in\n> > prepare_temp_file().\n> \n> Another thing that may (or may not) help would be to move that\n> prepare_temp_file() and stuff to happen in the forked child\n> process instead of the parent, so that we do not have to worry\n> about freeing the buffer; it has been some time since I worked\n> on that part of the code so this may not be an option to make\n> things easier.\n\nwell, I couldn't figure out how to do it in child. because the parent\nneed to know temp files in order to delete after child used it.\n\nanyway, here is a patch to fix the problem in the simplest way.\n\nregards,\n--\n yashi\n\n-----------\nMake diff_flush a bit more system friendly.\n\nThis seems to be the simplest way to fix the peak time\nhigh memory usage in diff_flush. Don't wait all iteration\nto free each filepair.\n\nSigned-off-by: Yasushi SHOJI \n\n\n---\n\n diff.c | 3 +--\n 1 files changed, 1 insertions(+), 2 deletions(-)\n\ne0f7f97ddc29a9172c2f6c1221eca936aa80835c\ndiff --git a/diff.c b/diff.c\n--- a/diff.c\n+++ b/diff.c\n@@ -1010,9 +1010,8 @@ void diff_flush(int diff_output_style, i\n \t\t\tdiff_flush_name(p, line_termination);\n \t\t\tbreak;\n \t\t}\n-\t}\n-\tfor (i = 0; i < q->nr; i++)\n \t\tdiff_free_filepair(q->queue[i]);\n+\t}\n \tfree(q->queue);\n \tq->queue = NULL;\n \tq->nr = q->alloc = 0;\n"}]}