Re: [RFD PATCH 0/3] Free all the memory!
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!)
Stefan Beller
