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

From: Stefan Beller <sbeller@google.com>
Date: May 17, 2016, 17:58 UTC
Message-ID: <CAGZ79kaJoAxqtsTuErQSgJiVeD_vdZ1MQXKr-LTtyro-FbscTQ@mail.gmail.com>
In-Reply-To: <CAPig+cT9UKEcNXvsQsFDrQycKwMJR5bCter76UL-zpkncniPBw@mail.gmail.com>

On Mon, May 16, 2016 at 8:41 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 14 quoted lines

> 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:

Show 8 quoted lines

> 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 :)

Show 16 quoted lines

>
> 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!)