{"thread":{"id":"42353","subject":"[RFD PATCH 0/3] Free all the memory!","startedAt":"2016-05-17T03:22:44Z","lastAt":"2016-05-18T15:19:58Z","messageCount":15,"participants":["Stefan Beller","Eric Sunshine","Torsten Bögershausen","David Turner","Matthieu Moy","Eric Wong","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"286762","messageId":"20160517032247.25092-1-sbeller@google.com","threadId":"42353","inReplyTo":null,"subject":"[RFD PATCH 0/3] Free all the memory!","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-17T03:22:44Z","receivedAt":"2016-05-17T03:22:44Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When using automated tools to find memory leaks, it is hard to distinguish\nbetween actual leaks and intentional non-cleanups at the end of the program,\nsuch that the actual leaks hide in the noise.\n\nThe end goal of this (unfinished) series is to close all intentional memory\nleaks when enabling the -DFREE_ALL_MEMORY switch. This is just\ndemonstrating how the beginning of such a series could look like.\n\nThanks,\nStefan\n\n\nStefan Beller (3):\n mv: free memory at the end if desired\n pack-redundant: free all memory\n rev-parse: free all memory\n\n Makefile | 7 ++++++-\n builtin/mv.c | 7 +++++++\n builtin/pack-redundant.c | 17 +++++++++++++++++\n builtin/rev-parse.c | 12 +++++++++++-\n 4 files changed, 41 insertions(+), 2 deletions(-)\n\n-- \n2.8.2.401.g9c0faef\n"},{"id":"286765","messageId":"20160517032247.25092-2-sbeller@google.com","threadId":"42353","inReplyTo":"20160517032247.25092-1-sbeller@google.com","subject":"[PATCH 1/3] mv: free memory at the end if desired","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-17T03:22:45Z","receivedAt":"2016-05-17T03:22:45Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Stefan Beller \n\nSigned-off-by: Stefan Beller \n---\n Makefile | 7 ++++++-\n builtin/mv.c | 7 +++++++\n 2 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex 3f03366..e533257 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -389,7 +389,8 @@ CFLAGS += -Werror \\\n \t-Wpointer-arith \\\n \t-Wstrict-prototypes \\\n \t-Wunused \\\n-\t-Wvla\n+\t-Wvla \\\n+\t-DFREE_ALL_MEMORY\n endif\n \n # Create as necessary, replace existing, make ranlib unneeded.\n@@ -1479,6 +1480,10 @@ ifdef HAVE_GETDELIM\n \tBASIC_CFLAGS += -DHAVE_GETDELIM\n endif\n \n+ifdef FREE_ALL_MEMORY\n+\tBASIC_CFLAGS += -DFREE_ALL_MEMORY\n+endif\n+\n ifeq ($(TCLTK_PATH),)\n NO_TCLTK = NoThanks\n endif\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex a201426..c48dbb9 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -282,5 +282,12 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(_(\"Unable to write new index file\"));\n \n+#ifdef FREE_ALL_MEMORY\n+\tfree(destination);\n+\tfree(source);\n+\tfree(submodule_gitfile);\n+\tfree(modes);\n+#endif\n+\n \treturn 0;\n }\n-- \n2.8.2.401.g9c0faef\n"},{"id":"286763","messageId":"20160517032247.25092-3-sbeller@google.com","threadId":"42353","inReplyTo":"20160517032247.25092-1-sbeller@google.com","subject":"[PATCH 2/3] pack-redundant: free all memory","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-17T03:22:46Z","receivedAt":"2016-05-17T03:22:46Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Stefan Beller \n\nSigned-off-by: Stefan Beller \n---\n builtin/pack-redundant.c | 17 +++++++++++++++++\n 1 file changed, 17 insertions(+)\n\ndiff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c\nindex 72c8158..c75c5c9 100644\n--- a/builtin/pack-redundant.c\n+++ b/builtin/pack-redundant.c\n@@ -223,6 +223,18 @@ static inline size_t pack_list_size(struct pack_list *pl)\n \treturn ret;\n }\n \n+static inline void pack_list_free(struct pack_list *pl)\n+{\n+\tstruct pack_list *cur_pl;\n+\twhile (pl) {\n+\t\tllist_free(pl->unique_objects);\n+\t\tllist_free(pl->all_objects);\n+\t\tcur_pl = pl;\n+\t\tpl = pl->next;\n+\t\tfree(cur_pl);\n+\t}\n+}\n+\n static struct pack_list * pack_list_difference(const struct pack_list *A,\n \t\t\t\t\t const struct pack_list *B)\n {\n@@ -691,5 +703,10 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix)\n \t\tfprintf(stderr, \"%luMB of redundant packs in total.\\n\",\n \t\t\t(unsigned long)pack_set_bytecount(red)/(1024*1024));\n \n+#ifdef FREE_ALL_MEMORY\n+\tpack_list_free(red);\n+\tllist_free(ignore);\n+#endif\n+\n \treturn 0;\n }\n-- \n2.8.2.401.g9c0faef\n"},{"id":"286764","messageId":"20160517032247.25092-4-sbeller@google.com","threadId":"42353","inReplyTo":"20160517032247.25092-1-sbeller@google.com","subject":"[PATCH 3/3] rev-parse: free all memory","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-17T03:22:47Z","receivedAt":"2016-05-17T03:22:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Stefan Beller \n\nSigned-off-by: Stefan Beller \n---\n builtin/rev-parse.c | 12 +++++++++++-\n 1 file changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex c961b74..3296d22 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -371,7 +371,7 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)\n \tstatic const char * const flag_chars = \"*=?!\";\n \n \tstruct strbuf sb = STRBUF_INIT, parsed = STRBUF_INIT;\n-\tconst char **usage = NULL;\n+\tconst char **usage = NULL, **curr_usage;\n \tstruct option *opts = NULL;\n \tint onb = 0, osz = 0, unb = 0, usz = 0;\n \n@@ -472,6 +472,16 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)\n \tstrbuf_addf(&parsed, \" --\");\n \tsq_quote_argv(&parsed, argv, 0);\n \tputs(parsed.buf);\n+\n+#ifdef FREE_ALL_MEMORY\n+\tcurr_usage = usage;\n+\twhile (curr_usage) {\n+\t\tfree(*curr_usage);\n+\t\tcurr_usage++;\n+\t}\n+\tfree(usage);\n+#endif\n+\n \treturn 0;\n }\n \n-- \n2.8.2.401.g9c0faef\n"},{"id":"286769","messageId":"CAPig+cT9UKEcNXvsQsFDrQycKwMJR5bCter76UL-zpkncniPBw@mail.gmail.com","threadId":"42353","inReplyTo":"20160517032247.25092-1-sbeller@google.com","subject":"Re: [RFD PATCH 0/3] Free all the memory!","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-17T03:41:18Z","receivedAt":"2016-05-17T03:41:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, May 16, 2016 at 11:22 PM, Stefan Beller wrote:\n> When using automated tools to find memory leaks, it is hard to distinguish\n> between actual leaks and intentional non-cleanups at the end of the program,\n> such that the actual leaks hide in the noise.\n>\n> The end goal of this (unfinished) series is to close all intentional memory\n> leaks when enabling the -DFREE_ALL_MEMORY switch. This is just\n> demonstrating how the beginning of such a series could look like.\n\nConsidering the signal-to-noise ratio mentioned above, the goal seems\nreasonable, but why pollute the code with #ifdef's all over the place\nby making the cleanup conditional? If you're going though the effort\nof plugging all these leaks, it probably makes sense to do them\nunconditionally.\n"},{"id":"286770","messageId":"CAPig+cTAZfRp=bD=CNgcfYcpkRfGuxdCbPCimhpM1Q_atv8G4g@mail.gmail.com","threadId":"42353","inReplyTo":"20160517032247.25092-3-sbeller@google.com","subject":"Re: [PATCH 2/3] pack-redundant: free all memory","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-17T03:42:46Z","receivedAt":"2016-05-17T03:42:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, May 16, 2016 at 11:22 PM, Stefan Beller wrote:\n> Signed-off-by: Stefan Beller \n> ---\n> diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c\n> @@ -223,6 +223,18 @@ static inline size_t pack_list_size(struct pack_list *pl)\n> +static inline void pack_list_free(struct pack_list *pl)\n\ns/inline//\n\n> +{\n> + struct pack_list *cur_pl;\n\nYou can declare this within the scope of the while-loop.\n\n> + while (pl) {\n> + llist_free(pl->unique_objects);\n> + llist_free(pl->all_objects);\n> + cur_pl = pl;\n> + pl = pl->next;\n> + free(cur_pl);\n> + }\n> +}\n"},{"id":"286774","messageId":"573A9AB5.3010109@web.de","threadId":"42353","inReplyTo":"20160517032247.25092-2-sbeller@google.com","subject":"Re: [PATCH 1/3] mv: free memory at the end if desired","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-05-17T04:14:45Z","receivedAt":"2016-05-17T04:14:45Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":" >[PATCH 1/3] mv: free memory at the end if desired\ns/desired/DEVELOPER/\n"},{"id":"286779","messageId":"1463464006.24478.71.camel@twopensource.com","threadId":"42353","inReplyTo":"20160517032247.25092-1-sbeller@google.com","subject":"Re: [RFD PATCH 0/3] Free all the memory!","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2016-05-17T05:46:46Z","receivedAt":"2016-05-17T05:46:46Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Mon, 2016-05-16 at 20:22 -0700, Stefan Beller wrote:\n> When using automated tools to find memory leaks, it is hard to\n> distinguish\n> between actual leaks and intentional non-cleanups at the end of the\n> program,\n> such that the actual leaks hide in the noise.\n\nvalgrind on git rev-parse HEAD shows:\n==21785== definitely lost: 153\nbytes in 2 blocks\n==21785== indirectly lost: 0 bytes in 0 blocks\n==217\n85== possibly lost: 0 bytes in 0 blocks\n==21785== still\nreachable: 58,635 bytes in 570 blocks\n==21785== suppressed: 0\nbytes in 0 blocks\n\nAFAIK, nothing in the \"definitely lost\" category is fixed by your rev-parse patch. \n\nI don't think we care that much about \"still reachable\" memory -- I only care about lost memory. I could imagine, I guess, something that happens to save a pointer to a bunch of memory that should be freed, but I don't think that's the common case.\n\nUnless, I guess, you're planning on making a library out of git. Then it would be worth doing much more cleanup.\n"},{"id":"286783","messageId":"vpqvb2drr5p.fsf@anie.imag.fr","threadId":"42353","inReplyTo":"20160517032247.25092-1-sbeller@google.com","subject":"Re: [RFD PATCH 0/3] Free all the memory!","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-05-17T09:05:06Z","receivedAt":"2016-05-17T09:05:06Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Stefan Beller writes:\n\n> The end goal of this (unfinished) series is to close all intentional memory\n> leaks when enabling the -DFREE_ALL_MEMORY switch. This is just\n> demonstrating how the beginning of such a series could look like.\n\nOne potential issue with this is: if all developers and the testsuite\nuse this -DFREE_ALL_MEMORY, the non-free-all-memory setup will not be\nwell tested, and still this is the one used by real people. For example,\nif there's a really annoying memory leak hidden by FREE_ALL_MEMORY, we\nmay not notice it.\n\nPerhaps it'd be better to activate FREE_ALL_MEMORY only when tools like\nvalgrind or so is used.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"286786","messageId":"20160517120815.GA15546@dcvr.yhbt.net","threadId":"42353","inReplyTo":"CAPig+cT9UKEcNXvsQsFDrQycKwMJR5bCter76UL-zpkncniPBw@mail.gmail.com","subject":"Re: [RFD PATCH 0/3] Free all the memory!","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-05-17T12:08:15Z","receivedAt":"2016-05-17T12:08:15Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Eric Sunshine wrote:\n> On Mon, May 16, 2016 at 11:22 PM, Stefan Beller wrote:\n> > When using automated tools to find memory leaks, it is hard to distinguish\n> > between actual leaks and intentional non-cleanups at the end of the program,\n> > such that the actual leaks hide in the noise.\n> \n> Considering the signal-to-noise ratio mentioned above, the goal seems\n> reasonable, but why pollute the code with #ifdef's all over the place\n> by making the cleanup conditional? If you're going though the effort\n> of plugging all these leaks, it probably makes sense to do them\n> unconditionally.\n\nI haven't checked for git, but I suspect we get speedups by not\ncalling free(). I've never needed to profile git, but free() at\nexit has been a measurable bottleneck in other projects I've\nworked on. Often, free() was more expensive than *alloc().\n\nIn any case, I like constant conditionals in C or inline wrappers\nmacros over CPP #ifdefs littered inside functions:\n\n/* in git-compat-util.h */\n#ifdef FREE_ALL_MEMORY\nstatic inline void optional_free(void *ptr) {}\n#else\n# define FREE_ALL_MEMORY (0)\n# define optional_free(ptr) free(ptr)\n#endif\n\n/* inside any function: */\n\tif (FREE_ALL_MEMORY)\n\t\tbig_function_which_calls_multiple_frees();\n\n\nAlso Valgrind has suppression files, so code modifications may\nnot be necessary at all.\n"},{"id":"286808","messageId":"CAGZ79kaJoAxqtsTuErQSgJiVeD_vdZ1MQXKr-LTtyro-FbscTQ@mail.gmail.com","threadId":"42353","inReplyTo":"CAPig+cT9UKEcNXvsQsFDrQycKwMJR5bCter76UL-zpkncniPBw@mail.gmail.com","subject":"Re: [RFD PATCH 0/3] Free all the memory!","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-17T17:58:30Z","receivedAt":"2016-05-17T17:58:30Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, May 16, 2016 at 8:41 PM, Eric Sunshine wrote:\n> On Mon, May 16, 2016 at 11:22 PM, Stefan Beller wrote:\n>> When using automated tools to find memory leaks, it is hard to distinguish\n>> between actual leaks and intentional non-cleanups at the end of the program,\n>> such that the actual leaks hide in the noise.\n>>\n>> The end goal of this (unfinished) series is to close all intentional memory\n>> leaks when enabling the -DFREE_ALL_MEMORY switch. This is just\n>> demonstrating how the beginning of such a series could look like.\n>\n> Considering the signal-to-noise ratio mentioned above, the goal seems\n> reasonable, but why pollute the code with #ifdef's all over the place\n> by making the cleanup conditional? If you're going though the effort\n> of plugging all these leaks, it probably makes sense to do them\n> unconditionally.\n\nI tried that once upon a time. The resentment from the list was:\n\n We're exiting soon anyway (e.g. some cmd_foo function). Letting the\n operating system clean up after us is faster than when we do it, so don't\n do it.\n\nHowever it would be nice to have a distinction between \"I know we're leaking\nthis memory, but we don't care for $REASONS\" and a genuine leak that\nshould concern the developers.\n\nSo as a developer I wish we would close all leaks that are non-concerning.\nAs a user I don't care about those leaks.\n\nDavid writes:\n> AFAIK, nothing in the \"definitely lost\" category is fixed by your rev-parse patch.\n>\n> I don't think we care that much about \"still reachable\" memory -- I only care about lost memory. I could imagine, I guess, something that happens to save a pointer to a bunch of memory that should be freed, but I don't think that's the common case.\n\nAs said above I'd want them to be fixed for me as a developer for\nbetter automated tooling and detection. (The alternative to fix the automated\ntooling is a no-no for me ;)\n\nMatthieu Moy writes:\n> One potential issue with this is: if all developers and the testsuite\n> use this -DFREE_ALL_MEMORY, the non-free-all-memory setup will not be\n> well tested, and still this is the one used by real people. For example,\n> if there's a really annoying memory leak hidden by FREE_ALL_MEMORY, we\n> may not notice it.\n>\n> Perhaps it'd be better to activate FREE_ALL_MEMORY only when tools like\n> valgrind or so is used.\n\nThat's a really good point. I'l keep it in mind for a reroll.\n\n\nEric Wong writes:\n> I haven't checked for git, but I suspect we get speedups by not\n> calling free(). I've never needed to profile git, but free() at\n> exit has been a measurable bottleneck in other projects I've\n> worked on. Often, free() was more expensive than *alloc().\n\nThanks for reiterating that original response I got back then :)\n\n>\n> In any case, I like constant conditionals in C or inline wrappers\n> macros over CPP #ifdefs littered inside functions:\n>\n> /* in git-compat-util.h */\n> #ifdef FREE_ALL_MEMORY\n> static inline void optional_free(void *ptr) {}\n> #else\n> # define FREE_ALL_MEMORY (0)\n> # define optional_free(ptr) free(ptr)\n> #endif\n>\n> /* inside any function: */\n> if (FREE_ALL_MEMORY)\n> big_function_which_calls_multiple_frees();\n>\n\nShouldn't that be \"#ifndef\" instead? I really like the\n\"optional_free\", I'll keep it in mind!\n\n>\n> Also Valgrind has suppression files, so code modifications may\n> not be necessary at all.\n\nBut there are more tools than just valgrind. (Although valgrind is really good!)\n"},{"id":"286812","messageId":"xmqqinycwnx2.fsf@gitster.mtv.corp.google.com","threadId":"42353","inReplyTo":"CAGZ79kaJoAxqtsTuErQSgJiVeD_vdZ1MQXKr-LTtyro-FbscTQ@mail.gmail.com","subject":"Re: [RFD PATCH 0/3] Free all the memory!","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-17T18:16:09Z","receivedAt":"2016-05-17T18:16:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> So as a developer I wish we would close all leaks that are non-concerning.\n\nValgrind suppression (and if you use other tools, suppression for\nthem) sounds like the way to go, I would think.\n\nReducing false positive is a good goal; it helps to highlight the\nreal problems. But we need to find a way to do so without hurting\nthe use by the end users by making them pay the unnecessary cost to\nfree() at the end and by cluttering the code with #ifdefs that makes\nit easier to introduce subtle bugs.\n\n> David writes:\n>> AFAIK, nothing in the \"definitely lost\" category is fixed by your rev-parse patch.\n>>\n>> I don't think we care that much about \"still reachable\" memory -- I only care about lost memory. I could imagine, I guess, something that happens to save a pointer to a bunch of memory that should be freed, but I don't think that's the common case.\n>\n> As said above I'd want them to be fixed for me as a developer for\n> better automated tooling and detection. (The alternative to fix the automated\n> tooling is a no-no for me ;)\n\nDoes the word \"no-no\" mean what you seem to think it means? It\nsounds as if you are saying \"fixing tools to reduce false positives\nis fundamentally wrong, I refuse to go in that direction\".\n"},{"id":"286813","messageId":"CAGZ79kaawUGOtxcFPEdx+tSn60Zzv=jLAMU-BnS6GGp_1wBWWg@mail.gmail.com","threadId":"42353","inReplyTo":"xmqqinycwnx2.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFD PATCH 0/3] Free all the memory!","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-17T18:20:45Z","receivedAt":"2016-05-17T18:20:45Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, May 17, 2016 at 11:16 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> So as a developer I wish we would close all leaks that are non-concerning.\n>\n> Valgrind suppression (and if you use other tools, suppression for\n> them) sounds like the way to go, I would think.\n>\n> Reducing false positive is a good goal; it helps to highlight the\n> real problems. But we need to find a way to do so without hurting\n> the use by the end users by making them pay the unnecessary cost to\n> free() at the end and by cluttering the code with #ifdefs that makes\n> it easier to introduce subtle bugs.\n\nThat's why I think the `optional_free` is a good thing as it doesn't clutter\nthe code?\n\n>\n>> David writes:\n>>> AFAIK, nothing in the \"definitely lost\" category is fixed by your rev-parse patch.\n>>>\n>>> I don't think we care that much about \"still reachable\" memory -- I only care about lost memory. I could imagine, I guess, something that happens to save a pointer to a bunch of memory that should be freed, but I don't think that's the common case.\n>>\n>> As said above I'd want them to be fixed for me as a developer for\n>> better automated tooling and detection. (The alternative to fix the automated\n>> tooling is a no-no for me ;)\n>\n> Does the word \"no-no\" mean what you seem to think it means? It\n> sounds as if you are saying \"fixing tools to reduce false positives\n> is fundamentally wrong, I refuse to go in that direction\".\n>\n\nI just mean, that I have not enough time to do that, so I won't.\nI know however how to send patches to this list.\n"},{"id":"286859","messageId":"20160518072329.GA1723@dcvr.yhbt.net","threadId":"42353","inReplyTo":"CAGZ79kaJoAxqtsTuErQSgJiVeD_vdZ1MQXKr-LTtyro-FbscTQ@mail.gmail.com","subject":"Re: [RFD PATCH 0/3] Free all the memory!","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-05-18T07:23:29Z","receivedAt":"2016-05-18T07:23:29Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Stefan Beller wrote:\n> Eric Wong writes:\n> > I haven't checked for git, but I suspect we get speedups by not\n> > calling free(). I've never needed to profile git, but free() at\n> > exit has been a measurable bottleneck in other projects I've\n> > worked on. Often, free() was more expensive than *alloc().\n> \n> Thanks for reiterating that original response I got back then :)\n\nHeh, I suspected I missed some of the original discussion.\n\n> > #ifdef FREE_ALL_MEMORY\n> > static inline void optional_free(void *ptr) {}\n> > #else\n> > # define FREE_ALL_MEMORY (0)\n> > # define optional_free(ptr) free(ptr)\n> > #endif\n> \n> Shouldn't that be \"#ifndef\" instead? I really like the\n> \"optional_free\", I'll keep it in mind!\n\nYes, I got my conditionals backwards. That's what I get\nfor being awake way past my bedtime :x\n\nWe may also want to find a way to annotate *alloc calls as\n\"lifetime\" so they can pair with optional_free.\nMaybe sparse can help with that?\n"},{"id":"286865","messageId":"CACBZZX6t5RsTe0GCPifA9cq7wbChY-ekVXZOYSqPTuOHp_D4Vw@mail.gmail.com","threadId":"42353","inReplyTo":"CAGZ79kaJoAxqtsTuErQSgJiVeD_vdZ1MQXKr-LTtyro-FbscTQ@mail.gmail.com","subject":"Re: [RFD PATCH 0/3] Free all the memory!","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2016-05-18T15:19:58Z","receivedAt":"2016-05-18T15:19:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Tue, May 17, 2016 at 7:58 PM, Stefan Beller wrote:\n> On Mon, May 16, 2016 at 8:41 PM, Eric Sunshine wrote:\n>> On Mon, May 16, 2016 at 11:22 PM, Stefan Beller wrote:\n>>> When using automated tools to find memory leaks, it is hard to distinguish\n>>> between actual leaks and intentional non-cleanups at the end of the program,\n>>> such that the actual leaks hide in the noise.\n>>>\n>>> The end goal of this (unfinished) series is to close all intentional memory\n>>> leaks when enabling the -DFREE_ALL_MEMORY switch. This is just\n>>> demonstrating how the beginning of such a series could look like.\n>>\n>> Considering the signal-to-noise ratio mentioned above, the goal seems\n>> reasonable, but why pollute the code with #ifdef's all over the place\n>> by making the cleanup conditional? If you're going though the effort\n>> of plugging all these leaks, it probably makes sense to do them\n>> unconditionally.\n>\n> I tried that once upon a time. The resentment from the list was:\n>\n> We're exiting soon anyway (e.g. some cmd_foo function). Letting the\n> operating system clean up after us is faster than when we do it, so don't\n> do it.\n\nNot a direct comment on this patch, but has anyone done some detailed\nperformance testing of this with git? E.g. using a free() that just\nno-ops, or one that no-ops unless runtime > x_seconds, or no-ops\nunless total_allocated > some_limit.\n\nIt might be interesting to play with that as a performance optimization.\n"}]}