[RFD PATCH 0/3] Free all the memory!

[RFD PATCH 0/3] Free all the memory!

[Stefan Beller]

When using automated tools to find memory leaks, it is hard to distinguish
between actual leaks and intentional non-cleanups at the end of the program,
such that the actual leaks hide in the noise.

The end goal of this (unfinished) series is to close all intentional memory
leaks when enabling the -DFREE_ALL_MEMORY switch. This is just
demonstrating how the beginning of such a series could look like.

Thanks,
Stefan


Stefan Beller (3):
  mv: free memory at the end if desired
  pack-redundant: free all memory
  rev-parse: free all memory

 Makefile                 |  7 ++++++-
 builtin/mv.c             |  7 +++++++
 builtin/pack-redundant.c | 17 +++++++++++++++++
 builtin/rev-parse.c      | 12 +++++++++++-
 4 files changed, 41 insertions(+), 2 deletions(-)

-- 
2.8.2.401.g9c0faef

[PATCH 1/3] mv: free memory at the end if desired

[Stefan Beller]

From: Stefan Beller <stefanbeller@gmail.com>

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Makefile     | 7 ++++++-
 builtin/mv.c | 7 +++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 3f03366..e533257 100644
--- a/Makefile
+++ b/Makefile
@@ -389,7 +389,8 @@ CFLAGS += -Werror \
 	-Wpointer-arith \
 	-Wstrict-prototypes \
 	-Wunused \
-	-Wvla
+	-Wvla \
+	-DFREE_ALL_MEMORY
 endif
 
 # Create as necessary, replace existing, make ranlib unneeded.
@@ -1479,6 +1480,10 @@ ifdef HAVE_GETDELIM
 	BASIC_CFLAGS += -DHAVE_GETDELIM
 endif
 
+ifdef FREE_ALL_MEMORY
+	BASIC_CFLAGS += -DFREE_ALL_MEMORY
+endif
+
 ifeq ($(TCLTK_PATH),)
 NO_TCLTK = NoThanks
 endif
diff --git a/builtin/mv.c b/builtin/mv.c
index a201426..c48dbb9 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -282,5 +282,12 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	    write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
 		die(_("Unable to write new index file"));
 
+#ifdef FREE_ALL_MEMORY
+	free(destination);
+	free(source);
+	free(submodule_gitfile);
+	free(modes);
+#endif
+
 	return 0;
 }
-- 
2.8.2.401.g9c0faef

Re: [PATCH 1/3] mv: free memory at the end if desired

[Torsten Bögershausen]

 >[PATCH 1/3] mv: free memory at the end if desired
s/desired/DEVELOPER/

[PATCH 2/3] pack-redundant: free all memory

[Stefan Beller]

From: Stefan Beller <stefanbeller@gmail.com>

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/pack-redundant.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index 72c8158..c75c5c9 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -223,6 +223,18 @@ static inline size_t pack_list_size(struct pack_list *pl)
 	return ret;
 }
 
+static inline void pack_list_free(struct pack_list *pl)
+{
+	struct pack_list *cur_pl;
+	while (pl) {
+		llist_free(pl->unique_objects);
+		llist_free(pl->all_objects);
+		cur_pl = pl;
+		pl = pl->next;
+		free(cur_pl);
+	}
+}
+
 static struct pack_list * pack_list_difference(const struct pack_list *A,
 					       const struct pack_list *B)
 {
@@ -691,5 +703,10 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix)
 		fprintf(stderr, "%luMB of redundant packs in total.\n",
 			(unsigned long)pack_set_bytecount(red)/(1024*1024));
 
+#ifdef FREE_ALL_MEMORY
+	pack_list_free(red);
+	llist_free(ignore);
+#endif
+
 	return 0;
 }
-- 
2.8.2.401.g9c0faef

Re: [PATCH 2/3] pack-redundant: free all memory

[Eric Sunshine]

On Mon, May 16, 2016 at 11:22 PM, Stefan Beller <sbeller@google.com> wrote:
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
> @@ -223,6 +223,18 @@ static inline size_t pack_list_size(struct pack_list *pl)
> +static inline void pack_list_free(struct pack_list *pl)

s/inline//

> +{
> +       struct pack_list *cur_pl;

You can declare this within the scope of the while-loop.

> +       while (pl) {
> +               llist_free(pl->unique_objects);
> +               llist_free(pl->all_objects);
> +               cur_pl = pl;
> +               pl = pl->next;
> +               free(cur_pl);
> +       }
> +}

[PATCH 3/3] rev-parse: free all memory

[Stefan Beller]

From: Stefan Beller <stefanbeller@gmail.com>

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/rev-parse.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index c961b74..3296d22 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -371,7 +371,7 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)
 	static const char * const flag_chars = "*=?!";
 
 	struct strbuf sb = STRBUF_INIT, parsed = STRBUF_INIT;
-	const char **usage = NULL;
+	const char **usage = NULL, **curr_usage;
 	struct option *opts = NULL;
 	int onb = 0, osz = 0, unb = 0, usz = 0;
 
@@ -472,6 +472,16 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)
 	strbuf_addf(&parsed, " --");
 	sq_quote_argv(&parsed, argv, 0);
 	puts(parsed.buf);
+
+#ifdef FREE_ALL_MEMORY
+	curr_usage = usage;
+	while (curr_usage) {
+		free(*curr_usage);
+		curr_usage++;
+	}
+	free(usage);
+#endif
+
 	return 0;
 }
 
-- 
2.8.2.401.g9c0faef

Re: [RFD PATCH 0/3] Free all the memory!

[Eric Sunshine]

On Mon, May 16, 2016 at 11:22 PM, Stefan Beller <sbeller@google.com> wrote:
> When using automated tools to find memory leaks, it is hard to distinguish
> between actual leaks and intentional non-cleanups at the end of the program,
> such that the actual leaks hide in the noise.
>
> The end goal of this (unfinished) series is to close all intentional memory
> leaks when enabling the -DFREE_ALL_MEMORY switch. This is just
> demonstrating how the beginning of such a series could look like.

Considering the signal-to-noise ratio mentioned above, the goal seems
reasonable, but why pollute the code with #ifdef's all over the place
by making the cleanup conditional? If you're going though the effort
of plugging all these leaks, it probably makes sense to do them
unconditionally.

Re: [RFD PATCH 0/3] Free all the memory!

[Eric Wong]

Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Mon, May 16, 2016 at 11:22 PM, Stefan Beller <sbeller@google.com> wrote:
> > When using automated tools to find memory leaks, it is hard to distinguish
> > between actual leaks and intentional non-cleanups at the end of the program,
> > such that the actual leaks hide in the noise.
> 
> Considering the signal-to-noise ratio mentioned above, the goal seems
> reasonable, but why pollute the code with #ifdef's all over the place
> by making the cleanup conditional? If you're going though the effort
> of plugging all these leaks, it probably makes sense to do them
> unconditionally.

I haven't checked for git, but I suspect we get speedups by not
calling free().  I've never needed to profile git, but free() at
exit has been a measurable bottleneck in other projects I've
worked on.  Often, free() was more expensive than *alloc().

In any case, I like constant conditionals in C or inline wrappers
macros over CPP #ifdefs littered inside functions:

/* in git-compat-util.h */
#ifdef FREE_ALL_MEMORY
static inline void optional_free(void *ptr) {}
#else
#  define FREE_ALL_MEMORY (0)
#  define optional_free(ptr) free(ptr)
#endif

/* inside any function: */
	if (FREE_ALL_MEMORY)
		big_function_which_calls_multiple_frees();


Also Valgrind has suppression files, so code modifications may
not be necessary at all.

Re: [RFD PATCH 0/3] Free all the memory!

[Stefan Beller]

On Mon, May 16, 2016 at 8:41 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Mon, May 16, 2016 at 11:22 PM, Stefan Beller <sbeller@google.com> wrote:
>> When using automated tools to find memory leaks, it is hard to distinguish
>> between actual leaks and intentional non-cleanups at the end of the program,
>> such that the actual leaks hide in the noise.
>>
>> The end goal of this (unfinished) series is to close all intentional memory
>> leaks when enabling the -DFREE_ALL_MEMORY switch. This is just
>> demonstrating how the beginning of such a series could look like.
>
> Considering the signal-to-noise ratio mentioned above, the goal seems
> reasonable, but why pollute the code with #ifdef's all over the place
> by making the cleanup conditional? If you're going though the effort
> of plugging all these leaks, it probably makes sense to do them
> unconditionally.

I tried that once upon a time. The resentment from the list was:

    We're exiting soon anyway (e.g. some cmd_foo function). Letting the
    operating system clean up after us is faster than when we do it, so don't
    do it.

However it would be nice to have a distinction between "I know we're leaking
this memory, but we don't care for $REASONS" and a genuine leak that
should concern the developers.

So as a developer I wish we would close all leaks that are non-concerning.
As a user I don't care about those leaks.

David writes:
> AFAIK, nothing in the "definitely lost" category is fixed by your rev-parse patch.
>
> 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.

As said above I'd want them to be fixed for me as a developer for
better automated tooling and detection. (The alternative to fix the automated
tooling is a no-no for me ;)

Matthieu Moy writes:
> One potential issue with this is: if all developers and the testsuite
> use this -DFREE_ALL_MEMORY, the non-free-all-memory setup will not be
> well tested, and still this is the one used by real people. For example,
> if there's a really annoying memory leak hidden by FREE_ALL_MEMORY, we
> may not notice it.
>
> Perhaps it'd be better to activate FREE_ALL_MEMORY only when tools like
> valgrind or so is used.

That's a really good point. I'l keep it in mind for a reroll.


Eric Wong writes:
> I haven't checked for git, but I suspect we get speedups by not
> calling free().  I've never needed to profile git, but free() at
> exit has been a measurable bottleneck in other projects I've
> worked on.  Often, free() was more expensive than *alloc().

Thanks for reiterating that original response I got back then :)

>
> In any case, I like constant conditionals in C or inline wrappers
> macros over CPP #ifdefs littered inside functions:
>
> /* in git-compat-util.h */
> #ifdef FREE_ALL_MEMORY
> static inline void optional_free(void *ptr) {}
> #else
> #  define FREE_ALL_MEMORY (0)
> #  define optional_free(ptr) free(ptr)
> #endif
>
> /* inside any function: */
>        if (FREE_ALL_MEMORY)
>                big_function_which_calls_multiple_frees();
>

Shouldn't that be "#ifndef" instead? I really like the
"optional_free", I'll keep it in mind!

>
> Also Valgrind has suppression files, so code modifications may
> not be necessary at all.

But there are more tools than just valgrind. (Although valgrind is really good!)

Re: [RFD PATCH 0/3] Free all the memory!

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> So as a developer I wish we would close all leaks that are non-concerning.

Valgrind suppression (and if you use other tools, suppression for
them) sounds like the way to go, I would think.

Reducing false positive is a good goal; it helps to highlight the
real problems.  But we need to find a way to do so without hurting
the use by the end users by making them pay the unnecessary cost to
free() at the end and by cluttering the code with #ifdefs that makes
it easier to introduce subtle bugs.

> David writes:
>> AFAIK, nothing in the "definitely lost" category is fixed by your rev-parse patch.
>>
>> 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.
>
> As said above I'd want them to be fixed for me as a developer for
> better automated tooling and detection. (The alternative to fix the automated
> tooling is a no-no for me ;)

Does the word "no-no" mean what you seem to think it means?  It
sounds as if you are saying "fixing tools to reduce false positives
is fundamentally wrong, I refuse to go in that direction".

Re: [RFD PATCH 0/3] Free all the memory!

[Stefan Beller]

On Tue, May 17, 2016 at 11:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> So as a developer I wish we would close all leaks that are non-concerning.
>
> Valgrind suppression (and if you use other tools, suppression for
> them) sounds like the way to go, I would think.
>
> Reducing false positive is a good goal; it helps to highlight the
> real problems.  But we need to find a way to do so without hurting
> the use by the end users by making them pay the unnecessary cost to
> free() at the end and by cluttering the code with #ifdefs that makes
> it easier to introduce subtle bugs.

That's why I think the `optional_free` is a good thing as it doesn't clutter
the code?

>
>> David writes:
>>> AFAIK, nothing in the "definitely lost" category is fixed by your rev-parse patch.
>>>
>>> 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.
>>
>> As said above I'd want them to be fixed for me as a developer for
>> better automated tooling and detection. (The alternative to fix the automated
>> tooling is a no-no for me ;)
>
> Does the word "no-no" mean what you seem to think it means?  It
> sounds as if you are saying "fixing tools to reduce false positives
> is fundamentally wrong, I refuse to go in that direction".
>

I just mean, that I have not enough time to do that, so I won't.
I know however how to send patches to this list.

Re: [RFD PATCH 0/3] Free all the memory!

[Eric Wong]

Stefan Beller <sbeller@google.com> wrote:
> Eric Wong writes:
> > I haven't checked for git, but I suspect we get speedups by not
> > calling free().  I've never needed to profile git, but free() at
> > exit has been a measurable bottleneck in other projects I've
> > worked on.  Often, free() was more expensive than *alloc().
> 
> Thanks for reiterating that original response I got back then :)

Heh, I suspected I missed some of the original discussion.

> > #ifdef FREE_ALL_MEMORY
> > static inline void optional_free(void *ptr) {}
> > #else
> > #  define FREE_ALL_MEMORY (0)
> > #  define optional_free(ptr) free(ptr)
> > #endif
> 
> Shouldn't that be "#ifndef" instead? I really like the
> "optional_free", I'll keep it in mind!

Yes, I got my conditionals backwards.  That's what I get
for being awake way past my bedtime :x

We may also want to find a way to annotate *alloc calls as
"lifetime" so they can pair with optional_free.
Maybe sparse can help with that?

Re: [RFD PATCH 0/3] Free all the memory!

[Ævar Arnfjörð Bjarmason]

On Tue, May 17, 2016 at 7:58 PM, Stefan Beller <sbeller@google.com> wrote:
> On Mon, May 16, 2016 at 8:41 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Mon, May 16, 2016 at 11:22 PM, Stefan Beller <sbeller@google.com> wrote:
>>> When using automated tools to find memory leaks, it is hard to distinguish
>>> between actual leaks and intentional non-cleanups at the end of the program,
>>> such that the actual leaks hide in the noise.
>>>
>>> The end goal of this (unfinished) series is to close all intentional memory
>>> leaks when enabling the -DFREE_ALL_MEMORY switch. This is just
>>> demonstrating how the beginning of such a series could look like.
>>
>> Considering the signal-to-noise ratio mentioned above, the goal seems
>> reasonable, but why pollute the code with #ifdef's all over the place
>> by making the cleanup conditional? If you're going though the effort
>> of plugging all these leaks, it probably makes sense to do them
>> unconditionally.
>
> I tried that once upon a time. The resentment from the list was:
>
>     We're exiting soon anyway (e.g. some cmd_foo function). Letting the
>     operating system clean up after us is faster than when we do it, so don't
>     do it.

Not a direct comment on this patch, but has anyone done some detailed
performance testing of this with git? E.g. using a free() that just
no-ops, or one that no-ops unless runtime > x_seconds, or no-ops
unless total_allocated > some_limit.

It might be interesting to play with that as a performance optimization.

Re: [RFD PATCH 0/3] Free all the memory!

[David Turner]

On Mon, 2016-05-16 at 20:22 -0700, Stefan Beller wrote:
> When using automated tools to find memory leaks, it is hard to
> distinguish
> between actual leaks and intentional non-cleanups at the end of the
> program,
> such that the actual leaks hide in the noise.

valgrind on git rev-parse HEAD shows:
==21785==    definitely lost: 153
bytes in 2 blocks
==21785==    indirectly lost: 0 bytes in 0 blocks
==217
85==      possibly lost: 0 bytes in 0 blocks
==21785==    still
reachable: 58,635 bytes in 570 blocks
==21785==         suppressed: 0
bytes in 0 blocks

AFAIK, nothing in the "definitely lost" category is fixed by your rev-parse patch. 

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.

Unless, I guess, you're planning on making a library out of git.  Then it would be worth doing much more cleanup.

Re: [RFD PATCH 0/3] Free all the memory!

[Matthieu Moy]

Stefan Beller <sbeller@google.com> writes:

> The end goal of this (unfinished) series is to close all intentional memory
> leaks when enabling the -DFREE_ALL_MEMORY switch. This is just
> demonstrating how the beginning of such a series could look like.

One potential issue with this is: if all developers and the testsuite
use this -DFREE_ALL_MEMORY, the non-free-all-memory setup will not be
well tested, and still this is the one used by real people. For example,
if there's a really annoying memory leak hidden by FREE_ALL_MEMORY, we
may not notice it.

Perhaps it'd be better to activate FREE_ALL_MEMORY only when tools like
valgrind or so is used.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/